-
Notifications
You must be signed in to change notification settings - Fork 17.9k
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/compile: user symbols can be in reserved namespace #37762
Comments
The debug/gosym package is intended to be used to read object files generated by the compiler. It's not intended to be used by creating your own instances of Can you show us a complete program that demonstrates this problem when opening an object generated by the compiler? Please also include the sources for the object that you want to open. Thanks. |
Yeah, sure (go.uber.org/zap is a public available library):
And output (just a few lines):
|
Thanks. If user packages can be in the compiler-reserved "go." namespace, that seems like a potential problem. |
Bitten again by our ad hoc symbol mangling rules. :(
Edit: Nevermind, I'm mistaken:
|
Can we change |
Seems doable, I make a quick change to |
Some 2 years ago, there was a somewhat similar issue #25113 reported and fixed by @aarzilli. |
Change https://golang.org/cl/226282 mentions this issue: |
Change https://golang.org/cl/226283 mentions this issue: |
Howdy @jeremyfaller @randall77 @thanm @cuonglm? Kindly pinging you since @cuonglm's CLs were abandoned in lieu of the new linker landing, which made tests fail, however no confirmation on whether this issue has been addressed, and it is marked for Go1.15. Thank you. |
@odeke-em I think it's better to leave it for go1.16 instead. This will make a large change in both compiler and linker. |
Awesome, thank you for the advice @cuonglm! |
Rather than doubling the period, it would probably be more robust if we included a character that can't appear in user package paths at all. There's a whole bunch of reserved characters at https://golang.org/ref/spec#Import_declarations for us to choose. For example, I think we can change |
One thing that affects the choice or the character is that the symbol names may go to the external linker or dynamic linker in some cases, and some linkers don't like certain characters in symbol names. We could let the Go linker rewrite them, but I'd rather not to. |
@cherrymui Do you happen to know/remember any examples of external linkers not liking certain characters? I remembered Go switching from center dot and division to real dot and slash in linker symbols a long time back, and I thought I remembered it was related to linker (or maybe debugger) issues. But looking now, I can't find any evidence of that. It appears to have been motivated by consistency (a6736fa). |
Issue #19529 is one, where the external linker doesn't like the |
@cherrymui Interesting, thanks. As far as I can tell skimming https://www.akkadia.org/drepper/symbol-versioning and looking at If
Looking at other reserved symbols, it seems like these get used for compiling the runtime, so presumably they're all safe: I wonder if it's worth putting special characters in a runtime struct tag or something to help smoke out linker issues. |
You won't see an |
Ah, I see. I thought the |
Thet probably should have used the section in object files too. I don't know why they didn't. |
Ping. Should we bump this to 1.17? Changing these prefixes to use a |
I can make a CL, just need to decide which character to use. As @mdempsky's above comment, |
Bumping to 1.17 because this seems like too much potential risk for relatively little reward. But feel free to send a CL and we can land it as soon as the tree opens. |
Looks like we didn't get to this for 1.17. Rolling forward another cycle. |
Change https://golang.org/cl/317917 mentions this issue: |
@ianlancetaylor Sorry, I actually have the CL in my local, but forgot to send it early in go1.17 cycle. I send it now for reviewing, and mark it for go1.18 (It's a big change, though, so sending it early for reviewing is better.). |
@mdempsky @cherrymui trybot failed in CL 317917 due to this https://sourceware.org/git/?p=binutils-gdb.git;a=blob;f=ld/pe-dll.c;h=e7b82ba6ffadf74dc1b9ee71dc13d48336941e51;hb=HEAD#l972 According to PE file format, the name format of forwarder RVA (symbols in other DLL) have "." in that, otherwise, the linker can't find it. |
This issue is currently labeled as early-in-cycle for Go 1.18. |
As per @cuonglm's comment on golang.org/cl/317917 (https://go-review.googlesource.com/c/go/+/317917/comments/d31ae680_570d8f68), punting to 1.19. |
This issue is currently labeled as early-in-cycle for Go 1.19. |
Based on the comments on the CL, punting this to 1.20. |
Change https://go.dev/cl/422315 mentions this issue: |
Updates #37762 Change-Id: Ib587f472304a04ebd9794666228f81ae6cb5c2a3 Reviewed-on: https://go-review.googlesource.com/c/go/+/422315 Auto-Submit: Cuong Manh Le <cuong.manhle.vn@gmail.com> Run-TryBot: Cuong Manh Le <cuong.manhle.vn@gmail.com> TryBot-Result: Gopher Robot <gobot@golang.org> Reviewed-by: Cherry Mui <cherryyz@google.com> Reviewed-by: Than McIntosh <thanm@google.com>
As it can't appear in user package paths. There is a hack for handling "go:buildid" and "type:*" on windows/386. Previously, windows/386 requires underscore prefix on external symbols, but that's only applied for SHOSTOBJ/SUNDEFEXT or cgo export symbols. "go.buildid" is STEXT, "type.*" is STYPE, thus they are not prefixed with underscore. In external linking mode, the external linker can't resolve them as external symbols. But we are lucky that they have "." in their name, so the external linker see them as Forwarder RVA exports. See: - https://docs.microsoft.com/en-us/windows/win32/debug/pe-format#export-address-table - https://sourceware.org/git/?p=binutils-gdb.git;a=blob;f=ld/pe-dll.c;h=e7b82ba6ffadf74dc1b9ee71dc13d48336941e51;hb=HEAD#l972) This CL changes "." to ":" in symbols name, so theses symbols can not be found by external linker anymore. So a hacky way is adding the underscore prefix for these 2 symbols. I don't have enough knowledge to verify whether adding the underscore for all STEXT/STYPE symbols are fine, even if it could be, that would be done in future CL. Fixes golang#37762 Change-Id: I92eaaf24c0820926a36e0530fdb07b07af1fcc35 Reviewed-on: https://go-review.googlesource.com/c/go/+/317917 Reviewed-by: Than McIntosh <thanm@google.com> Run-TryBot: Cuong Manh Le <cuong.manhle.vn@gmail.com> Reviewed-by: Cherry Mui <cherryyz@google.com> TryBot-Result: Gopher Robot <gobot@golang.org>
Updates golang#37762 Change-Id: Ib587f472304a04ebd9794666228f81ae6cb5c2a3 Reviewed-on: https://go-review.googlesource.com/c/go/+/422315 Auto-Submit: Cuong Manh Le <cuong.manhle.vn@gmail.com> Run-TryBot: Cuong Manh Le <cuong.manhle.vn@gmail.com> TryBot-Result: Gopher Robot <gobot@golang.org> Reviewed-by: Cherry Mui <cherryyz@google.com> Reviewed-by: Than McIntosh <thanm@google.com>
What version of Go are you using (
go version
)?What did you do?
What did you expect to see?
go.uber.org/zap/zapcore
What did you see instead?
empty string
I see problem is right here but since I'm not aware how linker uses these prefixes I can't figure out how to properly fix this condition
go/src/debug/gosym/symtab.go
Lines 40 to 44 in 43c6ada
The text was updated successfully, but these errors were encountered: