[clang] [DependencyScanningFilesystem] Make sure the local/shared cache filename lookups use only absolute paths (PR #66122)
Argyrios Kyrtzidis via cfe-commits
cfe-commits at lists.llvm.org
Tue Sep 12 16:20:02 PDT 2023
https://github.com/akyrtzi updated https://github.com/llvm/llvm-project/pull/66122:
>From 98e56d4d8d1f138cfc69c035425fafcdd57d5916 Mon Sep 17 00:00:00 2001
From: Argyrios Kyrtzidis <kyrtzidis at apple.com>
Date: Tue, 12 Sep 2023 11:26:46 -0700
Subject: [PATCH] [DependencyScanningFilesystem] Make sure the local/shared
cache filename lookups use only absolute paths
Previously a relative path would be used as a key for cache lookup and if the same relative path
was used from another compiler invocation with a different working directory then the first cache entry
was erroneously returned.
---
.../DependencyScanningFilesystem.h | 18 ++++-
.../DependencyScanningFilesystem.cpp | 76 +++++++++++++++----
clang/test/ClangScanDeps/relative-filenames.c | 38 ++++++++++
3 files changed, 114 insertions(+), 18 deletions(-)
create mode 100644 clang/test/ClangScanDeps/relative-filenames.c
diff --git a/clang/include/clang/Tooling/DependencyScanning/DependencyScanningFilesystem.h b/clang/include/clang/Tooling/DependencyScanning/DependencyScanningFilesystem.h
index 4b4e3c7eb2ecd06..7c8382024a8a4f4 100644
--- a/clang/include/clang/Tooling/DependencyScanning/DependencyScanningFilesystem.h
+++ b/clang/include/clang/Tooling/DependencyScanning/DependencyScanningFilesystem.h
@@ -215,6 +215,7 @@ class DependencyScanningFilesystemLocalCache {
public:
/// Returns entry associated with the filename or nullptr if none is found.
const CachedFileSystemEntry *findEntryByFilename(StringRef Filename) const {
+ assert(llvm::sys::path::is_absolute_gnu(Filename));
auto It = Cache.find(Filename);
return It == Cache.end() ? nullptr : It->getValue();
}
@@ -224,6 +225,7 @@ class DependencyScanningFilesystemLocalCache {
const CachedFileSystemEntry &
insertEntryForFilename(StringRef Filename,
const CachedFileSystemEntry &Entry) {
+ assert(llvm::sys::path::is_absolute_gnu(Filename));
const auto *InsertedEntry = Cache.insert({Filename, &Entry}).first->second;
assert(InsertedEntry == &Entry && "entry already present");
return *InsertedEntry;
@@ -282,13 +284,14 @@ class DependencyScanningWorkerFilesystem : public llvm::vfs::ProxyFileSystem {
public:
DependencyScanningWorkerFilesystem(
DependencyScanningFilesystemSharedCache &SharedCache,
- IntrusiveRefCntPtr<llvm::vfs::FileSystem> FS)
- : ProxyFileSystem(std::move(FS)), SharedCache(SharedCache) {}
+ IntrusiveRefCntPtr<llvm::vfs::FileSystem> FS);
llvm::ErrorOr<llvm::vfs::Status> status(const Twine &Path) override;
llvm::ErrorOr<std::unique_ptr<llvm::vfs::File>>
openFileForRead(const Twine &Path) override;
+ std::error_code setCurrentWorkingDirectory(const Twine &Path) override;
+
/// Returns entry for the given filename.
///
/// Attempts to use the local and shared caches first, then falls back to
@@ -304,8 +307,11 @@ class DependencyScanningWorkerFilesystem : public llvm::vfs::ProxyFileSystem {
/// For a filename that's not yet associated with any entry in the caches,
/// uses the underlying filesystem to either look up the entry based in the
/// shared cache indexed by unique ID, or creates new entry from scratch.
+ /// \p FilenameForLookup will always be an absolute path, and different than
+ /// \p OriginalFilename if \p OriginalFilename is relative.
llvm::ErrorOr<const CachedFileSystemEntry &>
- computeAndStoreResult(StringRef Filename);
+ computeAndStoreResult(StringRef OriginalFilename,
+ StringRef FilenameForLookup);
/// Scan for preprocessor directives for the given entry if necessary and
/// returns a wrapper object with reference semantics.
@@ -388,6 +394,12 @@ class DependencyScanningWorkerFilesystem : public llvm::vfs::ProxyFileSystem {
/// The local cache is used by the worker thread to cache file system queries
/// locally instead of querying the global cache every time.
DependencyScanningFilesystemLocalCache LocalCache;
+
+ /// The working directory to use for making relative paths absolute before
+ /// using them for cache lookups.
+ std::string WorkingDirForCacheLookup;
+
+ void updateWorkingDirForCacheLookup();
};
} // end namespace dependencies
diff --git a/clang/lib/Tooling/DependencyScanning/DependencyScanningFilesystem.cpp b/clang/lib/Tooling/DependencyScanning/DependencyScanningFilesystem.cpp
index 31404855e3b1dc3..dd329f41cf5c30a 100644
--- a/clang/lib/Tooling/DependencyScanning/DependencyScanningFilesystem.cpp
+++ b/clang/lib/Tooling/DependencyScanning/DependencyScanningFilesystem.cpp
@@ -96,6 +96,7 @@ DependencyScanningFilesystemSharedCache::
DependencyScanningFilesystemSharedCache::CacheShard &
DependencyScanningFilesystemSharedCache::getShardForFilename(
StringRef Filename) const {
+ assert(llvm::sys::path::is_absolute_gnu(Filename));
return CacheShards[llvm::hash_value(Filename) % NumShards];
}
@@ -109,6 +110,7 @@ DependencyScanningFilesystemSharedCache::getShardForUID(
const CachedFileSystemEntry *
DependencyScanningFilesystemSharedCache::CacheShard::findEntryByFilename(
StringRef Filename) const {
+ assert(llvm::sys::path::is_absolute_gnu(Filename));
std::lock_guard<std::mutex> LockGuard(CacheLock);
auto It = EntriesByFilename.find(Filename);
return It == EntriesByFilename.end() ? nullptr : It->getValue();
@@ -189,6 +191,13 @@ static bool shouldCacheStatFailures(StringRef Filename) {
return shouldScanForDirectivesBasedOnExtension(Filename);
}
+DependencyScanningWorkerFilesystem::DependencyScanningWorkerFilesystem(
+ DependencyScanningFilesystemSharedCache &SharedCache,
+ IntrusiveRefCntPtr<llvm::vfs::FileSystem> FS)
+ : ProxyFileSystem(std::move(FS)), SharedCache(SharedCache) {
+ updateWorkingDirForCacheLookup();
+}
+
bool DependencyScanningWorkerFilesystem::shouldScanForDirectives(
StringRef Filename) {
return shouldScanForDirectivesBasedOnExtension(Filename);
@@ -215,44 +224,58 @@ DependencyScanningWorkerFilesystem::findEntryByFilenameWithWriteThrough(
}
llvm::ErrorOr<const CachedFileSystemEntry &>
-DependencyScanningWorkerFilesystem::computeAndStoreResult(StringRef Filename) {
- llvm::ErrorOr<llvm::vfs::Status> Stat = getUnderlyingFS().status(Filename);
+DependencyScanningWorkerFilesystem::computeAndStoreResult(
+ StringRef OriginalFilename, StringRef FilenameForLookup) {
+ llvm::ErrorOr<llvm::vfs::Status> Stat =
+ getUnderlyingFS().status(OriginalFilename);
if (!Stat) {
- if (!shouldCacheStatFailures(Filename))
+ if (!shouldCacheStatFailures(OriginalFilename))
return Stat.getError();
const auto &Entry =
- getOrEmplaceSharedEntryForFilename(Filename, Stat.getError());
- return insertLocalEntryForFilename(Filename, Entry);
+ getOrEmplaceSharedEntryForFilename(FilenameForLookup, Stat.getError());
+ return insertLocalEntryForFilename(FilenameForLookup, Entry);
}
if (const auto *Entry = findSharedEntryByUID(*Stat))
- return insertLocalEntryForFilename(Filename, *Entry);
+ return insertLocalEntryForFilename(FilenameForLookup, *Entry);
auto TEntry =
- Stat->isDirectory() ? TentativeEntry(*Stat) : readFile(Filename);
+ Stat->isDirectory() ? TentativeEntry(*Stat) : readFile(OriginalFilename);
const CachedFileSystemEntry *SharedEntry = [&]() {
if (TEntry) {
const auto &UIDEntry = getOrEmplaceSharedEntryForUID(std::move(*TEntry));
- return &getOrInsertSharedEntryForFilename(Filename, UIDEntry);
+ return &getOrInsertSharedEntryForFilename(FilenameForLookup, UIDEntry);
}
- return &getOrEmplaceSharedEntryForFilename(Filename, TEntry.getError());
+ return &getOrEmplaceSharedEntryForFilename(FilenameForLookup,
+ TEntry.getError());
}();
- return insertLocalEntryForFilename(Filename, *SharedEntry);
+ return insertLocalEntryForFilename(FilenameForLookup, *SharedEntry);
}
llvm::ErrorOr<EntryRef>
DependencyScanningWorkerFilesystem::getOrCreateFileSystemEntry(
- StringRef Filename, bool DisableDirectivesScanning) {
- if (const auto *Entry = findEntryByFilenameWithWriteThrough(Filename))
- return scanForDirectivesIfNecessary(*Entry, Filename,
+ StringRef OriginalFilename, bool DisableDirectivesScanning) {
+ StringRef FilenameForLookup;
+ SmallString<256> PathBuf;
+ if (llvm::sys::path::is_absolute_gnu(OriginalFilename)) {
+ FilenameForLookup = OriginalFilename;
+ } else {
+ PathBuf = WorkingDirForCacheLookup;
+ llvm::sys::path::append(PathBuf, OriginalFilename);
+ FilenameForLookup = PathBuf.str();
+ }
+ assert(llvm::sys::path::is_absolute_gnu(FilenameForLookup));
+ if (const auto *Entry =
+ findEntryByFilenameWithWriteThrough(FilenameForLookup))
+ return scanForDirectivesIfNecessary(*Entry, OriginalFilename,
DisableDirectivesScanning)
.unwrapError();
- auto MaybeEntry = computeAndStoreResult(Filename);
+ auto MaybeEntry = computeAndStoreResult(OriginalFilename, FilenameForLookup);
if (!MaybeEntry)
return MaybeEntry.getError();
- return scanForDirectivesIfNecessary(*MaybeEntry, Filename,
+ return scanForDirectivesIfNecessary(*MaybeEntry, OriginalFilename,
DisableDirectivesScanning)
.unwrapError();
}
@@ -330,3 +353,26 @@ DependencyScanningWorkerFilesystem::openFileForRead(const Twine &Path) {
return Result.getError();
return DepScanFile::create(Result.get());
}
+
+std::error_code DependencyScanningWorkerFilesystem::setCurrentWorkingDirectory(
+ const Twine &Path) {
+ if (std::error_code EC = ProxyFileSystem::setCurrentWorkingDirectory(Path))
+ return EC;
+ updateWorkingDirForCacheLookup();
+ return std::error_code();
+}
+
+void DependencyScanningWorkerFilesystem::updateWorkingDirForCacheLookup() {
+ llvm::ErrorOr<std::string> CWD =
+ getUnderlyingFS().getCurrentWorkingDirectory();
+ if (!CWD) {
+ // The cache lookup functions will not accept relative paths for safety, so
+ // at least make it absolute from a "root".
+ WorkingDirForCacheLookup = "/";
+ } else if (!llvm::sys::path::is_absolute_gnu(*CWD)) {
+ WorkingDirForCacheLookup = (Twine("/") + *CWD).str();
+ } else {
+ WorkingDirForCacheLookup = *CWD;
+ }
+ assert(llvm::sys::path::is_absolute_gnu(WorkingDirForCacheLookup));
+}
diff --git a/clang/test/ClangScanDeps/relative-filenames.c b/clang/test/ClangScanDeps/relative-filenames.c
new file mode 100644
index 000000000000000..03f2be7ec4c1f6a
--- /dev/null
+++ b/clang/test/ClangScanDeps/relative-filenames.c
@@ -0,0 +1,38 @@
+// RUN: rm -rf %t
+// RUN: split-file %s %t
+// RUN: sed -e "s|DIR|%/t|g" %t/cdb.json.template > %t/cdb.json
+
+// RUN: clang-scan-deps -compilation-database %t/cdb.json -format make -j 1 > %t/result.txt
+// RUN: FileCheck %s -input-file=%t/result.txt
+
+// CHECK: {{/|\\}}dir1{{/|\\}}t1.c
+// CHECK: {{/|\\}}dir1{{/|\\}}head.h
+// CHECK: {{/|\\}}dir2{{/|\\}}t2.c
+// CHECK: {{/|\\}}dir2{{/|\\}}head.h
+
+//--- cdb.json.template
+[
+ {
+ "directory": "DIR/dir1",
+ "command": "clang -fsyntax-only t1.c",
+ "file": "t1.c"
+ },
+ {
+ "directory": "DIR/dir2",
+ "command": "clang -fsyntax-only t2.c",
+ "file": "t2.c"
+ }
+]
+
+//--- dir1/t1.c
+#include "head.h"
+
+//--- dir1/head.h
+#ifndef BBB
+#define BBB
+#endif
+
+//--- dir2/t2.c
+#include "head.h"
+
+//--- dir2/head.h
More information about the cfe-commits
mailing list