Reuse preamble even if an unsaved file does not exist

[nik]

Summary

When a preamble is created an unsaved file not existing on disk is
already part of PrecompiledPreamble::FilesInPreamble. However, when
checking whether the preamble can be re-used, a failed stat of such an
unsaved file invalidated the preamble, which led to pointless and time
consuming preamble regenerations on subsequent reparses.

Do not require anymore that unsaved files should exist on disk.

This avoids costly preamble invalidations depending on timing issues for
the cases where the file on disk might be removed just to be regenerated
a bit later.

It also allows an IDE to provide in-memory files that might not exist on
disk, e.g. because the build system hasn't generated those yet.

[ilya-biryukov]

This looks useful. Could we avoid creating the OverlayFileSystem, though?

include/clang/Frontend/PrecompiledPreamble.h

109

Having this time stamp in the interface of PrecompiledPreamble doesn't really makes sense.

I propose we remove this method and test in a different way instead.

For example, we could add a counter to ASTUnit that increases when preamble is built and check this counter.
Or we could add a unit-test that uses PrecompiledPreamble directly.

lib/Frontend/PrecompiledPreamble.cpp

437

Can we do the same thing without creating an additional OverlayFileSystem?

If we add a separate map to check for those:

llvm::StringMap<PreambleFileHash> OverriddenFilesWithoutFS.
// Populate the map with paths not existing in VFS.

for (const auto &F : FilesInPreamble) {
    vfs::Status Status;
    if (!moveOnNoError(OFS.status(F.first()), Status)) {
      // If we can't stat the file, check whether it was overriden
      auto It = OverriddenFilesWithoutFS[F.first()];
      if (It == OverriddenFilesWithoutFS.end())
        return false;  
      //.....
    }
//......

}

444

assert macro is a no-op when NDEBUG is defined.
One must never put an operation with side-effects inside assert.

[cameron314]

It's been a while since I was in this code, but I seem to recall the file needed to exist on disk in order to uniquely identify it (via inode). Does this break the up-to-date check?

[ilya-biryukov]

In D41005#949550, @cameron314 wrote:

It's been a while since I was in this code, but I seem to recall the file needed to exist on disk in order to uniquely identify it (via inode). Does this break the up-to-date check?

When the file is missing from the disk, but was remapped, preamble can not be reused. (Because we're always looking at uniqueids).
If the remapped file did not exist on disk originally when building the preamble, we should allow the preamble to be reused if it's still remapped, but not created on disk.

[nik]

include/clang/Frontend/PrecompiledPreamble.h

109

For example, we could add a counter to ASTUnit that increases when preamble is built and check this counter.

OK, I've followed this proposal because there is already test infrastructure in PCHPreambleTest that I can easily reuse.

lib/Frontend/PrecompiledPreamble.cpp

437

Can we do the same thing without creating an additional OverlayFileSystem?

Hmm, I thought I had a good reason for this one. I don't remember it and the test still passes, so I did it without it now.

Is there any real advantage in first trying the stat, then checking OverriddenFilesWithoutFS? Since I don't see any, I've changed order because the stat can then be avoided; also, it removes some nesting.

444

Huch, forgot to remove it on cleanup. Anyway, it's obsolete now.

[nik]

Addressed Ilya's comments.

[nik]

include/clang/Frontend/ASTUnit.h

196

Any better name for this one? Otherwise I would suggest renaming PreambleRebuildCounter to PreambleRebuildCountdownCounter to make them more distinct.

[ilya-biryukov]

include/clang/Frontend/ASTUnit.h

196

+1, names PreambleRebuildCounter and PreambleCounter are too similar.

lib/Frontend/PrecompiledPreamble.cpp

437

I don't see any advantage.
I used to think it had something to do with overriden symlinks, but after thinking more about it I'm not even sure what should the semantics of those be. And I don't think the current code handles that (we have neither tests nor comments suggesting that this use-case was considered in the first place).

This possibly handles case-insensitive filesystems, like NTFS on Windows. But I'm not sure if that was the original intention of this code.

457

Will anything fail if we remove the map from UniqueID to hashes of overriden files and the corresponding checks?

We should either document why having UniqueID-based checks is important or remove the code doing those checks.

[nik]

include/clang/Frontend/ASTUnit.h

196

OK, will address in the next patch set / diff.

lib/Frontend/PrecompiledPreamble.cpp

457

Will anything fail if we remove the map from UniqueID to hashes of overriden files and the corresponding checks?

Hmm, I would need to remove/replace it and run the tests.

I see these reasons for UniqueID:

• It's already there :)
• Normalization of file paths is not necessary.
• It potentially can deal with hard links, though I'm not sure whether this is real world use case at all.

We should either document why having UniqueID-based checks is important or remove the code doing those checks.

Agree.

[nik]

lib/Frontend/PrecompiledPreamble.cpp

457

Will anything fail if we remove the map from UniqueID to hashes of overriden files and the corresponding checks?

OK, I've applied the following patch (on this patch set/revision here) applied and run check-clang - no failures here.

From 8f755128d1e167193c8f6de1456aec01d14307f6 Mon Sep 17 00:00:00 2001
From: Nikolai Kosjar <[e-mail]>
Date: Tue, 2 Jan 2018 15:14:07 +0100
Subject: [PATCH] Use file path instead of uniqueID

---
 lib/Frontend/PrecompiledPreamble.cpp | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/lib/Frontend/PrecompiledPreamble.cpp b/lib/Frontend/PrecompiledPreamble.cpp
index a43205a1c3..09e2b20a48 100644
--- a/lib/Frontend/PrecompiledPreamble.cpp
+++ b/lib/Frontend/PrecompiledPreamble.cpp
@@ -433,7 +433,7 @@ bool PrecompiledPreamble::CanReuse(const CompilerInvocation &Invocation,
   // Check that none of the files used by the preamble have changed.
   // First, make a record of those files that have been overridden via
   // remapping or unsaved_files.
-  std::map<llvm::sys::fs::UniqueID, PreambleFileHash> OverriddenFiles;
+  std::map<std::string, PreambleFileHash> OverriddenFiles;
   for (const auto &R : PreprocessorOpts.RemappedFiles) {
     vfs::Status Status;
     if (!moveOnNoError(VFS->status(R.second), Status)) {
@@ -442,7 +442,7 @@ bool PrecompiledPreamble::CanReuse(const CompilerInvocation &Invocation,
       return false;
     }
 
-    OverriddenFiles[Status.getUniqueID()] = PreambleFileHash::createForFile(
+    OverriddenFiles[R.first] = PreambleFileHash::createForFile(
         Status.getSize(), llvm::sys::toTimeT(Status.getLastModificationTime()));
   }
 
@@ -470,8 +470,8 @@ bool PrecompiledPreamble::CanReuse(const CompilerInvocation &Invocation,
         return false;
     }
 
-    std::map<llvm::sys::fs::UniqueID, PreambleFileHash>::iterator Overridden =
-        OverriddenFiles.find(Status.getUniqueID());
+    std::map<std::string, 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.
-- 
2.15.1

[nik]

Rebased and renamed the counter variable only.

I do not feel comfortable changing the "std::map<llvm::sys::fs::UniqueID,
PreambleFileHash> OverriddenFiles". I can do this in a follow-up change if you
want.

@Ivan: Coul you please run the tests with this change on Windows?! If it goes
well (no failures), then please also give it also a try with the additional
change (reply to "Will anything fail if we remove the map from UniqueID to
hashes of overriden files and the corresponding checks?").

[yvvan]

No regression in tests on Windows with and without extra patch ([PATCH] Use file path instead of uniqueID)

[ilya-biryukov]

include/clang/Frontend/ASTUnit.h

193

NIT: Maybe shorten to PreambleRebuildCountdown?
It's not a great name, but a bit shorter than now and seems to convey the same meaning.

unittests/Frontend/PCHPreambleTest.cpp

130

NIT: Maybe use raw string literals? Escpated string are hard to read.
E.g.,

R"cpp(
#include "//./header1.h"
int main() { return ZERO; }
)cpp"

133

Are we testing the right thing here?

We're testing that preamble does not get rebuild when some header that was previously unresolved when seen inside #include directive is now added to the filesystem. That is actually a bug, we should rebuild the preamble in that case.

We should probably call RemapFile before calling ParseAST instead and make sure it's properly resolved.

AddFile(MainName, ...);
// We don't call AddFile for the header at this point, so that it does not exist on the filesystem.
RemapFile(Header1, "#define ZERO 0\n");

std::unique_ptr<ASTUnit> AST(ParseAST(MainName));
// Also assert that there were no compiler errors? (I.e. file was resolved properly)
// .... 

// Now the file is on the filesystem, call reparse and check that we reused the preamble.
AddFile(Header1, "#define ZERO 0\n");
ASSERT_TRUE(ReparseAST(AST));
ASSERT_EQ(AST->getPreambleCounter(), 1U);

[cameron314]

@yvvan: The clang frontend tests (PCHPreambleTest and friends) are disabled on Windows in the makefile (I think because the VFS tests depend on linux-like paths). So running the tests on Windows without failures is encouraging but not the whole story.

[yvvan]

In D41005#1001854, @cameron314 wrote:

@yvvan: The clang frontend tests (PCHPreambleTest and friends) are disabled on Windows in the makefile (I think because the VFS tests depend on linux-like paths). So running the tests on Windows without failures is encouraging but not the whole story.

Nice to know. But the bad thing is that it is not obvious at all from the code. And another bad thing is that tests are platform dependent.

[nik]

Sorry for the delay, I think I'll come back to this one soon.

[nik]

Coming back to this one, I see a failing test: PCHPreambleTest.ReparseWithOverriddenFileDoesNotInvalidatePreamble

Note that PCHPreambleTest.ReparseWithOverriddenFileDoesNotInvalidatePreamble references the header paths in different ways ("./header1.h" vs "./foo/../header1.h"). Before this change, this was not a problem because OverriddenFiles were keyed on Status.getUniqueID(). Starting with this change, the key is the file path.

Is there a nice way to map different file paths of the same file to the same id without touching the real file system? Would it be sufficient to normalize the file paths? If yes, is there a utility function for this (can't find it right now).

include/clang/Frontend/ASTUnit.h

193

Will do.

unittests/Frontend/PCHPreambleTest.cpp

130

Will do.

133

Are we testing the right thing here?

Huch, indeed, the test is wrong.

I'll incorporate your suggested test (real file is added after providing unsaved file) and add also another one: real file is removed after providing an unsaved file.

That is actually a bug, we should rebuild the preamble in that case.

Agree, the preamble should be rebuild in such a case, but that's something for a separate change.

[nik]

Ping.

[ilya-biryukov]

Before this change, this was not a problem because OverriddenFiles were keyed on Status.getUniqueID(). Starting with this change, the key is the file path.

I suggest keeping two maps for overridden files: one for existing files (key is UniqueID), another one for the ones that don't exist (key is the file path).

Is there a nice way to map different file paths of the same file to the same id without touching the real file system? Would it be sufficient to normalize the file paths? If yes, is there a utility function for this (can't find it right now).

I don't think there is one, the unique ids are closely tied to the filesystem. IIUC the unique ids are the same whenever two different paths are symlinks (or hardlinks?) pointing to the same physical file and there's no way to tell if they're the same without resolving the symlink.

[nik]

In D41005#1295632, @ilya-biryukov wrote:

Before this change, this was not a problem because OverriddenFiles were keyed on Status.getUniqueID(). Starting with this change, the key is the file path.

I suggest keeping two maps for overridden files: one for existing files (key is UniqueID), another one for the ones that don't exist (key is the file path).

Done.

Is there a nice way to map different file paths of the same file to the same id without touching the real file system? Would it be sufficient to normalize the file paths? If yes, is there a utility function for this (can't find it right now).

I don't think there is one, the unique ids are closely tied to the filesystem. IIUC the unique ids are the same whenever two different paths are symlinks (or hardlinks?) pointing to the same physical file and there's no way to tell if they're the same without resolving the symlink.

OK, so if the unsaved file is not being backed up by a real file on disk and symlinks are used, we can't do much about it.
I've normalized the file paths to handle at least that case where they might differ.

[nik]

Addressed comments.

[nik]

Ping.

[nik]

Ping.

[ilya-biryukov]

include/clang/Frontend/ASTUnit.h

558

NIT: getPreambleCounterForTests()? This is clearly an internal detail, would try giving it a name that discourages using it outside the testing code.

lib/Frontend/ASTUnit.cpp

1305

Why not increase this counter unconditionally for every error? (And a non-error case too, I guess)

lib/Frontend/PrecompiledPreamble.cpp

451

Could we avoid adding path normalization in this patch? Would it break anything?
This is clearly an important detail that preamble might get wrong, but I'd rather add it in a separate patch to avoid putting multiple things in the same patch.

[nik]

include/clang/Frontend/ASTUnit.h

558

Done.

Side note: I hardly see something like this used in clang, but I agree that it's good.

[nik]

Addressed comments.

[ilya-biryukov]

lib/Frontend/PrecompiledPreamble.cpp

449

NIT: remove braces around single-statement branches, they are usually omitted in the LLVM code

[nik]

Addressed comment.

[nik]

Meh, something changed in the meanwhile. ReparseReusesPreambleAfterUnsavedFileWasRemovedFromDisk fails now. Looking into it.

[nik]

Meh, something changed in the meanwhile. ReparseReusesPreambleAfterUnsavedFileWasRemovedFromDisk fails now. Looking into it.

No, it's just me ;) I've referenced the header file wrong.

[nik]

Ping.

[nik]

Ping.

[nik]

Minor diff update fixing indentation and removing not needed include.

Ping.

[ilya-biryukov]

Again, sorry for the delay. This looks good, just a few NITs from me before I stamp it

lib/Frontend/PrecompiledPreamble.cpp

437

Could you add a comment that this contains only the files there were not found on disk (i.e. the vfs call failed and we couldn't get a UniqueID)

453

NIT: change to: The file's buffer was remapped and the file was not found in VFS

[nik]

Addressed inline comments.

[ilya-biryukov]

Thanks! LGTM

include/clang/Frontend/ASTUnit.h

private:
   /// precompiled preamble.
   ///
   /// If zero, we will never build a precompiled preamble. Otherwise,
   /// it's treated as a counter that decrements each time we reparse
   /// without the benefit of a precompiled preamble. When it hits 1,
   /// we'll attempt to rebuild the precompiled header. This way, if
   /// building the precompiled preamble fails, we won't try again for
   /// some number of calls.
   unsigned PreambleRebuildCounter;
 
   /// \brief Cache pairs "filename - source location"
   ///
   /// Cache contains only source locations from preamble so it is
   /// guaranteed that they stay valid when the SourceManager is recreated.
   /// This cache is used when loading preambule to increase performance
   /// of that loading. It must be cleared when preamble is recreated.
   llvm::StringMap<SourceLocation> PreambleSrcLocCache;
 
 private:
   /// The contents of the preamble.
return SourceRange(mapLocationFromPreamble(R.getBegin()),
                        mapLocationFromPreamble(R.getEnd()));
   }
 
   /// \see mapLocationToPreamble.
   SourceRange mapRangeToPreamble(SourceRange R) const {
     return SourceRange(mapLocationToPreamble(R.getBegin()),
                        mapLocationToPreamble(R.getEnd()));
   }
-  
+  std::chrono::steady_clock::time_point getPreambleCreationTimePoint() const
+  {
+      if (Preamble)
+          return Preamble->getCreationTimePoint();
+      return std::chrono::steady_clock::time_point();
+  }
+  
   // Retrieve the diagnostics associated with this AST
   typedef StoredDiagnostic *stored_diag_iterator;
   typedef const StoredDiagnostic *stored_diag_const_iterator;
   stored_diag_const_iterator stored_diag_begin() const { 
     return StoredDiagnostics.begin(); 
   }
   stored_diag_iterator stored_diag_begin() { 
     return StoredDiagnostics.begin(); 
   }

include/clang/Frontend/PrecompiledPreamble.h

public:
 
   /// PreambleBounds used to build the preamble.
   PreambleBounds getBounds() const;
 
   /// Check whether PrecompiledPreamble can be reused for the new contents(\p
   /// MainFileBuffer) of the main file.
   bool CanReuse(const CompilerInvocation &Invocation,
                 const llvm::MemoryBuffer *MainFileBuffer, PreambleBounds Bounds,
-                vfs::FileSystem *VFS) const;
+                IntrusiveRefCntPtr<vfs::FileSystem> VFS) const;
 
   /// Changes options inside \p CI to use PCH from this preamble. Also remaps
   /// main file to \p MainFileBuffer and updates \p VFS to ensure the preamble
   /// is accessible.
   /// For in-memory preambles, PrecompiledPreamble instance continues to own
   /// the MemoryBuffer with the Preamble after this method returns. The caller
   /// is reponsible for making sure the PrecompiledPreamble instance outlives
   /// the compiler run and the AST that will be using the PCH.
   void AddImplicitPreamble(CompilerInvocation &CI,
                            IntrusiveRefCntPtr<vfs::FileSystem> &VFS,
                            llvm::MemoryBuffer *MainFileBuffer) const;
 
+  std::chrono::steady_clock::time_point getCreationTimePoint() const {
+    return CreationTimePoint;
+  }
+
 private:
   PrecompiledPreamble(PCHStorage Storage, std::vector<char> PreambleBytes,
                       bool PreambleEndsAtStartOfLine,
                       llvm::StringMap<PreambleFileHash> FilesInPreamble);
 
   /// A temp file that would be deleted on destructor call. If destructor is not
   /// called for any reason, the file will be deleted at static objects'
   /// destruction.
private:
   /// the preamble must be thrown away.
   llvm::StringMap<PreambleFileHash> FilesInPreamble;
   /// The contents of the file that was used to precompile the preamble. Only
   /// contains first PreambleBounds::Size bytes. Used to compare if the relevant
   /// part of the file has not changed, so that preamble can be reused.
   std::vector<char> PreambleBytes;
   /// See PreambleBounds::PreambleEndsAtStartOfLine
   bool PreambleEndsAtStartOfLine;
+  std::chrono::steady_clock::time_point CreationTimePoint;
 };
 
 /// A set of callbacks to gather useful information while building a preamble.
 class PreambleCallbacks {
 public:
   virtual ~PreambleCallbacks() = default;
 
   /// Called after FrontendAction::Execute(), but before

lib/Frontend/ASTUnit.cpp

ASTUnit::getMainBufferWithPrecompiledPreamble(
   PreambleBounds Bounds =
       ComputePreambleBounds(*PreambleInvocationIn.getLangOpts(),
                             MainFileBuffer.get(), MaxLines);
   if (!Bounds.Size)
     return nullptr;
 
   if (Preamble) {
     if (Preamble->CanReuse(PreambleInvocationIn, MainFileBuffer.get(), Bounds,
-                           VFS.get())) {
+                           VFS)) {
       // Okay! We can re-use the precompiled preamble.
 
       // Set the state of the diagnostic object to mimic its state
       // after parsing the preamble.
       getDiagnostics().Reset();
       ProcessWarningOptions(getDiagnostics(),
                             PreambleInvocationIn.getDiagnosticOpts());
       getDiagnostics().setNumWarnings(NumWarningsInPreamble);
ASTUnitPreambleCallbacks Callbacks;
     if (NewPreamble) {
       Preamble = std::move(*NewPreamble);
       PreambleRebuildCounter = 1;
     } else {
       switch (static_cast<BuildPreambleError>(NewPreamble.getError().value())) {
       case BuildPreambleError::CouldntCreateTempFile:
       case BuildPreambleError::PreambleIsEmpty:
         // Try again next time.
         PreambleRebuildCounter = 1;
         return nullptr;
       case BuildPreambleError::CouldntCreateTargetInfo:
       case BuildPreambleError::BeginSourceFileFailed:
       case BuildPreambleError::CouldntEmitPCH:
       case BuildPreambleError::CouldntCreateVFSOverlay:
         // These erros are more likely to repeat, retry after some period.
         PreambleRebuildCounter = DefaultPreambleRebuildInterval;
         return nullptr;

lib/Frontend/PrecompiledPreamble.cpp

return PrecompiledPreamble(std::move(Storage), std::move(PreambleBytes),
                              PreambleEndsAtStartOfLine,
                              std::move(FilesInPreamble));
 }
 
 PreambleBounds PrecompiledPreamble::getBounds() const {
   return PreambleBounds(PreambleBytes.size(), PreambleEndsAtStartOfLine);
 }
 
-bool PrecompiledPreamble::CanReuse(const CompilerInvocation &Invocation,
-                                   const llvm::MemoryBuffer *MainFileBuffer,
-                                   PreambleBounds Bounds,
-                                   vfs::FileSystem *VFS) const {
+bool PrecompiledPreamble::CanReuse(
+    const CompilerInvocation &Invocation,
+    const llvm::MemoryBuffer *MainFileBuffer, PreambleBounds Bounds,
+    IntrusiveRefCntPtr<vfs::FileSystem> VFS) const {
 
   assert(
       Bounds.Size <= MainFileBuffer->getBufferSize() &&
       "Buffer is too large. Bounds were calculated from a different buffer?");
 
   auto PreambleInvocation = std::make_shared<CompilerInvocation>(Invocation);
   PreprocessorOptions &PreprocessorOpts =
       PreambleInvocation->getPreprocessorOpts();
if (!moveOnNoError(VFS->status(R.second), Status)) {
       // horrible happened.
       return false;
     }
 
     OverriddenFiles[Status.getUniqueID()] = PreambleFileHash::createForFile(
         Status.getSize(), llvm::sys::toTimeT(Status.getLastModificationTime()));
   }
 
+  vfs::OverlayFileSystem OFS(VFS);
+  IntrusiveRefCntPtr<vfs::InMemoryFileSystem> IMFS(
+      new vfs::InMemoryFileSystem());
+  OFS.pushOverlay(IMFS);
   for (const auto &RB : PreprocessorOpts.RemappedFileBuffers) {
+    IMFS->addFile(RB.first, 0, llvm::MemoryBuffer::getMemBuffer(""));
     vfs::Status Status;
-    if (!moveOnNoError(VFS->status(RB.first), Status))
-      return false;
-
+    assert(moveOnNoError(IMFS->status(RB.first), Status));
     OverriddenFiles[Status.getUniqueID()] =
         PreambleFileHash::createForMemoryBuffer(RB.second);
   }
 
   // Check whether anything has changed.
   for (const auto &F : FilesInPreamble) {
     vfs::Status Status;
-    if (!moveOnNoError(VFS->status(F.first()), Status)) {
+    if (!moveOnNoError(OFS.status(F.first()), Status)) {
       // If we can't stat the file, assume that something horrible happened.
       return false;
     }
 
     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)
         return false;
       continue;
     }

 }
 
 PrecompiledPreamble::PrecompiledPreamble(
     PCHStorage Storage, std::vector<char> PreambleBytes,
     bool PreambleEndsAtStartOfLine,
     llvm::StringMap<PreambleFileHash> FilesInPreamble)
     : Storage(std::move(Storage)), FilesInPreamble(std::move(FilesInPreamble)),
       PreambleBytes(std::move(PreambleBytes)),
-      PreambleEndsAtStartOfLine(PreambleEndsAtStartOfLine) {
+      PreambleEndsAtStartOfLine(PreambleEndsAtStartOfLine),
+      CreationTimePoint(std::chrono::steady_clock::now()){
   assert(this->Storage.getKind() != PCHStorage::Kind::Empty);
 }
 
 llvm::ErrorOr<PrecompiledPreamble::TempPCHFile>
 PrecompiledPreamble::TempPCHFile::CreateNewPreamblePCHFile() {
   // FIXME: This is a hack so that we can override the preamble file during
   // crash-recovery testing, which is the only case where the preamble files
   // are not necessarily cleaned up.

unittests/Frontend/PCHPreambleTest.cpp

for (const auto &RemappedFile : RemappedFiles) {
       std::unique_ptr<MemoryBuffer> buf = MemoryBuffer::getMemBufferCopy(
         RemappedFile.second, RemappedFile.first());
       Remapped.emplace_back(RemappedFile.first(), buf.release());
     }
     return Remapped;
   }
 };
 
+TEST_F(PCHPreambleTest, ReparseDoesNotInvalidatePreambleDueToNotExistingUnsavedFile) {
+  std::string Header1 = "//./header1.h";
+  std::string MainName = "//./main.cpp";
+  AddFile(MainName, "#include \"//./header1.h\"\n"
+                    "int main() { return ZERO; }");
+
+  std::unique_ptr<ASTUnit> AST(ParseAST(MainName));
+  ASSERT_TRUE(AST.get());
+  auto InitialPreambleCreationTimePoint = AST->getPreambleCreationTimePoint();
+  ASSERT_NE(std::chrono::steady_clock::time_point(), InitialPreambleCreationTimePoint);
+
+  RemapFile(Header1, "#define ZERO 0\n");
+  ASSERT_TRUE(ReparseAST(AST));
+
+  ASSERT_EQ(InitialPreambleCreationTimePoint, AST->getPreambleCreationTimePoint());
+}
+
 TEST_F(PCHPreambleTest, ReparseWithOverriddenFileDoesNotInvalidatePreamble) {
   std::string Header1 = "//./header1.h";
   std::string Header2 = "//./header2.h";
   std::string MainName = "//./main.cpp";
   AddFile(Header1, "");
   AddFile(Header2, "#pragma once");
   AddFile(MainName,
     "#include \"//./foo/../header1.h\"\n"