[clangd] Revert "[clangd] Fix crash-bug in preamble indexing when using modules."

[adamcz]

Summary

This reverts commit 4061d9e42cff621462931ac7df9666806c77a237.
Tests are failing in some configuration, likely due to not cleaning up
module cache path before running the test.

clang-tools-extra/clangd/unittests/SymbolCollectorTests.cpp

 
 TEST_F(SymbolCollectorTest, MacrosInHeaders) {
   CollectorOpts.CollectMacro = true;
   TestFileName = testPath("test.h");
   runSymbolCollector("", "#define X");
   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

clang-tools-extra/clangd/unittests/TestFS.h

 llvm::IntrusiveRefCntPtr<llvm::vfs::FileSystem>
 buildTestFS(llvm::StringMap<std::string> const &Files,
             llvm::StringMap<time_t> const &Timestamps = {});
 
 // A VFS provider that returns TestFSes containing a provided set of files.
 class MockFS : public ThreadsafeFS {
 public:
   IntrusiveRefCntPtr<llvm::vfs::FileSystem> viewImpl() const override {
-    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;
+    return buildTestFS(Files, Timestamps);
   }
 
   // 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.
 class MockCompilationDatabase : public GlobalCompilationDatabase {
 public:
   /// If \p Directory is not empty, use that as the Directory field of the
   /// CompileCommand, and as project SourceRoot.
   ///

clang-tools-extra/clangd/unittests/TestTU.h

struct TestTU {
   llvm::Optional<std::string> ClangTidyChecks;
   llvm::Optional<std::string> ClangTidyWarningsAsErrors;
   // Index to use when building AST.
   const SymbolIndex *ExternalIndex = nullptr;
 
   // 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;
   std::shared_ptr<const PreambleData> preamble() const;
   ParseInputs inputs(MockFS &FS) const;
   SymbolSlab headerSymbols() const;
   RefSlab headerRefs() const;
   std::unique_ptr<SymbolIndex> index() const;

clang-tools-extra/clangd/unittests/TestTU.cpp

ParseInputs TestTU::inputs(MockFS &FS) const {
   }
   Argv.insert(Argv.end(), ExtraArgs.begin(), ExtraArgs.end());
   // Put the file name at the end -- this allows the extra arg (-xc++) to
   // override the language setting.
   Argv.push_back(FullFilename);
   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;
   Inputs.Opts.PreserveRecoveryASTType = true;
   Inputs.Opts.ClangTidyOpts.Checks = ClangTidyChecks;
   Inputs.Opts.ClangTidyOpts.WarningsAsErrors = ClangTidyWarningsAsErrors;
   Inputs.Index = ExternalIndex;
   if (Inputs.Index)

clang/lib/Index/IndexingAction.cpp

 
 static void indexTranslationUnit(ASTUnit &Unit, IndexingContext &IndexCtx) {
   Unit.visitLocalTopLevelDecls(&IndexCtx, topLevelDeclVisitor);
 }
 
 static void indexPreprocessorMacros(const Preprocessor &PP,
                                     IndexDataConsumer &DataConsumer) {
   for (const auto &M : PP.macros())
-    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;
-
+    if (MacroDirective *MD = M.second.getLatest())
       DataConsumer.handleMacroOccurrence(
           M.first, MD->getMacroInfo(),
           static_cast<unsigned>(index::SymbolRole::Definition),
           MD->getLocation());
-    }
-}
 
 void index::indexASTUnit(ASTUnit &Unit, IndexDataConsumer &DataConsumer,
                          IndexingOptions Opts) {
   IndexingContext IndexCtx(Opts, DataConsumer);
   IndexCtx.setASTContext(Unit.getASTContext());
   DataConsumer.initialize(Unit.getASTContext());
   DataConsumer.setPreprocessor(Unit.getPreprocessorPtr());