Make include pruning work in Bazel.

Fixes #2372.

--
PiperOrigin-RevId: 145539067
MOS_MIGRATED_REVID=145539067

7 files changed, 178 insertions, 40 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 9a91c3c970..12cb38cb85 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
@@ -173,6 +173,18 @@ public class CppCompileAction extends AbstractAction
   private final Label sourceLabel;
   private final Artifact optionalSourceFile;
   private final NestedSet<Artifact> mandatoryInputs;
+
+  /**
+   * The set of include files that we unconditionally add to the inputs of the action (but they
+   * may be pruned after execution).
+   *
+   * <p>This is necessary because the inputs that can be pruned by .d file parsing must be returned
+   * from {@link #discoverInputs(ActionExecutionContext)} and they cannot be in
+   * {@link #mandatoryInputs}. Thus, even with include scanning turned off, we pretend that we
+   * "discover" these headers.
+   */
+  private final NestedSet<Artifact> includesExemptFromDiscovery;
+
   private final boolean shouldScanIncludes;
   private final boolean shouldPruneModules;
   private final boolean usePic;
@@ -210,13 +222,13 @@ public class CppCompileAction extends AbstractAction
    * Set when the action prepares for execution. Used to preserve state between preparation and
    * execution.
    */
-  private Collection<Artifact> additionalInputs = null;
+  private Iterable<Artifact> additionalInputs = null;
 
   /** Set when a two-stage input discovery is used. */
   private Collection<Artifact> usedModules = null;
 
   /** Used modules that are not transitively used through other topLevelModules. */
-  private Collection<Artifact> topLevelModules = null;
+  private Iterable<Artifact> topLevelModules = null;
 
   private CcToolchainFeatures.Variables overwrittenVariables = null;
 
@@ -274,6 +286,7 @@ public class CppCompileAction extends AbstractAction
       boolean useHeaderModules,
       Label sourceLabel,
       NestedSet<Artifact> mandatoryInputs,
+      NestedSet<Artifact> includesExemptFromDiscovery,
       Artifact outputFile,
       DotdFile dotdFile,
       @Nullable Artifact gcnoFile,
@@ -335,6 +348,7 @@ public class CppCompileAction extends AbstractAction
     // We do not need to include the middleman artifact since it is a generated
     // artifact and will definitely exist prior to this action execution.
     this.mandatoryInputs = mandatoryInputs;
+    this.includesExemptFromDiscovery = includesExemptFromDiscovery;
     this.builtinIncludeFiles = CppHelper.getToolchain(ruleContext).getBuiltinIncludeFiles();
     this.cppSemantics = cppSemantics;
     if (cppSemantics.needsIncludeValidation()) {
@@ -466,8 +480,8 @@ public class CppCompileAction extends AbstractAction
    * and clears the stored list. {@link #prepare} must be called before this method is called, on
    * each action execution.
    */
-  public Collection<? extends ActionInput> getAdditionalInputs() {
-    Collection<? extends ActionInput> result = Preconditions.checkNotNull(additionalInputs);
+  public Iterable<? extends ActionInput> getAdditionalInputs() {
+    Iterable<? extends ActionInput> result = Preconditions.checkNotNull(additionalInputs);
     additionalInputs = null;
     return result;
   }
@@ -482,12 +496,18 @@ public class CppCompileAction extends AbstractAction
     return true;
   }
 
+  @VisibleForTesting  // productionVisibility = Visibility.PRIVATE
+  public Iterable<Artifact> getPossibleInputsForTesting() {
+    return Iterables.concat(getInputs(), includesExemptFromDiscovery);
+  }
+
   @Nullable
   @Override
   public Iterable<Artifact> discoverInputs(ActionExecutionContext actionExecutionContext)
       throws ActionExecutionException, InterruptedException {
     Executor executor = actionExecutionContext.getExecutor();
     Collection<Artifact> initialResult;
+
     try {
       initialResult = executor.getContext(actionContext)
           .findAdditionalInputs(this, actionExecutionContext);
@@ -502,8 +522,9 @@ public class CppCompileAction extends AbstractAction
       return null;
     }
 
+    Set<Artifact> initialResultSet = Sets.newLinkedHashSet(initialResult);
+    Iterables.addAll(initialResultSet, includesExemptFromDiscovery);
     if (shouldPruneModules) {
-      Set<Artifact> initialResultSet = Sets.newLinkedHashSet(initialResult);
       usedModules = Sets.newLinkedHashSet();
       topLevelModules = null;
       for (CppCompilationContext.TransitiveModuleHeaders usedModule :
@@ -511,9 +532,9 @@ public class CppCompileAction extends AbstractAction
         usedModules.add(usedModule.getModule());
       }
       initialResultSet.addAll(usedModules);
-      initialResult = initialResultSet;
     }
 
+    initialResult = initialResultSet;
     this.additionalInputs = initialResult;
     // In some cases, execution backends need extra files for each included file. Add them
     // to the set of inputs the caller may need to be aware of.
@@ -579,14 +600,16 @@ public class CppCompileAction extends AbstractAction
 
   @Override
   public Iterable<Artifact> getInputsWhenSkippingInputDiscovery() {
+    NestedSetBuilder<Artifact> result = NestedSetBuilder.stableOrder();
     if (useHeaderModules) {
-      additionalInputs = Sets.newLinkedHashSet(context.getTransitiveModules(usePic).toCollection());
       // Here, we cannot really know what the top-level modules are, so we just mark all
       // transitive modules as "top level".
-      topLevelModules = additionalInputs;
-      return additionalInputs;
+      topLevelModules = Sets.newLinkedHashSet(context.getTransitiveModules(usePic).toCollection());
+      result.addTransitive(context.getTransitiveModules(usePic));
     }
-    return null;
+    result.addTransitive(includesExemptFromDiscovery);
+    additionalInputs = result.build();
+    return result.build();
   }
 
   @Override
@@ -856,7 +879,7 @@ public class CppCompileAction extends AbstractAction
     IncludeProblems errors = new IncludeProblems();
     IncludeProblems warnings = new IncludeProblems();
     Set<Artifact> allowedIncludes = new HashSet<>();
-    for (Artifact input : mandatoryInputs) {
+    for (Artifact input : Iterables.concat(mandatoryInputs, includesExemptFromDiscovery)) {
       if (input.isMiddlemanArtifact() || input.isTreeArtifact()) {
         artifactExpander.expand(input, allowedIncludes);
       }
@@ -1114,6 +1137,7 @@ public class CppCompileAction extends AbstractAction
   protected Map<PathFragment, Artifact> getAllowedDerivedInputsMap() {
     Map<PathFragment, Artifact> allowedDerivedInputMap = new HashMap<>();
     addToMap(allowedDerivedInputMap, mandatoryInputs);
+    addToMap(allowedDerivedInputMap, includesExemptFromDiscovery);
     addToMap(allowedDerivedInputMap, getDeclaredIncludeSrcs());
     addToMap(allowedDerivedInputMap, context.getTransitiveCompilationPrerequisites());
     addToMap(allowedDerivedInputMap, context.getTransitiveModules(usePic));
@@ -1223,6 +1247,10 @@ public class CppCompileAction extends AbstractAction
     for (Artifact input : getMandatoryInputs()) {
       f.addPath(input.getExecPath());
     }
+    f.addInt(0);
+    for (Artifact input : includesExemptFromDiscovery) {
+      f.addPath(input.getExecPath());
+    }
     return f.hexDigestAndReset();
   }
 
@@ -1253,18 +1281,7 @@ public class CppCompileAction extends AbstractAction
 
     // Post-execute "include scanning", which modifies the action inputs to match what the compile
     // action actually used by incorporating the results of .d file parsing.
-    //
-    // We enable this when "include scanning" itself is enabled, or when hdrs_check is set to loose
-    // or warn, as otherwise the action might be missing inputs that the compiler used and rebuilds
-    // become incorrect.
-    //
-    // Note that this effectively disables post-execute "include scanning" in Bazel, because
-    // hdrs_check is forced to "strict" and "include scanning" is forced to off.
-    boolean usesStrictHdrsChecks = context.getDeclaredIncludeDirs().isEmpty()
-        && context.getDeclaredIncludeWarnDirs().isEmpty();
-    if (shouldScanIncludes() || !usesStrictHdrsChecks) {
-      updateActionInputs(discoveredInputs);
-    }
+    updateActionInputs(discoveredInputs);
 
     // hdrs_check: This cannot be switched off, because doing so would allow for incorrect builds.
     validateInclusions(
diff --git a/src/main/java/com/google/devtools/build/lib/rules/cpp/CppCompileActionBuilder.java b/src/main/java/com/google/devtools/build/lib/rules/cpp/CppCompileActionBuilder.java
index 63a0ec1fd7..60ed762d1b 100644
--- a/src/main/java/com/google/devtools/build/lib/rules/cpp/CppCompileActionBuilder.java
+++ b/src/main/java/com/google/devtools/build/lib/rules/cpp/CppCompileActionBuilder.java
@@ -30,6 +30,7 @@ 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.collect.nestedset.Order;
 import com.google.devtools.build.lib.rules.cpp.CcToolchainFeatures.FeatureConfiguration;
 import com.google.devtools.build.lib.rules.cpp.CppCompileAction.DotdFile;
 import com.google.devtools.build.lib.rules.cpp.CppCompileAction.SpecialInputsHandler;
@@ -256,13 +257,9 @@ public class CppCompileActionBuilder {
             && featureConfiguration.isEnabled(CppRuleClasses.USE_HEADER_MODULES);
 
     boolean fake = tempOutputFile != null;
-
     // Configuration can be null in tests.
     NestedSetBuilder<Artifact> realMandatoryInputsBuilder = NestedSetBuilder.compileOrder();
     realMandatoryInputsBuilder.addTransitive(mandatoryInputsBuilder.build());
-    if (!fake && !shouldScanIncludes) {
-      realMandatoryInputsBuilder.addTransitive(context.getDeclaredIncludeSrcs());
-    }
     boolean shouldPruneModules =
         cppConfiguration.getPruneCppModules() && shouldScanIncludes && useHeaderModules;
     if (useHeaderModules && !shouldPruneModules) {
@@ -281,6 +278,12 @@ public class CppCompileActionBuilder {
           featureConfiguration.getToolForAction(getActionName()).getExecutionRequirements();
     }
 
+    NestedSet<Artifact> realMandatoryInputs = realMandatoryInputsBuilder.build();
+
+    NestedSet<Artifact> includesExemptFromDiscovery = shouldScanIncludes
+        ? NestedSetBuilder.<Artifact>emptySet(Order.STABLE_ORDER)
+        : context.getDeclaredIncludeSrcs();
+
     // Copying the collections is needed to make the builder reusable.
     if (fake) {
       return new FakeCppCompileAction(
@@ -294,7 +297,8 @@ public class CppCompileActionBuilder {
           usePic,
           useHeaderModules,
           sourceLabel,
-          realMandatoryInputsBuilder.build(),
+          realMandatoryInputs,
+          includesExemptFromDiscovery,
           outputFile,
           tempOutputFile,
           dotdFile,
@@ -307,8 +311,6 @@ public class CppCompileActionBuilder {
           ruleContext,
           cppSemantics);
     } else {
-      NestedSet<Artifact> realMandatoryInputs = realMandatoryInputsBuilder.build();
-
       return new CppCompileAction(
           owner,
           ImmutableList.copyOf(features),
@@ -321,6 +323,7 @@ public class CppCompileActionBuilder {
           useHeaderModules,
           sourceLabel,
           realMandatoryInputs,
+          includesExemptFromDiscovery,
           outputFile,
           dotdFile,
           gcnoFile,
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 3e99af5af1..a1b55f202c 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
@@ -72,6 +72,7 @@ public class FakeCppCompileAction extends CppCompileAction {
       boolean useHeaderModules,
       Label sourceLabel,
       NestedSet<Artifact> mandatoryInputs,
+      NestedSet<Artifact> includesExemptFromDiscovery,
       Artifact outputFile,
       PathFragment tempOutputFile,
       DotdFile dotdFile,
@@ -95,6 +96,7 @@ public class FakeCppCompileAction extends CppCompileAction {
         useHeaderModules,
         sourceLabel,
         mandatoryInputs,
+        includesExemptFromDiscovery,
         outputFile,
         dotdFile,
         null,
diff --git a/src/main/java/com/google/devtools/build/lib/rules/cpp/SpawnGccStrategy.java b/src/main/java/com/google/devtools/build/lib/rules/cpp/SpawnGccStrategy.java
index 24daa883f6..1025068f76 100644
--- a/src/main/java/com/google/devtools/build/lib/rules/cpp/SpawnGccStrategy.java
+++ b/src/main/java/com/google/devtools/build/lib/rules/cpp/SpawnGccStrategy.java
@@ -15,7 +15,10 @@
 package com.google.devtools.build.lib.rules.cpp;
 
 import com.google.common.collect.ImmutableList;
+import com.google.common.collect.ImmutableSet;
+import com.google.common.collect.Iterables;
 import com.google.devtools.build.lib.actions.ActionExecutionContext;
+import com.google.devtools.build.lib.actions.ActionInput;
 import com.google.devtools.build.lib.actions.Artifact;
 import com.google.devtools.build.lib.actions.BaseSpawn;
 import com.google.devtools.build.lib.actions.ExecException;
@@ -24,11 +27,10 @@ import com.google.devtools.build.lib.actions.Executor;
 import com.google.devtools.build.lib.actions.ResourceSet;
 import com.google.devtools.build.lib.actions.Spawn;
 import com.google.devtools.build.lib.actions.SpawnActionContext;
-
 import java.util.Collection;
 
 /**
- * A cpp strategy that simply passes everything through to the default spawn action strategy.
+ * A context for C++ compilation that calls into a {@link SpawnActionContext}.
  */
 @ExecutionStrategy(
   contextType = CppCompileActionContext.class,
@@ -36,6 +38,25 @@ import java.util.Collection;
 )
 public class SpawnGccStrategy implements CppCompileActionContext {
 
+  /**
+   * A {@link Spawn} that wraps a {@link CppCompileAction} and adds its
+   * {@code additionalInputs} (potential files included) to its inputs.
+   */
+  private static class GccSpawn extends BaseSpawn {
+    private final Iterable<? extends ActionInput> inputs;
+
+    public GccSpawn(CppCompileAction action, ResourceSet resources) {
+      super(action.getArgv(), action.getEnvironment(), action.getExecutionInfo(), action,
+          resources);
+      this.inputs = Iterables.concat(action.getInputs(), action.getAdditionalInputs());
+    }
+
+    @Override
+    public Iterable<? extends ActionInput> getInputFiles() {
+      return ImmutableSet.copyOf(inputs);
+    }
+  }
+
   @Override
   public Collection<Artifact> findAdditionalInputs(
       CppCompileAction action, ActionExecutionContext actionExecutionContext)
@@ -49,13 +70,7 @@ public class SpawnGccStrategy implements CppCompileActionContext {
       throws ExecException, InterruptedException {
     Executor executor = actionExecutionContext.getExecutor();
     SpawnActionContext spawnActionContext = executor.getSpawnActionContext(action.getMnemonic());
-    Spawn spawn =
-        new BaseSpawn(
-            action.getArgv(),
-            action.getEnvironment(),
-            action.getExecutionInfo(),
-            action,
-            estimateResourceConsumption(action));
+    Spawn spawn = new GccSpawn(action, estimateResourceConsumption(action));
     spawnActionContext.exec(spawn, actionExecutionContext);
     return null;
   }
diff --git a/src/main/java/com/google/devtools/build/lib/sandbox/SpawnHelpers.java b/src/main/java/com/google/devtools/build/lib/sandbox/SpawnHelpers.java
index 0abf331917..b5b920cc31 100644
--- a/src/main/java/com/google/devtools/build/lib/sandbox/SpawnHelpers.java
+++ b/src/main/java/com/google/devtools/build/lib/sandbox/SpawnHelpers.java
@@ -14,6 +14,7 @@
 
 package com.google.devtools.build.lib.sandbox;
 
+import com.google.common.collect.Iterables;
 import com.google.common.io.Files;
 import com.google.devtools.build.lib.actions.ActionExecutionContext;
 import com.google.devtools.build.lib.actions.ActionInput;
@@ -184,7 +185,7 @@ public final class SpawnHelpers {
     if (spawn.getResourceOwner() instanceof CppCompileAction) {
       CppCompileAction action = (CppCompileAction) spawn.getResourceOwner();
       if (action.shouldScanIncludes()) {
-        inputs.addAll(action.getAdditionalInputs());
+        Iterables.addAll(inputs, action.getAdditionalInputs());
       }
     }
 
diff --git a/src/test/shell/integration/BUILD b/src/test/shell/integration/BUILD
index 6f1bb7f34b..ac8f037d0e 100644
--- a/src/test/shell/integration/BUILD
+++ b/src/test/shell/integration/BUILD
@@ -192,6 +192,13 @@ sh_test(
 )
 
 sh_test(
+    name = "cpp_test",
+    size = "large",
+    srcs = ["cpp_test.sh"],
+    data = [":test-deps"],
+)
+
+sh_test(
     name = "action_env_test",
     size = "large",
     srcs = ["action_env_test.sh"],
diff --git a/src/test/shell/integration/cpp_test.sh b/src/test/shell/integration/cpp_test.sh
new file mode 100755
index 0000000000..0e8fd3ae3f
--- /dev/null
+++ b/src/test/shell/integration/cpp_test.sh
@@ -0,0 +1,93 @@
+#!/bin/bash
+#
+# Copyright 2016 The Bazel Authors. All rights reserved.
+#
+# Licensed under the Apache License, Version 2.0 (the "License");
+# you may not use this file except in compliance with the License.
+# You may obtain a copy of the License at
+#
+#    http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+#
+# Load the test setup defined in the parent directory
+CURRENT_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)"
+source "${CURRENT_DIR}/../integration_test_setup.sh" \
+  || { echo "integration_test_setup.sh not found!" >&2; exit 1; }
+
+#### SETUP #############################################################
+
+set -e
+
+#### TESTS #############################################################
+
+function test_no_rebuild_on_irrelevant_header_change() {
+  mkdir -p a
+  cat > a/BUILD <<EOF
+cc_binary(name="a", srcs=["a.cc"], deps=["b"])
+cc_library(name="b", srcs=["b1.h", "b2.h"])
+EOF
+
+  cat > a/a.cc <<EOF
+#include "a/b1.h"
+
+int main(void) {
+  return B_RETURN_VALUE;
+}
+EOF
+
+  cat > a/b1.h <<EOF
+#define B_RETURN_VALUE 31
+EOF
+
+  cat > a/b2.h <<EOF
+=== BANANA ===
+EOF
+
+  bazel build //a || fail "build failed"
+  echo "CHERRY" > a/b2.h
+  bazel build //a >& $TEST_log || fail "build failed"
+  expect_not_log "Compiling a/a.cc"
+}
+
+function test_new_header_is_required() {
+  mkdir -p a
+  cat > a/BUILD <<EOF
+cc_binary(name="a", srcs=["a.cc"], deps=[":b"])
+cc_library(name="b", srcs=["b1.h", "b2.h"])
+EOF
+
+  cat > a/a.cc << EOF
+#include "a/b1.h"
+
+int main(void) {
+    return B1;
+}
+EOF
+
+  cat > a/b1.h <<EOF
+#define B1 3
+EOF
+
+  cat > a/b2.h <<EOF
+#define B2 4
+EOF
+
+  bazel build //a || fail "build failed"
+  cat > a/a.cc << EOF
+#include "a/b1.h"
+#include "a/b2.h"
+
+int main(void) {
+    return B1 + B2;
+}
+EOF
+
+  bazel build //a || fail "build failled"
+}
+
+run_suite "Tests for Bazel's C++ rules"