From ef6f4cff9ee3e0c92b61f59ca0585f63ff17e9a4 Mon Sep 17 00:00:00 2001 From: shahan Date: Tue, 26 Jun 2018 11:24:59 -0700 Subject: Instead of depending on mutable inputs of upstream CppCompileAction instances, CppCompileAction.discoverInputsStage2 retrieves values of discovered modules from ActionExecutionValue. This addresses a possible a correctness issue. PiperOrigin-RevId: 202162180 --- .../devtools/build/lib/actions/AbstractAction.java | 7 +- .../com/google/devtools/build/lib/rules/cpp/BUILD | 1 + .../build/lib/rules/cpp/CppCompileAction.java | 277 +++++++++++---------- .../build/lib/rules/cpp/IncludeScannable.java | 3 + .../lib/skyframe/ActionExecutionFunction.java | 6 + .../build/lib/skyframe/ActionExecutionValue.java | 28 ++- .../build/lib/skyframe/SkyframeActionExecutor.java | 4 + 7 files changed, 183 insertions(+), 143 deletions(-) (limited to 'src/main') diff --git a/src/main/java/com/google/devtools/build/lib/actions/AbstractAction.java b/src/main/java/com/google/devtools/build/lib/actions/AbstractAction.java index d5dbb8b686..44db64bf46 100644 --- a/src/main/java/com/google/devtools/build/lib/actions/AbstractAction.java +++ b/src/main/java/com/google/devtools/build/lib/actions/AbstractAction.java @@ -206,9 +206,8 @@ public abstract class AbstractAction implements Action, ActionApi { } /** - * Should be called when the inputs of the action become known, that is, either during - * {@link #discoverInputs(ActionExecutionContext)} or during - * {@link #execute(ActionExecutionContext)}. + * Should be called when the inputs of the action become known, that is, either during {@link + * #discoverInputs(ActionExecutionContext)} or during {@link #execute(ActionExecutionContext)}. * *

When an action discovers inputs, it must have been called by the time {@code #execute()} * returns. It can be called both during {@code discoverInputs} and during {@code execute()}. @@ -217,7 +216,7 @@ public abstract class AbstractAction implements Action, ActionApi { * itself when an action is loaded from the on-disk action cache. */ @Override - public final synchronized void updateInputs(Iterable inputs) { + public synchronized void updateInputs(Iterable inputs) { this.inputs = CollectionUtils.makeImmutable(inputs); inputsDiscovered = true; } diff --git a/src/main/java/com/google/devtools/build/lib/rules/cpp/BUILD b/src/main/java/com/google/devtools/build/lib/rules/cpp/BUILD index 034f816261..b1bb9cafc6 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/cpp/BUILD +++ b/src/main/java/com/google/devtools/build/lib/rules/cpp/BUILD @@ -61,6 +61,7 @@ java_library( "//src/main/java/com/google/devtools/build/lib/actions", "//src/main/java/com/google/devtools/build/lib/collect/nestedset", "//src/main/java/com/google/devtools/build/lib/vfs", + "//third_party:guava", "//third_party:jsr305", ], ) 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 3f79b6cb52..7640a1ca2f 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 @@ -28,6 +28,7 @@ import com.google.devtools.build.lib.actions.ActionEnvironment; import com.google.devtools.build.lib.actions.ActionExecutionContext; import com.google.devtools.build.lib.actions.ActionExecutionException; import com.google.devtools.build.lib.actions.ActionKeyContext; +import com.google.devtools.build.lib.actions.ActionLookupData; import com.google.devtools.build.lib.actions.ActionLookupValue; import com.google.devtools.build.lib.actions.ActionLookupValue.ActionLookupKey; import com.google.devtools.build.lib.actions.ActionOwner; @@ -58,8 +59,7 @@ import com.google.devtools.build.lib.rules.cpp.CcCommon.CoptsFilter; import com.google.devtools.build.lib.rules.cpp.CcToolchainFeatures.FeatureConfiguration; import com.google.devtools.build.lib.rules.cpp.CppCompileActionContext.Reply; import com.google.devtools.build.lib.rules.cpp.CppHelper.PregreppedHeader; -import com.google.devtools.build.lib.skyframe.serialization.autocodec.AutoCodec; -import com.google.devtools.build.lib.skyframe.serialization.autocodec.AutoCodec.VisibleForSerialization; +import com.google.devtools.build.lib.skyframe.ActionExecutionValue; import com.google.devtools.build.lib.util.DependencySet; import com.google.devtools.build.lib.util.Fingerprint; import com.google.devtools.build.lib.util.ShellEscaper; @@ -152,14 +152,36 @@ public class CppCompileAction extends AbstractAction */ private Iterable additionalInputs = null; - /** Set when a two-stage input discovery is used. */ - private Collection usedModules = null; + /** + * Set when a two-stage input discovery is used. + * + *

Used only during action execution. + */ + private Set usedModules = null; /** Used modules that are not transitively used through other topLevelModules. */ private Iterable topLevelModules = null; private CcToolchainVariables overwrittenVariables = null; + /** + * Set when a two-stage input discovery is used. + * + *

This field is used in the following scenarios. + * + *

+ */ + // TODO(djasper): investigate releasing memory used by this field as early as possible, for + // example, by including these values in additionalInputs. + private ImmutableSet discoveredModules = null; + /** * Creates a new action to compile C/C++ source files. * @@ -223,7 +245,7 @@ public class CppCompileAction extends AbstractAction CppSemantics cppSemantics, CcToolchainProvider cppProvider, @Nullable Artifact grepIncludes) { - this( + super( owner, allInputs, CollectionUtils.asSetWithoutNulls( @@ -232,89 +254,19 @@ public class CppCompileAction extends AbstractAction gcnoFile, dwoFile, ltoIndexingFile), - env, - Preconditions.checkNotNull(outputFile), - sourceFile, - // We do not need to include the middleman artifact since it is a generated - // artifact and will definitely exist prior to this action execution. - mandatoryInputs, - inputsForInvalidation, - prunableHeaders, - // inputsKnown begins as the logical negation of shouldScanIncludes. - // When scanning includes, the inputs begin as not known, and become - // known after inclusion scanning. When *not* scanning includes, - // the inputs are as declared, hence known, and remain so. - shouldScanIncludes, - shouldPruneModules, - pruneCppInputDiscovery, - usePic, - useHeaderModules, - isStrictSystemIncludes, - ccCompilationContext, - builtinIncludeFiles, - ImmutableList.copyOf(additionalIncludeScanningRoots), - CompileCommandLine.builder(sourceFile, coptsFilter, actionName, dotdFile) - .setFeatureConfiguration(featureConfiguration) - .setVariables(variables) - .build(), - executionInfo, - actionName, - featureConfiguration, - actionClassId, - shouldScanIncludes || cppSemantics.needsDotdInputPruning(), - ImmutableList.copyOf(cppProvider.getBuiltInIncludeDirectories()), - /* additionalInputs= */ null, - /* usedModules= */ null, - /* topLevelModules= */ null, - /* overwrittenVariables= */ null, - cppSemantics.needsDotdInputPruning(), - cppSemantics.needsIncludeValidation(), - cppSemantics.getIncludeProcessing(), - grepIncludes); + env); Preconditions.checkArgument(!shouldPruneModules || shouldScanIncludes); - } - - @VisibleForSerialization - CppCompileAction( - ActionOwner owner, - NestedSet inputs, - ImmutableSet outputs, - ActionEnvironment env, - Artifact outputFile, - Artifact sourceFile, - NestedSet mandatoryInputs, - Iterable inputsForInvalidation, - NestedSet prunableHeaders, - boolean shouldScanIncludes, - boolean shouldPruneModules, - boolean pruneCppInputDiscovery, - boolean usePic, - boolean useHeaderModules, - boolean isStrictSystemIncludes, - CcCompilationContext ccCompilationContext, - ImmutableList builtinIncludeFiles, - ImmutableList additionalIncludeScanningRoots, - CompileCommandLine compileCommandLine, - ImmutableMap executionInfo, - String actionName, - FeatureConfiguration featureConfiguration, - UUID actionClassId, - boolean discoversInputs, - ImmutableList builtInIncludeDirectories, - Iterable additionalInputs, - Collection usedModules, - Iterable topLevelModules, - CcToolchainVariables overwrittenVariables, - boolean needsDotdInputPruning, - boolean needsIncludeValidation, - IncludeProcessing includeProcessing, - @Nullable Artifact grepIncludes) { - super(owner, inputs, outputs, env); - this.outputFile = outputFile; + this.outputFile = Preconditions.checkNotNull(outputFile); this.sourceFile = sourceFile; + // 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.inputsForInvalidation = inputsForInvalidation; this.prunableHeaders = prunableHeaders; + // inputsKnown begins as the logical negation of shouldScanIncludes. + // When scanning includes, the inputs begin as not known, and become + // known after inclusion scanning. When *not* scanning includes, + // the inputs are as declared, hence known, and remain so. this.shouldScanIncludes = shouldScanIncludes; this.shouldPruneModules = shouldPruneModules; this.pruneCppInputDiscovery = pruneCppInputDiscovery; @@ -323,21 +275,26 @@ public class CppCompileAction extends AbstractAction this.isStrictSystemIncludes = isStrictSystemIncludes; this.ccCompilationContext = ccCompilationContext; this.builtinIncludeFiles = builtinIncludeFiles; - this.additionalIncludeScanningRoots = additionalIncludeScanningRoots; - this.compileCommandLine = compileCommandLine; + this.additionalIncludeScanningRoots = ImmutableList.copyOf(additionalIncludeScanningRoots); + this.compileCommandLine = + CompileCommandLine.builder(sourceFile, coptsFilter, actionName, dotdFile) + .setFeatureConfiguration(featureConfiguration) + .setVariables(variables) + .build(); this.executionInfo = executionInfo; this.actionName = actionName; this.featureConfiguration = featureConfiguration; - this.needsDotdInputPruning = needsDotdInputPruning; - this.needsIncludeValidation = needsIncludeValidation; - this.includeProcessing = includeProcessing; + this.needsDotdInputPruning = cppSemantics.needsDotdInputPruning(); + this.needsIncludeValidation = cppSemantics.needsIncludeValidation(); + this.includeProcessing = cppSemantics.getIncludeProcessing(); this.actionClassId = actionClassId; - this.discoversInputs = discoversInputs; - this.builtInIncludeDirectories = builtInIncludeDirectories; - this.additionalInputs = additionalInputs; - this.usedModules = usedModules; - this.topLevelModules = topLevelModules; - this.overwrittenVariables = overwrittenVariables; + this.discoversInputs = shouldScanIncludes || cppSemantics.needsDotdInputPruning(); + this.builtInIncludeDirectories = + ImmutableList.copyOf(cppProvider.getBuiltInIncludeDirectories()); + this.additionalInputs = null; + this.usedModules = null; + this.topLevelModules = null; + this.overwrittenVariables = null; this.grepIncludes = grepIncludes; } @@ -459,51 +416,31 @@ public class CppCompileAction extends AbstractAction return additionalInputs; } + /** @return null when either {@link usedModules} was null or on Skyframe lookup failure */ + @Nullable @Override public Iterable discoverInputsStage2(SkyFunction.Environment env) throws InterruptedException { if (this.usedModules == null) { return null; } - Map skyKeys = new HashMap<>(); - for (Artifact artifact : this.usedModules) { - skyKeys.put(artifact, (ActionLookupKey) artifact.getArtifactOwner()); - } - Map skyValues = env.getValues(skyKeys.values()); - Set additionalModules = Sets.newLinkedHashSet(); - for (Artifact artifact : this.usedModules) { - SkyKey skyKey = skyKeys.get(artifact); - ActionLookupValue value = (ActionLookupValue) skyValues.get(skyKey); - Preconditions.checkNotNull( - value, "Owner %s of %s not in graph %s", artifact.getArtifactOwner(), artifact, skyKey); - // We can get the generating action here because #canRemoveAfterExecution is overridden. - Preconditions.checkState( - artifact.isFileType(CppFileTypes.CPP_MODULE), - "Non-module? %s (%s %s)", - artifact, - this, - value); - CppCompileAction action = - (CppCompileAction) value.getGeneratingActionDangerousReadJavadoc(artifact); - for (Artifact input : action.getInputs()) { - if (input.isFileType(CppFileTypes.CPP_MODULE)) { - additionalModules.add(input); - } - } + + Set additionalModules = computeTransitivelyUsedModules(env, usedModules); + if (additionalModules == null) { + return null; } + + this.discoveredModules = + new ImmutableSet.Builder().addAll(usedModules).addAll(additionalModules).build(); + ImmutableSet.Builder topLevelModules = ImmutableSet.builder(); for (Artifact artifact : this.usedModules) { if (!additionalModules.contains(artifact)) { topLevelModules.add(artifact); } } - this.topLevelModules = topLevelModules.build(); - this.additionalInputs = - new ImmutableList.Builder() - .addAll(this.additionalInputs) - .addAll(additionalModules) - .build(); this.usedModules = null; + this.topLevelModules = topLevelModules.build(); return additionalModules; } @@ -562,6 +499,13 @@ public class CppCompileAction extends AbstractAction return grepIncludes; } + /** Set by {@link discoverInputsStage2} */ + @Override + @Nullable + public ImmutableSet getDiscoveredModules() { + return discoveredModules; + } + /** * Returns the path where gcc should put the discovered dependency * information. @@ -910,7 +854,7 @@ public class CppCompileAction extends AbstractAction inputs.addTransitive(mandatoryInputs); inputs.addAll(inputsForInvalidation); inputs.addTransitive(discoveredInputs); - updateInputs(inputs.build()); + super.updateInputs(inputs.build()); } } @@ -959,6 +903,24 @@ public class CppCompileAction extends AbstractAction return unmodifiableSet(result); } + /** + * Called by {@link com.google.devtools.build.lib.actions.ActionCacheChecker} + * + *

Restores the value of {@link discoveredModules}, which is used to create the {@link + * com.google.devtools.build.lib.skyframe.ActionExecutionValue} after an action cache hit. + */ + @Override + public synchronized void updateInputs(Iterable inputs) { + super.updateInputs(inputs); + ImmutableSet.Builder discoveredModules = ImmutableSet.builder(); + for (Artifact input : inputs) { + if (input.isFileType(CppFileTypes.CPP_MODULE)) { + discoveredModules.add(input); + } + } + this.discoveredModules = discoveredModules.build(); + } + private static void addNonSources(HashSet result, Iterable artifacts) { for (Artifact a : artifacts) { if (!a.isSourceArtifact()) { @@ -1289,6 +1251,61 @@ public class CppCompileAction extends AbstractAction return compileCommandLine; } + /** + * For the given {@code usedModules}, looks up modules discovered by their generating actions. + * + *

The returned value contains elements of {@code usedModules}. It can be null when skyframe + * lookups return null. + */ + @Nullable + private static Set computeTransitivelyUsedModules( + SkyFunction.Environment env, Set usedModules) throws InterruptedException { + // ActionLookupKey → ActionLookupValue + Map actionLookupValues = + env.getValues( + Iterables.transform( + usedModules, module -> (ActionLookupKey) module.getArtifactOwner())); + ArrayList executionValueLookups = new ArrayList<>(usedModules.size()); + for (Artifact module : usedModules) { + ActionLookupData lookupData = lookupDataFromModule(actionLookupValues, module); + if (lookupData == null) { + return null; + } + executionValueLookups.add(lookupData); + } + + Set additionalModules = Sets.newLinkedHashSet(); + // ActionLookupData → ActionExecutionValue + Map actionExecutionValues = env.getValues(executionValueLookups); + for (ActionLookupData lookup : executionValueLookups) { + ActionExecutionValue value = (ActionExecutionValue) actionExecutionValues.get(lookup); + if (value == null) { + return null; + } + additionalModules.addAll(value.getDiscoveredModules()); + } + return additionalModules; + } + + @Nullable + private static ActionLookupData lookupDataFromModule( + Map actionLookupValues, Artifact module) { + ActionLookupKey lookupKey = (ActionLookupKey) module.getArtifactOwner(); + ActionLookupValue lookupValue = (ActionLookupValue) actionLookupValues.get(lookupKey); + if (lookupValue == null) { + return null; + } + Preconditions.checkState( + module.isFileType(CppFileTypes.CPP_MODULE), "Non-module? %s (%s)", module, lookupValue); + return ActionLookupData.create( + lookupKey, + Preconditions.checkNotNull( + lookupValue.getGeneratingActionIndex(module), + "%s missing action index for module %s", + lookupValue, + module)); + } + /** * A reference to a .d file. There are two modes: * @@ -1297,7 +1314,6 @@ public class CppCompileAction extends AbstractAction *

  • just an execPath that refers to a virtual .d file that is not written to disk * */ - @AutoCodec public static class DotdFile { private final Artifact artifact; private final PathFragment execPath; @@ -1312,13 +1328,6 @@ public class CppCompileAction extends AbstractAction this.execPath = execPath; } - @AutoCodec.Instantiator - @VisibleForSerialization - DotdFile(Artifact artifact, PathFragment execPath) { - this.artifact = artifact; - this.execPath = execPath; - } - /** * @return the Artifact or null */ diff --git a/src/main/java/com/google/devtools/build/lib/rules/cpp/IncludeScannable.java b/src/main/java/com/google/devtools/build/lib/rules/cpp/IncludeScannable.java index c9dbcd120e..f35a34bfb2 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/cpp/IncludeScannable.java +++ b/src/main/java/com/google/devtools/build/lib/rules/cpp/IncludeScannable.java @@ -14,6 +14,7 @@ package com.google.devtools.build.lib.rules.cpp; +import com.google.common.collect.ImmutableSet; import com.google.devtools.build.lib.actions.Artifact; import com.google.devtools.build.lib.collect.nestedset.NestedSet; import com.google.devtools.build.lib.vfs.PathFragment; @@ -113,4 +114,6 @@ public interface IncludeScannable { * Returns an artifact that is the executable for {@link ExtractInclusionAction}. */ Artifact getGrepIncludes(); + + ImmutableSet getDiscoveredModules(); } 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 ae34b1b945..611e3f2059 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 @@ -413,6 +413,9 @@ public class ActionExecutionFunction implements SkyFunction, CompletionReceiver metadataHandler.getOutputTreeArtifactData(), metadataHandler.getAdditionalOutputData(), /*outputSymlinks=*/ null, + (action instanceof IncludeScannable) + ? ((IncludeScannable) action).getDiscoveredModules() + : null, action instanceof NotifyOnActionCacheHit); } @@ -454,6 +457,9 @@ public class ActionExecutionFunction implements SkyFunction, CompletionReceiver // available. if (state.discoveredInputsStage2 == null) { state.discoveredInputsStage2 = action.discoverInputsStage2(env); + if (env.valuesMissing()) { + return null; + } } if (state.discoveredInputsStage2 != null) { addDiscoveredInputs( 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 37f8b2031f..a1feff8bd9 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 @@ -19,6 +19,7 @@ import com.google.common.base.Objects; import com.google.common.base.Preconditions; import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableMap; +import com.google.common.collect.ImmutableSet; import com.google.common.collect.Maps; import com.google.devtools.build.lib.actions.ActionLookupData; import com.google.devtools.build.lib.actions.ActionLookupValue; @@ -73,6 +74,8 @@ public class ActionExecutionValue implements SkyValue { @Nullable private final ImmutableList outputSymlinks; + @Nullable private final ImmutableSet discoveredModules; + /** * @param artifactData Map from Artifacts to corresponding FileValues. * @param treeArtifactData All tree artifact data. @@ -82,16 +85,19 @@ public class ActionExecutionValue implements SkyValue { * data are not used by the {@link FilesystemValueChecker} to invalidate * ActionExecutionValues. * @param outputSymlinks This represents the SymlinkTree which is the output of a fileset action. + * @param discoveredModules cpp modules discovered */ private ActionExecutionValue( Map artifactData, Map treeArtifactData, Map additionalOutputData, - @Nullable ImmutableList outputSymlinks) { + @Nullable ImmutableList outputSymlinks, + @Nullable ImmutableSet discoveredModules) { this.artifactData = ImmutableMap.copyOf(artifactData); this.additionalOutputData = ImmutableMap.copyOf(additionalOutputData); this.treeArtifactData = ImmutableMap.copyOf(treeArtifactData); this.outputSymlinks = outputSymlinks; + this.discoveredModules = discoveredModules; } static ActionExecutionValue create( @@ -99,12 +105,17 @@ public class ActionExecutionValue implements SkyValue { Map treeArtifactData, Map additionalOutputData, @Nullable ImmutableList outputSymlinks, + @Nullable ImmutableSet discoveredModules, boolean notifyOnActionCacheHitAction) { return notifyOnActionCacheHitAction ? new CrossServerUnshareableActionExecutionValue( - artifactData, treeArtifactData, additionalOutputData, outputSymlinks) + artifactData, treeArtifactData, additionalOutputData, outputSymlinks, discoveredModules) : new ActionExecutionValue( - artifactData, treeArtifactData, additionalOutputData, outputSymlinks); + artifactData, + treeArtifactData, + additionalOutputData, + outputSymlinks, + discoveredModules); } /** @@ -155,6 +166,11 @@ public class ActionExecutionValue implements SkyValue { return outputSymlinks; } + @Nullable + public ImmutableSet getDiscoveredModules() { + return discoveredModules; + } + /** * @param lookupKey A {@link SkyKey} whose argument is an {@code ActionLookupKey}, whose * corresponding {@code ActionLookupValue} contains the action to be executed. @@ -224,8 +240,10 @@ public class ActionExecutionValue implements SkyValue { Map artifactData, Map treeArtifactData, Map additionalOutputData, - @Nullable ImmutableList outputSymlinks) { - super(artifactData, treeArtifactData, additionalOutputData, outputSymlinks); + @Nullable ImmutableList outputSymlinks, + @Nullable ImmutableSet discoveredModules) { + super( + artifactData, treeArtifactData, additionalOutputData, outputSymlinks, discoveredModules); } } } diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeActionExecutor.java b/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeActionExecutor.java index a415e4859f..a42fc65807 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeActionExecutor.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeActionExecutor.java @@ -79,6 +79,7 @@ import com.google.devtools.build.lib.events.Reporter; import com.google.devtools.build.lib.profiler.Profiler; import com.google.devtools.build.lib.profiler.ProfilerTask; import com.google.devtools.build.lib.profiler.SilentCloseable; +import com.google.devtools.build.lib.rules.cpp.IncludeScannable; import com.google.devtools.build.lib.util.Pair; import com.google.devtools.build.lib.util.io.FileOutErr; import com.google.devtools.build.lib.util.io.OutErr; @@ -773,6 +774,9 @@ public final class SkyframeActionExecutor { metadataHandler.getOutputTreeArtifactData(), metadataHandler.getAdditionalOutputData(), actionExecutionContext.getOutputSymlinks(), + (action instanceof IncludeScannable) + ? ((IncludeScannable) action).getDiscoveredModules() + : null, action instanceof NotifyOnActionCacheHit); } } -- cgit v1.2.3