In openFileForRead don't cache erroneous entries if the error relates to them being directories. Add tests.

[kousikk]

Summary

It seems that when the CachingFileSystem is first given a file to open that is actually a directory, it incorrectly
caches that path to be errenous and throws an error when subsequently a directory open call is made for the same
path.
This change makes it so that we do NOT cache a path if it turns out we asked for a file when its a directory.

[dexonsmith]

clang/lib/Tooling/DependencyScanning/DependencyScanningFilesystem.cpp

23–24

Note that the file is opened here.

226–227

It seems wasteful to do an extra stat here when the file is already open in createFileEntry.

Can you instead change createFileEntry to return std::errc::is_a_directory as appropriate to avoid the extra filesystem access? (Is it possible that it's already doing that, and you just need to check for that?)

[kousikk]

Avoid the extra stat() call

[kousikk]

clang/lib/Tooling/DependencyScanning/DependencyScanningFilesystem.cpp

226–227

Thanks for the suggestion, done.

[dexonsmith]

Sorry for bouncing you around, but I just had a look at the other user of createFileEntry and I think the right thing to do is to somehow share the code between DependencyScanningWorkerFilesystem::status and this. I suggest splitting a function out of status (called getOrCreateFileSystemEntry?) that returns a CachedFileSystemEntry (or an error).

The fix I just asked you to make (to only call stat once) could be done on getOrCreateFileSystemEntry as a follow-up, using std::unique_ptr<llvm::vfs::File> and changing getFileEntry to take that instead of a filename.

[kousikk]

Address code-reorg comment

[kousikk]

In D68193#1688838, @dexonsmith wrote:

Sorry for bouncing you around, but I just had a look at the other user of createFileEntry and I think the right thing to do is to somehow share the code between DependencyScanningWorkerFilesystem::status and this. I suggest splitting a function out of status (called getOrCreateFileSystemEntry?) that returns a CachedFileSystemEntry (or an error).

The fix I just asked you to make (to only call stat once) could be done on getOrCreateFileSystemEntry as a follow-up, using std::unique_ptr<llvm::vfs::File> and changing getFileEntry to take that instead of a filename.

IIUC,
DependencyScanningWorkerFilesystem::status() and DependencyScanningWorkerFilesystem::openFileForRead() both inturn call into CachedFileSystemEntry::createFileEntry().

Given that, I made getOrCreateFileEntry() a private-member of DependencyScanningWorkerFilesystem and made both these functions use it first.

In a subsequent step, I passed in llvm::vfs::Status object to createFileEntry() instead of Filename. I hope this aligns with the code-organization you are looking for(happy to re-write if not).

I also ran it against a canonical set of tests I'm maintaining internally and I didn't notice a difference in either performance and didn't get new crashes of the tool, so I think there should NOT be any regressions because of this change.

[kousikk]

Add missed const

[arphaman]

I don't quite understand the specific issue you're hitting, as it sounds that the logic right now should handle it:

It seems that when the CachingFileSystem is first given a file to open that is actually a directory, it incorrectly
caches that path to be errenous and throws an error when subsequently a directory open >call is made for the same
path.

In this case, right now (without this patch) createFileEntry will return std::errc::is_a_directory. DependencyScanningWorkerFilesystem::openFileForRead will then invalidate the entry in the global cache:

if (CacheEntry.getError() == std::errc::is_a_directory) {
    // Reset the CacheEntry to avoid setting an error entry in the
    // cache for the given directory path.
    CacheEntry = CachedFileSystemEntry();

Which means that when the call to stat happens, it should be fine as the global cache doesn't have a valid entry, so it will be able to recognize a directory and won't return an error.

Does this not happen in your case?

[kousikk]

In D68193#1692293, @arphaman wrote:

I don't quite understand the specific issue you're hitting, as it sounds that the logic right now should handle it:

It seems that when the CachingFileSystem is first given a file to open that is actually a directory, it incorrectly
caches that path to be errenous and throws an error when subsequently a directory open >call is made for the same
path.

In this case, right now (without this patch) createFileEntry will return std::errc::is_a_directory. DependencyScanningWorkerFilesystem::openFileForRead will then invalidate the entry in the global cache:

createFileEntry does return std::errc::is_a_directory, but the cache entry is NOT invalidated from the global cache. It stays on in the global cache, as an error entry for that directory path. I think the current version is slightly better because the first time we see a path, we determine if its a file / directory and store the appropriate CachedFileSystemEntry in the global cache.

if (CacheEntry.getError() == std::errc::is_a_directory) {
    // Reset the CacheEntry to avoid setting an error entry in the
    // cache for the given directory path.
    CacheEntry = CachedFileSystemEntry();

Which means that when the call to stat happens, it should be fine as the global cache doesn't have a valid entry, so it will be able to recognize a directory and won't return an error.

Does this not happen in your case?

I think the difference is the global cache does have the entry (but its set to an Error object). The test-case I added should currently fail without the changes in this CL.

[kousikk]

Update diff to include all commits

[kousikk]

@dexonsmith when you get a chance, can you take another look at this? Thanks!

[arphaman]

@kousikk Thanks, I understand your patch better now. It makes more sense for sure.

When we're opening the file we shouldn't stat before calling open, as there's a race condition introduced, where the value of the stat could change between the call between stat and open is performed. We've seen problems like this before, and it ends up in crashes and mismatch size errors as Clang is getting invalid size from the stat if the file is modified in that time. We should still call open + fstat like we used. So I would recommend not changing the createFileEntry function to take in a stat, and do the fstat after opening the file like it used to.

[kousikk]

@arphaman I don't mind changing this if there are race conditions as you say, but isn't the assumption of the tool that the filesystem remains unchanged for a single run of the tool? If so, should we actually throw error conditions instead of crashing in those cases?

[Bigcheese]

In D68193#1701426, @kousikk wrote:

@arphaman I don't mind changing this if there are race conditions as you say, but isn't the assumption of the tool that the filesystem remains unchanged for a single run of the tool? If so, should we actually throw error conditions instead of crashing in those cases?

This is true for a normal build, but it is not true with modules. All file system access goes through this, even for modules. I'm currently fixing an issue where we cache the non-existence of the module cache directory, causing the subsequent module load to fail. There is also an issue due to minimization where if we return the underlying stat result, the file size after minimization is different.

[kousikk]

• Revert changes to createFileEntry since that may result in a race condition if stat is called before file open

[Bigcheese]

clang/include/clang/Tooling/DependencyScanning/DependencyScanningFilesystem.h

83

Is this actually called anywhere?

[kousikk]

Remove getError() function that's now unused

[Bigcheese]

lgtm, but wait for Alex or Duncan to also take a look.

[dexonsmith]

LGTM too.

[arphaman]

LGTM

clang/include/clang/Tooling/DependencyScanning/DependencyScanningFilesystem.h

public:
   /// Create an entry that represents an opened source file with minimized or
   /// original contents.
   ///
   /// The filesystem opens the file even for `stat` calls open to avoid the
   /// issues with stat + open of minimized files that might lead to a
   /// mismatching size of the file. If file is not minimized, the full file is
   /// read and copied into memory to ensure that it's not memory mapped to avoid
   /// running out of file descriptors.
-  static CachedFileSystemEntry createFileEntry(StringRef Filename,
+  static CachedFileSystemEntry createFileEntry(llvm::vfs::Status &&Stat,
                                                llvm::vfs::FileSystem &FS,
                                                bool Minimize = true);
 
   /// Create an entry that represents a directory on the filesystem.
   static CachedFileSystemEntry createDirectoryEntry(llvm::vfs::Status &&Stat);
 
   /// \returns True if the entry is valid.
   bool isValid() const { return !MaybeStat || MaybeStat->isStatusKnown(); }
public:
   }
 
   /// \returns the name of the file.
   StringRef getName() const {
     assert(isValid() && "not initialized");
     return MaybeStat->getName();
   }
 
   std::error_code getError() const {
     assert(isValid() && "not initialized");
     return MaybeStat.getError();
   }
 
   /// Return the mapping between location -> distance that is used to speed up
   /// the block skipping in the preprocessor.
   const PreprocessorSkippedRangeMapping &getPPSkippedRangeMapping() const {
     return PPSkippedRangeMapping;
void setCachedEntry(StringRef Filename, const CachedFileSystemEntry *Entry) {
     assert(IsInserted && "local cache is updated more than once");
   }
 
   const CachedFileSystemEntry *getCachedEntry(StringRef Filename) {
     auto It = Cache.find(Filename);
     return It == Cache.end() ? nullptr : It->getValue();
   }
 
+  llvm::ErrorOr<const CachedFileSystemEntry *>
+  getOrCreateFileSystemEntry(const StringRef Filename);
+
   DependencyScanningFilesystemSharedCache &SharedCache;
   /// The local cache is used by the worker thread to cache file system queries
   /// locally instead of querying the global cache every time.
   llvm::StringMap<const CachedFileSystemEntry *, llvm::BumpPtrAllocator> Cache;
   /// The optional mapping structure which records information about the
   /// excluded conditional directive skip mappings that are used by the
   /// currently active preprocessor.
   ExcludedPreprocessorDirectiveSkipMapping *PPSkipMappings;
 };
 
 } // end namespace dependencies
 } // end namespace tooling
 } // end namespace clang
 
 #endif // LLVM_CLANG_TOOLING_DEPENDENCY_SCANNING_FILESYSTEM_H

clang/lib/Tooling/DependencyScanning/DependencyScanningFilesystem.cpp

 #include "clang/Lex/DependencyDirectivesSourceMinimizer.h"
 #include "llvm/Support/MemoryBuffer.h"
 #include "llvm/Support/Threading.h"
 
 using namespace clang;
 using namespace tooling;
 using namespace dependencies;
 
+
 CachedFileSystemEntry CachedFileSystemEntry::createFileEntry(
-    StringRef Filename, llvm::vfs::FileSystem &FS, bool Minimize) {
+    llvm::vfs::Status &&Stat, llvm::vfs::FileSystem &FS, bool Minimize) {
   // Load the file and its content from the file system.
+  StringRef Filename = Stat.getName();
   llvm::ErrorOr<std::unique_ptr<llvm::vfs::File>> MaybeFile =
       FS.openFileForRead(Filename);
   if (!MaybeFile)
     return MaybeFile.getError();
-  llvm::ErrorOr<llvm::vfs::Status> Stat = (*MaybeFile)->status();
-  if (!Stat)
-    return Stat.getError();
-  if (Stat->isDirectory())
-    return std::make_error_code(std::errc::is_a_directory);
 
   llvm::vfs::File &F = **MaybeFile;
   llvm::ErrorOr<std::unique_ptr<llvm::MemoryBuffer>> MaybeBuffer =
-      F.getBuffer(Stat->getName());
+      F.getBuffer(Filename);
   if (!MaybeBuffer)
     return MaybeBuffer.getError();
 
   llvm::SmallString<1024> MinimizedFileContents;
   // Minimize the file down to directives that might affect the dependencies.
   const auto &Buffer = *MaybeBuffer;
   SmallVector<minimize_source_to_dependency_directives::Token, 64> Tokens;
   if (!Minimize || minimizeSourceToDependencyDirectives(
                        Buffer->getBuffer(), MinimizedFileContents, Tokens)) {
     // Use the original file unless requested otherwise, or
     // if the minimization failed.
     // FIXME: Propage the diagnostic if desired by the client.
     CachedFileSystemEntry Result;
-    Result.MaybeStat = std::move(*Stat);
+    Result.MaybeStat = Stat;
     Result.Contents.reserve(Buffer->getBufferSize() + 1);
     Result.Contents.append(Buffer->getBufferStart(), Buffer->getBufferEnd());
     // Implicitly null terminate the contents for Clang's lexer.
     Result.Contents.push_back('\0');
     Result.Contents.pop_back();
     return Result;
   }
 
   CachedFileSystemEntry Result;
   size_t Size = MinimizedFileContents.size();
-  Result.MaybeStat = llvm::vfs::Status(Stat->getName(), Stat->getUniqueID(),
-                                       Stat->getLastModificationTime(),
-                                       Stat->getUser(), Stat->getGroup(), Size,
-                                       Stat->getType(), Stat->getPermissions());
+  Result.MaybeStat = llvm::vfs::Status(Stat.getName(), Stat.getUniqueID(),
+                                       Stat.getLastModificationTime(),
+                                       Stat.getUser(), Stat.getGroup(), Size,
+                                       Stat.getType(), Stat.getPermissions());
   // The contents produced by the minimizer must be null terminated.
   assert(MinimizedFileContents.data()[MinimizedFileContents.size()] == '\0' &&
          "not null terminated contents");
   // Even though there's an implicit null terminator in the minimized contents,
   // we want to temporarily make it explicit. This will ensure that the
   // std::move will preserve it even if it needs to do a copy if the
   // SmallString still has the small capacity.
   MinimizedFileContents.push_back('\0');

 DependencyScanningFilesystemSharedCache::SharedFileSystemEntry &
 DependencyScanningFilesystemSharedCache::get(StringRef Key) {
   CacheShard &Shard = CacheShards[llvm::hash_value(Key) % NumShards];
   std::unique_lock<std::mutex> LockGuard(Shard.CacheLock);
   auto It = Shard.Cache.try_emplace(Key);
   return It.first->getValue();
 }
 
-llvm::ErrorOr<llvm::vfs::Status>
-DependencyScanningWorkerFilesystem::status(const Twine &Path) {
-  SmallString<256> OwnedFilename;
-  StringRef Filename = Path.toStringRef(OwnedFilename);
-
-  // Check the local cache first.
-  if (const CachedFileSystemEntry *Entry = getCachedEntry(Filename))
-    return Entry->getStatus();
+llvm::ErrorOr<const CachedFileSystemEntry *>
+DependencyScanningWorkerFilesystem::getOrCreateFileSystemEntry(const StringRef Filename) {
+  if (const CachedFileSystemEntry* Entry = getCachedEntry(Filename)) {
+    return Entry;
+  }
 
   // FIXME: Handle PCM/PCH files.
   // FIXME: Handle module map files.
 
   bool KeepOriginalSource = IgnoredFiles.count(Filename);
   DependencyScanningFilesystemSharedCache::SharedFileSystemEntry
       &SharedCacheEntry = SharedCache.get(Filename);
   const CachedFileSystemEntry *Result;
   {
     std::unique_lock<std::mutex> LockGuard(SharedCacheEntry.ValueLock);
     CachedFileSystemEntry &CacheEntry = SharedCacheEntry.Value;
 
     if (!CacheEntry.isValid()) {
       llvm::vfs::FileSystem &FS = getUnderlyingFS();
       auto MaybeStatus = FS.status(Filename);
       if (!MaybeStatus)
         CacheEntry = CachedFileSystemEntry(MaybeStatus.getError());
       else if (MaybeStatus->isDirectory())
         CacheEntry = CachedFileSystemEntry::createDirectoryEntry(
             std::move(*MaybeStatus));
       else
         CacheEntry = CachedFileSystemEntry::createFileEntry(
-            Filename, FS, !KeepOriginalSource);
+            std::move(*MaybeStatus), FS, !KeepOriginalSource);
     }
 
     Result = &CacheEntry;
   }
 
   // Store the result in the local cache.
   setCachedEntry(Filename, Result);
-  return Result->getStatus();
+  return Result;
+}
+
+llvm::ErrorOr<llvm::vfs::Status>
+DependencyScanningWorkerFilesystem::status(const Twine &Path) {
+  SmallString<256> OwnedFilename;
+  StringRef Filename = Path.toStringRef(OwnedFilename);
+  const llvm::ErrorOr<const CachedFileSystemEntry *> Result = getOrCreateFileSystemEntry(Filename);
+  if (!Result)
+    return Result.getError();
+  return (*Result)->getStatus();
 }
 
 namespace {
 
 /// The VFS that is used by clang consumes the \c CachedFileSystemEntry using
 /// this subclass.
 class MinimizedVFSFile final : public llvm::vfs::File {
 public:

 
 } // end anonymous namespace
 
 llvm::ErrorOr<std::unique_ptr<llvm::vfs::File>>
 DependencyScanningWorkerFilesystem::openFileForRead(const Twine &Path) {
   SmallString<256> OwnedFilename;
   StringRef Filename = Path.toStringRef(OwnedFilename);
 
-  // Check the local cache first.
-  if (const CachedFileSystemEntry *Entry = getCachedEntry(Filename))
-    return createFile(Entry, PPSkipMappings);
-
-  // FIXME: Handle PCM/PCH files.
-  // FIXME: Handle module map files.
-
-  bool KeepOriginalSource = IgnoredFiles.count(Filename);
-  DependencyScanningFilesystemSharedCache::SharedFileSystemEntry
-      &SharedCacheEntry = SharedCache.get(Filename);
-  const CachedFileSystemEntry *Result;
-  {
-    std::unique_lock<std::mutex> LockGuard(SharedCacheEntry.ValueLock);
-    CachedFileSystemEntry &CacheEntry = SharedCacheEntry.Value;
-
-    if (!CacheEntry.isValid()) {
-      CacheEntry = CachedFileSystemEntry::createFileEntry(
-        Filename, getUnderlyingFS(), !KeepOriginalSource);
-      if (CacheEntry.getError() == std::errc::is_a_directory) {
-        // Reset the CacheEntry to avoid setting an error entry in the
-        // cache for the given directory path.
-        CacheEntry = CachedFileSystemEntry();
-        return llvm::ErrorOr<std::unique_ptr<llvm::vfs::File>>(std::errc::is_a_directory);
-      }
-    }
-
-    Result = &CacheEntry;
-  }
-
-  // Store the result in the local cache.
-  setCachedEntry(Filename, Result);
-  return createFile(Result, PPSkipMappings);
+  const llvm::ErrorOr<const CachedFileSystemEntry *> Result = getOrCreateFileSystemEntry(Filename);
+  if (!Result)
+    return Result.getError();
+  return createFile(Result.get(), PPSkipMappings);
 }