[interp] Register runtime symbols for clang-repl #726
Conversation
aaronj0
force-pushed
the
register-runtime-symbols
branch
from
7ab924a to
b912765
Compare
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ 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
... and 1 file with indirect coverage changes
🚀 New features to boost your workflow:
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
lib/CppInterOp/CppInterOp.cpp
Outdated
| #include <unistd.h> | ||
| #endif // WIN32 | ||
|
|
||
| extern "C" void __clang_Interpreter_SetValueNoAlloc(void* This, void* OutVal, |
There was a problem hiding this comment.
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
lib/CppInterOp/CppInterOp.cpp
Outdated
| #include <unistd.h> | ||
| #endif // WIN32 | ||
|
|
||
| extern "C" void __clang_Interpreter_SetValueNoAlloc(void* This, void* OutVal, |
There was a problem hiding this comment.
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
lib/CppInterOp/CppInterOp.cpp
Outdated
| #ifndef CPPINTEROP_USE_CLING | ||
| static bool DefineAbsoluteSymbol(compat::Interpreter& I, | ||
| const char* linker_mangled_name, | ||
| uint64_t address) { |
There was a problem hiding this comment.
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(); |
There was a problem hiding this comment.
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(); |
There was a problem hiding this comment.
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; |
There was a problem hiding this comment.
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(); |
There was a problem hiding this comment.
warning: 'auto &DL' can be declared as 'const auto &DL' [readability-qualified-auto]
| 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))) { |
There was a problem hiding this comment.
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); |
There was a problem hiding this comment.
warning: do not use C-style cast to convert between unrelated types [cppcoreguidelines-pro-type-cstyle-cast]
(uint64_t)&__clang_Interpreter_SetValueNoAlloc);
^
aaronj0
force-pushed
the
register-runtime-symbols
branch
from
d7da73e to
9c6be69
Compare
| extern "C" void* __clang_Interpreter_SetValueWithAlloc(void* This, void* OutVal, | ||
| void* OpaqueType) | ||
| #else | ||
| void* __clang_Interpreter_SetValueWithAlloc(void* This, void* OutVal, |
There was a problem hiding this comment.
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, |
There was a problem hiding this comment.
warning: function '__clang_Interpreter_SetValueWithAlloc' can be made static or moved into an anonymous namespace to enforce internal linkage [misc-use-internal-linkage]
| 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, |
There was a problem hiding this comment.
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, |
There was a problem hiding this comment.
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, |
There was a problem hiding this comment.
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); |
There was a problem hiding this comment.
warning: do not use C-style cast to convert between unrelated types [cppcoreguidelines-pro-type-cstyle-cast]
(uint64_t)&__clang_Interpreter_SetValueWithAlloc);
^
lib/CppInterOp/CppInterOp.cpp
Outdated
| #endif | ||
|
|
||
| #if CLANG_VERSION_MAJOR >= 19 | ||
| extern "C" void __clang_Interpreter_SetValueNoAlloc(void* This, |
There was a problem hiding this comment.
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
lib/CppInterOp/CppInterOp.cpp
Outdated
| #endif | ||
|
|
||
| #if CLANG_VERSION_MAJOR >= 19 | ||
| extern "C" void __clang_Interpreter_SetValueNoAlloc(void* This, |
There was a problem hiding this comment.
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
lib/CppInterOp/CppInterOp.cpp
Outdated
| } | ||
| #else | ||
| DefineAbsoluteSymbol(*I, "__clang_Interpreter_SetValueNoAlloc", | ||
| (uint64_t)&__clang_Interpreter_SetValueNoAlloc); |
There was a problem hiding this comment.
warning: do not use C-style cast to convert between unrelated types [cppcoreguidelines-pro-type-cstyle-cast]
(uint64_t)&__clang_Interpreter_SetValueNoAlloc);
^
aaronj0
force-pushed
the
register-runtime-symbols
branch
from
fdffc1d to
740bcb3
Compare
| #endif | ||
|
|
||
| #if CLANG_VERSION_MAJOR >= 19 | ||
| extern "C" void __clang_Interpreter_SetValueNoAlloc(void* This, |
There was a problem hiding this comment.
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, |
There was a problem hiding this comment.
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); |
There was a problem hiding this comment.
warning: do not use C-style cast to convert between unrelated types [cppcoreguidelines-pro-type-cstyle-cast]
(uint64_t)&__clang_Interpreter_SetValueNoAlloc);
^
aaronj0
force-pushed
the
register-runtime-symbols
branch
from
aadc275 to
4ef7df9
Compare
lib/CppInterOp/CppInterOp.cpp
Outdated
| // 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; |
There was a problem hiding this comment.
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
lib/CppInterOp/CppInterOp.cpp
Outdated
| // 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; |
There was a problem hiding this comment.
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
lib/CppInterOp/CppInterOp.cpp
Outdated
| // 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; |
There was a problem hiding this comment.
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
lib/CppInterOp/CppInterOp.cpp
Outdated
| // 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; |
There was a problem hiding this comment.
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
lib/CppInterOp/CppInterOp.cpp
Outdated
| // 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; |
There was a problem hiding this comment.
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); |
There was a problem hiding this comment.
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
force-pushed
the
register-runtime-symbols
branch
from
02883df to
b45c436
Compare
aaronj0
force-pushed
the
register-runtime-symbols
branch
from
b45c436 to
8d91bcd
Compare
| // 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 { |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
We should add the relevant clang-tidy suppression.
| TEST(InterpreterTest, EvaluateExtensive) { | ||
| #ifdef EMSCRIPTEN | ||
| GTEST_SKIP() << "Test fails for Emscipten builds"; | ||
| #endif |
There was a problem hiding this comment.
All the tests at the start of this test are the same as InterpreterTest, Evaluate. Can we not remove that old test, and just replace it with this one?
Can you expand the test to cover the 3 lines not currently covered? |
aaronj0
force-pushed
the
register-runtime-symbols
branch
from
7412591 to
409f115
Compare
lib/CppInterOp/Compatibility.h
Outdated
| // 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 { |
There was a problem hiding this comment.
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
lib/CppInterOp/Compatibility.h
Outdated
| // 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 { |
There was a problem hiding this comment.
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
lib/CppInterOp/Compatibility.h
Outdated
| // link to llvm | ||
| #if !defined(CPPINTEROP_USE_CLING) && !defined(EMSCRIPTEN) | ||
| struct __clang_Interpreter_NewTag { | ||
| } __ci_newtag; |
There was a problem hiding this comment.
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
lib/CppInterOp/Compatibility.h
Outdated
| // link to llvm | ||
| #if !defined(CPPINTEROP_USE_CLING) && !defined(EMSCRIPTEN) | ||
| struct __clang_Interpreter_NewTag { | ||
| } __ci_newtag; |
There was a problem hiding this comment.
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
lib/CppInterOp/Compatibility.h
Outdated
| // link to llvm | ||
| #if !defined(CPPINTEROP_USE_CLING) && !defined(EMSCRIPTEN) | ||
| struct __clang_Interpreter_NewTag { | ||
| } __ci_newtag; |
There was a problem hiding this comment.
warning: variable '__ci_newtag' defined in a header file; variable definitions in header files can lead to ODR violations [misc-definitions-in-headers]
} __ci_newtag;
^
lib/CppInterOp/Compatibility.h
Outdated
| // link to llvm | ||
| #if !defined(CPPINTEROP_USE_CLING) && !defined(EMSCRIPTEN) | ||
| struct __clang_Interpreter_NewTag { | ||
| } __ci_newtag; |
There was a problem hiding this comment.
warning: variable '__ci_newtag' is non-const and globally accessible, consider making it const [cppcoreguidelines-avoid-non-const-global-variables]
} __ci_newtag;
^
lib/CppInterOp/Compatibility.h
Outdated
| extern "C" void* __clang_Interpreter_SetValueWithAlloc(void* This, void* OutVal, | ||
| void* OpaqueType) | ||
| #else | ||
| void* __clang_Interpreter_SetValueWithAlloc(void* This, void* OutVal, |
There was a problem hiding this comment.
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
lib/CppInterOp/Compatibility.h
Outdated
| extern "C" void* __clang_Interpreter_SetValueWithAlloc(void* This, void* OutVal, | ||
| void* OpaqueType) | ||
| #else | ||
| void* __clang_Interpreter_SetValueWithAlloc(void* This, void* OutVal, |
There was a problem hiding this comment.
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
lib/CppInterOp/Compatibility.h
Outdated
| #endif | ||
|
|
||
| #if CLANG_VERSION_MAJOR > 18 | ||
| extern "C" void __clang_Interpreter_SetValueNoAlloc(void* This, |
There was a problem hiding this comment.
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
lib/CppInterOp/Compatibility.h
Outdated
| #endif | ||
|
|
||
| #if CLANG_VERSION_MAJOR > 18 | ||
| extern "C" void __clang_Interpreter_SetValueNoAlloc(void* This, |
There was a problem hiding this comment.
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
aaronj0
force-pushed
the
register-runtime-symbols
branch
from
409f115 to
1eabe0c
Compare
aaronj0
force-pushed
the
register-runtime-symbols
branch
from
1eabe0c to
e15be64
Compare
| namespace { | ||
| #if !defined(CPPINTEROP_USE_CLING) && !defined(EMSCRIPTEN) | ||
| bool DefineAbsoluteSymbol(compat::Interpreter& I, | ||
| const char* linker_mangled_name, uint64_t address) { |
There was a problem hiding this comment.
warning: no header providing "uint64_t" is directly included [misc-include-cleaner]
const char* linker_mangled_name, uint64_t address) {
^
Registering this runtime symbol fixes JIT session errors when calling
Cpp::Evaluateif the process does not see symbols fromlibClangCppInterOp.so.