[Preamble] Invalidate preamble when missing headers become present.

Summary:
To avoid excessive extra stat()s, only check the possible locations of
headers that weren't found at all (leading to a compile error).
For headers that *were* found, we don't check for files earlier on the
search path that could override them.

Reviewers: kadircet

Subscribers: javed.absar, jkorous, arphaman, usaxena95, cfe-commits

Tags: #clang

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

clang-tools-extra/clangd/unittests/ClangdLSPServerTests.cpp

@@ -131,11 +131,19 @@ TEST_F(LSPTest, Diagnostics) {
 
 TEST_F(LSPTest, DiagnosticsHeaderSaved) {
   auto &Client = start();
-  FS.Files["foo.h"] = "#define VAR original";
   Client.didOpen("foo.cpp", R"cpp(
     #include "foo.h"
     int x = VAR;
   )cpp");
+  EXPECT_THAT(Client.diagnostics("foo.cpp"),
+              llvm::ValueIs(testing::ElementsAre(
+                  DiagMessage("'foo.h' file not found"),
+                  DiagMessage("Use of undeclared identifier 'VAR'"))));
+  // Now create the header.
+  FS.Files["foo.h"] = "#define VAR original";
+  Client.notify(
+      "textDocument/didSave",
+      llvm::json::Object{{"textDocument", Client.documentID("foo.h")}});
   EXPECT_THAT(Client.diagnostics("foo.cpp"),
               llvm::ValueIs(testing::ElementsAre(
                   DiagMessage("Use of undeclared identifier 'original'"))));

clang-tools-extra/clangd/unittests/TUSchedulerTests.cpp

@@ -735,15 +735,24 @@ TEST_F(TUSchedulerTests, NoopOnEmptyChanges) {
   ASSERT_EQ(S.fileStats().lookup(Source).PreambleBuilds, 3u);
 }
 
-TEST_F(TUSchedulerTests, ForceRebuild) {
+// We rebuild if a completely missing header exists, but not if one is added
+// on a higher-priority include path entry (for performance).
+// (Previously we wouldn't automatically rebuild when files were added).
+TEST_F(TUSchedulerTests, MissingHeader) {
+  CDB.ExtraClangFlags.push_back("-I" + testPath("a"));
+  CDB.ExtraClangFlags.push_back("-I" + testPath("b"));
+  // Force both directories to exist so they don't get pruned.
+  FSProvider.Files.try_emplace("a/__unused__");
+  FSProvider.Files.try_emplace("b/__unused__");
   TUScheduler S(CDB, optsForTest(), captureDiags());
 
   auto Source = testPath("foo.cpp");
-  auto Header = testPath("foo.h");
+  auto HeaderA = testPath("a/foo.h");
+  auto HeaderB = testPath("b/foo.h");
 
   auto SourceContents = R"cpp(
       #include "foo.h"
-      int b = a;
+      int c = b;
     )cpp";
 
   ParseInputs Inputs = getInputs(Source, SourceContents);
@@ -758,37 +767,48 @@ TEST_F(TUSchedulerTests, ForceRebuild) {
         EXPECT_THAT(Diags,
                     ElementsAre(Field(&Diag::Message, "'foo.h' file not found"),
                                 Field(&Diag::Message,
-                                      "use of undeclared identifier 'a'")));
+                                      "use of undeclared identifier 'b'")));
       });
   S.blockUntilIdle(timeoutSeconds(10));
 
-  // Add the header file. We need to recreate the inputs since we changed a
+  FSProvider.Files[HeaderB] = "int b;";
-  // file from underneath the test FS.
+  FSProvider.Timestamps[HeaderB] = time_t(1);
-  FSProvider.Files[Header] = "int a;";
-  FSProvider.Timestamps[Header] = time_t(1);
-  Inputs = getInputs(Source, SourceContents);
 
-  // The addition of the missing header file shouldn't trigger a rebuild since
+  // The addition of the missing header file triggers a rebuild, no errors.
-  // we don't track missing files.
   updateWithDiags(
       S, Source, Inputs, WantDiagnostics::Yes,
-      [&DiagCount](std::vector<Diag> Diags) {
-        ++DiagCount;
-        ADD_FAILURE() << "Did not expect diagnostics for missing header update";
-      });
-
-  // Forcing the reload should should cause a rebuild which no longer has any
-  // errors.
-  Inputs.ForceRebuild = true;
-  updateWithDiags(S, Source, Inputs, WantDiagnostics::Yes,
       [&DiagCount](std::vector<Diag> Diags) {
         ++DiagCount;
         EXPECT_THAT(Diags, IsEmpty());
       });
 
+  // Ensure previous assertions are done before we touch the FS again.
   ASSERT_TRUE(S.blockUntilIdle(timeoutSeconds(10)));
-  EXPECT_EQ(DiagCount, 2U);
+  // Add the high-priority header file, which should reintroduce the error.
+  FSProvider.Files[HeaderA] = "int a;";
+  FSProvider.Timestamps[HeaderA] = time_t(1);
+
+  // This isn't detected: we don't stat a/foo.h to validate the preamble.
+  updateWithDiags(S, Source, Inputs, WantDiagnostics::Yes,
+                  [&DiagCount](std::vector<Diag> Diags) {
+                    ++DiagCount;
+                    ADD_FAILURE()
+                        << "Didn't expect new diagnostics when adding a/foo.h";
+                  });
+
+  // Forcing the reload should should cause a rebuild.
+  Inputs.ForceRebuild = true;
+  updateWithDiags(S, Source, Inputs, WantDiagnostics::Yes,
+                  [&DiagCount](std::vector<Diag> Diags) {
+                    ++DiagCount;
+                    ElementsAre(Field(&Diag::Message,
+                                      "use of undeclared identifier 'b'"));
+                  });
+
+  ASSERT_TRUE(S.blockUntilIdle(timeoutSeconds(10)));
+  EXPECT_EQ(DiagCount, 3U);
 }
+
 TEST_F(TUSchedulerTests, NoChangeDiags) {
   trace::TestTracer Tracer;
   TUScheduler S(CDB, optsForTest(), captureDiags());

clang/include/clang/Frontend/PrecompiledPreamble.h

@@ -128,7 +128,8 @@ public:
 private:
   PrecompiledPreamble(PCHStorage Storage, std::vector<char> PreambleBytes,
                       bool PreambleEndsAtStartOfLine,
-                      llvm::StringMap<PreambleFileHash> FilesInPreamble);
+                      llvm::StringMap<PreambleFileHash> FilesInPreamble,
+                      llvm::StringSet<> MissingFiles);
 
   /// 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'
@@ -249,6 +250,15 @@ private:
   /// If any of the files have changed from one compile to the next,
   /// the preamble must be thrown away.
   llvm::StringMap<PreambleFileHash> FilesInPreamble;
+  /// Files that were not found during preamble building. If any of these now
+  /// exist then the preamble should not be reused.
+  ///
+  /// Storing *all* the missing files that could invalidate the preamble would
+  /// make it too expensive to revalidate (when the include path has many
+  /// entries, each #include will miss half of them on average).
+  /// Instead, we track only files that could have satisfied an #include that
+  /// was ultimately not found.
+  llvm::StringSet<> MissingFiles;
   /// 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.

clang/lib/Frontend/PrecompiledPreamble.cpp

@@ -19,15 +19,19 @@
 #include "clang/Frontend/CompilerInvocation.h"
 #include "clang/Frontend/FrontendActions.h"
 #include "clang/Frontend/FrontendOptions.h"
+#include "clang/Lex/HeaderSearch.h"
 #include "clang/Lex/Lexer.h"
 #include "clang/Lex/Preprocessor.h"
 #include "clang/Lex/PreprocessorOptions.h"
 #include "clang/Serialization/ASTWriter.h"
+#include "llvm/ADT/SmallString.h"
 #include "llvm/ADT/StringExtras.h"
 #include "llvm/ADT/StringSet.h"
+#include "llvm/ADT/iterator_range.h"
 #include "llvm/Config/llvm-config.h"
 #include "llvm/Support/CrashRecoveryContext.h"
 #include "llvm/Support/FileSystem.h"
+#include "llvm/Support/Path.h"
 #include "llvm/Support/Process.h"
 #include "llvm/Support/VirtualFileSystem.h"
 #include <limits>
@@ -74,6 +78,68 @@ public:
   bool needSystemDependencies() override { return true; }
 };
 
+// Collects files whose existence would invalidate the preamble.
+// Collecting *all* of these would make validating it too slow though, so we
+// just find all the candidates for 'file not found' diagnostics.
+//
+// A caveat that may be significant for generated files: we'll omit files under
+// search path entries whose roots don't exist when the preamble is built.
+// These are pruned by InitHeaderSearch and so we don't see the search path.
+// It would be nice to include them but we don't want to duplicate all the rest
+// of the InitHeaderSearch logic to reconstruct them.
+class MissingFileCollector : public PPCallbacks {
+  llvm::StringSet<> &Out;
+  const HeaderSearch &Search;
+  const SourceManager &SM;
+
+public:
+  MissingFileCollector(llvm::StringSet<> &Out, const HeaderSearch &Search,
+                       const SourceManager &SM)
+      : Out(Out), Search(Search), SM(SM) {}
+
+  void InclusionDirective(SourceLocation HashLoc, const Token &IncludeTok,
+                          StringRef FileName, bool IsAngled,
+                          CharSourceRange FilenameRange, const FileEntry *File,
+                          StringRef SearchPath, StringRef RelativePath,
+                          const Module *Imported,
+                          SrcMgr::CharacteristicKind FileType) override {
+    // File is null if it wasn't found.
+    // (We have some false negatives if PP recovered e.g. <foo> -> "foo")
+    if (File != nullptr)
+      return;
+
+    // If it's a rare absolute include, we know the full path already.
+    if (llvm::sys::path::is_absolute(FileName)) {
+      Out.insert(FileName);
+      return;
+    }
+
+    // Reconstruct the filenames that would satisfy this directive...
+    llvm::SmallString<256> Buf;
+    auto NotFoundRelativeTo = [&](const DirectoryEntry *DE) {
+      Buf = DE->getName();
+      llvm::sys::path::append(Buf, FileName);
+      llvm::sys::path::remove_dots(Buf, /*remove_dot_dot=*/true);
+      Out.insert(Buf);
+    };
+    // ...relative to the including file.
+    if (!IsAngled) {
+      if (const FileEntry *IncludingFile =
+              SM.getFileEntryForID(SM.getFileID(IncludeTok.getLocation())))
+        if (IncludingFile->getDir())
+          NotFoundRelativeTo(IncludingFile->getDir());
+    }
+    // ...relative to the search paths.
+    for (const auto &Dir : llvm::make_range(
+             IsAngled ? Search.angled_dir_begin() : Search.search_dir_begin(),
+             Search.search_dir_end())) {
+      // No support for frameworks or header maps yet.
+      if (Dir.isNormalDir())
+        NotFoundRelativeTo(Dir.getDir());
+    }
+  }
+};
+
 /// Keeps a track of files to be deleted in destructor.
 class TemporaryFiles {
 public:
@@ -353,6 +419,11 @@ llvm::ErrorOr<PrecompiledPreamble> PrecompiledPreamble::Build(
     Clang->getPreprocessor().addPPCallbacks(std::move(DelegatedPPCallbacks));
   if (auto CommentHandler = Callbacks.getCommentHandler())
     Clang->getPreprocessor().addCommentHandler(CommentHandler);
+  llvm::StringSet<> MissingFiles;
+  Clang->getPreprocessor().addPPCallbacks(
+      std::make_unique<MissingFileCollector>(
+          MissingFiles, Clang->getPreprocessor().getHeaderSearchInfo(),
+          Clang->getSourceManager()));
 
   if (llvm::Error Err = Act->Execute())
     return errorToErrorCode(std::move(Err));
@@ -387,9 +458,9 @@ llvm::ErrorOr<PrecompiledPreamble> PrecompiledPreamble::Build(
     }
   }
 
-  return PrecompiledPreamble(std::move(Storage), std::move(PreambleBytes),
+  return PrecompiledPreamble(
-                             PreambleEndsAtStartOfLine,
+      std::move(Storage), std::move(PreambleBytes), PreambleEndsAtStartOfLine,
-                             std::move(FilesInPreamble));
+      std::move(FilesInPreamble), std::move(MissingFiles));
 }
 
 PreambleBounds PrecompiledPreamble::getBounds() const {
@@ -446,6 +517,7 @@ bool PrecompiledPreamble::CanReuse(const CompilerInvocation &Invocation,
   // First, make a record of those files that have been overridden via
   // remapping or unsaved_files.
   std::map<llvm::sys::fs::UniqueID, PreambleFileHash> OverriddenFiles;
+  llvm::StringSet<> OverriddenAbsPaths; // Either by buffers or files.
   for (const auto &R : PreprocessorOpts.RemappedFiles) {
     llvm::vfs::Status Status;
     if (!moveOnNoError(VFS->status(R.second), Status)) {
@@ -453,6 +525,10 @@ bool PrecompiledPreamble::CanReuse(const CompilerInvocation &Invocation,
       // horrible happened.
       return false;
     }
+    // If a mapped file was previously missing, then it has changed.
+    llvm::SmallString<128> MappedPath(R.first);
+    if (!VFS->makeAbsolute(MappedPath))
+      OverriddenAbsPaths.insert(MappedPath);
 
     OverriddenFiles[Status.getUniqueID()] = PreambleFileHash::createForFile(
         Status.getSize(), llvm::sys::toTimeT(Status.getLastModificationTime()));
@@ -468,6 +544,10 @@ bool PrecompiledPreamble::CanReuse(const CompilerInvocation &Invocation,
       OverriddenFiles[Status.getUniqueID()] = PreambleHash;
     else
       OverridenFileBuffers[RB.first] = PreambleHash;
+
+    llvm::SmallString<128> MappedPath(RB.first);
+    if (!VFS->makeAbsolute(MappedPath))
+      OverriddenAbsPaths.insert(MappedPath);
   }
 
   // Check whether anything has changed.
@@ -505,6 +585,17 @@ bool PrecompiledPreamble::CanReuse(const CompilerInvocation &Invocation,
             F.second.ModTime)
       return false;
   }
+  for (const auto &F : MissingFiles) {
+    // A missing file may be "provided" by an override buffer or file.
+    if (OverriddenAbsPaths.count(F.getKey()))
+      return false;
+    // If a file previously recorded as missing exists as a regular file, then
+    // consider the preamble out-of-date.
+    if (auto Status = VFS->status(F.getKey())) {
+      if (Status->isRegularFile())
+        return false;
+    }
+  }
   return true;
 }
 
@@ -525,8 +616,10 @@ void PrecompiledPreamble::OverridePreamble(
 PrecompiledPreamble::PrecompiledPreamble(
     PCHStorage Storage, std::vector<char> PreambleBytes,
     bool PreambleEndsAtStartOfLine,
-    llvm::StringMap<PreambleFileHash> FilesInPreamble)
+    llvm::StringMap<PreambleFileHash> FilesInPreamble,
+    llvm::StringSet<> MissingFiles)
     : Storage(std::move(Storage)), FilesInPreamble(std::move(FilesInPreamble)),
+      MissingFiles(std::move(MissingFiles)),
       PreambleBytes(std::move(PreambleBytes)),
       PreambleEndsAtStartOfLine(PreambleEndsAtStartOfLine) {
   assert(this->Storage.getKind() != PCHStorage::Kind::Empty);