Remove the genfilesPath parameter from FdoSupport.prepareToBuild.

This makes the fdo support independent of the configuration, and therefore
safe to cache.

The genfilesPath was only used in AutoFDO builds, which now use the full
exec path to look up profiling data. The profile already refers to imported
files through their full exec paths, so this approach is more consistent.
Furthermore, if we ever have a source file with the same root-relative path as
a generated file, these would conflict in the FdoSupport map, potentially
leading to errors in the build. This may not be possible right now since such
files would conflict in the label namespace and such conflicts are always
resolved towards the generated file.

--
MOS_MIGRATED_REVID=114204994

4 files changed, 31 insertions, 32 deletions

diff --git a/src/main/java/com/google/devtools/build/lib/analysis/config/BuildConfiguration.java b/src/main/java/com/google/devtools/build/lib/analysis/config/BuildConfiguration.java
index 167fc8cbba..728b55c432 100644
--- a/src/main/java/com/google/devtools/build/lib/analysis/config/BuildConfiguration.java
+++ b/src/main/java/com/google/devtools/build/lib/analysis/config/BuildConfiguration.java
@@ -164,8 +164,7 @@ public final class BuildConfiguration {
      */
     @SuppressWarnings("unused")
     public void prepareHook(Path execPath, ArtifactFactory artifactFactory,
-        PathFragment genfilesPath, PackageRootResolver resolver)
-        throws ViewCreationFailedException {
+        PackageRootResolver resolver) throws ViewCreationFailedException {
     }
 
     /**
@@ -2345,7 +2344,7 @@ public final class BuildConfiguration {
   public void prepareToBuild(Path execRoot, ArtifactFactory artifactFactory,
       PackageRootResolver resolver) throws ViewCreationFailedException {
     for (Fragment fragment : fragments.values()) {
-      fragment.prepareHook(execRoot, artifactFactory, getGenfilesFragment(), resolver);
+      fragment.prepareHook(execRoot, artifactFactory, resolver);
     }
   }
 
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 d67f0058d1..8f10dd99f7 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
@@ -1896,7 +1896,7 @@ public class CppConfiguration extends BuildConfiguration.Fragment {
   }
 
   @Override
-  public void prepareHook(Path execRoot, ArtifactFactory artifactFactory, PathFragment genfilesPath,
+  public void prepareHook(Path execRoot, ArtifactFactory artifactFactory,
       PackageRootResolver resolver) throws ViewCreationFailedException {
     // TODO(bazel-team): Remove the "relative" guard. sysroot should always be relative, and this
     // should be enforced in the creation of CppConfiguration.
@@ -1919,7 +1919,7 @@ public class CppConfiguration extends BuildConfiguration.Fragment {
       }
     }
     try {
-      getFdoSupport().prepareToBuild(execRoot, genfilesPath, artifactFactory, resolver);
+      getFdoSupport().prepareToBuild(execRoot, artifactFactory, resolver);
     } catch (ZipException e) {
       throw new ViewCreationFailedException("Error reading provided FDO zip file", e);
     } catch (FdoException | IOException | PackageRootResolutionException e) {
diff --git a/src/main/java/com/google/devtools/build/lib/rules/cpp/CppModel.java b/src/main/java/com/google/devtools/build/lib/rules/cpp/CppModel.java
index 91ab7d89f1..ff9d9a3ff9 100644
--- a/src/main/java/com/google/devtools/build/lib/rules/cpp/CppModel.java
+++ b/src/main/java/com/google/devtools/build/lib/rules/cpp/CppModel.java
@@ -333,6 +333,7 @@ public final class CppModel {
       CppCompileActionBuilder builder,
       boolean usePic,
       PathFragment ccRelativeName,
+      PathFragment autoFdoImportPath,
       Artifact gcnoFile) {
     CcToolchainFeatures.Variables.Builder buildVariables =
         new CcToolchainFeatures.Variables.Builder();
@@ -367,7 +368,7 @@ public final class CppModel {
 
     if (ccRelativeName != null) {
       cppConfiguration.getFdoSupport().configureCompilation(builder, buildVariables, ruleContext,
-          ccRelativeName, usePic, featureConfiguration);
+          ccRelativeName, autoFdoImportPath, usePic, featureConfiguration);
     }
     if (gcnoFile != null) {
       buildVariables.addVariable("gcov_gcno_file", gcnoFile.getExecPathString());
@@ -426,7 +427,8 @@ public final class CppModel {
         .setDotdFile(outputName, ".h.d")
         // If we generate pic actions, we prefer the header actions to use the pic artifacts.
         .setPicMode(this.getGeneratePicActions());
-    setupBuildVariables(builder, this.getGeneratePicActions(), null, null);
+    setupBuildVariables(builder, this.getGeneratePicActions(), /*ccRelativeName=*/null,
+        /*autoFdoImportPath=*/null, /*gcnoFile=*/null);
     semantics.finalizeCompileActionBuilder(ruleContext, builder);
     CppCompileAction compileAction = builder.build();
     env.registerAction(compileAction);
@@ -458,7 +460,7 @@ public final class CppModel {
     if (fake) {
       boolean usePic = !generateNoPicAction;
       createFakeSourceAction(outputName, result, env, builder, outputExtension,
-          dependencyFileExtension, addObject, ccRelativeName, usePic);
+          dependencyFileExtension, addObject, ccRelativeName, sourceArtifact.getExecPath(), usePic);
     } else {
       // Create PIC compile actions (same as non-PIC, but use -fPIC and
       // generate .pic.o, .pic.d, .pic.gcno instead of .o, .d, .gcno.)
@@ -472,7 +474,8 @@ public final class CppModel {
         if (gcnoFile != null) {
           picBuilder.setGcnoFile(gcnoFile);
         }
-        setupBuildVariables(picBuilder, /*usePic=*/ true, ccRelativeName, gcnoFile);
+        setupBuildVariables(picBuilder, /*usePic=*/ true, ccRelativeName,
+            sourceArtifact.getExecPath(), gcnoFile);
 
         if (maySaveTemps) {
           result.addTemps(
@@ -512,7 +515,8 @@ public final class CppModel {
         if (gcnoFile != null) {
           builder.setGcnoFile(gcnoFile);
         }
-        setupBuildVariables(builder, /*usePic=*/ false, ccRelativeName, gcnoFile);
+        setupBuildVariables(builder, /*usePic=*/false, ccRelativeName, sourceArtifact.getExecPath(),
+            gcnoFile);
 
         if (maySaveTemps) {
           result.addTemps(
@@ -549,7 +553,7 @@ public final class CppModel {
   private void createFakeSourceAction(PathFragment outputName, CcCompilationOutputs.Builder result,
       AnalysisEnvironment env, CppCompileActionBuilder builder, String outputExtension,
       String dependencyFileExtension, boolean addObject, PathFragment ccRelativeName,
-      boolean usePic) {
+      PathFragment execPath, boolean usePic) {
     if (usePic) {
       outputExtension = ".pic" + outputExtension;
       dependencyFileExtension = ".pic" + dependencyFileExtension;
@@ -563,7 +567,7 @@ public final class CppModel {
         .setOutputFile(outputFile)
         .setDotdFile(outputName, dependencyFileExtension)
         .setTempOutputFile(tempOutputName);
-    setupBuildVariables(builder, getGeneratePicActions(), ccRelativeName, null);
+    setupBuildVariables(builder, getGeneratePicActions(), ccRelativeName, execPath, null);
     semantics.finalizeCompileActionBuilder(ruleContext, builder);
     CppCompileAction action = builder.build();
     env.registerAction(action);
@@ -772,9 +776,9 @@ public final class CppModel {
     String iExt = isCFile ? ".i" : ".ii";
     String picExt = usePic ? ".pic" : "";
     CppCompileActionBuilder dBuilder = new CppCompileActionBuilder(builder);
-    setupBuildVariables(dBuilder, usePic, ccRelativeName, null);
+    setupBuildVariables(dBuilder, usePic, ccRelativeName, source.getExecPath(), null);
     CppCompileActionBuilder sdBuilder = new CppCompileActionBuilder(builder);
-    setupBuildVariables(sdBuilder, usePic, ccRelativeName, null);
+    setupBuildVariables(sdBuilder, usePic, ccRelativeName, source.getExecPath(), null);
 
     dBuilder
         .setOutputFile(ruleContext.getRelatedArtifact(outputName, picExt + iExt))
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 73c0544173..f07f7eb38f 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
@@ -269,9 +269,9 @@ public class FdoSupport {
    * @throws PackageRootResolutionException
    */
   @ThreadHostile // must be called before starting the build
-  public void prepareToBuild(Path execRoot, PathFragment genfilesPath,
-      ArtifactFactory artifactDeserializer, PackageRootResolver resolver)
-      throws IOException, FdoException, PackageRootResolutionException {
+  public void prepareToBuild(Path execRoot, ArtifactFactory artifactDeserializer,
+      PackageRootResolver resolver)
+          throws IOException, FdoException, PackageRootResolutionException {
     // The execRoot != null case is only there for testing. We cannot provide a real ZIP file in
     // tests because ZipFileSystem does not work with a ZIP on an in-memory file system.
     // IMPORTANT: Keep in sync with #declareSkyframeDependencies to avoid incrementality issues.
@@ -285,7 +285,7 @@ public class FdoSupport {
         Path fdoImports = fdoProfile.getParentDirectory().getRelative(
             fdoProfile.getBaseName() + ".imports");
         if (isLipoEnabled()) {
-          imports = readAutoFdoImports(artifactDeserializer, fdoImports, genfilesPath, resolver);
+          imports = readAutoFdoImports(artifactDeserializer, fdoImports, resolver);
         }
         FileSystemUtils.ensureSymbolicLink(
             execRoot.getRelative(getAutoProfilePath()), fdoProfile);
@@ -392,21 +392,16 @@ public class FdoSupport {
    * @throws PackageRootResolutionException
    */
   private ImmutableMultimap<PathFragment, Artifact> readAutoFdoImports(
-      ArtifactFactory artifactFactory, Path importsFile, PathFragment genFilePath,
-      PackageRootResolver resolver)
+      ArtifactFactory artifactFactory, Path importsFile, PackageRootResolver resolver)
           throws IOException, FdoException, PackageRootResolutionException {
     ImmutableMultimap.Builder<PathFragment, Artifact> importBuilder = ImmutableMultimap.builder();
     for (String line : FileSystemUtils.iterateLinesAsLatin1(importsFile)) {
       if (!line.isEmpty()) {
         PathFragment key = new PathFragment(line.substring(0, line.indexOf(':')));
-        if (key.startsWith(genFilePath)) {
-          key = key.relativeTo(genFilePath);
-        }
         if (key.isAbsolute()) {
           throw new FdoException("Absolute paths not allowed in afdo imports file " + importsFile
               + ": " + key);
         }
-        key = FileSystemUtils.replaceSegments(key, "PROTECTED", "_protected", true);
         for (String auxFile : line.substring(line.indexOf(':') + 1).split(" ")) {
           if (auxFile.length() == 0) {
             continue;
@@ -426,13 +421,13 @@ public class FdoSupport {
   /**
    * Returns the imports from the .afdo.imports file of a source file.
    *
-   * @param sourceName the source file
+   * @param sourceExecPath the source file
    */
-  private Collection<Artifact> getAutoFdoImports(PathFragment sourceName) {
+  private Collection<Artifact> getAutoFdoImports(PathFragment sourceExecPath) {
     Preconditions.checkState(isLipoEnabled());
-    ImmutableCollection<Artifact> afdoImports = imports.get(sourceName);
+    ImmutableCollection<Artifact> afdoImports = imports.get(sourceExecPath);
     Preconditions.checkState(afdoImports != null,
-        "AutoFDO import data missing for %s", sourceName);
+        "AutoFDO import data missing for %s", sourceExecPath);
     return afdoImports;
   }
 
@@ -460,7 +455,8 @@ public class FdoSupport {
   @ThreadSafe
   public void configureCompilation(CppCompileActionBuilder builder,
       CcToolchainFeatures.Variables.Builder buildVariables, RuleContext ruleContext,
-      PathFragment sourceName, boolean usePic, FeatureConfiguration featureConfiguration) {
+      PathFragment sourceName, PathFragment sourceExecPath, boolean usePic,
+      FeatureConfiguration featureConfiguration) {
     // It is a bug if this method is called with useLipo if lipo is disabled. However, it is legal
     // if is is called with !useLipo, even though lipo is enabled.
     LipoContextProvider lipoInputProvider = CppHelper.getLipoContextProvider(ruleContext);
@@ -484,7 +480,7 @@ public class FdoSupport {
         return;
       }
       Iterable<Artifact> auxiliaryInputs = getAuxiliaryInputs(
-          ruleContext, env, sourceName, usePic, lipoInputProvider);
+          ruleContext, env, sourceName, sourceExecPath, usePic, lipoInputProvider);
       builder.addMandatoryInputs(auxiliaryInputs);
       if (!Iterables.isEmpty(auxiliaryInputs)) {
         if (featureConfiguration.isEnabled(CppRuleClasses.AUTOFDO)) {
@@ -509,7 +505,7 @@ public class FdoSupport {
    */
   private Iterable<Artifact> getAuxiliaryInputs(
       RuleContext ruleContext, AnalysisEnvironment env, PathFragment sourceName,
-      boolean usePic, LipoContextProvider lipoContextProvider) {
+      PathFragment sourceExecPath, boolean usePic, LipoContextProvider lipoContextProvider) {
     // If --fdo_optimize was not specified, we don't have any additional inputs.
     if (fdoProfile == null) {
       return ImmutableSet.of();
@@ -524,7 +520,7 @@ public class FdoSupport {
       env.registerAction(new FdoStubAction(ruleContext.getActionOwner(), artifact));
       auxiliaryInputs.add(artifact);
       if (lipoContextProvider != null) {
-        auxiliaryInputs.addAll(getAutoFdoImports(sourceName));
+        auxiliaryInputs.addAll(getAutoFdoImports(sourceExecPath));
       }
       return auxiliaryInputs.build();
     } else {