[PCH] Fixed overridden files always invalidating preamble even when unchanged

[cameron314]

Summary

Remapped files would always cause the preamble's PCH to be invalidated (even if they hadn't changed) because the file manager was replacing the remapped virtual entry (no modified time) with a real file entry (which has a modified time) when an include directive was processed. This happens when the include directive results in a slightly different path for the same file (e.g. different slash direction, which happens very often on Windows).

Note: This was initially part of D20137 but I had incorrectly applied my own patch, so the IsVirtual = true line was in the wrong spot and served no purpose (and was subsequently removed from the patch). Now it actually does something!

In order to test this, I also extended the support for using custom virtual filesystems not just for basic parses, but also PCH-enabled parses and reparses.

[bruno]

Hi Cameron,

Can you add a testcase?

[cameron314]

I'm not sure how to test this (originally I found this bug by stepping through with a debugger) -- is there a way to determine if an ASTUnit used a PCH for the preamble or not? I'd call the getMainBufferWithPrecompiledPreamble method manually but it's private.

[bruno]

You can probably find a way to test this by taking a look at unittests/Basic/VirtualFileSystemTest.cpp

[rsmith]

lib/Basic/FileManager.cpp

389

Rather than adding this IsVirtual flag, could you just set UFE->IsValid to true here? It looks like a simple oversight that this code fails to set the IsValid flag properly.

[cameron314]

Thanks @bruno, I'll have a look at using a VFS for the test.

lib/Basic/FileManager.cpp

389

I could, but I didn't want to accidentally break something else that depends on virtual files not being valid. That's the type of change that can easily introduce a subtle bug not caught by the tests.

Semantically, is a virtual file always valid?

[cameron314]

It took some modifications to the ASTUnit to support a virtual file system with a PCH parse/reparse (preliminary VFS support had already been added in rL249410 but it did not support initial parses using PCHs, nor reparses), but I was finally able to write a test that checks that the reparse actually uses the PCH with my fix, and rejects the PCH (rereading everything and failing the test) without it.

[cameron314]

This is a fairly important bug for anyone hosting clang as a library (e.g. IDEs).
Can someone have a look at this patch when they have a free moment?

[rsmith]

lib/Basic/FileManager.cpp

389

Yes. The IsValid flag is just supposed to mean that this file has actually been added to the UniqueRealFiles map rather than having been default-constructed by operator[].

[cameron314]

lib/Basic/FileManager.cpp

389

Excellent then, I'll get rid of IsVirtual and use IsValid in place. This will condense things down to a one-line change plus a large diff for the test ^^

[cameron314]

Here's the final fix (it's the line in FileManager.cpp, plus a test).

[cameron314]

Ping? :-)

[cameron314]

Anyone have a few minutes to look at this?

[cameron314]

Anyone have time to check this out this week?
It's a one-line fix, includes a test, and is for a fairly important bug :-)

[cameron314]

This patch is obsolete. While waiting over a year for a review, somebody else came across the same fix (for a different manifestation of the same bug) in D27810 and managed to get it through. I think my test case and supporting changes are still useful, however, so I've submitted that separately in D37474.

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; }

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;
FileManager::getVirtualFile(StringRef Filename, off_t Size,
   }
 
   UFE->Name    = InterndFileName;
   UFE->Size    = Size;
   UFE->ModTime = ModificationTime;
   UFE->Dir     = DirInfo;
   UFE->UID     = NextFileUID++;
   UFE->File.reset();
+  UFE->IsVirtual = true;
   return UFE;
 }
 
 bool FileManager::FixupRelativePath(SmallVectorImpl<char> &path) const {
   StringRef pathRef(path.data(), path.size());
 
   if (FileSystemOpts.WorkingDir.empty() 
       || llvm::sys::path::is_absolute(pathRef))