[GSoC 2025] Enabling Out-Of-Process JIT Execution in CppInterOp #658
Conversation
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| size_t newLen = | ||
| fread(content.get(), sizeof(char), bufsize, m_TempFile.get()); | ||
| if (ferror(m_TempFile.get()) != 0) | ||
| size_t newLen = fread(content.get(), sizeof(char), bufsize, m_TempFile); |
There was a problem hiding this comment.
warning: The 1st argument to 'fread' is a buffer with size 0 but should be a buffer with size equal to or greater than the value of the 2nd argument (which is 1) times the 3rd argument (which is 18446744073709551615) [clang-analyzer-unix.StdCLibraryFunctions]
size_t newLen = fread(content.get(), sizeof(char), bufsize, m_TempFile);
^Additional context
lib/CppInterOp/CppInterOp.cpp:3941: Calling 'StreamCaptureInfo::GetCapturedString'
std::string result = SCI.GetCapturedString();
^lib/CppInterOp/CppInterOp.cpp:3888: Taking false branch
if (dup2(m_DupFD, m_FD) < 0)
^lib/CppInterOp/CppInterOp.cpp:3891: Taking false branch
if (fseek(m_TempFile, 0L, SEEK_END) != 0)
^lib/CppInterOp/CppInterOp.cpp:3895: Assuming that 'ftell' fails
long bufsize = ftell(m_TempFile);
^lib/CppInterOp/CppInterOp.cpp:3895: 'bufsize' initialized here
long bufsize = ftell(m_TempFile);
^lib/CppInterOp/CppInterOp.cpp:3896: Taking true branch
if (bufsize == -1)
^lib/CppInterOp/CppInterOp.cpp:3900: Storing uninitialized value
std::unique_ptr<char[]> content(new char[bufsize + 1]);
^lib/CppInterOp/CppInterOp.cpp:3900: Passing value via 1st parameter '__p'
std::unique_ptr<char[]> content(new char[bufsize + 1]);
^lib/CppInterOp/CppInterOp.cpp:3900: Calling constructor for 'unique_ptr<char[], std::default_delete<char[]>>'
std::unique_ptr<char[]> content(new char[bufsize + 1]);
^/usr/include/c++/13/bits/unique_ptr.h:603: Calling constructor for '__uniq_ptr_data<char, std::default_delete<char[]>, true, true>'
: _M_t(__p)
^/usr/include/c++/13/bits/unique_ptr.h:603: Passing '' via 1st parameter '__p'
: _M_t(__p)
^/usr/include/c++/13/bits/unique_ptr.h:603: Calling constructor for '__uniq_ptr_impl<char, std::default_delete<char[]>>'
: _M_t(__p)
^/usr/include/c++/13/bits/unique_ptr.h:175: Calling '__uniq_ptr_impl::_M_ptr'
__uniq_ptr_impl(pointer __p) : _M_t() { _M_ptr() = __p; }
^/usr/include/c++/13/bits/unique_ptr.h:196: Calling 'get<0UL, char *, std::default_delete<char[]>>'
pointer& _M_ptr() noexcept { return std::get<0>(_M_t); }
^/usr/include/c++/13/tuple:1803: Calling '__get_helper<0UL, char *, std::default_delete<char[]>>'
{ return std::__get_helper<__i>(__t); }
^/usr/include/c++/13/tuple:1787: Calling '_Tuple_impl::_M_head'
{ return _Tuple_impl<__i, _Head, _Tail...>::_M_head(__t); }
^/usr/include/c++/13/tuple:268: Calling '_Head_base::_M_head'
_M_head(_Tuple_impl& __t) noexcept { return _Base::_M_head(__t); }
^/usr/include/c++/13/tuple:268: Returning from '_Head_base::_M_head'
_M_head(_Tuple_impl& __t) noexcept { return _Base::_M_head(__t); }
^/usr/include/c++/13/tuple:1787: Returning from '_Tuple_impl::_M_head'
{ return _Tuple_impl<__i, _Head, _Tail...>::_M_head(__t); }
^/usr/include/c++/13/tuple:1803: Returning from '__get_helper<0UL, char *, std::default_delete<char[]>>'
{ return std::__get_helper<__i>(__t); }
^/usr/include/c++/13/bits/unique_ptr.h:196: Returning from 'get<0UL, char *, std::default_delete<char[]>>'
pointer& _M_ptr() noexcept { return std::get<0>(_M_t); }
^/usr/include/c++/13/bits/unique_ptr.h:175: Returning from '__uniq_ptr_impl::_M_ptr'
__uniq_ptr_impl(pointer __p) : _M_t() { _M_ptr() = __p; }
^/usr/include/c++/13/bits/unique_ptr.h:175: The value of '__p' is assigned to 'content._M_t._M_t._M_head_impl'
__uniq_ptr_impl(pointer __p) : _M_t() { _M_ptr() = __p; }
^/usr/include/c++/13/bits/unique_ptr.h:603: Returning from constructor for '__uniq_ptr_impl<char, std::default_delete<char[]>>'
: _M_t(__p)
^/usr/include/c++/13/bits/unique_ptr.h:603: Returning from constructor for '__uniq_ptr_data<char, std::default_delete<char[]>, true, true>'
: _M_t(__p)
^lib/CppInterOp/CppInterOp.cpp:3900: Returning from constructor for 'unique_ptr<char[], std::default_delete<char[]>>'
std::unique_ptr<char[]> content(new char[bufsize + 1]);
^lib/CppInterOp/CppInterOp.cpp:3903: Taking false branch
if (fseek(m_TempFile, 0L, SEEK_SET) != 0)
^lib/CppInterOp/CppInterOp.cpp:3907: The 1st argument to 'fread' is a buffer with size 0 but should be a buffer with size equal to or greater than the value of the 2nd argument (which is 1) times the 3rd argument (which is 18446744073709551615)
size_t newLen = fread(content.get(), sizeof(char), bufsize, m_TempFile);
^
| # Omitting CUDATest.cpp since Emscripten build currently has no GPU support | ||
| # For Emscripten builds linking to gtest_main will not suffice for gtest to run | ||
| # the tests and an explicitly main.cpp is needed | ||
| set(EXTRA_TEST_SOURCE_FILES main.cpp) |
There was a problem hiding this comment.
This change was only originally needed for Emscripten builds, and you tell me that oop doesn't support oop, so why would this PR include this change?
There was a problem hiding this comment.
Because the main.cpp file handles the extra flags passed for gtest. Here in our case: --use-oop-jit.
I don't understand this line though:
and you tell me that oop doesn't support oop
There was a problem hiding this comment.
I meant why were you changing something that was changing this, so main.cpp is including for non Emscripten builds now. You answered it, but I don't understand why this --use-oop-jit option is handled outside the cmake.
There was a problem hiding this comment.
Yeah, you are right that cmake should handle this. But, to have difference between the tests, we are using --use-oop-jit option. Otherwise, our test util will not be able to understand which tests to run. So, it kind of gives the user power to whether run OOP tests or not. Even with your cmake approach, I don't think that we can get rid of --use-oop-jit option because my current approach is something like this:
CppInterOp/unittests/CppInterOp/Utils.cpp
Lines 66 to 73 in fda48a4
There was a problem hiding this comment.
So, I need this option to create interpreters differently for both in-process and out-process approach.
| valgrind --show-error-list=yes --track-origins=yes --error-exitcode=1 unittests/CppInterOp/CppInterOpTests/unittests/bin/${{ env.BUILD_TYPE }}/CppInterOpTests | ||
| fi | ||
| if [[ "${{ matrix.oop-jit }}" == "On" ]]; then | ||
| ./unittests/CppInterOp/CppInterOpTests/unittests/bin/${{ env.BUILD_TYPE }}/CppInterOpTests --use-oop-jit |
There was a problem hiding this comment.
This should be built into the cmake build system, if this flag is needed for some reason when oop is on.
| if (llvm::sys::RunningOnValgrind()) | ||
| GTEST_SKIP() << "XFAIL due to Valgrind report"; | ||
| TestUtils::CreateInterpreter(); | ||
| pid_t pid = Cpp::GetExecutorPID(); |
There was a problem hiding this comment.
warning: no header providing "pid_t" is directly included [misc-include-cleaner]
unittests/CppInterOp/FunctionReflectionTest.cpp:15:
- #include <string>
+ #include <sched.h>
+ #include <string>
lib/CppInterOp/Compatibility.h
Outdated
|
|
||
| #include "clang/Interpreter/CodeCompletion.h" | ||
|
|
||
| #include "clang/Interpreter/OutOfProcessJITConfig.h" |
There was a problem hiding this comment.
warning: 'clang/Interpreter/OutOfProcessJITConfig.h' file not found [clang-diagnostic-error]
#include "clang/Interpreter/OutOfProcessJITConfig.h"
^|
|
||
| #include <algorithm> | ||
| #include <unistd.h> | ||
|
|
There was a problem hiding this comment.
warning: included header unistd.h is not used directly [misc-include-cleaner]
| } | ||
|
|
||
| #ifndef _WIN32 | ||
| FILE* getTempFileForOOP(int FD) { |
There was a problem hiding this comment.
warning: method 'getTempFileForOOP' can be made static [readability-convert-member-functions-to-static]
| FILE* getTempFileForOOP(int FD) { | |
| static FILE* getTempFileForOOP(int FD) { |
| if (!io_context) | ||
| return nullptr; | ||
| switch (FD) { | ||
| case (STDIN_FILENO): |
There was a problem hiding this comment.
warning: switch has 3 consecutive identical branches [bugprone-branch-clone]
case (STDIN_FILENO):
^Additional context
lib/CppInterOp/CppInterOpInterpreter.h:247: last of these clones ends here
case (STDERR_FILENO):
^| return llvm::orc::ExecutorAddr(*AddrOrErr); | ||
| } | ||
|
|
||
| pid_t getOutOfProcessExecutorPID() const { |
There was a problem hiding this comment.
warning: function 'getOutOfProcessExecutorPID' should be marked [[nodiscard]] [modernize-use-nodiscard]
| pid_t getOutOfProcessExecutorPID() const { | |
| [[nodiscard]] pid_t getOutOfProcessExecutorPID() const { |
| return llvm::orc::ExecutorAddr(*AddrOrErr); | ||
| } | ||
|
|
||
| pid_t getOutOfProcessExecutorPID() const { |
There was a problem hiding this comment.
warning: method 'getOutOfProcessExecutorPID' can be made static [readability-convert-member-functions-to-static]
| pid_t getOutOfProcessExecutorPID() const { | |
| static pid_t getOutOfProcessExecutorPID() { |
| return llvm::orc::ExecutorAddr(*AddrOrErr); | ||
| } | ||
|
|
||
| pid_t getOutOfProcessExecutorPID() const { |
There was a problem hiding this comment.
warning: no header providing "pid_t" is directly included [misc-include-cleaner]
lib/CppInterOp/CppInterOpInterpreter.h:25:
- #if CLANG_VERSION_MAJOR >= 19
+ #include <sched.h>
+ #if CLANG_VERSION_MAJOR >= 19| @@ -1,3 +1,4 @@ | |||
| #include "Utils.h" | |||
| #include "CppInterOp/CppInterOp.h" | |||
There was a problem hiding this comment.
warning: included header Utils.h is not used directly [misc-include-cleaner]
| #include "CppInterOp/CppInterOp.h" | |
| #include "CppInterOp/CppInterOp.h" |
| } | ||
|
|
||
| void TestUtils::GetAllSubDecls(Decl *D, std::vector<Decl*>& SubDecls, | ||
| void TestUtils::GetAllSubDecls(Decl* D, std::vector<Decl*>& SubDecls, |
There was a problem hiding this comment.
warning: function 'GetAllSubDecls' can be made static or moved into an anonymous namespace to enforce internal linkage [misc-use-internal-linkage]
| void TestUtils::GetAllSubDecls(Decl* D, std::vector<Decl*>& SubDecls, | |
| static void TestUtils::GetAllSubDecls(Decl* D, std::vector<Decl*>& SubDecls, |
| } | ||
| } | ||
|
|
||
| TInterp_t TestUtils::CreateInterpreter(const std::vector<const char*>& Args, |
There was a problem hiding this comment.
warning: function 'CreateInterpreter' can be made static or moved into an anonymous namespace to enforce internal linkage [misc-use-internal-linkage]
| TInterp_t TestUtils::CreateInterpreter(const std::vector<const char*>& Args, | |
| static TInterp_t TestUtils::CreateInterpreter(const std::vector<const char*>& Args, |
|
|
||
| inline std::unique_ptr<clang::Interpreter> | ||
| createClangInterpreter(std::vector<const char*>& args) { | ||
| createClangInterpreter(std::vector<const char*>& args, int stdin_fd = 0, |
There was a problem hiding this comment.
warning: no header providing "std::vector" is directly included [misc-include-cleaner]
lib/CppInterOp/Compatibility.h:14:
+ #include <vector>
lib/CppInterOp/CppInterOp.cpp
Outdated
| Cpp_utils::Lookup::Named(&S, R, DC); | ||
|
|
||
| if (R.getResultKind() == clang::LookupResult::NotFound && funcs.empty()) | ||
| if (R.getResultKind() == clang::LookupResultKind::NotFound && funcs.empty()) |
There was a problem hiding this comment.
warning: no member named 'LookupResultKind' in namespace 'clang'; did you mean 'LookupResult'? [clang-diagnostic-error]
| if (R.getResultKind() == clang::LookupResultKind::NotFound && funcs.empty()) | |
| if (R.getResultKind() == clang::LookupResult::NotFound && funcs.empty()) |
Additional context
llvm/include/clang/Sema/Lookup.h:45: 'LookupResult' declared here
class LookupResult {
^
lib/CppInterOp/CppInterOp.cpp
Outdated
|
|
||
| // Distinct match, single Decl | ||
| else if (R.getResultKind() == clang::LookupResult::Found) { | ||
| else if (R.getResultKind() == clang::LookupResultKind::Found) { |
There was a problem hiding this comment.
warning: no member named 'LookupResultKind' in namespace 'clang'; did you mean 'LookupResult'? [clang-diagnostic-error]
else if (R.getResultKind() == clang::LookupResultKind::Found) {
^this fix will not be applied because it overlaps with another fix
lib/CppInterOp/CppInterOp.cpp
Outdated
| } | ||
| // Loop over overload set | ||
| else if (R.getResultKind() == clang::LookupResult::FoundOverloaded) { | ||
| else if (R.getResultKind() == clang::LookupResultKind::FoundOverloaded) { |
There was a problem hiding this comment.
warning: no member named 'LookupResultKind' in namespace 'clang'; did you mean 'LookupResult'? [clang-diagnostic-error]
else if (R.getResultKind() == clang::LookupResultKind::FoundOverloaded) {
^this fix will not be applied because it overlaps with another fix
|
|
||
| return std::unique_ptr<Interpreter>(new Interpreter(std::move(CI))); | ||
| return std::unique_ptr<Interpreter>( | ||
| new Interpreter(std::move(CI), std::move(io_ctx))); |
There was a problem hiding this comment.
warning: use std::make_unique instead [modernize-make-unique]
| new Interpreter(std::move(CI), std::move(io_ctx))); | |
| return std::make_unique<Interpreter>( | |
| std::move(CI), std::move(io_ctx)); |
|
|
||
| pid_t getOutOfProcessExecutorPID() const { | ||
| #ifndef _WIN32 | ||
| return inner->getOutOfProcessExecutorPID(); |
There was a problem hiding this comment.
warning: no member named 'getOutOfProcessExecutorPID' in 'clang::Interpreter' [clang-diagnostic-error]
return inner->getOutOfProcessExecutorPID();
^| if (TestUtils::use_oop_jit()) { | ||
| GTEST_SKIP() << "Test fails for OOP JIT builds"; | ||
| } | ||
| std::vector<Decl*> Decls, SubDecls; |
There was a problem hiding this comment.
warning: multiple declarations in a single statement reduces readability [readability-isolate-declaration]
| std::vector<Decl*> Decls, SubDecls; | |
| std::vector<Decl*> Decls; | |
| std::vector<Decl*> SubDecls; |
| EXPECT_EQ(pid, -1); | ||
| } | ||
|
|
||
| pid = Cpp::GetNthExecutorPID(1); |
There was a problem hiding this comment.
warning: no member named 'GetNthExecutorPID' in namespace 'Cpp' [clang-diagnostic-error]
pid = Cpp::GetNthExecutorPID(1);
^
| size_t newLen = | ||
| fread(content.get(), sizeof(char), bufsize, m_TempFile.get()); | ||
| if (ferror(m_TempFile.get()) != 0) | ||
| size_t newLen = fread(content.get(), sizeof(char), bufsize, m_TempFile); |
There was a problem hiding this comment.
warning: The 1st argument to 'fread' is a buffer with size 0 but should be a buffer with size equal to or greater than the value of the 2nd argument (which is 1) times the 3rd argument (which is 18446744073709551615) [clang-analyzer-unix.StdCLibraryFunctions]
size_t newLen = fread(content.get(), sizeof(char), bufsize, m_TempFile);
^Additional context
lib/CppInterOp/CppInterOp.cpp:4409: Calling 'StreamCaptureInfo::GetCapturedString'
std::string result = SCI.GetCapturedString();
^lib/CppInterOp/CppInterOp.cpp:4356: Taking false branch
if (dup2(m_DupFD, m_FD) < 0)
^lib/CppInterOp/CppInterOp.cpp:4359: Taking false branch
if (fseek(m_TempFile, 0L, SEEK_END) != 0)
^lib/CppInterOp/CppInterOp.cpp:4363: Assuming that 'ftell' fails
long bufsize = ftell(m_TempFile);
^lib/CppInterOp/CppInterOp.cpp:4363: 'bufsize' initialized here
long bufsize = ftell(m_TempFile);
^lib/CppInterOp/CppInterOp.cpp:4364: Taking true branch
if (bufsize == -1)
^lib/CppInterOp/CppInterOp.cpp:4368: Storing uninitialized value
std::unique_ptr<char[]> content(new char[bufsize + 1]);
^lib/CppInterOp/CppInterOp.cpp:4368: Passing value via 1st parameter '__p'
std::unique_ptr<char[]> content(new char[bufsize + 1]);
^lib/CppInterOp/CppInterOp.cpp:4368: Calling constructor for 'unique_ptr<char[], std::default_delete<char[]>>'
std::unique_ptr<char[]> content(new char[bufsize + 1]);
^/usr/include/c++/13/bits/unique_ptr.h:603: Calling constructor for '__uniq_ptr_data<char, std::default_delete<char[]>, true, true>'
: _M_t(__p)
^/usr/include/c++/13/bits/unique_ptr.h:603: Passing '' via 1st parameter '__p'
: _M_t(__p)
^/usr/include/c++/13/bits/unique_ptr.h:603: Calling constructor for '__uniq_ptr_impl<char, std::default_delete<char[]>>'
: _M_t(__p)
^/usr/include/c++/13/bits/unique_ptr.h:175: Calling '__uniq_ptr_impl::_M_ptr'
__uniq_ptr_impl(pointer __p) : _M_t() { _M_ptr() = __p; }
^/usr/include/c++/13/bits/unique_ptr.h:196: Calling 'get<0UL, char *, std::default_delete<char[]>>'
pointer& _M_ptr() noexcept { return std::get<0>(_M_t); }
^/usr/include/c++/13/tuple:1803: Calling '__get_helper<0UL, char *, std::default_delete<char[]>>'
{ return std::__get_helper<__i>(__t); }
^/usr/include/c++/13/tuple:1787: Calling '_Tuple_impl::_M_head'
{ return _Tuple_impl<__i, _Head, _Tail...>::_M_head(__t); }
^/usr/include/c++/13/tuple:268: Calling '_Head_base::_M_head'
_M_head(_Tuple_impl& __t) noexcept { return _Base::_M_head(__t); }
^/usr/include/c++/13/tuple:268: Returning from '_Head_base::_M_head'
_M_head(_Tuple_impl& __t) noexcept { return _Base::_M_head(__t); }
^/usr/include/c++/13/tuple:1787: Returning from '_Tuple_impl::_M_head'
{ return _Tuple_impl<__i, _Head, _Tail...>::_M_head(__t); }
^/usr/include/c++/13/tuple:1803: Returning from '__get_helper<0UL, char *, std::default_delete<char[]>>'
{ return std::__get_helper<__i>(__t); }
^/usr/include/c++/13/bits/unique_ptr.h:196: Returning from 'get<0UL, char *, std::default_delete<char[]>>'
pointer& _M_ptr() noexcept { return std::get<0>(_M_t); }
^/usr/include/c++/13/bits/unique_ptr.h:175: Returning from '__uniq_ptr_impl::_M_ptr'
__uniq_ptr_impl(pointer __p) : _M_t() { _M_ptr() = __p; }
^/usr/include/c++/13/bits/unique_ptr.h:175: The value of '__p' is assigned to 'content._M_t._M_t._M_head_impl'
__uniq_ptr_impl(pointer __p) : _M_t() { _M_ptr() = __p; }
^/usr/include/c++/13/bits/unique_ptr.h:603: Returning from constructor for '__uniq_ptr_impl<char, std::default_delete<char[]>>'
: _M_t(__p)
^/usr/include/c++/13/bits/unique_ptr.h:603: Returning from constructor for '__uniq_ptr_data<char, std::default_delete<char[]>, true, true>'
: _M_t(__p)
^lib/CppInterOp/CppInterOp.cpp:4368: Returning from constructor for 'unique_ptr<char[], std::default_delete<char[]>>'
std::unique_ptr<char[]> content(new char[bufsize + 1]);
^lib/CppInterOp/CppInterOp.cpp:4371: Taking false branch
if (fseek(m_TempFile, 0L, SEEK_SET) != 0)
^lib/CppInterOp/CppInterOp.cpp:4375: The 1st argument to 'fread' is a buffer with size 0 but should be a buffer with size equal to or greater than the value of the 2nd argument (which is 1) times the 3rd argument (which is 18446744073709551615)
size_t newLen = fread(content.get(), sizeof(char), bufsize, m_TempFile);
^
Description
This PR introduces creation of interpreter that runs JIT code in different process(out-of-process). This is the PR on LLVM that supports this feature on LLVM side. Since, this feature on LLVM is expected to be released with 21.x, till then CppInterOp will be using a patch for LLVM build.
Changes
CreateInterpreterwhen passed with appropriate flags, spins up an executor that runs code remotely.Type of change
Testing
Checklist