[clang] 615673f - [Preamble] Invalidate preamble when missing headers become present.
Sam McCall via cfe-commits
cfe-commits at lists.llvm.org
Mon Jun 8 05:15:35 PDT 2020
Author: Sam McCall
Date: 2020-06-08T14:03:08+02:00
New Revision: 615673f3a10e98f33e2db64512be0452145236fe
URL: https://github.com/llvm/llvm-project/commit/615673f3a10e98f33e2db64512be0452145236fe
DIFF: https://github.com/llvm/llvm-project/commit/615673f3a10e98f33e2db64512be0452145236fe.diff
LOG: [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
Added:
Modified:
clang-tools-extra/clangd/unittests/ClangdLSPServerTests.cpp
clang-tools-extra/clangd/unittests/TUSchedulerTests.cpp
clang/include/clang/Frontend/PrecompiledPreamble.h
clang/lib/Frontend/PrecompiledPreamble.cpp
Removed:
################################################################################
diff --git a/clang-tools-extra/clangd/unittests/ClangdLSPServerTests.cpp b/clang-tools-extra/clangd/unittests/ClangdLSPServerTests.cpp
index fe752821859d..431c36c72b91 100644
--- a/clang-tools-extra/clangd/unittests/ClangdLSPServerTests.cpp
+++ b/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'"))));
diff --git a/clang-tools-extra/clangd/unittests/TUSchedulerTests.cpp b/clang-tools-extra/clangd/unittests/TUSchedulerTests.cpp
index ee54a2f71e10..c04e186ca6b4 100644
--- a/clang-tools-extra/clangd/unittests/TUSchedulerTests.cpp
+++ b/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
- // file from underneath the test FS.
- FSProvider.Files[Header] = "int a;";
- FSProvider.Timestamps[Header] = time_t(1);
- Inputs = getInputs(Source, SourceContents);
+ FSProvider.Files[HeaderB] = "int b;";
+ FSProvider.Timestamps[HeaderB] = time_t(1);
- // 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<Diag> 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.
+ // Ensure previous assertions are done before we touch the FS again.
+ ASSERT_TRUE(S.blockUntilIdle(timeoutSeconds(10)));
+ // 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;
- 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
index 538f6c92ad55..94c9d0135013 100644
--- a/clang/include/clang/Frontend/PrecompiledPreamble.h
+++ b/clang/include/clang/Frontend/PrecompiledPreamble.h
@@ -128,7 +128,8 @@ class PrecompiledPreamble {
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 @@ class PrecompiledPreamble {
/// 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.
diff --git a/clang/lib/Frontend/PrecompiledPreamble.cpp b/clang/lib/Frontend/PrecompiledPreamble.cpp
index 442ad63cee0e..31c5f18eeeec 100644
--- 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 <limits>
@@ -74,6 +78,68 @@ class PreambleDependencyCollector : public DependencyCollector {
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),
- 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 @@ 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);
More information about the cfe-commits
mailing list