[clang-tools-extra] 53b9199 - [clangd] Fix crash-bug in preamble indexing when using modules.
Adam Czachorowski via cfe-commits
cfe-commits at lists.llvm.org
Thu Aug 20 05:33:27 PDT 2020
Author: Adam Czachorowski
Date: 2020-08-20T14:19:52+02:00
New Revision: 53b9199a5cdba8a6e294e1fb183f308ec558db22
URL: https://github.com/llvm/llvm-project/commit/53b9199a5cdba8a6e294e1fb183f308ec558db22
DIFF: https://github.com/llvm/llvm-project/commit/53b9199a5cdba8a6e294e1fb183f308ec558db22.diff
LOG: [clangd] Fix crash-bug in preamble indexing when using modules.
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.
This was previously attempted in
4061d9e42cff621462931ac7df9666806c77a237, but had to be reverted due to
broken test. This version fixes that test-only bug by setting a custom module
cache path to avoid re-use of modules across test invocations.
Differential Revision: https://reviews.llvm.org/D85923
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 d89db8f015ce..3940946d8016 100644
--- a/clang-tools-extra/clangd/unittests/SymbolCollectorTests.cpp
+++ b/clang-tools-extra/clangd/unittests/SymbolCollectorTests.cpp
@@ -1623,6 +1623,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..c468642e6338 100644
--- a/clang-tools-extra/clangd/unittests/TestTU.cpp
+++ b/clang-tools-extra/clangd/unittests/TestTU.cpp
@@ -16,6 +16,7 @@
#include "clang/Basic/Diagnostic.h"
#include "clang/Frontend/CompilerInvocation.h"
#include "clang/Frontend/Utils.h"
+#include "llvm/ADT/ScopeExit.h"
namespace clang {
namespace clangd {
@@ -54,6 +55,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;
@@ -66,12 +69,31 @@ ParseInputs TestTU::inputs(MockFS &FS) const {
return Inputs;
}
+void initializeModuleCache(CompilerInvocation &CI) {
+ llvm::SmallString<128> ModuleCachePath;
+ ASSERT_FALSE(
+ llvm::sys::fs::createUniqueDirectory("module-cache", ModuleCachePath));
+ CI.getHeaderSearchOpts().ModuleCachePath = ModuleCachePath.c_str();
+ llvm::errs() << "MC: " << ModuleCachePath << "\n";
+ llvm::errs().flush();
+}
+
+void deleteModuleCache(const std::string ModuleCachePath) {
+ if (!ModuleCachePath.empty()) {
+ ASSERT_FALSE(llvm::sys::fs::remove_directories(ModuleCachePath));
+ }
+}
+
std::shared_ptr<const PreambleData> TestTU::preamble() const {
MockFS FS;
auto Inputs = inputs(FS);
IgnoreDiagnostics Diags;
auto CI = buildCompilerInvocation(Inputs, Diags);
assert(CI && "Failed to build compilation invocation.");
+ if (OverlayRealFileSystemForModules)
+ initializeModuleCache(*CI);
+ auto ModuleCacheDeleter = llvm::make_scope_exit(
+ std::bind(deleteModuleCache, CI->getHeaderSearchOpts().ModuleCachePath));
return clang::clangd::buildPreamble(testPath(Filename), *CI, Inputs,
/*StoreInMemory=*/true,
/*PreambleCallback=*/nullptr);
@@ -83,6 +105,11 @@ ParsedAST TestTU::build() const {
StoreDiags Diags;
auto CI = buildCompilerInvocation(Inputs, Diags);
assert(CI && "Failed to build compilation invocation.");
+ if (OverlayRealFileSystemForModules)
+ initializeModuleCache(*CI);
+ auto ModuleCacheDeleter = llvm::make_scope_exit(
+ std::bind(deleteModuleCache, CI->getHeaderSearchOpts().ModuleCachePath));
+
auto Preamble = clang::clangd::buildPreamble(testPath(Filename), *CI, Inputs,
/*StoreInMemory=*/true,
/*PreambleCallback=*/nullptr);
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