Skip to content

Commit

Permalink
Replace most usages of SyscallCache with the weaker XattrProvider. Sy…
Browse files Browse the repository at this point in the history
…scallCache is unsafe to use with files that may change over the course of the build, like outputs and files in external repositories. The weaker XattrProvider type helps to ensure that a stale cached value is not used.

Also thread SyscallCache through to IncludeParser.

FilesystemValueChecker is still using SyscallCache on outputs/external repository files. It will be fixed in a follow-up.

PiperOrigin-RevId: 433254855
  • Loading branch information
janakdr authored and copybara-github committed Mar 8, 2022
1 parent 7bb2525 commit e285322
Show file tree
Hide file tree
Showing 36 changed files with 183 additions and 138 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@
import com.google.devtools.build.lib.vfs.PathFragment;
import com.google.devtools.build.lib.vfs.Symlinks;
import com.google.devtools.build.lib.vfs.SyscallCache;
import com.google.devtools.build.lib.vfs.XattrProvider;
import com.google.devtools.build.skyframe.SkyValue;
import java.io.ByteArrayInputStream;
import java.io.IOException;
Expand Down Expand Up @@ -191,7 +192,7 @@ interface Singleton {}
public static final FileArtifactValue OMITTED_FILE_MARKER = new OmittedFileValue();

public static FileArtifactValue createForSourceArtifact(
Artifact artifact, FileValue fileValue, SyscallCache syscallCache) throws IOException {
Artifact artifact, FileValue fileValue, XattrProvider xattrProvider) throws IOException {
// Artifacts with known generating actions should obtain the derived artifact's SkyValue
// from the generating action, instead.
Preconditions.checkState(!artifact.hasKnownGeneratingAction());
Expand All @@ -203,7 +204,7 @@ public static FileArtifactValue createForSourceArtifact(
isFile ? fileValue.getSize() : 0,
isFile ? fileValue.realFileStateValue().getContentsProxy() : null,
isFile ? fileValue.getDigest() : null,
syscallCache);
xattrProvider);
}

public static FileArtifactValue createFromInjectedDigest(
Expand All @@ -225,14 +226,14 @@ public static FileArtifactValue createForTesting(Path path) throws IOException {
}

public static FileArtifactValue createFromStat(
Path path, FileStatus stat, SyscallCache syscallCache) throws IOException {
Path path, FileStatus stat, XattrProvider xattrProvider) throws IOException {
return create(
path,
stat.isFile(),
stat.getSize(),
FileContentsProxy.create(stat),
/*digest=*/ null,
syscallCache);
xattrProvider);
}

private static FileArtifactValue create(
Expand All @@ -241,7 +242,7 @@ private static FileArtifactValue create(
long size,
FileContentsProxy proxy,
@Nullable byte[] digest,
SyscallCache syscallCache)
XattrProvider xattrProvider)
throws IOException {
if (!isFile) {
// In this case, we need to store the mtime because the action cache uses mtime for
Expand All @@ -250,7 +251,7 @@ private static FileArtifactValue create(
return new DirectoryArtifactValue(path.getLastModifiedTime());
}
if (digest == null) {
digest = DigestUtils.getDigestWithManualFallback(path, size, syscallCache);
digest = DigestUtils.getDigestWithManualFallback(path, size, xattrProvider);
}
Preconditions.checkState(digest != null, path);
return createForNormalFile(digest, proxy, size);
Expand Down Expand Up @@ -289,8 +290,8 @@ public static FileArtifactValue createForNormalFile(
* handle getting the digest using the Path and size values.
*/
public static FileArtifactValue createForNormalFileUsingPath(
Path path, long size, SyscallCache syscallCache) throws IOException {
return create(path, /*isFile=*/ true, size, /*proxy=*/ null, /*digest=*/ null, syscallCache);
Path path, long size, XattrProvider xattrProvider) throws IOException {
return create(path, /*isFile=*/ true, size, /*proxy=*/ null, /*digest=*/ null, xattrProvider);
}

public static FileArtifactValue createForDirectoryWithHash(byte[] digest) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@
import com.google.devtools.build.lib.vfs.RootedPath;
import com.google.devtools.build.lib.vfs.Symlinks;
import com.google.devtools.build.lib.vfs.SyscallCache;
import com.google.devtools.build.lib.vfs.XattrProvider;
import com.google.devtools.build.skyframe.SkyValue;
import java.io.IOException;
import java.util.Arrays;
Expand Down Expand Up @@ -112,15 +113,15 @@ public static FileStateValue createWithStatNoFollow(
RootedPath rootedPath,
FileStatusWithDigest statNoFollow,
boolean digestWillBeInjected,
SyscallCache syscallCache,
XattrProvider xattrProvider,
@Nullable TimestampGranularityMonitor tsgm)
throws IOException {
Path path = rootedPath.asPath();
if (statNoFollow.isFile()) {
return statNoFollow.isSpecialFile()
? SpecialFileStateValue.fromStat(path.asFragment(), statNoFollow, tsgm)
: RegularFileStateValue.fromPath(
path, statNoFollow, digestWillBeInjected, syscallCache, tsgm);
path, statNoFollow, digestWillBeInjected, xattrProvider, tsgm);
} else if (statNoFollow.isDirectory()) {
return DIRECTORY_FILE_STATE_NODE;
} else if (statNoFollow.isSymbolicLink()) {
Expand Down Expand Up @@ -223,7 +224,7 @@ private static RegularFileStateValue fromPath(
Path path,
FileStatusWithDigest stat,
boolean digestWillBeInjected,
SyscallCache syscallCache,
XattrProvider xattrProvider,
@Nullable TimestampGranularityMonitor tsgm)
throws InconsistentFilesystemException {
Preconditions.checkState(stat.isFile(), path);
Expand All @@ -232,7 +233,7 @@ private static RegularFileStateValue fromPath(
// If the digest will be injected, we can skip calling getFastDigest, but we need to store a
// contents proxy because if the digest is injected but is not available from the
// filesystem, we will need the proxy to determine whether the file was modified.
byte[] digest = digestWillBeInjected ? null : tryGetDigest(path, stat, syscallCache);
byte[] digest = digestWillBeInjected ? null : tryGetDigest(path, stat, xattrProvider);
if (digest == null) {
// Note that TimestampGranularityMonitor#notifyDependenceOnFileTime is a thread-safe
// method.
Expand All @@ -257,10 +258,10 @@ private static RegularFileStateValue fromPath(

@Nullable
private static byte[] tryGetDigest(
Path path, FileStatusWithDigest stat, SyscallCache syscallCache) throws IOException {
Path path, FileStatusWithDigest stat, XattrProvider xattrProvider) throws IOException {
try {
byte[] digest = stat.getDigest();
return digest != null ? digest : syscallCache.getFastDigest(path);
return digest != null ? digest : xattrProvider.getFastDigest(path);
} catch (IOException ioe) {
if (!path.isReadable()) {
return null;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -15,13 +15,13 @@
package com.google.devtools.build.lib.bazel;

import com.google.devtools.build.lib.events.ExtendedEventHandler;
import com.google.devtools.build.lib.vfs.SyscallCache;
import com.google.devtools.build.lib.vfs.XattrProvider;

/** Interface for events reporting information to be added to a resolved file. */
public interface ResolvedEvent extends ExtendedEventHandler.ProgressLike {
/** The name of the resolved entity, e.g., the name of an external repository. */
String getName();

/** The entry for the list of resolved Information. */
Object getResolvedInformation(SyscallCache syscallCache);
Object getResolvedInformation(XattrProvider xattrProvider);
}
Original file line number Diff line number Diff line change
Expand Up @@ -120,7 +120,7 @@ private void initOutputs(CommandEnvironment env) throws IOException {
env.getExecRoot(),
outStream,
env.getOptions().getOptions(RemoteOptions.class),
env.getSyscallCache());
env.getXattrProvider());
}

@Override
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -52,7 +52,7 @@
import com.google.devtools.build.lib.util.ExitCode;
import com.google.devtools.build.lib.util.InterruptedFailureDetails;
import com.google.devtools.build.lib.vfs.RootedPath;
import com.google.devtools.build.lib.vfs.SyscallCache;
import com.google.devtools.build.lib.vfs.XattrProvider;
import com.google.devtools.build.skyframe.EvaluationContext;
import com.google.devtools.build.skyframe.EvaluationResult;
import com.google.devtools.build.skyframe.SkyKey;
Expand Down Expand Up @@ -264,7 +264,7 @@ public String getName() {
}

@Override
public Object getResolvedInformation(SyscallCache syscallCache) {
public Object getResolvedInformation(XattrProvider xattrProvider) {
return ImmutableMap.<String, Object>builder()
.put(ResolvedHashesFunction.ORIGINAL_RULE_CLASS, "bind")
.put(
Expand Down Expand Up @@ -296,7 +296,7 @@ public String getName() {
}

@Override
public Object getResolvedInformation(SyscallCache syscallCache) {
public Object getResolvedInformation(XattrProvider xattrProvider) {
return ImmutableMap.<String, Object>builder()
.put(ResolvedHashesFunction.ORIGINAL_RULE_CLASS, ruleName)
.put(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@
import com.google.devtools.build.lib.util.CPU;
import com.google.devtools.build.lib.util.OS;
import com.google.devtools.build.lib.vfs.Path;
import com.google.devtools.build.lib.vfs.SyscallCache;
import com.google.devtools.build.lib.vfs.XattrProvider;
import com.google.devtools.build.skyframe.SkyFunction.Environment;
import com.google.devtools.build.skyframe.SkyFunctionException.Transience;
import com.google.devtools.build.skyframe.SkyKey;
Expand Down Expand Up @@ -86,7 +86,7 @@ public String getName() {
}

@Override
public Object getResolvedInformation(SyscallCache syscallCache) {
public Object getResolvedInformation(XattrProvider xattrProvider) {
String repr = String.format("local_config_platform(name = '%s')", name);
return ImmutableMap.<String, Object>builder()
.put(ResolvedHashesFunction.ORIGINAL_RULE_CLASS, LocalConfigPlatformRule.NAME)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,7 @@
import com.google.devtools.build.lib.packages.StructImpl;
import com.google.devtools.build.lib.util.Pair;
import com.google.devtools.build.lib.vfs.Path;
import com.google.devtools.build.lib.vfs.SyscallCache;
import com.google.devtools.build.lib.vfs.XattrProvider;
import java.io.IOException;
import java.util.List;
import java.util.Map;
Expand Down Expand Up @@ -165,13 +165,13 @@ public RepositoryResolvedEvent(Rule rule, StructImpl attrs, Path outputDirectory
* Ensure that the {@code resolvedInformation} and the {@code directoryDigest} fields are
* initialized properly. Does nothing, if the values are computed already.
*/
private synchronized void finalizeResolvedInformation(SyscallCache syscallCache) {
private synchronized void finalizeResolvedInformation(XattrProvider xattrProvider) {
if (resolvedInformation != null) {
return;
}
String digest = "[unavailable]";
try {
digest = outputDirectory.getDirectoryDigest(syscallCache);
digest = outputDirectory.getDirectoryDigest(xattrProvider);
repositoryBuilder.put(OUTPUT_TREE_HASH, digest);
} catch (IOException e) {
// Digest not available, but we still have to report that a repository rule
Expand All @@ -191,8 +191,8 @@ private synchronized void finalizeResolvedInformation(SyscallCache syscallCache)
* Returns the entry for the given rule invocation in a format suitable for WORKSPACE.resolved.
*/
@Override
public Object getResolvedInformation(SyscallCache syscallCache) {
finalizeResolvedInformation(syscallCache);
public Object getResolvedInformation(XattrProvider xattrProvider) {
finalizeResolvedInformation(xattrProvider);
return resolvedInformation;
}

Expand All @@ -202,8 +202,8 @@ public String getName() {
return name;
}

public String getDirectoryDigest(SyscallCache syscallCache) {
finalizeResolvedInformation(syscallCache);
public String getDirectoryDigest(XattrProvider xattrProvider) {
finalizeResolvedInformation(xattrProvider);
return directoryDigest;
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@
import com.google.devtools.build.lib.runtime.BlazeModule;
import com.google.devtools.build.lib.runtime.Command;
import com.google.devtools.build.lib.runtime.CommandEnvironment;
import com.google.devtools.build.lib.vfs.SyscallCache;
import com.google.devtools.build.lib.vfs.XattrProvider;
import com.google.devtools.common.options.OptionsBase;
import java.io.File;
import java.io.IOException;
Expand All @@ -42,7 +42,7 @@ public final class RepositoryResolvedModule extends BlazeModule {
private Map<String, Object> resolvedValues;
private String resolvedFile;
private ImmutableList<String> orderedNames;
private SyscallCache syscallCache;
private XattrProvider xattrProvider;

@Override
public Iterable<Class<? extends OptionsBase>> getCommandOptions(Command command) {
Expand All @@ -63,7 +63,7 @@ public void beforeCommand(CommandEnvironment env) {
} else {
this.resolvedFile = null;
}
this.syscallCache = env.getSyscallCache();
this.xattrProvider = env.getXattrProvider();
}

@Override
Expand Down Expand Up @@ -100,7 +100,7 @@ public void repositoryOrderEvent(RepositoryOrderEvent event) {
@Subscribe
public void resolved(ResolvedEvent event) {
if (resolvedValues != null) {
resolvedValues.put(event.getName(), event.getResolvedInformation(syscallCache));
resolvedValues.put(event.getName(), event.getResolvedInformation(xattrProvider));
}
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,8 @@

package com.google.devtools.build.lib.bazel.repository.starlark;

import com.google.common.base.Preconditions;
import static com.google.common.base.Preconditions.checkNotNull;

import com.google.common.collect.ImmutableList;
import com.google.common.collect.ImmutableMap;
import com.google.common.collect.ImmutableSet;
Expand Down Expand Up @@ -82,7 +83,7 @@ public void setProcessWrapper(@Nullable ProcessWrapper processWrapper) {
}

public void setSyscallCache(SyscallCache syscallCache) {
this.syscallCache = syscallCache;
this.syscallCache = checkNotNull(syscallCache);
}

static String describeSemantics(StarlarkSemantics semantics) {
Expand Down Expand Up @@ -138,8 +139,7 @@ public RepositoryDirectoryValue.Builder fetch(
if (env.valuesMissing()) {
return null;
}
Map<String, String> resolvedHashes =
Preconditions.checkNotNull(resolvedHashesValue).getHashes();
Map<String, String> resolvedHashes = checkNotNull(resolvedHashesValue).getHashes();

PathPackageLocator packageLocator = PrecomputedValue.PATH_PACKAGE_LOCATOR.get(env);
if (env.valuesMissing()) {
Expand All @@ -151,8 +151,7 @@ public RepositoryDirectoryValue.Builder fetch(
if (env.valuesMissing()) {
return null;
}
ImmutableSet<PathFragment> ignoredPatterns =
Preconditions.checkNotNull(ignoredPackagesValue).getPatterns();
ImmutableSet<PathFragment> ignoredPatterns = checkNotNull(ignoredPackagesValue).getPatterns();

try (Mutability mu = Mutability.create("Starlark repository")) {
StarlarkThread thread = new StarlarkThread(mu, starlarkSemantics);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@
import com.google.devtools.build.lib.vfs.DigestUtils;
import com.google.devtools.build.lib.vfs.Path;
import com.google.devtools.build.lib.vfs.PathFragment;
import com.google.devtools.build.lib.vfs.SyscallCache;
import com.google.devtools.build.lib.vfs.XattrProvider;
import java.io.IOException;
import java.util.Arrays;
import java.util.HashMap;
Expand Down Expand Up @@ -68,7 +68,7 @@ private static void updateRunfilesTree(
ImmutableMap<String, String> env,
OutErr outErr,
boolean enableRunfiles,
SyscallCache syscallCache)
XattrProvider xattrProvider)
throws IOException, ExecException, InterruptedException {
Path runfilesDirPath = execRoot.getRelative(runfilesDir);
Path inputManifest = RunfilesSupport.inputManifestPath(runfilesDirPath);
Expand All @@ -84,9 +84,9 @@ private static void updateRunfilesTree(
// an up-to-date check.
if (!outputManifest.isSymbolicLink()
&& Arrays.equals(
DigestUtils.getDigestWithManualFallbackWhenSizeUnknown(outputManifest, syscallCache),
DigestUtils.getDigestWithManualFallbackWhenSizeUnknown(outputManifest, xattrProvider),
DigestUtils.getDigestWithManualFallbackWhenSizeUnknown(
inputManifest, syscallCache))) {
inputManifest, xattrProvider))) {
return;
}
} catch (IOException e) {
Expand Down Expand Up @@ -135,7 +135,7 @@ public void updateRunfilesDirectory(
BinTools binTools,
ImmutableMap<String, String> env,
OutErr outErr,
SyscallCache syscallCache)
XattrProvider xattrProvider)
throws ExecException, IOException, InterruptedException {
for (Map.Entry<PathFragment, Map<PathFragment, Artifact>> runfiles :
runfilesSupplier.getMappings().entrySet()) {
Expand All @@ -159,7 +159,7 @@ public void updateRunfilesDirectory(
env,
outErr,
runfilesSupplier.isRunfileLinksEnabled(runfilesDir),
syscallCache);
xattrProvider);
}
} finally {
decrementRefcnt(runfilesDir);
Expand Down
Loading

0 comments on commit e285322

Please sign in to comment.