r361226 - [Preamble] Reuse preamble even if an unsaved file does not exist

Nikolai Kosjar via cfe-commits cfe-commits at lists.llvm.org
Tue May 21 00:26:59 PDT 2019


Author: nik
Date: Tue May 21 00:26:59 2019
New Revision: 361226

URL: http://llvm.org/viewvc/llvm-project?rev=361226&view=rev
Log:
[Preamble] Reuse preamble even if an unsaved file does not exist

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.

Differential Revision: https://reviews.llvm.org/D41005

Modified:
    cfe/trunk/include/clang/Frontend/ASTUnit.h
    cfe/trunk/lib/Frontend/ASTUnit.cpp
    cfe/trunk/lib/Frontend/PrecompiledPreamble.cpp
    cfe/trunk/unittests/Frontend/PCHPreambleTest.cpp

Modified: cfe/trunk/include/clang/Frontend/ASTUnit.h
URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Frontend/ASTUnit.h?rev=361226&r1=361225&r2=361226&view=diff
==============================================================================
--- cfe/trunk/include/clang/Frontend/ASTUnit.h (original)
+++ cfe/trunk/include/clang/Frontend/ASTUnit.h Tue May 21 00:26:59 2019
@@ -205,7 +205,10 @@ private:
   /// 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"
   ///
@@ -574,6 +577,8 @@ public:
                        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 *;

Modified: cfe/trunk/lib/Frontend/ASTUnit.cpp
URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Frontend/ASTUnit.cpp?rev=361226&r1=361225&r2=361226&view=diff
==============================================================================
--- cfe/trunk/lib/Frontend/ASTUnit.cpp (original)
+++ cfe/trunk/lib/Frontend/ASTUnit.cpp Tue May 21 00:26:59 2019
@@ -1304,22 +1304,22 @@ ASTUnit::getMainBufferWithPrecompiledPre
                             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;
   }
 
@@ -1329,6 +1329,8 @@ ASTUnit::getMainBufferWithPrecompiledPre
   if (!AllowRebuild)
     return nullptr;
 
+  ++PreambleCounter;
+
   SmallVector<StandaloneDiagnostic, 4> NewPreambleDiagsStandalone;
   SmallVector<StoredDiagnostic, 4> NewPreambleDiags;
   ASTUnitPreambleCallbacks Callbacks;
@@ -1356,18 +1358,18 @@ ASTUnit::getMainBufferWithPrecompiledPre
 
     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");
@@ -1507,7 +1509,7 @@ ASTUnit *ASTUnit::LoadFromCompilerInvoca
   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
@@ -1641,7 +1643,7 @@ bool ASTUnit::LoadFromCompilerInvocation
 
   std::unique_ptr<llvm::MemoryBuffer> OverrideMainBuffer;
   if (PrecompilePreambleAfterNParses > 0) {
-    PreambleRebuildCounter = PrecompilePreambleAfterNParses;
+    PreambleRebuildCountdown = PrecompilePreambleAfterNParses;
     OverrideMainBuffer =
         getMainBufferWithPrecompiledPreamble(PCHContainerOps, *Invocation, VFS);
     getDiagnostics().Reset();
@@ -1819,7 +1821,7 @@ bool ASTUnit::Reparse(std::shared_ptr<PC
   // 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);
 

Modified: cfe/trunk/lib/Frontend/PrecompiledPreamble.cpp
URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Frontend/PrecompiledPreamble.cpp?rev=361226&r1=361225&r2=361226&view=diff
==============================================================================
--- cfe/trunk/lib/Frontend/PrecompiledPreamble.cpp (original)
+++ cfe/trunk/lib/Frontend/PrecompiledPreamble.cpp Tue May 21 00:26:59 2019
@@ -454,20 +454,33 @@ bool PrecompiledPreamble::CanReuse(const
         Status.getSize(), llvm::sys::toTimeT(Status.getLastModificationTime()));
   }
 
+  // OverridenFileBuffers tracks only the files not found in VFS.
+  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 and the file was not found in VFS.
+      // 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;
     }
 
@@ -481,7 +494,8 @@ bool PrecompiledPreamble::CanReuse(const
       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)

Modified: cfe/trunk/unittests/Frontend/PCHPreambleTest.cpp
URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/unittests/Frontend/PCHPreambleTest.cpp?rev=361226&r1=361225&r2=361226&view=diff
==============================================================================
--- cfe/trunk/unittests/Frontend/PCHPreambleTest.cpp (original)
+++ cfe/trunk/unittests/Frontend/PCHPreambleTest.cpp Tue May 21 00:26:59 2019
@@ -52,7 +52,10 @@ class PCHPreambleTest : public ::testing
   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
@@ -63,9 +66,6 @@ public:
     VFS->setCurrentWorkingDirectory("//./");
   }
 
-  void TearDown() override {
-  }
-
   void AddFile(const std::string &Filename, const std::string &Contents) {
     ::time_t now;
     ::time(&now);
@@ -123,6 +123,72 @@ private:
   }
 };
 
+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 "//./foo/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";




More information about the cfe-commits mailing list