[clang-repl] Teach clang-repl how to load PCHs (reprise) #157359
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Thank you for submitting a Pull Request (PR) to the LLVM Project! This PR will be automatically labeled and the relevant teams will be notified. If you wish to, you can add reviewers by using the "Reviewers" section on this page. If this is not working for you, it is probably because you do not have write permissions for the repository. In which case you can instead tag reviewers by name in a comment by using If you have received no comments on your PR for a week, you can request a review by "ping"ing the PR by adding a comment “Ping”. The common courtesy "ping" rate is once a week. Please remember that you are asking for valuable time from other developers. If you have further questions, they may be answered by the LLVM GitHub User Guide. You can also ask questions in a comment on this PR, on the LLVM Discord or on the forums. |
llvmbot
added
clang
|
@llvm/pr-subscribers-clang @llvm/pr-subscribers-clang-codegen Author: Jeaye Wilkerson (jeaye) ChangesThis is an updated version of @vgvassilev's PR from last year here: #94166 In short, it includes:
Full diff: https://github.com/llvm/llvm-project/pull/157359.diff 10 Files Affected:
diff --git a/clang/include/clang/CodeGen/ModuleBuilder.h b/clang/include/clang/CodeGen/ModuleBuilder.h
index 59b9840d02e08..f1b8229edd362 100644
--- a/clang/include/clang/CodeGen/ModuleBuilder.h
+++ b/clang/include/clang/CodeGen/ModuleBuilder.h
@@ -52,6 +52,12 @@ namespace CodeGen {
class CodeGenerator : public ASTConsumer {
virtual void anchor();
+protected:
+ /// True if we've finished generating IR. This prevents us from generating
+ /// additional LLVM IR after emitting output in HandleTranslationUnit. This
+ /// can happen when Clang plugins trigger additional AST deserialization.
+ bool IRGenFinished = false;
+
public:
/// Return an opaque reference to the CodeGenModule object, which can
/// be used in various secondary APIs. It is valid as long as the
diff --git a/clang/lib/CodeGen/BackendConsumer.h b/clang/lib/CodeGen/BackendConsumer.h
index ad3adfca36785..b7bbb81074836 100644
--- a/clang/lib/CodeGen/BackendConsumer.h
+++ b/clang/lib/CodeGen/BackendConsumer.h
@@ -40,11 +40,6 @@ class BackendConsumer : public ASTConsumer {
llvm::Timer LLVMIRGeneration;
unsigned LLVMIRGenerationRefCount = 0;
- /// True if we've finished generating IR. This prevents us from generating
- /// additional LLVM IR after emitting output in HandleTranslationUnit. This
- /// can happen when Clang plugins trigger additional AST deserialization.
- bool IRGenFinished = false;
-
bool TimerIsEnabled = false;
BackendAction Action;
diff --git a/clang/lib/CodeGen/CodeGenAction.cpp b/clang/lib/CodeGen/CodeGenAction.cpp
index dc54c97eeae8e..db38351118f26 100644
--- a/clang/lib/CodeGen/CodeGenAction.cpp
+++ b/clang/lib/CodeGen/CodeGenAction.cpp
@@ -189,9 +189,7 @@ void BackendConsumer::HandleInlineFunctionDefinition(FunctionDecl *D) {
}
void BackendConsumer::HandleInterestingDecl(DeclGroupRef D) {
- // Ignore interesting decls from the AST reader after IRGen is finished.
- if (!IRGenFinished)
- HandleTopLevelDecl(D);
+ HandleTopLevelDecl(D);
}
// Links each entry in LinkModules into our module. Returns true on error.
@@ -240,10 +238,11 @@ void BackendConsumer::HandleTranslationUnit(ASTContext &C) {
Gen->HandleTranslationUnit(C);
- if (TimerIsEnabled && !--LLVMIRGenerationRefCount)
- LLVMIRGeneration.yieldTo(CI.getFrontendTimer());
-
- IRGenFinished = true;
+ if (TimerIsEnabled) {
+ LLVMIRGenerationRefCount -= 1;
+ if (LLVMIRGenerationRefCount == 0)
+ LLVMIRGeneration.stopTimer();
+ }
}
// Silently ignore if we weren't initialized for some reason.
diff --git a/clang/lib/CodeGen/ModuleBuilder.cpp b/clang/lib/CodeGen/ModuleBuilder.cpp
index 8c1fee8c974f1..3eb0595864c6c 100644
--- a/clang/lib/CodeGen/ModuleBuilder.cpp
+++ b/clang/lib/CodeGen/ModuleBuilder.cpp
@@ -138,6 +138,8 @@ namespace {
assert(!M && "Replacing existing Module?");
M.reset(new llvm::Module(ExpandModuleName(ModuleName, CodeGenOpts), C));
+ IRGenFinished = false;
+
std::unique_ptr<CodeGenModule> OldBuilder = std::move(Builder);
Initialize(*Ctx);
@@ -179,6 +181,10 @@ namespace {
}
bool HandleTopLevelDecl(DeclGroupRef DG) override {
+ // Ignore interesting decls from the AST reader after IRGen is finished.
+ if (IRGenFinished)
+ return true; // We can't CodeGen more but pass to other consumers.
+
// FIXME: Why not return false and abort parsing?
if (Diags.hasUnrecoverableErrorOccurred())
return true;
@@ -282,6 +288,7 @@ namespace {
}
void HandleTranslationUnit(ASTContext &Ctx) override {
+
// Release the Builder when there is no error.
if (!Diags.hasUnrecoverableErrorOccurred() && Builder)
Builder->Release();
@@ -292,8 +299,9 @@ namespace {
if (Builder)
Builder->clear();
M.reset();
- return;
}
+
+ IRGenFinished = true;
}
void AssignInheritanceModel(CXXRecordDecl *RD) override {
diff --git a/clang/lib/Interpreter/IncrementalAction.cpp b/clang/lib/Interpreter/IncrementalAction.cpp
index 4d1bc4c59e851..3d489fce54bc6 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() &&
diff --git a/clang/lib/Interpreter/IncrementalParser.cpp b/clang/lib/Interpreter/IncrementalParser.cpp
index 32d1663fbe1a9..bf08911e23533 100644
--- a/clang/lib/Interpreter/IncrementalParser.cpp
+++ b/clang/lib/Interpreter/IncrementalParser.cpp
@@ -37,6 +37,10 @@ IncrementalParser::IncrementalParser(CompilerInstance &Instance,
llvm::ErrorAsOutParameter EAO(&Err);
Consumer = &S.getASTConsumer();
P.reset(new Parser(S.getPreprocessor(), S, /*SkipBodies=*/false));
+
+ if (ExternalASTSource *External = S.getASTContext().getExternalSource())
+ External->StartTranslationUnit(Consumer);
+
P->Initialize();
}
diff --git a/clang/lib/Interpreter/Interpreter.cpp b/clang/lib/Interpreter/Interpreter.cpp
index 043e0c1e5754e..8944937f19115 100644
--- a/clang/lib/Interpreter/Interpreter.cpp
+++ b/clang/lib/Interpreter/Interpreter.cpp
@@ -276,9 +276,10 @@ Interpreter::Interpreter(std::unique_ptr<CompilerInstance> Instance,
if (Act->getCodeGen()) {
Act->CacheCodeGenModule();
- // The initial PTU is filled by `-include` or by CUDA includes
- // automatically.
- if (!CI->getPreprocessorOpts().Includes.empty()) {
+ // The initial PTU is filled by `-include`/`-include-pch` or by CUDA
+ // includes automatically.
+ if (!CI->getPreprocessorOpts().Includes.empty() ||
+ !CI->getPreprocessorOpts().ImplicitPCHInclude.empty()) {
// We can't really directly pass the CachedInCodeGenModule to the Jit
// because it will steal it, causing dangling references as explained in
// Interpreter::Execute
diff --git a/clang/test/Interpreter/execute-pch.cpp b/clang/test/Interpreter/execute-pch.cpp
new file mode 100644
index 0000000000000..1ee8221463977
--- /dev/null
+++ b/clang/test/Interpreter/execute-pch.cpp
@@ -0,0 +1,23 @@
+// REQUIRES: host-supports-jit
+// UNSUPPORTED: system-aix
+//
+// RUN: rm -rf %t
+// RUN: mkdir -p %t
+// RUN: split-file %s %t
+//
+// RUN: %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 %host-jit-triple -Xclang -fblocks -Xclang -fmax-type-align=8 -Xclang -fincremental-extensions -Xclang -emit-pch -x c++-header -o %t/include.pch %t/include.hpp
+//
+// RUN: cat %t/main.cpp \
+// RUN: | 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 %t/include.pch \
+// RUN: | FileCheck %s
+
+//--- include.hpp
+
+int f_pch() { return 5; }
+
+//--- main.cpp
+
+extern "C" int printf(const char *, ...);
+printf("f_pch = %d\n", f_pch());
+
+// CHECK: f_pch = 5
diff --git a/clang/test/lit.cfg.py b/clang/test/lit.cfg.py
index d34319131ab6d..bec7f70517471 100644
--- a/clang/test/lit.cfg.py
+++ b/clang/test/lit.cfg.py
@@ -191,6 +191,10 @@ def have_host_clang_repl_cuda():
if have_host_clang_repl_cuda():
config.available_features.add('host-supports-cuda')
+ hosttriple = run_clang_repl("--host-jit-triple")
+ config.substitutions.append(
+ ("%host-jit-triple", hosttriple.strip())
+ )
if have_host_out_of_process_jit_feature_support():
config.available_features.add("host-supports-out-of-process-jit")
diff --git a/clang/tools/clang-repl/ClangRepl.cpp b/clang/tools/clang-repl/ClangRepl.cpp
index 1d508816d7047..c7879422cd7df 100644
--- a/clang/tools/clang-repl/ClangRepl.cpp
+++ b/clang/tools/clang-repl/ClangRepl.cpp
@@ -85,6 +85,8 @@ static llvm::cl::list<std::string>
llvm::cl::CommaSeparated);
static llvm::cl::opt<bool> OptHostSupportsJit("host-supports-jit",
llvm::cl::Hidden);
+static llvm::cl::opt<bool> OptHostJitTriple("host-jit-triple",
+ llvm::cl::Hidden);
static llvm::cl::list<std::string> OptInputs(llvm::cl::Positional,
llvm::cl::desc("[code to run]"));
@@ -279,6 +281,11 @@ int main(int argc, const char **argv) {
llvm::outs() << "false\n";
}
return 0;
+ } else if (OptHostJitTriple) {
+ auto J = ExitOnErr(llvm::orc::LLJITBuilder().create());
+ auto T = J->getTargetTriple();
+ llvm::outs() << T.normalize() << '\n';
+ return 0;
}
clang::IncrementalCompilerBuilder CB;
|
|
✅ With the latest revision this PR passed the Python code formatter. |
jeaye
force-pushed
the
jeaye/fix-pch-pr
branch
2 times, most recently
from
7779512 to
42019e6
Compare
jeaye
force-pushed
the
jeaye/fix-pch-pr
branch
from
42019e6 to
3ec38ab
Compare
|
Looks like the previous run of the Linux job failed in an unrelated area: https://productionresultssa6.blob.core.windows.net/actions-results/de1b4c8c-9dc9-4d5e-90f7-2ed4ede76dce/workflow-job-run-20b49bb8-304c-5069-9f6b-84a81382ac9a/logs/job/job-logs.txt?rsct=text%2Fplain&se=2025-09-07T20%3A05%3A27Z&sig=WPuhDVUUJD9i8lxrEkJACxl4ITjw1d7KGiI6QENb4Gg%3D&ske=2025-09-08T06%3A16%3A06Z&skoid=ca7593d4-ee42-46cd-af88-8b886a2f84eb&sks=b&skt=2025-09-07T18%3A16%3A06Z&sktid=398a6654-997b-47e9-b12b-9515b896b4de&skv=2025-05-05&sp=r&spr=https&sr=b&st=2025-09-07T19%3A55%3A22Z&sv=2025-05-05 However, the Windows failure was related to my changes and has been addressed. Locally (x64_64 Linux), the regression suite passes cleanly. |
|
I've scoured the latest Linux/Windows build+test logs and the failures in there have seemingly nothing to do with this PR. Based on comparing these results with other recently opened PRs, it's unclear to me if these are consistent. @vgvassilev do you know? |
Probably somebody break llvm. You can rebase probably tomorrow... |
vgvassilev
reviewed
Sep 16, 2025
vgvassilev
reviewed
Sep 16, 2025
jeaye
force-pushed
the
jeaye/fix-pch-pr
branch
from
54ad640 to
b0943f2
Compare
vgvassilev
approved these changes
Sep 16, 2025
There was a problem hiding this comment.
LGTM!
@AaronBallman, I'd appreciate if you take a look before we land this.
erichkeane
approved these changes
Sep 27, 2025
clang/lib/CodeGen/ModuleBuilder.cpp
Outdated
| @@ -282,6 +288,7 @@ namespace { | |||
| } | |||
|
|
|||
| void HandleTranslationUnit(ASTContext &Ctx) override { | |||
|
|
|||
There was a problem hiding this comment.
Yep, removed now. Thanks for the review!
|
@jeaye Congratulations on having your first Pull Request (PR) merged into the LLVM Project! Your changes will be combined with recent changes from other authors, then tested by our build bots. If there is a problem with a build, you may receive a report in an email or a comment on this PR. Please check whether problems have been caused by your change specifically, as the builds can include changes from many authors. It is not uncommon for your change to be included in a build that fails due to someone else's changes, or infrastructure issues. How to do this, and the rest of the post-merge process, is covered in detail here. If your change does cause a problem, it may be reverted, or you can revert it yourself. This is a normal part of LLVM development. You can fix your changes and open a new PR to merge them again. If you don't get any reports, no action is required from you. Your changes are working as expected, well done! |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/11/builds/25024 Here is the relevant piece of the build log for the reference |
jeaye
added a commit
to jank-lang/llvm-project
that referenced
this pull request
Sep 30, 2025
On the happy path, when `clang-repl` is present, we will invoke it in order to determine if the host supports JIT features. That will return a string containing "true". However, in cases where `clang-repl` is not present or we fail to invoke it, we previously returned `False`, which would then trigger a failure with our substring check. This PR updates the function to return `""` instead, so the substring check is still valid. This is related to llvm#157359, where the original change was introduced.
vgvassilev
pushed a commit
that referenced
this pull request
Sep 30, 2025
On the happy path, when `clang-repl` is present, we will invoke it in order to determine if the host supports JIT features. That will return a string containing "true". However, in cases where `clang-repl` is not present or we fail to invoke it, we previously returned `False`, which would then trigger a failure with our substring check. This PR updates the function to return `""` instead, so the substring check is still valid. This is related to #157359, where the original change was introduced.
llvm-sync bot
pushed a commit
to arm/arm-toolchain
that referenced
this pull request
Sep 30, 2025
On the happy path, when `clang-repl` is present, we will invoke it in order to determine if the host supports JIT features. That will return a string containing "true". However, in cases where `clang-repl` is not present or we fail to invoke it, we previously returned `False`, which would then trigger a failure with our substring check. This PR updates the function to return `""` instead, so the substring check is still valid. This is related to llvm/llvm-project#157359, where the original change was introduced.
mahesh-attarde
pushed a commit
to mahesh-attarde/llvm-project
that referenced
this pull request
Oct 3, 2025
This is an updated version of @vgvassilev's PR from last year here: llvm#94166 In short, it includes: 1. The fix for a blocking issue where `clang::Interpreter` (and thus `clang-repl`) cannot resolve symbols defined in a PCH 2. A test to prove this is working 3. A new hidden flag for `clang-repl` so that `llvm-lit` can match the host JIT triple between the PCH and `clang-repl`; previously, they may differ in some cases 4. Everything based on the latest LLVM main Shout out to @kylc for finding a logic issue which had us stumped for a while (and securing the [bounty](jank-lang/jank#446)). --------- Co-authored-by: Vassil Vassilev <v.g.vassilev@gmail.com> Co-authored-by: Kyle Cesare <kcesare@gmail.com>
mahesh-attarde
pushed a commit
to mahesh-attarde/llvm-project
that referenced
this pull request
Oct 3, 2025
On the happy path, when `clang-repl` is present, we will invoke it in order to determine if the host supports JIT features. That will return a string containing "true". However, in cases where `clang-repl` is not present or we fail to invoke it, we previously returned `False`, which would then trigger a failure with our substring check. This PR updates the function to return `""` instead, so the substring check is still valid. This is related to llvm#157359, where the original change was introduced.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
6 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This is an updated version of @vgvassilev's PR from last year here: #94166
In short, it includes:
clang::Interpreter(and thusclang-repl) cannot resolve symbols defined in a PCHclang-replso thatllvm-litcan match the host JIT triple between the PCH andclang-repl; previously, they may differ in some casesShout out to @kylc for finding a logic issue which had us stumped for a while (and securing the bounty).