[PCH] Fixed bugs with preamble invalidation when files change (on Windows)

[cameron314]

Summary

There was a bug related to remapped files, that is not strictly specific to Windows but happens more often there:
When remapped files were changed, they would not always cause the preamble's PCH to be invalidated, because the remapped path didn't necessarily match the include path (e.g. slash direction). I fixed this by moving to a llvm::sys::fs::UniqueID-based map instead of using strings for paths and hoping they match.

[cameron314]

Updated patch to include later fixes I had made after the initial change.

[rsmith]

lib/Basic/FileManager.cpp

304–307

It looks like this is unreachable: IsVirtual is only ever true when IsValid is also true, and we don't reach this line if UFE.isValid(). As this is the only consumer of FileEntry::IsValid, it looks like it does nothing. Am I missing something?

lib/Frontend/ASTUnit.cpp

1402–1406

If there are multiple file names with the same inode, RB.first is an arbitrarily-chosen one of those names, and stat'ing it isn't sufficient to check that none of the other names for the file have changed. Maybe we need to store a list of filenames used for each UniqueID?

[cameron314]

lib/Basic/FileManager.cpp

304–307

Hmm, interesting, good catch. Back when I made this change UFE.IsValid was not set to true in the fall-through after this if, which is why I had to introduce IsVirtual in the first place.

But looking at the history on this file, it seems it was set to true since the very beginning. I think we'd removed it in our local fork at one point to work around a different bug, but it's back to normal in ours as well. So I guess this part of the patch is unnecessary now. Yay! I'll remove it, it wasn't very pretty anyway.

lib/Frontend/ASTUnit.cpp

1402–1406

I'm not sure I understand -- if the file changed on disk, why would it matter which name we're accessing it by?

[cameron314]

Removed workaround for case that can no longer happen.

[rsmith]

lib/Frontend/ASTUnit.cpp

1402–1406

Suppose file names A and B refer to file (inode) X, and the map contains X -> (A, hash). If B is deleted (and maybe recreated pointing at a new inode), this approach won't detect that anything has changed.

[cameron314]

lib/Frontend/ASTUnit.cpp

1402–1406

Ah, I understand. The problem isn't really with the overridden file map, which still has to be by inode in order for lookups to find the right file regardless of name, but with the FilesInPreamble map, which needs to stay as a list of independent filenames in case the inodes associated to those filenames changes in between parses.

I'll make the change, thanks!

[cameron314]

This version of the patch takes into account potential changes between filenames and their unique IDs between parses.

[rsmith]

LGTM

include/clang/Basic/FileManager.h

class FileEntry {
   off_t Size;                 // File size in bytes.
   time_t ModTime;             // Modification time of file.
   const DirectoryEntry *Dir;  // Directory file lives in.
   unsigned UID;               // A unique (small) ID for the file.
   llvm::sys::fs::UniqueID UniqueID;
   bool IsNamedPipe;
   bool InPCH;
   bool IsValid;               // Is this \c FileEntry initialized and valid?
+  bool IsVirtual;             // Is this \c FileEntry a virtual file?
 
   /// \brief The open file, if it is owned by the \p FileEntry.
   mutable std::unique_ptr<vfs::File> File;
   friend class FileManager;
 
   void operator=(const FileEntry &) = delete;
 
 public:
   FileEntry()
-      : UniqueID(0, 0), IsNamedPipe(false), InPCH(false), IsValid(false)
+      : UniqueID(0, 0), IsNamedPipe(false), InPCH(false), IsValid(false),
+        IsVirtual(false)
   {}
 
   // FIXME: this is here to allow putting FileEntry in std::map.  Once we have
   // emplace, we shouldn't need a copy constructor anymore.
   /// Intentionally does not copy fields that are not set in an uninitialized
   /// \c FileEntry.
   FileEntry(const FileEntry &FE) : UniqueID(FE.UniqueID),
-      IsNamedPipe(FE.IsNamedPipe), InPCH(FE.InPCH), IsValid(FE.IsValid) {
+      IsNamedPipe(FE.IsNamedPipe), InPCH(FE.InPCH), IsValid(FE.IsValid),
+      IsVirtual(FE.IsVirtual) {
     assert(!isValid() && "Cannot copy an initialized FileEntry");
   }
 
   const char *getName() const { return Name; }
   bool isValid() const { return IsValid; }
+  bool isVirtual() const { return IsVirtual; }
   off_t getSize() const { return Size; }
   unsigned getUID() const { return UID; }
   const llvm::sys::fs::UniqueID &getUniqueID() const { return UniqueID; }
   bool isInPCH() const { return InPCH; }
   time_t getModificationTime() const { return ModTime; }
 
   /// \brief Return the directory the file lives in.
   const DirectoryEntry *getDir() const { return Dir; }

include/clang/Frontend/ASTUnit.h

 #include "clang/Lex/PreprocessingRecord.h"
 #include "clang/Sema/CodeCompleteConsumer.h"
 #include "clang/Serialization/ASTBitCodes.h"
 #include "llvm/ADT/IntrusiveRefCntPtr.h"
 #include "llvm/ADT/SmallVector.h"
 #include "llvm/ADT/StringMap.h"
 #include "llvm/Support/MD5.h"
 #include "llvm/Support/Path.h"
+#include "llvm/Support/FileSystem.h"
 #include <cassert>
 #include <map>
 #include <memory>
 #include <string>
 #include <sys/types.h>
 #include <utility>
 #include <vector>
 
private:
   PreambleData Preamble;
 
   /// \brief Whether the preamble ends at the start of a new line.
   /// 
   /// Used to inform the lexer as to whether it's starting at the beginning of
   /// a line after skipping the preamble.
   bool PreambleEndsAtStartOfLine;
 
+  typedef std::map<llvm::sys::fs::UniqueID,
+      std::pair<StringRef, PreambleFileHash>> FilesInPreambleMap;
+
   /// \brief Keeps track of the files that were used when computing the 
   /// preamble, with both their buffer size and their modification time.
   ///
   /// If any of the files have changed from one compile to the next,
   /// the preamble must be thrown away.
-  llvm::StringMap<PreambleFileHash> FilesInPreamble;
+  FilesInPreambleMap FilesInPreamble;
 
   /// \brief When non-NULL, this is the buffer used to store the contents of
   /// the main file when it has been padded for use with the precompiled
   /// preamble.
   std::unique_ptr<llvm::MemoryBuffer> SavedMainFileBuffer;
 
   /// \brief When non-NULL, this is the buffer used to store the
   /// contents of the preamble when it has been padded to build the

lib/Basic/FileManager.cpp

if (UFE.isValid()) { // Already have an entry with this inode, return it.
     // to switch towards a design where we return a FileName object that
     // encapsulates both the name by which the file was accessed and the
     // corresponding FileEntry.
     UFE.Name = InterndFileName;
 
     return &UFE;
   }
 
+  if (UFE.isVirtual()) {
+    UFE.Name = InterndFileName;
+    return &UFE;
+  }
+
   // Otherwise, we don't have this file yet, add it.
   UFE.Name    = InterndFileName;
   UFE.Size = Data.Size;
   UFE.ModTime = Data.ModTime;
   UFE.Dir     = DirInfo;
   UFE.UID     = NextFileUID++;
   UFE.UniqueID = Data.UniqueID;
   UFE.IsNamedPipe = Data.IsNamedPipe;
   UFE.InPCH = Data.InPCH;
   UFE.File = std::move(F);
   UFE.IsValid = true;
+  UFE.IsVirtual = true;
   return &UFE;
 }
 
 const FileEntry *
 FileManager::getVirtualFile(StringRef Filename, off_t Size,
                             time_t ModificationTime) {
   ++NumFileLookups;
 

lib/Frontend/ASTUnit.cpp

if (Preamble.size() == NewPreamble.Size &&
       // The preamble has not changed. We may be able to re-use the precompiled
       // preamble.
 
       // Check that none of the files used by the preamble have changed.
       bool AnyFileChanged = false;
           
       // First, make a record of those files that have been overridden via
       // remapping or unsaved_files.
-      llvm::StringMap<PreambleFileHash> OverriddenFiles;
+      std::map<llvm::sys::fs::UniqueID, PreambleFileHash> OverriddenFiles;
       for (const auto &R : PreprocessorOpts.RemappedFiles) {
         if (AnyFileChanged)
           break;
 
         vfs::Status Status;
         if (FileMgr->getNoncachedStatValue(R.second, Status)) {
           // If we can't stat the file we're remapping to, assume that something
           // horrible happened.
           AnyFileChanged = true;
           break;
         }
 
-        OverriddenFiles[R.first] = PreambleFileHash::createForFile(
+        OverriddenFiles[Status.getUniqueID()] = PreambleFileHash::createForFile(
             Status.getSize(), Status.getLastModificationTime().toEpochTime());
       }
 
       for (const auto &RB : PreprocessorOpts.RemappedFileBuffers) {
         if (AnyFileChanged)
           break;
-        OverriddenFiles[RB.first] =
+
+        vfs::Status Status;
+        if (FileMgr->getNoncachedStatValue(RB.first, Status)) {
+          AnyFileChanged = true;
+          break;
+        }
+
+        OverriddenFiles[Status.getUniqueID()] =
             PreambleFileHash::createForMemoryBuffer(RB.second);
       }
        
       // Check whether anything has changed.
-      for (llvm::StringMap<PreambleFileHash>::iterator 
+      for (FilesInPreambleMap::iterator
              F = FilesInPreamble.begin(), FEnd = FilesInPreamble.end();
            !AnyFileChanged && F != FEnd; 
            ++F) {
-        llvm::StringMap<PreambleFileHash>::iterator Overridden
-          = OverriddenFiles.find(F->first());
+        std::map<llvm::sys::fs::UniqueID, PreambleFileHash>::iterator Overridden
+          = OverriddenFiles.find(F->first);
         if (Overridden != OverriddenFiles.end()) {
           // This file was remapped; check whether the newly-mapped file 
           // matches up with the previous mapping.
-          if (Overridden->second != F->second)
+          if (Overridden->second != F->second.second)
             AnyFileChanged = true;
           continue;
         }
         
         // The file was not remapped; check whether it has changed on disk.
         vfs::Status Status;
-        if (FileMgr->getNoncachedStatValue(F->first(), Status)) {
+        if (FileMgr->getNoncachedStatValue(F->second.first, Status)) {
           // If we can't stat the file, assume that something horrible happened.
           AnyFileChanged = true;
-        } else if (Status.getSize() != uint64_t(F->second.Size) ||
+        } else if (Status.getSize() != uint64_t(F->second.second.Size) ||
                    Status.getLastModificationTime().toEpochTime() !=
-                       uint64_t(F->second.ModTime))
+                       uint64_t(F->second.second.ModTime))
           AnyFileChanged = true;
       }
           
       if (!AnyFileChanged) {
         // Okay! We can re-use the precompiled preamble.
 
         // Set the state of the diagnostic object to mimic its state
         // after parsing the preamble.
ASTUnit::getMainBufferWithPrecompiledPreamble(
   // so we can verify whether they have changed or not.
   FilesInPreamble.clear();
   SourceManager &SourceMgr = Clang->getSourceManager();
   for (auto &Filename : PreambleDepCollector->getDependencies()) {
     const FileEntry *File = Clang->getFileManager().getFile(Filename);
     if (!File || File == SourceMgr.getFileEntryForID(SourceMgr.getMainFileID()))
       continue;
     if (time_t ModTime = File->getModificationTime()) {
-      FilesInPreamble[File->getName()] = PreambleFileHash::createForFile(
-          File->getSize(), ModTime);
+      FilesInPreamble[File->getUniqueID()] = std::make_pair(
+        StringRef(File->getName()),
+        PreambleFileHash::createForFile(File->getSize(), ModTime));
     } else {
       llvm::MemoryBuffer *Buffer = SourceMgr.getMemoryBufferForFile(File);
-      FilesInPreamble[File->getName()] =
-          PreambleFileHash::createForMemoryBuffer(Buffer);
+      FilesInPreamble[File->getUniqueID()] = std::make_pair(
+        StringRef(File->getName()),
+        PreambleFileHash::createForMemoryBuffer(Buffer));
     }
   }
 
   PreambleRebuildCounter = 1;
   PreprocessorOpts.RemappedFileBuffers.pop_back();
 
   // If the hash of top-level entities differs from the hash of the top-level
   // entities the last time we rebuilt the preamble, clear out the completion