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/nm: decide on symbol index presentation #38875

Closed
josharian opened this issue May 5, 2020 · 8 comments
Closed

cmd/nm: decide on symbol index presentation #38875

josharian opened this issue May 5, 2020 · 8 comments
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done. okay-after-beta1 Used by release team to mark a release-blocker issue as okay to resolve either before or after beta1 release-blocker
Milestone

Comments

@josharian
Copy link
Contributor

Sample reproduction:

Run go tool nm ../pkg/darwin_amd64/net.a. With tip, I see entries like:

../pkg/darwin_amd64/net.a(_go_.o):        1bedb2 T net.(*TCPConn).readFrom#423

Note the trailing #423. Those are not present with Go 1.14:

../pkg/darwin_amd64/net.a(_go_.o):         a5e67 T net.(*TCPConn).readFrom

cc @cherrymui @jeremyfaller @thanm

@josharian josharian added NeedsFix The path to resolution is known, but the work has not been done. release-blocker labels May 5, 2020
@josharian josharian added this to the Go1.15 milestone May 5, 2020
@cherrymui
Copy link
Member

Yeah, this was done in CL https://go-review.googlesource.com/c/go/+/229246 . The reason is that in the new object files, symbols are referenced by indices instead of by names. So, if you objdump an object file which imports the fmt package and call fmt.Printf, instead of CALL fmt.Printf, you'll see CALL fmt.#33. The object file itself doesn't have any information about what name that symbol actually is. To make it more usable, I think there should be a way to tell what symbol #33 is in the fmt package, and nm fmt.a seems a natural place to me. Therefore I added the index to the symbol name.

Does this format cause any problem for you? Should we add a flag to switch it on/off? I'd also be happy to hear if you have a better suggestion of the format. Thanks!

@cherrymui
Copy link
Member

cc @aclements

@aclements
Copy link
Member

I just added a RELNOTE marker on that CL. It may be worth mentioning this to golang-dev@, too, since I imagine Josh won't be the only person surprised by this!

@josharian
Copy link
Contributor Author

Does this format cause any problem for you? Should we add a flag to switch it on/off? I'd also be happy to hear if you have a better suggestion of the format. Thanks!

It confused some tools I have that parse the output of go tool nm. Easy enough to unmangle, now that I know I can safely ignore the numbers.

Although there would be some value in having a flag (default on or off? not sure.) Or maybe another column in the output for easier parsing (perhaps enabled by a flag)? Those would move the knowledge about how to unmangle from N tools--each possibly using different algorithms and thus getting it wrong in the future in different ways--to one tool.

An alternative, which I would like even better, since I would no longer have to exec nm, would be to expose cmd/internal/objfile, maybe without a Go 1 compatibility guarantee, or living in the Go 1 compat gray zone that other rarely used packages like debug/* occupy. Then we could just add a field for the index instead of having to squish it into the name,

@cherrymui
Copy link
Member

Or maybe another column in the output for easier parsing

I originally had a space between the index and the name. Austin pointed out that adding a space may makes tools harder to parse.

@dmitshur dmitshur added the okay-after-beta1 Used by release team to mark a release-blocker issue as okay to resolve either before or after beta1 label May 14, 2020
@aclements aclements changed the title cmd/nm: many symbol names contain # (regression from 1.14) cmd/nm: decide on symbol index presentation Jun 2, 2020
@gopherbot
Copy link

Change https://golang.org/cl/236167 mentions this issue: Revert "cmd/internal/goobj: add index to symbol name for indexed symbols"

@gopherbot
Copy link

Change https://golang.org/cl/236168 mentions this issue: cmd/internal/goobj2: add referenced symbol names to object file

gopherbot pushed a commit that referenced this issue Jun 3, 2020
…ols"

This reverts CL 229246.

For new indexed object files, in CL 229246 we added symbol index
to tools (nm, objdump) output. This affects external tools that
parse those outputs. And the added index doesn't look very nice.
In this release we take it out. For future releases we may
introduce a flag to tools (nm, objdump) and optionally dump the
symbol index.

For refererenced (not defined) indexed symbols, currently the
symbol is still referenced only by index, not by name. The next
CL will make the object file self-contained, so tools can dump
the symbol names properly (as before).

For #38875.

Change-Id: I07375e85a8e826e15c82fa452d11f0eaf8535a00
Reviewed-on: https://go-review.googlesource.com/c/go/+/236167
Reviewed-by: Than McIntosh <thanm@google.com>
Reviewed-by: Jeremy Faller <jeremy@golang.org>
@aclements
Copy link
Member

To close the loop on this, nm and objdump now print full symbol names even for cross-package references, so we don't need to print the index any more. Cherry did this by adding a table of referenced symbol names to object files so they're self-contained (but only tools use these tables). We may add an option in the future to show these indexes again, since they can be useful for debugging linker issues. (And I'd love a similar option to show ABI information.)

@golang golang locked and limited conversation to collaborators Jun 3, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done. okay-after-beta1 Used by release team to mark a release-blocker issue as okay to resolve either before or after beta1 release-blocker
Projects
None yet
Development

No branches or pull requests

5 participants