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

runtime: make it easier to connect runtime types to DWARF types #24814

Closed
heschik opened this issue Apr 11, 2018 · 7 comments

Comments

Projects
None yet
6 participants
@heschik
Copy link
Contributor

commented Apr 11, 2018

When inspecting an interface object, debuggers need to tie the interface's dynamic type to a DWARF type so that they know how to decode the object the interface points to. This turns out to be quite tricky, because there isn't anything that directly ties a runtime._type to a DWARF DIE. Silly as it sounds, this is one of the more annoying parts of writing a Go debugging tool.

Both Delve and x/debug/gocore need to address this somehow, and both use the type name to do it. The problem is that the runtime type string and the DWARF type name are different. DWARF type names include the full package path (e.g. golang.org/x/debug/core.Process), but the runtime only has the package (e.g. core.Process). runtime._type and runtime.uncommontype have a package path, but that isn't good enough, because anonymous struct types are named by their fields (e.g. struct { sync.Mutex; runtime/pprof.profiling bool; runtime/pprof.done chan bool }), and there's no way to recover the package paths of the individual fields.

Because the runtime's information is incomplete, there's no reliable way to do the mapping. gocore strips the package paths off with some string manipulation, and Delve tries to guess the package path based only on the package. Both will fail in the presence of packages with the same name.

I propose we change the runtime's type strings to include package paths, and therefore match the names used in the DWARF. AFAICT the runtime uses these strings primarily for panic messages, so the primary user-visible effect of the change will be longer, but less ambiguous, panics. The other (minor) usage is to link methods across plugin boundaries, but this should be a strict improvement there, because it won't need to look at the package path specially. In fact, it might be possible to eliminate package paths from _type and uncommontype entirely.

/cc @hyangah @aarzilli @derekparker @aclements @crawshaw

@heschik heschik added the Debugging label Apr 11, 2018

@ianlancetaylor

This comment has been minimized.

Copy link
Contributor

commented Apr 11, 2018

The runtime type structures are the same as the reflect type structures, and the reflect package uses these strings for the String method, which has to continue producing the string that it always has. So if you want to change this string somehow, you have to do it in a way that preserves the behavior of the reflect package.

Also I note that these strings are a surprisingly large percentage of a Go binary, so we want to be careful about adding in a lot of duplicate package paths.

@heschik

This comment has been minimized.

Copy link
Contributor Author

commented Apr 11, 2018

Ugh. Of course. So we'd have to store two strings for anonymous structs, or do some substantial rewriting for them in reflect. And obviously the reason that package path is a separate field is so it can be shared. I don't know why I didn't realize that.

Okay, I will think harder about this. Maybe we should include runtime._type.hash in the type DIE or something.

@randall77

This comment has been minimized.

Copy link
Contributor

commented Apr 11, 2018

@aclements

This comment has been minimized.

Copy link
Member

commented Apr 12, 2018

+1. I've been bitten by this, too.

I agree that we should put the extra information in the DIEs if at all possible to keep the growth in strippable sections. I think adding the address of the runtime._type should be relatively straightforward, since the DWARF type DIEs are all being generated by the linker anyway. However, it may make it more difficult if we ever wanted to pull that out to the compiler.

One disadvantage of using a custom field in the DIE is that the GDB Python integration script won't be able to take advantage of it.

@gopherbot

This comment has been minimized.

Copy link

commented Apr 12, 2018

Change https://golang.org/cl/106775 mentions this issue: cmd/ld: link to runtime types from DWARF

@heschik

This comment has been minimized.

Copy link
Contributor Author

commented Apr 12, 2018

I gave it a try. For a linker change, it was actually pretty easy to get something that seems to mostly work. Would appreciate any experienced eyes.

@aarzilli

This comment has been minimized.

Copy link
Contributor

commented Apr 13, 2018

there's no way to recover the package paths of the individual fields.

Technically this isn't true, delve scans each struct field individually and recursively reconstructs its name. Writing that code wasn't especially fun though and since anonymous types are relatively rare who knows if it actually is bug free.

I too would vote for an extra DIE attribute.

@heschik heschik changed the title runtime: runtime._type name should match DWARF names runtime: make it easier to connect runtime types to DWARF types Apr 18, 2018

@gopherbot gopherbot closed this in 393f84a Apr 20, 2018

@golang golang locked and limited conversation to collaborators Apr 20, 2019

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
You can’t perform that action at this time.