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

421 ↗

(On Diff #283922)

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

421 ↗

(On Diff #283922)

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 ↗

(On Diff #284329)

nit: ASSRET_TRUE(AU);

146 ↗

(On Diff #284329)

ASSERT_TRUE again

[adamcz]

more review comments

clang/unittests/Frontend/ASTUnitTest.cpp

146 ↗

(On Diff #284329)

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 ↗

(On Diff #284329)

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("-fmodules");
+  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