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: record optimization level in binary #22168

Closed
aclements opened this Issue Oct 6, 2017 · 15 comments

Comments

Projects
None yet
6 participants
@aclements
Member

aclements commented Oct 6, 2017

Go's debug information for optimized binaries needs a lot of improvement (we're working on it). In the mean time, it would be valuable for debuggers to know whether a binary optimized or unoptimized. For example, this could be used (in combination with the toolchain version already recorded in the binary) to warn users that local variable information may be incorrect and that they should consider recompiling with optimizations disabled.

One complication is that it's currently possible (and fairly common) to compile different packages with different optimization levels. If the cmd/go changes go as planned, this will no longer be possible in 1.10, but may again become possible in the future.

Some reasonable ways to record this information are:

  1. Create a per-package symbol to indicate the optimization level. We could create this symbol only if the package is unoptimized, so we don't bloat optimized binaries with lots of extra symbols. This would be easy to get at from any symbolic debugger.

  2. Record this as a custom attribute in each DWARF function (there is no DIE corresponding to a package). This is arguably the "right" place to record this, but it requires help from the debugger's DWARF decoder to access.

I think both are pretty easy to implement. I would lean toward solution 1.

/cc @rsc @dr2chase @zombiezen

@aclements aclements added this to the Go1.10 milestone Oct 6, 2017

@ianlancetaylor

This comment has been minimized.

Show comment
Hide comment
@ianlancetaylor

ianlancetaylor Oct 6, 2017

Contributor

One place to put this kind of information in DWARF is in the DW_AT_producer attribute of the DW_TAG_compile_unit DIE. That's where GCC puts it. Admittedly the consumer has to parse the string.

Contributor

ianlancetaylor commented Oct 6, 2017

One place to put this kind of information in DWARF is in the DW_AT_producer attribute of the DW_TAG_compile_unit DIE. That's where GCC puts it. Admittedly the consumer has to parse the string.

@aclements

This comment has been minimized.

Show comment
Hide comment
@aclements

aclements Oct 6, 2017

Member

That doesn't let us record per-package information, though.

Member

aclements commented Oct 6, 2017

That doesn't let us record per-package information, though.

@ianlancetaylor

This comment has been minimized.

Show comment
Hide comment
@ianlancetaylor

ianlancetaylor Oct 6, 2017

Contributor

A package is a compilation unit.

Contributor

ianlancetaylor commented Oct 6, 2017

A package is a compilation unit.

@aclements

This comment has been minimized.

Show comment
Hide comment
@aclements

aclements Oct 6, 2017

Member

A package is a compilation unit.

In gc's DWARF output there's just one big compilation unit. Though it would be reasonable to change that.

Member

aclements commented Oct 6, 2017

A package is a compilation unit.

In gc's DWARF output there's just one big compilation unit. Though it would be reasonable to change that.

@thanm

This comment has been minimized.

Show comment
Hide comment
@thanm

thanm Oct 6, 2017

Member

I was also confused by this... right now the gc compiler doesn't emit the comp unit DIE-- rather a single unit is emitted by the linker. Having the compiler emit a compilation unit DIE seems like it might be nice.

Member

thanm commented Oct 6, 2017

I was also confused by this... right now the gc compiler doesn't emit the comp unit DIE-- rather a single unit is emitted by the linker. Having the compiler emit a compilation unit DIE seems like it might be nice.

@aclements

This comment has been minimized.

Show comment
Hide comment
@aclements

aclements Oct 8, 2017

Member

I looked into creating a separate CU for each package a bit. It's a little tricky since, as far as I can tell, the linker has lost track of which package a symbol belongs to by the point it's stitching together the DWARF. Symbols from different packages are also interleaved, so we'd have to produce DW_AT_ranges attributes for the CU PCs instead of just DW_AT_low_pc and DT_AT_high_pc like we do now, and those ranges could potentially be quite verbose.

Member

aclements commented Oct 8, 2017

I looked into creating a separate CU for each package a bit. It's a little tricky since, as far as I can tell, the linker has lost track of which package a symbol belongs to by the point it's stitching together the DWARF. Symbols from different packages are also interleaved, so we'd have to produce DW_AT_ranges attributes for the CU PCs instead of just DW_AT_low_pc and DT_AT_high_pc like we do now, and those ranges could potentially be quite verbose.

@aclements

This comment has been minimized.

Show comment
Hide comment
@aclements

aclements Oct 8, 2017

Member

Maybe the CU ranges wouldn't be so bad. In the cmd/go binary, there are 146 packages and only 240 transitions from one package to another in the symbol table based on:

readelf --debug-dump=info go | awk 'x {x=0;print $4} /subprogram/ {x=1}' | sed 's/\..*//' | grep -v ^type$ | sort | uniq | wc -l
readelf --debug-dump=info go | awk 'x {x=0;print $4} /subprogram/ {x=1}' | sed 's/\..*//' | grep -v ^type$ | uniq | wc -l
Member

aclements commented Oct 8, 2017

Maybe the CU ranges wouldn't be so bad. In the cmd/go binary, there are 146 packages and only 240 transitions from one package to another in the symbol table based on:

readelf --debug-dump=info go | awk 'x {x=0;print $4} /subprogram/ {x=1}' | sed 's/\..*//' | grep -v ^type$ | sort | uniq | wc -l
readelf --debug-dump=info go | awk 'x {x=0;print $4} /subprogram/ {x=1}' | sed 's/\..*//' | grep -v ^type$ | uniq | wc -l
@thanm

This comment has been minimized.

Show comment
Hide comment
@thanm

thanm Oct 8, 2017

Member

My understanding was that the hi/lo PC range for the CU DIE was optional -- perhaps it would make sense to just leave it out altogether. Certainly for C++ programs where there is linker reordering of functions (for example, based on profile data), hi/lo PC or ranges would not be generated.

Member

thanm commented Oct 8, 2017

My understanding was that the hi/lo PC range for the CU DIE was optional -- perhaps it would make sense to just leave it out altogether. Certainly for C++ programs where there is linker reordering of functions (for example, based on profile data), hi/lo PC or ranges would not be generated.

@aclements

This comment has been minimized.

Show comment
Hide comment
@aclements

aclements Oct 9, 2017

Member

My understanding was that the hi/lo PC range for the CU DIE was optional -- perhaps it would make sense to just leave it out altogether.

Ah, well that would be more convenient.

Still, I think the harder part is that it's lost track of the packages when it creates the CU. I compared the symbols in ctxt.Textp (which is what it uses now) with the symbols in ctxt.Library[x].Textp, but the lists are different. ctxt.Library[x].Textp has a bunch of extra symbols, which I assume are ones that were dead-code eliminated. And ctxt.Textp has a bunch of symbols that aren't in the per-package list: the type symbols and what appear to be method symbols for generated wrappers and embedded types. Maybe we could add a *Library field to symbols and make sure it's filled in for these generated symbols.

Given that the CU DIE is constructed by the linker, we would also need a way to communicate the optimization level from the compiler to the linker to be put in this DIE.

Member

aclements commented Oct 9, 2017

My understanding was that the hi/lo PC range for the CU DIE was optional -- perhaps it would make sense to just leave it out altogether.

Ah, well that would be more convenient.

Still, I think the harder part is that it's lost track of the packages when it creates the CU. I compared the symbols in ctxt.Textp (which is what it uses now) with the symbols in ctxt.Library[x].Textp, but the lists are different. ctxt.Library[x].Textp has a bunch of extra symbols, which I assume are ones that were dead-code eliminated. And ctxt.Textp has a bunch of symbols that aren't in the per-package list: the type symbols and what appear to be method symbols for generated wrappers and embedded types. Maybe we could add a *Library field to symbols and make sure it's filled in for these generated symbols.

Given that the CU DIE is constructed by the linker, we would also need a way to communicate the optimization level from the compiler to the linker to be put in this DIE.

@dr2chase

This comment has been minimized.

Show comment
Hide comment
@dr2chase

dr2chase Oct 9, 2017

Contributor

I would normally assume that we want this on a per-function granularity, for the use case of "this fails in production, optimization hides the debugging info, but I cannot afford to run the whole thing unoptimized, therefore I will use the (as-yet non-existent) //go:noopt comment to disable optimization of the single function I really need to catch in the act".

However, if we plan to get our optimized-debugging story good enough, we don't want to introduce a feature that we hope to deprecate within a year.

Do I have this right?

Contributor

dr2chase commented Oct 9, 2017

I would normally assume that we want this on a per-function granularity, for the use case of "this fails in production, optimization hides the debugging info, but I cannot afford to run the whole thing unoptimized, therefore I will use the (as-yet non-existent) //go:noopt comment to disable optimization of the single function I really need to catch in the act".

However, if we plan to get our optimized-debugging story good enough, we don't want to introduce a feature that we hope to deprecate within a year.

Do I have this right?

@aarzilli

This comment has been minimized.

Show comment
Hide comment
@aarzilli

aarzilli Oct 10, 2017

Contributor

I should point out that #19340 is about insuring that optimized and non-optimized packages aren't mixed together accidentally.

I would normally assume that we want this on a per-function granularity

I don't think there is a standard way to say this in DWARF, it would have to be an extension attribute.

Contributor

aarzilli commented Oct 10, 2017

I should point out that #19340 is about insuring that optimized and non-optimized packages aren't mixed together accidentally.

I would normally assume that we want this on a per-function granularity

I don't think there is a standard way to say this in DWARF, it would have to be an extension attribute.

@aclements

This comment has been minimized.

Show comment
Hide comment
@aclements

aclements Oct 10, 2017

Member

I should point out that #19340 is about insuring that optimized and non-optimized packages aren't mixed together accidentally.

Right. Russ is hoping to get something out for 1.10 that will prevent accidentally mixing and matching gcflags. (Though it's possible that, in the future, there will be a way to intentionally specify gcflags for specific packages in a way that plays nicely with caching.)

I don't think there is a standard way to say this in DWARF, it would have to be an extension attribute.

Yes. I believe this is true whether this is function granularity or package granularity or anything else. I'm open to suggestions for what this attribute should be, especially if there are already existing vendor attributes like this.

Member

aclements commented Oct 10, 2017

I should point out that #19340 is about insuring that optimized and non-optimized packages aren't mixed together accidentally.

Right. Russ is hoping to get something out for 1.10 that will prevent accidentally mixing and matching gcflags. (Though it's possible that, in the future, there will be a way to intentionally specify gcflags for specific packages in a way that plays nicely with caching.)

I don't think there is a standard way to say this in DWARF, it would have to be an extension attribute.

Yes. I believe this is true whether this is function granularity or package granularity or anything else. I'm open to suggestions for what this attribute should be, especially if there are already existing vendor attributes like this.

@aclements

This comment has been minimized.

Show comment
Hide comment
@aclements

aclements Oct 10, 2017

Member

I discussed this with several other runtime and compiler people today and we decided to split up the packages into separate compilation unit DIEs (which I've already implemented), and add the compile command line flags to DW_AT_producer. This is similar to what other toolchains do and doesn't require a vendor attribute. It's also more flexible than a boolean optimized/unoptimized flag, especially considering there are already multiple compiler flags that affect optimization behavior.

Member

aclements commented Oct 10, 2017

I discussed this with several other runtime and compiler people today and we decided to split up the packages into separate compilation unit DIEs (which I've already implemented), and add the compile command line flags to DW_AT_producer. This is similar to what other toolchains do and doesn't require a vendor attribute. It's also more flexible than a boolean optimized/unoptimized flag, especially considering there are already multiple compiler flags that affect optimization behavior.

@gopherbot

This comment has been minimized.

Show comment
Hide comment
@gopherbot

gopherbot Oct 11, 2017

Change https://golang.org/cl/69973 mentions this issue: cmd/link: one DWARF compilation unit per package

gopherbot commented Oct 11, 2017

Change https://golang.org/cl/69973 mentions this issue: cmd/link: one DWARF compilation unit per package

gopherbot pushed a commit that referenced this issue Oct 12, 2017

cmd/link: one DWARF compilation unit per package
Currently, the linker generates one huge DWARF compilation unit for
the entire Go binary. This commit creates a separate compilation unit
and line table per Go package.

We temporarily lose compilation unit PC range information, since it's
now discontiguous, so harder to emit. We'll bring it back in the next
commit.

Beyond being "more traditional", this has various technical
advantages:

* It should speed up line table lookup, since that requires a
  sequential scan of the line table. With this change, a debugger can
  first locate the per-package line table and then scan only that line
  table.

* Once we emit compilation unit PC ranges again, this should also
  speed up various other debugger reverse PC lookups.

* It puts us in a good position to move more DWARF generation into the
  compiler, which could produce at least the CU header, per-function
  line table fragments, and per-function frame unwinding info that the
  linker could simply paste together.

* It will let us record a per-package compiler command-line flags
  (#22168).

Change-Id: Ibac642890984636b3ef1d4b37fe97f4453c2cc84
Reviewed-on: https://go-review.googlesource.com/69973
Run-TryBot: Austin Clements <austin@google.com>
Reviewed-by: Heschi Kreinick <heschi@google.com>
Reviewed-by: Than McIntosh <thanm@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
@gopherbot

This comment has been minimized.

Show comment
Hide comment
@gopherbot

gopherbot Oct 17, 2017

Change https://golang.org/cl/71430 mentions this issue: cmd/compile, cmd/link: record compiler flags in DW_AT_producer

gopherbot commented Oct 17, 2017

Change https://golang.org/cl/71430 mentions this issue: cmd/compile, cmd/link: record compiler flags in DW_AT_producer

@gopherbot gopherbot closed this in 2c1d2e0 Oct 18, 2017

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