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

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.

Reviewers: arphaman

Subscribers: dexonsmith, cfe-commits

Tags: #clang

Differential Revision: https://reviews.llvm.org/D68193

llvm-svn: 374366

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

@@ -168,6 +168,9 @@ private:
     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.

clang/lib/Tooling/DependencyScanning/DependencyScanningFilesystem.cpp

@@ -122,14 +122,11 @@ DependencyScanningFilesystemSharedCache::get(StringRef 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.
@@ -160,7 +157,17 @@ DependencyScanningWorkerFilesystem::status(const Twine &Path) {
 
   // 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 {
@@ -217,30 +224,8 @@ 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);
-    }
-
-    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);
 }

clang/test/ClangScanDeps/Inputs/headerwithdirnamefollowedbyinclude.json

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

clang/test/ClangScanDeps/headerwithdirnamefollowedbyinclude.cpp

@@ -0,0 +1,21 @@
+// 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