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/compile: user symbols can be in reserved namespace #37762

Open
Pr0Ger opened this issue Mar 9, 2020 · 20 comments
Open

cmd/compile: user symbols can be in reserved namespace #37762

Pr0Ger opened this issue Mar 9, 2020 · 20 comments
Milestone

Comments

@Pr0Ger
Copy link

@Pr0Ger Pr0Ger commented Mar 9, 2020

What version of Go are you using (go version)?

$ go version
go version go1.14 darwin/amd64

What did you do?

package main

import "debug/gosym"

func main()  {
	s1 := gosym.Sym{Name: "go.uber.org/zap/zapcore.(*CheckedEntry).Write"}

	println(s1.PackageName())
}

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

// A prefix of "type." and "go." is a compiler-generated symbol that doesn't belong to any package.
// See variable reservedimports in cmd/compile/internal/gc/subr.go
if strings.HasPrefix(name, "go.") || strings.HasPrefix(name, "type.") {
return ""
}

@ianlancetaylor
Copy link
Contributor

@ianlancetaylor ianlancetaylor commented Mar 9, 2020

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 gosym.Sym.

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.

@Pr0Ger
Copy link
Author

@Pr0Ger Pr0Ger commented Mar 9, 2020

Yeah, sure (go.uber.org/zap is a public available library):

package main

import (
	"debug/gosym"
	"debug/macho"
	"fmt"
	"os"
	"strings"

	"go.uber.org/zap"
)

func main() {
	logger, _ := zap.NewDevelopment()
	logger.Debug("using logger just a little bit")

	executable, _ := os.Executable()
	exe, _ := macho.Open(executable)

	pclndat, _ := exe.Section("__gopclntab").Data()
	symTabRaw, _ := exe.Section("__gosymtab").Data()

	pcln := gosym.NewLineTable(pclndat, exe.Section("__text").Addr)
	symTab, _ := gosym.NewTable(symTabRaw, pcln)
	for _, funk := range symTab.Funcs {
		if strings.Contains(funk.Name, "go.uber.org") {
			fmt.Printf("%s -> %s\n", funk.Name, funk.PackageName())
		}
	}
}

And output (just a few lines):

❯ go run test.go
go: finding module for package go.uber.org/zap
go: downloading go.uber.org/zap v1.14.0
go: found go.uber.org/zap in go.uber.org/zap v1.14.0
2020-03-09T22:16:31.280+0300	DEBUG	test_package_name/test.go:15	using logger just a little bit
go.uber.org/zap/buffer.(*Buffer).AppendString ->
go.uber.org/zap/buffer.(*Buffer).AppendBool ->
...
@ianlancetaylor
Copy link
Contributor

@ianlancetaylor ianlancetaylor commented Mar 9, 2020

Thanks. If user packages can be in the compiler-reserved "go." namespace, that seems like a potential problem.

CC @griesemer @mdempsky @josharian @randall77

@ianlancetaylor ianlancetaylor added this to the Go1.15 milestone Mar 9, 2020
@ianlancetaylor ianlancetaylor changed the title debug/gosym: PackageName() cmd/compile: user symbols can be in reserved namespace Mar 9, 2020
@mdempsky
Copy link
Member

@mdempsky mdempsky commented Mar 9, 2020

Bitten again by our ad hoc symbol mangling rules. :(

I think a short-term workaround could be debug/gosym only checks for "go." and "type." when it can't find a '/'. I don't think the compiler ever generates go.* or type.* symbols that contain a /.

This would still cause problems for top-level user package paths like "go.mystuff" (as opposed to "go.mystuff/dir"). Not sure how to best address that off hand.

Edit: Nevermind, I'm mistaken:

$ nm $(go tool -n compile) | grep 'go\..*/' | head -1
0000000000d9aca0 R go.itab.*cmd/compile/internal/gc.blockHeap,container/heap.Interface
@cuonglm
Copy link
Contributor

@cuonglm cuonglm commented Mar 11, 2020

Can we change go. to go.. instead for compiler generated symbols?

@cuonglm
Copy link
Contributor

@cuonglm cuonglm commented Mar 18, 2020

Can we change go. to go.. instead for compiler generated symbols?

Seems doable, I make a quick change to go.itab -> go..itab and it's compiled ok and test passed.

@odeke-em
Copy link
Member

@odeke-em odeke-em commented Mar 28, 2020

Some 2 years ago, there was a somewhat similar issue #25113 reported and fixed by @aarzilli.
We had clashes with userland variables that collided with compiler generated names such as statictmp_0, and we had to rename from statictmp -> .stmp, from @josharian's suggestion in CL https://go-review.googlesource.com/c/go/+/142497. Seems like a similar mangling scheme could help here, as @cuonglm suggested in #37762 (comment), thank you!

@gopherbot
Copy link

@gopherbot gopherbot commented Mar 29, 2020

Change https://golang.org/cl/226282 mentions this issue: cmd/compile,link: use "go.." for compiler genereated types

@gopherbot
Copy link

@gopherbot gopherbot commented Mar 29, 2020

Change https://golang.org/cl/226283 mentions this issue: debug/gosym: correct checking condition for compiler generated symbols

@odeke-em
Copy link
Member

@odeke-em odeke-em commented May 26, 2020

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.

@cuonglm
Copy link
Contributor

@cuonglm cuonglm commented May 26, 2020

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

@odeke-em
Copy link
Member

@odeke-em odeke-em commented May 26, 2020

Awesome, thank you for the advice @cuonglm!

@odeke-em odeke-em modified the milestones: Go1.15, Go1.16 May 26, 2020
@mdempsky
Copy link
Member

@mdempsky mdempsky commented Sep 10, 2020

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 go. and type. to go: and type:.

@cherrymui
Copy link
Contributor

@cherrymui cherrymui commented Sep 10, 2020

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.

@mdempsky
Copy link
Member

@mdempsky mdempsky commented Sep 10, 2020

@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).

@cherrymui
Copy link
Contributor

@cherrymui cherrymui commented Sep 10, 2020

Issue #19529 is one, where the external linker doesn't like the @ character.

@mdempsky
Copy link
Member

@mdempsky mdempsky commented Sep 10, 2020

@cherrymui Interesting, thanks. As far as I can tell skimming https://www.akkadia.org/drepper/symbol-versioning and looking at strings -a /lib/x86_64-linux-gnu/libc.so.6, the @ isn't actually part of the ELF symbol. Instead it seems just like a UI convention (e.g., recognized by the assembler, and used by readelf for printing, but not nm -D). So it at least seems like @ should be safe. I'm curious what exactly the underlying issue there is.

If : was an issue though, I think we'd know about it because of struct tag conventions; for example:

$ nm `which go` | grep :
00000000007bfae0 T type..eq.struct { Logentry struct { Revision int64 "xml:\"revision,attr\""; Date string "xml:\"date\"" } "xml:\"logentry\"" }
00000000007bfa60 T type..eq.struct { Revision int64 "xml:\"revision,attr\""; Date string "xml:\"date\"" }

Looking at other reserved symbols, it seems like these get used for compiling the runtime, so presumably they're all safe: ()[]{},;*. So we could use one of those instead if we're concerned.

I wonder if it's worth putting special characters in a runtime struct tag or something to help smoke out linker issues.

@ianlancetaylor
Copy link
Contributor

@ianlancetaylor ianlancetaylor commented Sep 10, 2020

You won't see an @ in a symbol name in a shared library or an executable, because they store the version information in .gnu.version sections, but you will see them in object files. That is how an object file indicates which version to use for a symbol. See https://www.airs.com/blog/archives/220 .

@mdempsky
Copy link
Member

@mdempsky mdempsky commented Sep 10, 2020

Ah, I see. I thought the .gnu.version stuff was also used in object files. That makes sense then.

@ianlancetaylor
Copy link
Contributor

@ianlancetaylor ianlancetaylor commented Sep 11, 2020

The probably should have used the section in object files too. I don't know why they didn't.

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
7 participants
You can’t perform that action at this time.