Skip to content

[interpreter] Drop IncrementalExecutor offsets to obtain LLJIT for Cling #589

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
May 23, 2025

Conversation

aaronj0
Copy link
Collaborator

@aaronj0 aaronj0 commented May 22, 2025

This patch drops pointer arithmetic used to obtain the llvm::orc::LLJIT from older Cling versions. This would make for safer usage of the cling::Interpreter and good to have for the 1.7.0 stable release that ROOT will move to.

@aaronj0 aaronj0 requested a review from vgvassilev May 22, 2025 16:31
@mcbarton
Copy link
Collaborator

mcbarton commented May 22, 2025

@aaronj0 When I was looking at this in my PR to drop cling 1.0 support I was planning to replace every occurance of compat::getExecutionEngine in the code (given its now just a return statement). I just never got around to doing it. Maybe you can do it here?

Copy link
Contributor

clang-tidy review says "All clean, LGTM! 👍"

Copy link

codecov bot commented May 22, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 76.43%. Comparing base (fd69d55) to head (3e80d55).
Report is 3 commits behind head on main.

Additional details and impacted files

Impacted file tree graph

@@           Coverage Diff           @@
##             main     #589   +/-   ##
=======================================
  Coverage   76.43%   76.43%           
=======================================
  Files           9        9           
  Lines        3666     3666           
=======================================
  Hits         2802     2802           
  Misses        864      864           
Files with missing lines Coverage Δ
lib/Interpreter/Compatibility.h 91.96% <ø> (ø)
Files with missing lines Coverage Δ
lib/Interpreter/Compatibility.h 91.96% <ø> (ø)
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@mcbarton
Copy link
Collaborator

@aaronj0 Given v1.7.0 just got released without this, do you mind me just doing this patch as part of my PR as part of my effort to drop cling 1.0 support here #581 ?

@aaronj0
Copy link
Collaborator Author

aaronj0 commented May 22, 2025

@aaronj0 Given v1.7.0 just got released without this, do you mind me just doing this patch as part of my PR as part of my effort to drop cling 1.0 support here #581 ?

I prefer to land this here as an atomic change...

@aaronj0 When I was looking at this in my PR to drop cling 1.0 support I was planning to replace every occurance of compat::getExecutionEngine in the code (given its not just a return statement). I just never got around to doing it. Maybe you can do it here?

hmm that doesn't make sense to me:

In a function that accepts a compat::Interpreter :

bool InsertOrReplaceJitSymbol(compat::Interpreter& I,
const char* linker_mangled_name,
uint64_t address) {

That uses this API:

// Let's inject it.
llvm::orc::SymbolMap InjectedSymbols;
auto& DL = compat::getExecutionEngine(I)->getDataLayout();
char GlobalPrefix = DL.getGlobalPrefix();
std::string tmp(linker_mangled_name);

we need the abstraction anyway since clang-repl requires extra handling:

inline llvm::orc::LLJIT* getExecutionEngine(clang::Interpreter& I) {
#if CLANG_VERSION_MAJOR >= 14
auto* engine = &llvm::cantFail(I.getExecutionEngine());
return const_cast<llvm::orc::LLJIT*>(engine);

Another motivation is also to keep the compat:: abstraction layer consistent, and perform operations defined under compat to compat::Interpreter

@aaronj0 aaronj0 merged commit 951775f into compiler-research:main May 23, 2025
78 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants