From e718f3240a57454ae14022cf3ccaba5245786a53 Mon Sep 17 00:00:00 2001 From: Argyrios Kyrtzidis Date: Tue, 19 Sep 2023 18:18:23 -0700 Subject: [PATCH] [DependencyScanningFilesystem] Make sure the local/shared cache filename lookups use only absolute paths (#66122) 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. (cherry picked from commit 36b37c775c285bbff9b57630e7ea9d00b918cc91) --- .../DependencyScanningFilesystem.h | 18 ++++- .../DependencyScanningFilesystem.cpp | 79 +++++++++++++++---- clang/test/ClangScanDeps/relative-filenames.c | 38 +++++++++ 3 files changed, 117 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 4b4e3c7eb2ecd0..dbe219b6dd8d72 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 FS) - : ProxyFileSystem(std::move(FS)), SharedCache(SharedCache) {} + IntrusiveRefCntPtr FS); llvm::ErrorOr status(const Twine &Path) override; llvm::ErrorOr> 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 - 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. + llvm::ErrorOr WorkingDirForCacheLookup; + + void updateWorkingDirForCacheLookup(); }; } // end namespace dependencies diff --git a/clang/lib/Tooling/DependencyScanning/DependencyScanningFilesystem.cpp b/clang/lib/Tooling/DependencyScanning/DependencyScanningFilesystem.cpp index 31404855e3b1dc..3e53c8fc574087 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 LockGuard(CacheLock); auto It = EntriesByFilename.find(Filename); return It == EntriesByFilename.end() ? nullptr : It->getValue(); @@ -189,6 +191,14 @@ static bool shouldCacheStatFailures(StringRef Filename) { return shouldScanForDirectivesBasedOnExtension(Filename); } +DependencyScanningWorkerFilesystem::DependencyScanningWorkerFilesystem( + DependencyScanningFilesystemSharedCache &SharedCache, + IntrusiveRefCntPtr FS) + : ProxyFileSystem(std::move(FS)), SharedCache(SharedCache), + WorkingDirForCacheLookup(llvm::errc::invalid_argument) { + updateWorkingDirForCacheLookup(); +} + bool DependencyScanningWorkerFilesystem::shouldScanForDirectives( StringRef Filename) { return shouldScanForDirectivesBasedOnExtension(Filename); @@ -215,44 +225,62 @@ DependencyScanningWorkerFilesystem::findEntryByFilenameWithWriteThrough( } llvm::ErrorOr -DependencyScanningWorkerFilesystem::computeAndStoreResult(StringRef Filename) { - llvm::ErrorOr Stat = getUnderlyingFS().status(Filename); +DependencyScanningWorkerFilesystem::computeAndStoreResult( + StringRef OriginalFilename, StringRef FilenameForLookup) { + llvm::ErrorOr 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 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 if (!WorkingDirForCacheLookup) { + return WorkingDirForCacheLookup.getError(); + } else { + StringRef RelFilename = OriginalFilename; + RelFilename.consume_front("./"); + PathBuf = *WorkingDirForCacheLookup; + llvm::sys::path::append(PathBuf, RelFilename); + 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 +358,24 @@ DependencyScanningWorkerFilesystem::openFileForRead(const Twine &Path) { return Result.getError(); return DepScanFile::create(Result.get()); } + +std::error_code DependencyScanningWorkerFilesystem::setCurrentWorkingDirectory( + const Twine &Path) { + std::error_code EC = ProxyFileSystem::setCurrentWorkingDirectory(Path); + updateWorkingDirForCacheLookup(); + return EC; +} + +void DependencyScanningWorkerFilesystem::updateWorkingDirForCacheLookup() { + llvm::ErrorOr CWD = + getUnderlyingFS().getCurrentWorkingDirectory(); + if (!CWD) { + WorkingDirForCacheLookup = CWD.getError(); + } else if (!llvm::sys::path::is_absolute_gnu(*CWD)) { + WorkingDirForCacheLookup = llvm::errc::invalid_argument; + } else { + WorkingDirForCacheLookup = *CWD; + } + assert(!WorkingDirForCacheLookup || + 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 00000000000000..03f2be7ec4c1f6 --- /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