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: generate correct DW_AT_external flag #22399

Open
thanm opened this Issue Oct 23, 2017 · 4 comments

Comments

Projects
None yet
5 participants
@thanm
Member

thanm commented Oct 23, 2017

The current (1.9) Go compiler emits DWARF records for subprograms and variables that contain the "DW_AT_external" attribute, a flag which DWARF defines as:

If the name of the subroutine described by an entry with the tag ... is visible
outside of its containing compilation unit, that entry has a DW_AT_external attribute,
which is a flag.

The compiler is assigning a value to DW_AT_external based on whether the LSym in question is marked as static (AttrStatic), which I don't think is the same as the definition above. When I look at the generated DWARF for things like the Go compiler, the only things being marked as non-external are a small number of vars and functions from assembly files.

Not sure this makes a huge amount of difference (as far as I can tell there is no degradation to the debugging experience as a result of package-local functions being marked as external), but it might be worth fixing this up just for consistency / POLA.

@heschik heschik added the Debugging label Oct 25, 2017

@ianlancetaylor ianlancetaylor added this to the Go1.11 milestone Mar 29, 2018

@odeke-em odeke-em changed the title from compiler: generated correct DW_AT_external flag to cmd/compile: generate correct DW_AT_external flag Apr 24, 2018

@odeke-em

This comment has been minimized.

Member

odeke-em commented Apr 24, 2018

Thank you @thanm for the report!

I've tweaked the title just a little and I'll page some of compiler folks @mdempsky @heschik @randall77 @griesemer.

@heschik

This comment has been minimized.

Contributor

heschik commented Apr 25, 2018

If it matters, I presume this is just a matter of checking the first rune of the name to see if it's upper case. But unless the flag is actually used for something somewhere, I might be inclined to just get rid of it instead.

@thanm

This comment has been minimized.

Member

thanm commented Apr 25, 2018

Well, hmm. I looked at the delve sources and it looks as though it's being relied on in github.com/derekparker/delve/pkg/dwarf/reader/reader.go right at the moment for variables. So at the moment it is being interpreted as a "this is interesting" flag and not a "this symbol visible outside its translation unit" flag. hmm.

@mdempsky

This comment has been minimized.

Member

mdempsky commented Apr 25, 2018

If it matters, I presume this is just a matter of checking the first rune of the name to see if it's upper case.

FWIW, it's unfortunately a little trickier than that, because of method symbols (e.g., "T.m"), method expression wrappers ("T.m-fm"), function value symbols ("f·f"), and function closures ("f.func1").

@ianlancetaylor ianlancetaylor modified the milestones: Go1.11, Go1.12 Jun 30, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment