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

21–22

Note that the file is opened here.

236–237

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

236–237

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 ↗

(On Diff #224219)

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/lib/Tooling/DependencyScanning/DependencyScanningFilesystem.cpp

 
 using namespace clang;
 using namespace tooling;
 using namespace dependencies;
 
 CachedFileSystemEntry CachedFileSystemEntry::createFileEntry(
     StringRef Filename, llvm::vfs::FileSystem &FS, bool Minimize) {
   // Load the file and its content from the file system.
   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();
 
   llvm::vfs::File &F = **MaybeFile;
   llvm::ErrorOr<std::unique_ptr<llvm::MemoryBuffer>> MaybeBuffer =
DependencyScanningWorkerFilesystem::openFileForRead(const Twine &Path) {
   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 && MaybeStatus->isDirectory())
+        return llvm::ErrorOr<std::unique_ptr<llvm::vfs::File>>(
+          std::make_error_code(std::errc::is_a_directory));
+
       CacheEntry = CachedFileSystemEntry::createFileEntry(
-          Filename, getUnderlyingFS(), !KeepOriginalSource);
+          Filename, FS, !KeepOriginalSource);
     }
 
     Result = &CacheEntry;
   }
 
   // Store the result in the local cache.
   setCachedEntry(Filename, Result);
   return createFile(Result, PPSkipMappings);
 }

clang/test/ClangScanDeps/Inputs/headerwithdirnamefollowedbyinclude.json

+[
+    {
+      "directory": "DIR",
+      "command": "clang -c -IDIR -IInputs DIR/headerwithdirname_input.cpp",
+      "file": "DIR/headerwithdirname_input.cpp"
+    }
+]

clang/test/ClangScanDeps/headerwithdirnamefollowedbyinclude.cpp

+// RUN: rm -rf %t.dir
+// RUN: rm -rf %t.dir/foodir
+// RUN: rm -rf %t.cdb
+
+// RUN: mkdir -p %t.dir
+// RUN: mkdir -p %t.dir/foodir
+
+// RUN: cp %S/Inputs/header.h %t.dir/foodir/foodirheader.h
+// RUN: cp %s %t.dir/headerwithdirname_input.cpp
+// RUN: mkdir %t.dir/Inputs
+// RUN: cp %S/Inputs/foodir %t.dir/Inputs/foodir
+// RUN: sed -e "s|DIR|%/t.dir|g" %S/Inputs/headerwithdirnamefollowedbyinclude.json > %t.cdb
+//
+// RUN: clang-scan-deps -compilation-database %t.cdb -j 1 | FileCheck %s
+
+#include <foodir>
+#include "foodir/foodirheader.h"
+
+// CHECK: headerwithdirname_input.o
+// CHECK-NEXT: headerwithdirname_input.cpp
+// CHECK-NEXT: Inputs{{/|\\}}foodir