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 ↗

(On Diff #126134)

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

453

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

}

454–455

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 ↗

(On Diff #126134)

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

453

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.

454–455

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

[nik]

Addressed Ilya's comments.

[nik]

include/clang/Frontend/ASTUnit.h

215

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

215

+1, names PreambleRebuildCounter and PreambleCounter are too similar.

lib/Frontend/PrecompiledPreamble.cpp

453

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.

483

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

215

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

lib/Frontend/PrecompiledPreamble.cpp

483

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

483

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

209–212

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

209–212

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

581

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

1370

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

lib/Frontend/PrecompiledPreamble.cpp

461

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

581

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

459

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

453

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)

469

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 = 0;
+  unsigned PreambleRebuildCountdown = 0;
+
+  /// Counter indicating how often the preamble was build in total.
+  unsigned PreambleCounter = 0;
 
   /// 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 preamble to increase performance
   /// of that loading. It must be cleared when preamble is recreated.
   llvm::StringMap<SourceLocation> PreambleSrcLocCache;
 
   /// The contents of the preamble.
   llvm::Optional<PrecompiledPreamble> Preamble;
public:
   }
 
   /// \see mapLocationToPreamble.
   SourceRange mapRangeToPreamble(SourceRange R) const {
     return SourceRange(mapLocationToPreamble(R.getBegin()),
                        mapLocationToPreamble(R.getEnd()));
   }
 
+  unsigned getPreambleCounterForTests() const { return PreambleCounter; }
+
   // Retrieve the diagnostics associated with this AST
   using stored_diag_iterator = StoredDiagnostic *;
   using stored_diag_const_iterator = const StoredDiagnostic *;
 
   stored_diag_const_iterator stored_diag_begin() const {
     return StoredDiagnostics.begin();
   }
 

lib/Frontend/ASTUnit.cpp

if (Preamble->CanReuse(PreambleInvocationIn, MainFileBuffer.get(), Bounds,
 
       // Set the state of the diagnostic object to mimic its state
       // after parsing the preamble.
       getDiagnostics().Reset();
       ProcessWarningOptions(getDiagnostics(),
                             PreambleInvocationIn.getDiagnosticOpts());
       getDiagnostics().setNumWarnings(NumWarningsInPreamble);
 
-      PreambleRebuildCounter = 1;
+      PreambleRebuildCountdown = 1;
       return MainFileBuffer;
     } else {
       Preamble.reset();
       PreambleDiagnostics.clear();
       TopLevelDeclsInPreamble.clear();
       PreambleSrcLocCache.clear();
-      PreambleRebuildCounter = 1;
+      PreambleRebuildCountdown = 1;
     }
   }
 
   // If the preamble rebuild counter > 1, it's because we previously
   // failed to build a preamble and we're not yet ready to try
   // again. Decrement the counter and return a failure.
-  if (PreambleRebuildCounter > 1) {
-    --PreambleRebuildCounter;
+  if (PreambleRebuildCountdown > 1) {
+    --PreambleRebuildCountdown;
     return nullptr;
   }
 
   assert(!Preamble && "No Preamble should be stored at that point");
   // If we aren't allowed to rebuild the precompiled preamble, just
   // return now.
   if (!AllowRebuild)
     return nullptr;
 
+  ++PreambleCounter;
+
   SmallVector<StandaloneDiagnostic, 4> NewPreambleDiagsStandalone;
   SmallVector<StoredDiagnostic, 4> NewPreambleDiags;
   ASTUnitPreambleCallbacks Callbacks;
   {
     llvm::Optional<CaptureDroppedDiagnostics> Capture;
     if (CaptureDiagnostics)
       Capture.emplace(/*RequestCapture=*/true, *Diagnostics, &NewPreambleDiags,
                       &NewPreambleDiagsStandalone);
llvm::ErrorOr<PrecompiledPreamble> NewPreamble = PrecompiledPreamble::Build(
         PreambleInvocationIn, MainFileBuffer.get(), Bounds, *Diagnostics, VFS,
         PCHContainerOps, /*StoreInMemory=*/false, Callbacks);
 
     PreambleInvocationIn.getFrontendOpts().SkipFunctionBodies =
         PreviousSkipFunctionBodies;
 
     if (NewPreamble) {
       Preamble = std::move(*NewPreamble);
-      PreambleRebuildCounter = 1;
+      PreambleRebuildCountdown = 1;
     } else {
       switch (static_cast<BuildPreambleError>(NewPreamble.getError().value())) {
       case BuildPreambleError::CouldntCreateTempFile:
         // Try again next time.
-        PreambleRebuildCounter = 1;
+        PreambleRebuildCountdown = 1;
         return nullptr;
       case BuildPreambleError::CouldntCreateTargetInfo:
       case BuildPreambleError::BeginSourceFileFailed:
       case BuildPreambleError::CouldntEmitPCH:
         // These erros are more likely to repeat, retry after some period.
-        PreambleRebuildCounter = DefaultPreambleRebuildInterval;
+        PreambleRebuildCountdown = DefaultPreambleRebuildInterval;
         return nullptr;
       }
       llvm_unreachable("unexpected BuildPreambleError");
     }
   }
 
   assert(Preamble && "Preamble wasn't built");
 
ASTUnit *ASTUnit::LoadFromCompilerInvocationAction(
 
   if (!ResourceFilesPath.empty()) {
     // Override the resources path.
     CI->getHeaderSearchOpts().ResourceDir = ResourceFilesPath;
   }
   AST->OnlyLocalDecls = OnlyLocalDecls;
   AST->CaptureDiagnostics = CaptureDiagnostics;
   if (PrecompilePreambleAfterNParses > 0)
-    AST->PreambleRebuildCounter = PrecompilePreambleAfterNParses;
+    AST->PreambleRebuildCountdown = PrecompilePreambleAfterNParses;
   AST->TUKind = Action ? Action->getTranslationUnitKind() : TU_Complete;
   AST->ShouldCacheCodeCompletionResults = CacheCodeCompletionResults;
   AST->IncludeBriefCommentsInCodeCompletion
     = IncludeBriefCommentsInCodeCompletion;
 
   // Recover resources if we crash before exiting this method.
   llvm::CrashRecoveryContextCleanupRegistrar<ASTUnit>
     ASTUnitCleanup(OwnAST.get());
bool ASTUnit::LoadFromCompilerInvocation(
   // We'll manage file buffers ourselves.
   Invocation->getPreprocessorOpts().RetainRemappedFileBuffers = true;
   Invocation->getFrontendOpts().DisableFree = false;
   getDiagnostics().Reset();
   ProcessWarningOptions(getDiagnostics(), Invocation->getDiagnosticOpts());
 
   std::unique_ptr<llvm::MemoryBuffer> OverrideMainBuffer;
   if (PrecompilePreambleAfterNParses > 0) {
-    PreambleRebuildCounter = PrecompilePreambleAfterNParses;
+    PreambleRebuildCountdown = PrecompilePreambleAfterNParses;
     OverrideMainBuffer =
         getMainBufferWithPrecompiledPreamble(PCHContainerOps, *Invocation, VFS);
     getDiagnostics().Reset();
     ProcessWarningOptions(getDiagnostics(), Invocation->getDiagnosticOpts());
   }
 
   SimpleTimer ParsingTimer(WantTiming);
   ParsingTimer.setOutput("Parsing " + getMainFileName());
bool ASTUnit::Reparse(std::shared_ptr<PCHContainerOperations> PCHContainerOps,
   for (const auto &RemappedFile : RemappedFiles) {
     Invocation->getPreprocessorOpts().addRemappedFile(RemappedFile.first,
                                                       RemappedFile.second);
   }
 
   // If we have a preamble file lying around, or if we might try to
   // build a precompiled preamble, do so now.
   std::unique_ptr<llvm::MemoryBuffer> OverrideMainBuffer;
-  if (Preamble || PreambleRebuildCounter > 0)
+  if (Preamble || PreambleRebuildCountdown > 0)
     OverrideMainBuffer =
         getMainBufferWithPrecompiledPreamble(PCHContainerOps, *Invocation, VFS);
 
   // Clear out the diagnostics state.
   FileMgr.reset();
   getDiagnostics().Reset();
   ProcessWarningOptions(getDiagnostics(), Invocation->getDiagnosticOpts());
   if (OverrideMainBuffer)

lib/Frontend/PrecompiledPreamble.cpp

 #include "clang/Serialization/ASTWriter.h"
 #include "llvm/ADT/StringExtras.h"
 #include "llvm/ADT/StringSet.h"
 #include "llvm/Config/llvm-config.h"
 #include "llvm/Support/CrashRecoveryContext.h"
 #include "llvm/Support/FileSystem.h"
 #include "llvm/Support/Mutex.h"
 #include "llvm/Support/MutexGuard.h"
+#include "llvm/Support/Path.h"
 #include "llvm/Support/Process.h"
 #include "llvm/Support/VirtualFileSystem.h"
 #include <limits>
 #include <utility>
 
 using namespace clang;
 
 namespace {
if (!moveOnNoError(VFS->status(R.second), Status)) {
       // horrible happened.
       return false;
     }
 
     OverriddenFiles[Status.getUniqueID()] = PreambleFileHash::createForFile(
         Status.getSize(), llvm::sys::toTimeT(Status.getLastModificationTime()));
   }
 
+  llvm::StringMap<PreambleFileHash> OverridenFileBuffers;
   for (const auto &RB : PreprocessorOpts.RemappedFileBuffers) {
-    llvm::vfs::Status Status;
-    if (!moveOnNoError(VFS->status(RB.first), Status))
-      return false;
-
-    OverriddenFiles[Status.getUniqueID()] =
+    const PrecompiledPreamble::PreambleFileHash PreambleHash =
         PreambleFileHash::createForMemoryBuffer(RB.second);
+    llvm::vfs::Status Status;
+    if (moveOnNoError(VFS->status(RB.first), Status)) {
+      OverriddenFiles[Status.getUniqueID()] = PreambleHash;
+    } else {
+      OverridenFileBuffers[RB.first] = PreambleHash;
+    }
   }
 
   // Check whether anything has changed.
   for (const auto &F : FilesInPreamble) {
+    auto OverridenFileBuffer = OverridenFileBuffers.find(F.first());
+    if (OverridenFileBuffer != OverridenFileBuffers.end()) {
+      // The file's buffer was remapped; check whether it matches up
+      // with the previous mapping.
+      if (OverridenFileBuffer->second != F.second)
+        return false;
+      continue;
+    }
+
     llvm::vfs::Status Status;
     if (!moveOnNoError(VFS->status(F.first()), Status)) {
-      // If we can't stat the file, assume that something horrible happened.
+      // If the file's buffer is not remapped and we can't stat it,
+      // 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;
     }
 
-    // The file was not remapped; check whether it has changed on disk.
+    // Neither the file's buffer nor the file itself was remapped;
+    // check whether it has changed on disk.
     if (Status.getSize() != uint64_t(F.second.Size) ||
          llvm::sys::toTimeT(Status.getLastModificationTime()) !=
             F.second.ModTime)
       return false;
   }
   return true;
 }
 
 void PrecompiledPreamble::AddImplicitPreamble(
     CompilerInvocation &CI, IntrusiveRefCntPtr<llvm::vfs::FileSystem> &VFS,

unittests/Frontend/PCHPreambleTest.cpp

 
 class PCHPreambleTest : public ::testing::Test {
   IntrusiveRefCntPtr<ReadCountingInMemoryFileSystem> VFS;
   StringMap<std::string> RemappedFiles;
   std::shared_ptr<PCHContainerOperations> PCHContainerOpts;
   FileSystemOptions FSOpts;
 
 public:
-  void SetUp() override {
+  void SetUp() override { ResetVFS(); }
+  void TearDown() override {}
+
+  void ResetVFS() {
     VFS = new ReadCountingInMemoryFileSystem();
     // We need the working directory to be set to something absolute,
     // otherwise it ends up being inadvertently set to the current
     // working directory in the real file system due to a series of
     // unfortunate conditions interacting badly.
     // What's more, this path *must* be absolute on all (real)
     // filesystems, so just '/' won't work (e.g. on Win32).
     VFS->setCurrentWorkingDirectory("//./");
   }
 
-  void TearDown() override {
-  }
-
   void AddFile(const std::string &Filename, const std::string &Contents) {
     ::time_t now;
     ::time(&now);
     VFS->addFile(Filename, now, MemoryBuffer::getMemBufferCopy(Contents, Filename));
   }
 
   void RemapFile(const std::string &Filename, const std::string &Contents) {
     RemappedFiles[Filename] = Contents;
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, ReparseReusesPreambleWithUnsavedFileNotExistingOnDisk) {
+  std::string Header1 = "//./header1.h";
+  std::string MainName = "//./main.cpp";
+  AddFile(MainName, R"cpp(
+#include "//./header1.h"
+int main() { return ZERO; }
+)cpp");
+  RemapFile(Header1, "#define ZERO 0\n");
+
+  // Parse with header file provided as unsaved file, which does not exist on
+  // disk.
+  std::unique_ptr<ASTUnit> AST(ParseAST(MainName));
+  ASSERT_TRUE(AST.get());
+  ASSERT_FALSE(AST->getDiagnostics().hasErrorOccurred());
+
+  // Reparse and check that the preamble was reused.
+  ASSERT_TRUE(ReparseAST(AST));
+  ASSERT_EQ(AST->getPreambleCounterForTests(), 1U);
+}
+
+TEST_F(PCHPreambleTest, ReparseReusesPreambleAfterUnsavedFileWasCreatedOnDisk) {
+  std::string Header1 = "//./header1.h";
+  std::string MainName = "//./main.cpp";
+  AddFile(MainName, R"cpp(
+#include "//./header1.h"
+int main() { return ZERO; }
+)cpp");
+  RemapFile(Header1, "#define ZERO 0\n");
+
+  // Parse with header file provided as unsaved file, which does not exist on
+  // disk.
+  std::unique_ptr<ASTUnit> AST(ParseAST(MainName));
+  ASSERT_TRUE(AST.get());
+  ASSERT_FALSE(AST->getDiagnostics().hasErrorOccurred());
+
+  // Create the unsaved file also on disk and check that preamble was reused.
+  AddFile(Header1, "#define ZERO 0\n");
+  ASSERT_TRUE(ReparseAST(AST));
+  ASSERT_EQ(AST->getPreambleCounterForTests(), 1U);
+}
+
+TEST_F(PCHPreambleTest,
+       ReparseReusesPreambleAfterUnsavedFileWasRemovedFromDisk) {
+  std::string Header1 = "//./foo/header1.h";
+  std::string MainName = "//./main.cpp";
+  std::string MainFileContent = R"cpp(
+#include "//./header1.h"
+int main() { return ZERO; }
+)cpp";
+  AddFile(MainName, MainFileContent);
+  AddFile(Header1, "#define ZERO 0\n");
+  RemapFile(Header1, "#define ZERO 0\n");
+
+  // Parse with header file provided as unsaved file, which exists on disk.
+  std::unique_ptr<ASTUnit> AST(ParseAST(MainName));
+  ASSERT_TRUE(AST.get());
+  ASSERT_FALSE(AST->getDiagnostics().hasErrorOccurred());
+  ASSERT_EQ(AST->getPreambleCounterForTests(), 1U);
+
+  // Remove the unsaved file from disk and check that the preamble was reused.
+  ResetVFS();
+  AddFile(MainName, MainFileContent);
+  ASSERT_TRUE(ReparseAST(AST));
+  ASSERT_EQ(AST->getPreambleCounterForTests(), 1U);
+}
+
 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"