Skip to content
Open
Show file tree
Hide file tree
Changes from 6 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
138 changes: 138 additions & 0 deletions lib/CppInterOp/CppInterOp.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -90,6 +90,34 @@
#include <unistd.h>
#endif // WIN32

// 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

Copy link
Contributor

Choose a reason for hiding this comment

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

We should add the relevant clang-tidy suppression.

} __ci_newtag;
#if CLANG_VERSION_MAJOR >= 22
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

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,

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

#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

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

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

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

void* OutVal,
void* OpaqueType, ...);
#elif CLANG_VERSION_MAJOR == 18
void __clang_Interpreter_SetValueNoAlloc(void*, void*, void*);
void __clang_Interpreter_SetValueNoAlloc(void*, void*, void*, void*);
void __clang_Interpreter_SetValueNoAlloc(void*, void*, void*, float);
void __clang_Interpreter_SetValueNoAlloc(void*, void*, void*, double);
void __clang_Interpreter_SetValueNoAlloc(void*, void*, void*, long double);
void __clang_Interpreter_SetValueNoAlloc(void*, void*, void*,
unsigned long long);
#endif
#endif // CPPINTEROP_USE_CLING

namespace Cpp {

using namespace clang;
Expand Down Expand Up @@ -3330,6 +3358,37 @@
}

namespace {
#if !defined(CPPINTEROP_USE_CLING) && !defined(EMSCRIPTEN)
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;
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

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::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

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();

char GlobalPrefix = DL.getGlobalPrefix();
std::string tmp(linker_mangled_name);
if (GlobalPrefix != '\0') {
tmp = std::string(1, GlobalPrefix) + tmp;

Check warning on line 3377 in lib/CppInterOp/CppInterOp.cpp

View check run for this annotation

Codecov / codecov/patch

lib/CppInterOp/CppInterOp.cpp#L3377

Added line #L3377 was not covered by tests
}
auto Name = ES.intern(tmp);
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

logAllUnhandledErrors(std::move(Err), errs(),

Check warning on line 3384 in lib/CppInterOp/CppInterOp.cpp

View check run for this annotation

Codecov / codecov/patch

lib/CppInterOp/CppInterOp.cpp#L3384

Added line #L3384 was not covered by tests
"DefineAbsoluteSymbol error: ");
return true;

Check warning on line 3386 in lib/CppInterOp/CppInterOp.cpp

View check run for this annotation

Codecov / codecov/patch

lib/CppInterOp/CppInterOp.cpp#L3386

Added line #L3386 was not covered by tests
}
return false;
}
#endif

static std::string MakeResourcesPath() {
StringRef Dir;
#ifdef LLVM_BINARY_DIR
Expand Down Expand Up @@ -3442,6 +3501,85 @@
sInterpreters->back().get()});

assert(sInterpreters->size() == sInterpreterASTMap->size());

// 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);
                                          ^

// llvm > 22 has this defined as a C symbol that does not require mangling
#if CLANG_VERSION_MAJOR >= 22
DefineAbsoluteSymbol(*I, "__clang_Interpreter_SetValueWithAlloc",
(uint64_t)&__clang_Interpreter_SetValueWithAlloc);
#else
// obtain mangled name
auto* D = static_cast<clang::Decl*>(
Cpp::GetNamed("__clang_Interpreter_SetValueWithAlloc"));
if (auto* FD = llvm::dyn_cast<FunctionDecl>(D)) {
auto GD = GlobalDecl(FD);
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);
                         ^

}
#endif
// llvm < 19 has multiple overloads of __clang_Interpreter_SetValueNoAlloc
#if CLANG_VERSION_MAJOR < 19
// obtain all 6 candidates, and obtain the correct Decl for each overload
// using BestOverloadFunctionMatch. We then map the decl to the correct
// function pointer (force the compiler to find the right declarion by casting
// to the corresponding function pointer signature) and then register it.
const std::vector<TCppFunction_t> Methods = Cpp::GetFunctionsUsingName(
Cpp::GetGlobalScope(), "__clang_Interpreter_SetValueNoAlloc");
std::string mangledName;
ASTContext& Ctxt = I->getSema().getASTContext();
auto* TAI = Ctxt.VoidPtrTy.getAsOpaquePtr();

// possible parameter lists for __clang_Interpreter_SetValueNoAlloc overloads
// in LLVM 18
const std::vector<std::vector<Cpp::TemplateArgInfo>> a_params = {
{TAI, TAI, TAI},
{TAI, TAI, TAI, TAI},
{TAI, TAI, TAI, Ctxt.FloatTy.getAsOpaquePtr()},
{TAI, TAI, TAI, Ctxt.DoubleTy.getAsOpaquePtr()},
{TAI, TAI, TAI, Ctxt.LongDoubleTy.getAsOpaquePtr()},
{TAI, TAI, TAI, Ctxt.UnsignedLongLongTy.getAsOpaquePtr()}};

using FP0 = void (*)(void*, void*, void*);
using FP1 = void (*)(void*, void*, void*, void*);
using FP2 = void (*)(void*, void*, void*, float);
using FP3 = void (*)(void*, void*, void*, double);
using FP4 = void (*)(void*, void*, void*, long double);
using FP5 = void (*)(void*, void*, void*, unsigned long long);

const std::vector<void*> func_pointers = {
reinterpret_cast<void*>(
static_cast<FP0>(&__clang_Interpreter_SetValueNoAlloc)),
reinterpret_cast<void*>(
static_cast<FP1>(&__clang_Interpreter_SetValueNoAlloc)),
reinterpret_cast<void*>(
static_cast<FP2>(&__clang_Interpreter_SetValueNoAlloc)),
reinterpret_cast<void*>(
static_cast<FP3>(&__clang_Interpreter_SetValueNoAlloc)),
reinterpret_cast<void*>(
static_cast<FP4>(&__clang_Interpreter_SetValueNoAlloc)),
reinterpret_cast<void*>(
static_cast<FP5>(&__clang_Interpreter_SetValueNoAlloc))};

// these symbols are not externed, so we need to mangle their names
for (size_t i = 0; i < a_params.size(); ++i) {
auto* decl = static_cast<clang::Decl*>(
Cpp::BestOverloadFunctionMatch(Methods, {}, a_params[i]));
if (auto* fd = llvm::dyn_cast<clang::FunctionDecl>(decl)) {
auto gd = clang::GlobalDecl(fd);
compat::maybeMangleDeclName(gd, mangledName);
DefineAbsoluteSymbol(*I, mangledName.c_str(),
reinterpret_cast<uint64_t>(func_pointers[i]));
}
}
#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);
                       ^

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);
                       ^

#endif
#endif
return I;
}

Expand Down
32 changes: 32 additions & 0 deletions unittests/CppInterOp/InterpreterTest.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -84,6 +84,38 @@ TEST(InterpreterTest, Evaluate) {
EXPECT_FALSE(HadError) ;
}

TEST(InterpreterTest, EvaluateExtensive) {
#ifdef EMSCRIPTEN
GTEST_SKIP() << "Test fails for Emscipten builds";
#endif
Comment on lines +87 to +90
Copy link
Collaborator

Choose a reason for hiding this comment

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

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?

#ifdef _WIN32
GTEST_SKIP() << "Disabled on Windows. Needs fixing.";
#endif
if (llvm::sys::RunningOnValgrind())
GTEST_SKIP() << "XFAIL due to Valgrind report";
Cpp::CreateInterpreter();
EXPECT_TRUE(Cpp::Evaluate("__cplusplus") == 201402);

bool HadError;
EXPECT_TRUE(Cpp::Evaluate("#error", &HadError) == (intptr_t)~0UL);
EXPECT_TRUE(HadError);
// for llvm < 19 this tests all different overloads of __clang_Interpreter_SetValueNoAlloc
EXPECT_EQ(Cpp::Evaluate("int i = 11; ++i", &HadError), 12);
EXPECT_FALSE(HadError) ;
EXPECT_EQ(Cpp::Evaluate("double a = 12.; a", &HadError), 12.);
EXPECT_FALSE(HadError) ;
EXPECT_EQ(Cpp::Evaluate("float b = 13.; b", &HadError), 13.);
EXPECT_FALSE(HadError) ;
EXPECT_EQ(Cpp::Evaluate("long double c = 14.; c", &HadError), 14.);
EXPECT_FALSE(HadError) ;
EXPECT_EQ(Cpp::Evaluate("long double d = 15.; d", &HadError), 15.);
EXPECT_FALSE(HadError);
EXPECT_EQ(Cpp::Evaluate("unsigned long long e = 16; e", &HadError), 16);
EXPECT_FALSE(HadError) ;
EXPECT_NE(Cpp::Evaluate("struct S{} s; s", &HadError), (intptr_t)~0UL);
EXPECT_FALSE(HadError) ;
}

TEST(InterpreterTest, DeleteInterpreter) {
auto* I1 = Cpp::CreateInterpreter();
auto* I2 = Cpp::CreateInterpreter();
Expand Down
Loading