Do not use CppConfiguration in CcToolchainProvider internals

This cl removes further uses of CppConfiguration, this time internal to CcToolchainProvider. Progress towards #6516. RELNOTES: None. PiperOrigin-RevId: 240209998
bazelbuild · Mar 25, 2019 · 7363e84 · 7363e84
1 parent 4712959
commit 7363e84
Show file tree

Hide file tree

Showing 10 changed files with 56 additions and 81 deletions.
diff --git a/src/main/java/com/google/devtools/build/lib/rules/cpp/CcBinary.java b/src/main/java/com/google/devtools/build/lib/rules/cpp/CcBinary.java
@@ -518,7 +518,7 @@ public static ConfiguredTarget init(CppSemantics semantics, RuleContext ruleCont
             ruleContext,
             ccCompilationOutputs,
             linkingMode,
-            ccToolchain.shouldCreatePerObjectDebugInfo(featureConfiguration),
+            ccToolchain.shouldCreatePerObjectDebugInfo(featureConfiguration, cppConfiguration),
             usePic,
             ccLinkingOutputsBinary.getAllLtoArtifacts());
     Artifact dwpFile =
@@ -528,7 +528,7 @@ public static ConfiguredTarget init(CppSemantics semantics, RuleContext ruleCont
 
     // The debug package should include the dwp file only if it was explicitly requested.
     Artifact explicitDwpFile = dwpFile;
-    if (!ccToolchain.shouldCreatePerObjectDebugInfo(featureConfiguration)) {
+    if (!ccToolchain.shouldCreatePerObjectDebugInfo(featureConfiguration, cppConfiguration)) {
       explicitDwpFile = null;
     } else {
       // For cc_test rules, include the dwp in the runfiles if Fission is enabled and the test was

diff --git a/src/main/java/com/google/devtools/build/lib/rules/cpp/CcCommon.java b/src/main/java/com/google/devtools/build/lib/rules/cpp/CcCommon.java
@@ -857,19 +857,19 @@ public static FeatureConfiguration configureFeaturesOrThrowEvalException(
     // according to compilation mode.
     if (requestedFeatures.contains(CppRuleClasses.STATIC_LINK_MSVCRT)) {
       allRequestedFeaturesBuilder.add(
-          toolchain.getCompilationMode() == CompilationMode.DBG
+          cppConfiguration.getCompilationMode() == CompilationMode.DBG
               ? CppRuleClasses.STATIC_LINK_MSVCRT_DEBUG
               : CppRuleClasses.STATIC_LINK_MSVCRT_NO_DEBUG);
     } else {
       allRequestedFeaturesBuilder.add(
-          toolchain.getCompilationMode() == CompilationMode.DBG
+          cppConfiguration.getCompilationMode() == CompilationMode.DBG
               ? CppRuleClasses.DYNAMIC_LINK_MSVCRT_DEBUG
               : CppRuleClasses.DYNAMIC_LINK_MSVCRT_NO_DEBUG);
     }
 
     ImmutableList.Builder<String> allFeatures =
         new ImmutableList.Builder<String>()
-            .addAll(ImmutableSet.of(toolchain.getCompilationMode().toString()))
+            .addAll(ImmutableSet.of(cppConfiguration.getCompilationMode().toString()))
             .addAll(
                 cppConfiguration.disableLegacyCrosstoolFields()
                     ? ImmutableList.of()
@@ -886,7 +886,9 @@ public static FeatureConfiguration configureFeaturesOrThrowEvalException(
       }
     }
 
-    if (toolchain.useFission() && !cppConfiguration.disableLegacyCrosstoolFields()) {
+    if (cppConfiguration.fissionIsActiveForCurrentCompilationMode()
+        && toolchain.supportsFission()
+        && !cppConfiguration.disableLegacyCrosstoolFields()) {
       allFeatures.add(CppRuleClasses.PER_OBJECT_DEBUG_INFO);
     }
 
@@ -898,7 +900,7 @@ public static FeatureConfiguration configureFeaturesOrThrowEvalException(
     }
 
     FdoContext.BranchFdoProfile branchFdoProvider = toolchain.getFdoContext().getBranchFdoProfile();
-    if (branchFdoProvider != null && toolchain.getCompilationMode() == CompilationMode.OPT) {
+    if (branchFdoProvider != null && cppConfiguration.getCompilationMode() == CompilationMode.OPT) {
       if (branchFdoProvider.isLlvmFdo()
           && !allUnsupportedFeatures.contains(CppRuleClasses.FDO_OPTIMIZE)) {
         allFeatures.add(CppRuleClasses.FDO_OPTIMIZE);

diff --git a/src/main/java/com/google/devtools/build/lib/rules/cpp/CcCompilationHelper.java b/src/main/java/com/google/devtools/build/lib/rules/cpp/CcCompilationHelper.java
@@ -414,7 +414,8 @@ private CcCompilationHelper addPrivateHeader(Artifact privateHeader, Label label
         CppFileTypes.CPP_TEXTUAL_INCLUDE.matches(privateHeader.getExecPath());
     Preconditions.checkState(isHeader || isTextualInclude);
 
-    if (ccToolchain.shouldProcessHeaders(featureConfiguration) && !isTextualInclude) {
+    if (ccToolchain.shouldProcessHeaders(featureConfiguration, cppConfiguration)
+        && !isTextualInclude) {
       compilationUnitSources.put(
           privateHeader, CppSource.create(privateHeader, label, CppSource.Type.HEADER));
     }
@@ -470,7 +471,9 @@ private void addHeader(Artifact header, Label label) {
         CppFileTypes.CPP_HEADER.matches(header.getExecPath()) || header.isTreeArtifact();
     boolean isTextualInclude = CppFileTypes.CPP_TEXTUAL_INCLUDE.matches(header.getExecPath());
     publicHeaders.add(header);
-    if (isTextualInclude || !isHeader || !ccToolchain.shouldProcessHeaders(featureConfiguration)) {
+    if (isTextualInclude
+        || !isHeader
+        || !ccToolchain.shouldProcessHeaders(featureConfiguration, cppConfiguration)) {
       return;
     }
 
@@ -1284,7 +1287,7 @@ private CcCompilationOutputs createCcCompileActions() throws RuleErrorException
                 // The source action does not generate dwo when it has bitcode
                 // output (since it isn't generating a native object with debug
                 // info). In that case the LtoBackendAction will generate the dwo.
-                ccToolchain.shouldCreatePerObjectDebugInfo(featureConfiguration),
+                ccToolchain.shouldCreatePerObjectDebugInfo(featureConfiguration, cppConfiguration),
                 bitcodeOutput,
                 isGenerateDotdFile(sourceArtifact));
             break;
@@ -1514,7 +1517,8 @@ private void createModuleCodegenAction(
                 actionConstructionContext, label, gcnoFileName, configuration)
             : null;
 
-    boolean generateDwo = ccToolchain.shouldCreatePerObjectDebugInfo(featureConfiguration);
+    boolean generateDwo =
+        ccToolchain.shouldCreatePerObjectDebugInfo(featureConfiguration, cppConfiguration);
     Artifact dwoFile = generateDwo ? getDwoFile(builder.getOutputFile()) : null;
     // TODO(tejohnson): Add support for ThinLTO if needed.
     boolean bitcodeOutput =

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
@@ -123,8 +123,6 @@ public final class CcToolchainProvider extends ToolchainInfo
   private final boolean useLLVMCoverageMapFormat;
   private final boolean codeCoverageEnabled;
   private final boolean isHostConfiguration;
-  private final boolean forcePic;
-  private final boolean shouldStripBinaries;
   /**
    * WARNING: We don't like {@link FdoContext}. Its {@link FdoContext#fdoProfilePath} is pure path
    * and that is horrible as it breaks many Bazel assumptions! Don't do bad stuff with it, don't
@@ -210,13 +208,6 @@ public CcToolchainProvider(
     this.useLLVMCoverageMapFormat = useLLVMCoverageMapFormat;
     this.codeCoverageEnabled = codeCoverageEnabled;
     this.isHostConfiguration = isHostConfiguration;
-    if (cppConfiguration != null) {
-      this.forcePic = cppConfiguration.forcePic();
-      this.shouldStripBinaries = cppConfiguration.shouldStripBinaries();
-    } else {
-      this.forcePic = false;
-      this.shouldStripBinaries = false;
-    }
     this.licensesProvider = licensesProvider;
   }
 
@@ -303,20 +294,12 @@ public boolean usePicForDynamicLibraries(
         || featureConfiguration.isEnabled(CppRuleClasses.SUPPORTS_PIC);
   }
 
-  /**
-   * Returns true if Fission is specified and supported by the CROSSTOOL for the build implied by
-   * the given configuration and toolchain.
-   */
-  public boolean useFission() {
-    return Preconditions.checkNotNull(cppConfiguration).fissionIsActiveForCurrentCompilationMode()
-        && 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) {
+  public boolean shouldCreatePerObjectDebugInfo(
+      FeatureConfiguration featureConfiguration, CppConfiguration cppConfiguration) {
     return cppConfiguration.fissionIsActiveForCurrentCompilationMode()
         && featureConfiguration.isEnabled(CppRuleClasses.PER_OBJECT_DEBUG_INFO);
   }
@@ -333,7 +316,8 @@ public boolean supportsHeaderParsing() {
    * It will run compiler's parser to ensure the header is self-contained. This is required for
    * layering_check to work.
    */
-  public boolean shouldProcessHeaders(FeatureConfiguration featureConfiguration) {
+  public boolean shouldProcessHeaders(
+      FeatureConfiguration featureConfiguration, CppConfiguration cppConfiguration) {
     // If parse_headers_verifies_modules is switched on, we verify that headers are
     // self-contained by building the module instead.
     return !cppConfiguration.getParseHeadersVerifiesModules()
@@ -584,14 +568,6 @@ public String getSolibDirectory() {
     return toolchainInfo.getSolibDirectory();
   }
 
-  /**
-   * Returns the compilation mode.
-   */
-  @Nullable
-  public CompilationMode getCompilationMode() {
-    return cppConfiguration == null ? null : cppConfiguration.getCompilationMode();
-  }
-
   /** Returns whether the toolchain supports dynamic linking. */
   public boolean supportsDynamicLinker(FeatureConfiguration featureConfiguration) {
     return toolchainInfo.supportsDynamicLinker()
@@ -624,16 +600,6 @@ public boolean supportsInterfaceSharedLibraries(FeatureConfiguration featureConf
         || featureConfiguration.isEnabled(CppRuleClasses.SUPPORTS_INTERFACE_SHARED_LIBRARIES);
   }
 
-  /**
-   * Deprecated, do not use. Read the javadoc for {@link
-   * #getCppConfigurationEvenThoughItCanBeDifferentThatWhatTargetHas()}.
-   */
-  @Nullable
-  @Deprecated
-  public CppConfiguration getCppConfiguration() {
-    return cppConfiguration;
-  }
-
   /**
    * Return CppConfiguration instance that was used to configure CcToolchain.
    *
@@ -1046,14 +1012,6 @@ public boolean isHostConfiguration() {
     return isHostConfiguration;
   }
 
-  public boolean getForcePic() {
-    return forcePic;
-  }
-
-  public boolean getShouldStripBinaries() {
-    return shouldStripBinaries;
-  }
-
   public LicensesProvider getLicensesProvider() {
     return licensesProvider;
   }

diff --git a/src/main/java/com/google/devtools/build/lib/rules/cpp/CcToolchainProviderHelper.java b/src/main/java/com/google/devtools/build/lib/rules/cpp/CcToolchainProviderHelper.java
@@ -450,7 +450,10 @@ static CcToolchainProvider getCcToolchainProvider(
     CppToolchainInfo toolchainInfo =
         getCppToolchainInfo(
             ruleContext,
-            cppConfiguration,
+            cppConfiguration.disableLegacyCrosstoolFields(),
+            cppConfiguration.disableGenruleCcToolchainDependency(),
+            cppConfiguration.getTransformedCpuFromOptions(),
+            cppConfiguration.getCompilerFromOptions(),
             attributes,
             ccSkyframeSupportValue,
             toolchain,
@@ -694,7 +697,10 @@ private static FdoInputFile fdoInputFileFromArtifacts(
   /** Finds an appropriate {@link CppToolchainInfo} for this target. */
   private static CppToolchainInfo getCppToolchainInfo(
       RuleContext ruleContext,
-      CppConfiguration cppConfiguration,
+      boolean disableLegacyCrosstoolFields,
+      boolean disableGenruleCcToolchainDependency,
+      String cpuFromOptions,
+      String compilerFromOptions,
       CcToolchainAttributesProvider attributes,
       CcSkyframeSupportValue ccSkyframeSupportValue,
       CToolchain toolchainFromCcToolchainAttribute,
@@ -708,8 +714,8 @@ private static CppToolchainInfo getCppToolchainInfo(
         return CppToolchainInfo.create(
             ruleContext.getLabel(),
             configInfo,
-            cppConfiguration.disableLegacyCrosstoolFields(),
-            cppConfiguration.disableGenruleCcToolchainDependency());
+            disableLegacyCrosstoolFields,
+            disableGenruleCcToolchainDependency);
       } catch (EvalException e) {
         throw ruleContext.throwWithRuleError(e.getMessage());
       }
@@ -722,7 +728,8 @@ private static CppToolchainInfo getCppToolchainInfo(
           getToolchainFromAttributes(
               ruleContext,
               attributes,
-              cppConfiguration,
+              cpuFromOptions,
+              compilerFromOptions,
               crosstoolFromCcToolchainSuiteProtoAttribute,
               ccSkyframeSupportValue);
     }
@@ -742,8 +749,8 @@ private static CppToolchainInfo getCppToolchainInfo(
       return CppToolchainInfo.create(
           attributes.getCcToolchainLabel(),
           ccToolchainConfigInfo,
-          cppConfiguration.disableLegacyCrosstoolFields(),
-          cppConfiguration.disableGenruleCcToolchainDependency());
+          disableLegacyCrosstoolFields,
+          disableGenruleCcToolchainDependency);
     } catch (EvalException e) {
       throw ruleContext.throwWithRuleError(e.getMessage());
     }
@@ -767,23 +774,23 @@ private static CToolchain parseToolchainFromAttributes(
   }
 
   private static void reportInvalidOptions(RuleContext ruleContext, CppToolchainInfo toolchain) {
-    CppOptions options = ruleContext.getConfiguration().getOptions().get(CppOptions.class);
-    CppConfiguration config = ruleContext.getFragment(CppConfiguration.class);
-    if (options.fissionModes.contains(config.getCompilationMode())
+    CppConfiguration cppConfiguration = ruleContext.getFragment(CppConfiguration.class);
+    if (cppConfiguration.fissionIsActiveForCurrentCompilationMode()
         && !toolchain.supportsFission()) {
       ruleContext.ruleWarning(
           "Fission is not supported by this crosstool.  Please use a "
               + "supporting crosstool to enable fission");
     }
-    if (options.buildTestDwp
-        && !(toolchain.supportsFission() && config.fissionIsActiveForCurrentCompilationMode())) {
+    if (cppConfiguration.buildTestDwpIsActivated()
+        && !(toolchain.supportsFission()
+            && cppConfiguration.fissionIsActiveForCurrentCompilationMode())) {
       ruleContext.ruleWarning(
           "Test dwp file requested, but Fission is not enabled.  To generate a "
               + "dwp for the test executable, use '--fission=yes' with a toolchain that supports "
               + "Fission to build statically.");
     }
 
-    if (config.getLibcTopLabel() != null && toolchain.getDefaultSysroot() == null) {
+    if (cppConfiguration.getLibcTopLabel() != null && toolchain.getDefaultSysroot() == null) {
       ruleContext.ruleError(
           "The selected toolchain "
               + toolchain.getToolchainIdentifier()
@@ -795,7 +802,8 @@ private static void reportInvalidOptions(RuleContext ruleContext, CppToolchainIn
   private static CToolchain getToolchainFromAttributes(
       RuleContext ruleContext,
       CcToolchainAttributesProvider attributes,
-      CppConfiguration cppConfiguration,
+      String cpuFromOptions,
+      String compilerFromOptions,
       CrosstoolRelease crosstoolFromCcToolchainSuiteProtoAttribute,
       CcSkyframeSupportValue ccSkyframeSupportValue)
       throws RuleErrorException {
@@ -813,8 +821,8 @@ private static CToolchain getToolchainFromAttributes(
           attributes.getToolchainIdentifier(),
           attributes.getCpu(),
           attributes.getCompiler(),
-          cppConfiguration.getTransformedCpuFromOptions(),
-          cppConfiguration.getCompilerFromOptions(),
+          cpuFromOptions,
+          compilerFromOptions,
           crosstoolRelease);
     } catch (InvalidConfigurationException e) {
       ruleContext.throwWithRuleError(

diff --git a/src/main/java/com/google/devtools/build/lib/rules/cpp/CppCompileActionBuilder.java b/src/main/java/com/google/devtools/build/lib/rules/cpp/CppCompileActionBuilder.java
@@ -620,6 +620,6 @@ public CppCompileActionBuilder setBuiltinIncludeDirectories(
 
   public boolean shouldCompileHeaders() {
     Preconditions.checkNotNull(featureConfiguration);
-    return ccToolchain.shouldProcessHeaders(featureConfiguration);
+    return ccToolchain.shouldProcessHeaders(featureConfiguration, cppConfiguration);
   }
 }
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
@@ -361,7 +361,7 @@ private LtoBackendArtifacts createLtoArtifact(
                 toolchain,
                 fdoContext,
                 usePicForLtoBackendActions,
-                toolchain.shouldCreatePerObjectDebugInfo(featureConfiguration),
+                toolchain.shouldCreatePerObjectDebugInfo(featureConfiguration, cppConfiguration),
                 argv)
             : new LtoBackendArtifacts(
                 ltoOutputRootPrefix,
@@ -375,7 +375,7 @@ private LtoBackendArtifacts createLtoArtifact(
                 toolchain,
                 fdoContext,
                 usePicForLtoBackendActions,
-                toolchain.shouldCreatePerObjectDebugInfo(featureConfiguration),
+                toolchain.shouldCreatePerObjectDebugInfo(featureConfiguration, cppConfiguration),
                 argv);
     return ltoArtifact;
   }

diff --git a/src/main/java/com/google/devtools/build/lib/rules/cpp/LinkBuildVariables.java b/src/main/java/com/google/devtools/build/lib/rules/cpp/LinkBuildVariables.java
@@ -125,12 +125,13 @@ public static CcToolchainVariables setupVariables(
       buildVariables.addStringVariable(FORCE_PIC.getVariableName(), "");
     }
 
-    if (!mustKeepDebug && ccToolchainProvider.getShouldStripBinaries()) {
+    if (!mustKeepDebug && cppConfiguration.shouldStripBinaries()) {
       buildVariables.addStringVariable(STRIP_DEBUG_SYMBOLS.getVariableName(), "");
     }
 
     if (isUsingLinkerNotArchiver
-        && ccToolchainProvider.shouldCreatePerObjectDebugInfo(featureConfiguration)) {
+        && ccToolchainProvider.shouldCreatePerObjectDebugInfo(
+            featureConfiguration, cppConfiguration)) {
       buildVariables.addStringVariable(IS_USING_FISSION.getVariableName(), "");
     }
 

diff --git a/src/main/java/com/google/devtools/build/lib/rules/java/JavaBinary.java b/src/main/java/com/google/devtools/build/lib/rules/java/JavaBinary.java
@@ -155,7 +155,7 @@ public ConfiguredTarget create(RuleContext ruleContext)
             /* unsupportedFeatures= */ ruleContext.getDisabledFeatures(),
             ccToolchain);
     boolean stripAsDefault =
-        ccToolchain.shouldCreatePerObjectDebugInfo(featureConfiguration)
+        ccToolchain.shouldCreatePerObjectDebugInfo(featureConfiguration, cppConfiguration)
             && cppConfiguration.getCompilationMode() == CompilationMode.OPT;
     DeployArchiveBuilder unstrippedDeployArchiveBuilder = null;
     if (stripAsDefault) {

diff --git a/src/main/java/com/google/devtools/build/lib/rules/objc/CompilationSupport.java b/src/main/java/com/google/devtools/build/lib/rules/objc/CompilationSupport.java
@@ -570,8 +570,10 @@ private FeatureConfiguration getFeatureConfiguration(
     if (objcProvider.is(Flag.USES_OBJC)) {
       activatedCrosstoolSelectables.add(CONTAINS_OBJC);
     }
-    if (toolchain.useFission()
-        && !ruleContext.getFragment(CppConfiguration.class).disableLegacyCrosstoolFields()) {
+    CppConfiguration cppConfiguration = ruleContext.getFragment(CppConfiguration.class);
+    if (cppConfiguration.fissionIsActiveForCurrentCompilationMode()
+        && toolchain.supportsFission()
+        && !cppConfiguration.disableLegacyCrosstoolFields()) {
       activatedCrosstoolSelectables.add(CppRuleClasses.PER_OBJECT_DEBUG_INFO);
     }