respect global strip config

[malt3]

What type of PR is this?

Bug fix

What does this PR do? Why is it needed?

The docs currently state:

Strips symbols from compiled packages and linked binaries (using the -w flag). May also be set with the --strip command line option, which affects C/C++ targets, too.

This is not true. This option cannot be set at all (as far as I can tell). It is currently not exposed as a config flag and the global bazel option for stripping is not considered either.
This PR removes the @io_bazel_rules_go//go/config:strip config setting and always uses the global bazel setting --strip and --compilation_mode to deduce if stripping should be enabled.
As a little extension, I also changed the linker flags to also include -s if stripping is enabled.

Which issues(s) does this PR fix?

Fixes #3325

Other notes for review

This is one possible resolution. See also #3526 for an alternative solution that exposes this setting as a flag.

[fmeum]

This looks good to me, including the addition of the -s flag.

Given the very terse descriptions of the -s and -w flags in the go tool link documentation, could you add a test that verifies that stack traces in panics still have all the expected information?

Ideally that test would also check that stripping is enabled when it should be, which would probably have caught the typo below.

[malt3]

could you add a test that verifies that stack traces in panics still have all the expected information

Yes! I'll try to get to it very soon.

Ideally that test would also check that stripping is enabled when it should be, which would probably have caught the typo below.

Do you know any simple, platform agnostic way to validate this? As far as I know, checking for a stripped binary involves parsing the OS-specific binary format (ELF, MACH-O, PE).

[malt3]

I implemented tests for ELF, MACH-O and PE. The checks for debug symbols in the resulting binaries are not very good and could certainly be improved if desired. I'm not sure where to go with this or what to test for in particular.

I also added tests to trigger a panic, recover from it and compare the stack trace against a known good stack trace.

[fmeum]

This is great test coverage. That will make it much easier to maintain this feature.

I see that the windows run is failing, but everything else looks good.

[malt3]

I see that the windows run is failing

EDIT: should work now.

This is the difference in output of readpe -S <binary>.exe between stripped vs non-stripped pe files.
I don't have a windows test environment to debug this properly but I'll try to get my hands on one. Maybe the values reported by the go pe library are off by some bit shift.

--- <unnamed>
+++ <unnamed>
@@ -13,7 +13,7 @@
                                              IMAGE_SCN_MEM_READ
     Section
         Name:                            .rdata
-        Virtual Size:                    0x9e3b0 (648112 bytes)
+        Virtual Size:                    0x9e3d0 (648144 bytes)
         Virtual Address:                 0x8f000
         Size Of Raw Data:                0x9e400 (648192 bytes)
         Pointer To Raw Data:             0x8d800
@@ -24,7 +24,7 @@
                                              IMAGE_SCN_MEM_READ
     Section
         Name:                            .data
-        Virtual Size:                    0x6f350 (455504 bytes)
+        Virtual Size:                    0x6f370 (455536 bytes)
         Virtual Address:                 0x12e000
         Size Of Raw Data:                0x17a00 (96768 bytes)
         Pointer To Raw Data:             0x12bc00
@@ -35,95 +35,11 @@
                                              IMAGE_SCN_MEM_READ
                                              IMAGE_SCN_MEM_WRITE
     Section
-        Name:                            /4
-        Virtual Size:                    0x127 (295 bytes)
-        Virtual Address:                 0x19e000
-        Size Of Raw Data:                0x200 (512 bytes)
-        Pointer To Raw Data:             0x143600
-        Number Of Relocations:           0
-        Characteristics:                 0x42100040
-        Characteristic Names
-                                             IMAGE_SCN_CNT_INITIALIZED_DATA
-                                             IMAGE_SCN_MEM_DISCARDABLE
-                                             IMAGE_SCN_MEM_READ
-    Section
-        Name:                            /19
-        Virtual Size:                    0x1df6b (122731 bytes)
-        Virtual Address:                 0x19f000
-        Size Of Raw Data:                0x1e000 (122880 bytes)
-        Pointer To Raw Data:             0x143800
-        Number Of Relocations:           0
-        Characteristics:                 0x42100040
-        Characteristic Names
-                                             IMAGE_SCN_CNT_INITIALIZED_DATA
-                                             IMAGE_SCN_MEM_DISCARDABLE
-                                             IMAGE_SCN_MEM_READ
-    Section
-        Name:                            /32
-        Virtual Size:                    0x5a27 (23079 bytes)
-        Virtual Address:                 0x1bd000
-        Size Of Raw Data:                0x5c00 (23552 bytes)
-        Pointer To Raw Data:             0x161800
-        Number Of Relocations:           0
-        Characteristics:                 0x42100040
-        Characteristic Names
-                                             IMAGE_SCN_CNT_INITIALIZED_DATA
-                                             IMAGE_SCN_MEM_DISCARDABLE
-                                             IMAGE_SCN_MEM_READ
-    Section
-        Name:                            /46
-        Virtual Size:                    0x2c (44 bytes)
-        Virtual Address:                 0x1c3000
-        Size Of Raw Data:                0x200 (512 bytes)
-        Pointer To Raw Data:             0x167400
-        Number Of Relocations:           0
-        Characteristics:                 0x42100040
-        Characteristic Names
-                                             IMAGE_SCN_CNT_INITIALIZED_DATA
-                                             IMAGE_SCN_MEM_DISCARDABLE
-                                             IMAGE_SCN_MEM_READ
-    Section
-        Name:                            /65
-        Virtual Size:                    0x388f0 (231664 bytes)
-        Virtual Address:                 0x1c4000
-        Size Of Raw Data:                0x38a00 (231936 bytes)
-        Pointer To Raw Data:             0x167600
-        Number Of Relocations:           0
-        Characteristics:                 0x42100040
-        Characteristic Names
-                                             IMAGE_SCN_CNT_INITIALIZED_DATA
-                                             IMAGE_SCN_MEM_DISCARDABLE
-                                             IMAGE_SCN_MEM_READ
-    Section
-        Name:                            /78
-        Virtual Size:                    0x1ccc3 (117955 bytes)
-        Virtual Address:                 0x1fd000
-        Size Of Raw Data:                0x1ce00 (118272 bytes)
-        Pointer To Raw Data:             0x1a0000
-        Number Of Relocations:           0
-        Characteristics:                 0x42100040
-        Characteristic Names
-                                             IMAGE_SCN_CNT_INITIALIZED_DATA
-                                             IMAGE_SCN_MEM_DISCARDABLE
-                                             IMAGE_SCN_MEM_READ
-    Section
-        Name:                            /90
-        Virtual Size:                    0x974f (38735 bytes)
-        Virtual Address:                 0x21a000
-        Size Of Raw Data:                0x9800 (38912 bytes)
-        Pointer To Raw Data:             0x1bce00
-        Number Of Relocations:           0
-        Characteristics:                 0x42100040
-        Characteristic Names
-                                             IMAGE_SCN_CNT_INITIALIZED_DATA
-                                             IMAGE_SCN_MEM_DISCARDABLE
-                                             IMAGE_SCN_MEM_READ
-    Section
         Name:                            .idata
         Virtual Size:                    0x490 (1168 bytes)
-        Virtual Address:                 0x224000
+        Virtual Address:                 0x19e000
         Size Of Raw Data:                0x600 (1536 bytes)
-        Pointer To Raw Data:             0x1c6600
+        Pointer To Raw Data:             0x143600
         Number Of Relocations:           0
         Characteristics:                 0xc0000040
         Characteristic Names
@@ -133,9 +49,9 @@
     Section
         Name:                            .reloc
         Virtual Size:                    0x2f06 (12038 bytes)
-        Virtual Address:                 0x225000
+        Virtual Address:                 0x19f000
         Size Of Raw Data:                0x3000 (12288 bytes)
-        Pointer To Raw Data:             0x1c6c00
+        Pointer To Raw Data:             0x143c00
         Number Of Relocations:           0
         Characteristics:                 0x42000040
         Characteristic Names
@@ -144,10 +60,10 @@
                                              IMAGE_SCN_MEM_READ
     Section
         Name:                            .symtab
-        Virtual Size:                    0x17e7a (97914 bytes)
-        Virtual Address:                 0x228000
-        Size Of Raw Data:                0x18000 (98304 bytes)
-        Pointer To Raw Data:             0x1c9c00
+        Virtual Size:                    0x4 (4 bytes)
+        Virtual Address:                 0x1a2000
+        Size Of Raw Data:                0x200 (512 bytes)
+        Pointer To Raw Data:             0x146c00
         Number Of Relocations:           0
         Characteristics:                 0x42000000
         Characteristic Names

[malt3]

Should there be a warning to users like @linzhp since we change the default mode to strip (as a result of respecting the global defaults)?

[linzhp]

Thanks for letting me know. We can pull this into Uber's Go repo and test it

[fmeum]

We should definitely mention this in the release notes. Since we are preserving debug information and -c opt builds with default settings aren't affected, I would have said that's sufficient, but would like to hear what @linzhp and @achew22 think.

[linzhp]

Should we also pass -dwarf=false to the Go compiler when go.mode.strip is True?

[malt3]

I'm not sure I can make a statement on the behavior with regards to the bazel cache when setting -dwarf=false.
Or to rephrase:
Could this lead to missing dwarf info if I first build a stripped binary and then rebuild with debugging info? Maybe someone more experienced with the internals can comment.

This discussion has some stats on the performance impact of dwarf generation: golang/go#38777
Not a huge difference, but not nothing either so I'd be happy to add the flag.

[linzhp]

Bazel would rebuild everything when switching from -c fastbuild to -c dbg. In addition, -c dbg adds -N -l to the compiler, which makes it unnecessary to generate dwarf symbols.

On the other hand, I am thinking about add -N -l to fastbuild too, to speed up the build. In that case, whether -dwarf is true or false no longer matters. Is that correct? @sywhang

[linzhp]

All tests passed in Uber, after some fixes removing the usage of the old strip flag, and using transitions to dbg mode when debug symbols are needed.