diff --git a/clang-tools-extra/clangd/unittests/ClangdLSPServerTests.cpp b/clang-tools-extra/clangd/unittests/ClangdLSPServerTests.cpp --- a/clang-tools-extra/clangd/unittests/ClangdLSPServerTests.cpp +++ b/clang-tools-extra/clangd/unittests/ClangdLSPServerTests.cpp @@ -128,11 +128,19 @@ 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'")))); diff --git a/clang-tools-extra/clangd/unittests/TUSchedulerTests.cpp b/clang-tools-extra/clangd/unittests/TUSchedulerTests.cpp --- a/clang-tools-extra/clangd/unittests/TUSchedulerTests.cpp +++ b/clang-tools-extra/clangd/unittests/TUSchedulerTests.cpp @@ -723,15 +723,24 @@ ASSERT_FALSE(DoUpdate(OtherSourceContents)); } -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. + Files.try_emplace("a/__unused__"); + 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); @@ -746,36 +755,49 @@ 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'"))); }); // Add the header file. We need to recreate the inputs since we changed a // file from underneath the test FS. - Files[Header] = "int a;"; - Timestamps[Header] = time_t(1); + Files[HeaderB] = "int b;"; + Timestamps[HeaderB] = time_t(1); Inputs = getInputs(Source, SourceContents); - // The addition of the missing header file shouldn't trigger a rebuild since - // we don't track missing files. + // The addition of the missing header file triggers a rebuild, no errors. updateWithDiags( S, Source, Inputs, WantDiagnostics::Yes, [&DiagCount](std::vector Diags) { ++DiagCount; - ADD_FAILURE() << "Did not expect diagnostics for missing header update"; + EXPECT_THAT(Diags, IsEmpty()); }); - // Forcing the reload should should cause a rebuild which no longer has any - // errors. + // Add the high-priority header file, which should reintroduce the error. + Files[HeaderA] = "int a;"; + Timestamps[HeaderA] = time_t(1); + Inputs = getInputs(Source, SourceContents); + + // This isn't detected: we don't stat a/foo.h to validate the preamble. + updateWithDiags(S, Source, Inputs, WantDiagnostics::Yes, + [&DiagCount](std::vector 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 Diags) { ++DiagCount; - EXPECT_THAT(Diags, IsEmpty()); + ElementsAre(Field(&Diag::Message, + "use of undeclared identifier 'b'")); }); ASSERT_TRUE(S.blockUntilIdle(timeoutSeconds(10))); - EXPECT_EQ(DiagCount, 2U); + EXPECT_EQ(DiagCount, 3U); } + TEST_F(TUSchedulerTests, NoChangeDiags) { TUScheduler S(CDB, optsForTest(), captureDiags()); diff --git a/clang/include/clang/Frontend/PrecompiledPreamble.h b/clang/include/clang/Frontend/PrecompiledPreamble.h --- a/clang/include/clang/Frontend/PrecompiledPreamble.h +++ b/clang/include/clang/Frontend/PrecompiledPreamble.h @@ -122,7 +122,8 @@ private: PrecompiledPreamble(PCHStorage Storage, std::vector PreambleBytes, bool PreambleEndsAtStartOfLine, - llvm::StringMap FilesInPreamble); + llvm::StringMap 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' @@ -243,6 +244,15 @@ /// If any of the files have changed from one compile to the next, /// the preamble must be thrown away. llvm::StringMap 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. diff --git a/clang/lib/Frontend/PrecompiledPreamble.cpp b/clang/lib/Frontend/PrecompiledPreamble.cpp --- a/clang/lib/Frontend/PrecompiledPreamble.cpp +++ b/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 @@ -74,6 +78,78 @@ 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; + std::string LastNotFound; + const SourceManager &SM; + +public: + MissingFileCollector(llvm::StringSet<> &Out, const HeaderSearch &Search, + const SourceManager &SM) + : Out(Out), Search(Search), SM(SM) {} + + // We always receive FileNotFound followed by InclusionDirective. + // We want the former, so we're completely sure the file was missing. + // We also need the latter to see whether the include was . + bool FileNotFound(StringRef FileName, + SmallVectorImpl &RecoveryPath) override { + // If it's a rare absolute include, we know the full path already. + if (llvm::sys::path::is_absolute(FileName)) { + Out.insert(FileName); + LastNotFound.clear(); + } + LastNotFound = FileName.str(); + return false; + } + + 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 { + // Only process this if we match it with a previous FileNotFound. + // Allow this even if File is set, it may be set from recovery. + bool Match = LastNotFound == FileName; + LastNotFound.clear(); + if (!Match) + 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 +429,11 @@ Clang->getPreprocessor().addPPCallbacks(std::move(DelegatedPPCallbacks)); if (auto CommentHandler = Callbacks.getCommentHandler()) Clang->getPreprocessor().addCommentHandler(CommentHandler); + llvm::StringSet<> MissingFiles; + Clang->getPreprocessor().addPPCallbacks( + std::make_unique( + MissingFiles, Clang->getPreprocessor().getHeaderSearchInfo(), + Clang->getSourceManager())); if (llvm::Error Err = Act->Execute()) return errorToErrorCode(std::move(Err)); @@ -387,9 +468,9 @@ } } - return PrecompiledPreamble(std::move(Storage), std::move(PreambleBytes), - PreambleEndsAtStartOfLine, - std::move(FilesInPreamble)); + return PrecompiledPreamble( + std::move(Storage), std::move(PreambleBytes), PreambleEndsAtStartOfLine, + std::move(FilesInPreamble), std::move(MissingFiles)); } PreambleBounds PrecompiledPreamble::getBounds() const { @@ -505,6 +586,17 @@ F.second.ModTime) return false; } + for (const auto &F : MissingFiles) { + // A missing file may be "provided" by an override buffer. + if (OverridenFileBuffers.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 +617,10 @@ PrecompiledPreamble::PrecompiledPreamble( PCHStorage Storage, std::vector PreambleBytes, bool PreambleEndsAtStartOfLine, - llvm::StringMap FilesInPreamble) + llvm::StringMap 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);