Get Clang's PCH fixes merged (bounty: $1000)

[jeaye]

Overview

We have an open PR waiting to be merged into LLVM. The PR fixes an issue with PCHs in jank. The PR is here: llvm/llvm-project#94166

However, I've done work to the PR which hasn't been merged. That can be found here: https://github.com/jank-lang/llvm-project/tree/jeaye/original-fix-pch

The test within my branch passes, but when we try to catch this branch up with the latest LLVM, the test no longer passes. I have a branch with all of this merged into the latest LLVM available here: https://github.com/jank-lang/llvm-project/tree/jeaye/fix-pch

The culprit

I have bisected the LLVM commits to find the commit which introduced the breakage in our test. That commit is here: llvm/llvm-project@a72d7eea54134

There are some big changes to the IncrementalParser constructor, which is what I believe to be the issue.

Building Clang

To debug this, you should have two clones of Clang.

1. jeaye/original-fix-pch: The original, working code
2. jeaye/fix-pch: The original patched merged on top of LLVM 22

To build both of them, just use this script (build.sh). It assumes you have python and ccache and ninja installed.

#!/usr/bin/env bash

set -euo pipefail

mkdir -p "build"
cd "build"

cmake -DCMAKE_BUILD_TYPE=Debug \
      -DCMAKE_CXX_COMPILER_LAUNCHER=ccache \
      -DLLVM_ENABLE_RUNTIMES=all \
      -DLLVM_BUILD_LLVM_DYLIB=ON \
      -DLLVM_LINK_LLVM_DYLIB=ON \
      -DLLVM_ENABLE_PROJECTS="clang;clang-tools-extra;compiler-rt" \
      -DLLVM_TARGETS_TO_BUILD="host" \
      -DLLVM_ENABLE_EH=OFF \
      -DLLVM_ENABLE_RTTI=ON \
      -DLLVM_INCLUDE_BENCHMARKS=OFF \
      -DLLVM_ENABLE_BINDINGS=OFF \
      -DLLVM_INCLUDE_EXAMPLES=OFF \
      -DLLVM_INCLUDE_TESTS=ON \
      -DLLVM_ENABLE_ZSTD=OFF \
      -DLLVM_BUILD_OCAML_BINDINGS=OFF \
      -DLLVM_ENABLE_OCAMLDOC=OFF \
      -G "Ninja" \
      ../llvm

ninja clang clang-repl FileCheck -j30

The test (and the PCH problem)

The original PR was written to solve a problem with symbols defined in a PCH not being able to JIT link. The test for this is straightforward.

1. Define a symbol in a header file
2. Compiler the header to a PCH
3. Load the PCH into Clang's JIT runtime
4. Try to reference the symbol

If step 4 passes, we win. In my jeaye/original-fix-pch branch, it passes. In my jeaye/fix-pch branch, it fails. Here are the steps to reproduce this test easily.

The header file

Create a build/include.hpp with this for contents:

int f_pch() { return 5; }

The script

Create a script called test.sh with this for contents:

#!/usr/bin/env bash

set -euo pipefail

cd build
./bin/clang -fmax-type-align=16 -Xclang -pic-level -Xclang 2 -Xclang -fdeprecated-macro -fno-stack-protector -Xclang -fwrapv -fPIC -Xclang -fblocks -Xclang -fskip-odr-check-in-gmf -fexceptions -fcxx-exceptions -fgnuc-version=0 -target x86_64-unknown-linux-gnu -Xclang -fblocks -Xclang -fmax-type-align=8 -Xclang -fincremental-extensions -Xclang -emit-pch -x c++-header -o include.pch include.hpp

./bin/clang-repl -Xcc -fgnuc-version=0 -Xcc -fno-stack-protector -Xcc -fwrapv -Xcc -fPIC -Xcc -fblocks -Xcc -fskip-odr-check-in-gmf -Xcc -fmax-type-align=8 -Xcc -include-pch -Xcc include.pch "f_pch();"

echo "The test passes!"

When you invoke this with Clang/LLVM built from jeaye/original-fix-pch, it should pass.

NOTE: If you're not building on x86_64 Linux, you will want to change the -target flag in the script to match your own target triple.

Recommended steps

We need to figure out how the problem commit (llvm/llvm-project@a72d7eea54134) broke our test case. One way would be to read through the code, but another way would be to step through clang-repl with both builds at the same time, to spot differences. I suspect a combination of the two will be needed.

Support

For support, join the Compiler Research Group discord here: https://discord.gg/8AaAnfc8
Post to the #clang-repl channel and ping me and @vvassilev.

Bounty

I will pay $1000 out of pocket to whoever provides a fix for this, which then leads to this PR being merged into upstream LLVM. This would allow jank to start using packaged LLVM once the next version (22) is released, which would greatly simplify our packaging and distribution. If multiple people have provide fixes, which is unlikely, I will use my discretion to determine who gets the bounty. Payout method is flexible, but Github sponsors is preferred.

If you want to tackle this, leave a comment on this PR so that others can have an idea of how many folks are interested. This would help them gauge whether or not to take a look, since we're not assigning this to one person.

[Flinner]

If you want to tackle this, leave a comment on this PR so that others can have an idea of how many folks are interested. This would help them gauge whether or not to take a look, since we're not assigning this to one person.

I am interested. btw I can't build with clang version 20.1.8 (see: llvm/llvm-project#139072)

I had to change the standard to C++17: -DCMAKE_CXX_STANDARD=17, don't think this affects the build. It was removed in jank (2244b0a) so maybe it should have been removed from here as well.

[Lambeaux]

Just as an FYI, I'm interested but I won't have bandwidth to fully go heads down on this until the weekend.

[kylc]

@jeaye I have a fix for you, but I really know nothing about LLVM so I can't say whether it is fully correct or has any downstream effects on the ongoing work for device offloading/CUDA. At least, I think it points to the root cause of our failure.

The problem stems from some new logic which skips PTU evaluation on interpreter startup if there are no includes here. I guess this is intuitive if the only reason codegen on initialization is necessary is if there are includes, or else all of the source code is to follow. The problem is that PCH includes are not in this list, so if there is a PCH include but no regular includes, then still the codegen on initialization is skipped and the PCH is never evaluated.

You can verify this by additionally passing -Xcc -include -Xcc dummy.hpp to the clang-repl invocation in your test script where dummy.hpp is just an empty file, and you will see that the test now passes.

The fix is very simple: check if there is a PCH include and do the PTU evaluation on startup, just as if there was a regular include.

kylc/jank-lang-llvm-project@65fc0b2

[jeaye]

@jeaye I have a fix for you, but I really know nothing about LLVM so I can't say whether it is fully correct or has any downstream effects on the ongoing work for device offloading/CUDA. At least, I think it points to the root cause of our failure.

The problem stems from some new logic which skips PTU evaluation on interpreter startup if there are no includes here. I guess this is intuitive if the only reason codegen on initialization is necessary is if there are includes, or else all of the source code is to follow. The problem is that PCH includes are not in this list, so if there is a PCH include but no regular includes, then still the codegen on initialization is skipped and the PCH is never evaluated.

You can verify this by additionally passing -Xcc -include -Xcc dummy.hpp to the clang-repl invocation in your test script where dummy.hpp is just an empty file, and you will see that the test now passes.

The fix is very simple: check if there is a PCH include and do the PTU evaluation on startup, just as if there was a regular include.

kylc/jank-lang-llvm-project@65fc0b2

Thanks, Kyle! Excellent work. This makes sense. Flinner also submitted a fix today, which seems to tackle things a bit differently: https://gist.github.com/Flinner/4b79b620b11835f22dbd10bc3cbaac83

I'm not certain which one makes the most sense yet, but I'm surfacing both of these to the Clang dev who originally made the PR to help us and we'll figure out where to go from there.

[Flinner]

🏳️ 🏳️ 🏳️ 🏳️
Nice solution @kylc 😭 , need this as well for latest LLVM

Click to expand error

clang-repl: /home/lambda/code/llvm/llvm-project-1/clang/lib/Interpreter/IncrementalAction.cpp:108: std::unique_ptr<llvm::Module> clang::IncrementalAction::GenModule(): Assertion `((!CachedInCodeGenModule || !CI.getPreprocessorOpts().Includes.empty()) || (CachedInCodeGenModule->empty() && CachedInCodeGenModule->global_empty() && CachedInCodeGenModule->alias_empty() && CachedInCodeGenModule->ifunc_empty())) && "CodeGen wrote to a readonly module"' failed.
 #0 0x00007fde5ee29ad0 llvm::sys::PrintStackTrace(llvm::raw_ostream&, int) /home/lambda/code/llvm/llvm-project-1/llvm/lib/Support/Unix/Signals.inc:834:22
 #1 0x00007fde5ee29f5d PrintStackTraceSignalHandler(void*) /home/lambda/code/llvm/llvm-project-1/llvm/lib/Support/Unix/Signals.inc:918:1
 #2 0x00007fde5ee271dc llvm::sys::RunSignalHandlers() /home/lambda/code/llvm/llvm-project-1/llvm/lib/Support/Signals.cpp:104:20
 #3 0x00007fde5ee29346 SignalHandler(int, siginfo_t*, void*) /home/lambda/code/llvm/llvm-project-1/llvm/lib/Support/Unix/Signals.inc:426:14
 #4 0x00007fde5d83e540 (/usr/lib/libc.so.6+0x3e540)
 #5 0x00007fde5d89894c (/usr/lib/libc.so.6+0x9894c)
 #6 0x00007fde5d83e410 raise (/usr/lib/libc.so.6+0x3e410)
 #7 0x00007fde5d82557a abort (/usr/lib/libc.so.6+0x2557a)
 #8 0x00007fde5d8254e3 __assert_perror_fail (/usr/lib/libc.so.6+0x254e3)
 #9 0x00007fde6e725162 clang::IncrementalAction::GenModule() /home/lambda/code/llvm/llvm-project-1/clang/lib/Interpreter/IncrementalAction.cpp:115:56
#10 0x00007fde6e7371a5 clang::Interpreter::Parse(llvm::StringRef) /home/lambda/code/llvm/llvm-project-1/clang/lib/Interpreter/Interpreter.cpp:481:57
#11 0x00007fde6e737d71 clang::Interpreter::ParseAndExecute(llvm::StringRef, clang::Value*) /home/lambda/code/llvm/llvm-project-1/clang/lib/Interpreter/Interpreter.cpp:578:8
#12 0x00007fde6e73656c clang::Interpreter::create(std::unique_ptr<clang::CompilerInstance, std::default_delete<clang::CompilerInstance>>, std::unique_ptr<llvm::orc::LLJITBuilder, std::default_delete<llvm::orc::LLJITBuilder>>) /home/lambda/code/llvm/llvm-project-1/clang/lib/Interpreter/Interpreter.cpp:362:41
#13 0x00005647591ec4ee main /home/lambda/code/llvm/llvm-project-1/clang/tools/clang-repl/ClangRepl.cpp:340:23
#14 0x00007fde5d827675 (/usr/lib/libc.so.6+0x27675)
#15 0x00007fde5d827729 __libc_start_main (/usr/lib/libc.so.6+0x27729)
#16 0x00005647591ea6a5 _start (./bin/clang-repl+0x86a5)
./test.sh: line 8: 2399448 Aborted                    (core dumped) ./bin/clang-repl -Xcc -fgnuc-version=0 -Xcc -fno-stack-protector -Xcc -fwrapv -Xcc -fPIC -Xcc -fblocks -Xcc -fskip-odr-check-in-gmf -Xcc -fmax-type-align=8 -Xcc -include-pch -Xcc include.pch

diff --git a/clang/lib/Interpreter/IncrementalAction.cpp b/clang/lib/Interpreter/IncrementalAction.cpp
index 4d1bc4c59..3d489fce5 100644
--- a/clang/lib/Interpreter/IncrementalAction.cpp
+++ b/clang/lib/Interpreter/IncrementalAction.cpp
@@ -106,7 +106,8 @@ std::unique_ptr<llvm::Module> IncrementalAction::GenModule() {
     // around we created an empty module to make CodeGen happy. We should make
     // sure it always stays empty.
     assert(((!CachedInCodeGenModule ||
-             !CI.getPreprocessorOpts().Includes.empty()) ||
+             !CI.getPreprocessorOpts().Includes.empty() ||
+             !CI.getPreprocessorOpts().ImplicitPCHInclude.empty()) ||
             (CachedInCodeGenModule->empty() &&
              CachedInCodeGenModule->global_empty() &&
              CachedInCodeGenModule->alias_empty() &&

[jeaye]

There's a PR up for this, with Kyle's fix. It's not yet merged, but I have initial sign off by the Clang dev who wrote this code. llvm/llvm-project#157359

Great job, Kyle and Flinner. 💪 This is something which has been on my plate for months, with me never prioritizing it, and it feels great to have some movement on it now.

[jeaye]

Kyle opted to donate his bounty to the Compiler Research Group, which is the group of folks at CERN who built a lot of the Clang JIT tech which jank is using. I have also contacted the lead of that group to explain why this $1k was donated and from whom it came.

I have made that contribution on his behalf, as shown here: https://opencollective.com/compiler-research/contributions/882379

Thank you, Kyle! Thank you Flinner, as well, for getting another working solution.

The LLVM PR still isn't merged, but it's on me at this point, since the main issue which was preventing the test from passing is now resolved.

[jeaye]

This has been merged! All of jank's pending LLVM changes are now in LLVM main and will be included in LLVM 22, which should release in January. I'll be porting jank away from our own LLVM fork in the coming weeks.