Using PrecompiledPreamble in module compilation results in no predefines

[droptopx]

Starting note, I tried to debug this issue as best as I can, but as I had no experience with llvm internals before this, some small parts are not fully clear to me and I would be very glad if someone more knowledgable can come up with a more minimal reproduction of this that doesn't depend on clangd internals.

The bug is what causes this issue on recent versions of clangd:

It seems that any system module imports in an objc file errors. For people who may find this issue from google as I have searched for these keywords rigorously in my debugging endeavors (and yet couldn't find a solution): Could not build module 'Foundation' clang(module_not_built)

After some debugging, I discovered that this issue happens when:

1. An active file imports a module that:
2. Includes a header that:
3. Depends on predefines.

This issue seems like it would affect clang as well, but I have not been able to reproduce this issue from the command line (of clang) as this issue occurs when precompiled preambles (which are not precompiled headers I have come to learn) are used, and from what I can tell there is no way to easily do this from the command line. Please correct me if I'm wrong.

Therefore, I'm reporting this as a clangd issue for the time being. Please see https://github.com/droptopx/clangd_bug_repro which is the minimal setup CMake example that recreates this issue.

System information

Output of clangd --version: clangd version 18.0.0 (https://github.com/llvm/llvm-project.git cae46f6210293ba4d3568eb21b935d438934290d)

Editor/LSP plugin: vscode/llvm-vs-code-extensions.vscode-clangd
Operating system: macOS Sonoma

Cause of Issue

I have pinned down the exact lines that cause this issue.

If provided with a preamble,
clangd::prepareCompilerInstance (Compiler.cpp)
calls clang::PrecompiledPreamble::OverridePreamble
which calls clang::PrecompiledPreamble::configurePreamble
which does PreprocessorOpts.UsePredefines = false

This is because the PrecompiledPreamble is expected to house all the predefines and this optimization skips an unnecessary step during compilation.

The PrecompiledPreamble will then get moved to PreprocessorOptions.ImplicitPCHInclude somewhere along the way but I am not sure where.

Afterwards, if the CompilerInstance returned by clangd::prepareCompilerInstance is used to compile a module, it
will somewhere along the lines call PreprocessorOptions::resetNonModularOptions which calls ImplicitPCHInclude.clear().

The issue here is that PreprocessorOptions::resetNonModularOptions assumes UsePredefines is set to true, i.e. that ImplicitPCHInclude is not the sole houser of the predefines. Though I am not sure if the only two possible housers/generators of the predefines are ImplicitPCHInclude and UsePredefines, respectively. If this is the case I think it would be a good idea to add an assert(UsePredefines) to the body of PreprocessorOptions::resetNonModularOptions.

The function which calls clangd::prepareCompilerInstance with a preamble and causes this issue is ParsedAST::build, but semaCodeComplete in CodeComplete.cpp can also call clangd::prepareCompilerInstance with a preamble, so that function could cause the same issue as well.

One detail that isn't clear to me is why this issue only happens when predefines are used in a header included from a header imported by a module, but not when predefines are used in the header imported by the module itself. If someone could clarify this, I would greatly appreciate it.

Suggested Fix

Two cases are possible here:

1. Only clangd is affected because it wrongly uses preamble defining CompilerInstance's when building modules and this is not valid. Therefore only clangd needs to be fixed.
2. It is possible that clang does this/will do in the future and this is a valid use case for preambles. Therefore Clang needs to be fixed.

I am not sure which is the case here, so I am deferring the answer to someone more knowledgeable.

I'd be glad to help with creating the fix though I am guessing I would need some help from someone more knowledgable, as going through the internals alone is hard.

For the time being though, a dirty patch like the following is possible:

diff --git a/clang-tools-extra/clangd/Compiler.cpp b/clang-tools-extra/clangd/Compiler.cpp
index c60ab8e1b806..23c6532bb979 100644
--- a/clang-tools-extra/clangd/Compiler.cpp
+++ b/clang-tools-extra/clangd/Compiler.cpp
@@ -140,6 +140,7 @@ prepareCompilerInstance(std::unique_ptr<clang::CompilerInvocation> CI,
   // sure it will be released if no error is emitted.
   if (Preamble) {
     Preamble->OverridePreamble(*CI, VFS, Buffer.get());
+    CI->getPreprocessorOpts().UsePredefines = true;
   } else {
     CI->getPreprocessorOpts().addRemappedFile(
         CI->getFrontendOpts().Inputs[0].getFile(), Buffer.get());

[HighCommander4]

Clangd's support for modules is known to be broken, and is actively being worked on in #1293.

(The issue talks about "C++20 modules" and "clang header modules". I assume @import is a syntax for using clang header modules in objective-c, or in any case a variant of the same feature? Apologies if I have this completely wrong, I'm not familiar with objective-c.)

Regarding the specific issue described, cc'ing @ChuanqiXu9, maybe you can advise if this is something you've run into or if the suggested fix is the right one.

[ChuanqiXu9]

I didn't meet the same or similar issue before.

(The issue talks about "C++20 modules" and "clang header modules". I assume @import is a syntax for using clang header modules in objective-c, or in any case a variant of the same feature? Apologies if I have this completely wrong, I'm not familiar with objective-c.)

Yeah, @import should be the syntax in objective-c.

[droptopx]

I can't speak on clangd's support for C++ modules, as I have no experience with them, however the objective-c modules seem to work well, even if this isn't fully intentional (?)

I didn't run across the term "clang header modules" before, I'm guessing they refer to the .modulemap modules¹? In that case, obj-c also uses the same module system, so I'm surprised there are problems with the support clangd has for these. I saw that issue, but didn't realize it was applicable to obj-c modules as well. Other than the bug this issue is about, I didn't find any problems in obj-c module support in my admittedly pretty light testing.

One small issue but not really a bug that could be better addressed is the "go to file"/"go to definition of {module, header}" functionality doesn't work like it does for headers where you can quickly open the header file, but that's another topic for another issue.

Footnotes

1. https://releases.llvm.org/15.0.0/tools/clang/docs/StandardCPlusPlusModules.html#id2 ↩