respect global strip config (#3527)

10 files changed, 312 insertions, 21 deletions

diff --git a/BUILD.bazel b/BUILD.bazel
index 97380f97..60bf72eb 100644
--- a/BUILD.bazel
+++ b/BUILD.bazel
@@ -130,7 +130,11 @@ go_config(
         "//conditions:default": False,
     }),
     static = "//go/config:static",
-    strip = "//go/config:strip",
+    strip = select({
+        "//go/private:is_strip_always": True,
+        "//go/private:is_strip_sometimes_fastbuild": True,
+        "//conditions:default": False,
+    }),
     visibility = ["//visibility:public"],
 )
 
diff --git a/go/config/BUILD.bazel b/go/config/BUILD.bazel
index 12075e37..a292fc9d 100644
--- a/go/config/BUILD.bazel
+++ b/go/config/BUILD.bazel
@@ -1,7 +1,6 @@
 load(
     "@bazel_skylib//rules:common_settings.bzl",
     "bool_flag",
-    "bool_setting",
     "string_flag",
     "string_list_flag",
 )
@@ -34,12 +33,6 @@ bool_flag(
     visibility = ["//visibility:public"],
 )
 
-bool_setting(
-    name = "strip",
-    build_setting_default = False,
-    visibility = ["//visibility:public"],
-)
-
 bool_flag(
     name = "debug",
     build_setting_default = False,
diff --git a/go/modes.rst b/go/modes.rst
index 0c8b4c1f..431049d5 100644
--- a/go/modes.rst
+++ b/go/modes.rst
@@ -69,12 +69,6 @@ or using `Bazel configuration transitions`_.
 | ``CGO_ENABLED=0``). Packages that contain cgo code may still be built, but   |
 | the cgo code will be filtered out, and the ``cgo`` build tag will be false.  |
 +-------------------+---------------------+------------------------------------+
-| :param:`strip`    | :type:`bool`        | :value:`false`                     |
-+-------------------+---------------------+------------------------------------+
-| 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.                                                  |
-+-------------------+---------------------+------------------------------------+
 | :param:`debug`    | :type:`bool`        | :value:`false`                     |
 +-------------------+---------------------+------------------------------------+
 | Includes debugging information in compiled packages (using the ``-N`` and    |
diff --git a/go/private/BUILD.bazel b/go/private/BUILD.bazel
index 2a83778e..16907b53 100644
--- a/go/private/BUILD.bazel
+++ b/go/private/BUILD.bazel
@@ -30,6 +30,21 @@ config_setting(
     visibility = ["//:__pkg__"],
 )
 
+config_setting(
+    name = "is_strip_always",
+    values = {"strip": "always"},
+    visibility = ["//:__pkg__"],
+)
+
+config_setting(
+    name = "is_strip_sometimes_fastbuild",
+    values = {
+        "strip": "sometimes",
+        "compilation_mode": "fastbuild",
+    },
+    visibility = ["//:__pkg__"],
+)
+
 bzl_library(
     name = "context",
     srcs = ["context.bzl"],
diff --git a/go/private/actions/link.bzl b/go/private/actions/link.bzl
index b0c1a108..a13d4de9 100644
--- a/go/private/actions/link.bzl
+++ b/go/private/actions/link.bzl
@@ -181,7 +181,7 @@ def emit_link(
     # Do not remove, somehow this is needed when building for darwin/arm only.
     tool_args.add("-buildid=redacted")
     if go.mode.strip:
-        tool_args.add("-w")
+        tool_args.add("-s", "-w")
     tool_args.add_joined("-extldflags", extldflags, join_with = " ")
 
     conflict_err = _check_conflicts(arcs)
diff --git a/go/private/context.bzl b/go/private/context.bzl
index 65f73b57..3ee11e35 100644
--- a/go/private/context.bzl
+++ b/go/private/context.bzl
@@ -830,7 +830,7 @@ def _go_config_impl(ctx):
         race = ctx.attr.race[BuildSettingInfo].value,
         msan = ctx.attr.msan[BuildSettingInfo].value,
         pure = ctx.attr.pure[BuildSettingInfo].value,
-        strip = ctx.attr.strip[BuildSettingInfo].value,
+        strip = ctx.attr.strip,
         debug = ctx.attr.debug[BuildSettingInfo].value,
         linkmode = ctx.attr.linkmode[BuildSettingInfo].value,
         gc_linkopts = ctx.attr.gc_linkopts[BuildSettingInfo].value,
@@ -860,10 +860,7 @@ go_config = rule(
             mandatory = True,
             providers = [BuildSettingInfo],
         ),
-        "strip": attr.label(
-            mandatory = True,
-            providers = [BuildSettingInfo],
-        ),
+        "strip": attr.bool(mandatory = True),
         "debug": attr.label(
             mandatory = True,
             providers = [BuildSettingInfo],
diff --git a/go/private/rules/transition.bzl b/go/private/rules/transition.bzl
index 8ee26ae7..4e87e30e 100644
--- a/go/private/rules/transition.bzl
+++ b/go/private/rules/transition.bzl
@@ -174,7 +174,6 @@ _common_reset_transition_dict = dict({
     "//go/config:msan": False,
     "//go/config:race": False,
     "//go/config:pure": False,
-    "//go/config:strip": False,
     "//go/config:debug": False,
     "//go/config:linkmode": LINKMODE_NORMAL,
     "//go/config:tags": [],
diff --git a/tests/core/strip/BUILD.bazel b/tests/core/strip/BUILD.bazel
new file mode 100644
index 00000000..bc2f6bd8
--- /dev/null
+++ b/tests/core/strip/BUILD.bazel
@@ -0,0 +1,6 @@
+load("@io_bazel_rules_go//go/tools/bazel_testing:def.bzl", "go_bazel_test")
+
+go_bazel_test(
+    name = "strip_test",
+    srcs = ["strip_test.go"],
+)
diff --git a/tests/core/strip/README.rst b/tests/core/strip/README.rst
new file mode 100644
index 00000000..1b5c1f96
--- /dev/null
+++ b/tests/core/strip/README.rst
@@ -0,0 +1,13 @@
+symbol stripping
+====================
+
+strip_test
+---------
+
+Tests that the global Bazel configuration for stripping are applied to go_binary
+targets.
+In particular, it tests that stripping is performed iff the bazel flag ``--strip``
+is set to ``always`` or ``--strip`` is set to ``sometimes`` and ``--compilation_mode``
+is ``fastbuild``.
+Additionally, it tests that stack traces still contain the same information when stripping
+is enabled.
diff --git a/tests/core/strip/strip_test.go b/tests/core/strip/strip_test.go
new file mode 100644
index 00000000..b7183033
--- /dev/null
+++ b/tests/core/strip/strip_test.go
@@ -0,0 +1,270 @@
+// Copyright 2019 The Bazel Authors. All rights reserved.
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+//    http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package strip_test
+
+import (
+	"bytes"
+	"errors"
+	"fmt"
+	"os/exec"
+	"strings"
+	"testing"
+
+	"github.com/bazelbuild/rules_go/go/tools/bazel_testing"
+)
+
+func TestMain(m *testing.M) {
+	bazel_testing.TestMain(m, bazel_testing.Args{
+		Main: `
+-- BUILD.bazel --
+load("@io_bazel_rules_go//go:def.bzl", "go_binary")
+
+go_binary(
+    name = "strip",
+    srcs = ["strip.go"],
+)
+-- strip.go --
+package main
+
+import (
+	"debug/elf"
+	"debug/macho"
+	"debug/pe"
+	"errors"
+	"flag"
+	"fmt"
+	"io"
+	"os"
+	"regexp"
+	"runtime"
+	"runtime/debug"
+	"strings"
+)
+
+var wantStrip = flag.Bool("wantstrip", false, "")
+
+func main() {
+	flag.Parse()
+	stackTrace, err := panicAndRecover()
+	if err != nil {
+		panic(err)
+	}
+	gotStackTrace := strings.Split(stackTrace, "\n")
+	if len(gotStackTrace) != len(wantStackTrace) {
+		panic(fmt.Sprintf("got %d lines of stack trace, want %d", len(gotStackTrace), len(wantStackTrace)))
+	}
+	for i := range gotStackTrace {
+		expectedLine := regexp.MustCompile(wantStackTrace[i])
+		if !expectedLine.MatchString(gotStackTrace[i]) {
+			panic(fmt.Sprintf("got unexpected stack trace line %q at index %d", gotStackTrace[i], i))
+		}
+	}
+	stripped, err := isStripped()
+	if err != nil {
+		panic(err)
+	}
+	if stripped != *wantStrip {
+		panic(fmt.Sprintf("got stripped=%t, want stripped=%t", stripped, *wantStrip))
+	}
+}
+
+func panicAndRecover() (stackTrace string, err error) {
+	defer func() {
+		if r := recover(); r != nil {
+			stackTrace = string(debug.Stack())
+		}
+	}()
+	panic("test")
+	return "", errors.New("should not reach here")
+}
+
+func isStripped() (bool, error) {
+	ownLocation, err := os.Executable()
+	if err != nil {
+		return false, err
+	}
+	ownBinary, err := os.Open(ownLocation)
+	if err != nil {
+		return false, err
+	}
+	defer ownBinary.Close()
+	switch runtime.GOOS {
+	case "darwin":
+		return isStrippedMachO(ownBinary)
+	case "linux":
+		return isStrippedElf(ownBinary)
+	case "windows":
+		return isStrippedPE(ownBinary)
+	default:
+		return false, fmt.Errorf("unsupported OS: %s", runtime.GOOS)
+	}
+}
+
+func isStrippedMachO(f io.ReaderAt) (bool, error) {
+	macho, err := macho.NewFile(f)
+	if err != nil {
+		return false, err
+	}
+	gotDwarf := macho.Segment("__DWARF") != nil
+	gotDebugInfo := macho.Section("__zdebug_info") != nil
+	if gotDwarf != gotDebugInfo {
+		return false, fmt.Errorf("inconsistent stripping: gotDwarf=%v, gotDebugInfo=%v", gotDwarf, gotDebugInfo)
+	}
+	return !gotDwarf, nil
+}
+
+func isStrippedElf(f io.ReaderAt) (bool, error) {
+	elf, err := elf.NewFile(f)
+	if err != nil {
+		return false, err
+	}
+	var gotSymtab bool
+	for _, section := range elf.Sections {
+		if section.Name == ".symtab" {
+			gotSymtab = true
+			break
+		}
+	}
+	return !gotSymtab, nil
+}
+
+
+func isStrippedPE(f io.ReaderAt) (bool, error) {
+	pe, err := pe.NewFile(f)
+	if err != nil {
+		return false, err
+	}
+	symtab := pe.Section(".symtab")
+	if symtab == nil {
+		return false, fmt.Errorf("no .symtab section")
+	}
+	emptySymtab := (symtab.VirtualSize <= 4) && (symtab.Size <= 512)
+	return emptySymtab, nil
+}
+
+
+` + embedWantedStackTraces(),
+	})
+}
+
+func Test(t *testing.T) {
+	for _, test := range []struct {
+		desc, stripFlag, compilationMode string
+		wantStrip                        bool
+	}{
+		{
+			desc:      "run_auto",
+			wantStrip: true,
+		},
+		{
+			desc:            "run_fastbuild",
+			compilationMode: "fastbuild",
+			wantStrip:       true,
+		},
+		{
+			desc:            "run_dbg",
+			compilationMode: "dbg",
+		},
+		{
+			desc:            "run_opt",
+			compilationMode: "opt",
+		},
+		{
+			desc:      "run_always",
+			stripFlag: "always",
+			wantStrip: true,
+		},
+		{
+			desc:            "run_always_opt",
+			stripFlag:       "always",
+			compilationMode: "opt",
+			wantStrip:       true,
+		},
+		{
+			desc:      "run_never",
+			stripFlag: "never",
+		},
+		{
+			desc:            "run_sometimes_fastbuild",
+			stripFlag:       "sometimes",
+			compilationMode: "fastbuild",
+			wantStrip:       true,
+		},
+		{
+			desc:            "run_sometimes_dbg",
+			stripFlag:       "sometimes",
+			compilationMode: "dbg",
+		},
+		{
+			desc:            "run_sometimes_opt",
+			stripFlag:       "sometimes",
+			compilationMode: "opt",
+		},
+	} {
+		t.Run(test.desc, func(t *testing.T) {
+			args := []string{"run"}
+			if len(test.stripFlag) > 0 {
+				args = append(args, "--strip", test.stripFlag)
+			}
+			if len(test.compilationMode) > 0 {
+				args = append(args, "--compilation_mode", test.compilationMode)
+			}
+			args = append(args, "//:strip", "--", fmt.Sprintf("-wantstrip=%v", test.wantStrip))
+			cmd := bazel_testing.BazelCmd(args...)
+			stderr := &bytes.Buffer{}
+			cmd.Stderr = stderr
+			t.Logf("running: bazel %s", strings.Join(args, " "))
+			if err := cmd.Run(); err != nil {
+				var xerr *exec.ExitError
+				if !errors.As(err, &xerr) {
+					t.Fatalf("unexpected error: %v", err)
+				}
+				if xerr.ExitCode() == bazel_testing.BUILD_FAILURE {
+					t.Fatalf("unexpected build failure: %v\nstderr:\n%s", err, stderr.Bytes())
+					return
+				} else if xerr.ExitCode() == bazel_testing.TESTS_FAILED {
+					t.Fatalf("error running %s:\n%s", strings.Join(cmd.Args, " "), stderr.Bytes())
+				} else {
+					t.Fatalf("unexpected error: %v\nstderr:\n%s", err, stderr.Bytes())
+				}
+			}
+		})
+	}
+}
+
+var wantStackTrace = []string{
+	`^goroutine \d+ \[running\]:$`,
+	`^runtime/debug\.Stack\(\)$`,
+	`^	GOROOT/src/runtime/debug/stack\.go:\d+ \+0x[0-9a-f]+$`,
+	`^main\.panicAndRecover\.func1\(\)$`,
+	`^	strip\.go:\d+ \+0x[0-9a-f]+$`,
+	`^panic\({0x[0-9a-f]+, 0x[0-9a-f]+}\)$`,
+	`^	GOROOT/src/runtime/panic\.go:\d+ \+0x[0-9a-f]+$`,
+	`^main\.panicAndRecover\(\)$`,
+	`^	strip\.go:\d+ \+0x[0-9a-f]+$`,
+	`^main\.main\(\)$`,
+	`^	strip\.go:\d+ \+0x[0-9a-f]+$`,
+	`^$`,
+}
+
+func embedWantedStackTraces() string {
+	buf := &bytes.Buffer{}
+	fmt.Fprintln(buf, "var wantStackTrace = []string{")
+	for _, s := range wantStackTrace {
+		fmt.Fprintf(buf, "`%s`,\n", s)
+	}
+	fmt.Fprintln(buf, "}")
+	return buf.String()
+}