Skip to content

Commit

Permalink
Use syscall cache for loose headers check in CppCompileAction.
Browse files Browse the repository at this point in the history 
Admittedly, the action shouldn't access the file system at this point at all,
but this is a rather easy fix to avoid pathological cases.

PiperOrigin-RevId: 373131861
  • Loading branch information
@meisterT @copybara-github
meisterT authored and copybara-github committed May 11, 2021
1 parent caaf905 commit d05f92e
Show file tree
Hide file tree
Showing 18 changed files with 103 additions and 30 deletions.
30 changes: 24 additions & 6 deletions src/main/java/com/google/devtools/build/lib/actions/ActionExecutionContext.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@
import com.google.devtools.build.lib.vfs.FileSystem;
import com.google.devtools.build.lib.vfs.Path;
import com.google.devtools.build.lib.vfs.Root;
import com.google.devtools.build.lib.vfs.UnixGlob.FilesystemCalls;
import com.google.devtools.build.skyframe.SkyFunction.Environment;
import com.google.devtools.common.options.OptionsProvider;
import java.io.Closeable;
Expand Down Expand Up @@ -74,6 +75,7 @@ public enum ShowSubcommands {

private final ArtifactPathResolver pathResolver;
private final NestedSetExpander nestedSetExpander;
private final FilesystemCalls syscalls;

private ActionExecutionContext(
Executor executor,
Expand All @@ -91,7 +93,8 @@ private ActionExecutionContext(
@Nullable Environment env,
@Nullable FileSystem actionFileSystem,
@Nullable Object skyframeDepsResult,
NestedSetExpander nestedSetExpander) {
NestedSetExpander nestedSetExpander,
FilesystemCalls syscalls) {
this.actionInputFileCache = actionInputFileCache;
this.actionInputPrefetcher = actionInputPrefetcher;
this.actionKeyContext = actionKeyContext;
Expand All @@ -111,6 +114,7 @@ private ActionExecutionContext(
// executor is only ever null in testing.
executor == null ? null : executor.getExecRoot());
this.nestedSetExpander = nestedSetExpander;
this.syscalls = syscalls;
}

public ActionExecutionContext(
Expand All @@ -128,7 +132,8 @@ public ActionExecutionContext(
ArtifactExpander artifactExpander,
@Nullable FileSystem actionFileSystem,
@Nullable Object skyframeDepsResult,
NestedSetExpander nestedSetExpander) {
NestedSetExpander nestedSetExpander,
FilesystemCalls syscalls) {
this(
executor,
actionInputFileCache,
Expand All @@ -145,7 +150,8 @@ public ActionExecutionContext(
/*env=*/ null,
actionFileSystem,
skyframeDepsResult,
nestedSetExpander);
nestedSetExpander,
syscalls);
}

public static ActionExecutionContext forInputDiscovery(
Expand All @@ -161,7 +167,8 @@ public static ActionExecutionContext forInputDiscovery(
Map<String, String> clientEnv,
Environment env,
@Nullable FileSystem actionFileSystem,
NestedSetExpander nestedSetExpander) {
NestedSetExpander nestedSetExpander,
FilesystemCalls syscalls) {
return new ActionExecutionContext(
executor,
actionInputFileCache,
Expand All @@ -178,7 +185,8 @@ public static ActionExecutionContext forInputDiscovery(
env,
actionFileSystem,
/*skyframeDepsResult=*/ null,
nestedSetExpander);
nestedSetExpander,
syscalls);
}

public ActionInputPrefetcher getActionInputPrefetcher() {
Expand Down Expand Up @@ -353,6 +361,15 @@ public NestedSetExpander getNestedSetExpander() {
return nestedSetExpander;
}

/**
* This only exists for loose header checking (and shouldn't be exist at all).
*
* <p>Do NOT use from any other place.
*/
public FilesystemCalls getSyscalls() {
return syscalls;
}

@Override
public void close() throws IOException {
// Ensure that we close both fileOutErr and actionFileSystem even if one throws.
Expand Down Expand Up @@ -386,7 +403,8 @@ public ActionExecutionContext withFileOutErr(FileOutErr fileOutErr) {
env,
actionFileSystem,
skyframeDepsResult,
nestedSetExpander);
nestedSetExpander,
syscalls);
}

/**
Expand Down
18 changes: 17 additions & 1 deletion src/main/java/com/google/devtools/build/lib/rules/cpp/CppCompileAction.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -86,11 +86,13 @@
import com.google.devtools.build.lib.util.Fingerprint;
import com.google.devtools.build.lib.util.ShellEscaper;
import com.google.devtools.build.lib.util.io.FileOutErr;
import com.google.devtools.build.lib.vfs.FileStatus;
import com.google.devtools.build.lib.vfs.FileSystemUtils;
import com.google.devtools.build.lib.vfs.IORuntimeException;
import com.google.devtools.build.lib.vfs.Path;
import com.google.devtools.build.lib.vfs.PathFragment;
import com.google.devtools.build.lib.vfs.Root;
import com.google.devtools.build.lib.vfs.Symlinks;
import com.google.devtools.build.skyframe.SkyFunction;
import com.google.devtools.build.skyframe.SkyKey;
import com.google.devtools.build.skyframe.SkyValue;
Expand Down Expand Up @@ -1152,7 +1154,21 @@ private static boolean isDeclaredIn(
}
if (declaredIncludeDirs.contains(root.relativize(dir))) {
for (Path dirOrPackage : packagesToCheckForBuildFiles) {
if (dirOrPackage.getRelative(BUILD_PATH_FRAGMENT).exists()) {
FileStatus fileStatus = null;
try {
// This file system access shouldn't exist at all and has to go away when this is
// rewritten in Starlark.
// TODO(b/187366935): Consider globbing everything eagerly instead.
fileStatus =
actionExecutionContext
.getSyscalls()
.statIfFound(dirOrPackage.getRelative(BUILD_PATH_FRAGMENT), Symlinks.FOLLOW);
} catch (IOException e) {
// Previously, we used Path.exists() to check whether the BUILD file exists. This did
// return false on any error. So by ignoring the exception are maintaining current
// behaviour.
}
if (fileStatus != null && fileStatus.isFile()) {
return false; // Bad: this is a sub-package, not a subdir of a declared package.
}
}
Expand Down
18 changes: 13 additions & 5 deletions src/main/java/com/google/devtools/build/lib/skyframe/SkyframeActionExecutor.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -108,6 +108,7 @@
import com.google.devtools.build.lib.vfs.PathFragment;
import com.google.devtools.build.lib.vfs.Root;
import com.google.devtools.build.lib.vfs.Symlinks;
import com.google.devtools.build.lib.vfs.UnixGlob.FilesystemCalls;
import com.google.devtools.build.skyframe.SkyFunction.Environment;
import com.google.devtools.build.skyframe.SkyKey;
import com.google.devtools.common.options.OptionsProvider;
Expand Down Expand Up @@ -158,6 +159,7 @@ public void injectTree(SpecialArtifact output, TreeArtifactValue tree) {
private final ActionKeyContext actionKeyContext;
private final MetadataConsumerForMetrics outputArtifactsSeen;
private final MetadataConsumerForMetrics outputArtifactsFromActionCache;
private final AtomicReference<FilesystemCalls> syscalls;
private Reporter reporter;
private Map<String, String> clientEnv = ImmutableMap.of();
private Executor executorEngine;
Expand Down Expand Up @@ -236,13 +238,15 @@ private enum DirectoryState {
MetadataConsumerForMetrics outputArtifactsFromActionCache,
AtomicReference<ActionExecutionStatusReporter> statusReporterRef,
Supplier<ImmutableList<Root>> sourceRootSupplier,
PathFragment relativeOutputPath) {
PathFragment relativeOutputPath,
AtomicReference<FilesystemCalls> syscalls) {
this.actionKeyContext = actionKeyContext;
this.outputArtifactsSeen = outputArtifactsSeen;
this.outputArtifactsFromActionCache = outputArtifactsFromActionCache;
this.statusReporterRef = statusReporterRef;
this.sourceRootSupplier = sourceRootSupplier;
this.relativeOutputPath = relativeOutputPath;
this.syscalls = syscalls;
}

SharedActionCallback getSharedActionCallback(
Expand Down Expand Up @@ -474,7 +478,8 @@ ActionExecutionValue executeAction(
artifactExpander,
topLevelFilesets,
actionFileSystem,
skyframeDepsResult);
skyframeDepsResult,
syscalls);

if (actionCacheChecker.isActionExecutionProhibited(action)) {
// We can't execute an action (e.g. because --check_???_up_to_date option was used). Fail the
Expand Down Expand Up @@ -540,7 +545,8 @@ private ActionExecutionContext getContext(
ArtifactExpander artifactExpander,
ImmutableMap<Artifact, ImmutableList<FilesetOutputSymlink>> topLevelFilesets,
@Nullable FileSystem actionFileSystem,
@Nullable Object skyframeDepsResult)
@Nullable Object skyframeDepsResult,
AtomicReference<FilesystemCalls> syscalls)
throws InterruptedException {
boolean emitProgressEvents = shouldEmitProgressEvents(action);
ArtifactPathResolver artifactPathResolver =
Expand Down Expand Up @@ -575,7 +581,8 @@ private ActionExecutionContext getContext(
artifactExpander,
actionFileSystem,
skyframeDepsResult,
nestedSetExpander);
nestedSetExpander,
syscalls.get());
}

private static void closeContext(
Expand Down Expand Up @@ -776,7 +783,8 @@ NestedSet<Artifact> discoverInputs(
clientEnv,
env,
actionFileSystem,
nestedSetExpander);
nestedSetExpander,
syscalls.get());
if (actionFileSystem != null) {
updateActionFileSystemContext(
action,
Expand Down
3 changes: 2 additions & 1 deletion src/main/java/com/google/devtools/build/lib/skyframe/SkyframeExecutor.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -455,7 +455,8 @@ protected SkyframeExecutor(
outputArtifactsFromActionCache,
statusReporterRef,
this::getPathEntries,
PathFragment.create(directories.getRelativeOutputPath()));
PathFragment.create(directories.getRelativeOutputPath()),
syscalls);
this.artifactFactory =
new ArtifactFactory(
/* execRootParent= */ directories.getExecRootBase(),
Expand Down
4 changes: 3 additions & 1 deletion src/test/java/com/google/devtools/build/lib/actions/ExecutableSymlinkActionTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@
import com.google.devtools.build.lib.vfs.FileSystemUtils;
import com.google.devtools.build.lib.vfs.Path;
import com.google.devtools.build.lib.vfs.Root;
import com.google.devtools.build.lib.vfs.UnixGlob;
import org.junit.Before;
import org.junit.Test;
import org.junit.runner.RunWith;
Expand Down Expand Up @@ -81,7 +82,8 @@ private ActionExecutionContext createContext() {
/*artifactExpander=*/ null,
/*actionFileSystem=*/ null,
/*skyframeDepsResult=*/ null,
NestedSetExpander.DEFAULT);
NestedSetExpander.DEFAULT,
UnixGlob.DEFAULT_SYSCALLS);
}

@Test
Expand Down
10 changes: 7 additions & 3 deletions src/test/java/com/google/devtools/build/lib/actions/util/ActionsTestUtil.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -88,6 +88,7 @@
import com.google.devtools.build.lib.vfs.Path;
import com.google.devtools.build.lib.vfs.PathFragment;
import com.google.devtools.build.lib.vfs.Root;
import com.google.devtools.build.lib.vfs.UnixGlob;
import com.google.devtools.build.lib.vfs.inmemoryfs.InMemoryFileSystem;
import com.google.devtools.build.skyframe.AbstractSkyFunctionEnvironment;
import com.google.devtools.build.skyframe.BuildDriver;
Expand Down Expand Up @@ -176,7 +177,8 @@ public static ActionExecutionContext createContext(
: ActionInputHelper.actionGraphArtifactExpander(actionGraph),
/*actionFileSystem=*/ null,
/*skyframeDepsResult=*/ null,
NestedSetExpander.DEFAULT);
NestedSetExpander.DEFAULT,
UnixGlob.DEFAULT_SYSCALLS);
}

public static ActionExecutionContext createContext(ExtendedEventHandler eventHandler) {
Expand All @@ -196,7 +198,8 @@ public static ActionExecutionContext createContext(ExtendedEventHandler eventHan
(artifact, output) -> {},
/*actionFileSystem=*/ null,
/*skyframeDepsResult=*/ null,
NestedSetExpander.DEFAULT);
NestedSetExpander.DEFAULT,
UnixGlob.DEFAULT_SYSCALLS);
}

public static ActionExecutionContext createContextForInputDiscovery(
Expand All @@ -221,7 +224,8 @@ public static ActionExecutionContext createContextForInputDiscovery(
ImmutableMap.of(),
new BlockingSkyFunctionEnvironment(buildDriver, eventHandler),
/*actionFileSystem=*/ null,
nestedSetExpander);
nestedSetExpander,
UnixGlob.DEFAULT_SYSCALLS);
}

public static Artifact createArtifact(ArtifactRoot root, Path path) {
Expand Down
4 changes: 3 additions & 1 deletion src/test/java/com/google/devtools/build/lib/analysis/actions/FileWriteActionTestCase.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,7 @@
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.UnixGlob;
import java.util.Collection;
import org.junit.Before;

Expand Down Expand Up @@ -79,7 +80,8 @@ public final void createExecutorAndContext() throws Exception {
/*artifactExpander=*/ null,
/*actionFileSystem=*/ null,
/*skyframeDepsResult=*/ null,
NestedSetExpander.DEFAULT);
NestedSetExpander.DEFAULT,
UnixGlob.DEFAULT_SYSCALLS);
}

protected void checkNoInputsByDefault() {
Expand Down
4 changes: 3 additions & 1 deletion src/test/java/com/google/devtools/build/lib/analysis/actions/ParamFileWriteActionTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,7 @@
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.UnixGlob;
import java.util.Collection;
import java.util.List;
import org.junit.Before;
Expand Down Expand Up @@ -203,7 +204,8 @@ public void expand(Artifact artifact, Collection<? super Artifact> output) {
artifactExpander,
/*actionFileSystem=*/ null,
/*skyframeDepsResult=*/ null,
NestedSetExpander.DEFAULT);
NestedSetExpander.DEFAULT,
UnixGlob.DEFAULT_SYSCALLS);
}

private enum KeyAttributes {
Expand Down
4 changes: 3 additions & 1 deletion src/test/java/com/google/devtools/build/lib/analysis/actions/SymlinkActionTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,7 @@
import com.google.devtools.build.lib.vfs.FileSystemUtils;
import com.google.devtools.build.lib.vfs.Path;
import com.google.devtools.build.lib.vfs.Root;
import com.google.devtools.build.lib.vfs.UnixGlob;
import org.junit.Before;
import org.junit.Test;
import org.junit.runner.RunWith;
Expand Down Expand Up @@ -100,7 +101,8 @@ public void testSymlink() throws Exception {
/*artifactExpander=*/ null,
/*actionFileSystem=*/ null,
/*skyframeDepsResult=*/ null,
NestedSetExpander.DEFAULT));
NestedSetExpander.DEFAULT,
UnixGlob.DEFAULT_SYSCALLS));
assertThat(actionResult.spawnResults()).isEmpty();
assertThat(output.isSymbolicLink()).isTrue();
assertThat(output.resolveSymbolicLinks()).isEqualTo(input);
Expand Down
4 changes: 3 additions & 1 deletion ...test/java/com/google/devtools/build/lib/analysis/actions/TemplateExpansionActionTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,7 @@
import com.google.devtools.build.lib.vfs.FileSystemUtils;
import com.google.devtools.build.lib.vfs.Path;
import com.google.devtools.build.lib.vfs.Root;
import com.google.devtools.build.lib.vfs.UnixGlob;
import java.nio.charset.StandardCharsets;
import java.util.List;
import org.junit.Before;
Expand Down Expand Up @@ -206,7 +207,8 @@ private ActionExecutionContext createContext(Executor executor) {
/*artifactExpander=*/ null,
/*actionFileSystem=*/ null,
/*skyframeDepsResult=*/ null,
NestedSetExpander.DEFAULT);
NestedSetExpander.DEFAULT,
UnixGlob.DEFAULT_SYSCALLS);
}

private void executeTemplateExpansion(String expected) throws Exception {
Expand Down
4 changes: 3 additions & 1 deletion src/test/java/com/google/devtools/build/lib/analysis/util/BuildViewTestCase.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -169,6 +169,7 @@
import com.google.devtools.build.lib.vfs.Path;
import com.google.devtools.build.lib.vfs.PathFragment;
import com.google.devtools.build.lib.vfs.Root;
import com.google.devtools.build.lib.vfs.UnixGlob;
import com.google.devtools.build.skyframe.ErrorInfo;
import com.google.devtools.build.skyframe.InMemoryMemoizingEvaluator;
import com.google.devtools.build.skyframe.MemoizingEvaluator;
Expand Down Expand Up @@ -2450,7 +2451,8 @@ public ActionExecutionContext build() {
artifactExpander,
/*actionFileSystem=*/ null,
/*skyframeDepsResult*/ null,
NestedSetExpander.DEFAULT);
NestedSetExpander.DEFAULT,
UnixGlob.DEFAULT_SYSCALLS);
}
}
}
4 changes: 3 additions & 1 deletion src/test/java/com/google/devtools/build/lib/exec/StandaloneTestStrategyTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -68,6 +68,7 @@
import com.google.devtools.build.lib.vfs.FileSystem;
import com.google.devtools.build.lib.vfs.FileSystemUtils;
import com.google.devtools.build.lib.vfs.Path;
import com.google.devtools.build.lib.vfs.UnixGlob;
import com.google.devtools.build.lib.view.test.TestStatus.BlazeTestStatus;
import com.google.devtools.build.lib.view.test.TestStatus.TestResultData;
import com.google.devtools.common.options.Options;
Expand Down Expand Up @@ -148,7 +149,8 @@ public FakeActionExecutionContext(
/*artifactExpander=*/ null,
/*actionFileSystem=*/ null,
/*skyframeDepsResult=*/ null,
NestedSetExpander.DEFAULT);
NestedSetExpander.DEFAULT,
UnixGlob.DEFAULT_SYSCALLS);
this.actionContextRegistry = actionContextRegistry;
}

Expand Down
Loading

0 comments on commit d05f92e

Please sign in to comment.