Skip to content

Conversation

aaronj0
Copy link
Collaborator

@aaronj0 aaronj0 commented Oct 1, 2025

Registering this runtime symbol fixes JIT session errors when calling Cpp::Evaluate if the process does not see symbols from libClangCppInterOp.so.

@aaronj0 aaronj0 requested a review from vgvassilev October 1, 2025 11:41
// define __clang_Interpreter_SetValueNoAlloc in the JIT dylib for clang-repl
#ifndef CPPINTEROP_USE_CLING
DefineAbsoluteSymbol(*I, "__clang_Interpreter_SetValueNoAlloc",
(uint64_t)&__clang_Interpreter_SetValueNoAlloc);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We will need the entire runtime not only that function.

@aaronj0 aaronj0 force-pushed the register-runtime-symbols branch from 7ab924a to b912765 Compare October 1, 2025 11:49
Copy link

codecov bot commented Oct 1, 2025

Codecov Report

❌ Patch coverage is 89.28571% with 3 lines in your changes missing coverage. Please review.
✅ Project coverage is 81.11%. Comparing base (bcde755) to head (8d91bcd).
⚠️ Report is 3 commits behind head on main.

Files with missing lines Patch % Lines
lib/CppInterOp/CppInterOp.cpp 89.28% 3 Missing ⚠️
Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #726      +/-   ##
==========================================
+ Coverage   81.03%   81.11%   +0.08%     
==========================================
  Files           9        9              
  Lines        4213     4242      +29     
==========================================
+ Hits         3414     3441      +27     
- Misses        799      801       +2     
Files with missing lines Coverage Δ
lib/CppInterOp/CppInterOp.cpp 88.84% <89.28%> (+0.04%) ⬆️

... and 1 file with indirect coverage changes

Files with missing lines Coverage Δ
lib/CppInterOp/CppInterOp.cpp 88.84% <89.28%> (+0.04%) ⬆️

... and 1 file with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

clang-tidy made some suggestions

#include <unistd.h>
#endif // WIN32

extern "C" void __clang_Interpreter_SetValueNoAlloc(void* This, void* OutVal,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

warning: declaration uses identifier '__clang_Interpreter_SetValueNoAlloc', which is a reserved identifier [bugprone-reserved-identifier]

extern "C" void __clang_Interpreter_SetValueNoAlloc(void* This, void* OutVal,
                ^

this fix will not be applied because it overlaps with another fix

#include <unistd.h>
#endif // WIN32

extern "C" void __clang_Interpreter_SetValueNoAlloc(void* This, void* OutVal,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

warning: invalid case style for function '__clang_Interpreter_SetValueNoAlloc' [readability-identifier-naming]

extern "C" void __clang_Interpreter_SetValueNoAlloc(void* This, void* OutVal,
                ^

this fix will not be applied because it overlaps with another fix


namespace {
#ifndef CPPINTEROP_USE_CLING
static bool DefineAbsoluteSymbol(compat::Interpreter& I,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

warning: 'DefineAbsoluteSymbol' is a static definition in anonymous namespace; static is redundant here [readability-static-definition-in-anonymous-namespace]

Suggested change
static bool DefineAbsoluteSymbol(compat::Interpreter& I,
bool DefineAbsoluteSymbol(compat::Interpreter& I,

#ifndef CPPINTEROP_USE_CLING
static bool DefineAbsoluteSymbol(compat::Interpreter& I,
const char* linker_mangled_name,
uint64_t address) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

warning: no header providing "uint64_t" is directly included [misc-include-cleaner]

                                 uint64_t address) {
                                 ^

using namespace llvm::orc;

llvm::orc::LLJIT& Jit = *compat::getExecutionEngine(I);
llvm::orc::ExecutionSession& ES = Jit.getExecutionSession();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

warning: no header providing "llvm::orc::ExecutionSession" is directly included [misc-include-cleaner]

lib/CppInterOp/CppInterOp.cpp:41:

- #if CLANG_VERSION_MAJOR >= 19
+ #include <llvm/ExecutionEngine/Orc/Core.h>
+ #if CLANG_VERSION_MAJOR >= 19


llvm::orc::LLJIT& Jit = *compat::getExecutionEngine(I);
llvm::orc::ExecutionSession& ES = Jit.getExecutionSession();
JITDylib& DyLib = *Jit.getProcessSymbolsJITDylib().get();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

warning: no header providing "llvm::orc::JITDylib" is directly included [misc-include-cleaner]

  JITDylib& DyLib = *Jit.getProcessSymbolsJITDylib().get();
  ^

llvm::orc::ExecutionSession& ES = Jit.getExecutionSession();
JITDylib& DyLib = *Jit.getProcessSymbolsJITDylib().get();

llvm::orc::SymbolMap InjectedSymbols;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

warning: no header providing "llvm::orc::SymbolMap" is directly included [misc-include-cleaner]

lib/CppInterOp/CppInterOp.cpp:41:

- #if CLANG_VERSION_MAJOR >= 19
+ #include <llvm/ExecutionEngine/Orc/CoreContainers.h>
+ #if CLANG_VERSION_MAJOR >= 19

JITDylib& DyLib = *Jit.getProcessSymbolsJITDylib().get();

llvm::orc::SymbolMap InjectedSymbols;
auto& DL = compat::getExecutionEngine(I)->getDataLayout();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

warning: 'auto &DL' can be declared as 'const auto &DL' [readability-qualified-auto]

Suggested change
auto& DL = compat::getExecutionEngine(I)->getDataLayout();
const auto& DL = compat::getExecutionEngine(I)->getDataLayout();

InjectedSymbols[Name] =
ExecutorSymbolDef(ExecutorAddr(address), JITSymbolFlags::Exported);

if (Error Err = DyLib.define(absoluteSymbols(InjectedSymbols))) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

warning: no header providing "llvm::orc::absoluteSymbols" is directly included [misc-include-cleaner]

lib/CppInterOp/CppInterOp.cpp:41:

- #if CLANG_VERSION_MAJOR >= 19
+ #include <llvm/ExecutionEngine/Orc/AbsoluteSymbols.h>
+ #if CLANG_VERSION_MAJOR >= 19

// define __clang_Interpreter_SetValueNoAlloc in the JIT dylib for clang-repl
#ifndef CPPINTEROP_USE_CLING
DefineAbsoluteSymbol(*I, "__clang_Interpreter_SetValueNoAlloc",
(uint64_t)&__clang_Interpreter_SetValueNoAlloc);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

warning: do not use C-style cast to convert between unrelated types [cppcoreguidelines-pro-type-cstyle-cast]

                       (uint64_t)&__clang_Interpreter_SetValueNoAlloc);
                       ^

@aaronj0 aaronj0 force-pushed the register-runtime-symbols branch from d7da73e to 9c6be69 Compare October 1, 2025 12:46
Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

clang-tidy made some suggestions

extern "C" void* __clang_Interpreter_SetValueWithAlloc(void* This, void* OutVal,
void* OpaqueType)
#else
void* __clang_Interpreter_SetValueWithAlloc(void* This, void* OutVal,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

warning: declaration uses identifier '__clang_Interpreter_SetValueWithAlloc', which is a reserved identifier [bugprone-reserved-identifier]

void* __clang_Interpreter_SetValueWithAlloc(void* This, void* OutVal,
      ^

this fix will not be applied because it overlaps with another fix

extern "C" void* __clang_Interpreter_SetValueWithAlloc(void* This, void* OutVal,
void* OpaqueType)
#else
void* __clang_Interpreter_SetValueWithAlloc(void* This, void* OutVal,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

warning: function '__clang_Interpreter_SetValueWithAlloc' can be made static or moved into an anonymous namespace to enforce internal linkage [misc-use-internal-linkage]

Suggested change
void* __clang_Interpreter_SetValueWithAlloc(void* This, void* OutVal,
static void* __clang_Interpreter_SetValueWithAlloc(void* This, void* OutVal,

extern "C" void* __clang_Interpreter_SetValueWithAlloc(void* This, void* OutVal,
void* OpaqueType)
#else
void* __clang_Interpreter_SetValueWithAlloc(void* This, void* OutVal,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

warning: invalid case style for function '__clang_Interpreter_SetValueWithAlloc' [readability-identifier-naming]

void* __clang_Interpreter_SetValueWithAlloc(void* This, void* OutVal,
      ^

this fix will not be applied because it overlaps with another fix

void* OpaqueType);
#endif

extern "C" void __clang_Interpreter_SetValueNoAlloc(void* This,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

warning: declaration uses identifier '__clang_Interpreter_SetValueNoAlloc', which is a reserved identifier [bugprone-reserved-identifier]

    extern "C" void __clang_Interpreter_SetValueNoAlloc(void* This,
                    ^

this fix will not be applied because it overlaps with another fix

void* OpaqueType);
#endif

extern "C" void __clang_Interpreter_SetValueNoAlloc(void* This,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

warning: invalid case style for function '__clang_Interpreter_SetValueNoAlloc' [readability-identifier-naming]

    extern "C" void __clang_Interpreter_SetValueNoAlloc(void* This,
                    ^

this fix will not be applied because it overlaps with another fix

std::string mangledName;
compat::maybeMangleDeclName(GD, mangledName);
DefineAbsoluteSymbol(*I, mangledName.c_str(),
(uint64_t)&__clang_Interpreter_SetValueWithAlloc);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

warning: do not use C-style cast to convert between unrelated types [cppcoreguidelines-pro-type-cstyle-cast]

                         (uint64_t)&__clang_Interpreter_SetValueWithAlloc);
                         ^

Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

clang-tidy made some suggestions

#endif

#if CLANG_VERSION_MAJOR >= 19
extern "C" void __clang_Interpreter_SetValueNoAlloc(void* This,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

warning: declaration uses identifier '__clang_Interpreter_SetValueNoAlloc', which is a reserved identifier [bugprone-reserved-identifier]

extern "C" void __clang_Interpreter_SetValueNoAlloc(void* This,
                ^

this fix will not be applied because it overlaps with another fix

#endif

#if CLANG_VERSION_MAJOR >= 19
extern "C" void __clang_Interpreter_SetValueNoAlloc(void* This,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

warning: invalid case style for function '__clang_Interpreter_SetValueNoAlloc' [readability-identifier-naming]

extern "C" void __clang_Interpreter_SetValueNoAlloc(void* This,
                ^

this fix will not be applied because it overlaps with another fix

}
#else
DefineAbsoluteSymbol(*I, "__clang_Interpreter_SetValueNoAlloc",
(uint64_t)&__clang_Interpreter_SetValueNoAlloc);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

warning: do not use C-style cast to convert between unrelated types [cppcoreguidelines-pro-type-cstyle-cast]

(uint64_t)&__clang_Interpreter_SetValueNoAlloc);
^

@aaronj0 aaronj0 force-pushed the register-runtime-symbols branch from fdffc1d to 740bcb3 Compare October 3, 2025 14:10
Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

clang-tidy made some suggestions

#endif

#if CLANG_VERSION_MAJOR >= 19
extern "C" void __clang_Interpreter_SetValueNoAlloc(void* This,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

warning: declaration uses identifier '__clang_Interpreter_SetValueNoAlloc', which is a reserved identifier [bugprone-reserved-identifier]

    extern "C" void __clang_Interpreter_SetValueNoAlloc(void* This,
                    ^

this fix will not be applied because it overlaps with another fix

#endif

#if CLANG_VERSION_MAJOR >= 19
extern "C" void __clang_Interpreter_SetValueNoAlloc(void* This,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

warning: invalid case style for function '__clang_Interpreter_SetValueNoAlloc' [readability-identifier-naming]

    extern "C" void __clang_Interpreter_SetValueNoAlloc(void* This,
                    ^

this fix will not be applied because it overlaps with another fix

}
#else
DefineAbsoluteSymbol(*I, "__clang_Interpreter_SetValueNoAlloc",
(uint64_t)&__clang_Interpreter_SetValueNoAlloc);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

warning: do not use C-style cast to convert between unrelated types [cppcoreguidelines-pro-type-cstyle-cast]

                       (uint64_t)&__clang_Interpreter_SetValueNoAlloc);
                       ^

@aaronj0 aaronj0 force-pushed the register-runtime-symbols branch from aadc275 to 4ef7df9 Compare October 3, 2025 15:05
Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

clang-tidy made some suggestions

// Runtime symbols required if the library using JIT (Cpp::Evaluate) does not
// link to llvm
#if !defined(CPPINTEROP_USE_CLING) && !defined(EMSCRIPTEN)
struct __clang_Interpreter_NewTag {} __ci_newtag;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

warning: declaration uses identifier '__ci_newtag', which is a reserved identifier [bugprone-reserved-identifier]

struct __clang_Interpreter_NewTag {} __ci_newtag;
                                     ^

this fix will not be applied because it overlaps with another fix

// Runtime symbols required if the library using JIT (Cpp::Evaluate) does not
// link to llvm
#if !defined(CPPINTEROP_USE_CLING) && !defined(EMSCRIPTEN)
struct __clang_Interpreter_NewTag {} __ci_newtag;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

warning: declaration uses identifier '__clang_Interpreter_NewTag', which is a reserved identifier [bugprone-reserved-identifier]

struct __clang_Interpreter_NewTag {} __ci_newtag;
       ^

this fix will not be applied because it overlaps with another fix

// Runtime symbols required if the library using JIT (Cpp::Evaluate) does not
// link to llvm
#if !defined(CPPINTEROP_USE_CLING) && !defined(EMSCRIPTEN)
struct __clang_Interpreter_NewTag {} __ci_newtag;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

warning: invalid case style for class '__clang_Interpreter_NewTag' [readability-identifier-naming]

struct __clang_Interpreter_NewTag {} __ci_newtag;
       ^

this fix will not be applied because it overlaps with another fix

// Runtime symbols required if the library using JIT (Cpp::Evaluate) does not
// link to llvm
#if !defined(CPPINTEROP_USE_CLING) && !defined(EMSCRIPTEN)
struct __clang_Interpreter_NewTag {} __ci_newtag;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

warning: invalid case style for variable '__ci_newtag' [readability-identifier-naming]

struct __clang_Interpreter_NewTag {} __ci_newtag;
                                     ^

this fix will not be applied because it overlaps with another fix

// Runtime symbols required if the library using JIT (Cpp::Evaluate) does not
// link to llvm
#if !defined(CPPINTEROP_USE_CLING) && !defined(EMSCRIPTEN)
struct __clang_Interpreter_NewTag {} __ci_newtag;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

warning: variable '__ci_newtag' can be made static or moved into an anonymous namespace to enforce internal linkage [misc-use-internal-linkage]

Suggested change
struct __clang_Interpreter_NewTag {} __ci_newtag;
static struct __clang_Interpreter_NewTag {} __ci_newtag;

// Runtime symbols required if the library using JIT (Cpp::Evaluate) does not
// link to llvm
#if !defined(CPPINTEROP_USE_CLING) && !defined(EMSCRIPTEN)
struct __clang_Interpreter_NewTag {} __ci_newtag;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

warning: variable '__ci_newtag' is non-const and globally accessible, consider making it const [cppcoreguidelines-avoid-non-const-global-variables]

struct __clang_Interpreter_NewTag {} __ci_newtag;
                                     ^


// Define runtime symbols in the JIT dylib for clang-repl
#if !defined(CPPINTEROP_USE_CLING) && !defined(EMSCRIPTEN)
DefineAbsoluteSymbol(*I, "__ci_newtag", (uint64_t)&__ci_newtag);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

warning: do not use C-style cast to convert between unrelated types [cppcoreguidelines-pro-type-cstyle-cast]

  DefineAbsoluteSymbol(*I, "__ci_newtag", (uint64_t)&__ci_newtag);
                                          ^

@aaronj0 aaronj0 force-pushed the register-runtime-symbols branch from 02883df to b45c436 Compare October 3, 2025 16:11
@aaronj0 aaronj0 force-pushed the register-runtime-symbols branch from b45c436 to 8d91bcd Compare October 3, 2025 16:13
Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

clang-tidy made some suggestions

// Runtime symbols required if the library using JIT (Cpp::Evaluate) does not
// link to llvm
#if !defined(CPPINTEROP_USE_CLING) && !defined(EMSCRIPTEN)
struct __clang_Interpreter_NewTag {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

warning: declaration uses identifier '__clang_Interpreter_NewTag', which is a reserved identifier [bugprone-reserved-identifier]

struct __clang_Interpreter_NewTag {
       ^

this fix will not be applied because it overlaps with another fix

// Runtime symbols required if the library using JIT (Cpp::Evaluate) does not
// link to llvm
#if !defined(CPPINTEROP_USE_CLING) && !defined(EMSCRIPTEN)
struct __clang_Interpreter_NewTag {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

warning: invalid case style for class '__clang_Interpreter_NewTag' [readability-identifier-naming]

struct __clang_Interpreter_NewTag {
       ^

this fix will not be applied because it overlaps with another fix

// link to llvm
#if !defined(CPPINTEROP_USE_CLING) && !defined(EMSCRIPTEN)
struct __clang_Interpreter_NewTag {
} __ci_newtag;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

warning: declaration uses identifier '__ci_newtag', which is a reserved identifier [bugprone-reserved-identifier]

} __ci_newtag;
  ^

this fix will not be applied because it overlaps with another fix

// link to llvm
#if !defined(CPPINTEROP_USE_CLING) && !defined(EMSCRIPTEN)
struct __clang_Interpreter_NewTag {
} __ci_newtag;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

warning: invalid case style for variable '__ci_newtag' [readability-identifier-naming]

} __ci_newtag;
  ^

this fix will not be applied because it overlaps with another fix

// link to llvm
#if !defined(CPPINTEROP_USE_CLING) && !defined(EMSCRIPTEN)
struct __clang_Interpreter_NewTag {
} __ci_newtag;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

warning: variable '__ci_newtag' can be made static or moved into an anonymous namespace to enforce internal linkage [misc-use-internal-linkage]

lib/CppInterOp/CppInterOp.cpp:95:

- struct __clang_Interpreter_NewTag {
+ static struct __clang_Interpreter_NewTag {

// link to llvm
#if !defined(CPPINTEROP_USE_CLING) && !defined(EMSCRIPTEN)
struct __clang_Interpreter_NewTag {
} __ci_newtag;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

warning: variable '__ci_newtag' is non-const and globally accessible, consider making it const [cppcoreguidelines-avoid-non-const-global-variables]

} __ci_newtag;
  ^

Comment on lines +106 to +110
#if CLANG_VERSION_MAJOR >= 19
extern "C" void __clang_Interpreter_SetValueNoAlloc(void* This,
void* OutVal,
void* OpaqueType, ...);
#elif CLANG_VERSION_MAJOR == 18
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
#if CLANG_VERSION_MAJOR >= 19
extern "C" void __clang_Interpreter_SetValueNoAlloc(void* This,
void* OutVal,
void* OpaqueType, ...);
#elif CLANG_VERSION_MAJOR == 18
#if CLANG_VERSION_MAJOR > 18
extern "C" void __clang_Interpreter_SetValueNoAlloc(void* This,
void* OutVal,
void* OpaqueType, ...);
#else

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.

2 participants