Route --fdo_optimize information through an implicit dependency instead of CppConfiguration

RELNOTES: None.
PiperOrigin-RevId: 185527875

6 files changed, 181 insertions, 69 deletions

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 5f848b4a29..f52a13c4a6 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
@@ -55,6 +55,7 @@ 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.syntax.Type;
 import com.google.devtools.build.lib.util.FileType;
+import com.google.devtools.build.lib.util.FileTypeSet;
 import com.google.devtools.build.lib.util.Pair;
 import com.google.devtools.build.lib.vfs.FileSystemUtils;
 import com.google.devtools.build.lib.vfs.Path;
@@ -334,9 +335,42 @@ public class CcToolchain implements RuleConfiguredTargetFactory {
       toolchainInfo = cppConfiguration.getCppToolchainInfo();
     }
 
-    Path fdoZip = ruleContext.getConfiguration().getCompilationMode() == CompilationMode.OPT
-        ? cppConfiguration.getFdoZip()
-        : null;
+    Path fdoZip = null;
+    if (ruleContext.getConfiguration().getCompilationMode() == CompilationMode.OPT) {
+      if (cppConfiguration.getFdoProfileAbsolutePath() != null) {
+        fdoZip = cppConfiguration.getFdoProfileAbsolutePath();
+      } else if (cppConfiguration.getFdoProfileLabel() != null) {
+        Artifact fdoArtifact = ruleContext.getPrerequisiteArtifact(":fdo_optimize", Mode.TARGET);
+        if (fdoArtifact != null) {
+          if (!fdoArtifact.isSourceArtifact()) {
+            ruleContext.ruleError("--fdo_optimize points to a target that is not an input file");
+            return null;
+          }
+          Label fdoLabel = ruleContext.getPrerequisite(":fdo_optimize", Mode.TARGET).getLabel();
+          if (!fdoLabel
+              .getPackageIdentifier()
+              .getPathUnderExecRoot()
+              .getRelative(fdoLabel.getName())
+              .equals(fdoArtifact.getExecPath())) {
+            ruleContext.ruleError("--fdo_optimize points to a target that is not an input file");
+            return null;
+          }
+          fdoZip = fdoArtifact.getPath();
+        }
+      }
+    }
+
+    FileTypeSet validExtensions =
+        FileTypeSet.of(
+            CppFileTypes.GCC_AUTO_PROFILE,
+            CppFileTypes.LLVM_PROFILE,
+            CppFileTypes.LLVM_PROFILE_RAW,
+            FileType.of(".zip"));
+    if (fdoZip != null && !validExtensions.matches(fdoZip.getPathString())) {
+      ruleContext.ruleError("invalid extension for FDO profile file.");
+      return null;
+    }
+
     SkyKey fdoKey =
         FdoSupportValue.key(
             cppConfiguration.getLipoMode(),
diff --git a/src/main/java/com/google/devtools/build/lib/rules/cpp/CcToolchainRule.java b/src/main/java/com/google/devtools/build/lib/rules/cpp/CcToolchainRule.java
index 6a66fb6f2c..e7bb964bbe 100644
--- a/src/main/java/com/google/devtools/build/lib/rules/cpp/CcToolchainRule.java
+++ b/src/main/java/com/google/devtools/build/lib/rules/cpp/CcToolchainRule.java
@@ -54,6 +54,12 @@ public final class CcToolchainRule implements RuleDefinition {
           null,
           (rule, attributes, cppConfig) -> cppConfig.getSysrootLabel());
 
+  private static final LabelLateBoundDefault<?> FDO_LABEL =
+      LabelLateBoundDefault.fromTargetConfiguration(
+          CppConfiguration.class,
+          null,
+          (rule, attributes, cppConfig) -> cppConfig.getFdoProfileLabel());
+
   @Override
   public RuleClass build(Builder builder, RuleDefinitionEnvironment env) {
     final Label zipper = env.getToolsLabel("//tools/zip:zipper");
@@ -126,6 +132,7 @@ public final class CcToolchainRule implements RuleDefinition {
                         (rule, attributes, cppConfig) ->
                             cppConfig.isLLVMOptimizedFdo() ? zipper : null)))
         .add(attr(":libc_top", LABEL).value(LIBC_TOP))
+        .add(attr(":fdo_optimize", LABEL).singleArtifact().value(FDO_LABEL))
         .add(
             attr(TransitiveLipoInfoProvider.LIPO_CONTEXT_COLLECTOR, LABEL)
                 .cfg(LipoContextCollectorTransition.INSTANCE)
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 107a4cce77..36624d85a7 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
@@ -168,7 +168,8 @@ public final class CppConfiguration extends BuildConfiguration.Fragment {
   private final boolean convertLipoToThinLto;
   private final PathFragment crosstoolTopPathFragment;
 
-  private final Path fdoZip;
+  private final Path fdoProfileAbsolutePath;
+  private final Label fdoProfileLabel;
 
   // TODO(bazel-team): All these labels (except for ccCompilerRuleLabel) can be removed once the
   // transition to the cc_compiler rule is complete.
@@ -272,7 +273,8 @@ public final class CppConfiguration extends BuildConfiguration.Fragment {
         Preconditions.checkNotNull(params.commonOptions.cpu),
         cppOptions.convertLipoToThinLto,
         crosstoolTopPathFragment,
-        params.fdoZip,
+        params.fdoProfileAbsolutePath,
+        params.fdoProfileLabel,
         params.ccToolchainLabel,
         params.stlLabel,
         params.sysrootLabel == null
@@ -294,30 +296,22 @@ public final class CppConfiguration extends BuildConfiguration.Fragment {
         ImmutableList.copyOf(cppOptions.conlyoptList),
         new FlagList(
             cppToolchainInfo.configureLinkerOptions(
-                compilationMode,
-                cppOptions.getLipoMode(),
-                LinkingMode.FULLY_STATIC),
+                compilationMode, cppOptions.getLipoMode(), LinkingMode.FULLY_STATIC),
             FlagList.convertOptionalOptions(toolchain.getOptionalLinkerFlagList()),
             ImmutableList.<String>of()),
         new FlagList(
             cppToolchainInfo.configureLinkerOptions(
-                compilationMode,
-                cppOptions.getLipoMode(),
-                LinkingMode.MOSTLY_STATIC),
+                compilationMode, cppOptions.getLipoMode(), LinkingMode.MOSTLY_STATIC),
             FlagList.convertOptionalOptions(toolchain.getOptionalLinkerFlagList()),
             ImmutableList.<String>of()),
         new FlagList(
             cppToolchainInfo.configureLinkerOptions(
-                compilationMode,
-                cppOptions.getLipoMode(),
-                LinkingMode.MOSTLY_STATIC_LIBRARIES),
+                compilationMode, cppOptions.getLipoMode(), LinkingMode.MOSTLY_STATIC_LIBRARIES),
             FlagList.convertOptionalOptions(toolchain.getOptionalLinkerFlagList()),
             ImmutableList.<String>of()),
         new FlagList(
             cppToolchainInfo.configureLinkerOptions(
-                compilationMode,
-                cppOptions.getLipoMode(),
-                LinkingMode.DYNAMIC),
+                compilationMode, cppOptions.getLipoMode(), LinkingMode.DYNAMIC),
             FlagList.convertOptionalOptions(toolchain.getOptionalLinkerFlagList()),
             ImmutableList.<String>of()),
         ImmutableList.copyOf(cppOptions.coptList),
@@ -343,7 +337,8 @@ public final class CppConfiguration extends BuildConfiguration.Fragment {
       String desiredCpu,
       boolean convertLipoToThinLto,
       PathFragment crosstoolTopPathFragment,
-      Path fdoZip,
+      Path fdoProfileAbsolutePath,
+      Label fdoProfileLabel,
       Label ccToolchainLabel,
       Label stlLabel,
       PathFragment nonConfiguredSysroot,
@@ -373,7 +368,8 @@ public final class CppConfiguration extends BuildConfiguration.Fragment {
     this.desiredCpu = desiredCpu;
     this.convertLipoToThinLto = convertLipoToThinLto;
     this.crosstoolTopPathFragment = crosstoolTopPathFragment;
-    this.fdoZip = fdoZip;
+    this.fdoProfileAbsolutePath = fdoProfileAbsolutePath;
+    this.fdoProfileLabel = fdoProfileLabel;
     this.ccToolchainLabel = ccToolchainLabel;
     this.stlLabel = stlLabel;
     this.nonConfiguredSysroot = nonConfiguredSysroot;
@@ -1333,8 +1329,12 @@ public final class CppConfiguration extends BuildConfiguration.Fragment {
     return cppOptions.getFdoInstrument();
   }
 
-  public Path getFdoZip() {
-    return fdoZip;
+  public Path getFdoProfileAbsolutePath() {
+    return fdoProfileAbsolutePath;
+  }
+
+  public Label getFdoProfileLabel() {
+    return fdoProfileLabel;
   }
 
   /**
@@ -1349,6 +1349,12 @@ public final class CppConfiguration extends BuildConfiguration.Fragment {
       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);
diff --git a/src/main/java/com/google/devtools/build/lib/rules/cpp/CppConfigurationLoader.java b/src/main/java/com/google/devtools/build/lib/rules/cpp/CppConfigurationLoader.java
index 57a5a0487f..92754dbce8 100644
--- a/src/main/java/com/google/devtools/build/lib/rules/cpp/CppConfigurationLoader.java
+++ b/src/main/java/com/google/devtools/build/lib/rules/cpp/CppConfigurationLoader.java
@@ -28,9 +28,6 @@ import com.google.devtools.build.lib.cmdline.Label;
 import com.google.devtools.build.lib.cmdline.LabelSyntaxException;
 import com.google.devtools.build.lib.events.Event;
 import com.google.devtools.build.lib.packages.BuildType;
-import com.google.devtools.build.lib.packages.InputFile;
-import com.google.devtools.build.lib.packages.NoSuchPackageException;
-import com.google.devtools.build.lib.packages.NoSuchTargetException;
 import com.google.devtools.build.lib.packages.NoSuchThingException;
 import com.google.devtools.build.lib.packages.NonconfigurableAttributeMapper;
 import com.google.devtools.build.lib.packages.Rule;
@@ -88,7 +85,8 @@ public class CppConfigurationLoader implements ConfigurationFragmentFactory {
     protected final Label crosstoolTop;
     protected final Label ccToolchainLabel;
     protected final Label stlLabel;
-    protected final Path fdoZip;
+    protected final Path fdoProfileAbsolutePath;
+    protected final Label fdoProfileLabel;
     protected final Label sysrootLabel;
     protected final CpuTransformer cpuTransformer;
 
@@ -97,7 +95,8 @@ public class CppConfigurationLoader implements ConfigurationFragmentFactory {
         CrosstoolConfigurationLoader.CrosstoolFile crosstoolFile,
         String cacheKeySuffix,
         BuildOptions buildOptions,
-        Path fdoZip,
+        Path fdoProfileAbsolutePath,
+        Label fdoProfileLabel,
         Label crosstoolTop,
         Label ccToolchainLabel,
         Label stlLabel,
@@ -108,7 +107,8 @@ public class CppConfigurationLoader implements ConfigurationFragmentFactory {
       this.cacheKeySuffix = cacheKeySuffix;
       this.commonOptions = buildOptions.get(BuildConfiguration.Options.class);
       this.cppOptions = buildOptions.get(CppOptions.class);
-      this.fdoZip = fdoZip;
+      this.fdoProfileAbsolutePath = fdoProfileAbsolutePath;
+      this.fdoProfileLabel = fdoProfileLabel;
       this.crosstoolTop = crosstoolTop;
       this.ccToolchainLabel = ccToolchainLabel;
       this.stlLabel = stlLabel;
@@ -151,35 +151,25 @@ public class CppConfigurationLoader implements ConfigurationFragmentFactory {
 
     // FDO
     // TODO(bazel-team): move this to CppConfiguration.prepareHook
-    Path fdoZip;
-    if (cppOptions.getFdoOptimize() == null) {
-      fdoZip = null;
-    } else if (cppOptions.getFdoOptimize().startsWith("//")) {
-      try {
-        Target target = env.getTarget(Label.parseAbsolute(cppOptions.getFdoOptimize()));
-        if (target == null) {
-          return null;
-        }
-        if (!(target instanceof InputFile)) {
-          throw new InvalidConfigurationException(
-              "--fdo_optimize cannot accept targets that do not refer to input files");
+    Path fdoProfileAbsolutePath = null;
+    Label fdoProfileLabel = null;
+    if (cppOptions.getFdoOptimize() != null) {
+      if (cppOptions.getFdoOptimize().startsWith("//")) {
+        try {
+          fdoProfileLabel = Label.parseAbsolute(cppOptions.getFdoOptimize());
+        } catch (LabelSyntaxException e) {
+          env.getEventHandler().handle(Event.error(e.getMessage()));
+          throw new InvalidConfigurationException(e);
         }
-        fdoZip = env.getPath(target.getPackage(), target.getName());
-        if (fdoZip == null) {
-          throw new InvalidConfigurationException(
-              "The --fdo_optimize parameter you specified resolves to a file that does not exist");
+      } else {
+        fdoProfileAbsolutePath =
+            directories.getWorkspace().getRelative(cppOptions.getFdoOptimize());
+        try {
+          // We don't check for file existence, but at least the filename should be well-formed.
+          FileSystemUtils.checkBaseName(fdoProfileAbsolutePath.getBaseName());
+        } catch (IllegalArgumentException e) {
+          throw new InvalidConfigurationException(e);
         }
-      } catch (NoSuchPackageException | NoSuchTargetException | LabelSyntaxException e) {
-        env.getEventHandler().handle(Event.error(e.getMessage()));
-        throw new InvalidConfigurationException(e);
-      }
-    } else {
-      fdoZip = directories.getWorkspace().getRelative(cppOptions.getFdoOptimize());
-      try {
-        // We don't check for file existence, but at least the filename should be well-formed.
-        FileSystemUtils.checkBaseName(fdoZip.getBaseName());
-      } catch (IllegalArgumentException e) {
-        throw new InvalidConfigurationException(e);
       }
     }
 
@@ -232,7 +222,8 @@ public class CppConfigurationLoader implements ConfigurationFragmentFactory {
         file,
         file.getMd5(),
         options,
-        fdoZip,
+        fdoProfileAbsolutePath,
+        fdoProfileLabel,
         crosstoolTopLabel,
         ccToolchainLabel,
         stlLabel,
diff --git a/src/test/java/com/google/devtools/build/lib/analysis/BuildViewTest.java b/src/test/java/com/google/devtools/build/lib/analysis/BuildViewTest.java
index 6af2d40a07..81df28f11c 100644
--- a/src/test/java/com/google/devtools/build/lib/analysis/BuildViewTest.java
+++ b/src/test/java/com/google/devtools/build/lib/analysis/BuildViewTest.java
@@ -1140,20 +1140,6 @@ public class BuildViewTest extends BuildViewTestBase {
   }
 
   @Test
-  public void testMissingFdoOptimize() throws Exception {
-    // The fdo_optimize flag uses a different code path, because it also accepts paths.
-    useConfiguration("--fdo_optimize=//does/not/exist");
-    reporter.removeHandler(failFastHandler);
-    try {
-      update(defaultFlags().with(Flag.KEEP_GOING));
-      fail();
-    } catch (InvalidConfigurationException e) {
-      assertContainsEvent(
-          "no such package 'does/not/exist': BUILD file not found on package path");
-    }
-  }
-
-  @Test
   public void testVisibilityReferencesNonexistentPackage() throws Exception {
     scratch.file("z/a/BUILD",
         "py_library(name='a', visibility=['//nonexistent:nothing'])");
diff --git a/src/test/java/com/google/devtools/build/lib/rules/cpp/CcToolchainTest.java b/src/test/java/com/google/devtools/build/lib/rules/cpp/CcToolchainTest.java
index 86414b8cfe..f6c0577ac8 100644
--- a/src/test/java/com/google/devtools/build/lib/rules/cpp/CcToolchainTest.java
+++ b/src/test/java/com/google/devtools/build/lib/rules/cpp/CcToolchainTest.java
@@ -699,4 +699,92 @@ public class CcToolchainTest extends BuildViewTestCase {
         "cc_toolchain_alias(name='ref')");
     assertThat(reference.get(ToolchainInfo.PROVIDER.getKey())).isNotNull();
   }
+
+  @Test
+  public void testFdoOptimizeInvalidUseGeneratedArtifact() throws Exception {
+    reporter.removeHandler(failFastHandler);
+    scratch.file(
+        "a/BUILD",
+        "filegroup(",
+        "   name='empty')",
+        "filegroup(",
+        "    name = 'banana',",
+        "    srcs = ['banana1', 'banana2'])",
+        "cc_toolchain(",
+        "    name = 'b',",
+        "    cpu = 'banana',",
+        "    all_files = ':banana',",
+        "    compiler_files = ':empty',",
+        "    dwp_files = ':empty',",
+        "    linker_files = ':empty',",
+        "    strip_files = ':empty',",
+        "    objcopy_files = ':empty',",
+        "    dynamic_runtime_libs = [':empty'],",
+        "    static_runtime_libs = [':empty'])",
+        "genrule(",
+        "    name ='gen_artifact',",
+        "    outs=['profile.profdata'],",
+        "    cmd='touch $@')");
+    useConfiguration("-c", "opt", "--fdo_optimize=//a:gen_artifact");
+    assertThat(getConfiguredTarget("//a:b")).isNull();
+    assertContainsEvent("--fdo_optimize points to a target that is not an input file");
+  }
+
+  @Test
+  public void testFdoOptimizeUnexpectedExtension() throws Exception {
+    reporter.removeHandler(failFastHandler);
+    scratch.file(
+        "a/BUILD",
+        "filegroup(",
+        "   name='empty')",
+        "filegroup(",
+        "    name = 'banana',",
+        "    srcs = ['banana1', 'banana2'])",
+        "cc_toolchain(",
+        "    name = 'b',",
+        "    cpu = 'banana',",
+        "    all_files = ':banana',",
+        "    compiler_files = ':empty',",
+        "    dwp_files = ':empty',",
+        "    linker_files = ':empty',",
+        "    strip_files = ':empty',",
+        "    objcopy_files = ':empty',",
+        "    dynamic_runtime_libs = [':empty'],",
+        "    static_runtime_libs = [':empty'])",
+        "exports_files(['profile.unexpected'])");
+    scratch.file("a/profile.unexpected", "");
+    useConfiguration("-c", "opt", "--fdo_optimize=//a:profile.unexpected");
+    assertThat(getConfiguredTarget("//a:b")).isNull();
+    assertContainsEvent("invalid extension for FDO profile file");
+  }
+
+  @Test
+  public void testFdoOptimizeNotInputFile() throws Exception {
+    reporter.removeHandler(failFastHandler);
+    scratch.file(
+        "a/BUILD",
+        "filegroup(",
+        "   name='empty')",
+        "filegroup(",
+        "    name = 'banana',",
+        "    srcs = ['banana1', 'banana2'])",
+        "cc_toolchain(",
+        "    name = 'b',",
+        "    cpu = 'banana',",
+        "    all_files = ':banana',",
+        "    compiler_files = ':empty',",
+        "    dwp_files = ':empty',",
+        "    linker_files = ':empty',",
+        "    strip_files = ':empty',",
+        "    objcopy_files = ':empty',",
+        "    dynamic_runtime_libs = [':empty'],",
+        "    static_runtime_libs = [':empty'])",
+        "filegroup(",
+        "    name ='profile',",
+        "    srcs=['my_profile.afdo'])");
+    scratch.file("my_profile.afdo", "");
+    useConfiguration("-c", "opt", "--fdo_optimize=//a:profile");
+    assertThat(getConfiguredTarget("//a:b")).isNull();
+    assertContainsEvent("--fdo_optimize points to a target that is not an input file");
+  }
 }