From c0633bc7785f47035d719d964e66964469d01ec1 Mon Sep 17 00:00:00 2001 From: "L. Sousa" Date: Tue, 25 Aug 2026 18:02:29 +0100 Subject: [PATCH 1/7] perf(clang): skip unchanged AST dumps --- .../pt/up/fe/specs/clang/AstDumpCache.java | 580 ++++++++++++++++++ .../src/pt/up/fe/specs/clang/CacheFiles.java | 18 +- .../clang/codeparser/ParallelCodeParser.java | 8 +- .../fe/specs/clang/dumper/ClangAstDumper.java | 154 ++++- .../up/fe/specs/clang/AstDumpCacheTest.java | 260 ++++++++ .../ClangAstDumperCacheIntegrationTest.java | 124 ++++ 6 files changed, 1113 insertions(+), 31 deletions(-) create mode 100644 ClangAstParser/src/pt/up/fe/specs/clang/AstDumpCache.java create mode 100644 ClangAstParser/test/pt/up/fe/specs/clang/AstDumpCacheTest.java create mode 100644 ClangAstParser/test/pt/up/fe/specs/clang/dumper/ClangAstDumperCacheIntegrationTest.java diff --git a/ClangAstParser/src/pt/up/fe/specs/clang/AstDumpCache.java b/ClangAstParser/src/pt/up/fe/specs/clang/AstDumpCache.java new file mode 100644 index 000000000..3af09fa46 --- /dev/null +++ b/ClangAstParser/src/pt/up/fe/specs/clang/AstDumpCache.java @@ -0,0 +1,580 @@ +/** + * Copyright 2026 SPeCS. + *

+ * 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. + */ + +package pt.up.fe.specs.clang; + +import com.google.gson.Gson; +import pt.up.fe.specs.util.SpecsLogs; + +import java.io.File; +import java.io.IOException; +import java.io.InputStream; +import java.io.OutputStream; +import java.io.UncheckedIOException; +import java.nio.charset.StandardCharsets; +import java.nio.file.Files; +import java.nio.file.InvalidPathException; +import java.nio.file.Path; +import java.nio.file.StandardOpenOption; +import java.security.MessageDigest; +import java.security.NoSuchAlgorithmException; +import java.time.Duration; +import java.time.Instant; +import java.util.ArrayList; +import java.util.Collection; +import java.util.HashSet; +import java.util.HexFormat; +import java.util.List; +import java.util.Map; +import java.util.Objects; +import java.util.Optional; +import java.util.Set; +import java.util.TreeMap; +import java.util.function.Function; +import java.util.function.Predicate; +import java.util.zip.GZIPInputStream; +import java.util.zip.GZIPOutputStream; + +/** + * Immutable, dependency-aware cache for one Clang AST dumper invocation. + * + *

The cache deliberately knows nothing about the dump format. The caller supplies the stream parser and tells the + * cache which files the parsed result depends on. A published entry is a complete immutable directory containing a + * gzip stream and its dependency manifest.

+ */ +public final class AstDumpCache { + + /** Cache namespace and manifest schema version. */ + public static final String FORMAT_VERSION = "v1"; + + private static final String KEY_FORMAT = "ast-dump-key-v1"; + private static final String MANIFEST_FILENAME = "manifest.json"; + private static final String DUMP_FILENAME = "dump.gz"; + private static final Duration STALE_ENTRY_AGE = Duration.ofDays(60); + private static final Gson GSON = new Gson(); + + private final Path cacheRoot; + private final Path entriesRoot; + private final Path canonicalSource; + private final List command; + + /** + * Creates a cache for an exact source and dumper command. + * + * @param cacheRoot common Clava cache root + * @param sourceFile source file being dumped + * @param command exact ordered dumper command, including executable and arguments + */ + public AstDumpCache(File cacheRoot, File sourceFile, List command) { + this(cacheRoot.toPath(), sourceFile.toPath(), command); + } + + /** + * Creates a cache for an exact source and dumper command. + * + * @param cacheRoot common Clava cache root + * @param sourceFile source file being dumped + * @param command exact ordered dumper command, including executable and arguments + */ + public AstDumpCache(Path cacheRoot, Path sourceFile, List command) { + this.cacheRoot = Objects.requireNonNull(cacheRoot, "cacheRoot").toAbsolutePath().normalize(); + this.entriesRoot = this.cacheRoot.resolve("ast-dumps").resolve(FORMAT_VERSION).resolve("entries"); + this.canonicalSource = canonicalizeAtConstruction(Objects.requireNonNull(sourceFile, "sourceFile")); + this.command = List.copyOf(Objects.requireNonNull(command, "command")); + } + + /** + * Attempts to load a valid cached dump. Any cache or parser failure is an ordinary miss. + * + * @param parser parser for the gzip stream; it must not close the supplied stream + * @return the parsed value on a valid hit, or an empty optional on a miss + */ + public Optional load(InputStreamParser parser) { + Objects.requireNonNull(parser, "parser"); + + String key = calculateKey(); + if (key == null) { + return Optional.empty(); + } + + Path entry = entriesRoot.resolve(key); + if (!claimValidEntry(entry)) { + return Optional.empty(); + } + + try (InputStream compressed = Files.newInputStream(entry.resolve(DUMP_FILENAME)); + InputStream input = new GZIPInputStream(compressed)) { + T result = parser.parse(input); + + // Force the gzip stream to EOF so a truncated stream or invalid trailer cannot be accepted merely because + // a parser stopped after the first record. + input.transferTo(OutputStream.nullOutputStream()); + if (result == null) { + throw new RuntimeException("AST dump parser returned null"); + } + + return Optional.of(result); + } catch (Exception e) { + reportCacheFailure("Could not read cached AST dump entry '" + entry + "'", e); + deleteEntryQuietly(entry); + return Optional.empty(); + } + } + + /** + * Runs a producer and publishes its exact gzip output only when the caller accepts the result. + * + *

The producer is always run. Cache setup, output, manifest, and publication failures are swallowed as cache + * misses so that parsing can continue. Producer failures themselves are propagated to preserve dumper semantics.

+ * + * @param producer callback which writes the exact stderr bytes to the supplied stream and returns the parsed value + * @param dependencyPaths extracts named dependency paths from the parsed value + * @param publishDecision says whether the dumper and stream parse succeeded + * @return the producer's parsed value + */ + public T capture(DumpProducer producer, + Function> dependencyPaths, + Predicate publishDecision) { + Objects.requireNonNull(producer, "producer"); + Objects.requireNonNull(dependencyPaths, "dependencyPaths"); + Objects.requireNonNull(publishDecision, "publishDecision"); + + String key = calculateKey(); + if (key == null) { + return runWithoutCache(producer); + } + + Path entry = entriesRoot.resolve(key); + try { + prepareEntriesDirectory(entry); + } catch (RuntimeException e) { + reportCacheFailure("Could not prepare AST dump cache", e); + return runWithoutCache(producer); + } + + CacheFiles.StagingDirectory staging; + try { + staging = CacheFiles.createStagingDirectory(cacheRoot, entriesRoot, "." + key + ".tmp-"); + } catch (RuntimeException e) { + reportCacheFailure("Could not create AST dump cache staging directory", e); + return runWithoutCache(producer); + } + + T result = null; + boolean producerStarted = false; + boolean outputUsable = true; + try { + Path stagedDump = staging.path().resolve(DUMP_FILENAME); + try (OutputStream fileOutput = Files.newOutputStream(stagedDump, StandardOpenOption.CREATE_NEW, + StandardOpenOption.WRITE)) { + var bestEffortOutput = new BestEffortOutputStream(fileOutput); + try (var gzipOutput = new GZIPOutputStream(bestEffortOutput)) { + producerStarted = true; + result = runProducer(producer, gzipOutput); + } + outputUsable = !bestEffortOutput.failed(); + } catch (IOException e) { + // A failed cache stream must not turn a successful dumper run into a parse failure. The producer has + // not necessarily run if opening the cache stream itself failed, so fall back to the producer with a + // null sink in that case. + outputUsable = false; + reportCacheFailure("Could not write cached AST dump", e); + discardStagingQuietly(staging); + return producerStarted ? result : runWithoutCache(producer); + } + } catch (RuntimeException e) { + discardStagingQuietly(staging); + throw e; + } + + if (!outputUsable) { + discardStagingQuietly(staging); + return result; + } + + if (!shouldPublish(result, publishDecision)) { + discardStagingQuietly(staging); + return result; + } + + try { + Manifest manifest = buildManifest(dependencyPaths.apply(result)); + Path stagedManifest = staging.path().resolve(MANIFEST_FILENAME); + Files.writeString(stagedManifest, GSON.toJson(manifest), StandardCharsets.UTF_8, + StandardOpenOption.CREATE_NEW, StandardOpenOption.WRITE); + + // The directory move is the publication point; readers cannot observe only one of dump.gz/manifest.json. + CacheFiles.publish(staging.path(), entry); + } catch (RuntimeException | IOException e) { + reportCacheFailure("Could not publish AST dump cache entry '" + entry + "'", e); + CacheFiles.deleteQuietly(staging.path()); + } finally { + closeStagingQuietly(staging); + } + + return result; + } + + /** Parses a cached gzip stream. */ + @FunctionalInterface + public interface InputStreamParser { + T parse(InputStream inputStream) throws Exception; + } + + /** Produces and parses one dumper invocation while writing exact stderr bytes to the supplied stream. */ + @FunctionalInterface + public interface DumpProducer { + T produce(OutputStream outputStream) throws Exception; + } + + private static final class Manifest { + private String schema; + private List dependencies; + + private Manifest(String schema, List dependencies) { + this.schema = schema; + this.dependencies = dependencies; + } + } + + private static final class Dependency { + private String path; + private String sha256; + + private Dependency(String path, String sha256) { + this.path = path; + this.sha256 = sha256; + } + } + + /** Swallows writes after a cache I/O failure so the producer can still complete its real parse. */ + private static final class BestEffortOutputStream extends OutputStream { + private final OutputStream delegate; + private boolean failed; + + private BestEffortOutputStream(OutputStream delegate) { + this.delegate = delegate; + } + + @Override + public void write(int value) { + if (failed) { + return; + } + + try { + delegate.write(value); + } catch (IOException e) { + failed = true; + } + } + + @Override + public void write(byte[] bytes, int offset, int length) { + if (failed) { + return; + } + + try { + delegate.write(bytes, offset, length); + } catch (IOException e) { + failed = true; + } + } + + @Override + public void flush() { + if (failed) { + return; + } + + try { + delegate.flush(); + } catch (IOException e) { + failed = true; + } + } + + @Override + public void close() { + try { + delegate.close(); + } catch (IOException e) { + failed = true; + } + } + + private boolean failed() { + return failed; + } + } + + private static Path canonicalizeAtConstruction(Path sourceFile) { + Path absolute = sourceFile.toAbsolutePath().normalize(); + try { + return absolute.toRealPath(); + } catch (IOException e) { + // Source disappearance is handled as a cache miss when the key or manifest is next needed. + return absolute; + } + } + + private String calculateKey() { + try { + MessageDigest digest = MessageDigest.getInstance("SHA-256"); + putString(digest, FORMAT_VERSION); + putString(digest, KEY_FORMAT); + putString(digest, canonicalSource.toString()); + putLong(digest, command.size()); + for (String argument : command) { + putString(digest, argument); + } + + long sourceSize = Files.size(canonicalSource); + putLong(digest, sourceSize); + try (InputStream input = Files.newInputStream(canonicalSource)) { + byte[] buffer = new byte[8192]; + int read; + while ((read = input.read(buffer)) != -1) { + digest.update(buffer, 0, read); + } + } + + return HexFormat.of().formatHex(digest.digest()); + } catch (IOException | NoSuchAlgorithmException e) { + reportCacheFailure("Could not calculate AST dump cache key for '" + canonicalSource + "'", e); + return null; + } + } + + private static void putString(MessageDigest digest, String value) { + byte[] bytes = value.getBytes(StandardCharsets.UTF_8); + putLong(digest, bytes.length); + digest.update(bytes); + } + + private static void putLong(MessageDigest digest, long value) { + for (int shift = Long.SIZE - Byte.SIZE; shift >= 0; shift -= Byte.SIZE) { + digest.update((byte) (value >>> shift)); + } + } + + private boolean claimValidEntry(Path entry) { + try { + return CacheFiles.withMaintenanceLock(cacheRoot, () -> { + try { + Files.createDirectories(entriesRoot); + } catch (IOException e) { + throw new UncheckedIOException("Could not create AST dump cache entries directory", e); + } + + // Claim the entry before cleanup. A concurrent cleanup therefore cannot remove an entry that this + // reader is about to validate and use. + if (Files.isDirectory(entry)) { + CacheFiles.touch(entry); + } + + CacheFiles.deleteStaleDirectoriesLocked(entriesRoot, Instant.now().minus(STALE_ENTRY_AGE), entry); + CacheFiles.deleteUnlockedStagingLocksLocked(entriesRoot); + + if (!Files.isDirectory(entry)) { + return false; + } + + try { + if (!isManifestValid(entry)) { + CacheFiles.deleteQuietly(entry); + return false; + } + } catch (RuntimeException e) { + CacheFiles.deleteQuietly(entry); + throw e; + } + + CacheFiles.touch(entry); + return true; + }); + } catch (RuntimeException e) { + reportCacheFailure("Could not validate AST dump cache entry '" + entry + "'", e); + return false; + } + } + + private void prepareEntriesDirectory(Path currentEntry) { + CacheFiles.withMaintenanceLock(cacheRoot, () -> { + try { + Files.createDirectories(entriesRoot); + } catch (IOException e) { + throw new UncheckedIOException("Could not create AST dump cache entries directory", e); + } + + CacheFiles.deleteStaleDirectoriesLocked(entriesRoot, Instant.now().minus(STALE_ENTRY_AGE), currentEntry); + CacheFiles.deleteUnlockedStagingLocksLocked(entriesRoot); + }); + } + + private boolean isManifestValid(Path entry) { + Path manifestPath = entry.resolve(MANIFEST_FILENAME); + Path dumpPath = entry.resolve(DUMP_FILENAME); + if (!Files.isRegularFile(manifestPath) || !Files.isRegularFile(dumpPath)) { + return false; + } + + Manifest manifest = GSON.fromJson(readManifest(manifestPath), Manifest.class); + if (manifest == null || !FORMAT_VERSION.equals(manifest.schema) || manifest.dependencies == null + || manifest.dependencies.isEmpty()) { + return false; + } + + Set paths = new HashSet<>(); + boolean sourceFound = false; + for (Dependency dependency : manifest.dependencies) { + if (dependency == null || dependency.path == null || dependency.sha256 == null + || !dependency.sha256.matches("[0-9a-fA-F]{64}") || !paths.add(dependency.path)) { + return false; + } + + Path path; + try { + path = Path.of(dependency.path); + } catch (InvalidPathException e) { + return false; + } + + if (!path.isAbsolute() || !Files.isRegularFile(path)) { + return false; + } + + try { + if (!path.toRealPath().equals(path.normalize())) { + return false; + } + } catch (IOException e) { + return false; + } + + if (!dependency.sha256.equalsIgnoreCase(CacheFiles.calculateSha256(path))) { + return false; + } + + sourceFound |= path.equals(canonicalSource); + } + + return sourceFound; + } + + private static String readManifest(Path path) { + try { + return Files.readString(path, StandardCharsets.UTF_8); + } catch (IOException e) { + throw new UncheckedIOException("Could not read AST dump cache manifest '" + path + "'", e); + } + } + + private Manifest buildManifest(Collection namedDependencies) { + Map dependencies = new TreeMap<>(); + addRequiredDependency(dependencies, canonicalSource); + + if (namedDependencies != null) { + for (Path dependency : namedDependencies) { + if (dependency == null) { + continue; + } + + addOptionalDependency(dependencies, dependency); + } + } + + var entries = new ArrayList(); + for (Map.Entry dependency : dependencies.entrySet()) { + entries.add(new Dependency(dependency.getKey(), CacheFiles.calculateSha256(dependency.getValue()))); + } + + return new Manifest(FORMAT_VERSION, List.copyOf(entries)); + } + + private static void addRequiredDependency(Map dependencies, Path path) { + try { + Path canonical = path.toRealPath(); + if (!Files.isRegularFile(canonical)) { + throw new IOException("Source is not a regular file: " + path); + } + + dependencies.put(canonical.toString(), canonical); + } catch (IOException e) { + throw new UncheckedIOException("Could not include source dependency '" + path + "'", e); + } + } + + private static void addOptionalDependency(Map dependencies, Path path) { + try { + Path canonical = path.toRealPath(); + if (!Files.isRegularFile(canonical)) { + return; + } + + dependencies.put(canonical.toString(), canonical); + } catch (IOException e) { + // Headers can disappear between the dumper's report and manifest creation. Such a path simply cannot + // participate in invalidation and is intentionally omitted. + } + } + + private static boolean shouldPublish(T result, Predicate publishDecision) { + try { + return publishDecision.test(result); + } catch (RuntimeException e) { + reportCacheFailure("Could not decide whether to publish AST dump cache result", e); + return false; + } + } + + private static T runWithoutCache(DumpProducer producer) { + return runProducer(producer, OutputStream.nullOutputStream()); + } + + private static T runProducer(DumpProducer producer, OutputStream output) { + try { + return producer.produce(output); + } catch (RuntimeException e) { + throw e; + } catch (Exception e) { + throw new RuntimeException("AST dumper producer failed", e); + } + } + + private static void closeStagingQuietly(CacheFiles.StagingDirectory staging) { + try { + staging.close(); + } catch (RuntimeException e) { + reportCacheFailure("Could not close AST dump cache staging lock", e); + } + } + + private static void discardStagingQuietly(CacheFiles.StagingDirectory staging) { + CacheFiles.deleteQuietly(staging.path()); + closeStagingQuietly(staging); + } + + private void deleteEntryQuietly(Path entry) { + try { + CacheFiles.withMaintenanceLock(cacheRoot, () -> CacheFiles.deleteQuietly(entry)); + } catch (RuntimeException e) { + reportCacheFailure("Could not remove invalid AST dump cache entry '" + entry + "'", e); + } + } + + private static void reportCacheFailure(String message, Throwable cause) { + SpecsLogs.debug(() -> message + ": " + cause.getMessage()); + } +} diff --git a/ClangAstParser/src/pt/up/fe/specs/clang/CacheFiles.java b/ClangAstParser/src/pt/up/fe/specs/clang/CacheFiles.java index 62fc87a58..0bd60e4ce 100644 --- a/ClangAstParser/src/pt/up/fe/specs/clang/CacheFiles.java +++ b/ClangAstParser/src/pt/up/fe/specs/clang/CacheFiles.java @@ -241,6 +241,10 @@ static void touch(Path path) { } static void deleteStaleDirectories(Path cacheRoot, Path parent, Instant cutoff, Path excluded) { + withMaintenanceLock(cacheRoot, () -> deleteStaleDirectoriesLocked(parent, cutoff, excluded)); + } + + static void deleteStaleDirectoriesLocked(Path parent, Instant cutoff, Path excluded) { if (!Files.isDirectory(parent)) { return; } @@ -255,7 +259,7 @@ static void deleteStaleDirectories(Path cacheRoot, Path parent, Instant cutoff, continue; } - withMaintenanceLock(cacheRoot, () -> deleteIfStale(child, cutoff)); + deleteIfStale(child, cutoff); } } catch (IOException e) { throw new UncheckedIOException("Could not clean stale cache directories below '" + parent + "'", e); @@ -274,13 +278,17 @@ private static void deleteIfStale(Path path, Instant cutoff) { } static void deleteUnlockedStagingLocks(Path cacheRoot, Path parent) { + withMaintenanceLock(cacheRoot, () -> deleteUnlockedStagingLocksLocked(parent)); + } + + static void deleteUnlockedStagingLocksLocked(Path parent) { if (!Files.isDirectory(parent)) { return; } try (DirectoryStream locks = Files.newDirectoryStream(parent, ".*.tmp-*.lock")) { for (Path lock : locks) { - withMaintenanceLock(cacheRoot, () -> deleteIfUnlockedStagingLock(lock)); + deleteIfUnlockedStagingLock(lock); } } catch (IOException e) { throw new UncheckedIOException("Could not clean cache staging directories below '" + parent + "'", @@ -327,7 +335,7 @@ static void delete(Path path) { } } - private static void deleteQuietly(Path path) { + static void deleteQuietly(Path path) { try { delete(path); } catch (RuntimeException ignored) { @@ -347,4 +355,8 @@ private static String calculateSha256(File file) { throw new RuntimeException("Could not calculate SHA-256 for file '" + file + "'", e); } } + + static String calculateSha256(Path path) { + return calculateSha256(path.toFile()); + } } diff --git a/ClangAstParser/src/pt/up/fe/specs/clang/codeparser/ParallelCodeParser.java b/ClangAstParser/src/pt/up/fe/specs/clang/codeparser/ParallelCodeParser.java index 91b7ca5b9..a1c9da3e3 100644 --- a/ClangAstParser/src/pt/up/fe/specs/clang/codeparser/ParallelCodeParser.java +++ b/ClangAstParser/src/pt/up/fe/specs/clang/codeparser/ParallelCodeParser.java @@ -388,12 +388,8 @@ private ClangAstData parseSource(File sourceFile, String id, Standard standard, } if (get(CLEAN)) { - // if (clangParser.getLastWorkingFolder() == null) { - // workingFolders.add(clangParser.getLastWorkingFolder()); - // } - if (clangParser.getLastWorkingFolder() == null) { - SpecsLogs.msgInfo("No working folder found for source file '" + sourceFile + "'"); - } else { + // Cache hits intentionally do not create a working folder. + if (clangParser.getLastWorkingFolder() != null) { SpecsIo.deleteFolder(clangParser.getLastWorkingFolder()); } diff --git a/ClangAstParser/src/pt/up/fe/specs/clang/dumper/ClangAstDumper.java b/ClangAstParser/src/pt/up/fe/specs/clang/dumper/ClangAstDumper.java index b0c1463f0..258bb4ca6 100644 --- a/ClangAstParser/src/pt/up/fe/specs/clang/dumper/ClangAstDumper.java +++ b/ClangAstParser/src/pt/up/fe/specs/clang/dumper/ClangAstDumper.java @@ -16,6 +16,7 @@ import org.suikasoft.jOptions.Interfaces.DataStore; import org.suikasoft.jOptions.JOptionsUtils; import org.suikasoft.jOptions.streamparser.LineStreamParser; +import pt.up.fe.specs.clang.AstDumpCache; import pt.up.fe.specs.clang.ClangAstKeys; import pt.up.fe.specs.clang.ClangResources; import pt.up.fe.specs.clang.LibcMode; @@ -39,11 +40,19 @@ import pt.up.fe.specs.util.utilities.LineStream; import java.io.File; +import java.io.IOException; import java.io.InputStream; +import java.io.OutputStream; +import java.nio.file.InvalidPathException; +import java.nio.file.Path; import java.util.ArrayList; import java.util.Arrays; +import java.util.Collection; +import java.util.HashSet; import java.util.List; +import java.util.Map; import java.util.Objects; +import java.util.Set; /** * Calls the ClangAstDumper executable and returns the dumped information. Clava AST can be built based on this output. @@ -150,10 +159,47 @@ public ClangAstData parse(File sourceFile, String id, Standard standard, DataSto private ClangAstData parsePrivate(File sourceFile, String id, Standard standard, DataStore config) { ClavaLog.debug(() -> "Data store config for single file parser: " + config); + // A cache hit does not create a working folder. Do not expose the folder from a previous parse as the result + // of this one. + lastWorkingFolder = null; DataStore localData = JOptionsUtils.loadDataStore(LocalOptionsKeys.getLocalOptionsFilename(), getClass(), LocalOptionsKeys.getProvider().getStoreDefinition()); + // Keep this phase free of side effects. The cache key must describe the exact invocation that would be + // launched, including all defaults and resource paths resolved below. + List arguments = buildArguments(sourceFile, id, standard, config, localData); + + ClavaLog.debug(() -> "Calling Clang AST Dumper: " + arguments); + + ParsedDump parsedDump; + boolean showClangDump = config.get(CodeParser.SHOW_CLANG_DUMP); + if (!showClangDump) { + AstDumpCache cache = new AstDumpCache(parserConfig.get(CodeParser.DUMPER_FOLDER).toPath(), + sourceFile.toPath(), arguments); + + var cachedDump = cache.load(input -> processStdErr(input, config.get(ClavaNode.CONTEXT), + OutputStream.nullOutputStream(), false)); + if (cachedDump.isPresent()) { + parsedDump = cachedDump.get(); + parsedDump.data().set(ClangAstData.HAS_ERRORS, false); + return materializeTranslationUnit(parsedDump, sourceFile, config); + } + + parsedDump = cache.capture(cacheOutput -> runDumper(arguments, sourceFile, id, config, cacheOutput), + this::getDependencies, + result -> !result.data().get(ClangAstData.HAS_ERRORS) && !result.parserHadExceptions()); + } else { + // clangDump.txt is a side effect of the process and has no cache representation. In particular, do not + // publish a cache entry that would make a subsequent SHOW_CLANG_DUMP parse silently lose that output. + parsedDump = runDumper(arguments, sourceFile, id, config, OutputStream.nullOutputStream()); + } + + return materializeTranslationUnit(parsedDump, sourceFile, config); + } + + private List buildArguments(File sourceFile, String id, Standard standard, DataStore config, + DataStore localData) { List arguments = new ArrayList<>(); if (USE_PLUGIN && SpecsPlatforms.isLinux()) { arguments.add("clang-16"); @@ -285,19 +331,13 @@ else if (SourceType.isHeader(sourceFile)) { arguments.addAll(config.get(ClavaOptions.FLAGS_LIST)); - ClavaLog.debug(() -> "Calling Clang AST Dumper: " + arguments); - - ClangAstData parsedData = null; - ProcessOutput output = null; - - try (LineStreamParser lineStreamParser = ClangStreamParserV2 - .newInstance(config.get(ClavaNode.CONTEXT))) { - - if (SpecsSystem.isDebug()) { - lineStreamParser.getData().set(ClangAstData.DEBUG, true); - } + return arguments; + } - // Create temporary working folder, in order to support running several dumps in parallel + private ParsedDump runDumper(List arguments, File sourceFile, String id, DataStore config, + OutputStream cacheOutput) { + try { + // Create temporary working folder only after the cache lookup has missed. A hit has no working folder. lastWorkingFolder = SpecsIo.mkdir(baseFolder, sourceFile.getName() + "_" + id); // Ensure folder is empty @@ -305,9 +345,9 @@ else if (SourceType.isHeader(sourceFile)) { workingFolders.add(lastWorkingFolder); - output = SpecsSystem.runProcess(arguments, lastWorkingFolder, + ProcessOutput output = SpecsSystem.runProcess(arguments, lastWorkingFolder, this::processOutput, - inputStream -> this.processStdErr(inputStream, config.get(ClavaNode.CONTEXT))); + inputStream -> this.processStdErr(inputStream, config.get(ClavaNode.CONTEXT), cacheOutput, true)); if (output.isError()) { ClavaLog.debug("Dumper returned an error value: '" + output.getReturnValue() + "'"); @@ -318,22 +358,27 @@ else if (SourceType.isHeader(sourceFile)) { throw new RuntimeException("Exception while processing the output streams", exception); }); - parsedData = output.getStdErr(); - Objects.requireNonNull(parsedData, () -> "Did not expect error output to be null"); - parsedData.set(ClangAstData.HAS_ERRORS, output.isError()); + ParsedDump parsedDump = Objects.requireNonNull(output.getStdErr(), + () -> "Did not expect error output to be null"); + parsedDump.data().set(ClangAstData.HAS_ERRORS, output.isError()); // If console output streaming is disabled, show output only at the end if (!streamConsoleOutput) { ClavaLog.info(output.getStdOut()); } - if (lineStreamParser.hasExceptions()) { + if (parsedDump.parserHadExceptions()) { SpecsLogs.warn("Exceptions happened while parsing the file '" + sourceFile.getAbsolutePath() + "'"); } + + return parsedDump; } catch (Exception e) { throw new RuntimeException("Error while running Clang AST dumper", e); } + } + private ClangAstData materializeTranslationUnit(ParsedDump parsedDump, File sourceFile, DataStore config) { + ClangAstData parsedData = parsedDump.data(); ClangAstParser clangStreamParser = new ClangAstParser(parsedData, SpecsSystem.isDebug(), config); TranslationUnit tUnit = clangStreamParser.parseTu(sourceFile); @@ -343,6 +388,28 @@ else if (SourceType.isHeader(sourceFile)) { return parsedData; } + private Collection getDependencies(ParsedDump parsedDump) { + Set dependencies = new HashSet<>(); + dependencies.add(clangExecutable.toPath()); + + Map idToFilename = parsedDump.data().get(ClangAstData.ID_TO_FILENAME_MAP); + if (idToFilename != null) { + for (String filename : idToFilename.values()) { + if (filename == null) { + continue; + } + + try { + dependencies.add(Path.of(filename)); + } catch (InvalidPathException ignored) { + // Pseudo-paths from the dumper are not file dependencies. + } + } + } + + return dependencies; + } + private void addCudaPathArgument(List arguments, String cudaPath) { var useBuiltinCudaLib = cudaPath.toUpperCase().equals(CodeParser.getBuiltinOption()); @@ -379,7 +446,8 @@ private String processOutput(InputStream inputStream) { return output.toString(); } - private ClangAstData processStdErr(InputStream inputStream, ClavaContext context) { + private ParsedDump processStdErr(InputStream inputStream, ClavaContext context, OutputStream cacheOutput, + boolean closeInputStream) { // Create LineStreamParser try (LineStreamParser lineStreamParser = ClangStreamParserV2.newInstance(context)) { @@ -392,20 +460,62 @@ private ClangAstData processStdErr(InputStream inputStream, ClavaContext context File dumpfile = SpecsSystem.isDebug() ? new File(STDERR_DUMP_FILENAME) : null; // Parse input stream - String linesNotParsed = lineStreamParser.parse(inputStream, dumpfile); + String linesNotParsed = lineStreamParser.parse( + new TeeInputStream(inputStream, cacheOutput, closeInputStream), dumpfile); // Add lines not parsed to DataStore ClangAstData data = lineStreamParser.getData(); data.set(ClangAstData.LINES_NOT_PARSED, linesNotParsed); - // Return data - return data; + // Return data and retain parser exceptions for the cache publication decision. + return new ParsedDump(data, lineStreamParser.hasExceptions()); } catch (Exception e) { throw new RuntimeException("Error while parsing output of Clang AST dumper", e); } } + private record ParsedDump(ClangAstData data, boolean parserHadExceptions) { + } + + /** Copies stderr bytes as they are consumed, without buffering the complete dumper output. */ + private static final class TeeInputStream extends InputStream { + private final InputStream delegate; + private final OutputStream copy; + private final boolean closeDelegate; + + private TeeInputStream(InputStream delegate, OutputStream copy, boolean closeDelegate) { + this.delegate = delegate; + this.copy = copy; + this.closeDelegate = closeDelegate; + } + + @Override + public int read() throws IOException { + int value = delegate.read(); + if (value != -1) { + copy.write(value); + } + return value; + } + + @Override + public int read(byte[] bytes, int offset, int length) throws IOException { + int count = delegate.read(bytes, offset, length); + if (count > 0) { + copy.write(bytes, offset, count); + } + return count; + } + + @Override + public void close() throws IOException { + if (closeDelegate) { + delegate.close(); + } + } + } + /** * TODO: Current implementation only shows the last file, show all files */ diff --git a/ClangAstParser/test/pt/up/fe/specs/clang/AstDumpCacheTest.java b/ClangAstParser/test/pt/up/fe/specs/clang/AstDumpCacheTest.java new file mode 100644 index 000000000..9aca2f147 --- /dev/null +++ b/ClangAstParser/test/pt/up/fe/specs/clang/AstDumpCacheTest.java @@ -0,0 +1,260 @@ +/** + * Copyright 2026 SPeCS. + *

+ * 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. + */ + +package pt.up.fe.specs.clang; + +import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.io.TempDir; + +import java.io.IOException; +import java.io.InputStream; +import java.nio.charset.StandardCharsets; +import java.nio.file.Files; +import java.nio.file.Path; +import java.nio.file.StandardOpenOption; +import java.nio.file.attribute.FileTime; +import java.time.Duration; +import java.time.Instant; +import java.util.Collection; +import java.util.List; +import java.util.Optional; +import java.util.concurrent.CountDownLatch; +import java.util.concurrent.ExecutorService; +import java.util.concurrent.Executors; +import java.util.concurrent.Future; +import java.util.concurrent.TimeUnit; +import java.util.concurrent.atomic.AtomicInteger; +import java.util.stream.Stream; + +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertFalse; +import static org.junit.jupiter.api.Assertions.assertNotNull; +import static org.junit.jupiter.api.Assertions.assertTrue; + +public class AstDumpCacheTest { + + @TempDir + Path tempFolder; + + @Test + public void successfulCaptureIsAStableHitWithoutProducerRerun() throws IOException { + Path source = source("source.cpp", "int value = 1;\n"); + AstDumpCache cache = cache(source, "clang", source.toString(), "-std=c++17"); + AtomicInteger producerRuns = new AtomicInteger(); + + String produced = cache.capture(output -> { + producerRuns.incrementAndGet(); + output.write("dump bytes\n".getBytes(StandardCharsets.UTF_8)); + return "parsed"; + }, ignored -> List.of(source), ignored -> true); + + Optional loaded = cache.load(this::readUtf8); + Optional loadedAgain = cache.load(this::readUtf8); + + assertEquals("parsed", produced); + assertEquals(Optional.of("dump bytes\n"), loaded); + assertEquals(loaded, loadedAgain); + assertEquals(1, producerRuns.get()); + } + + @Test + public void sourceContentChangesTheKeyAndMisses() throws IOException { + Path source = source("source.cpp", "int value = 1;\n"); + AstDumpCache cache = cache(source, "clang", source.toString()); + AtomicInteger producerRuns = new AtomicInteger(); + + capture(cache, "first", List.of(source), producerRuns); + Files.writeString(source, "int value = 2;\n"); + + assertTrue(cache.load(this::readUtf8).isEmpty()); + capture(cache, "second", List.of(source), producerRuns); + assertEquals(2, producerRuns.get()); + } + + @Test + public void commandOrderAndValueArePartOfTheKey() throws IOException { + Path source = source("source.cpp", "int value;\n"); + AstDumpCache original = cache(source, "clang", "-a", "-b"); + capture(original, "original", List.of(source), new AtomicInteger()); + + AstDumpCache orderChanged = cache(source, "clang", "-b", "-a"); + AstDumpCache valueChanged = cache(source, "clang", "-a", "-c"); + + assertTrue(orderChanged.load(this::readUtf8).isEmpty()); + assertTrue(valueChanged.load(this::readUtf8).isEmpty()); + } + + @Test + public void transitiveDependencyChangeInvalidatesHit() throws IOException { + Path source = source("source.cpp", "#include \"header.h\"\n"); + Path header = source("header.h", "#define VALUE 1\n"); + AstDumpCache cache = cache(source, "clang", source.toString()); + + capture(cache, "with header", List.of(source, header), new AtomicInteger()); + assertTrue(cache.load(this::readUtf8).isPresent()); + + Files.writeString(header, "#define VALUE 2\n"); + + assertTrue(cache.load(this::readUtf8).isEmpty()); + } + + @Test + public void corruptManifestAndGzipFailOpen() throws IOException { + Path source = source("source.cpp", "int value;\n"); + AstDumpCache cache = cache(source, "clang", source.toString()); + capture(cache, "valid", List.of(source), new AtomicInteger()); + + Path entry = onlyEntry(); + Files.writeString(entry.resolve("manifest.json"), "not json"); + assertTrue(cache.load(this::readUtf8).isEmpty()); + assertFalse(Files.exists(entry)); + + capture(cache, "valid again", List.of(source), new AtomicInteger()); + entry = onlyEntry(); + Files.write(entry.resolve("dump.gz"), new byte[] {1, 2, 3, 4}, StandardOpenOption.TRUNCATE_EXISTING); + + assertTrue(cache.load(this::readUtf8).isEmpty()); + assertFalse(Files.exists(entry)); + } + + @Test + public void failedProducerIsNeverPublished() throws IOException { + Path source = source("source.cpp", "int value;\n"); + AstDumpCache cache = cache(source, "clang", source.toString()); + + cache.capture(output -> { + output.write("failed dump".getBytes(StandardCharsets.UTF_8)); + return "failed"; + }, ignored -> List.of(source), ignored -> false); + + assertEquals(List.of(), entryDirectories()); + assertTrue(cache.load(this::readUtf8).isEmpty()); + } + + @Test + public void concurrentWritersPublishOneCompleteEntry() throws Exception { + Path source = source("source.cpp", "int value;\n"); + AstDumpCache first = cache(source, "clang", source.toString()); + AstDumpCache second = cache(source, "clang", source.toString()); + ExecutorService executor = Executors.newFixedThreadPool(2); + CountDownLatch producerReady = new CountDownLatch(2); + CountDownLatch releaseProducers = new CountDownLatch(1); + + try { + Future firstResult = executor.submit(() -> concurrentCapture(first, "first", producerReady, + releaseProducers)); + Future secondResult = executor.submit(() -> concurrentCapture(second, "second", producerReady, + releaseProducers)); + + assertTrue(producerReady.await(30, TimeUnit.SECONDS)); + releaseProducers.countDown(); + assertNotNull(firstResult.get(30, TimeUnit.SECONDS)); + assertNotNull(secondResult.get(30, TimeUnit.SECONDS)); + } finally { + executor.shutdownNow(); + } + + assertEquals(1, entryDirectories().size()); + String dump = first.load(this::readUtf8).orElseThrow(); + assertTrue(dump.equals("first") || dump.equals("second")); + } + + @Test + public void staleCleanupPreservesCurrentEntryAndRemovesOtherEntry() throws IOException { + Path currentSource = source("current.cpp", "int current;\n"); + Path staleSource = source("stale.cpp", "int stale;\n"); + AstDumpCache current = cache(currentSource, "clang", currentSource.toString()); + AstDumpCache stale = cache(staleSource, "clang", staleSource.toString()); + + capture(current, "current", List.of(currentSource), new AtomicInteger()); + capture(stale, "stale", List.of(staleSource), new AtomicInteger()); + Path currentEntry = entryForSource("current"); + Path staleEntry = entryForSource("stale"); + Instant old = Instant.now().minus(Duration.ofDays(61)); + Files.setLastModifiedTime(currentEntry, FileTime.from(old)); + Files.setLastModifiedTime(staleEntry, FileTime.from(old)); + + assertEquals(Optional.of("current"), current.load(this::readUtf8)); + assertTrue(Files.exists(currentEntry)); + assertFalse(Files.exists(staleEntry)); + } + + private String concurrentCapture(AstDumpCache cache, String dump, CountDownLatch producerReady, + CountDownLatch releaseProducers) { + return cache.capture(output -> { + producerReady.countDown(); + await(releaseProducers); + output.write(dump.getBytes(StandardCharsets.UTF_8)); + return dump; + }, ignored -> List.of(), ignored -> true); + } + + private void capture(AstDumpCache cache, String dump, Collection dependencies, AtomicInteger runs) { + cache.capture(output -> { + runs.incrementAndGet(); + output.write(dump.getBytes(StandardCharsets.UTF_8)); + return dump; + }, ignored -> dependencies, ignored -> true); + } + + private AstDumpCache cache(Path source, String... command) { + return new AstDumpCache(tempFolder.resolve("cache"), source, List.of(command)); + } + + private Path source(String filename, String contents) throws IOException { + return Files.writeString(tempFolder.resolve(filename), contents); + } + + private String readUtf8(InputStream input) throws IOException { + return new String(input.readAllBytes(), StandardCharsets.UTF_8); + } + + private Path onlyEntry() throws IOException { + List entries = entryDirectories(); + assertEquals(1, entries.size()); + return entries.get(0); + } + + private List entryDirectories() throws IOException { + Path entries = tempFolder.resolve("cache/ast-dumps/v1/entries"); + if (!Files.isDirectory(entries)) { + return List.of(); + } + + try (Stream paths = Files.list(entries)) { + return paths.filter(Files::isDirectory) + .filter(path -> !path.getFileName().toString().startsWith(".")) + .toList(); + } + } + + private Path entryForSource(String dump) throws IOException { + // Manifest paths are absolute, so the source filename is enough to distinguish these test entries. + for (Path entry : entryDirectories()) { + Path manifest = entry.resolve("manifest.json"); + if (Files.readString(manifest).contains("/" + dump + ".cpp")) { + return entry; + } + } + throw new AssertionError("Could not find entry for " + dump); + } + + private static void await(CountDownLatch latch) { + try { + assertTrue(latch.await(30, TimeUnit.SECONDS)); + } catch (InterruptedException e) { + Thread.currentThread().interrupt(); + throw new RuntimeException(e); + } + } +} diff --git a/ClangAstParser/test/pt/up/fe/specs/clang/dumper/ClangAstDumperCacheIntegrationTest.java b/ClangAstParser/test/pt/up/fe/specs/clang/dumper/ClangAstDumperCacheIntegrationTest.java new file mode 100644 index 000000000..f367503a5 --- /dev/null +++ b/ClangAstParser/test/pt/up/fe/specs/clang/dumper/ClangAstDumperCacheIntegrationTest.java @@ -0,0 +1,124 @@ +/** + * Copyright 2026 SPeCS. + * + * 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. + */ + +package pt.up.fe.specs.clang.dumper; + +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertNotNull; +import static org.junit.jupiter.api.Assertions.assertNull; +import static org.junit.jupiter.api.Assertions.assertTrue; + +import java.io.File; +import java.io.IOException; +import java.nio.file.Files; +import java.nio.file.Path; +import java.util.List; + +import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.io.TempDir; +import org.suikasoft.jOptions.Interfaces.DataStore; + +import pt.up.fe.specs.clang.ClangAstKeys; +import pt.up.fe.specs.clang.ClangFiles; +import pt.up.fe.specs.clang.ClangResources; +import pt.up.fe.specs.clang.LibcMode; +import pt.up.fe.specs.clang.codeparser.CodeParser; +import pt.up.fe.specs.clava.ClavaNode; +import pt.up.fe.specs.clava.context.ClavaContext; +import pt.up.fe.specs.clava.language.Standard; +import pt.up.fe.specs.util.SpecsSystem; + +class ClangAstDumperCacheIntegrationTest { + + @TempDir + Path tempFolder; + + @Test + void cacheHitAvoidsProcessAndMaterializesEquivalentTranslationUnit() throws IOException { + SpecsSystem.programStandardInit(); + + File source = Files.writeString(tempFolder.resolve("source.cpp"), "int value = 1;\n").toFile(); + CodeParser parserConfig = parserConfig(); + ClangFiles clangFiles = new ClangResources(parserConfig).getClangFiles(LibcMode.BUILTIN_AND_LIBC); + File workingFolder = tempFolder.resolve("working").toFile(); + + ClangAstDumper missDumper = newDumper(parserConfig, clangFiles, workingFolder); + ClangAstData miss = missDumper.parse(source, "1", Standard.CXX17, config()); + assertNotNull(missDumper.getLastWorkingFolder(), "a cache miss must launch the dumper"); + + ClangAstData hit = missDumper.parse(source, "1", Standard.CXX17, config()); + assertNull(missDumper.getLastWorkingFolder(), "a cache hit must not expose a previous working folder"); + assertEquals(miss.get(ClangAstData.TRANSLATION_UNIT).getCode(), + hit.get(ClangAstData.TRANSLATION_UNIT).getCode()); + assertEquals(miss.get(ClangAstData.ID_TO_FILENAME_MAP), hit.get(ClangAstData.ID_TO_FILENAME_MAP)); + } + + @Test + void effectiveCommandChangeMissesAndShowDumpBypassesCache() throws IOException { + SpecsSystem.programStandardInit(); + + File source = Files.writeString(tempFolder.resolve("source.cpp"), "int value = 1;\n").toFile(); + CodeParser parserConfig = parserConfig(); + ClangFiles clangFiles = new ClangResources(parserConfig).getClangFiles(LibcMode.BUILTIN_AND_LIBC); + File workingFolder = tempFolder.resolve("working").toFile(); + + newDumper(parserConfig, clangFiles, workingFolder).parse(source, "1", Standard.CXX17, config()); + + ClangAstDumper changedDumper = newDumper(parserConfig, clangFiles, workingFolder); + changedDumper.parse(source, "1", Standard.CXX17, config("-DVALUE=2")); + assertNotNull(changedDumper.getLastWorkingFolder(), "an effective command change must miss the cache"); + + ClangAstDumper showDumper = newDumper(parserConfig, clangFiles, workingFolder); + DataStore showConfig = config(); + showConfig.set(CodeParser.SHOW_CLANG_DUMP, true); + showDumper.parse(source, "1", Standard.CXX17, showConfig); + assertNotNull(showDumper.getLastWorkingFolder(), "SHOW_CLANG_DUMP must bypass cached loading"); + } + + @Test + void failedDumperOutputIsNotReused() throws IOException { + SpecsSystem.programStandardInit(); + + File source = Files.writeString(tempFolder.resolve("source.cpp"), "int value = ;\n").toFile(); + CodeParser parserConfig = parserConfig(); + ClangFiles clangFiles = new ClangResources(parserConfig).getClangFiles(LibcMode.BUILTIN_AND_LIBC); + File workingFolder = tempFolder.resolve("working").toFile(); + + ClangAstDumper firstDumper = newDumper(parserConfig, clangFiles, workingFolder); + ClangAstData first = firstDumper.parse(source, "1", Standard.CXX17, config()); + assertTrue(first.get(ClangAstData.HAS_ERRORS)); + + ClangAstDumper secondDumper = newDumper(parserConfig, clangFiles, workingFolder); + ClangAstData second = secondDumper.parse(source, "1", Standard.CXX17, config()); + assertTrue(second.get(ClangAstData.HAS_ERRORS)); + assertNotNull(secondDumper.getLastWorkingFolder(), "failed output must not be loaded from the cache"); + } + + private CodeParser parserConfig() { + CodeParser parserConfig = CodeParser.newInstance(); + parserConfig.set(CodeParser.DUMPER_FOLDER, tempFolder.resolve("cache").toFile()); + return parserConfig; + } + + private ClangAstDumper newDumper(CodeParser parserConfig, ClangFiles clangFiles, File workingFolder) { + return new ClangAstDumper(false, clangFiles.clangExecutable(), clangFiles.builtinIncludes(), + clangFiles.systemResourceDir(), parserConfig).setBaseFolder(workingFolder); + } + + private DataStore config(String... flags) { + DataStore config = ClangAstKeys.toDataStore(List.of(flags)); + config.set(ClavaNode.CONTEXT, new ClavaContext()); + config.set(ClangAstKeys.LIBC_CXX_MODE, LibcMode.BUILTIN_AND_LIBC); + return config; + } +} From 01398630ee7ff7d8733d874165eaf30169fbd339 Mon Sep 17 00:00:00 2001 From: "L. Sousa" Date: Tue, 25 Aug 2026 19:49:30 +0100 Subject: [PATCH 2/7] fix(clang): harden AST dump cache validation --- .../pt/up/fe/specs/clang/AstDumpCache.java | 42 +++-- .../fe/specs/clang/dumper/ClangAstDumper.java | 148 +++++++++++++++++- .../up/fe/specs/clang/AstDumpCacheTest.java | 13 +- .../ClangAstDumperCacheIntegrationTest.java | 12 +- 4 files changed, 193 insertions(+), 22 deletions(-) diff --git a/ClangAstParser/src/pt/up/fe/specs/clang/AstDumpCache.java b/ClangAstParser/src/pt/up/fe/specs/clang/AstDumpCache.java index 3af09fa46..3d03b0b16 100644 --- a/ClangAstParser/src/pt/up/fe/specs/clang/AstDumpCache.java +++ b/ClangAstParser/src/pt/up/fe/specs/clang/AstDumpCache.java @@ -217,8 +217,10 @@ public T capture(DumpProducer producer, CacheFiles.publish(staging.path(), entry); } catch (RuntimeException | IOException e) { reportCacheFailure("Could not publish AST dump cache entry '" + entry + "'", e); - CacheFiles.deleteQuietly(staging.path()); } finally { + // publish() can report an existing destination without moving this writer's staging directory. Always + // remove it before releasing the lock, including the losing-writer case. + CacheFiles.deleteQuietly(staging.path()); closeStagingQuietly(staging); } @@ -370,8 +372,9 @@ private static void putLong(MessageDigest digest, long value) { } private boolean claimValidEntry(Path entry) { + boolean claimed; try { - return CacheFiles.withMaintenanceLock(cacheRoot, () -> { + claimed = CacheFiles.withMaintenanceLock(cacheRoot, () -> { try { Files.createDirectories(entriesRoot); } catch (IOException e) { @@ -390,24 +393,35 @@ private boolean claimValidEntry(Path entry) { if (!Files.isDirectory(entry)) { return false; } - - try { - if (!isManifestValid(entry)) { - CacheFiles.deleteQuietly(entry); - return false; - } - } catch (RuntimeException e) { - CacheFiles.deleteQuietly(entry); - throw e; - } - - CacheFiles.touch(entry); return true; }); } catch (RuntimeException e) { reportCacheFailure("Could not validate AST dump cache entry '" + entry + "'", e); return false; } + + if (!claimed) { + return false; + } + + // Hashing dependency files can be expensive. The entry was touched and excluded from cleanup above, so the + // full manifest validation can safely run without holding the process-wide maintenance lock. + boolean valid; + try { + valid = isManifestValid(entry); + } catch (RuntimeException e) { + reportCacheFailure("Could not validate AST dump cache entry '" + entry + "'", e); + valid = false; + } + + if (!valid) { + // Reacquire the maintenance lock before deleting. This keeps invalid-entry removal ordered with cleanup + // and with another reader claiming the same entry. + deleteEntryQuietly(entry); + return false; + } + + return true; } private void prepareEntriesDirectory(Path currentEntry) { diff --git a/ClangAstParser/src/pt/up/fe/specs/clang/dumper/ClangAstDumper.java b/ClangAstParser/src/pt/up/fe/specs/clang/dumper/ClangAstDumper.java index 258bb4ca6..073675aac 100644 --- a/ClangAstParser/src/pt/up/fe/specs/clang/dumper/ClangAstDumper.java +++ b/ClangAstParser/src/pt/up/fe/specs/clang/dumper/ClangAstDumper.java @@ -52,6 +52,7 @@ import java.util.List; import java.util.Map; import java.util.Objects; +import java.util.Optional; import java.util.Set; /** @@ -70,6 +71,7 @@ public static boolean usePlugin() { private final static String CLANG_DUMP_FILENAME = "clangDump.txt"; private final static String STDERR_DUMP_FILENAME = "stderr.txt"; + private static final String DEPENDENCY_DOT_FILENAME = "clangDependencies.dot"; private static final List CLANG_AST_DUMPER_TEMP_FILES = List.of("includes.txt", CLANG_DUMP_FILENAME, // "clavaDump.txt", "nodetypes.txt", "types.txt", "is_temporary.txt", "template_args.txt", @@ -188,7 +190,8 @@ private ClangAstData parsePrivate(File sourceFile, String id, Standard standard, parsedDump = cache.capture(cacheOutput -> runDumper(arguments, sourceFile, id, config, cacheOutput), this::getDependencies, - result -> !result.data().get(ClangAstData.HAS_ERRORS) && !result.parserHadExceptions()); + result -> !result.data().get(ClangAstData.HAS_ERRORS) && !result.parserHadExceptions() + && result.dependenciesAvailable()); } else { // clangDump.txt is a side effect of the process and has no cache representation. In particular, do not // publish a cache entry that would make a subsequent SHOW_CLANG_DUMP parse silently lose that output. @@ -331,6 +334,16 @@ else if (SourceType.isHeader(sourceFile)) { arguments.addAll(config.get(ClavaOptions.FLAGS_LIST)); + // ClangTool does not accept the driver's -MMD/-MF options. Its frontend dependency-dot option is accepted + // through -Xclang and emits the complete transitive include graph in the per-invocation working directory. + // Keep the filename stable: the working folder is intentionally not part of the cache key. + arguments.add("-Xclang"); + arguments.add("-dependency-dot"); + arguments.add("-Xclang"); + arguments.add(DEPENDENCY_DOT_FILENAME); + arguments.add("-Xclang"); + arguments.add("-sys-header-deps"); + return arguments; } @@ -361,6 +374,8 @@ private ParsedDump runDumper(List arguments, File sourceFile, String id, ParsedDump parsedDump = Objects.requireNonNull(output.getStdErr(), () -> "Did not expect error output to be null"); parsedDump.data().set(ClangAstData.HAS_ERRORS, output.isError()); + DependencyDot dependencyDot = readDependencyDot(lastWorkingFolder); + parsedDump = parsedDump.withDependencies(dependencyDot.paths(), dependencyDot.available()); // If console output streaming is disabled, show output only at the end if (!streamConsoleOutput) { @@ -389,7 +404,7 @@ private ClangAstData materializeTranslationUnit(ParsedDump parsedDump, File sour } private Collection getDependencies(ParsedDump parsedDump) { - Set dependencies = new HashSet<>(); + Set dependencies = new HashSet<>(parsedDump.dependencies()); dependencies.add(clangExecutable.toPath()); Map idToFilename = parsedDump.data().get(ClangAstData.ID_TO_FILENAME_MAP); @@ -468,14 +483,139 @@ private ParsedDump processStdErr(InputStream inputStream, ClavaContext context, data.set(ClangAstData.LINES_NOT_PARSED, linesNotParsed); // Return data and retain parser exceptions for the cache publication decision. - return new ParsedDump(data, lineStreamParser.hasExceptions()); + return new ParsedDump(data, lineStreamParser.hasExceptions(), Set.of(), true); } catch (Exception e) { throw new RuntimeException("Error while parsing output of Clang AST dumper", e); } } - private record ParsedDump(ClangAstData data, boolean parserHadExceptions) { + private record ParsedDump(ClangAstData data, boolean parserHadExceptions, Collection dependencies, + boolean dependenciesAvailable) { + + private ParsedDump withDependencies(Collection dependencies, boolean dependenciesAvailable) { + return new ParsedDump(data, parserHadExceptions, dependencies, dependenciesAvailable); + } + } + + private record DependencyDot(Collection paths, boolean available) { + } + + private DependencyDot readDependencyDot(File workingFolder) { + Path dependencyDot = workingFolder.toPath().resolve(DEPENDENCY_DOT_FILENAME); + if (!java.nio.file.Files.isRegularFile(dependencyDot)) { + SpecsLogs.debug(() -> "Clang dumper did not produce dependency file '" + dependencyDot + "'"); + return new DependencyDot(Set.of(), false); + } + + try { + Set dependencies = new HashSet<>(); + int labels = 0; + for (String line : java.nio.file.Files.readAllLines(dependencyDot)) { + Optional label = parseDependencyDotLabel(line); + if (label.isEmpty()) { + continue; + } + + labels++; + Optional path = resolveDependencyPath(label.get(), workingFolder); + if (path.isEmpty()) { + SpecsLogs.debug(() -> "Could not resolve dependency from Clang dependency file line '" + line + + "'"); + return new DependencyDot(Set.of(), false); + } + + dependencies.add(path.get()); + } + + return new DependencyDot(dependencies, labels > 0); + } catch (IOException | InvalidPathException e) { + SpecsLogs.debug(() -> "Could not read Clang dependency file '" + dependencyDot + "': " + e.getMessage()); + return new DependencyDot(Set.of(), false); + } + } + + private Optional parseDependencyDotLabel(String line) { + int start = line.indexOf("label=\""); + if (start < 0) { + return Optional.empty(); + } + + StringBuilder escaped = new StringBuilder(); + boolean isEscaped = false; + for (int i = start + "label=\"".length(); i < line.length(); i++) { + char current = line.charAt(i); + if (isEscaped) { + escaped.append('\\').append(current); + isEscaped = false; + } else if (current == '\\') { + isEscaped = true; + } else if (current == '"') { + return Optional.of(decodeDependencyDotLabel(escaped.toString())); + } else { + escaped.append(current); + } + } + + return Optional.empty(); + } + + private String decodeDependencyDotLabel(String escaped) { + StringBuilder decoded = new StringBuilder(escaped.length()); + boolean isEscaped = false; + for (int i = 0; i < escaped.length(); i++) { + char current = escaped.charAt(i); + if (!isEscaped) { + if (current == '\\') { + isEscaped = true; + } else { + decoded.append(current); + } + continue; + } + + decoded.append(switch (current) { + case 'n' -> '\n'; + case 'r' -> '\r'; + case 't' -> '\t'; + default -> current; + }); + isEscaped = false; + } + + if (isEscaped) { + decoded.append('\\'); + } + + return decoded.toString(); + } + + private Optional resolveDependencyPath(String dependency, File workingFolder) { + Path path = Path.of(dependency); + List candidates = new ArrayList<>(); + if (path.isAbsolute()) { + candidates.add(path); + } else { + candidates.add(workingFolder.toPath().resolve(path)); + // Clang's dependency-dot output strips the leading slash from absolute POSIX paths. + if (File.separatorChar == '/') { + candidates.add(Path.of(File.separator).resolve(path)); + } + candidates.add(path); + } + + for (Path candidate : candidates) { + try { + Path canonical = candidate.toRealPath(); + if (java.nio.file.Files.isRegularFile(canonical)) { + return Optional.of(canonical); + } + } catch (IOException ignored) { + // The compiler can report pseudo-paths or files that disappear between parsing and manifest build. + } + } + + return Optional.empty(); } /** Copies stderr bytes as they are consumed, without buffering the complete dumper output. */ diff --git a/ClangAstParser/test/pt/up/fe/specs/clang/AstDumpCacheTest.java b/ClangAstParser/test/pt/up/fe/specs/clang/AstDumpCacheTest.java index 9aca2f147..453c51fd6 100644 --- a/ClangAstParser/test/pt/up/fe/specs/clang/AstDumpCacheTest.java +++ b/ClangAstParser/test/pt/up/fe/specs/clang/AstDumpCacheTest.java @@ -165,6 +165,8 @@ public void concurrentWritersPublishOneCompleteEntry() throws Exception { } assertEquals(1, entryDirectories().size()); + assertTrue(entryChildren().stream() + .noneMatch(path -> path.getFileName().toString().startsWith("."))); String dump = first.load(this::readUtf8).orElseThrow(); assertTrue(dump.equals("first") || dump.equals("second")); } @@ -226,15 +228,20 @@ private Path onlyEntry() throws IOException { } private List entryDirectories() throws IOException { + return entryChildren().stream() + .filter(Files::isDirectory) + .filter(path -> !path.getFileName().toString().startsWith(".")) + .toList(); + } + + private List entryChildren() throws IOException { Path entries = tempFolder.resolve("cache/ast-dumps/v1/entries"); if (!Files.isDirectory(entries)) { return List.of(); } try (Stream paths = Files.list(entries)) { - return paths.filter(Files::isDirectory) - .filter(path -> !path.getFileName().toString().startsWith(".")) - .toList(); + return paths.toList(); } } diff --git a/ClangAstParser/test/pt/up/fe/specs/clang/dumper/ClangAstDumperCacheIntegrationTest.java b/ClangAstParser/test/pt/up/fe/specs/clang/dumper/ClangAstDumperCacheIntegrationTest.java index f367503a5..7755870d6 100644 --- a/ClangAstParser/test/pt/up/fe/specs/clang/dumper/ClangAstDumperCacheIntegrationTest.java +++ b/ClangAstParser/test/pt/up/fe/specs/clang/dumper/ClangAstDumperCacheIntegrationTest.java @@ -34,6 +34,7 @@ import pt.up.fe.specs.clang.LibcMode; import pt.up.fe.specs.clang.codeparser.CodeParser; import pt.up.fe.specs.clava.ClavaNode; +import pt.up.fe.specs.clava.ast.expr.IntegerLiteral; import pt.up.fe.specs.clava.context.ClavaContext; import pt.up.fe.specs.clava.language.Standard; import pt.up.fe.specs.util.SpecsSystem; @@ -47,7 +48,9 @@ class ClangAstDumperCacheIntegrationTest { void cacheHitAvoidsProcessAndMaterializesEquivalentTranslationUnit() throws IOException { SpecsSystem.programStandardInit(); - File source = Files.writeString(tempFolder.resolve("source.cpp"), "int value = 1;\n").toFile(); + File header = Files.writeString(tempFolder.resolve("header.h"), "#define VALUE 1\n").toFile(); + File source = Files.writeString(tempFolder.resolve("source.cpp"), + "#include \"header.h\"\nint value = VALUE;\n").toFile(); CodeParser parserConfig = parserConfig(); ClangFiles clangFiles = new ClangResources(parserConfig).getClangFiles(LibcMode.BUILTIN_AND_LIBC); File workingFolder = tempFolder.resolve("working").toFile(); @@ -61,6 +64,13 @@ void cacheHitAvoidsProcessAndMaterializesEquivalentTranslationUnit() throws IOEx assertEquals(miss.get(ClangAstData.TRANSLATION_UNIT).getCode(), hit.get(ClangAstData.TRANSLATION_UNIT).getCode()); assertEquals(miss.get(ClangAstData.ID_TO_FILENAME_MAP), hit.get(ClangAstData.ID_TO_FILENAME_MAP)); + + Files.writeString(header.toPath(), "#define VALUE 2\n"); + ClangAstDumper changedHeaderDumper = newDumper(parserConfig, clangFiles, workingFolder); + ClangAstData changedHeader = changedHeaderDumper.parse(source, "1", Standard.CXX17, config()); + assertNotNull(changedHeaderDumper.getLastWorkingFolder(), "a transitive header change must miss the cache"); + assertEquals("2", changedHeader.get(ClangAstData.TRANSLATION_UNIT).getDescendants(IntegerLiteral.class) + .stream().findFirst().orElseThrow().getCode()); } @Test From 084942c1b12f694f632796f78ba16aba7d151c9a Mon Sep 17 00:00:00 2001 From: "L. Sousa" Date: Tue, 25 Aug 2026 20:21:10 +0100 Subject: [PATCH 3/7] fix(clang): bypass unsafe AST cache inputs --- .../pt/up/fe/specs/clang/AstDumpCache.java | 60 +++++++++++++++++ .../fe/specs/clang/dumper/ClangAstDumper.java | 29 ++++++++- .../up/fe/specs/clang/AstDumpCacheTest.java | 64 +++++++++++++++++++ .../ClangAstDumperCacheIntegrationTest.java | 34 ++++++++++ 4 files changed, 184 insertions(+), 3 deletions(-) diff --git a/ClangAstParser/src/pt/up/fe/specs/clang/AstDumpCache.java b/ClangAstParser/src/pt/up/fe/specs/clang/AstDumpCache.java index 3d03b0b16..90b1c9876 100644 --- a/ClangAstParser/src/pt/up/fe/specs/clang/AstDumpCache.java +++ b/ClangAstParser/src/pt/up/fe/specs/clang/AstDumpCache.java @@ -62,6 +62,11 @@ public final class AstDumpCache { private static final String DUMP_FILENAME = "dump.gz"; private static final Duration STALE_ENTRY_AGE = Duration.ofDays(60); private static final Gson GSON = new Gson(); + private static final byte[][] VOLATILE_MACROS = { + "__TIME__".getBytes(StandardCharsets.US_ASCII), + "__DATE__".getBytes(StandardCharsets.US_ASCII), + "__TIMESTAMP__".getBytes(StandardCharsets.US_ASCII) + }; private final Path cacheRoot; private final Path entriesRoot; @@ -511,6 +516,11 @@ private Manifest buildManifest(Collection namedDependencies) { var entries = new ArrayList(); for (Map.Entry dependency : dependencies.entrySet()) { + if (containsVolatileMacro(dependency.getValue())) { + throw new RuntimeException("AST dump depends on volatile preprocessor macro in '" + + dependency.getKey() + "'"); + } + entries.add(new Dependency(dependency.getKey(), CacheFiles.calculateSha256(dependency.getValue()))); } @@ -544,6 +554,56 @@ private static void addOptionalDependency(Map dependencies, Path p } } + private static boolean containsVolatileMacro(Path path) { + int maximumMacroLength = 0; + for (byte[] macro : VOLATILE_MACROS) { + maximumMacroLength = Math.max(maximumMacroLength, macro.length); + } + + byte[] window = new byte[maximumMacroLength]; + int windowSize = 0; + boolean macroFound = false; + boolean binaryFound = false; + try (InputStream input = Files.newInputStream(path)) { + int next; + while ((next = input.read()) != -1) { + if (next == 0) { + binaryFound = true; + } + + if (windowSize < window.length) { + window[windowSize++] = (byte) next; + } else { + System.arraycopy(window, 1, window, 0, window.length - 1); + window[window.length - 1] = (byte) next; + } + + for (byte[] macro : VOLATILE_MACROS) { + if (windowSize < macro.length) { + continue; + } + + int start = windowSize - macro.length; + boolean matches = true; + for (int index = 0; index < macro.length; index++) { + if (window[start + index] != macro[index]) { + matches = false; + break; + } + } + + if (matches) { + macroFound = true; + } + } + } + + return macroFound && !binaryFound; + } catch (IOException e) { + throw new UncheckedIOException("Could not scan AST dump dependency '" + path + "'", e); + } + } + private static boolean shouldPublish(T result, Predicate publishDecision) { try { return publishDecision.test(result); diff --git a/ClangAstParser/src/pt/up/fe/specs/clang/dumper/ClangAstDumper.java b/ClangAstParser/src/pt/up/fe/specs/clang/dumper/ClangAstDumper.java index 073675aac..878f1beec 100644 --- a/ClangAstParser/src/pt/up/fe/specs/clang/dumper/ClangAstDumper.java +++ b/ClangAstParser/src/pt/up/fe/specs/clang/dumper/ClangAstDumper.java @@ -341,8 +341,6 @@ else if (SourceType.isHeader(sourceFile)) { arguments.add("-dependency-dot"); arguments.add("-Xclang"); arguments.add(DEPENDENCY_DOT_FILENAME); - arguments.add("-Xclang"); - arguments.add("-sys-header-deps"); return arguments; } @@ -375,7 +373,9 @@ private ParsedDump runDumper(List arguments, File sourceFile, String id, () -> "Did not expect error output to be null"); parsedDump.data().set(ClangAstData.HAS_ERRORS, output.isError()); DependencyDot dependencyDot = readDependencyDot(lastWorkingFolder); - parsedDump = parsedDump.withDependencies(dependencyDot.paths(), dependencyDot.available()); + boolean dependenciesAvailable = dependencyDot.available() + && !containsIncludeProbe(sourceFile.toPath(), dependencyDot.paths()); + parsedDump = parsedDump.withDependencies(dependencyDot.paths(), dependenciesAvailable); // If console output streaming is disabled, show output only at the end if (!streamConsoleOutput) { @@ -618,6 +618,29 @@ private Optional resolveDependencyPath(String dependency, File workingFold return Optional.empty(); } + /** + * Clang's dependency graph does not record files probed by __has_include when the probe is negative. Such a dump + * is therefore unsafe to publish: creating the previously absent header could otherwise leave a stale hit. Keep + * this conservative and lexical; comments and strings are acceptable false positives here. + */ + private boolean containsIncludeProbe(Path source, Collection dependencies) { + Set paths = new HashSet<>(dependencies); + paths.add(source); + + for (Path path : paths) { + try (var lines = java.nio.file.Files.lines(path)) { + if (lines.anyMatch(line -> line.contains("__has_include") || line.contains("__has_include_next"))) { + return true; + } + } catch (IOException e) { + // A dependency that cannot be inspected cannot safely participate in publication. + return true; + } + } + + return false; + } + /** Copies stderr bytes as they are consumed, without buffering the complete dumper output. */ private static final class TeeInputStream extends InputStream { private final InputStream delegate; diff --git a/ClangAstParser/test/pt/up/fe/specs/clang/AstDumpCacheTest.java b/ClangAstParser/test/pt/up/fe/specs/clang/AstDumpCacheTest.java index 453c51fd6..ec3aac517 100644 --- a/ClangAstParser/test/pt/up/fe/specs/clang/AstDumpCacheTest.java +++ b/ClangAstParser/test/pt/up/fe/specs/clang/AstDumpCacheTest.java @@ -141,6 +141,57 @@ public void failedProducerIsNeverPublished() throws IOException { assertTrue(cache.load(this::readUtf8).isEmpty()); } + @Test + public void timeMacroInSourceReturnsResultWithoutPublishing() throws IOException { + assertVolatileSourceDoesNotPublish("__TIME__"); + } + + @Test + public void dateMacroInSourceReturnsResultWithoutPublishing() throws IOException { + assertVolatileSourceDoesNotPublish("__DATE__"); + } + + @Test + public void timestampMacroInSourceReturnsResultWithoutPublishing() throws IOException { + assertVolatileSourceDoesNotPublish("__TIMESTAMP__"); + } + + @Test + public void volatileMacroInHeaderReturnsResultWithoutPublishing() throws IOException { + Path source = source("source.cpp", "#include \"header.h\"\n"); + Path header = source("header.h", "const char *build_date = __DATE__;\n"); + AstDumpCache cache = cache(source, "clang", source.toString()); + + String result = cache.capture(output -> { + output.write("header dump".getBytes(StandardCharsets.UTF_8)); + return "parsed"; + }, ignored -> List.of(source, header), ignored -> true); + + assertEquals("parsed", result); + assertEquals(List.of(), entryDirectories()); + } + + @Test + public void binaryDependencyWithMacroSpellingIsStillPublished() throws IOException { + Path source = source("source.cpp", "int value;\n"); + Path executable = tempFolder.resolve("clang"); + byte[] macro = "__TIME__".getBytes(StandardCharsets.US_ASCII); + byte[] binary = new byte[macro.length + 1]; + System.arraycopy(macro, 0, binary, 1, macro.length); + binary[0] = 0; + Files.write(executable, binary); + AstDumpCache cache = cache(source, "clang", source.toString()); + + String result = cache.capture(output -> { + output.write("binary dependency dump".getBytes(StandardCharsets.UTF_8)); + return "parsed"; + }, ignored -> List.of(source, executable), ignored -> true); + + assertEquals("parsed", result); + assertEquals(1, entryDirectories().size()); + assertEquals("binary dependency dump", cache.load(this::readUtf8).orElseThrow()); + } + @Test public void concurrentWritersPublishOneCompleteEntry() throws Exception { Path source = source("source.cpp", "int value;\n"); @@ -209,6 +260,19 @@ private void capture(AstDumpCache cache, String dump, Collection dependenc }, ignored -> dependencies, ignored -> true); } + private void assertVolatileSourceDoesNotPublish(String macro) throws IOException { + Path source = source("source.cpp", "const char *build = " + macro + ";\n"); + AstDumpCache cache = cache(source, "clang", source.toString()); + + String result = cache.capture(output -> { + output.write("volatile dump".getBytes(StandardCharsets.UTF_8)); + return "parsed"; + }, ignored -> List.of(source), ignored -> true); + + assertEquals("parsed", result); + assertEquals(List.of(), entryDirectories()); + } + private AstDumpCache cache(Path source, String... command) { return new AstDumpCache(tempFolder.resolve("cache"), source, List.of(command)); } diff --git a/ClangAstParser/test/pt/up/fe/specs/clang/dumper/ClangAstDumperCacheIntegrationTest.java b/ClangAstParser/test/pt/up/fe/specs/clang/dumper/ClangAstDumperCacheIntegrationTest.java index 7755870d6..0568156af 100644 --- a/ClangAstParser/test/pt/up/fe/specs/clang/dumper/ClangAstDumperCacheIntegrationTest.java +++ b/ClangAstParser/test/pt/up/fe/specs/clang/dumper/ClangAstDumperCacheIntegrationTest.java @@ -114,6 +114,40 @@ void failedDumperOutputIsNotReused() throws IOException { assertNotNull(secondDumper.getLastWorkingFolder(), "failed output must not be loaded from the cache"); } + @Test + void includeProbeResultsAreNeverCachedAcrossHeaderCreation() throws IOException { + SpecsSystem.programStandardInit(); + + File source = Files.writeString(tempFolder.resolve("probe.cpp"), """ + #if __has_include("optional.h") + #include "optional.h" + int selected = OPTIONAL_VALUE; + #else + int selected = 1; + #endif + """).toFile(); + CodeParser parserConfig = parserConfig(); + ClangFiles clangFiles = new ClangResources(parserConfig).getClangFiles(LibcMode.BUILTIN_AND_LIBC); + File workingFolder = tempFolder.resolve("working").toFile(); + + ClangAstDumper firstDumper = newDumper(parserConfig, clangFiles, workingFolder); + ClangAstData first = firstDumper.parse(source, "1", Standard.CXX17, config()); + assertNotNull(firstDumper.getLastWorkingFolder()); + assertEquals("1", first.get(ClangAstData.TRANSLATION_UNIT).getDescendants(IntegerLiteral.class) + .stream().findFirst().orElseThrow().getCode()); + + ClangAstDumper secondDumper = newDumper(parserConfig, clangFiles, workingFolder); + ClangAstData second = secondDumper.parse(source, "1", Standard.CXX17, config()); + assertNotNull(secondDumper.getLastWorkingFolder(), "a negative include probe must not be cached"); + + Files.writeString(tempFolder.resolve("optional.h"), "#define OPTIONAL_VALUE 2\n"); + ClangAstDumper thirdDumper = newDumper(parserConfig, clangFiles, workingFolder); + ClangAstData third = thirdDumper.parse(source, "1", Standard.CXX17, config()); + assertNotNull(thirdDumper.getLastWorkingFolder(), "probe-sensitive input must remain a cache miss"); + assertEquals("2", third.get(ClangAstData.TRANSLATION_UNIT).getDescendants(IntegerLiteral.class) + .stream().findFirst().orElseThrow().getCode()); + } + private CodeParser parserConfig() { CodeParser parserConfig = CodeParser.newInstance(); parserConfig.set(CodeParser.DUMPER_FOLDER, tempFolder.resolve("cache").toFile()); From 920f7d70ea3f1650c019120ec57b756aec1ba1e0 Mon Sep 17 00:00:00 2001 From: "L. Sousa" Date: Wed, 26 Aug 2026 00:13:43 +0100 Subject: [PATCH 4/7] perf(clang): buffer volatile macro dependency scans The volatile-macro scan reads every transitive dependency through an unbuffered stream, costing one syscall per byte on each cache publication over include graphs that can reach tens of megabytes. Wrap the stream so reads happen in 8 KiB chunks; the sliding-window match is unchanged. --- ClangAstParser/src/pt/up/fe/specs/clang/AstDumpCache.java | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/ClangAstParser/src/pt/up/fe/specs/clang/AstDumpCache.java b/ClangAstParser/src/pt/up/fe/specs/clang/AstDumpCache.java index 90b1c9876..4e2630ddf 100644 --- a/ClangAstParser/src/pt/up/fe/specs/clang/AstDumpCache.java +++ b/ClangAstParser/src/pt/up/fe/specs/clang/AstDumpCache.java @@ -16,6 +16,7 @@ import com.google.gson.Gson; import pt.up.fe.specs.util.SpecsLogs; +import java.io.BufferedInputStream; import java.io.File; import java.io.IOException; import java.io.InputStream; @@ -564,7 +565,7 @@ private static boolean containsVolatileMacro(Path path) { int windowSize = 0; boolean macroFound = false; boolean binaryFound = false; - try (InputStream input = Files.newInputStream(path)) { + try (InputStream input = new BufferedInputStream(Files.newInputStream(path))) { int next; while ((next = input.read()) != -1) { if (next == 0) { From c46ad9c8891cb36d508b3ed616932378bc11089c Mon Sep 17 00:00:00 2001 From: "L. Sousa" Date: Wed, 26 Aug 2026 00:14:02 +0100 Subject: [PATCH 5/7] fix(clang): scan include probes as raw bytes containsIncludeProbe decoded dependencies as UTF-8 through Files.lines, which throws an unchecked MalformedInputException for legacy encodings such as ISO-8859-1. That turned previously parseable translation units into ignored files whenever the source or any transitive header contained such bytes. Scan raw bytes with a sliding window instead, keeping the fail-closed policy for files that cannot be read at all. '__has_include_next' carries '__has_include' as a prefix, so a single needle covers both forms. --- .../fe/specs/clang/dumper/ClangAstDumper.java | 57 +++++++++++++++++-- .../ClangAstDumperCacheIntegrationTest.java | 27 +++++++++ 2 files changed, 78 insertions(+), 6 deletions(-) diff --git a/ClangAstParser/src/pt/up/fe/specs/clang/dumper/ClangAstDumper.java b/ClangAstParser/src/pt/up/fe/specs/clang/dumper/ClangAstDumper.java index 878f1beec..561a03394 100644 --- a/ClangAstParser/src/pt/up/fe/specs/clang/dumper/ClangAstDumper.java +++ b/ClangAstParser/src/pt/up/fe/specs/clang/dumper/ClangAstDumper.java @@ -39,10 +39,13 @@ import pt.up.fe.specs.util.system.ProcessOutput; import pt.up.fe.specs.util.utilities.LineStream; +import java.io.BufferedInputStream; import java.io.File; import java.io.IOException; import java.io.InputStream; import java.io.OutputStream; +import java.nio.charset.StandardCharsets; +import java.nio.file.Files; import java.nio.file.InvalidPathException; import java.nio.file.Path; import java.util.ArrayList; @@ -622,18 +625,19 @@ private Optional resolveDependencyPath(String dependency, File workingFold * Clang's dependency graph does not record files probed by __has_include when the probe is negative. Such a dump * is therefore unsafe to publish: creating the previously absent header could otherwise leave a stale hit. Keep * this conservative and lexical; comments and strings are acceptable false positives here. + * + *

Scans raw bytes instead of decoded text: sources and headers may legitimately use encodings like ISO-8859-1, + * which must not turn a parseable translation unit into a cache or parse failure. "__has_include_next" contains + * "__has_include" as a prefix, so a single needle covers both forms. */ private boolean containsIncludeProbe(Path source, Collection dependencies) { + byte[] needle = "__has_include".getBytes(StandardCharsets.US_ASCII); + Set paths = new HashSet<>(dependencies); paths.add(source); for (Path path : paths) { - try (var lines = java.nio.file.Files.lines(path)) { - if (lines.anyMatch(line -> line.contains("__has_include") || line.contains("__has_include_next"))) { - return true; - } - } catch (IOException e) { - // A dependency that cannot be inspected cannot safely participate in publication. + if (containsAsciiSequence(path, needle)) { return true; } } @@ -641,6 +645,47 @@ private boolean containsIncludeProbe(Path source, Collection dependencies) return false; } + /** + * Returns true if the file contains the given ASCII sequence, using a sliding window so that encodings with bytes + * outside US-ASCII are handled as ordinary content. A file that cannot be read is reported as containing the + * sequence, since it cannot be proven safe for publication. + */ + private static boolean containsAsciiSequence(Path path, byte[] sequence) { + byte[] window = new byte[sequence.length]; + int windowSize = 0; + try (InputStream input = new BufferedInputStream(Files.newInputStream(path))) { + int next; + while ((next = input.read()) != -1) { + if (windowSize < window.length) { + window[windowSize++] = (byte) next; + } else { + System.arraycopy(window, 1, window, 0, window.length - 1); + window[window.length - 1] = (byte) next; + } + + if (windowSize < sequence.length) { + continue; + } + + boolean matches = true; + for (int index = 0; index < sequence.length; index++) { + if (window[index] != sequence[index]) { + matches = false; + break; + } + } + + if (matches) { + return true; + } + } + + return false; + } catch (IOException e) { + return true; + } + } + /** Copies stderr bytes as they are consumed, without buffering the complete dumper output. */ private static final class TeeInputStream extends InputStream { private final InputStream delegate; diff --git a/ClangAstParser/test/pt/up/fe/specs/clang/dumper/ClangAstDumperCacheIntegrationTest.java b/ClangAstParser/test/pt/up/fe/specs/clang/dumper/ClangAstDumperCacheIntegrationTest.java index 0568156af..77bd65ce2 100644 --- a/ClangAstParser/test/pt/up/fe/specs/clang/dumper/ClangAstDumperCacheIntegrationTest.java +++ b/ClangAstParser/test/pt/up/fe/specs/clang/dumper/ClangAstDumperCacheIntegrationTest.java @@ -20,6 +20,7 @@ import java.io.File; import java.io.IOException; +import java.nio.charset.Charset; import java.nio.file.Files; import java.nio.file.Path; import java.util.List; @@ -148,6 +149,32 @@ void includeProbeResultsAreNeverCachedAcrossHeaderCreation() throws IOException .stream().findFirst().orElseThrow().getCode()); } + @Test + void nonUtf8DependenciesAreScannedAsBytesAndStillCached() throws IOException { + SpecsSystem.programStandardInit(); + + // ISO-8859-1 comment bytes are valid C content but not valid UTF-8. + File header = tempFolder.resolve("latin1.h").toFile(); + Files.write(header.toPath(), + "/* coment\341rio com acentua\347\343o */\n#define VALUE 1\n".getBytes(Charset.forName("ISO-8859-1"))); + File source = Files.writeString(tempFolder.resolve("source_latin.cpp"), + "#include \"latin1.h\"\nint value = VALUE;\n").toFile(); + CodeParser parserConfig = parserConfig(); + ClangFiles clangFiles = new ClangResources(parserConfig).getClangFiles(LibcMode.BUILTIN_AND_LIBC); + File workingFolder = tempFolder.resolve("working").toFile(); + + ClangAstDumper missDumper = newDumper(parserConfig, clangFiles, workingFolder); + ClangAstData miss = missDumper.parse(source, "1", Standard.CXX17, config()); + assertNotNull(missDumper.getLastWorkingFolder(), + "a translation unit with non-UTF-8 dependencies must still be published"); + assertEquals("1", miss.get(ClangAstData.TRANSLATION_UNIT).getDescendants(IntegerLiteral.class) + .stream().findFirst().orElseThrow().getCode()); + + ClangAstDumper hitDumper = newDumper(parserConfig, clangFiles, workingFolder); + hitDumper.parse(source, "1", Standard.CXX17, config()); + assertNull(hitDumper.getLastWorkingFolder(), "a non-UTF-8 dependency must not prevent a cache hit"); + } + private CodeParser parserConfig() { CodeParser parserConfig = CodeParser.newInstance(); parserConfig.set(CodeParser.DUMPER_FOLDER, tempFolder.resolve("cache").toFile()); From 141813dc3cfbb3bf1eedf376153c6f685c4b29cd Mon Sep 17 00:00:00 2001 From: "L. Sousa" Date: Wed, 26 Aug 2026 00:14:19 +0100 Subject: [PATCH 6/7] perf(clang): derive dumper ids from source paths ParallelCodeParser numbered ids positionally, so adding or removing a source file shifted the '-id' value of every later file. The id is part of the dump bytes and therefore of the cache key, so one file-set change invalidated every subsequent translation unit's entry. Derive a stable per-file id from the source path instead. The dumper parses '-id' as an int and embeds it in each node id, but ids are only interpreted inside one dump, and each file is dumped by its own process, so two files receiving the same id are harmless. --- .../clang/codeparser/ParallelCodeParser.java | 2 +- .../fe/specs/clang/dumper/ClangAstDumper.java | 16 ++++++ .../clang/dumper/ClangAstDumperIdTest.java | 57 +++++++++++++++++++ 3 files changed, 74 insertions(+), 1 deletion(-) create mode 100644 ClangAstParser/test/pt/up/fe/specs/clang/dumper/ClangAstDumperIdTest.java diff --git a/ClangAstParser/src/pt/up/fe/specs/clang/codeparser/ParallelCodeParser.java b/ClangAstParser/src/pt/up/fe/specs/clang/codeparser/ParallelCodeParser.java index a1c9da3e3..0f79aa31c 100644 --- a/ClangAstParser/src/pt/up/fe/specs/clang/codeparser/ParallelCodeParser.java +++ b/ClangAstParser/src/pt/up/fe/specs/clang/codeparser/ParallelCodeParser.java @@ -134,8 +134,8 @@ public App parse(List inputSources, List compilerOptions, ClavaCon List> futureTUnits = new ArrayList<>(); for (int i = 0; i < sources.size(); i++) { - String id = Integer.toString(i + 1); File source = sources.get(i); + String id = ClangAstDumper.getStableFileId(source); Future tUnit = executor .submit(() -> parseSource(source, id, standard, options, clangDump, diff --git a/ClangAstParser/src/pt/up/fe/specs/clang/dumper/ClangAstDumper.java b/ClangAstParser/src/pt/up/fe/specs/clang/dumper/ClangAstDumper.java index 561a03394..5a5bf9393 100644 --- a/ClangAstParser/src/pt/up/fe/specs/clang/dumper/ClangAstDumper.java +++ b/ClangAstParser/src/pt/up/fe/specs/clang/dumper/ClangAstDumper.java @@ -86,6 +86,22 @@ public static List getTempFiles() { return CLANG_AST_DUMPER_TEMP_FILES; } + /** + * Returns a per-file id that is stable across executions, to be passed as the '-id' dumper option. + * + *

The dumper parses '-id' as an int and embeds it in every node id of the dump, so the id must not depend on + * the number or order of the parsed files: a positional id would change the dumped bytes whenever a file is added + * or removed, invalidating the cache entry of every subsequent file. Ids are only interpreted inside a single dump, + * and each file is dumped by its own process, so two files receiving the same id are harmless. + * + * @param sourceFile source file that will be dumped + * @return a non-negative int-compatible id, stable for the same path spelling across JVMs + */ + public static String getStableFileId(File sourceFile) { + // String.hashCode is specified, so the value does not change between runs or JVM implementations. + return Integer.toString(sourceFile.getAbsolutePath().hashCode() & 0x7fffffff); + } + /** * TODO: Not implemented yet *

diff --git a/ClangAstParser/test/pt/up/fe/specs/clang/dumper/ClangAstDumperIdTest.java b/ClangAstParser/test/pt/up/fe/specs/clang/dumper/ClangAstDumperIdTest.java new file mode 100644 index 000000000..ac3b2c2d1 --- /dev/null +++ b/ClangAstParser/test/pt/up/fe/specs/clang/dumper/ClangAstDumperIdTest.java @@ -0,0 +1,57 @@ +/** + * Copyright 2026 SPeCS. + *

+ * 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. + */ + +package pt.up.fe.specs.clang.dumper; + +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertNotEquals; +import static org.junit.jupiter.api.Assertions.assertTrue; + +import java.io.File; +import java.util.HashSet; +import java.util.Set; + +import org.junit.jupiter.api.Test; + + +public class ClangAstDumperIdTest { + + @Test + public void idIsStableForTheSamePath() { + File source = new File("/tmp/project/source.cpp"); + + assertEquals(ClangAstDumper.getStableFileId(source), ClangAstDumper.getStableFileId(source)); + assertEquals(ClangAstDumper.getStableFileId(source), + ClangAstDumper.getStableFileId(new File("/tmp/project/source.cpp"))); + } + + @Test + public void idIsANonNegativeInt() { + String id = ClangAstDumper.getStableFileId(new File("/tmp/project/source.cpp")); + + int parsed = Integer.parseInt(id); + assertTrue(parsed >= 0, () -> "id must not be negative, got '" + id + "'"); + } + + @Test + public void idsOfDistinctPathsDoNotTriviallyCollide() { + Set ids = new HashSet<>(); + for (int i = 0; i < 1000; i++) { + ids.add(ClangAstDumper.getStableFileId(new File("/tmp/project/file" + i + ".cpp"))); + } + + // Collisions between two files are harmless, but they should still be rare enough that typical projects do + // not see them. + assertTrue(ids.size() > 990, () -> "unexpected number of collisions: " + (1000 - ids.size())); + } +} From 8ed0a98d3dc3ff8a7d3b7b5f10fecd5edbf9ad2c Mon Sep 17 00:00:00 2001 From: "L. Sousa" Date: Sun, 30 Aug 2026 16:52:05 +0100 Subject: [PATCH 7/7] refactor(clang): simplify cache lifecycle --- .../pt/up/fe/specs/clang/AstDumpCache.java | 200 +++++------------- .../src/pt/up/fe/specs/clang/CacheFiles.java | 93 +++++--- .../pt/up/fe/specs/clang/ClangResources.java | 90 +++----- .../pt/up/fe/specs/clang/CudaResources.java | 41 ++-- .../clang/codeparser/ParallelCodeParser.java | 4 + .../up/fe/specs/clang/AstDumpCacheTest.java | 5 +- .../up/fe/specs/clang/ClangResourcesTest.java | 67 +++--- .../up/fe/specs/clang/CudaResourcesTest.java | 8 +- 8 files changed, 207 insertions(+), 301 deletions(-) diff --git a/ClangAstParser/src/pt/up/fe/specs/clang/AstDumpCache.java b/ClangAstParser/src/pt/up/fe/specs/clang/AstDumpCache.java index 4e2630ddf..3325691cf 100644 --- a/ClangAstParser/src/pt/up/fe/specs/clang/AstDumpCache.java +++ b/ClangAstParser/src/pt/up/fe/specs/clang/AstDumpCache.java @@ -114,29 +114,44 @@ public Optional load(InputStreamParser parser) { } Path entry = entriesRoot.resolve(key); - if (!claimValidEntry(entry)) { - return Optional.empty(); - } - - try (InputStream compressed = Files.newInputStream(entry.resolve(DUMP_FILENAME)); - InputStream input = new GZIPInputStream(compressed)) { - T result = parser.parse(input); + try { + return CacheFiles.useDirectory(cacheRoot, entry, claimedEntry -> { + try { + if (!isManifestValid(claimedEntry)) { + return Optional.empty(); + } - // Force the gzip stream to EOF so a truncated stream or invalid trailer cannot be accepted merely because - // a parser stopped after the first record. - input.transferTo(OutputStream.nullOutputStream()); - if (result == null) { - throw new RuntimeException("AST dump parser returned null"); - } + try (InputStream compressed = Files.newInputStream(claimedEntry.resolve(DUMP_FILENAME)); + InputStream input = new GZIPInputStream(compressed)) { + T result = parser.parse(input); - return Optional.of(result); - } catch (Exception e) { - reportCacheFailure("Could not read cached AST dump entry '" + entry + "'", e); - deleteEntryQuietly(entry); + // Force the gzip stream to EOF so a truncated stream or invalid trailer cannot be accepted merely + // because a parser stopped after the first record. + input.transferTo(OutputStream.nullOutputStream()); + return Optional.ofNullable(result); + } + } catch (Exception e) { + reportCacheFailure("Could not read cached AST dump entry '" + claimedEntry + "'", e); + return Optional.empty(); + } + }); + } catch (RuntimeException e) { + reportCacheFailure("Could not use cached AST dump entry '" + entry + "'", e); return Optional.empty(); } } + /** Performs opportunistic cleanup once before a group of translation units is parsed. */ + public static void cleanup(Path cacheRoot) { + Path normalizedRoot = cacheRoot.toAbsolutePath().normalize(); + Path entries = normalizedRoot.resolve("ast-dumps").resolve(FORMAT_VERSION).resolve("entries"); + try { + CacheFiles.cleanupDirectories(normalizedRoot, entries, Instant.now().minus(STALE_ENTRY_AGE), null); + } catch (RuntimeException e) { + reportCacheFailure("Could not clean AST dump cache", e); + } + } + /** * Runs a producer and publishes its exact gzip output only when the caller accepts the result. * @@ -161,13 +176,6 @@ public T capture(DumpProducer producer, } Path entry = entriesRoot.resolve(key); - try { - prepareEntriesDirectory(entry); - } catch (RuntimeException e) { - reportCacheFailure("Could not prepare AST dump cache", e); - return runWithoutCache(producer); - } - CacheFiles.StagingDirectory staging; try { staging = CacheFiles.createStagingDirectory(cacheRoot, entriesRoot, "." + key + ".tmp-"); @@ -176,10 +184,10 @@ public T capture(DumpProducer producer, return runWithoutCache(producer); } - T result = null; - boolean producerStarted = false; - boolean outputUsable = true; - try { + try (staging) { + T result = null; + boolean producerStarted = false; + boolean outputUsable; Path stagedDump = staging.path().resolve(DUMP_FILENAME); try (OutputStream fileOutput = Files.newOutputStream(stagedDump, StandardOpenOption.CREATE_NEW, StandardOpenOption.WRITE)) { @@ -191,46 +199,29 @@ public T capture(DumpProducer producer, outputUsable = !bestEffortOutput.failed(); } catch (IOException e) { // A failed cache stream must not turn a successful dumper run into a parse failure. The producer has - // not necessarily run if opening the cache stream itself failed, so fall back to the producer with a - // null sink in that case. - outputUsable = false; + // not necessarily run if opening the cache stream itself failed, so fall back in that case. reportCacheFailure("Could not write cached AST dump", e); - discardStagingQuietly(staging); return producerStarted ? result : runWithoutCache(producer); } - } catch (RuntimeException e) { - discardStagingQuietly(staging); - throw e; - } - if (!outputUsable) { - discardStagingQuietly(staging); - return result; - } + if (!outputUsable || !shouldPublish(result, publishDecision)) { + return result; + } - if (!shouldPublish(result, publishDecision)) { - discardStagingQuietly(staging); - return result; - } + try { + Manifest manifest = buildManifest(dependencyPaths.apply(result)); + Path stagedManifest = staging.path().resolve(MANIFEST_FILENAME); + Files.writeString(stagedManifest, GSON.toJson(manifest), StandardCharsets.UTF_8, + StandardOpenOption.CREATE_NEW, StandardOpenOption.WRITE); + + // The directory move is the publication point; readers cannot observe a partial entry. + CacheFiles.publish(staging.path(), entry); + } catch (RuntimeException | IOException e) { + reportCacheFailure("Could not publish AST dump cache entry '" + entry + "'", e); + } - try { - Manifest manifest = buildManifest(dependencyPaths.apply(result)); - Path stagedManifest = staging.path().resolve(MANIFEST_FILENAME); - Files.writeString(stagedManifest, GSON.toJson(manifest), StandardCharsets.UTF_8, - StandardOpenOption.CREATE_NEW, StandardOpenOption.WRITE); - - // The directory move is the publication point; readers cannot observe only one of dump.gz/manifest.json. - CacheFiles.publish(staging.path(), entry); - } catch (RuntimeException | IOException e) { - reportCacheFailure("Could not publish AST dump cache entry '" + entry + "'", e); - } finally { - // publish() can report an existing destination without moving this writer's staging directory. Always - // remove it before releasing the lock, including the losing-writer case. - CacheFiles.deleteQuietly(staging.path()); - closeStagingQuietly(staging); + return result; } - - return result; } /** Parses a cached gzip stream. */ @@ -377,72 +368,6 @@ private static void putLong(MessageDigest digest, long value) { } } - private boolean claimValidEntry(Path entry) { - boolean claimed; - try { - claimed = CacheFiles.withMaintenanceLock(cacheRoot, () -> { - try { - Files.createDirectories(entriesRoot); - } catch (IOException e) { - throw new UncheckedIOException("Could not create AST dump cache entries directory", e); - } - - // Claim the entry before cleanup. A concurrent cleanup therefore cannot remove an entry that this - // reader is about to validate and use. - if (Files.isDirectory(entry)) { - CacheFiles.touch(entry); - } - - CacheFiles.deleteStaleDirectoriesLocked(entriesRoot, Instant.now().minus(STALE_ENTRY_AGE), entry); - CacheFiles.deleteUnlockedStagingLocksLocked(entriesRoot); - - if (!Files.isDirectory(entry)) { - return false; - } - return true; - }); - } catch (RuntimeException e) { - reportCacheFailure("Could not validate AST dump cache entry '" + entry + "'", e); - return false; - } - - if (!claimed) { - return false; - } - - // Hashing dependency files can be expensive. The entry was touched and excluded from cleanup above, so the - // full manifest validation can safely run without holding the process-wide maintenance lock. - boolean valid; - try { - valid = isManifestValid(entry); - } catch (RuntimeException e) { - reportCacheFailure("Could not validate AST dump cache entry '" + entry + "'", e); - valid = false; - } - - if (!valid) { - // Reacquire the maintenance lock before deleting. This keeps invalid-entry removal ordered with cleanup - // and with another reader claiming the same entry. - deleteEntryQuietly(entry); - return false; - } - - return true; - } - - private void prepareEntriesDirectory(Path currentEntry) { - CacheFiles.withMaintenanceLock(cacheRoot, () -> { - try { - Files.createDirectories(entriesRoot); - } catch (IOException e) { - throw new UncheckedIOException("Could not create AST dump cache entries directory", e); - } - - CacheFiles.deleteStaleDirectoriesLocked(entriesRoot, Instant.now().minus(STALE_ENTRY_AGE), currentEntry); - CacheFiles.deleteUnlockedStagingLocksLocked(entriesRoot); - }); - } - private boolean isManifestValid(Path entry) { Path manifestPath = entry.resolve(MANIFEST_FILENAME); Path dumpPath = entry.resolve(DUMP_FILENAME); @@ -628,27 +553,6 @@ private static T runProducer(DumpProducer producer, OutputStream output) } } - private static void closeStagingQuietly(CacheFiles.StagingDirectory staging) { - try { - staging.close(); - } catch (RuntimeException e) { - reportCacheFailure("Could not close AST dump cache staging lock", e); - } - } - - private static void discardStagingQuietly(CacheFiles.StagingDirectory staging) { - CacheFiles.deleteQuietly(staging.path()); - closeStagingQuietly(staging); - } - - private void deleteEntryQuietly(Path entry) { - try { - CacheFiles.withMaintenanceLock(cacheRoot, () -> CacheFiles.deleteQuietly(entry)); - } catch (RuntimeException e) { - reportCacheFailure("Could not remove invalid AST dump cache entry '" + entry + "'", e); - } - } - private static void reportCacheFailure(String message, Throwable cause) { SpecsLogs.debug(() -> message + ": " + cause.getMessage()); } diff --git a/ClangAstParser/src/pt/up/fe/specs/clang/CacheFiles.java b/ClangAstParser/src/pt/up/fe/specs/clang/CacheFiles.java index 0bd60e4ce..159c3580d 100644 --- a/ClangAstParser/src/pt/up/fe/specs/clang/CacheFiles.java +++ b/ClangAstParser/src/pt/up/fe/specs/clang/CacheFiles.java @@ -38,6 +38,8 @@ import java.security.NoSuchAlgorithmException; import java.time.Instant; import java.util.HexFormat; +import java.util.Optional; +import java.util.function.Function; import java.util.function.Supplier; final class CacheFiles { @@ -49,7 +51,7 @@ final class CacheFiles { private CacheFiles() { } - static T withMaintenanceLock(Path cacheRoot, Supplier action) { + private static T withMaintenanceLock(Path cacheRoot, Supplier action) { var lockPath = cacheRoot.resolve(MAINTENANCE_LOCK_FILENAME); synchronized (MAINTENANCE_MONITOR) { try { @@ -64,13 +66,53 @@ static T withMaintenanceLock(Path cacheRoot, Supplier action) { } } - static void withMaintenanceLock(Path cacheRoot, Runnable action) { + private static void withMaintenanceLock(Path cacheRoot, Runnable action) { withMaintenanceLock(cacheRoot, () -> { action.run(); return null; }); } + /** + * Claims a published directory, refreshes its use time, and uses it outside the maintenance lock. + * + *

An empty result means the directory was invalid and removes it before returning. Exceptions leave the + * directory untouched so callers can report malformed published resources instead of silently repairing them.

+ */ + static Optional useDirectory(Path cacheRoot, Path directory, + Function> use) { + boolean claimed = withMaintenanceLock(cacheRoot, () -> { + if (!Files.isDirectory(directory)) { + return false; + } + + touchLocked(directory); + return true; + }); + + if (!claimed) { + return Optional.empty(); + } + + var result = use.apply(directory); + if (result.isEmpty()) { + withMaintenanceLock(cacheRoot, () -> deleteQuietly(directory)); + } + + return result; + } + + /** Refreshes existing cache paths as one maintenance operation. */ + static void touch(Path cacheRoot, Path... paths) { + withMaintenanceLock(cacheRoot, () -> { + for (var path : paths) { + if (Files.exists(path)) { + touchLocked(path); + } + } + }); + } + static StagingDirectory createStagingDirectory(Path cacheRoot, Path parent, String prefix) { return withMaintenanceLock(cacheRoot, () -> createStagingDirectoryLocked(parent, prefix)); } @@ -138,16 +180,13 @@ record StagingDirectory(Path path, Path lockPath, FileChannel channel) implement @Override public void close() { - try { - channel.close(); - } catch (IOException e) { - throw new UncheckedIOException("Could not close cache staging lock '" + lockPath + "'", e); - } + deleteQuietly(path); try { + channel.close(); Files.deleteIfExists(lockPath); - } catch (IOException e) { - throw new UncheckedIOException("Could not close cache staging lock '" + lockPath + "'", e); + } catch (IOException ignored) { + // Staging cleanup is best-effort. A remaining lock lets a later cache cleanup safely retry. } } } @@ -163,9 +202,8 @@ static File installFile(Path cacheRoot, File destination, FileResourceProvider r return destination; } - var stagingDirectory = createStagingDirectory(cacheRoot, destination.getParentFile().toPath(), - "." + destination.getName() + ".tmp-"); - try { + try (var stagingDirectory = createStagingDirectory(cacheRoot, destination.getParentFile().toPath(), + "." + destination.getName() + ".tmp-")) { File stagedFile = resource.write(stagingDirectory.path().toFile()); if (stagedFile == null || !stagedFile.isFile()) { throw new RuntimeException("Could not download " + description); @@ -182,12 +220,6 @@ static File installFile(Path cacheRoot, File destination, FileResourceProvider r } return publish(stagedFile.toPath(), destination.toPath()).toFile(); - } finally { - try { - deleteQuietly(stagingDirectory.path()); - } finally { - stagingDirectory.close(); - } } } @@ -232,7 +264,7 @@ static boolean hasExpectedSha256(File file, String expectedSha256) { return expectedSha256.equalsIgnoreCase(calculateSha256(file)); } - static void touch(Path path) { + private static void touchLocked(Path path) { try { Files.setLastModifiedTime(path, FileTime.from(Instant.now())); } catch (IOException e) { @@ -240,11 +272,20 @@ static void touch(Path path) { } } - static void deleteStaleDirectories(Path cacheRoot, Path parent, Instant cutoff, Path excluded) { - withMaintenanceLock(cacheRoot, () -> deleteStaleDirectoriesLocked(parent, cutoff, excluded)); + /** Removes stale published directories and abandoned staging directories in one locked pass. */ + static void cleanupDirectories(Path cacheRoot, Path parent, Instant cutoff, Path excluded) { + withMaintenanceLock(cacheRoot, () -> { + deleteStaleDirectories(parent, cutoff, excluded); + deleteUnlockedStagingDirectories(parent); + }); + } + + /** Removes abandoned staging directories without treating other child directories as cache entries. */ + static void cleanupStagingDirectories(Path cacheRoot, Path parent) { + withMaintenanceLock(cacheRoot, () -> deleteUnlockedStagingDirectories(parent)); } - static void deleteStaleDirectoriesLocked(Path parent, Instant cutoff, Path excluded) { + private static void deleteStaleDirectories(Path parent, Instant cutoff, Path excluded) { if (!Files.isDirectory(parent)) { return; } @@ -277,11 +318,7 @@ private static void deleteIfStale(Path path, Instant cutoff) { } } - static void deleteUnlockedStagingLocks(Path cacheRoot, Path parent) { - withMaintenanceLock(cacheRoot, () -> deleteUnlockedStagingLocksLocked(parent)); - } - - static void deleteUnlockedStagingLocksLocked(Path parent) { + private static void deleteUnlockedStagingDirectories(Path parent) { if (!Files.isDirectory(parent)) { return; } @@ -335,7 +372,7 @@ static void delete(Path path) { } } - static void deleteQuietly(Path path) { + private static void deleteQuietly(Path path) { try { delete(path); } catch (RuntimeException ignored) { diff --git a/ClangAstParser/src/pt/up/fe/specs/clang/ClangResources.java b/ClangAstParser/src/pt/up/fe/specs/clang/ClangResources.java index 041f66e56..6fd38e2ac 100644 --- a/ClangAstParser/src/pt/up/fe/specs/clang/ClangResources.java +++ b/ClangAstParser/src/pt/up/fe/specs/clang/ClangResources.java @@ -37,6 +37,7 @@ import java.util.Locale; import java.util.Map; import java.util.Objects; +import java.util.Optional; import java.util.UUID; import java.util.concurrent.ConcurrentHashMap; @@ -138,41 +139,30 @@ private boolean isUsable(CachedClangFiles cached) { return false; } - return CacheFiles.withMaintenanceLock(getClangCacheRoot().toPath(), () -> { - if (!cached.files().clangExecutable().isFile()) { - return false; - } - - if (cached.files().systemResourceDir() != null - && !cached.files().systemResourceDir().isDirectory()) { - return false; - } - - var includesFolder = cached.includesFolder(); - if (includesFolder == null) { - return true; - } - - if (!includesFolder.exists()) { - return false; - } + if (!cached.files().clangExecutable().isFile()) { + return false; + } - CacheFiles.touch(includesFolder.toPath()); - if (!isIncludesCacheValid(includesFolder)) { - throw invalidIncludesCache(includesFolder, includesFolder.getName()); - } + if (cached.files().systemResourceDir() != null + && !cached.files().systemResourceDir().isDirectory()) { + return false; + } + var includesFolder = cached.includesFolder(); + if (includesFolder == null) { return true; - }); + } + + return useExistingIncludes(getClangCacheRoot(), includesFolder, includesFolder.getName()) != null; } private void touchUse(File resourceFolder, File includesFolder) { - CacheFiles.withMaintenanceLock(getClangCacheRoot().toPath(), () -> { - CacheFiles.touch(resourceFolder.toPath()); - if (includesFolder != null) { - CacheFiles.touch(includesFolder.toPath()); - } - }); + if (includesFolder == null) { + CacheFiles.touch(getClangCacheRoot().toPath(), resourceFolder.toPath()); + return; + } + + CacheFiles.touch(getClangCacheRoot().toPath(), resourceFolder.toPath(), includesFolder.toPath()); } static File getLocalExecutable(File buildFolder) { @@ -239,11 +229,9 @@ private void unblockWindowsFile(File executable) { public File getClangResourceFolder() { var cacheFolder = getClangCacheRoot(); - return CacheFiles.withMaintenanceLock(cacheFolder.toPath(), () -> { - var releaseFolder = SpecsIo.mkdir(getReleasesFolder(), ClangAstWebResource.getReleaseTag()); - CacheFiles.touch(releaseFolder.toPath()); - return releaseFolder; - }); + var releaseFolder = SpecsIo.mkdir(getReleasesFolder(), ClangAstWebResource.getReleaseTag()); + CacheFiles.touch(cacheFolder.toPath(), releaseFolder.toPath()); + return releaseFolder; } public static File getDefaultTempFolder() { @@ -400,10 +388,8 @@ static File resolveIncludes(File cacheFolder, ClangDumperManifestAsset includesA } var includesRoot = extractedFolder.getParentFile().toPath(); - CacheFiles.deleteUnlockedStagingLocks(cacheFolder.toPath(), includesRoot); - var stagingFolder = CacheFiles.createStagingDirectory(cacheFolder.toPath(), includesRoot, - "." + includesAsset.sha256() + ".tmp-"); - try { + try (var stagingFolder = CacheFiles.createStagingDirectory(cacheFolder.toPath(), includesRoot, + "." + includesAsset.sha256() + ".tmp-")) { var downloadFolder = CacheFiles.createTemporaryDirectory(stagingFolder.path(), ".download-"); try { var archive = archiveResource.write(downloadFolder.toFile()); @@ -439,32 +425,17 @@ static File resolveIncludes(File cacheFolder, ClangDumperManifestAsset includesA } return existingFolder; - } finally { - try { - CacheFiles.delete(stagingFolder.path()); - } finally { - stagingFolder.close(); - } } } private static File useExistingIncludes(File cacheFolder, File includesFolder, String sha256) { - if (!includesFolder.exists()) { - return null; - } - - return CacheFiles.withMaintenanceLock(cacheFolder.toPath(), () -> { - if (!includesFolder.exists()) { - return null; - } - - CacheFiles.touch(includesFolder.toPath()); + return CacheFiles.useDirectory(cacheFolder.toPath(), includesFolder.toPath(), path -> { if (!isIncludesCacheValid(includesFolder)) { throw invalidIncludesCache(includesFolder, sha256); } - return includesFolder; - }); + return Optional.of(includesFolder); + }).orElse(null); } private static RuntimeException invalidIncludesCache(File includesFolder, String sha256) { @@ -531,12 +502,11 @@ private void deleteStaleVersions(Instant now, File currentVersionFolder, File cu var cutoff = now.minus(STALE_CACHE_MAX_AGE); var cacheRoot = getClangCacheRoot().toPath(); try { - CacheFiles.deleteStaleDirectories(cacheRoot, getReleasesFolder().toPath(), cutoff, + CacheFiles.cleanupDirectories(cacheRoot, getReleasesFolder().toPath(), cutoff, currentVersionFolder.toPath()); - CacheFiles.deleteStaleDirectories(cacheRoot, getIncludesRoot().toPath(), cutoff, + CacheFiles.cleanupDirectories(cacheRoot, getIncludesRoot().toPath(), cutoff, currentIncludesFolder == null ? null : currentIncludesFolder.toPath()); - CacheFiles.deleteUnlockedStagingLocks(cacheRoot, currentVersionFolder.toPath()); - CacheFiles.deleteUnlockedStagingLocks(cacheRoot, getIncludesRoot().toPath()); + CacheFiles.cleanupStagingDirectories(cacheRoot, currentVersionFolder.toPath()); } catch (RuntimeException e) { SpecsLogs.warn("Could not clean stale clang-dumper cache resources", e); } diff --git a/ClangAstParser/src/pt/up/fe/specs/clang/CudaResources.java b/ClangAstParser/src/pt/up/fe/specs/clang/CudaResources.java index 28641b5ab..fd414262b 100644 --- a/ClangAstParser/src/pt/up/fe/specs/clang/CudaResources.java +++ b/ClangAstParser/src/pt/up/fe/specs/clang/CudaResources.java @@ -89,8 +89,9 @@ static File getBuiltinCudaLib(Path cacheRoot) { // A published installation is immutable. A malformed one is an operator error, not an invitation to repair it // in place, because doing so could race with a reader that already selected this release. - if (Files.exists(releaseFolder.toPath(), LinkOption.NOFOLLOW_LINKS)) { - return useExistingInstallation(cacheRoot, releaseFolder, releaseTag); + var existing = useExistingInstallation(cacheRoot, releaseFolder, releaseTag); + if (existing != null) { + return existing; } return install(cacheRoot, releaseTag, getManifestResource(releaseTag), CudaResources::getArchiveResource); @@ -196,16 +197,9 @@ private static NvidiaCudaManifest getCurrentManifest(Path cacheRoot, FileResourc private static NvidiaCudaManifest getCurrentManifest(Path cacheRoot, String releaseTag, FileResourceProvider manifestResource) { var cudaRoot = cacheRoot.resolve(CUDA_FOLDERNAME); - CacheFiles.deleteUnlockedStagingLocks(cacheRoot, cudaRoot); - var stagingDirectory = CacheFiles.createStagingDirectory(cacheRoot, cudaRoot, "." + releaseTag + ".tmp-"); - try { + try (var stagingDirectory = CacheFiles.createStagingDirectory(cacheRoot, cudaRoot, + "." + releaseTag + ".tmp-")) { return downloadManifest(cacheRoot, stagingDirectory.path(), releaseTag, manifestResource); - } finally { - try { - CacheFiles.delete(stagingDirectory.path()); - } finally { - stagingDirectory.close(); - } } } @@ -348,9 +342,8 @@ static File install(Path cacheRoot, String releaseTag, FileResourceProvider mani var cudaRoot = cacheRoot.resolve(CUDA_FOLDERNAME); var releaseFolder = getReleaseFolder(cacheRoot, releaseTag); - CacheFiles.deleteUnlockedStagingLocks(cacheRoot, cudaRoot); - var stagingDirectory = CacheFiles.createStagingDirectory(cacheRoot, cudaRoot, "." + releaseTag + ".tmp-"); - try { + try (var stagingDirectory = CacheFiles.createStagingDirectory(cacheRoot, cudaRoot, + "." + releaseTag + ".tmp-")) { var manifest = downloadManifest(cacheRoot, stagingDirectory.path(), releaseTag, manifestResource); var platform = requireSupportedPlatform(manifest); @@ -371,12 +364,6 @@ static File install(Path cacheRoot, String releaseTag, FileResourceProvider mani CacheFiles.publish(stagingDirectory.path(), releaseFolder.toPath()); return useExistingInstallation(cacheRoot, releaseFolder, releaseTag); - } finally { - try { - CacheFiles.delete(stagingDirectory.path()); - } finally { - stagingDirectory.close(); - } } } @@ -415,15 +402,18 @@ private static String getArchiveName(CudaPackage cudaPackage) { } private static File useExistingInstallation(Path cacheRoot, File releaseFolder, String releaseTag) { - var validInstallation = CacheFiles.withMaintenanceLock(cacheRoot, () -> { + var validInstallation = CacheFiles.useDirectory(cacheRoot, releaseFolder.toPath(), path -> { var platform = requireSupportedPlatform(readPublishedManifest(releaseFolder, releaseTag)); if (!isCudaInstallation(releaseFolder, releaseTag, platform.manifestName())) { throw invalidInstallation(releaseFolder, platform.manifestName()); } - CacheFiles.touch(releaseFolder.toPath()); - return releaseFolder; - }); + return Optional.of(releaseFolder); + }).orElse(null); + + if (validInstallation == null) { + return null; + } cleanup(cacheRoot, releaseFolder.toPath()); SpecsLogs.debug(() -> "Using cached CUDA resources: " + validInstallation); @@ -434,8 +424,7 @@ private static void cleanup(Path cacheRoot, Path releaseFolder) { var cudaRoot = cacheRoot.resolve(CUDA_FOLDERNAME); var cutoff = Instant.now().minus(Duration.ofDays(60)); try { - CacheFiles.deleteStaleDirectories(cacheRoot, cudaRoot, cutoff, releaseFolder); - CacheFiles.deleteUnlockedStagingLocks(cacheRoot, cudaRoot); + CacheFiles.cleanupDirectories(cacheRoot, cudaRoot, cutoff, releaseFolder); } catch (RuntimeException e) { SpecsLogs.warn("Could not clean stale CUDA cache resources", e); } diff --git a/ClangAstParser/src/pt/up/fe/specs/clang/codeparser/ParallelCodeParser.java b/ClangAstParser/src/pt/up/fe/specs/clang/codeparser/ParallelCodeParser.java index 0f79aa31c..c80ec91ac 100644 --- a/ClangAstParser/src/pt/up/fe/specs/clang/codeparser/ParallelCodeParser.java +++ b/ClangAstParser/src/pt/up/fe/specs/clang/codeparser/ParallelCodeParser.java @@ -16,6 +16,7 @@ import org.suikasoft.jOptions.Datakey.DataKey; import org.suikasoft.jOptions.Datakey.KeyFactory; import org.suikasoft.jOptions.Interfaces.DataStore; +import pt.up.fe.specs.clang.AstDumpCache; import pt.up.fe.specs.clang.ClangAstKeys; import pt.up.fe.specs.clang.ClangResources; import pt.up.fe.specs.clang.dumper.ClangAstData; @@ -115,6 +116,9 @@ public App parse(List inputSources, List compilerOptions, ClavaCon // get(ClangAstKeys.USE_PLATFORM_INCLUDES)); ClavaLog.info("Found " + sources.size() + " source files"); + if (!get(SHOW_CLANG_DUMP)) { + AstDumpCache.cleanup(get(DUMPER_FOLDER).toPath()); + } // ClavaLog.debug(() -> "[ParallelCodeParser] Files to parse:" + sources); File parsingFolder = SpecsIo.getTempFolder("clava_parsing_" + UUID.randomUUID().toString()); diff --git a/ClangAstParser/test/pt/up/fe/specs/clang/AstDumpCacheTest.java b/ClangAstParser/test/pt/up/fe/specs/clang/AstDumpCacheTest.java index ec3aac517..c65958329 100644 --- a/ClangAstParser/test/pt/up/fe/specs/clang/AstDumpCacheTest.java +++ b/ClangAstParser/test/pt/up/fe/specs/clang/AstDumpCacheTest.java @@ -223,7 +223,7 @@ public void concurrentWritersPublishOneCompleteEntry() throws Exception { } @Test - public void staleCleanupPreservesCurrentEntryAndRemovesOtherEntry() throws IOException { + public void cleanupRemovesStaleEntriesBeforeAParserRun() throws IOException { Path currentSource = source("current.cpp", "int current;\n"); Path staleSource = source("stale.cpp", "int stale;\n"); AstDumpCache current = cache(currentSource, "clang", currentSource.toString()); @@ -234,9 +234,10 @@ public void staleCleanupPreservesCurrentEntryAndRemovesOtherEntry() throws IOExc Path currentEntry = entryForSource("current"); Path staleEntry = entryForSource("stale"); Instant old = Instant.now().minus(Duration.ofDays(61)); - Files.setLastModifiedTime(currentEntry, FileTime.from(old)); Files.setLastModifiedTime(staleEntry, FileTime.from(old)); + AstDumpCache.cleanup(tempFolder.resolve("cache")); + assertEquals(Optional.of("current"), current.load(this::readUtf8)); assertTrue(Files.exists(currentEntry)); assertFalse(Files.exists(staleEntry)); diff --git a/ClangAstParser/test/pt/up/fe/specs/clang/ClangResourcesTest.java b/ClangAstParser/test/pt/up/fe/specs/clang/ClangResourcesTest.java index 79ea3ff68..6cc873a3b 100644 --- a/ClangAstParser/test/pt/up/fe/specs/clang/ClangResourcesTest.java +++ b/ClangAstParser/test/pt/up/fe/specs/clang/ClangResourcesTest.java @@ -42,6 +42,7 @@ import java.util.ArrayList; import java.util.HexFormat; import java.util.List; +import java.util.Optional; import java.util.concurrent.Executors; import java.util.concurrent.CountDownLatch; import java.util.concurrent.Future; @@ -274,28 +275,28 @@ public void concurrentInitializationLeavesOneValidIncludesTree() throws Exceptio @Test public void activelyLockedStagingDirectoriesArePreserved() throws Exception { var includesRoot = Files.createDirectories(tempFolder.resolve("includes")); - var staging = CacheFiles.createStagingDirectory(tempFolder, includesRoot, ".sha.tmp-"); - try { - CacheFiles.deleteUnlockedStagingLocks(tempFolder, includesRoot); - assertTrue(Files.exists(staging.path())); - assertTrue(Files.exists(staging.lockPath())); - } finally { - staging.close(); - CacheFiles.delete(staging.path()); + Path stagingPath; + Path lockPath; + try (var staging = CacheFiles.createStagingDirectory(tempFolder, includesRoot, ".sha.tmp-")) { + stagingPath = staging.path(); + lockPath = staging.lockPath(); + CacheFiles.cleanupDirectories(tempFolder, includesRoot, Instant.EPOCH, null); + assertTrue(Files.exists(stagingPath)); } + + assertFalse(Files.exists(stagingPath)); + assertFalse(Files.exists(lockPath)); } @Test public void unlockedStagingDirectoriesAreCleaned() throws Exception { var includesRoot = Files.createDirectories(tempFolder.resolve("includes")); - var staging = CacheFiles.createStagingDirectory(tempFolder, includesRoot, ".sha.tmp-"); - var stagingPath = staging.path(); - var lockPath = staging.lockPath(); - staging.close(); + var stagingPath = Files.createDirectory(includesRoot.resolve(".sha.tmp-123")); + var lockPath = includesRoot.resolve(".sha.tmp-123.lock"); Files.createFile(lockPath); assertTrue(Files.exists(lockPath)); - CacheFiles.deleteUnlockedStagingLocks(tempFolder, includesRoot); + CacheFiles.cleanupStagingDirectories(tempFolder, includesRoot); assertFalse(Files.exists(stagingPath)); assertFalse(Files.exists(lockPath)); @@ -307,7 +308,7 @@ public void orphanedStagingLocksAreCleaned() throws IOException { var lockPath = includesRoot.resolve(".orphan.tmp-123.lock"); Files.createFile(lockPath); - CacheFiles.deleteUnlockedStagingLocks(tempFolder, includesRoot); + CacheFiles.cleanupDirectories(tempFolder, includesRoot, Instant.EPOCH, null); assertFalse(Files.exists(lockPath)); } @@ -360,7 +361,7 @@ public void usingSharedIncludesRefreshesItsLastUsedTime() throws IOException { } @Test - public void maintenanceLockMakesUsageWinOverContendingCleanup() throws Exception { + public void claimedDirectoryRemainsUsableDuringContendingCleanup() throws Exception { var releases = Files.createDirectories(tempFolder.resolve("releases")); var stale = Files.createDirectories(releases.resolve("stale")); Files.setLastModifiedTime(stale, FileTime.from(Instant.now().minus(Duration.ofDays(61)))); @@ -370,24 +371,23 @@ public void maintenanceLockMakesUsageWinOverContendingCleanup() throws Exception var executor = Executors.newFixedThreadPool(2); try { - var usage = executor.submit(() -> CacheFiles.withMaintenanceLock(tempFolder, () -> { - CacheFiles.touch(stale); + var usage = executor.submit(() -> CacheFiles.useDirectory(tempFolder, stale, path -> { usageStarted.countDown(); awaitLatch(allowUsageToFinish); + return Optional.of(path); })); assertTrue(usageStarted.await(PROCESS_TIMEOUT.toMillis(), TimeUnit.MILLISECONDS)); var cleanup = executor.submit(() -> { cleanupStarted.countDown(); - CacheFiles.deleteStaleDirectories(tempFolder, releases, + CacheFiles.cleanupDirectories(tempFolder, releases, Instant.now().minus(Duration.ofDays(60)), null); }); assertTrue(cleanupStarted.await(PROCESS_TIMEOUT.toMillis(), TimeUnit.MILLISECONDS)); - assertFalse(cleanup.isDone()); + cleanup.get(PROCESS_TIMEOUT.toMillis(), TimeUnit.MILLISECONDS); allowUsageToFinish.countDown(); usage.get(PROCESS_TIMEOUT.toMillis(), TimeUnit.MILLISECONDS); - cleanup.get(PROCESS_TIMEOUT.toMillis(), TimeUnit.MILLISECONDS); assertTrue(Files.exists(stale)); } finally { allowUsageToFinish.countDown(); @@ -406,7 +406,7 @@ public void cleanupWinnerMakesSubsequentIncludesResolutionAcknowledgeTheMiss() t Files.writeString(shared.toPath().resolve("entrypoints.txt"), "builtin\n"); Files.setLastModifiedTime(shared.toPath(), FileTime.from(Instant.now().minus(Duration.ofDays(61)))); - CacheFiles.deleteStaleDirectories(tempFolder, shared.toPath().getParent(), + CacheFiles.cleanupDirectories(tempFolder, shared.toPath().getParent(), Instant.now().minus(Duration.ofDays(60)), null); assertFalse(shared.exists()); @@ -746,15 +746,18 @@ private MaintenanceLockHolderProcess() { } public static void main(String[] args) { - CacheFiles.withMaintenanceLock(Path.of(args[0]), () -> { - System.out.println("READY"); - System.out.flush(); - try { + var lockPath = Path.of(args[0], ".maintenance.lock"); + try { + Files.createDirectories(lockPath.getParent()); + try (var channel = FileChannel.open(lockPath, StandardOpenOption.CREATE, StandardOpenOption.WRITE); + var ignored = channel.lock()) { + System.out.println("READY"); + System.out.flush(); System.in.read(); - } catch (IOException e) { - throw new RuntimeException(e); } - }); + } catch (IOException e) { + throw new RuntimeException(e); + } System.out.println("DONE"); System.out.flush(); } @@ -788,10 +791,10 @@ public static void main(String[] args) { } catch (IOException e) { throw new RuntimeException(e); } - CacheFiles.withMaintenanceLock(Path.of(args[0]), () -> { - System.out.println("ENTERED"); - System.out.flush(); - }); + var cacheRoot = Path.of(args[0]); + CacheFiles.touch(cacheRoot, cacheRoot); + System.out.println("ENTERED"); + System.out.flush(); System.out.println("DONE"); System.out.flush(); } diff --git a/ClangAstParser/test/pt/up/fe/specs/clang/CudaResourcesTest.java b/ClangAstParser/test/pt/up/fe/specs/clang/CudaResourcesTest.java index 87d96f502..aa56b365b 100644 --- a/ClangAstParser/test/pt/up/fe/specs/clang/CudaResourcesTest.java +++ b/ClangAstParser/test/pt/up/fe/specs/clang/CudaResourcesTest.java @@ -338,14 +338,12 @@ public void invalidPublishedInstallationFailsWithoutRepair() throws IOException @Test public void abandonedStagingDirectoriesAreReclaimable() throws Exception { var cudaRoot = Files.createDirectories(tempFolder.resolve("cuda")); - var staging = CacheFiles.createStagingDirectory(tempFolder, cudaRoot, "." + RELEASE + ".tmp-"); - var stagingPath = staging.path(); - var lockPath = staging.lockPath(); + var stagingPath = Files.createDirectory(cudaRoot.resolve("." + RELEASE + ".tmp-123")); + var lockPath = cudaRoot.resolve("." + RELEASE + ".tmp-123.lock"); Files.writeString(stagingPath.resolve("partial"), "in progress"); - staging.close(); Files.createFile(lockPath); - CacheFiles.deleteUnlockedStagingLocks(tempFolder, cudaRoot); + CacheFiles.cleanupDirectories(tempFolder, cudaRoot, Instant.EPOCH, null); assertFalse(Files.exists(stagingPath)); assertFalse(Files.exists(lockPath));