Route fdo logic that deals with the path to the fdo profile through CcToolchainProvider

As --fdo_optimize can point to a label, the path to the fdo profile can not be reliably determined in CppConfiguration.
In order to enable the fdo features (which depend on the path to the fdo profile), the logic from CppConfiguration.configurationEnabledFeatures() has been moved to CcCommon.configureFeatures(). The latter method has access to the fdo profile path through CcToolchainProvider.

RELNOTES: None.
PiperOrigin-RevId: 186278311

9 files changed, 98 insertions, 68 deletions

diff --git a/src/main/java/com/google/devtools/build/lib/analysis/RuleContext.java b/src/main/java/com/google/devtools/build/lib/analysis/RuleContext.java
index 8d7979ba7c..2be679614c 100644
--- a/src/main/java/com/google/devtools/build/lib/analysis/RuleContext.java
+++ b/src/main/java/com/google/devtools/build/lib/analysis/RuleContext.java
@@ -177,7 +177,8 @@ public final class RuleContext extends TargetContext
   private final ListMultimap<String, ConfiguredFilesetEntry> filesetEntryMap;
   private final ImmutableMap<Label, ConfigMatchingProvider> configConditions;
   private final AspectAwareAttributeMapper attributes;
-  private final ImmutableSet<String> features;
+  private final ImmutableSet<String> enabledFeatures;
+  private final ImmutableSet<String> disabledFeatures;
   private final String ruleClassNameForLogging;
   private final BuildConfiguration hostConfiguration;
   private final PatchTransition disableLipoTransition;
@@ -217,7 +218,11 @@ public final class RuleContext extends TargetContext
     this.filesetEntryMap = filesetEntryMap;
     this.configConditions = configConditions;
     this.attributes = new AspectAwareAttributeMapper(attributes, aspectAttributes);
-    this.features = getEnabledFeatures();
+    Set<String> allEnabledFeatures = new HashSet<>();
+    Set<String> allDisabledFeatures = new HashSet<>();
+    getAllFeatures(allEnabledFeatures, allDisabledFeatures);
+    this.enabledFeatures = ImmutableSortedSet.copyOf(allEnabledFeatures);
+    this.disabledFeatures = ImmutableSortedSet.copyOf(allDisabledFeatures);
     this.ruleClassNameForLogging = ruleClassNameForLogging;
     this.hostConfiguration = builder.hostConfiguration;
     this.disableLipoTransition = builder.disableLipoTransition;
@@ -225,7 +230,7 @@ public final class RuleContext extends TargetContext
     this.toolchainContext = toolchainContext;
   }
 
-  private ImmutableSet<String> getEnabledFeatures() {
+  private void getAllFeatures(Set<String> allEnabledFeatures, Set<String> allDisabledFeatures) {
     Set<String> globallyEnabled = new HashSet<>();
     Set<String> globallyDisabled = new HashSet<>();
     parseFeatures(getConfiguration().getDefaultFeatures(), globallyEnabled, globallyDisabled);
@@ -237,23 +242,26 @@ public final class RuleContext extends TargetContext
     if (attributes().has("features", Type.STRING_LIST)) {
       parseFeatures(attributes().get("features", Type.STRING_LIST), ruleEnabled, ruleDisabled);
     }
+
     Set<String> ruleDisabledFeatures =
         Sets.union(ruleDisabled, Sets.difference(packageDisabled, ruleEnabled));
-    Set<String> disabledFeatures = Sets.union(ruleDisabledFeatures, globallyDisabled);
+    allDisabledFeatures.addAll(Sets.union(ruleDisabledFeatures, globallyDisabled));
     for (ImmutableMap.Entry<Class<? extends Fragment>, Fragment> entry :
         getConfiguration().getAllFragments().entrySet()) {
       if (isLegalFragment(entry.getKey())) {
         globallyEnabled.addAll(
             entry
                 .getValue()
-                .configurationEnabledFeatures(this, ImmutableSortedSet.copyOf(disabledFeatures)));
+                .configurationEnabledFeatures(
+                    this, ImmutableSortedSet.copyOf(allDisabledFeatures)));
       }
     }
+
     Set<String> packageFeatures =
         Sets.difference(Sets.union(globallyEnabled, packageEnabled), packageDisabled);
     Set<String> ruleFeatures =
         Sets.difference(Sets.union(packageFeatures, ruleEnabled), ruleDisabled);
-    return ImmutableSortedSet.copyOf(Sets.difference(ruleFeatures, globallyDisabled));
+    allEnabledFeatures.addAll(Sets.difference(ruleFeatures, globallyDisabled));
   }
 
   private void parseFeatures(Iterable<String> features, Set<String> enabled, Set<String> disabled) {
@@ -1371,7 +1379,12 @@ public final class RuleContext extends TargetContext
    * @return the set of features applicable for the current rule's package.
    */
   public ImmutableSet<String> getFeatures() {
-    return features;
+    return enabledFeatures;
+  }
+
+  /** @return the set of features that are disabled for the current rule's package. */
+  public ImmutableSet<String> getDisabledFeatures() {
+    return disabledFeatures;
   }
 
   @Override
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
index 69c3f6061d..a0629dab46 100644
--- 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
@@ -49,6 +49,7 @@ import com.google.devtools.build.lib.rules.cpp.CcToolchainFeatures.FeatureConfig
 import com.google.devtools.build.lib.rules.cpp.CcToolchainFeatures.Variables;
 import com.google.devtools.build.lib.rules.cpp.CppConfiguration.DynamicMode;
 import com.google.devtools.build.lib.rules.cpp.CppConfiguration.HeadersCheckingMode;
+import com.google.devtools.build.lib.rules.cpp.FdoSupport.FdoMode;
 import com.google.devtools.build.lib.shell.ShellUtils;
 import com.google.devtools.build.lib.skyframe.serialization.ObjectCodec;
 import com.google.devtools.build.lib.skyframe.serialization.autocodec.AutoCodec;
@@ -698,6 +699,7 @@ public final class CcCommon {
     }
     ImmutableSet<String> allUnsupportedFeatures = unsupportedFeaturesBuilder.build();
     ImmutableSet.Builder<String> allRequestedFeaturesBuilder = ImmutableSet.builder();
+
     // If STATIC_LINK_MSVCRT feature isn't specified by user, we add DYNAMIC_LINK_MSVCRT_* feature
     // according to compilation mode.
     // If STATIC_LINK_MSVCRT feature is specified, we add STATIC_LINK_MSVCRT_* feature
@@ -713,6 +715,38 @@ public final class CcCommon {
               ? CppRuleClasses.DYNAMIC_LINK_MSVCRT_DEBUG
               : CppRuleClasses.DYNAMIC_LINK_MSVCRT_NO_DEBUG);
     }
+
+    CppConfiguration cppConfiguration = toolchain.getCppConfiguration();
+    if (cppConfiguration.getFdoInstrument() != null
+        && !ruleContext.getDisabledFeatures().contains(CppRuleClasses.FDO_INSTRUMENT)) {
+      allRequestedFeaturesBuilder.add(CppRuleClasses.FDO_INSTRUMENT);
+    }
+
+    FdoMode fdoMode = toolchain.getFdoMode();
+    boolean isFdo = fdoMode != FdoMode.OFF && toolchain.getCompilationMode() == CompilationMode.OPT;
+    if (isFdo
+        && fdoMode != FdoMode.AUTO_FDO
+        && !ruleContext.getDisabledFeatures().contains(CppRuleClasses.FDO_OPTIMIZE)) {
+      allRequestedFeaturesBuilder.add(CppRuleClasses.FDO_OPTIMIZE);
+    }
+    if (isFdo && fdoMode == FdoMode.AUTO_FDO) {
+      allRequestedFeaturesBuilder.add(CppRuleClasses.AUTOFDO);
+      // For LLVM, support implicit enabling of ThinLTO for AFDO unless it has been
+      // explicitly disabled.
+      if (toolchain.isLLVMCompiler()
+          && !ruleContext.getDisabledFeatures().contains(CppRuleClasses.THIN_LTO)) {
+        allRequestedFeaturesBuilder.add(CppRuleClasses.ENABLE_AFDO_THINLTO);
+      }
+    }
+    if (cppConfiguration.isLipoOptimizationOrInstrumentation()) {
+      // Map LIPO to ThinLTO for LLVM builds.
+      if (toolchain.isLLVMCompiler() && fdoMode != FdoMode.OFF) {
+        allRequestedFeaturesBuilder.add(CppRuleClasses.THIN_LTO);
+      } else {
+        allRequestedFeaturesBuilder.add(CppRuleClasses.LIPO);
+      }
+    }
+
     ImmutableList.Builder<String> allFeatures =
         new ImmutableList.Builder<String>()
             .addAll(
diff --git a/src/main/java/com/google/devtools/build/lib/rules/cpp/CcToolchain.java b/src/main/java/com/google/devtools/build/lib/rules/cpp/CcToolchain.java
index 885f031fac..22ee5acfa6 100644
--- a/src/main/java/com/google/devtools/build/lib/rules/cpp/CcToolchain.java
+++ b/src/main/java/com/google/devtools/build/lib/rules/cpp/CcToolchain.java
@@ -53,6 +53,7 @@ import com.google.devtools.build.lib.rules.cpp.CcToolchainFeatures.Variables;
 import com.google.devtools.build.lib.rules.cpp.CcToolchainFeatures.Variables.Builder;
 import com.google.devtools.build.lib.rules.cpp.CppConfiguration.Tool;
 import com.google.devtools.build.lib.rules.cpp.FdoSupport.FdoException;
+import com.google.devtools.build.lib.rules.cpp.FdoSupport.FdoMode;
 import com.google.devtools.build.lib.syntax.Type;
 import com.google.devtools.build.lib.util.FileType;
 import com.google.devtools.build.lib.util.FileTypeSet;
@@ -371,12 +372,20 @@ public class CcToolchain implements RuleConfiguredTargetFactory {
       return null;
     }
 
+    FdoMode fdoMode;
+    if (fdoZip == null) {
+      fdoMode = FdoMode.OFF;
+    } else if (CppFileTypes.GCC_AUTO_PROFILE.matches(fdoZip.getBaseName())) {
+      fdoMode = FdoMode.AUTO_FDO;
+    } else if (cppConfiguration.isLLVMOptimizedFdo(toolchainInfo.isLLVMCompiler())) {
+      fdoMode = FdoMode.LLVM_FDO;
+    } else {
+      fdoMode = FdoMode.VANILLA;
+    }
+
     SkyKey fdoKey =
         FdoSupportValue.key(
-            cppConfiguration.getLipoMode(),
-            fdoZip,
-            cppConfiguration.getFdoInstrument(),
-            cppConfiguration.isLLVMOptimizedFdo(toolchainInfo.isLLVMCompiler()));
+            cppConfiguration.getLipoMode(), fdoZip, cppConfiguration.getFdoInstrument(), fdoMode);
 
     SkyFunction.Environment skyframeEnv = ruleContext.getAnalysisEnvironment().getSkyframeEnv();
     FdoSupportValue fdoSupport;
@@ -562,7 +571,8 @@ public class CcToolchain implements RuleConfiguredTargetFactory {
                 : null,
             getEnvironment(ruleContext),
             builtInIncludeDirectories,
-            sysroot);
+            sysroot,
+            fdoMode);
 
     TemplateVariableInfo templateVariableInfo =
         createMakeVariableProvider(cppConfiguration, sysroot);
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
index 2f4ca798f3..0f8358ec46 100644
--- 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
@@ -32,6 +32,7 @@ import com.google.devtools.build.lib.concurrent.ThreadSafety.Immutable;
 import com.google.devtools.build.lib.events.Location;
 import com.google.devtools.build.lib.rules.cpp.CcToolchainFeatures.Variables;
 import com.google.devtools.build.lib.rules.cpp.CppConfiguration.Tool;
+import com.google.devtools.build.lib.rules.cpp.FdoSupport.FdoMode;
 import com.google.devtools.build.lib.skylarkinterface.SkylarkCallable;
 import com.google.devtools.build.lib.skylarkinterface.SkylarkModule;
 import com.google.devtools.build.lib.util.Pair;
@@ -79,7 +80,8 @@ public final class CcToolchainProvider extends ToolchainInfo {
           null,
           ImmutableMap.<String, String>of(),
           ImmutableList.<PathFragment>of(),
-          null);
+          null,
+          FdoMode.OFF);
 
   @Nullable private final CppConfiguration cppConfiguration;
   private final CppToolchainInfo toolchainInfo;
@@ -109,6 +111,7 @@ public final class CcToolchainProvider extends ToolchainInfo {
   private final ImmutableMap<String, String> environment;
   private final ImmutableList<PathFragment> builtInIncludeDirectories;
   @Nullable private final PathFragment sysroot;
+  private final FdoMode fdoMode;
 
   public CcToolchainProvider(
       ImmutableMap<String, Object> skylarkToolchain,
@@ -139,7 +142,8 @@ public final class CcToolchainProvider extends ToolchainInfo {
       Artifact linkDynamicLibraryTool,
       ImmutableMap<String, String> environment,
       ImmutableList<PathFragment> builtInIncludeDirectories,
-      @Nullable PathFragment sysroot) {
+      @Nullable PathFragment sysroot,
+      FdoMode fdoMode) {
     super(skylarkToolchain, Location.BUILTIN);
     this.cppConfiguration = cppConfiguration;
     this.toolchainInfo = toolchainInfo;
@@ -169,6 +173,7 @@ public final class CcToolchainProvider extends ToolchainInfo {
     this.environment = environment;
     this.builtInIncludeDirectories = builtInIncludeDirectories;
     this.sysroot = sysroot;
+    this.fdoMode = fdoMode;
   }
 
   /** Returns c++ Make variables. */
@@ -782,6 +787,10 @@ public final class CcToolchainProvider extends ToolchainInfo {
     return toolchainInfo.isLLVMCompiler();
   }
 
+  public FdoMode getFdoMode() {
+    return fdoMode;
+  }
+
   // Not all of CcToolchainProvider is exposed to Skylark, which makes implementing deep equality
   // impossible: if Java-only parts are considered, the behavior is surprising in Skylark, if they
   // are not, the behavior is surprising in Java. Thus, object identity it is.
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 2049d04166..8302cc0380 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
@@ -1343,36 +1343,6 @@ public final class CppConfiguration extends BuildConfiguration.Fragment {
   public ImmutableSet<String> configurationEnabledFeatures(
       RuleContext ruleContext, ImmutableSet<String> disabledFeatures) {
     ImmutableSet.Builder<String> requestedFeatures = ImmutableSet.builder();
-    if (cppOptions.getFdoInstrument() != null) {
-      requestedFeatures.add(CppRuleClasses.FDO_INSTRUMENT);
-    }
-
-    String fdoZip = null;
-    if (fdoProfileAbsolutePath != null) {
-      fdoZip = fdoProfileAbsolutePath.getPathString();
-    } else if (fdoProfileLabel != null) {
-      fdoZip = fdoProfileLabel.getName();
-    }
-    boolean isFdo = fdoZip != null && compilationMode == CompilationMode.OPT;
-    if (isFdo && !CppFileTypes.GCC_AUTO_PROFILE.matches(fdoZip)) {
-      requestedFeatures.add(CppRuleClasses.FDO_OPTIMIZE);
-    }
-    if (isFdo && CppFileTypes.GCC_AUTO_PROFILE.matches(fdoZip)) {
-      requestedFeatures.add(CppRuleClasses.AUTOFDO);
-      // For LLVM, support implicit enabling of ThinLTO for AFDO unless it has been
-      // explicitly disabled.
-      if (isLLVMCompiler() && !disabledFeatures.contains(CppRuleClasses.THIN_LTO)) {
-        requestedFeatures.add(CppRuleClasses.ENABLE_AFDO_THINLTO);
-      }
-    }
-    if (isLipoOptimizationOrInstrumentation()) {
-      // Map LIPO to ThinLTO for LLVM builds.
-      if (isLLVMCompiler() && cppOptions.getFdoOptimize() != null) {
-        requestedFeatures.add(CppRuleClasses.THIN_LTO);
-      } else {
-        requestedFeatures.add(CppRuleClasses.LIPO);
-      }
-    }
     if (ruleContext.getConfiguration().isCodeCoverageEnabled()) {
       requestedFeatures.add(CppRuleClasses.COVERAGE);
       if (useLLVMCoverageMap) {
diff --git a/src/main/java/com/google/devtools/build/lib/rules/cpp/FdoSupport.java b/src/main/java/com/google/devtools/build/lib/rules/cpp/FdoSupport.java
index 41b1605ec0..bbaa0bf8c5 100644
--- a/src/main/java/com/google/devtools/build/lib/rules/cpp/FdoSupport.java
+++ b/src/main/java/com/google/devtools/build/lib/rules/cpp/FdoSupport.java
@@ -283,20 +283,10 @@ public class FdoSupport {
       PathFragment fdoInstrument,
       Path fdoProfile,
       LipoMode lipoMode,
-      boolean llvmFdo,
       Path execRoot,
-      String productName)
+      String productName,
+      FdoMode fdoMode)
       throws IOException, FdoException, InterruptedException {
-    FdoMode fdoMode;
-    if (fdoProfile != null && isAutoFdo(fdoProfile.getBaseName())) {
-      fdoMode = FdoMode.AUTO_FDO;
-    } else if (fdoProfile != null && llvmFdo) {
-      fdoMode = FdoMode.LLVM_FDO;
-    } else if (fdoProfile != null) {
-      fdoMode = FdoMode.VANILLA;
-    } else {
-      fdoMode = FdoMode.OFF;
-    }
 
     if (fdoProfile == null) {
       lipoMode = LipoMode.OFF;
diff --git a/src/main/java/com/google/devtools/build/lib/rules/cpp/FdoSupportFunction.java b/src/main/java/com/google/devtools/build/lib/rules/cpp/FdoSupportFunction.java
index 63c2d3f336..f864104b31 100644
--- a/src/main/java/com/google/devtools/build/lib/rules/cpp/FdoSupportFunction.java
+++ b/src/main/java/com/google/devtools/build/lib/rules/cpp/FdoSupportFunction.java
@@ -65,9 +65,9 @@ public class FdoSupportFunction implements SkyFunction {
               key.getFdoInstrument(),
               key.getFdoZip(),
               key.getLipoMode(),
-              key.getLLVMFdo(),
               execRoot,
-              directories.getProductName());
+              directories.getProductName(),
+              key.getFdoMode());
       if (env.valuesMissing()) {
         return null;
       }
diff --git a/src/main/java/com/google/devtools/build/lib/rules/cpp/FdoSupportValue.java b/src/main/java/com/google/devtools/build/lib/rules/cpp/FdoSupportValue.java
index 0f94dcd486..6cd722f8e4 100644
--- a/src/main/java/com/google/devtools/build/lib/rules/cpp/FdoSupportValue.java
+++ b/src/main/java/com/google/devtools/build/lib/rules/cpp/FdoSupportValue.java
@@ -14,6 +14,7 @@
 package com.google.devtools.build.lib.rules.cpp;
 
 import com.google.devtools.build.lib.concurrent.ThreadSafety.Immutable;
+import com.google.devtools.build.lib.rules.cpp.FdoSupport.FdoMode;
 import com.google.devtools.build.lib.vfs.Path;
 import com.google.devtools.build.lib.vfs.PathFragment;
 import com.google.devtools.build.lib.view.config.crosstool.CrosstoolConfig.LipoMode;
@@ -44,13 +45,13 @@ public class FdoSupportValue implements SkyValue {
     private final LipoMode lipoMode;
     private final Path fdoZip;
     private final PathFragment fdoInstrument;
-    private final boolean llvmFdo;
+    private final FdoMode fdoMode;
 
-    private Key(LipoMode lipoMode, Path fdoZip, PathFragment fdoInstrument, boolean llvmFdo) {
+    private Key(LipoMode lipoMode, Path fdoZip, PathFragment fdoInstrument, FdoMode fdoMode) {
       this.lipoMode = lipoMode;
       this.fdoZip = fdoZip;
       this.fdoInstrument = fdoInstrument;
-      this.llvmFdo = llvmFdo;
+      this.fdoMode = fdoMode;
     }
 
     public LipoMode getLipoMode() {
@@ -65,8 +66,8 @@ public class FdoSupportValue implements SkyValue {
       return fdoInstrument;
     }
 
-    public boolean getLLVMFdo() {
-      return llvmFdo;
+    public FdoMode getFdoMode() {
+      return fdoMode;
     }
 
     @Override
@@ -82,7 +83,7 @@ public class FdoSupportValue implements SkyValue {
       Key that = (Key) o;
       return Objects.equals(this.lipoMode, that.lipoMode)
           && Objects.equals(this.fdoZip, that.fdoZip)
-          && Objects.equals(this.llvmFdo, that.llvmFdo)
+          && Objects.equals(this.fdoMode, that.fdoMode)
           && Objects.equals(this.fdoInstrument, that.fdoInstrument);
     }
 
@@ -103,7 +104,7 @@ public class FdoSupportValue implements SkyValue {
   }
 
   public static SkyKey key(
-      LipoMode lipoMode, Path fdoZip, PathFragment fdoInstrument, boolean llvmFdo) {
-    return LegacySkyKey.create(SKYFUNCTION, new Key(lipoMode, fdoZip, fdoInstrument, llvmFdo));
+      LipoMode lipoMode, Path fdoZip, PathFragment fdoInstrument, FdoMode fdoMode) {
+    return LegacySkyKey.create(SKYFUNCTION, new Key(lipoMode, fdoZip, fdoInstrument, fdoMode));
   }
 }
diff --git a/src/test/java/com/google/devtools/build/lib/rules/cpp/CcToolchainProviderTest.java b/src/test/java/com/google/devtools/build/lib/rules/cpp/CcToolchainProviderTest.java
index 85b0c3d610..a6b67737c7 100644
--- a/src/test/java/com/google/devtools/build/lib/rules/cpp/CcToolchainProviderTest.java
+++ b/src/test/java/com/google/devtools/build/lib/rules/cpp/CcToolchainProviderTest.java
@@ -21,6 +21,7 @@ import com.google.devtools.build.lib.actions.Artifact;
 import com.google.devtools.build.lib.collect.nestedset.NestedSetBuilder;
 import com.google.devtools.build.lib.collect.nestedset.Order;
 import com.google.devtools.build.lib.rules.cpp.CcToolchainFeatures.Variables;
+import com.google.devtools.build.lib.rules.cpp.FdoSupport.FdoMode;
 import com.google.devtools.build.lib.util.Pair;
 import com.google.devtools.build.lib.vfs.PathFragment;
 import org.junit.Test;
@@ -64,7 +65,8 @@ public class CcToolchainProviderTest {
             null,
             ImmutableMap.<String, String>of(),
             ImmutableList.<PathFragment>of(),
-            null);
+            null,
+            FdoMode.OFF);
 
     CcToolchainProvider b =
         new CcToolchainProvider(
@@ -96,7 +98,8 @@ public class CcToolchainProviderTest {
             null,
             ImmutableMap.<String, String>of(),
             ImmutableList.<PathFragment>of(),
-            null);
+            null,
+            FdoMode.OFF);
 
     new EqualsTester()
         .addEqualityGroup(a)