Introduce strip_debug_symbols feature to crosstool

This cl removes hard coded -Wl,-S flag from Blaze and moves it to the
crosstool.

--
PiperOrigin-RevId: 149525225
MOS_MIGRATED_REVID=149525225

6 files changed, 74 insertions, 18 deletions

diff --git a/src/main/java/com/google/devtools/build/lib/rules/cpp/CppConfiguration.java b/src/main/java/com/google/devtools/build/lib/rules/cpp/CppConfiguration.java
index 8ce165fe3b..342ad72809 100644
--- a/src/main/java/com/google/devtools/build/lib/rules/cpp/CppConfiguration.java
+++ b/src/main/java/com/google/devtools/build/lib/rules/cpp/CppConfiguration.java
@@ -1115,10 +1115,6 @@ public class CppConfiguration extends BuildConfiguration.Fragment {
     List<String> result = new ArrayList<>();
     result.addAll(commonLinkOptions);
 
-    if (stripBinaries) {
-      result.add("-Wl,-S");
-    }
-
     result.addAll(linkOptionsFromCompilationMode.get(compilationMode));
     result.addAll(linkOptionsFromLipoMode.get(lipoMode));
     result.addAll(linkOptionsFromLinkingMode.get(linkingMode));
diff --git a/src/main/java/com/google/devtools/build/lib/rules/cpp/CppLinkActionBuilder.java b/src/main/java/com/google/devtools/build/lib/rules/cpp/CppLinkActionBuilder.java
index f8027391ae..9af79a77a4 100644
--- a/src/main/java/com/google/devtools/build/lib/rules/cpp/CppLinkActionBuilder.java
+++ b/src/main/java/com/google/devtools/build/lib/rules/cpp/CppLinkActionBuilder.java
@@ -124,6 +124,9 @@ public class CppLinkActionBuilder {
   /** A build variable whose presence indicates that PIC code should be generated. */
   public static final String FORCE_PIC_VARIABLE = "force_pic";
 
+  /** A build variable whose presence indicates that the debug symbols should be stripped. */
+  public static final String STRIP_DEBUG_SYMBOLS_VARIABLE = "strip_debug_symbols";
+
   /** A build variable whose presence indicates that this action is a cc_test linking action. */
   public static final String IS_CC_TEST_LINK_ACTION_VARIABLE = "is_cc_test_link_action";
 
@@ -1313,6 +1316,10 @@ public class CppLinkActionBuilder {
         buildVariables.addStringVariable(FORCE_PIC_VARIABLE, "");
       }
 
+      if (cppConfiguration.shouldStripBinaries()) {
+        buildVariables.addStringVariable(STRIP_DEBUG_SYMBOLS_VARIABLE, "");
+      }
+
       if (useTestOnlyFlags()) {
         buildVariables.addStringVariable(IS_CC_TEST_LINK_ACTION_VARIABLE, "");
       } else {
diff --git a/src/main/java/com/google/devtools/build/lib/rules/cpp/CppLinkActionConfigs.java b/src/main/java/com/google/devtools/build/lib/rules/cpp/CppLinkActionConfigs.java
index 2cb39abb86..14a053bb50 100644
--- a/src/main/java/com/google/devtools/build/lib/rules/cpp/CppLinkActionConfigs.java
+++ b/src/main/java/com/google/devtools/build/lib/rules/cpp/CppLinkActionConfigs.java
@@ -63,6 +63,7 @@ public class CppLinkActionConfigs {
                 "       tool_path: 'DUMMY_TOOL'",
                 "   }",
                 "   implies: 'symbol_counts'",
+                "   implies: 'strip_debug_symbols'",
                 "   implies: 'linkstamps'",
                 "   implies: 'output_execpath_flags_executable'",
                 "   implies: 'runtime_library_search_directories'",
@@ -81,6 +82,7 @@ public class CppLinkActionConfigs {
                 "   implies: 'build_interface_libraries'",
                 "   implies: 'dynamic_library_linker_tool'",
                 "   implies: 'symbol_counts'",
+                "   implies: 'strip_debug_symbols'",
                 "   implies: 'shared_flag'",
                 "   implies: 'linkstamps'",
                 "   implies: 'output_execpath_flags'",
@@ -96,6 +98,7 @@ public class CppLinkActionConfigs {
                 "   tool {",
                 "       tool_path: 'DUMMY_TOOL'",
                 "   }",
+                "   implies: 'strip_debug_symbols'",
                 "   implies: 'runtime_library_search_directories'",
                 "   implies: 'library_search_directories'",
                 "   implies: 'libraries_to_link'",
@@ -107,6 +110,7 @@ public class CppLinkActionConfigs {
                 "   tool {",
                 "       tool_path: 'DUMMY_TOOL'",
                 "   }",
+                "   implies: 'strip_debug_symbols'",
                 "   implies: 'runtime_library_search_directories'",
                 "   implies: 'library_search_directories'",
                 "   implies: 'libraries_to_link'",
@@ -118,6 +122,7 @@ public class CppLinkActionConfigs {
                 "   tool {",
                 "       tool_path: 'DUMMY_TOOL'",
                 "   }",
+                "   implies: 'strip_debug_symbols'",
                 "   implies: 'runtime_library_search_directories'",
                 "   implies: 'library_search_directories'",
                 "   implies: 'libraries_to_link'",
@@ -129,6 +134,7 @@ public class CppLinkActionConfigs {
                 "   tool {",
                 "       tool_path: 'DUMMY_TOOL'",
                 "   }",
+                "   implies: 'strip_debug_symbols'",
                 "   implies: 'runtime_library_search_directories'",
                 "   implies: 'library_search_directories'",
                 "   implies: 'libraries_to_link'",
@@ -444,6 +450,18 @@ public class CppLinkActionConfigs {
                 "   }",
                 "}",
                 "feature {",
+                "   name: 'strip_debug_symbols'",
+                "   flag_set {",
+                "       action: 'c++-link-executable'",
+                "       action: 'c++-link-dynamic-library'",
+                "       action: 'c++-link-interface-dynamic-library'",
+                "       flag_group {",
+                "           expand_if_all_available: 'strip_debug_symbols'",
+                "           flag: '-Wl,-S'",
+                "       }",
+                "   }",
+                "}",
+                "feature {",
                 "   name: 'linker_param_file'",
                 "   flag_set {",
                 "       expand_if_all_available: 'linker_param_file'",
diff --git a/src/test/java/com/google/devtools/build/lib/rules/cpp/CrosstoolConfigurationLoaderTest.java b/src/test/java/com/google/devtools/build/lib/rules/cpp/CrosstoolConfigurationLoaderTest.java
index 73c011956c..fce9586918 100644
--- a/src/test/java/com/google/devtools/build/lib/rules/cpp/CrosstoolConfigurationLoaderTest.java
+++ b/src/test/java/com/google/devtools/build/lib/rules/cpp/CrosstoolConfigurationLoaderTest.java
@@ -209,16 +209,16 @@ public class CrosstoolConfigurationLoaderTest extends AnalysisTestCase {
 
     assertEquals(Arrays.<String>asList(), toolchain.getLinkOptions());
     assertEquals(
-        Arrays.asList("linker", "-Wl,-S", "linker-fastbuild", "fully static"),
+        Arrays.asList("linker", "linker-fastbuild", "fully static"),
         toolchain.getFullyStaticLinkOptions(NO_FEATURES, false));
     assertEquals(
-        Arrays.asList("linker", "-Wl,-S", "linker-fastbuild", "dynamic"),
+        Arrays.asList("linker", "linker-fastbuild", "dynamic"),
         toolchain.getDynamicLinkOptions(NO_FEATURES, false));
     assertEquals(
-        Arrays.asList("linker", "-Wl,-S", "linker-fastbuild", "mostly static", "solinker"),
+        Arrays.asList("linker", "linker-fastbuild", "mostly static", "solinker"),
         toolchain.getFullyStaticLinkOptions(NO_FEATURES, true));
     assertEquals(
-        Arrays.asList("linker", "-Wl,-S", "linker-fastbuild", "dynamic", "solinker"),
+        Arrays.asList("linker", "linker-fastbuild", "dynamic", "solinker"),
         toolchain.getDynamicLinkOptions(NO_FEATURES, true));
 
     assertEquals(Arrays.asList("objcopy"), toolchain.getObjCopyOptionsForEmbedding());
@@ -514,7 +514,6 @@ public class CrosstoolConfigurationLoaderTest extends AnalysisTestCase {
         Arrays.asList(
             "linker-flag-A-1",
             "linker-flag-A-2",
-            "-Wl,-S",
             "linker-fastbuild-flag-A-1",
             "linker-fastbuild-flag-A-2",
             "solinker-flag-A-1",
@@ -527,7 +526,6 @@ public class CrosstoolConfigurationLoaderTest extends AnalysisTestCase {
         Arrays.asList(
             "linker-flag-A-1",
             "linker-flag-A-2",
-            "-Wl,-S",
             "linker-fastbuild-flag-A-1",
             "linker-fastbuild-flag-A-2",
             "fully-static-flag-A-1",
@@ -541,7 +539,6 @@ public class CrosstoolConfigurationLoaderTest extends AnalysisTestCase {
         Arrays.asList(
             "linker-flag-A-1",
             "linker-flag-A-2",
-            "-Wl,-S",
             "linker-dbg-flag-A-1",
             "linker-dbg-flag-A-2"),
         toolchainA.configureLinkerOptions(
@@ -553,7 +550,6 @@ public class CrosstoolConfigurationLoaderTest extends AnalysisTestCase {
         Arrays.asList(
             "linker-flag-A-1",
             "linker-flag-A-2",
-            "-Wl,-S",
             "fully-static-flag-A-1",
             "fully-static-flag-A-2"),
         toolchainA.configureLinkerOptions(
@@ -566,7 +562,6 @@ public class CrosstoolConfigurationLoaderTest extends AnalysisTestCase {
         Arrays.asList(
             "linker-flag-A-1",
             "linker-flag-A-2",
-            "-Wl,-S",
             "fully-static-flag-A-1",
             "fully-static-flag-A-2"),
         toolchainA.configureLinkerOptions(
@@ -628,23 +623,23 @@ public class CrosstoolConfigurationLoaderTest extends AnalysisTestCase {
     assertThat(toolchainC.getCOptions()).isEmpty();
     assertThat(toolchainC.getCxxOptions(NO_FEATURES)).isEmpty();
     assertThat(toolchainC.getUnfilteredCompilerOptions(NO_FEATURES)).isEmpty();
-    assertEquals(Arrays.asList("-Wl,-S"), toolchainC.getDynamicLinkOptions(NO_FEATURES, true));
+    assertEquals(Collections.EMPTY_LIST, toolchainC.getDynamicLinkOptions(NO_FEATURES, true));
     assertEquals(
-        Arrays.asList("-Wl,-S"),
+        Collections.EMPTY_LIST,
         toolchainC.configureLinkerOptions(
             CompilationMode.FASTBUILD,
             LipoMode.OFF,
             LinkingMode.FULLY_STATIC,
             new PathFragment("hello-world/ld")));
     assertEquals(
-        Arrays.asList("-Wl,-S"),
+        Collections.EMPTY_LIST,
         toolchainC.configureLinkerOptions(
             CompilationMode.DBG,
             LipoMode.OFF,
             LinkingMode.DYNAMIC,
             new PathFragment("hello-world/ld")));
     assertEquals(
-        Arrays.asList("-Wl,-S"),
+        Collections.EMPTY_LIST,
         toolchainC.configureLinkerOptions(
             CompilationMode.OPT,
             LipoMode.OFF,
@@ -682,7 +677,6 @@ public class CrosstoolConfigurationLoaderTest extends AnalysisTestCase {
         Arrays.asList(
             "linker-flag-B-1",
             "linker-flag-B-2",
-            "-Wl,-S",
             "linker-dbg-flag-B-1",
             "linker-dbg-flag-B-2",
             "linker-lipo_" + lipoSuffix),
diff --git a/src/test/java/com/google/devtools/build/lib/rules/cpp/LinkBuildVariablesTest.java b/src/test/java/com/google/devtools/build/lib/rules/cpp/LinkBuildVariablesTest.java
index 6ea0a1342d..ba5ef20507 100644
--- a/src/test/java/com/google/devtools/build/lib/rules/cpp/LinkBuildVariablesTest.java
+++ b/src/test/java/com/google/devtools/build/lib/rules/cpp/LinkBuildVariablesTest.java
@@ -266,4 +266,29 @@ public class LinkBuildVariablesTest extends BuildViewTestCase {
     assertThat(binaryVariables.isAvailable(CppLinkActionBuilder.IS_CC_TEST_LINK_ACTION_VARIABLE))
         .isFalse();
   }
+
+  @Test
+  public void testStripBinariesIsEnabledWhenStripModeIsAlwaysNoMatterWhat() throws Exception {
+    scratch.file("x/BUILD", "cc_binary(name = 'foo', srcs = ['a.cc'])");
+    scratch.file("x/a.cc");
+
+    assertStripBinaryVariableIsPresent("always", "opt", true);
+    assertStripBinaryVariableIsPresent("always", "fastbuild", true);
+    assertStripBinaryVariableIsPresent("always", "dbg", true);
+    assertStripBinaryVariableIsPresent("sometimes", "opt", false);
+    assertStripBinaryVariableIsPresent("sometimes", "fastbuild", true);
+    assertStripBinaryVariableIsPresent("sometimes", "dbg", false);
+    assertStripBinaryVariableIsPresent("never", "opt", false);
+    assertStripBinaryVariableIsPresent("never", "fastbuild", false);
+    assertStripBinaryVariableIsPresent("never", "dbg", false);
+  }
+
+  private void assertStripBinaryVariableIsPresent(
+      String stripMode, String compilationMode, boolean isEnabled) throws Exception {
+    useConfiguration("--strip=" + stripMode, "--compilation_mode=" + compilationMode);
+    ConfiguredTarget target = getConfiguredTarget("//x:foo");
+    Variables variables = getLinkBuildVariables(target, LinkTargetType.EXECUTABLE);
+    assertThat(variables.isAvailable(CppLinkActionBuilder.STRIP_DEBUG_SYMBOLS_VARIABLE))
+        .isEqualTo(isEnabled);
+  }
 }
diff --git a/tools/cpp/CROSSTOOL.tpl b/tools/cpp/CROSSTOOL.tpl
index 5b382926a3..1a18ebe549 100644
--- a/tools/cpp/CROSSTOOL.tpl
+++ b/tools/cpp/CROSSTOOL.tpl
@@ -337,6 +337,7 @@ toolchain {
      tool {
          tool_path: 'wrapper/bin/msvc_link.bat'
      }
+     implies: 'strip_debug_symbols'
      implies: 'linkstamps'
      implies: 'output_execpath_flags'
      implies: 'input_param_flags'
@@ -350,6 +351,7 @@ toolchain {
      tool {
          tool_path: 'wrapper/bin/msvc_link.bat'
      }
+     implies: 'strip_debug_symbols'
      implies: 'shared_flag'
      implies: 'linkstamps'
      implies: 'output_execpath_flags'
@@ -407,6 +409,7 @@ toolchain {
     tool {
       tool_path: 'wrapper/bin/msvc_link.bat'
     }
+    implies: 'strip_debug_symbols'
     implies: 'linker_param_file'
   }
 
@@ -415,6 +418,19 @@ toolchain {
   }
 
   feature {
+    name: 'strip_debug_symbols'
+    flag_set {
+      action: 'c++-link-executable'
+      action: 'c++-link-dynamic-library'
+      action: 'c++-link-interface-dynamic-library'
+      flag_group {
+        expand_if_all_available: 'strip_debug_symbols'
+        flag: '-Wl,-S'
+      }
+    }
+  }
+
+  feature {
      name: 'shared_flag'
      flag_set {
          action: 'c++-link-dynamic-library'