[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 ↗

(On Diff #57222)

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 ↗

(On Diff #57222)

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 ↗

(On Diff #57222)

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 ↗

(On Diff #57222)

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 ↗

(On Diff #57253)

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 ↗

(On Diff #57253)

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

cfe/trunk/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 (llvm::StringMap<PreambleFileHash>::iterator
              F = FilesInPreamble.begin(), FEnd = FilesInPreamble.end();
            !AnyFileChanged && F != FEnd; 
            ++F) {
-        llvm::StringMap<PreambleFileHash>::iterator Overridden
-          = OverriddenFiles.find(F->first());
+        vfs::Status Status;
+        if (FileMgr->getNoncachedStatValue(F->first(), Status)) {
+          // If we can't stat the file, assume that something horrible happened.
+          AnyFileChanged = true;
+          break;
+        }
+
+        std::map<llvm::sys::fs::UniqueID, PreambleFileHash>::iterator Overridden
+          = OverriddenFiles.find(Status.getUniqueID());
         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)
             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 we can't stat the file, assume that something horrible happened.
-          AnyFileChanged = true;
-        } else if (Status.getSize() != uint64_t(F->second.Size) ||
-                   Status.getLastModificationTime().toEpochTime() !=
-                       uint64_t(F->second.ModTime))
+        if (Status.getSize() != uint64_t(F->second.Size) ||
           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.