[clang] NFCI: Use FileEntryRef in FileManager's UID mapping

[jansvoboda11]

Summary

Per the FileManager documentation, VirtualFileEntries is a subset of SeenFileEntries. This means we only have to iterate over the latter to create complete UID mapping. This allows us to map UID to FileEntryRef instead of FileEntry.

[benlangmuir]

Iterating over SeenFileEntries and skipping VirtualFileEntries looks good to me.

I'm not sure about changing this to FileEntryRef though. The way this API works you only get one per unique file, which is well suited to FileEntry * which has the same uniquing behaviour. In this case you're going to get a FileEntryRef, but *which* ref you get is non-deterministic if there were multiple refs for the same file (depends on hash table iteration order). Also, it will never give you a vfs mapped path since it's skipping those (V.dyn_cast<FileEntry *>()).

I think if we want to change this to FileEntryRef it needs to be deterministic which ref you get.

[rmaz]

In D142780#4087236, @benlangmuir wrote:

I think if we want to change this to FileEntryRef it needs to be deterministic which ref you get.

I think this might be the root of the problem we are seeing: depending on build configuration sometimes our build inputs are hard links that in the case of identical inputs point to the same inode. In that case we are seeing non-deterministic header paths serialized in pcm files. IIUC the header files are serialized based in their unique ID, so it wouldn't be possible to handle this case, is this right?

[benlangmuir]

In D142780#4087250, @rmaz wrote:

In that case we are seeing non-deterministic header paths serialized in pcm files. IIUC the header files are serialized based in their unique ID, so it wouldn't be possible to handle this case, is this right?

If compiling a single pcm accesses multiple hard links with the same UID, then it would not be possible to use the set of UIDs to get the "right path". At best we could make it get a deterministic path -- e.g. if we tracked the order of access.

If compiling a single pcm accesses only one hard link but it's an implicit module build and the overall compiler invocation accesses another one with the same UID, you are currently in the same situation as above, though in theory you could avoid it by not sharing the FileManager across implicit module builds. In clang-scan-deps we don't share the FileManager and we get away with it because we have a separate filesystem caching layer. In a normal implicit modules build I don't know how much this would cost.

If you're on a platform that supports fcntl with F_GETPATH such as Darwin, another possible source of non-determinism is that the underlying OS may return a non-deterministic RealPath from openFileForRead when there are multiple hard links.

[jansvoboda11]

Sounds like the easies way out is serializing all FileEntryRef objects we know in deterministic order.

[rmaz]

In D142780#4087270, @benlangmuir wrote:

If compiling a single pcm accesses multiple hard links with the same UID, then it would not be possible to use the set of UIDs to get the "right path". At best we could make it get a deterministic path -- e.g. if we tracked the order of access.

With you so far. Is there a reason we need to use the UIDs in this case? Would it be possible to refactor GetUniqueIDMapping to instead populate an array with all the FileEntryRefs that had been seen and serialize that instead?

[rmaz]

In D142780#4087273, @jansvoboda11 wrote:

Sounds like the easies way out is serializing all FileEntryRef objects we know in deterministic order.

How about something like https://reviews.llvm.org/D143414 ?

clang/include/clang/Basic/FileManager.h

public:
   bool FixupRelativePath(SmallVectorImpl<char> &path) const;
 
   /// Makes \c Path absolute taking into account FileSystemOptions and the
   /// working directory option.
   /// \returns true if \c Path changed to absolute.
   bool makeAbsolutePath(SmallVectorImpl<char> &Path) const;
 
   /// Produce an array mapping from the unique IDs assigned to each
-  /// file to the corresponding FileEntry pointer.
-  void GetUniqueIDMapping(
-                    SmallVectorImpl<const FileEntry *> &UIDToFiles) const;
+  /// file to the corresponding FileEntryRef.
+  void
+  getUniqueIDMapping(SmallVectorImpl<OptionalFileEntryRef> &UIDToFiles) const;
 
   /// Retrieve the canonical name for a given directory.
   ///
   /// This is a very expensive operation, despite its results being cached,
   /// and should only be used when the physical layout of the file system is
   /// required, which is (almost) never.
   StringRef getCanonicalName(const DirectoryEntry *Dir);
 

clang/lib/Basic/FileManager.cpp

FileManager::getNoncachedStatValue(StringRef Path,
 
   llvm::ErrorOr<llvm::vfs::Status> S = FS->status(FilePath.c_str());
   if (!S)
     return S.getError();
   Result = *S;
   return std::error_code();
 }
 
-void FileManager::GetUniqueIDMapping(
-    SmallVectorImpl<const FileEntry *> &UIDToFiles) const {
+void FileManager::getUniqueIDMapping(
+    SmallVectorImpl<OptionalFileEntryRef> &UIDToFiles) const {
   UIDToFiles.clear();
   UIDToFiles.resize(NextFileUID);
 
-  // Map file entries
-  for (llvm::StringMap<llvm::ErrorOr<FileEntryRef::MapValue>,
-                       llvm::BumpPtrAllocator>::const_iterator
-           FE = SeenFileEntries.begin(),
-           FEEnd = SeenFileEntries.end();
-       FE != FEEnd; ++FE)
-    if (llvm::ErrorOr<FileEntryRef::MapValue> Entry = FE->getValue()) {
-      if (const auto *FE = Entry->V.dyn_cast<FileEntry *>())
-        UIDToFiles[FE->getUID()] = FE;
-    }
-
-  // Map virtual file entries
-  for (const auto &VFE : VirtualFileEntries)
-    UIDToFiles[VFE->getUID()] = VFE;
+  for (const auto &Entry : SeenFileEntries)
+    if (llvm::ErrorOr<FileEntryRef::MapValue> Value = Entry.getValue())
+      if (const auto *FE = Value->V.dyn_cast<FileEntry *>())
+        UIDToFiles[FE->getUID()] = FileEntryRef(Entry);
 }
 
 StringRef FileManager::getCanonicalName(const DirectoryEntry *Dir) {
   llvm::DenseMap<const void *, llvm::StringRef>::iterator Known
     = CanonicalNames.find(Dir);
   if (Known != CanonicalNames.end())
     return Known->second;
 

clang/lib/Serialization/ASTWriter.cpp

 std::set<const FileEntry *> GetAffectingModuleMaps(const Preprocessor &PP,
                                                    Module *RootModule) {
   std::set<const FileEntry *> ModuleMaps{};
   std::set<const Module *> ProcessedModules;
   SmallVector<const Module *> ModulesToProcess{RootModule};
 
   const HeaderSearch &HS = PP.getHeaderSearchInfo();
 
-  SmallVector<const FileEntry *, 16> FilesByUID;
-  HS.getFileMgr().GetUniqueIDMapping(FilesByUID);
+  SmallVector<OptionalFileEntryRef, 16> FilesByUID;
+  HS.getFileMgr().getUniqueIDMapping(FilesByUID);
 
   if (FilesByUID.size() > HS.header_file_size())
     FilesByUID.resize(HS.header_file_size());
 
   for (unsigned UID = 0, LastUID = FilesByUID.size(); UID != LastUID; ++UID) {
-    const FileEntry *File = FilesByUID[UID];
+    OptionalFileEntryRef File = FilesByUID[UID];
     if (!File)
       continue;
 
     const HeaderFileInfo *HFI =
-        HS.getExistingFileInfo(File, /*WantExternal*/ false);
+        HS.getExistingFileInfo(*File, /*WantExternal=*/false);
     if (!HFI || (HFI->isModuleHeader && !HFI->isCompilingModuleHeader))
       continue;
 
-    for (const auto &KH : HS.findAllModulesForHeader(File)) {
+    for (const auto &KH : HS.findAllModulesForHeader(*File)) {
       if (!KH.getModule())
         continue;
       ModulesToProcess.push_back(KH.getModule());
     }
   }
 
   const ModuleMap &MM = HS.getModuleMap();
   SourceManager &SourceMgr = PP.getSourceManager();
while (!Worklist.empty()) {
         Generator.insert(Key, Data, GeneratorTrait);
         ++NumHeaderSearchEntries;
       }
 
       Worklist.append(M->submodule_begin(), M->submodule_end());
     }
   }
 
-  SmallVector<const FileEntry *, 16> FilesByUID;
-  HS.getFileMgr().GetUniqueIDMapping(FilesByUID);
+  SmallVector<OptionalFileEntryRef, 16> FilesByUID;
+  HS.getFileMgr().getUniqueIDMapping(FilesByUID);
 
   if (FilesByUID.size() > HS.header_file_size())
     FilesByUID.resize(HS.header_file_size());
 
   for (unsigned UID = 0, LastUID = FilesByUID.size(); UID != LastUID; ++UID) {
-    const FileEntry *File = FilesByUID[UID];
+    OptionalFileEntryRef File = FilesByUID[UID];
     if (!File)
       continue;
 
     // Get the file info. This will load info from the external source if
     // necessary. Skip emitting this file if we have no information on it
     // as a header file (in which case HFI will be null) or if it hasn't
     // changed since it was loaded. Also skip it if it's for a modular header
     // from a different module; in that case, we rely on the module(s)
     // containing the header to provide this information.
     const HeaderFileInfo *HFI =
-        HS.getExistingFileInfo(File, /*WantExternal*/!Chain);
+        HS.getExistingFileInfo(*File, /*WantExternal=*/!Chain);
     if (!HFI || (HFI->isModuleHeader && !HFI->isCompilingModuleHeader))
       continue;
 
     // Massage the file path into an appropriate form.
     StringRef Filename = File->getName();
     SmallString<128> FilenameTmp(Filename);
     if (PreparePathForOutput(FilenameTmp)) {
       // If we performed any translation on the file name at all, we need to
       // save this string, since the generator will refer to it later.
       Filename = StringRef(strdup(FilenameTmp.c_str()));
       SavedStrings.push_back(Filename.data());
     }
 
     HeaderFileInfoTrait::key_type Key = {
-      Filename, File->getSize(), getTimestampForOutput(File)
+      Filename, File->getSize(), getTimestampForOutput(*File)
     };
     HeaderFileInfoTrait::data_type Data = {
-      *HFI, HS.getModuleMap().findResolvedModulesForHeader(File), {}
+      *HFI, HS.getModuleMap().findResolvedModulesForHeader(*File), {}
     };
     Generator.insert(Key, Data, GeneratorTrait);
     ++NumHeaderSearchEntries;
   }
 
   // Create the on-disk hash table in a buffer.
   SmallString<4096> TableData;
   uint32_t BucketOffset;