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

Jump to bottom

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
Labels
FrozenDueToAge help wanted NeedsFix The path to resolution is known, but the work has not been done.
Milestone
Go1.15

Comments

@josharian
Copy link
Contributor

Per @mwhudson's comment at https://go-review.googlesource.com/c/40864/5/src/cmd/link/internal/ld/data.go#1143.
@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 added help wanted NeedsFix The path to resolution is known, but the work has not been done. labels Jun 23, 2018
@ianlancetaylor ianlancetaylor modified the milestones: Go1.11, Unplanned Jun 23, 2018
@quasilyte
Copy link
Contributor

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

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
@josharian
cmd/link/internal/ld: inline dosymtab
850844e 
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

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

@josharian
Copy link
Contributor Author

@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
Contributor

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

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
@thanm
[dev.link] cmd/link: avoid allsyms loop in initarray setup
29f886e 
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
Contributor

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 as completed Dec 4, 2019
@golang golang locked and limited conversation to collaborators Dec 3, 2020
@rsc rsc unassigned thanm Jun 23, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
FrozenDueToAge help wanted NeedsFix The path to resolution is known, but the work has not been done.
Projects
None yet
Milestone
Go1.15
Development

No branches or pull requests
6 participants
@josharian @rsc @ianlancetaylor @quasilyte @gopherbot @thanm