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, _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
