Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

cmd/link: inline and simplify dosymtype #20205

Closed
josharian opened this issue May 2, 2017 · 7 comments
Closed

cmd/link: inline and simplify dosymtype #20205

josharian opened this issue May 2, 2017 · 7 comments
Assignees
Milestone

Comments

@josharian josharian added this to the Go1.10 milestone May 2, 2017
@rsc rsc modified the milestones: Go1.10, Go1.11 Nov 22, 2017
@ianlancetaylor ianlancetaylor modified the milestones: Go1.11, Unplanned Jun 23, 2018
@quasilyte
Copy link
Contributor

@quasilyte quasilyte commented Apr 12, 2019

I'll try encouraging someone to tackle this issue tomorrow on the go contributors workshop.
That function is still there and has only 1 usage.

@gopherbot
Copy link

@gopherbot gopherbot commented Apr 13, 2019

Change https://golang.org/cl/171733 mentions this issue: cmd/link/internal/ld: inline dosymtab

gopherbot pushed a commit that referenced this issue Apr 16, 2019
Updates #20205

Change-Id: I44a7ee46a1cdc7fe6fd36c4db4c0dd87a19f7f5d
Reviewed-on: https://go-review.googlesource.com/c/go/+/171733
Reviewed-by: Josh Bleecher Snyder <josharian@gmail.com>
Run-TryBot: Josh Bleecher Snyder <josharian@gmail.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
@quasilyte
Copy link
Contributor

@quasilyte quasilyte commented Dec 3, 2019

I believe this issue should be closed since mentioned function is inlined and removed. :)

@josharian
Copy link
Contributor Author

@josharian josharian commented Dec 3, 2019

@mwhudson's comment referenced in the OP was:

Seems to me that this function can now be deleted and replaced with

switch Buildmode {
	case BuildmodeCArchive, BuildmodeCShared:
		s := ctxt.Syms.ROLookup(*flagEntrySymbol)
		if s != nil {
			addinitarrdata(ctxt, s)
		}
}

CL 171733 inlined the code but did not do the simplification suggested. This issue was left open to do that simplification, if possible. I don't know enough about the linker to say whether that simplification is correct.

cc @cherrymui @thanm @jeremyfaller

@thanm
Copy link
Member

@thanm thanm commented Dec 4, 2019

These sorts of loops over ctxt.Syms.Allsym hunting for a specific symbol by name are one of the many things that we're trying to eliminate in the new linker. I'll send a CL on the dev.link branch.

@thanm thanm self-assigned this Dec 4, 2019
@thanm thanm modified the milestones: Unplanned, Go1.15 Dec 4, 2019
@gopherbot
Copy link

@gopherbot gopherbot commented Dec 4, 2019

Change https://golang.org/cl/209838 mentions this issue: [dev.link] cmd/link: avoid allsyms loop in initarray setup

gopherbot pushed a commit that referenced this issue Dec 4, 2019
In the linker's symtab() function, avoid looping over the context's
Syms.Allsyms array to locate the entry symbol when setting up the init
array section; do an explicit ABI0 symbol lookup instead. This is a
minor efficiency tweak / code cleanup.

Fixes #20205.

Change-Id: I2ebc17a3cb2cd63e9f5052bc80f1b0ac72c960e3
Reviewed-on: https://go-review.googlesource.com/c/go/+/209838
Run-TryBot: Than McIntosh <thanm@google.com>
Reviewed-by: Jeremy Faller <jeremy@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
@thanm
Copy link
Member

@thanm thanm commented Dec 4, 2019

I've submitted https://golang.org/cl/209838 ... it might be a bit premature (given that the dev.link branch is not yet merged into master) but I'm going to close out the bug, with the expectation that we'll be doing the merge in the 1.15 timeframe.

@thanm thanm closed this Dec 4, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
6 participants
You can’t perform that action at this time.