Description redacted.

--
MOS_MIGRATED_REVID=131990160

1 files changed, 128 insertions, 86 deletions

diff --git a/src/main/java/com/google/devtools/build/lib/rules/cpp/CppCompileAction.java b/src/main/java/com/google/devtools/build/lib/rules/cpp/CppCompileAction.java
index 5a284b314d..a6fc92c3d8 100644
--- a/src/main/java/com/google/devtools/build/lib/rules/cpp/CppCompileAction.java
+++ b/src/main/java/com/google/devtools/build/lib/rules/cpp/CppCompileAction.java
@@ -175,8 +175,8 @@ public class CppCompileAction extends AbstractAction
   @VisibleForTesting final CppConfiguration cppConfiguration;
   private final FeatureConfiguration featureConfiguration;
   protected final Class<? extends CppCompileActionContext> actionContext;
-  protected final SpecialInputsHandler specialInputsHandler;
-  protected final CppSemantics cppSemantics;
+  private final SpecialInputsHandler specialInputsHandler;
+  private final CppSemantics semantics;
 
   /**
    * Identifier for the actual execution time behavior of the action.
@@ -203,32 +203,29 @@ public class CppCompileAction extends AbstractAction
   /**
    * Creates a new action to compile C/C++ source files.
    *
-   * @param owner the owner of the action, usually the configured target that
-   *        emitted it
-   * @param sourceFile the source file that should be compiled. {@code mandatoryInputs} must
-   *        contain this file
-   * @param shouldScanIncludes a boolean indicating whether scanning of {@code sourceFile}
-   *        is to be performed looking for inclusions.
+   * @param owner the owner of the action, usually the configured target that emitted it
+   * @param sourceFile the source file that should be compiled. {@code mandatoryInputs} must contain
+   *     this file
+   * @param shouldScanIncludes a boolean indicating whether scanning of {@code sourceFile} is to be
+   *     performed looking for inclusions.
    * @param sourceLabel the label of the rule the source file is generated by
-   * @param mandatoryInputs any additional files that need to be present for the
-   *        compilation to succeed, can be empty but not null, for example, extra sources for FDO.
-   * @param outputFile the object file that is written as result of the
-   *        compilation, or the fake object for {@link FakeCppCompileAction}s
-   * @param dotdFile the .d file that is generated as a side-effect of
-   *        compilation
-   * @param gcnoFile the coverage notes that are written in coverage mode, can
-   *        be null
-   * @param dwoFile the .dwo output file where debug information is stored for Fission
-   *        builds (null if Fission mode is disabled)
+   * @param mandatoryInputs any additional files that need to be present for the compilation to
+   *     succeed, can be empty but not null, for example, extra sources for FDO.
+   * @param outputFile the object file that is written as result of the compilation, or the fake
+   *     object for {@link FakeCppCompileAction}s
+   * @param dotdFile the .d file that is generated as a side-effect of compilation
+   * @param gcnoFile the coverage notes that are written in coverage mode, can be null
+   * @param dwoFile the .dwo output file where debug information is stored for Fission builds (null
+   *     if Fission mode is disabled)
    * @param optionalSourceFile an additional optional source file (null if unneeded)
    * @param configuration the build configurations
    * @param context the compilation context
    * @param copts options for the compiler
    * @param coptsFilter regular expression to remove options from {@code copts}
    * @param executionRequirements out-of-band hints to be passed to the execution backend to signal
-   *        platform requirements
+   *     platform requirements
    * @param actionName a string giving the name of this action for the purpose of toolchain
-   *        evaluation
+   *     evaluation
    */
   protected CppCompileAction(
       ActionOwner owner,
@@ -260,7 +257,7 @@ public class CppCompileAction extends AbstractAction
       ImmutableMap<String, String> environment,
       String actionName,
       RuleContext ruleContext,
-      CppSemantics cppSemantics) {
+      CppSemantics semantics) {
     super(
         owner,
         createInputs(
@@ -299,7 +296,7 @@ public class CppCompileAction extends AbstractAction
     // artifact and will definitely exist prior to this action execution.
     this.mandatoryInputs = mandatoryInputs;
     this.builtinIncludeFiles = CppHelper.getToolchain(ruleContext).getBuiltinIncludeFiles();
-    this.cppSemantics = cppSemantics;
+    this.semantics = semantics;
     verifyIncludePaths(ruleContext);
   }
 
@@ -909,6 +906,105 @@ public class CppCompileAction extends AbstractAction
     }
   }
 
+  private DependencySet processDepset(Path execRoot, CppCompileActionContext.Reply reply)
+      throws IOException {
+    DotdFile dotdFile = getDotdFile();
+    Preconditions.checkNotNull(dotdFile);
+    DependencySet depSet = new DependencySet(execRoot);
+    // artifact() is null if we are using in-memory .d files. We also want to prepare for the
+    // case where we expected an in-memory .d file, but we did not get an appropriate response.
+    // Perhaps we produced the file locally.
+    if (dotdFile.artifact() != null || reply == null) {
+      return depSet.read(dotdFile.getPath());
+    } else {
+      // This is an in-memory .d file.
+      return depSet.process(reply.getContents());
+    }
+  }
+
+  /**
+   * Returns a collection with additional input artifacts relevant to the action by reading the
+   * dynamically-discovered dependency information from the .d file after the action has run.
+   *
+   * <p>Artifacts are considered inputs but not "mandatory" inputs.
+   *
+   * @param reply the reply from the compilation.
+   * @throws ActionExecutionException iff the .d is missing (when required), malformed, or has
+   *         unresolvable included artifacts.
+   */
+  @VisibleForTesting
+  @ThreadCompatible
+  public NestedSet<Artifact> discoverInputsFromDotdFiles(
+      Path execRoot, ArtifactResolver artifactResolver, Reply reply)
+      throws ActionExecutionException {
+    NestedSetBuilder<Artifact> inputs = NestedSetBuilder.stableOrder();
+    if (getDotdFile() == null) {
+      return inputs.build();
+    }
+    try {
+      // Read .d file.
+      DependencySet depSet = processDepset(execRoot, reply);
+
+      // Determine prefixes of allowed absolute inclusions.
+      CppConfiguration toolchain = cppConfiguration;
+      List<Path> systemIncludePrefixes = new ArrayList<>();
+      for (PathFragment includePath : toolchain.getBuiltInIncludeDirectories()) {
+        if (includePath.isAbsolute()) {
+          systemIncludePrefixes.add(execRoot.getFileSystem().getPath(includePath));
+        }
+      }
+
+      // Check inclusions.
+      IncludeProblems problems = new IncludeProblems();
+      Map<PathFragment, Artifact> allowedDerivedInputsMap = getAllowedDerivedInputsMap();
+      for (Path execPath : depSet.getDependencies()) {
+        // Module .pcm files are generated and thus aren't declared inputs.
+        if (execPath.getBaseName().endsWith(".pcm")) {
+          continue;
+        }
+        PathFragment execPathFragment = execPath.asFragment();
+        if (execPathFragment.isAbsolute()) {
+          // Absolute includes from system paths are ignored.
+          if (FileSystemUtils.startsWithAny(execPath, systemIncludePrefixes)) {
+            continue;
+          }
+          // Since gcc is given only relative paths on the command line,
+          // non-system include paths here should never be absolute. If they
+          // are, it's probably due to a non-hermetic #include, & we should stop
+          // the build with an error.
+          if (execPath.startsWith(execRoot)) {
+            execPathFragment = execPath.relativeTo(execRoot); // funky but tolerable path
+          } else {
+            problems.add(execPathFragment.getPathString());
+            continue;
+          }
+        }
+        Artifact artifact = allowedDerivedInputsMap.get(execPathFragment);
+        if (artifact == null) {
+          artifact = artifactResolver.resolveSourceArtifact(execPathFragment);
+        }
+        if (artifact != null) {
+          inputs.add(artifact);
+          // In some cases, execution backends need extra files for each included file. Add them
+          // to the set of actual inputs.
+          inputs.addAll(specialInputsHandler.getInputsForIncludedFile(artifact, artifactResolver));
+        } else {
+          // Abort if we see files that we can't resolve, likely caused by
+          // undeclared includes or illegal include constructs.
+          problems.add(execPathFragment.getPathString());
+        }
+      }
+      //TODO(b/22551695): Remove in favor of seperate implementations.
+      if (semantics == null || semantics.needsIncludeValidation()) {
+        problems.assertProblemFree(this, getSourceFile());
+      }
+    } catch (IOException e) {
+      // Some kind of IO or parse exception--wrap & rethrow it to stop the build.
+      throw new ActionExecutionException("error while parsing .d file", e, this, false);
+    }
+    return inputs.build();
+  }
+
   @Override
   public Iterable<Artifact> resolveInputsFromCache(
       ArtifactResolver artifactResolver,
@@ -960,7 +1056,7 @@ public class CppCompileAction extends AbstractAction
     }
   }
 
-  protected Map<PathFragment, Artifact> getAllowedDerivedInputsMap() {
+  private Map<PathFragment, Artifact> getAllowedDerivedInputsMap() {
     Map<PathFragment, Artifact> allowedDerivedInputMap = new HashMap<>();
     addToMap(allowedDerivedInputMap, mandatoryInputs);
     addToMap(allowedDerivedInputMap, getDeclaredIncludeSrcs());
@@ -1083,10 +1179,9 @@ public class CppCompileAction extends AbstractAction
 
     // This is the .d file scanning part.
     IncludeScanningContext scanningContext = executor.getContext(IncludeScanningContext.class);
-    Path execRoot = executor.getExecRoot();
-
     NestedSet<Artifact> discoveredInputs =
-        discoverInputsFromDotdFiles(execRoot, scanningContext.getArtifactResolver(), reply);
+        discoverInputsFromDotdFiles(
+            executor.getExecRoot(), scanningContext.getArtifactResolver(), reply);
     reply = null; // Clear in-memory .d files early.
 
     // Post-execute "include scanning", which modifies the action inputs to match what the compile
@@ -1104,67 +1199,14 @@ public class CppCompileAction extends AbstractAction
       updateActionInputs(discoveredInputs);
     }
 
-    // hdrs_check: This cannot be switched off, because doing so would allow for incorrect builds.
-    validateInclusions(
-        discoveredInputs,
-        actionExecutionContext.getArtifactExpander(),
-        executor.getEventHandler());
-  }
-
-  @VisibleForTesting
-  public NestedSet<Artifact> discoverInputsFromDotdFiles(
-      Path execRoot, ArtifactResolver artifactResolver, Reply reply)
-      throws ActionExecutionException {
-    if (getDotdFile() == null) {
-      return NestedSetBuilder.<Artifact>stableOrder().build();
-    }
-    HeaderDiscovery.Builder discoveryBuilder = new HeaderDiscovery.Builder()
-        .setAction(this)
-        .setDotdFile(getDotdFile())
-        .setSourceFile(getSourceFile())
-        .setSpecialInputsHandler(specialInputsHandler)
-        .setDependencySet(processDepset(execRoot, reply))
-        .setPermittedSystemIncludePrefixes(getPermittedSystemIncludePrefixes(execRoot))
-        .setAllowedDerivedinputsMap(getAllowedDerivedInputsMap());
-    
-   if (cppSemantics.needsIncludeValidation()) {
-     discoveryBuilder.shouldValidateInclusions();
-   }
-        
-   return discoveryBuilder
-        .build()
-        .discoverInputsFromDotdFiles(execRoot, artifactResolver);
-  }
-
-  public DependencySet processDepset(Path execRoot, Reply reply) throws ActionExecutionException {
-    try {
-      DotdFile dotdFile = getDotdFile();
-      Preconditions.checkNotNull(dotdFile);
-      DependencySet depSet = new DependencySet(execRoot);
-      // artifact() is null if we are using in-memory .d files. We also want to prepare for the
-      // case where we expected an in-memory .d file, but we did not get an appropriate response.
-      // Perhaps we produced the file locally.
-      if (dotdFile.artifact() != null || reply == null) {
-        return depSet.read(dotdFile.getPath());
-      } else {
-        // This is an in-memory .d file.
-        return depSet.process(reply.getContents());
-      }
-    } catch (IOException e) {
-      // Some kind of IO or parse exception--wrap & rethrow it to stop the build.
-      throw new ActionExecutionException("error while parsing .d file", e, this, false);
-    }
-  }
-
-  public List<Path> getPermittedSystemIncludePrefixes(Path execRoot) {
-    CppConfiguration toolchain = cppConfiguration;
-    List<Path> systemIncludePrefixes = new ArrayList<>();
-    for (PathFragment includePath : toolchain.getBuiltInIncludeDirectories()) {
-      if (includePath.isAbsolute()) {
-        systemIncludePrefixes.add(execRoot.getFileSystem().getPath(includePath));
-      }
+    // hdrs_check: Turning this off opens the door to incorrect builds.  However, we allow it
+    // to accommodate the current behavior in the objc rules.
+    if (semantics == null || semantics.needsIncludeValidation()) {
+      validateInclusions(
+          discoveredInputs,
+          actionExecutionContext.getArtifactExpander(),
+          executor.getEventHandler());
     }
-    return systemIncludePrefixes;
   }
 
   /**