Adds Out-Of-Process JIT execution for CppInterOp #635

kr-2003 · 2025-06-17T12:11:55Z

Description

Please include a summary of changes, motivation and context for this PR.

Fixes # (issue)

Type of change

Please tick all options which are relevant.

Bug fix
New feature
Requires documentation updates

Testing

Please describe the test(s) that you added and ran to verify your changes.

Checklist

I have read the contribution guide recently

github-actions

clang-tidy made some suggestions

lib/CppInterOp/Compatibility.h

Vipul-Cariappa

Nice work. I guess there will be multiple rounds of review on this.

.github/actions/Build_LLVM/action.yml

.github/workflows/main.yml

Vipul-Cariappa · 2025-06-17T12:23:36Z

CMakeLists.txt

+string(REGEX REPLACE "/build/lib/cmake/llvm$" "" LLVM_SOURCE_DIR "${LLVM_DIR}")
+add_definitions(-DLLVM_SOURCE_DIR="${LLVM_SOURCE_DIR}")


Suggested change

string(REGEX REPLACE "/build/lib/cmake/llvm$" "" LLVM_SOURCE_DIR "${LLVM_DIR}")

add_definitions(-DLLVM_SOURCE_DIR="${LLVM_SOURCE_DIR}")

string(REGEX REPLACE "/build/lib/cmake/llvm$" "" LLVM_BINARY_DIR "${LLVM_DIR}")

add_definitions(-DLLVM_SOURCE_DIR="${LLVM_BINARY_DIR}")

I guess is a better name.

LLVM_BINARY_DIR is already an env variable

Can we reuse LLVM_BINARY_DIR?

lib/CppInterOp/Compatibility.h

lib/CppInterOp/CppInterOp.cpp

Vipul-Cariappa · 2025-06-17T12:31:56Z

unittests/CppInterOp/InterpreterTest.cpp

+#ifdef CPPINTEROP_WITH_OOP_JIT
+TEST(InterpreterTest, OopJITProcess) {
+#ifdef _WIN32
+  GTEST_SKIP() << "Disabled on Windows. Needs fixing.";
+#endif
+  if (llvm::sys::RunningOnValgrind())
+    GTEST_SKIP() << "XFAIL due to Valgrind report";
+  std::vector<const char*> interpreter_args = { "-include", "new" };
+  auto* I = Cpp::CreateInterpreter(interpreter_args, {}, true);
+  EXPECT_TRUE(Cpp::Process("") == 0);
+  EXPECT_TRUE(Cpp::Process("int a = 12;") == 0);
+  EXPECT_FALSE(Cpp::Process("error_here;") == 0);
+  // Linker/JIT error.
+  EXPECT_FALSE(Cpp::Process("int f(); int res = f();") == 0);
+}
+#endif
+


According to this, we only run one test with out-of-process, but we should run all the tests with out-of-process. This should be done through the CI matrix.

yes, i added only this for now to test it out. In the future, all the tests should be ran against out-of-process JIT.

Vipul-Cariappa · 2025-06-17T12:38:46Z

lib/CppInterOp/Compatibility.h

+  std::unique_ptr<llvm::orc::LLJITBuilder> JB;
+
+  if(outOfProcess) {
+      std::string OOPExecutor = std::string(LLVM_SOURCE_DIR) + "/build/bin/llvm-jitlink-executor";


Will this work if LLVM is installed with conda or is installed with the system's package installer.
Could you make sure if working in the other cases too.
You can first check if the file exists; if not, try other paths like the CONDA_PREFIX or /usr/bin.

Vipul-Cariappa · 2025-06-17T12:42:38Z

Hi @mcbarton, do we have cache to add another version of LLVM? For the out-of-process.patch. We need the normal LLVM-20 and LLVM-20-OOP where OOP is for out-of-process?
Could you please help @kr-2003 achieve this?

Vipul-Cariappa · 2025-06-17T12:43:35Z

I would prefer if we could keep this as a draft PR.

github-actions · 2025-06-17T15:13:53Z

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

github-actions · 2025-06-18T04:16:33Z

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

mcbarton · 2025-06-18T08:46:43Z

.github/actions/Build_LLVM/action.yml

+            elif [[ "${{ runner.os }}" == "macOS" ]]; then
+              ninja clang clang-repl llvm-jitlink-executor orc_rt_osx -j ${{ env.ncpus }}
+            fi
+          fi


Why do we have target name for orc_rt difference for MacOS? The target name should be independent of the system your building on.

It is different in LLVM itself. I guess it might have to do with cross-compiling.

mcbarton · 2025-06-18T08:49:20Z

.github/actions/Build_LLVM/action.yml

+          if [[ "${{ matrix.oop-jit }}" == "On" ]]; then
+            git apply -v ../patches/llvm/clang20-1-out-of-process-jit-execution.patch
+            echo "Apply out-of-process-jit-execution.patch:"
+          fi


We should apply this patch regardless of whether matrix.oop-jit=on. If you only apply it if CPPINTEROP_WITH_OOP_JIT=ON , this would imply if you apply the patch, DCPPINTEROP_WITH_OOP_JIT=OFF, then somehow something will go wrong.

Also, why are we not applying this patch on Windows, and testing CPPINTEROP_WITH_OOP_JIT=ON/OFF?

Ok. The thing is, I would want CppInterOp to be compatible with upstream LLVM as well as the patched LLVM. Therefore, I was thinking about having 2 different CI workflows for version 20 with OOP-JIT and without OOP-JIT.
OOP-JIT is not supported on Windows.

Ideally, this is temporary till the patch is directly merged into LLVM and a new version is released.

I could be wrong, and @vgvassilev can correct me if I am wrong, but for such large patches, we have had them merged into llvm first, before applying them to previous versions in CppInterOp.

You could have a new llvm cache like you want. One where you build OOP-JIT and without OOP-JIT. This could only really be done after I find a way to make space in the cache, since we currently are unlikely to have enough space for all these new llvm cache builds. I have an idea on how to free up some space, and just need to find some time to do. I will try and do it tomorrow, as I am busy today.

mcbarton · 2025-06-18T08:52:34Z

.github/actions/Build_and_Test_CppInterOp/action.yml

+                -DCODE_COVERAGE=${{ env.CODE_COVERAGE }}     \
+                -DCMAKE_INSTALL_PREFIX=$CPPINTEROP_DIR       \
+                -DLLVM_ENABLE_WERROR=On                      \
+                -DCPPINTEROP_WITH_OOP_JIT=OFF                \


Feels like CPPINTEROP_WITH_OOP_JIT should have a default value, like we have for cppinterop_use_repl/cppinterop_use_cling

.github/workflows/main.yml

mcbarton · 2025-06-18T08:57:18Z

patches/llvm/clang20-1-out-of-process-jit-execution.patch

@@ -0,0 +1,710 @@
+diff --git a/clang/include/clang/Interpreter/Interpreter.h b/clang/include/clang/Interpreter/Interpreter.h


This is quite a large patch. Can you confirm that it has been upsteamed so will appear in llvm 21?

unittests/CppInterOp/InterpreterTest.cpp

README.md

.github/actions/Build_and_Test_CppInterOp/action.yml

.github/actions/Build_LLVM/action.yml

CMakeLists.txt

github-actions

clang-tidy made some suggestions

include/CppInterOp/CppInterOp.h

lib/CppInterOp/Compatibility.h

lib/CppInterOp/CppInterOp.cpp

github-actions · 2025-06-20T06:38:59Z

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

codecov · 2025-06-20T07:23:41Z

Codecov Report

Attention: Patch coverage is 44.44444% with 5 lines in your changes missing coverage. Please review.

Project coverage is 78.59%. Comparing base (130dd30) to head (43220d7).
Report is 1 commits behind head on main.

Files with missing lines	Patch %	Lines
lib/CppInterOp/Compatibility.h	28.57%	5 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #635      +/-   ##
==========================================
- Coverage   78.69%   78.59%   -0.10%     
==========================================
  Files           9        9              
  Lines        3835     3841       +6     
==========================================
+ Hits         3018     3019       +1     
- Misses        817      822       +5

Files with missing lines	Coverage Δ
include/CppInterOp/CppInterOp.h	`95.34% <ø> (ø)`
lib/CppInterOp/CppInterOp.cpp	`86.58% <100.00%> (ø)`
lib/CppInterOp/CppInterOpInterpreter.h	`86.97% <100.00%> (ø)`
lib/CppInterOp/Compatibility.h	`88.70% <28.57%> (-3.67%)`	⬇️

Files with missing lines	Coverage Δ
include/CppInterOp/CppInterOp.h	`95.34% <ø> (ø)`
lib/CppInterOp/CppInterOp.cpp	`86.58% <100.00%> (ø)`
lib/CppInterOp/CppInterOpInterpreter.h	`86.97% <100.00%> (ø)`
lib/CppInterOp/Compatibility.h	`88.70% <28.57%> (-3.67%)`	⬇️

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Vipul-Cariappa · 2025-06-20T11:46:49Z

patches/llvm/clang20-2-out-of-process-jit-execution.patch

+launchExecutor(llvm::StringRef ExecutablePath, bool UseSharedMemory,
+               llvm::StringRef SlabAllocateSizeString);


Suggested change

+launchExecutor(llvm::StringRef ExecutablePath, bool UseSharedMemory,

+ llvm::StringRef SlabAllocateSizeString);

+launchExecutor(llvm::StringRef ExecutablePath, bool UseSharedMemory,

+ llvm::StringRef SlabAllocateSizeString, int stdin_fd = 0, int stdout_fd = 1, int stderr_fd = 2);

Vipul-Cariappa · 2025-06-20T11:48:06Z

patches/llvm/clang20-2-out-of-process-jit-execution.patch

+    dup2(StdoutPipe[WriteEnd], STDOUT_FILENO);
+    dup2(StderrPipe[WriteEnd], STDERR_FILENO);


Suggested change

+ dup2(StdoutPipe[WriteEnd], STDOUT_FILENO);

+ dup2(StderrPipe[WriteEnd], STDERR_FILENO);

+ if (stdout_fd != 1) ...

+ dup2(stdout_fd, STDOUT_FILENO);

+ dup2(StderrPipe[WriteEnd], STDERR_FILENO);

kr-2003 added 5 commits June 17, 2025 16:56

addition of oop jit execution

5a1cac3

minor change

eea2a1c

merge with main

3ac5647

deleting extra workflows

cc3f601

adding compiler-rt

4396765

Vipul-Cariappa self-requested a review June 17, 2025 12:14

github-actions bot reviewed Jun 17, 2025

View reviewed changes

lib/CppInterOp/Compatibility.h Outdated Show resolved Hide resolved

lib/CppInterOp/Compatibility.h Outdated Show resolved Hide resolved

lib/CppInterOp/Compatibility.h Show resolved Hide resolved

Vipul-Cariappa requested changes Jun 17, 2025

View reviewed changes

Vipul-Cariappa requested a review from mcbarton June 17, 2025 12:40

Vipul-Cariappa marked this pull request as draft June 17, 2025 12:42

kr-2003 added 2 commits June 17, 2025 20:25

request changes addressed

5536edf

request changes addressed

fac8d38

change workflow

9510241

mcbarton requested changes Jun 18, 2025

View reviewed changes

mcbarton reviewed Jun 18, 2025

View reviewed changes

.github/actions/Build_and_Test_CppInterOp/action.yml Outdated Show resolved Hide resolved

.github/actions/Build_LLVM/action.yml Outdated Show resolved Hide resolved

CMakeLists.txt Show resolved Hide resolved

request changes addressed

34d5670

github-actions bot reviewed Jun 20, 2025

View reviewed changes

include/CppInterOp/CppInterOp.h Show resolved Hide resolved

lib/CppInterOp/Compatibility.h Show resolved Hide resolved

lib/CppInterOp/Compatibility.h Show resolved Hide resolved

lib/CppInterOp/CppInterOp.cpp Show resolved Hide resolved

kr-2003 added 3 commits June 20, 2025 11:33

changed patch

e562d13

added oop jit check for executorpid func

13a0caf

added oop jit check for executorpid func

43220d7

Vipul-Cariappa reviewed Jun 20, 2025

View reviewed changes

		string(REGEX REPLACE "/build/lib/cmake/llvm$" "" LLVM_SOURCE_DIR "${LLVM_DIR}")
		add_definitions(-DLLVM_SOURCE_DIR="${LLVM_SOURCE_DIR}")

		@@ -0,0 +1,710 @@
		diff --git a/clang/include/clang/Interpreter/Interpreter.h b/clang/include/clang/Interpreter/Interpreter.h

		+launchExecutor(llvm::StringRef ExecutablePath, bool UseSharedMemory,
		+ llvm::StringRef SlabAllocateSizeString);

		+ dup2(StdoutPipe[WriteEnd], STDOUT_FILENO);
		+ dup2(StderrPipe[WriteEnd], STDERR_FILENO);

Adds Out-Of-Process JIT execution for CppInterOp #635

Are you sure you want to change the base?

Adds Out-Of-Process JIT execution for CppInterOp #635

Uh oh!

Conversation

kr-2003 commented Jun 17, 2025

Description

Type of change

Testing

Checklist

Uh oh!

github-actions bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Vipul-Cariappa left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Vipul-Cariappa commented Jun 17, 2025

Uh oh!

Vipul-Cariappa commented Jun 17, 2025

Uh oh!

github-actions bot commented Jun 17, 2025

Uh oh!

github-actions bot commented Jun 18, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mcbarton Jun 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

github-actions bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

github-actions bot commented Jun 20, 2025

Uh oh!

codecov bot commented Jun 20, 2025

Codecov Report

Uh oh!

mcbarton Jun 18, 2025 •

edited

Loading