Clear potentially stale state left in `CppCompileAction` from inputs discovery from a previous build.
`CppCompileAction` [stores intermediate state](https://github.com/bazelbuild/bazel/blob/b341802700484d11c775bf02d80f43ba3f33b218/src/main/java/com/google/devtools/build/lib/rules/cpp/CppCompileAction.java#L524) of inputs discovery in the action
itself to avoid quadratic reevaluation on missing Skyframe dependencies. The
state is later [cleared when the execution of the action starts](https://github.com/bazelbuild/bazel/blob/b341802700484d11c775bf02d80f43ba3f33b218/src/main/java/com/google/devtools/build/lib/rules/cpp/CppCompileAction.java#L1425). This
is principally a problematic approach since we are mutating an analysis time
object (action) during execution.
In particular, if we happen to start discovering inputs, but not execute the compile spawn, the state will be left in the action object. A later build, at a different version, would pick up the stale state and use it as discovered inputs leading to incorrect results (or failures) for such incremental build.
This is possible e.g. when running a build without `--keep_going` when a failure from another action interrupts the execution after inputs discovery or when the `ActionExecutionFunction` encountered a missing dependency in the middle of discovering inputs (e.g. it did not have [dependent modules available](https://github.com/bazelbuild/bazel/blob/b341802700484d11c775bf02d80f43ba3f33b218/src/main/java/com/google/devtools/build/lib/rules/cpp/CppCompileAction.java#L549-L551)).
Add a new method to call before the first invocation of `Action::discoverInputs` in each build. Add a boolean flag to detect the first call to `Action::discoverInputs` stored in the Skyframe state used by `ActionExecutionFunction` and use it to clear the potentially stale state.
PiperOrigin-RevId: 420386548
diff --git a/src/main/java/com/google/devtools/build/lib/actions/Action.java b/src/main/java/com/google/devtools/build/lib/actions/Action.java
index 5a737cf..9459f9a 100644
--- a/src/main/java/com/google/devtools/build/lib/actions/Action.java
+++ b/src/main/java/com/google/devtools/build/lib/actions/Action.java
@@ -203,6 +203,9 @@
NestedSet<Artifact> discoverInputs(ActionExecutionContext actionExecutionContext)
throws ActionExecutionException, InterruptedException;
+ /** Prepare for input discovery, called before the first call to {@link #discoverInputs}. */
+ default void prepareInputDiscovery() {}
+
/**
* Resets this action's inputs to a pre {@linkplain #discoverInputs input discovery} state.
*
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 92151ad..fc377ee 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
@@ -126,7 +126,7 @@
@VisibleForTesting public static final String CPP_COMPILE_MNEMONIC = "CppCompile";
@VisibleForTesting public static final String OBJC_COMPILE_MNEMONIC = "ObjcCompile";
- protected final Artifact outputFile;
+ final Artifact outputFile;
private final Artifact sourceFile;
private final CppConfiguration cppConfiguration;
private final NestedSet<Artifact> mandatoryInputs;
@@ -144,7 +144,7 @@
private final boolean shouldScanIncludes;
private final boolean usePic;
private final boolean useHeaderModules;
- protected final boolean needsIncludeValidation;
+ final boolean needsIncludeValidation;
private final CcCompilationContext ccCompilationContext;
private final ImmutableList<Artifact> builtinIncludeFiles;
@@ -175,6 +175,7 @@
private final ImmutableList<PathFragment> builtInIncludeDirectories;
+ // TODO(b/213594908): Make CppCompileAction immutable.
/**
* Set when the action prepares for execution. Used to preserve state between preparation and
* execution.
@@ -459,6 +460,14 @@
}
}
+ // TODO(b/213594908): Remove this method from Action interface once CppCompileAction is immutable.
+ @Override
+ public void prepareInputDiscovery() {
+ // Make sure to clear the additional inputs potentially left over from an old build (in case we
+ // ran discoverInputs, but not beginExecution).
+ clearAdditionalInputs();
+ }
+
/**
* This method returns null when a required SkyValue is missing and a Skyframe restart is
* required.
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/ActionExecutionFunction.java b/src/main/java/com/google/devtools/build/lib/skyframe/ActionExecutionFunction.java
index 37f78b2..d8bdedc 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/ActionExecutionFunction.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/ActionExecutionFunction.java
@@ -794,6 +794,10 @@
if (action.discoversInputs()) {
Duration discoveredInputsDuration = Duration.ZERO;
if (state.discoveredInputs == null) {
+ if (!state.preparedInputDiscovery) {
+ action.prepareInputDiscovery();
+ state.preparedInputDiscovery = true;
+ }
try (SilentCloseable c = Profiler.instance().profile(ProfilerTask.INFO, "discoverInputs")) {
state.discoveredInputs =
skyframeActionExecutor.discoverInputs(
@@ -1390,6 +1394,7 @@
Token token = null;
NestedSet<Artifact> discoveredInputs = null;
FileSystem actionFileSystem = null;
+ boolean preparedInputDiscovery = false;
/**
* Stores the ArtifactNestedSetKeys created from the inputs of this actions. Objective: avoid
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/ActionExecutionValue.java b/src/main/java/com/google/devtools/build/lib/skyframe/ActionExecutionValue.java
index 65ee2fa..d75e6fd 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/ActionExecutionValue.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/ActionExecutionValue.java
@@ -123,6 +123,16 @@
artifactData, treeArtifactData, outputSymlinks, /*discoveredModules=*/ null);
}
+ @VisibleForTesting
+ public static ActionExecutionValue createForTesting(
+ ImmutableMap<Artifact, FileArtifactValue> artifactData,
+ ImmutableMap<Artifact, TreeArtifactValue> treeArtifactData,
+ @Nullable ImmutableList<FilesetOutputSymlink> outputSymlinks,
+ NestedSet<Artifact> discoveredModules) {
+ return new ActionExecutionValue(
+ artifactData, treeArtifactData, outputSymlinks, discoveredModules);
+ }
+
/**
* Retrieves a {@link FileArtifactValue} for a regular (non-tree) derived artifact.
*
