Introduce a DirectoryEntryRef that stores both a reference and an accessed name to the directory entry

[arphaman]

Summary

This patch is similar to the FileEntryRef patch, in that it introduces a parallel API to get the directory entry, without replacing all uses. The immediate use in the patch fixes the issue where a file manager was reused in clang-scan-deps, but reported an different file path whenever a framework lookup was done through a symlink.

[dexonsmith]

A few minor comments. Assuming the FIXME I point out was intentionally left for later, this LGTM.

clang/include/clang/Basic/FileManager.h

59 ↗

(On Diff #218181)

Should this return const Directory& because it's guaranteed to be valid.

clang/include/clang/Lex/DirectoryLookup.h

68–69 ↗

(On Diff #218181)

Since you're modifying this constructor it might be nice to cleanup the comment style (drop the Directory ctor - prefix). Up to you.

clang/lib/Lex/HeaderSearch.cpp

299 ↗

(On Diff #218181)

Did you mean to leave this behind for a future commit? Or did you miss fixing this?

[arphaman]

clang/lib/Lex/HeaderSearch.cpp

299 ↗

(On Diff #218181)

This is for a future commit.

cfe/trunk/include/clang/Basic/FileManager.h

 
 class FileSystemStatCache;
 
 /// Cached information about one directory (either on disk or in
 /// the virtual file system).
 class DirectoryEntry {
   friend class FileManager;
 
+  // FIXME: We should not be storing a directory entry name here.
   StringRef Name; // Name of the directory.
 
 public:
   StringRef getName() const { return Name; }
 };
 
+/// A reference to a \c DirectoryEntry  that includes the name of the directory
+/// as it was accessed by the FileManager's client.
+class DirectoryEntryRef {
+public:
+  const DirectoryEntry &getDirEntry() const { return *Entry->getValue(); }
+
+  StringRef getName() const { return Entry->getKey(); }
+
+private:
+  friend class FileManager;
+
+  DirectoryEntryRef(
+      llvm::StringMapEntry<llvm::ErrorOr<DirectoryEntry &>> *Entry)
+      : Entry(Entry) {}
+
+  const llvm::StringMapEntry<llvm::ErrorOr<DirectoryEntry &>> *Entry;
+};
+
 /// Cached information about one file (either on disk
 /// or in the virtual file system).
 ///
 /// If the 'File' member is valid, then this FileEntry has an open file
 /// descriptor for the file.
 class FileEntry {
   friend class FileManager;
 
public:
 
   /// Returns the number of unique real file entries cached by the file manager.
   size_t getNumUniqueRealFiles() const { return UniqueRealFiles.size(); }
 
   /// Lookup, cache, and verify the specified directory (real or
   /// virtual).
   ///
   /// This returns a \c std::error_code if there was an error reading the
+  /// directory. On success, returns the reference to the directory entry
+  /// together with the exact path that was used to access a file by a
+  /// particular call to getDirectoryRef.
+  ///
+  /// \param CacheFailure If true and the file does not exist, we'll cache
+  /// the failure to find this file.
+  llvm::Expected<DirectoryEntryRef> getDirectoryRef(StringRef DirName,
+                                                    bool CacheFailure = true);
+
+  /// Get a \c DirectoryEntryRef if it exists, without doing anything on error.
+  llvm::Optional<DirectoryEntryRef>
+  getOptionalDirectoryRef(StringRef DirName, bool CacheFailure = true) {
+    return llvm::expectedToOptional(getDirectoryRef(DirName, CacheFailure));
+  }
+
+  /// Lookup, cache, and verify the specified directory (real or
+  /// virtual).
+  ///
+  /// This function is deprecated and will be removed at some point in the
+  /// future, new clients should use
+  ///  \c getDirectoryRef.
+  ///
+  /// This returns a \c std::error_code if there was an error reading the
   /// directory. If there is no error, the DirectoryEntry is guaranteed to be
   /// non-NULL.
   ///
   /// \param CacheFailure If true and the file does not exist, we'll cache
   /// the failure to find this file.
   llvm::ErrorOr<const DirectoryEntry *>
   getDirectory(StringRef DirName, bool CacheFailure = true);
 

cfe/trunk/include/clang/Lex/DirectoryLookup.h

 class DirectoryLookup {
 public:
   enum LookupType_t {
     LT_NormalDir,
     LT_Framework,
     LT_HeaderMap
   };
 private:
-  union {  // This union is discriminated by isHeaderMap.
+  union DLU { // This union is discriminated by isHeaderMap.
     /// Dir - This is the actual directory that we're referring to for a normal
     /// directory or a framework.
-    const DirectoryEntry *Dir;
+    DirectoryEntryRef Dir;
 
     /// Map - This is the HeaderMap if this is a headermap lookup.
     ///
     const HeaderMap *Map;
+
+    DLU(DirectoryEntryRef Dir) : Dir(Dir) {}
+    DLU(const HeaderMap *Map) : Map(Map) {}
   } u;
 
   /// DirCharacteristic - The type of directory this is: this is an instance of
   /// SrcMgr::CharacteristicKind.
   unsigned DirCharacteristic : 2;
 
   /// LookupType - This indicates whether this DirectoryLookup object is a
   /// normal directory, a framework, or a headermap.
   unsigned LookupType : 2;
 
   /// Whether this is a header map used when building a framework.
   unsigned IsIndexHeaderMap : 1;
 
   /// Whether we've performed an exhaustive search for module maps
   /// within the subdirectories of this directory.
   unsigned SearchedAllModuleMaps : 1;
 
 public:
-  /// DirectoryLookup ctor - Note that this ctor *does not take ownership* of
-  /// 'dir'.
-  DirectoryLookup(const DirectoryEntry *dir, SrcMgr::CharacteristicKind DT,
+  /// This ctor *does not take ownership* of 'Dir'.
+  DirectoryLookup(DirectoryEntryRef Dir, SrcMgr::CharacteristicKind DT,
                   bool isFramework)
-    : DirCharacteristic(DT),
+      : u(Dir), DirCharacteristic(DT),
         LookupType(isFramework ? LT_Framework : LT_NormalDir),
-      IsIndexHeaderMap(false), SearchedAllModuleMaps(false) {
-    u.Dir = dir;
-  }
+        IsIndexHeaderMap(false), SearchedAllModuleMaps(false) {}
 
-  /// DirectoryLookup ctor - Note that this ctor *does not take ownership* of
-  /// 'map'.
-  DirectoryLookup(const HeaderMap *map, SrcMgr::CharacteristicKind DT,
+  /// This ctor *does not take ownership* of 'Map'.
+  DirectoryLookup(const HeaderMap *Map, SrcMgr::CharacteristicKind DT,
                   bool isIndexHeaderMap)
-    : DirCharacteristic(DT), LookupType(LT_HeaderMap),
-      IsIndexHeaderMap(isIndexHeaderMap), SearchedAllModuleMaps(false) {
-    u.Map = map;
-  }
+      : u(Map), DirCharacteristic(DT), LookupType(LT_HeaderMap),
+        IsIndexHeaderMap(isIndexHeaderMap), SearchedAllModuleMaps(false) {}
 
   /// getLookupType - Return the kind of directory lookup that this is: either a
   /// normal directory, a framework path, or a HeaderMap.
   LookupType_t getLookupType() const { return (LookupType_t)LookupType; }
 
   /// getName - Return the directory or filename corresponding to this lookup
   /// object.
   StringRef getName() const;
 
   /// getDir - Return the directory that this entry refers to.
   ///
   const DirectoryEntry *getDir() const {
-    return isNormalDir() ? u.Dir : nullptr;
+    return isNormalDir() ? &u.Dir.getDirEntry() : nullptr;
   }
 
   /// getFrameworkDir - Return the directory that this framework refers to.
   ///
   const DirectoryEntry *getFrameworkDir() const {
-    return isFramework() ? u.Dir : nullptr;
+    return isFramework() ? &u.Dir.getDirEntry() : nullptr;
+  }
+
+  Optional<DirectoryEntryRef> getFrameworkDirRef() const {
+    return isFramework() ? Optional<DirectoryEntryRef>(u.Dir) : None;
   }
 
   /// getHeaderMap - Return the directory that this entry refers to.
   ///
   const HeaderMap *getHeaderMap() const {
     return isHeaderMap() ? u.Map : nullptr;
   }
 

cfe/trunk/lib/Basic/FileManager.cpp

void FileManager::addAncestorsAsVirtualDirs(StringRef Path) {
   UDE->Name = NamedDirEnt.first();
   NamedDirEnt.second = *UDE.get();
   VirtualDirectoryEntries.push_back(std::move(UDE));
 
   // Recursively add the other ancestors.
   addAncestorsAsVirtualDirs(DirName);
 }
 
-/// Converts a llvm::ErrorOr<T &> to an llvm::ErrorOr<T *> by promoting
-/// the address of the inner reference to a pointer or by propagating the error.
-template <typename T>
-static llvm::ErrorOr<T *> promoteInnerReference(llvm::ErrorOr<T &> value) {
-  if (value) return &*value;
-  return value.getError();
-}
-
-llvm::ErrorOr<const DirectoryEntry *>
-FileManager::getDirectory(StringRef DirName, bool CacheFailure) {
+llvm::Expected<DirectoryEntryRef>
+FileManager::getDirectoryRef(StringRef DirName, bool CacheFailure) {
   // stat doesn't like trailing separators except for root directory.
   // At least, on Win32 MSVCRT, stat() cannot strip trailing '/'.
   // (though it can strip '\\')
   if (DirName.size() > 1 &&
       DirName != llvm::sys::path::root_path(DirName) &&
       llvm::sys::path::is_separator(DirName.back()))
     DirName = DirName.substr(0, DirName.size()-1);
 #ifdef _WIN32
   // Fixing a problem with "clang C:test.c" on Windows.
   // Stat("C:") does not recognize "C:" as a valid directory
   std::string DirNameStr;
   if (DirName.size() > 1 && DirName.back() == ':' &&
       DirName.equals_lower(llvm::sys::path::root_name(DirName))) {
     DirNameStr = DirName.str() + '.';
     DirName = DirNameStr;
   }
 #endif
 
   ++NumDirLookups;
 
   // See if there was already an entry in the map.  Note that the map
   // contains both virtual and real directories.
   auto SeenDirInsertResult =
       SeenDirEntries.insert({DirName, std::errc::no_such_file_or_directory});
-  if (!SeenDirInsertResult.second)
-    return promoteInnerReference(SeenDirInsertResult.first->second);
+  if (!SeenDirInsertResult.second) {
+    if (SeenDirInsertResult.first->second)
+      return DirectoryEntryRef(&*SeenDirInsertResult.first);
+    return llvm::errorCodeToError(SeenDirInsertResult.first->second.getError());
+  }
 
   // We've not seen this before. Fill it in.
   ++NumDirCacheMisses;
   auto &NamedDirEnt = *SeenDirInsertResult.first;
   assert(!NamedDirEnt.second && "should be newly-created");
 
   // Get the null-terminated directory name as stored as the key of the
   // SeenDirEntries map.
   StringRef InterndDirName = NamedDirEnt.first();
 
   // Check to see if the directory exists.
   llvm::vfs::Status Status;
   auto statError = getStatValue(InterndDirName, Status, false,
                                 nullptr /*directory lookup*/);
   if (statError) {
     // There's no real directory at the given path.
     if (CacheFailure)
       NamedDirEnt.second = statError;
     else
       SeenDirEntries.erase(DirName);
-    return statError;
+    return llvm::errorCodeToError(statError);
   }
 
   // It exists.  See if we have already opened a directory with the
   // same inode (this occurs on Unix-like systems when one dir is
   // symlinked to another, for example) or the same path (on
   // Windows).
   DirectoryEntry &UDE = UniqueRealDirs[Status.getUniqueID()];
 
   NamedDirEnt.second = UDE;
   if (UDE.getName().empty()) {
     // We don't have this directory yet, add it.  We use the string
     // key from the SeenDirEntries map as the string.
     UDE.Name  = InterndDirName;
   }
 
-  return &UDE;
+  return DirectoryEntryRef(&NamedDirEnt);
+}
+
+llvm::ErrorOr<const DirectoryEntry *>
+FileManager::getDirectory(StringRef DirName, bool CacheFailure) {
+  auto Result = getDirectoryRef(DirName, CacheFailure);
+  if (Result)
+    return &Result->getDirEntry();
+  return llvm::errorToErrorCode(Result.takeError());
 }
 
 llvm::ErrorOr<const FileEntry *>
 FileManager::getFile(StringRef Filename, bool openFile, bool CacheFailure) {
   auto Result = getFileRef(Filename, openFile, CacheFailure);
   if (Result)
     return &Result->getFileEntry();
   return llvm::errorToErrorCode(Result.takeError());

cfe/trunk/lib/Frontend/InitHeaderSearch.cpp

if (Group == Quoted || Group == Angled || Group == IndexHeaderMap) {
     Type = SrcMgr::C_User;
   } else if (Group == ExternCSystem) {
     Type = SrcMgr::C_ExternCSystem;
   } else {
     Type = SrcMgr::C_System;
   }
 
   // If the directory exists, add it.
-  if (auto DE = FM.getDirectory(MappedPathStr)) {
+  if (auto DE = FM.getOptionalDirectoryRef(MappedPathStr)) {
     IncludePath.push_back(
       std::make_pair(Group, DirectoryLookup(*DE, Type, isFramework)));
     return true;
   }
 
   // Check to see if this is an apple-style headermap (which are not allowed to
   // be frameworks).
   if (!isFramework) {

cfe/trunk/lib/Lex/HeaderSearch.cpp

 
 //===----------------------------------------------------------------------===//
 // File lookup within a DirectoryLookup scope
 //===----------------------------------------------------------------------===//
 
 /// getName - Return the directory or filename corresponding to this lookup
 /// object.
 StringRef DirectoryLookup::getName() const {
+  // FIXME: Use the name from \c DirectoryEntryRef.
   if (isNormalDir())
     return getDir()->getName();
   if (isFramework())
     return getFrameworkDir()->getName();
   assert(isHeaderMap() && "Unknown DirectoryLookup");
   return getHeaderMap()->getFileName();
 }
 
Optional<FileEntryRef> DirectoryLookup::DoFrameworkLookup(
   // If it is known and in some other directory, fail.
   if (CacheEntry.Directory && CacheEntry.Directory != getFrameworkDir())
     return None;
 
   // Otherwise, construct the path to this framework dir.
 
   // FrameworkName = "/System/Library/Frameworks/"
   SmallString<1024> FrameworkName;
-  FrameworkName += getFrameworkDir()->getName();
+  FrameworkName += getFrameworkDirRef()->getName();
   if (FrameworkName.empty() || FrameworkName.back() != '/')
     FrameworkName.push_back('/');
 
   // FrameworkName = "/System/Library/Frameworks/Cocoa"
   StringRef ModuleName(Filename.begin(), SlashPos);
   FrameworkName += ModuleName;
 
   // FrameworkName = "/System/Library/Frameworks/Cocoa.framework/"

cfe/trunk/lib/Lex/PPDirectives.cpp

Optional<FileEntryRef> File = LookupFile(
       &SuggestedModule, &IsMapped, &IsFrameworkFound);
   if (File)
     return File;
 
   if (Callbacks) {
     // Give the clients a chance to recover.
     SmallString<128> RecoveryPath;
     if (Callbacks->FileNotFound(Filename, RecoveryPath)) {
-      if (auto DE = FileMgr.getDirectory(RecoveryPath)) {
+      if (auto DE = FileMgr.getOptionalDirectoryRef(RecoveryPath)) {
         // Add the recovery path to the list of search paths.
         DirectoryLookup DL(*DE, SrcMgr::C_User, false);
         HeaderInfo.AddSearchPath(DL, isAngled);
 
         // Try the lookup again, skipping the cache.
         Optional<FileEntryRef> File = LookupFile(
             FilenameLoc,
             LangOpts.MSVCCompat ? NormalizedPath.c_str() : Filename, isAngled,

cfe/trunk/test/ClangScanDeps/Inputs/subframework_header_dir_symlink_cdb.json

 [
 {
   "directory": "DIR",
   "command": "clang -E DIR/subframework_header_dir_symlink.m -D EMPTY -iframework Inputs/frameworks",
   "file": "DIR/subframework_header_dir_symlink.m"
 },
 {
   "directory": "DIR",
-  "command": "clang -E DIR/subframework_header_dir_symlink2.m -FInputs/frameworks_symlink -iframework Inputs/frameworks",
+  "command": "clang -E DIR/subframework_header_dir_symlink2.m -FInputs/frameworks -iframework Inputs/frameworks_symlink",
   "file": "DIR/subframework_header_dir_symlink2.m"
 }
 ]

cfe/trunk/test/ClangScanDeps/subframework_header_dir_symlink.m

 // REQUIRES: shell
 // RUN: rm -rf %t.dir
 // RUN: rm -rf %t.cdb
 // RUN: mkdir -p %t.dir
 // RUN: cp %s %t.dir/subframework_header_dir_symlink.m
 // RUN: cp %s %t.dir/subframework_header_dir_symlink2.m
 // RUN: mkdir %t.dir/Inputs
 // RUN: cp -R %S/Inputs/frameworks %t.dir/Inputs/frameworks
 // RUN: ln -s %t.dir/Inputs/frameworks %t.dir/Inputs/frameworks_symlink
 // RUN: sed -e "s|DIR|%/t.dir|g" %S/Inputs/subframework_header_dir_symlink_cdb.json > %t.cdb
 // RUN: clang-scan-deps -compilation-database %t.cdb -j 1  -reuse-filemanager=0 | \
 // RUN:   FileCheck %s
-// FIXME: Make this work when the filemanager is reused:
 // RUN: clang-scan-deps -compilation-database %t.cdb -j 1 -reuse-filemanager=1 | \
-// RUN:   not FileCheck %s
+// RUN:   FileCheck %s
 
 #ifndef EMPTY
 #include "Framework/Framework.h"
 #endif
 
 // CHECK: clang-scan-deps dependency
 // CHECK-NEXT: subframework_header_dir_symlink.m
 // CHECK: clang-scan-deps dependency
 // CHECK-NEXT: subframework_header_dir_symlink.m
 // CHECK-NEXT: Inputs{{/|\\}}frameworks_symlink{{/|\\}}Framework.framework{{/|\\}}Headers{{/|\\}}Framework.h

cfe/trunk/unittests/Lex/HeaderSearchTest.cpp

HeaderSearchTest()
                LangOpts, Target.get()) {
     TargetOpts->Triple = "x86_64-apple-darwin11.1.0";
     Target = TargetInfo::CreateTargetInfo(Diags, TargetOpts);
   }
 
   void addSearchDir(llvm::StringRef Dir) {
     VFS->addFile(Dir, 0, llvm::MemoryBuffer::getMemBuffer(""), /*User=*/None,
                  /*Group=*/None, llvm::sys::fs::file_type::directory_file);
-    auto DE = FileMgr.getDirectory(Dir);
+    auto DE = FileMgr.getOptionalDirectoryRef(Dir);
     assert(DE);
     auto DL = DirectoryLookup(*DE, SrcMgr::C_User, /*isFramework=*/false);
     Search.AddSearchPath(DL, /*isAngled=*/false);
   }
 
   IntrusiveRefCntPtr<llvm::vfs::InMemoryFileSystem> VFS;
   FileSystemOptions FileMgrOpts;
   FileManager FileMgr;

cfe/trunk/unittests/Lex/PPCallbacksTest.cpp

protected:
   void AddFakeHeader(HeaderSearch &HeaderInfo, const char *HeaderPath,
                      bool IsSystemHeader) {
     // Tell FileMgr about header.
     InMemoryFileSystem->addFile(HeaderPath, 0,
                                 llvm::MemoryBuffer::getMemBuffer("\n"));
 
     // Add header's parent path to search path.
     StringRef SearchPath = llvm::sys::path::parent_path(HeaderPath);
-    auto DE = FileMgr.getDirectory(SearchPath);
+    auto DE = FileMgr.getOptionalDirectoryRef(SearchPath);
     DirectoryLookup DL(*DE, SrcMgr::C_User, false);
     HeaderInfo.AddSearchPath(DL, IsSystemHeader);
   }
 
   // Get the raw source string of the range.
   StringRef GetSourceString(CharSourceRange Range) {
     const char* B = SourceMgr.getCharacterData(Range.getBegin());
     const char* E = SourceMgr.getCharacterData(Range.getEnd());