rules_go icon indicating copy to clipboard operation
rules_go copied to clipboard

Expose an option or flag to disable stripping of symbols on `go_test` targets without `--@io_bazel_rules_go//go/config:debug`

Open rickystewart opened this issue 1 year ago • 4 comments

As of #2573, builds of go_test targets will always result in a binary with its symbols. In #2691, it's reiterated this is working as intended.

This behavior can be disabled with --@io_bazel_rules_go//go/config:debug, but that has the additional implication of disabling inlining. So, if you are trying to diagnose a problem or especially debug anything related to performance, this is not an option you realistically have as any investigation you will do will be on a generated binary that is probably substantively different from your production binary.

In the GitHub issues discussing this, it's claimed that this behavior was adopted for a performance benefit. Maybe that is true (I haven't profiled anything using recent versions of go, although it's possible the situation has changed in 4 years), but there should be some option that one can apply to disable this stripping where relevant without enabling any other command-line flags that would impact the performance or behavior of the generated binary.

I can see two obvious options:

  1. Provide a new flag (e.g. --@io_bazel-rules_go//go/config:no_strip_tests). Update the logic here to not add -s -w if either debug or no_strip_tests are set, instead of only checking debug.
  2. Honor --strip, i.e. do not strip these binaries if --strip=never is set.

Alternatively this stripping behavior could be updated to be opt-in instead of mandatory. It could also be re-evaluated whether this optimization is needed on the latest versions of the Go SDK.

If it's agreed option 1 ("Provide a new flag...") is best, I can submit a PR to that effect.

rickystewart avatar Sep 12 '24 18:09 rickystewart

I would very much like to do what go does by default. Do you happen to know what it does nowadays?

I like the idea of honoring --strip=never more than adding a new flag.

fmeum avatar Sep 12 '24 20:09 fmeum

By default, with no other flags set, go test -c builds a binary that does not have symbols stripped, i.e., go tool objdump on the binary does not return any sort of error. (With the symbols stripped with a Bazel-built test binary, we see that go tool objdump returns the error objdump: disassemble ...: no symbol section)

Interestingly, the previous comments on the topic seem to suggest that rules_go's current behavior was, at least at the time, consistent with what go does by default. Maybe that was the case at the time and go updated its default behavior. Today, the behavior is not consistent with what go does by default (and unfortunately outside of patching rules_go there is no way to get that behavior).

rickystewart avatar Sep 12 '24 21:09 rickystewart

@jayconrod Do you happen to know whether this changed?

fmeum avatar Sep 12 '24 21:09 fmeum

By default, with no other flags set, go test -c builds a binary that does not have symbols stripped, i.e., go tool objdump on the binary does not return any sort of error. (With the symbols stripped with a Bazel-built test binary, we see that go tool objdump returns the error objdump: disassemble ...: no symbol section)

(I'm talking specifically about Linux, by the way. Mac seems to behave slightly differently.)

rickystewart avatar Sep 12 '24 22:09 rickystewart

I think the confusion in part comes from go test without -c which compiles and runs the tests behaves differently from go test with the -c. From bazel's perspective, I don't think that there is a difference -- bazel always wants to compile the test binary and then run it.

I think if we look at @jayconrod's perspective here, it's correct only if you assume you never want to actually look at the built test binary or use it for anything other than running the tests.

This is all nuanced, but my sense is that honoring --strip=never is the most correct thing to do. As much as it'd be nice to "do what go does by default", that's not the state of the world regarding debug information and rules_go as it stands and it's not obvious to me the concepts even align. By default, go will not writing the binaries out which is the case for go run and go test (but not go build or go test -c). Bazel on the other hand will strip by default if using anything other than -c dbg (I think).

ajwerner avatar Nov 01 '24 14:11 ajwerner

Doubling down on this, the default for everything other than -c dbg builds will not change for today unless the user has explicitly set --strip=never, which means that they have opted into the slower link times. I'd suggest that that doesn't violate anything about what @jayconrod was saying in #2691, which is about the default regardless of the --strip flag.

ajwerner avatar Nov 01 '24 14:11 ajwerner

I put up https://github.com/bazel-contrib/rules_go/pull/4164 which makes tests respect --strip exactly like binaries. The tests now assert that. PTAL

ajwerner avatar Nov 01 '24 17:11 ajwerner