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 @@ -131,11 +131,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 @@ -715,15 +715,24 @@ 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. + 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); @@ -738,36 +747,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) { trace::TestTracer Tracer; 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 @@ -128,7 +128,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' @@ -249,6 +250,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,68 @@ 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") + 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 @@ 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 +458,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 { @@ -446,6 +517,7 @@ // First, make a record of those files that have been overridden via // remapping or unsaved_files. std::map 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 @@ // 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 @@ 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 @@ 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 @@ 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);