Don't require supports_fission to be set in the crosstool

Having a 'per_object_debug_data' feature enabled is giving the same signal as having supports_fission enabled. This is a safe change because all crosstools that have supports_fission: true have per_object_debug_info disabled, and with the legacy crosstool fields migration we will migrate this feature to be enabled by default for these crosstools. #6861 #5883 RELNOTES: None. PiperOrigin-RevId: 230701029
bazelbuild · Jan 24, 2019 · 0be35de · 0be35de
1 parent b2cb640
commit 0be35de
Show file tree

Hide file tree

Showing 4 changed files with 70 additions and 10 deletions.
diff --git a/src/main/java/com/google/devtools/build/lib/rules/cpp/CcToolchainProvider.java b/src/main/java/com/google/devtools/build/lib/rules/cpp/CcToolchainProvider.java
@@ -315,6 +315,15 @@ public boolean useFission() {
         && supportsFission();
   }
 
+  /**
+   * Returns true if PER_OBJECT_DEBUG_INFO are specified and supported by the CROSSTOOL for the
+   * build implied by the given configuration, toolchain and feature configuration.
+   */
+  public boolean shouldCreatePerObjectDebugInfo(FeatureConfiguration featureConfiguration) {
+    return cppConfiguration.fissionIsActiveForCurrentCompilationMode()
+        && featureConfiguration.isEnabled(CppRuleClasses.PER_OBJECT_DEBUG_INFO);
+  }
+
   /** Whether the toolchains supports header parsing. */
   public boolean supportsHeaderParsing() {
     return supportsHeaderParsing;
@@ -334,14 +343,6 @@ public boolean shouldProcessHeaders(FeatureConfiguration featureConfiguration) {
         && featureConfiguration.isEnabled(CppRuleClasses.PARSE_HEADERS);
   }
 
-  /**
-   * Returns true if Fission and PER_OBJECT_DEBUG_INFO are specified and supported by the CROSSTOOL
-   * for the build implied by the given configuration, toolchain and feature configuration.
-   */
-  public boolean shouldCreatePerObjectDebugInfo(FeatureConfiguration featureConfiguration) {
-    return useFission() && featureConfiguration.isEnabled(CppRuleClasses.PER_OBJECT_DEBUG_INFO);
-  }
-
   @Override
   public void addGlobalMakeVariables(ImmutableMap.Builder<String, String> globalMakeEnvBuilder) {
     globalMakeEnvBuilder.putAll(

diff --git a/src/test/java/com/google/devtools/build/lib/packages/util/MockCcSupport.java b/src/test/java/com/google/devtools/build/lib/packages/util/MockCcSupport.java
@@ -389,15 +389,16 @@ public boolean apply(Artifact artifact) {
       ""
           + "feature { "
           + "  name: 'per_object_debug_info'"
+          + "  enabled: true"
           + "  flag_set {"
           + "    action: 'c-compile'"
           + "    action: 'c++-compile'"
           + "    action: 'assemble'"
           + "    action: 'preprocess-assemble'"
           + "    action: 'c++-module-codegen'"
           + "    action: 'lto-backend'"
-          + "    expand_if_all_available: 'per_object_debug_info_file'"
           + "    flag_group {"
+          + "      expand_if_all_available: 'per_object_debug_info_file'"
           + "      flag: 'per_object_debug_info_option'"
           + "    }"
           + "  }"

diff --git a/src/test/java/com/google/devtools/build/lib/rules/cpp/CompileBuildVariablesTest.java b/src/test/java/com/google/devtools/build/lib/rules/cpp/CompileBuildVariablesTest.java
@@ -22,6 +22,7 @@
 import com.google.devtools.build.lib.actions.Artifact;
 import com.google.devtools.build.lib.analysis.util.AnalysisMock;
 import com.google.devtools.build.lib.analysis.util.BuildViewTestCase;
+import com.google.devtools.build.lib.packages.util.MockCcSupport;
 import org.junit.Test;
 import org.junit.runner.RunWith;
 import org.junit.runners.JUnit4;
@@ -166,6 +167,40 @@ public void testPresenceOfSysrootBuildVariable() throws Exception {
         .isEqualTo("/usr/local/custom-sysroot");
   }
 
+  @Test
+  public void testPresenceOfPerObjectDebugFileBuildVariable() throws Exception {
+    AnalysisMock.get()
+        .ccSupport()
+        .setupCrosstool(mockToolsConfig, MockCcSupport.PER_OBJECT_DEBUG_INFO_CONFIGURATION);
+    useConfiguration("--fission=yes");
+
+    scratch.file("x/BUILD", "cc_binary(name = 'bin', srcs = ['bin.cc'])");
+    scratch.file("x/bin.cc");
+
+    CcToolchainVariables variables = getCompileBuildVariables("//x:bin", "bin");
+
+    assertThat(
+            variables.getStringVariable(
+                CompileBuildVariables.PER_OBJECT_DEBUG_INFO_FILE.getVariableName()))
+        .isNotNull();
+  }
+
+  @Test
+  public void testPresenceOfPerObjectDebugFileBuildVariableUsingLegacyFields() throws Exception {
+    AnalysisMock.get().ccSupport().setupCrosstool(mockToolsConfig, "supports_fission: true");
+    useConfiguration("--fission=yes");
+
+    scratch.file("x/BUILD", "cc_binary(name = 'bin', srcs = ['bin.cc'])");
+    scratch.file("x/bin.cc");
+
+    CcToolchainVariables variables = getCompileBuildVariables("//x:bin", "bin");
+
+    assertThat(
+            variables.getStringVariable(
+                CompileBuildVariables.PER_OBJECT_DEBUG_INFO_FILE.getVariableName()))
+        .isNotNull();
+  }
+
   @Test
   public void testPresenceOfMinOsVersionBuildVariable() throws Exception {
     AnalysisMock.get()

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
@@ -388,7 +388,7 @@ private void assertStripBinaryVariableIsPresent(
   }
 
   @Test
-  public void testIsUsingFissionVariable() throws Exception {
+  public void testIsUsingFissionVariableUsingLegacyFields() throws Exception {
     scratch.file("x/BUILD",
         "cc_binary(name = 'foo', srcs = ['foo.cc'])");
     scratch.file("x/foo.cc");
@@ -411,6 +411,29 @@ public void testIsUsingFissionVariable() throws Exception {
         .isTrue();
   }
 
+  @Test
+  public void testIsUsingFissionVariable() throws Exception {
+    scratch.file("x/BUILD", "cc_binary(name = 'foo', srcs = ['foo.cc'])");
+    scratch.file("x/foo.cc");
+
+    AnalysisMock.get()
+        .ccSupport()
+        .setupCrosstool(mockToolsConfig, MockCcSupport.PER_OBJECT_DEBUG_INFO_CONFIGURATION);
+
+    useConfiguration("--fission=no");
+    ConfiguredTarget target = getConfiguredTarget("//x:foo");
+    CcToolchainVariables variables = getLinkBuildVariables(target, LinkTargetType.EXECUTABLE);
+    assertThat(variables.isAvailable(LinkBuildVariables.IS_USING_FISSION.getVariableName()))
+        .isFalse();
+
+    useConfiguration("--fission=yes");
+    ConfiguredTarget fissionTarget = getConfiguredTarget("//x:foo");
+    CcToolchainVariables fissionVariables =
+        getLinkBuildVariables(fissionTarget, LinkTargetType.EXECUTABLE);
+    assertThat(fissionVariables.isAvailable(LinkBuildVariables.IS_USING_FISSION.getVariableName()))
+        .isTrue();
+  }
+
   @Test
   public void testSysrootVariable() throws Exception {
     AnalysisMock.get()