Correctly set CompilingPCH in PrecompilePreambleAction.

[adamcz]

Summary

This fixes a crash bug in clangd when used with modules. ASTWriter would
end up writing references to submodules into the PCH file, but upon
reading the submodules would not exists and
HeaderFileInfoTrait::ReadData would crash.

[adamcz]

This time the fix is included ;-)

[kadircet]

Regarding tests, it feels like we can also test this in ASTUnitTests which is directly in clang, as it is also using PrecompiledPreamble::Build. What about moving the test there instead?

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

25

nit: indent the string literals by at least 4 spaces. (that's what other tests look like, same below)

29

this one is not needed right?

clang/lib/Frontend/PrecompiledPreamble.cpp

423

anything operating on CompilerInstance before and including the callbacks invoked here will see a broken LangOtps. Why not set it here (and maybe assert in the override, or just not do anything at all)

[adamcz]

addressed review comments

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

29

I suppose not.

clang/lib/Frontend/PrecompiledPreamble.cpp

423

I think we should at least assert() on this, so others don't waste as much time as I did looking for bugs like this. PrecompilePreambleAction is not usable with CompilingPCH = false.

I'm a bit conflicted on setting CompilingPCH outside of PrecompilePreambleAction, it makes it seem like you can change it somehow and it makes that Action class not self-contained (it requires that external bit that sets the options), but with assert it should be fine and it's essentially an internal detail of PrecompilePreamble class anyway.

[kadircet]

thanks, lgtm!

clang/unittests/Frontend/ASTUnitTest.cpp

144

nit: ASSRET_TRUE(AU);

146

ASSERT_TRUE again

[adamcz]

more review comments

clang/unittests/Frontend/ASTUnitTest.cpp

146

You can 't ASSERT_TRUE() on llvm::Expected, since operator bool() is not const. No idea why, but that's why it's assert()

[adamcz]

clang/unittests/Frontend/ASTUnitTest.cpp

146

Nevermind, I do know why - because it changes the "checked" status, that makes sense. I guess we could make that mutable, but that seems out of scope for this change.

[adamcz]

assert() -> ASSERT_TRUE

clang-tools-extra/clangd/unittests/CMakeLists.txt

add_unittest(ClangdUnitTests ClangdTests
   GlobalCompilationDatabaseTests.cpp
   HeadersTests.cpp
   HeaderSourceSwitchTests.cpp
   HoverTests.cpp
   IndexActionTests.cpp
   IndexTests.cpp
   JSONTransportTests.cpp
   LSPClient.cpp
+  ModulesTests.cpp
   ParsedASTTests.cpp
   PathMappingTests.cpp
   PreambleTests.cpp
   PrintASTTests.cpp
   QualityTests.cpp
   RenameTests.cpp
   RIFFTests.cpp
   SelectionTests.cpp

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

+//===-- ModulesTests.cpp  ---------------------------------------*- C++ -*-===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+#include "Annotations.h"
+#include "TestFS.h"
+#include "TestTU.h"
+#include "gmock/gmock.h"
+#include "gtest/gtest.h"
+
+#include <memory>
+#include <string>
+
+namespace clang {
+namespace clangd {
+namespace {
+
+TEST(Modules, TextualIncludeInPreamble) {
+  TestTU TU = TestTU::withCode(R"cpp(
+    #include "Textual.h"
+
+    void foo() {}
+)cpp");
+  TU.ExtraArgs.push_back("-fmodule-name=M");
+  TU.ExtraArgs.push_back("-fmodule-map-file=m.modulemap");
+  TU.AdditionalFiles["Textual.h"] = "void foo();";
+  TU.AdditionalFiles["m.modulemap"] = R"modulemap(
+    module M {
+      module Textual {
+        textual header "Textual.h"
+      }
+    }
+)modulemap";
+  // Test that we do not crash.
+  TU.index();
+}
+
+} // namespace
+} // namespace clangd
+} // namespace clang

clang/lib/Frontend/PrecompiledPreamble.cpp

public:
 
   bool hasEmittedPreamblePCH() const { return HasEmittedPreamblePCH; }
 
   void setEmittedPreamblePCH(ASTWriter &Writer) {
     this->HasEmittedPreamblePCH = true;
     Callbacks.AfterPCHEmitted(Writer);
   }
 
+  bool BeginSourceFileAction(CompilerInstance &CI) override {
+    assert(CI.getLangOpts().CompilingPCH);
+    return ASTFrontendAction::BeginSourceFileAction(CI);
+  }
+
   bool shouldEraseOutputFiles() override { return !hasEmittedPreamblePCH(); }
   bool hasCodeCompletionSupport() const override { return false; }
   bool hasASTFileSupport() const override { return false; }
   TranslationUnitKind getTranslationUnitKind() override { return TU_Prefix; }
 
 private:
   friend class PrecompilePreambleConsumer;
 
llvm::ErrorOr<PrecompiledPreamble> PrecompiledPreamble::Build(
 
   // Create the source manager.
   Clang->setSourceManager(
       new SourceManager(Diagnostics, Clang->getFileManager()));
 
   auto PreambleDepCollector = std::make_shared<PreambleDependencyCollector>();
   Clang->addDependencyCollector(PreambleDepCollector);
 
+  Clang->getLangOpts().CompilingPCH = true;
+
   // Remap the main source file to the preamble buffer.
   StringRef MainFilePath = FrontendOpts.Inputs[0].getFile();
   auto PreambleInputBuffer = llvm::MemoryBuffer::getMemBufferCopy(
       MainFileBuffer->getBuffer().slice(0, Bounds.Size), MainFilePath);
   if (PreprocessorOpts.RetainRemappedFileBuffers) {
     // MainFileBuffer will be deleted by unique_ptr after leaving the method.
     PreprocessorOpts.addRemappedFile(MainFilePath, PreambleInputBuffer.get());
   } else {
     // In that case, remapped buffer will be deleted by CompilerInstance on
     // BeginSourceFile, so we call release() to avoid double deletion.
     PreprocessorOpts.addRemappedFile(MainFilePath,
                                      PreambleInputBuffer.release());
   }
 
   std::unique_ptr<PrecompilePreambleAction> Act;
   Act.reset(new PrecompilePreambleAction(
       StoreInMemory ? &Storage.asMemory().Data : nullptr, Callbacks));
   Callbacks.BeforeExecute(*Clang);
   if (!Act->BeginSourceFile(*Clang.get(), Clang->getFrontendOpts().Inputs[0]))
     return BuildPreambleError::BeginSourceFileFailed;
 
   std::unique_ptr<PPCallbacks> DelegatedPPCallbacks =
       Callbacks.createPPCallbacks();
   if (DelegatedPPCallbacks)
     Clang->getPreprocessor().addPPCallbacks(std::move(DelegatedPPCallbacks));
   if (auto CommentHandler = Callbacks.getCommentHandler())

clang/unittests/Frontend/ASTUnitTest.cpp

 //===- unittests/Frontend/ASTUnitTest.cpp - ASTUnit tests -----------------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 #include <fstream>
 
 #include "clang/Basic/FileManager.h"
 #include "clang/Frontend/ASTUnit.h"
 #include "clang/Frontend/CompilerInstance.h"
 #include "clang/Frontend/CompilerInvocation.h"
 #include "clang/Frontend/PCHContainerOperations.h"
+#include "clang/Lex/HeaderSearch.h"
 #include "llvm/Support/FileSystem.h"
 #include "llvm/Support/Path.h"
 #include "llvm/Support/ToolOutputFile.h"
 #include "gtest/gtest.h"
 
 using namespace llvm;
 using namespace clang;
 
TEST_F(ASTUnitTest, GetBufferForFileMemoryMapping) {
 
   std::unique_ptr<llvm::MemoryBuffer> memoryBuffer =
       AST->getBufferForFile(InputFileName);
 
   EXPECT_NE(memoryBuffer->getBufferKind(),
             llvm::MemoryBuffer::MemoryBuffer_MMap);
 }
 
+TEST_F(ASTUnitTest, ModuleTextualHeader) {
+  llvm::IntrusiveRefCntPtr<llvm::vfs::InMemoryFileSystem> InMemoryFs =
+      new llvm::vfs::InMemoryFileSystem();
+  InMemoryFs->addFile("test.cpp", 0, llvm::MemoryBuffer::getMemBuffer(R"cpp(
+      #include "Textual.h"
+      void foo() {}
+    )cpp"));
+  InMemoryFs->addFile("m.modulemap", 0, llvm::MemoryBuffer::getMemBuffer(R"cpp(
+      module M {
+        module Textual {
+          textual header "Textual.h"
+        }
+      }
+    )cpp"));
+  InMemoryFs->addFile("Textual.h", 0, llvm::MemoryBuffer::getMemBuffer(R"cpp(
+      void foo();
+    )cpp"));
+
+  const char *Args[] = {"clang", "test.cpp", "-fmodule-map-file=m.modulemap",
+                        "-fmodule-name=M"};
+  Diags = CompilerInstance::createDiagnostics(new DiagnosticOptions());
+  CInvok = createInvocationFromCommandLine(Args, Diags);
+  ASSERT_TRUE(CInvok);
+
+  FileManager *FileMgr = new FileManager(FileSystemOptions(), InMemoryFs);
+  PCHContainerOps = std::make_shared<PCHContainerOperations>();
+
+  auto AU = ASTUnit::LoadFromCompilerInvocation(
+      CInvok, PCHContainerOps, Diags, FileMgr, false, CaptureDiagsKind::None, 1,
+      TU_Complete, false, false, false);
+  ASSERT_TRUE(AU);
+  auto File = AU->getFileManager().getFileRef("Textual.h", false, false);
+  assert(File);
+  // Verify that we do not crash here.
+  EXPECT_TRUE(AU->getPreprocessor().getHeaderSearchInfo().getExistingFileInfo(
+      &File->getFileEntry()));
+}
+
 } // anonymous namespace