[clang] 4061d9e - [clangd] Fix crash-bug in preamble indexing when using modules.
Adam Czachorowski via cfe-commits
cfe-commits at lists.llvm.org
Mon Aug 10 09:45:14 PDT 2020
Author: Adam Czachorowski
Date: 2020-08-10T18:42:57+02:00
New Revision: 4061d9e42cff621462931ac7df9666806c77a237
URL: https://github.com/llvm/llvm-project/commit/4061d9e42cff621462931ac7df9666806c77a237
DIFF: https://github.com/llvm/llvm-project/commit/4061d9e42cff621462931ac7df9666806c77a237.diff
LOG: [clangd] Fix crash-bug in preamble indexing when using modules.
Summary:
When preamble contains #undef, indexing code finds the matching #define
and uses that during indexing. However, it would only look for local
definitions. If the macro was defined in a module, MacroInfo
would be nullptr and clangd would crash.
This change makes clangd ignore any #undef without a matching #define
inside the same TU.
The indexing of macros happens for preamble only, so then #undef must be
in the preamble, which is why we need two .h files in a test.
Note that clangd is currently not ready for module support, but this
brings us one step closer.
Reviewers: sammccall
Subscribers: ilya-biryukov, MaskRay, jkorous, arphaman, kadircet, usaxena95, cfe-commits
Tags: #clang
Differential Revision: https://reviews.llvm.org/D80525
Added:
Modified:
clang-tools-extra/clangd/unittests/SymbolCollectorTests.cpp
clang-tools-extra/clangd/unittests/TestFS.h
clang-tools-extra/clangd/unittests/TestTU.cpp
clang-tools-extra/clangd/unittests/TestTU.h
clang/lib/Index/IndexingAction.cpp
Removed:
################################################################################
diff --git a/clang-tools-extra/clangd/unittests/SymbolCollectorTests.cpp b/clang-tools-extra/clangd/unittests/SymbolCollectorTests.cpp
index 3614ab2c5cb9..8e8f1a4f9af5 100644
--- a/clang-tools-extra/clangd/unittests/SymbolCollectorTests.cpp
+++ b/clang-tools-extra/clangd/unittests/SymbolCollectorTests.cpp
@@ -1520,6 +1520,31 @@ TEST_F(SymbolCollectorTest, MacrosInHeaders) {
EXPECT_THAT(Symbols,
UnorderedElementsAre(AllOf(QName("X"), ForCodeCompletion(true))));
}
+
+// Regression test for a crash-bug we used to have.
+TEST_F(SymbolCollectorTest, UndefOfModuleMacro) {
+ auto TU = TestTU::withCode(R"cpp(#include "bar.h")cpp");
+ TU.AdditionalFiles["bar.h"] = R"cpp(
+ #include "foo.h"
+ #undef X
+ )cpp";
+ TU.AdditionalFiles["foo.h"] = "#define X 1";
+ TU.AdditionalFiles["module.map"] = R"cpp(
+ module foo {
+ header "foo.h"
+ export *
+ }
+ )cpp";
+ TU.ExtraArgs.push_back("-fmodules");
+ TU.ExtraArgs.push_back("-fmodule-map-file=" + testPath("module.map"));
+ TU.OverlayRealFileSystemForModules = true;
+
+ TU.build();
+ // We mostly care about not crashing, but verify that we didn't insert garbage
+ // about X too.
+ EXPECT_THAT(TU.headerSymbols(), Not(Contains(QName("X"))));
+}
+
} // namespace
} // namespace clangd
} // namespace clang
diff --git a/clang-tools-extra/clangd/unittests/TestFS.h b/clang-tools-extra/clangd/unittests/TestFS.h
index 7972fe37c7fe..82de319d96cf 100644
--- a/clang-tools-extra/clangd/unittests/TestFS.h
+++ b/clang-tools-extra/clangd/unittests/TestFS.h
@@ -34,12 +34,21 @@ buildTestFS(llvm::StringMap<std::string> const &Files,
class MockFS : public ThreadsafeFS {
public:
IntrusiveRefCntPtr<llvm::vfs::FileSystem> viewImpl() const override {
- return buildTestFS(Files, Timestamps);
+ auto MemFS = buildTestFS(Files, Timestamps);
+ if (!OverlayRealFileSystemForModules)
+ return MemFS;
+ llvm::IntrusiveRefCntPtr<llvm::vfs::OverlayFileSystem> OverlayFileSystem =
+ new llvm::vfs::OverlayFileSystem(llvm::vfs::getRealFileSystem());
+ OverlayFileSystem->pushOverlay(MemFS);
+ return OverlayFileSystem;
}
// If relative paths are used, they are resolved with testPath().
llvm::StringMap<std::string> Files;
llvm::StringMap<time_t> Timestamps;
+ // If true, real file system will be used as fallback for the in-memory one.
+ // This is useful for testing module support.
+ bool OverlayRealFileSystemForModules = false;
};
// A Compilation database that returns a fixed set of compile flags.
diff --git a/clang-tools-extra/clangd/unittests/TestTU.cpp b/clang-tools-extra/clangd/unittests/TestTU.cpp
index 7d5c29f23393..16305400307a 100644
--- a/clang-tools-extra/clangd/unittests/TestTU.cpp
+++ b/clang-tools-extra/clangd/unittests/TestTU.cpp
@@ -54,6 +54,8 @@ ParseInputs TestTU::inputs(MockFS &FS) const {
Inputs.CompileCommand.Filename = FullFilename;
Inputs.CompileCommand.Directory = testRoot();
Inputs.Contents = Code;
+ if (OverlayRealFileSystemForModules)
+ FS.OverlayRealFileSystemForModules = true;
Inputs.TFS = &FS;
Inputs.Opts = ParseOptions();
Inputs.Opts.BuildRecoveryAST = true;
diff --git a/clang-tools-extra/clangd/unittests/TestTU.h b/clang-tools-extra/clangd/unittests/TestTU.h
index 06abd64dc623..dd0cecc406ac 100644
--- a/clang-tools-extra/clangd/unittests/TestTU.h
+++ b/clang-tools-extra/clangd/unittests/TestTU.h
@@ -66,6 +66,16 @@ struct TestTU {
// Simulate a header guard of the header (using an #import directive).
bool ImplicitHeaderGuard = true;
+ // Whether to use overlay the TestFS over the real filesystem. This is
+ // required for use of implicit modules.where the module file is written to
+ // disk and later read back.
+ // FIXME: Change the way reading/writing modules work to allow us to keep them
+ // in memory across multiple clang invocations, at least in tests, to
+ // eliminate the need for real file system here.
+ // Please avoid using this for things other than implicit modules. The plan is
+ // to eliminate this option some day.
+ bool OverlayRealFileSystemForModules = false;
+
// By default, build() will report Error diagnostics as GTest errors.
// Suppress this behavior by adding an 'error-ok' comment to the code.
ParsedAST build() const;
diff --git a/clang/lib/Index/IndexingAction.cpp b/clang/lib/Index/IndexingAction.cpp
index e698c07133a9..4986303cac47 100644
--- a/clang/lib/Index/IndexingAction.cpp
+++ b/clang/lib/Index/IndexingAction.cpp
@@ -165,11 +165,20 @@ static void indexTranslationUnit(ASTUnit &Unit, IndexingContext &IndexCtx) {
static void indexPreprocessorMacros(const Preprocessor &PP,
IndexDataConsumer &DataConsumer) {
for (const auto &M : PP.macros())
- if (MacroDirective *MD = M.second.getLatest())
+ if (MacroDirective *MD = M.second.getLatest()) {
+ auto *MI = MD->getMacroInfo();
+ // When using modules, it may happen that we find #undef of a macro that
+ // was defined in another module. In such case, MI may be nullptr, since
+ // we only look for macro definitions in the current TU. In that case,
+ // there is nothing to index.
+ if (!MI)
+ continue;
+
DataConsumer.handleMacroOccurrence(
M.first, MD->getMacroInfo(),
static_cast<unsigned>(index::SymbolRole::Definition),
MD->getLocation());
+ }
}
void index::indexASTUnit(ASTUnit &Unit, IndexDataConsumer &DataConsumer,
More information about the cfe-commits
mailing list