Skip to content

Stack the created interpreters. #607

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
May 30, 2025
Merged
Show file tree
Hide file tree
Changes from all 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
15 changes: 13 additions & 2 deletions include/CppInterOp/CppInterOp.h
Original file line number Diff line number Diff line change
Expand Up @@ -595,15 +595,26 @@ CPPINTEROP_API void GetOperator(TCppScope_t scope, Operator op,
std::vector<TCppFunction_t>& operators,
OperatorArity kind = kBoth);

/// Creates an instance of the interpreter we need for the various interop
/// services.
/// Creates an owned instance of the interpreter we need for the various interop
/// services and pushes it onto a stack.
///\param[in] Args - the list of arguments for interpreter constructor.
///\param[in] CPPINTEROP_EXTRA_INTERPRETER_ARGS - an env variable, if defined,
/// adds additional arguments to the interpreter.
///\returns nullptr on failure.
CPPINTEROP_API TInterp_t
CreateInterpreter(const std::vector<const char*>& Args = {},
const std::vector<const char*>& GpuArgs = {});

/// Deletes an instance of an interpreter.
///\param[in] I - the interpreter to be deleted, if nullptr, deletes the last.
///\returns false on failure or if \c I is not tracked in the stack.
CPPINTEROP_API bool DeleteInterpreter(TInterp_t I = nullptr);

/// Activates an instance of an interpreter to handle subsequent API requests
///\param[in] I - the interpreter to be activated.
///\returns false on failure.
CPPINTEROP_API bool ActivateInterpreter(TInterp_t I);

/// Checks which Interpreter backend was CppInterOp library built with (Cling,
/// Clang-REPL, etcetera). In practice, the selected interpreter should not
/// matter, since the library will function in the same way.
Expand Down
84 changes: 61 additions & 23 deletions lib/CppInterOp/CppInterOp.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -46,14 +46,18 @@
#include "llvm/ADT/StringRef.h"
#include "llvm/Demangle/Demangle.h"
#include "llvm/Support/Casting.h"
#include "llvm/Support/CommandLine.h"
#include "llvm/Support/Debug.h"
#include "llvm/Support/raw_os_ostream.h"

#include <algorithm>
#include <cassert>
#include <iterator>
#include <map>
#include <memory>
#include <set>
#include <sstream>
#include <stack>
#include <string>

// Stream redirect.
Expand All @@ -71,30 +75,29 @@
#include <unistd.h>
#endif // WIN32

#include <stack>

namespace Cpp {

using namespace clang;
using namespace llvm;
using namespace std;

// Flag to indicate ownership when an external interpreter instance is used.
static bool OwningSInterpreter = true;
static compat::Interpreter* sInterpreter = nullptr;
// Valgrind complains about __cxa_pure_virtual called when deleting
// llvm::SectionMemoryManager::~SectionMemoryManager as part of the dtor chain
// of the Interpreter.
// This might fix the issue https://reviews.llvm.org/D107087
// FIXME: For now we just leak the Interpreter.
struct InterpDeleter {
~InterpDeleter() = default;
} Deleter;
struct InterpreterInfo {
Copy link
Contributor

Choose a reason for hiding this comment

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

warning: class 'InterpreterInfo' defines a non-default destructor but does not define a copy constructor, a copy assignment operator, a move constructor or a move assignment operator [cppcoreguidelines-special-member-functions]

struct InterpreterInfo {
       ^

Copy link
Contributor

Choose a reason for hiding this comment

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

warning: class 'InterpreterInfo' defines a default destructor but does not define a copy constructor, a copy assignment operator, a move constructor or a move assignment operator [cppcoreguidelines-special-member-functions]

struct InterpreterInfo {
       ^

compat::Interpreter* Interpreter = nullptr;
bool isOwned = true;

// Valgrind complains about __cxa_pure_virtual called when deleting
// llvm::SectionMemoryManager::~SectionMemoryManager as part of the dtor
// chain of the Interpreter.
// This might fix the issue https://reviews.llvm.org/D107087
// FIXME: For now we just leak the Interpreter.
~InterpreterInfo() = default;
};
static llvm::SmallVector<InterpreterInfo, 8> sInterpreters;
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 'sInterpreters' is non-const and globally accessible, consider making it const [cppcoreguidelines-avoid-non-const-global-variables]

static llvm::SmallVector<InterpreterInfo, 8> sInterpreters;
                                             ^


static compat::Interpreter& getInterp() {
assert(sInterpreter &&
assert(!sInterpreters.empty() &&
"Interpreter instance must be set before calling this!");
return *sInterpreter;
return *sInterpreters.back().Interpreter;
}
static clang::Sema& getSema() { return getInterp().getCI()->getSema(); }
static clang::ASTContext& getASTContext() { return getSema().getASTContext(); }
Expand Down Expand Up @@ -2920,19 +2923,54 @@
Args[NumArgs + 1] = nullptr;
llvm::cl::ParseCommandLineOptions(NumArgs + 1, Args.get());
}
// FIXME: Enable this assert once we figure out how to fix the multiple
// calls to CreateInterpreter.
// assert(!sInterpreter && "Interpreter already set.");
sInterpreter = I;

sInterpreters.push_back({I, /*isOwned=*/true});
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 declare C-style arrays, use std::array<> instead [cppcoreguidelines-avoid-c-arrays]

    auto Args = std::make_unique<const char*[]>(NumArgs + 2);
                                       ^


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

View check run for this annotation

Codecov / codecov/patch

lib/CppInterOp/CppInterOp.cpp#L2928

Added line #L2928 was not covered by tests
return I;
}

TInterp_t GetInterpreter() { return sInterpreter; }
bool DeleteInterpreter(TInterp_t I /*=nullptr*/) {
Copy link
Contributor

Choose a reason for hiding this comment

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

warning: narrowing conversion from 'unsigned int' to signed type 'int' is implementation-defined [cppcoreguidelines-narrowing-conversions]

    llvm::cl::ParseCommandLineOptions(NumArgs + 1, Args.get());
                                      ^

if (!I) {
sInterpreters.pop_back();
Copy link
Contributor

Choose a reason for hiding this comment

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

warning: narrowing conversion from 'unsigned int' to signed type 'int' is implementation-defined [cppcoreguidelines-narrowing-conversions]

    llvm::cl::ParseCommandLineOptions(NumArgs + 1, Args.get());
                                      ^

return true;

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

View check run for this annotation

Codecov / codecov/patch

lib/CppInterOp/CppInterOp.cpp#L2932-L2935

Added lines #L2932 - L2935 were not covered by tests
}

auto* found =
std::find_if(sInterpreters.begin(), sInterpreters.end(),
[&I](const auto& Info) { return Info.Interpreter == I; });
if (found == sInterpreters.end())
return false; // failure

sInterpreters.erase(found);
return true;
}

bool ActivateInterpreter(TInterp_t I) {
if (!I)
return false;

auto* found =
std::find_if(sInterpreters.begin(), sInterpreters.end(),
[&I](const auto& Info) { return Info.Interpreter == I; });
if (found == sInterpreters.end())
return false;

if (std::next(found) != sInterpreters.end()) // if not already last element.
std::rotate(found, found + 1, sInterpreters.end());

return true; // success
}

TInterp_t GetInterpreter() {
if (sInterpreters.empty())
return nullptr;
return sInterpreters.back().Interpreter;
}

void UseExternalInterpreter(TInterp_t I) {
assert(!sInterpreter && "sInterpreter already in use!");
sInterpreter = static_cast<compat::Interpreter*>(I);
OwningSInterpreter = false;
assert(sInterpreters.empty() && "sInterpreter already in use!");
sInterpreters.push_back(
{static_cast<compat::Interpreter*>(I), /*isOwned=*/false});
}

void AddSearchPath(const char* dir, bool isUser, bool prepend) {
Expand Down
41 changes: 41 additions & 0 deletions unittests/CppInterOp/InterpreterTest.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -80,6 +80,47 @@ TEST(InterpreterTest, Evaluate) {
EXPECT_FALSE(HadError) ;
}

TEST(InterpreterTest, DeleteInterpreter) {
auto* I1 = Cpp::CreateInterpreter();
auto* I2 = Cpp::CreateInterpreter();
auto* I3 = Cpp::CreateInterpreter();
EXPECT_TRUE(I1 && I2 && I3) << "Failed to create interpreters";

EXPECT_EQ(I3, Cpp::GetInterpreter()) << "I3 is not active";

EXPECT_TRUE(Cpp::DeleteInterpreter(nullptr));
EXPECT_EQ(I2, Cpp::GetInterpreter());

auto* I4 = reinterpret_cast<void*>(static_cast<std::uintptr_t>(~0U));
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 reinterpret_cast [cppcoreguidelines-pro-type-reinterpret-cast]

  auto* I4 = reinterpret_cast<void*>(static_cast<std::uintptr_t>(~0U));
             ^

Copy link
Contributor

Choose a reason for hiding this comment

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

warning: integer to pointer cast pessimizes optimization opportunities [performance-no-int-to-ptr]

  auto* I4 = reinterpret_cast<void*>(static_cast<std::uintptr_t>(~0U));
             ^

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 "std::uintptr_t" is directly included [misc-include-cleaner]

  auto* I4 = reinterpret_cast<void*>(static_cast<std::uintptr_t>(~0U));
                                                      ^

EXPECT_FALSE(Cpp::DeleteInterpreter(I4));

EXPECT_TRUE(Cpp::DeleteInterpreter(I1));
EXPECT_EQ(I2, Cpp::GetInterpreter()) << "I2 is not active";
}

TEST(InterpreterTest, ActivateInterpreter) {
EXPECT_FALSE(Cpp::ActivateInterpreter(nullptr));
auto* Cpp14 = Cpp::CreateInterpreter({"-std=c++14"});
auto* Cpp17 = Cpp::CreateInterpreter({"-std=c++17"});
auto* Cpp20 = Cpp::CreateInterpreter({"-std=c++20"});

EXPECT_TRUE(Cpp14 && Cpp17 && Cpp20);
EXPECT_TRUE(Cpp::Evaluate("__cplusplus") == 202002L)
<< "Failed to activate C++20";

auto* UntrackedI = reinterpret_cast<void*>(static_cast<std::uintptr_t>(~0U));
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 reinterpret_cast [cppcoreguidelines-pro-type-reinterpret-cast]

  auto* UntrackedI = reinterpret_cast<void*>(static_cast<std::uintptr_t>(~0U));
                     ^

Copy link
Contributor

Choose a reason for hiding this comment

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

warning: integer to pointer cast pessimizes optimization opportunities [performance-no-int-to-ptr]

  auto* UntrackedI = reinterpret_cast<void*>(static_cast<std::uintptr_t>(~0U));
                     ^

EXPECT_FALSE(Cpp::ActivateInterpreter(UntrackedI));

EXPECT_TRUE(Cpp::ActivateInterpreter(Cpp14));
EXPECT_TRUE(Cpp::Evaluate("__cplusplus") == 201402L);

Cpp::DeleteInterpreter(Cpp14);
EXPECT_EQ(Cpp::GetInterpreter(), Cpp20);

EXPECT_TRUE(Cpp::ActivateInterpreter(Cpp17));
EXPECT_TRUE(Cpp::Evaluate("__cplusplus") == 201703L);
}

TEST(InterpreterTest, Process) {
#ifdef _WIN32
GTEST_SKIP() << "Disabled on Windows. Needs fixing.";
Expand Down
Loading