Implement input pruning using .d files in objc behind a flag that defaults to

false.

--
MOS_MIGRATED_REVID=134452391

4 files changed, 110 insertions, 125 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 322ed53c72..c3c93864a8 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
@@ -43,7 +43,6 @@ import com.google.devtools.build.lib.analysis.actions.ExecutionInfoSpecifier;
 import com.google.devtools.build.lib.analysis.config.BuildConfiguration;
 import com.google.devtools.build.lib.analysis.config.PerLabelOptions;
 import com.google.devtools.build.lib.cmdline.Label;
-import com.google.devtools.build.lib.cmdline.RepositoryName;
 import com.google.devtools.build.lib.collect.CollectionUtils;
 import com.google.devtools.build.lib.collect.nestedset.NestedSet;
 import com.google.devtools.build.lib.collect.nestedset.NestedSetBuilder;
@@ -189,8 +188,8 @@ public class CppCompileAction extends AbstractAction
   @VisibleForTesting final CppConfiguration cppConfiguration;
   private final FeatureConfiguration featureConfiguration;
   protected final Class<? extends CppCompileActionContext> actionContext;
-  private final SpecialInputsHandler specialInputsHandler;
-  private final CppSemantics semantics;
+  protected final SpecialInputsHandler specialInputsHandler;
+  protected final CppSemantics cppSemantics;
 
   /**
    * Identifier for the actual execution time behavior of the action.
@@ -285,7 +284,7 @@ public class CppCompileAction extends AbstractAction
       ImmutableMap<String, String> environment,
       String actionName,
       RuleContext ruleContext,
-      CppSemantics semantics) {
+      CppSemantics cppSemantics) {
     super(
         owner,
         createInputs(
@@ -325,8 +324,8 @@ 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.semantics = semantics;
-    if (semantics.needsIncludeValidation()) {
+    this.cppSemantics = cppSemantics;
+    if (cppSemantics.needsIncludeValidation()) {
       verifyIncludePaths(ruleContext);
     }
     this.additionalIncludeScannables = ImmutableList.copyOf(additionalIncludeScannables);
@@ -354,6 +353,12 @@ public class CppCompileAction extends AbstractAction
         continue;
       }
 
+      // One starting ../ is okay for getting to a sibling repository.
+      PathFragment originalInclude = include;
+      if (include.startsWith(new PathFragment(Label.EXTERNAL_PATH_PREFIX))) {
+        include = include.relativeTo(Label.EXTERNAL_PATH_PREFIX);
+      }
+
       if (include.isAbsolute()
           || !PathFragment.EMPTY_FRAGMENT.getRelative(include).normalize().isNormalized()) {
         ruleContext.ruleError(
@@ -462,9 +467,6 @@ public class CppCompileAction extends AbstractAction
       throws ActionExecutionException, InterruptedException {
     Executor executor = actionExecutionContext.getExecutor();
     Collection<Artifact> initialResult;
-    if (!shouldScanIncludes) {
-      return null;
-    }
     try {
       initialResult = executor.getContext(actionContext)
           .findAdditionalInputs(this, actionExecutionContext);
@@ -946,105 +948,6 @@ 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, RepositoryName.MAIN);
-        }
-        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,
@@ -1096,7 +999,7 @@ public class CppCompileAction extends AbstractAction
     }
   }
 
-  private Map<PathFragment, Artifact> getAllowedDerivedInputsMap() {
+  protected Map<PathFragment, Artifact> getAllowedDerivedInputsMap() {
     Map<PathFragment, Artifact> allowedDerivedInputMap = new HashMap<>();
     addToMap(allowedDerivedInputMap, mandatoryInputs);
     addToMap(allowedDerivedInputMap, getDeclaredIncludeSrcs());
@@ -1219,9 +1122,10 @@ 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(
-            executor.getExecRoot(), scanningContext.getArtifactResolver(), reply);
+        discoverInputsFromDotdFiles(execRoot, 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
@@ -1239,14 +1143,64 @@ public class CppCompileAction extends AbstractAction
       updateActionInputs(discoveredInputs);
     }
 
-    // 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());
+    // 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));
+      }
     }
+    return systemIncludePrefixes;
   }
 
   /**
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 d7f7e83bc9..9878dd6bb0 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
@@ -502,7 +502,7 @@ public final class CppModel {
           initializeCompileAction(moduleMapArtifact, moduleMapLabel, /*forInterface=*/ true);
 
       builder.setSemantics(semantics);
-
+      
       // A header module compile action is just like a normal compile action, but:
       // - the compiled source file is the module map
       // - it creates a header module (.pcm file).
diff --git a/src/main/java/com/google/devtools/build/lib/rules/cpp/FakeCppCompileAction.java b/src/main/java/com/google/devtools/build/lib/rules/cpp/FakeCppCompileAction.java
index ad7d5fe735..572853fc7d 100644
--- a/src/main/java/com/google/devtools/build/lib/rules/cpp/FakeCppCompileAction.java
+++ b/src/main/java/com/google/devtools/build/lib/rules/cpp/FakeCppCompileAction.java
@@ -34,6 +34,7 @@ import com.google.devtools.build.lib.analysis.RuleContext;
 import com.google.devtools.build.lib.analysis.config.BuildConfiguration;
 import com.google.devtools.build.lib.cmdline.Label;
 import com.google.devtools.build.lib.collect.nestedset.NestedSet;
+import com.google.devtools.build.lib.collect.nestedset.NestedSetBuilder;
 import com.google.devtools.build.lib.concurrent.ThreadSafety.ThreadCompatible;
 import com.google.devtools.build.lib.events.Event;
 import com.google.devtools.build.lib.rules.cpp.CcToolchainFeatures.FeatureConfiguration;
@@ -41,6 +42,7 @@ import com.google.devtools.build.lib.util.Preconditions;
 import com.google.devtools.build.lib.util.ShellEscaper;
 import com.google.devtools.build.lib.util.io.FileOutErr;
 import com.google.devtools.build.lib.vfs.FileSystemUtils;
+import com.google.devtools.build.lib.vfs.Path;
 import com.google.devtools.build.lib.vfs.PathFragment;
 import java.io.IOException;
 import java.util.UUID;
@@ -79,7 +81,7 @@ public class FakeCppCompileAction extends CppCompileAction {
       ImmutableList<String> copts,
       Predicate<String> nocopts,
       RuleContext ruleContext,
-      CppSemantics semantics) {
+      CppSemantics cppSemantics) {
     super(
         owner,
         features,
@@ -117,7 +119,7 @@ public class FakeCppCompileAction extends CppCompileAction {
         ImmutableMap.<String, String>of(),
         CppCompileAction.CPP_COMPILE,
         ruleContext,
-        semantics);
+        cppSemantics);
     this.tempOutputFile = Preconditions.checkNotNull(tempOutputFile);
   }
 
@@ -148,9 +150,32 @@ public class FakeCppCompileAction extends CppCompileAction {
       }
     }
     IncludeScanningContext scanningContext = executor.getContext(IncludeScanningContext.class);
-    NestedSet<Artifact> discoveredInputs =
-        discoverInputsFromDotdFiles(
-            executor.getExecRoot(), scanningContext.getArtifactResolver(), reply);
+    Path execRoot = executor.getExecRoot();
+
+    NestedSet<Artifact> discoveredInputs;
+    if (getDotdFile() == null) {
+      discoveredInputs = NestedSetBuilder.<Artifact>stableOrder().build();
+    } else {
+      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();
+      }
+
+      discoveredInputs =
+          discoveryBuilder
+              .build()
+              .discoverInputsFromDotdFiles(execRoot, scanningContext.getArtifactResolver());
+    }
+     
     reply = null; // Clear in-memory .d files early.
 
     // Even cc_fake_binary rules need to properly declare their dependencies...
diff --git a/src/main/java/com/google/devtools/build/lib/rules/cpp/HeaderDiscovery.java b/src/main/java/com/google/devtools/build/lib/rules/cpp/HeaderDiscovery.java
index 512281ad93..e80856b7c2 100644
--- a/src/main/java/com/google/devtools/build/lib/rules/cpp/HeaderDiscovery.java
+++ b/src/main/java/com/google/devtools/build/lib/rules/cpp/HeaderDiscovery.java
@@ -36,6 +36,12 @@ import java.util.Map;
 /** Manages the process of obtaining inputs used in a compilation from .d files. */
 public class HeaderDiscovery {
 
+  /** Indicates if a compile should perform dotd pruning. */
+  public static enum DotdPruningMode {
+    USE,
+    DO_NOT_USE
+  }
+  
   private final Action action;
   private final Artifact sourceFile;
   private final DotdFile dotdFile;
@@ -46,7 +52,7 @@ public class HeaderDiscovery {
   private final DependencySet depSet;
   private final List<Path> permittedSystemIncludePrefixes;
   private final Map<PathFragment, Artifact> allowedDerivedInputsMap;
-
+  
   /**
    * Creates a HeaderDiscover instance
    *