From f6fb8355454b54353ed31ff9cdf1e76fdf840fec Mon Sep 17 00:00:00 2001 From: Vipul Cariappa Date: Mon, 11 Aug 2025 14:40:26 +0200 Subject: [PATCH 1/5] Thread-Safty - Add Mutex per Interpreter --- lib/CppInterOp/CppInterOp.cpp | 154 +++++++++++++++++++++++++++++----- 1 file changed, 133 insertions(+), 21 deletions(-) diff --git a/lib/CppInterOp/CppInterOp.cpp b/lib/CppInterOp/CppInterOp.cpp index b6cf555e8..c0f615303 100755 --- a/lib/CppInterOp/CppInterOp.cpp +++ b/lib/CppInterOp/CppInterOp.cpp @@ -65,11 +65,13 @@ #include #include #include +#include #include #include #include #include #include +#include // Stream redirect. #ifdef _WIN32 @@ -92,9 +94,15 @@ using namespace clang; using namespace llvm; using namespace std; +#define LOCK(InterpInfo) \ + std::lock_guard interop_lock( \ + (InterpInfo).InterpreterLock) + struct InterpreterInfo { compat::Interpreter* Interpreter = nullptr; bool isOwned = true; + std::recursive_mutex InterpreterLock; + InterpreterInfo(compat::Interpreter* I, bool Owned) : Interpreter(I), isOwned(Owned) {} @@ -131,8 +139,17 @@ struct InterpreterInfo { // std::deque avoids relocations and calling the dtor of InterpreterInfo. static llvm::ManagedStatic> sInterpreters; +static std::mutex InterpreterStackLock; + +static InterpreterInfo& getInterpInfo() { + std::unique_lock Lock(InterpreterStackLock); + assert(!sInterpreters->empty() && + "Interpreter instance must be set before calling this!"); + return sInterpreters->back(); +} static compat::Interpreter& getInterp() { + std::unique_lock Lock(InterpreterStackLock); assert(!sInterpreters->empty() && "Interpreter instance must be set before calling this!"); return *sInterpreters->back().Interpreter; @@ -266,11 +283,18 @@ std::string Demangle(const std::string& mangled_name) { return demangle; } -void EnableDebugOutput(bool value /* =true*/) { llvm::DebugFlag = value; } +void EnableDebugOutput(bool value /* =true*/) { + LOCK(getInterpInfo()); + llvm::DebugFlag = value; +} -bool IsDebugOutputEnabled() { return llvm::DebugFlag; } +bool IsDebugOutputEnabled() { + LOCK(getInterpInfo()); + return llvm::DebugFlag; +} static void InstantiateFunctionDefinition(Decl* D) { + LOCK(getInterpInfo()); compat::SynthesizingCodeRAII RAII(&getInterp()); if (auto* FD = llvm::dyn_cast_or_null(D)) { getSema().InstantiateFunctionDefinition(SourceLocation(), FD, @@ -280,6 +304,7 @@ static void InstantiateFunctionDefinition(Decl* D) { } bool IsAggregate(TCppScope_t scope) { + LOCK(getInterpInfo()); Decl* D = static_cast(scope); // Aggregates are only arrays or tag decls. @@ -315,6 +340,7 @@ bool IsFunctionPointerType(TCppType_t type) { } bool IsClassPolymorphic(TCppScope_t klass) { + LOCK(getInterpInfo()); Decl* D = static_cast(klass); if (auto* CXXRD = llvm::dyn_cast(D)) if (auto* CXXRDD = CXXRD->getDefinition()) @@ -323,6 +349,7 @@ bool IsClassPolymorphic(TCppScope_t klass) { } static SourceLocation GetValidSLoc(Sema& semaRef) { + LOCK(getInterpInfo()); auto& SM = semaRef.getSourceManager(); return SM.getLocForStartOfFile(SM.getMainFileID()); } @@ -334,6 +361,7 @@ bool IsComplete(TCppScope_t scope) { Decl* D = static_cast(scope); + LOCK(getInterpInfo()); if (isa(D)) { QualType QT = QualType::getFromOpaquePtr(GetTypeFromScope(scope)); clang::Sema& S = getSema(); @@ -358,6 +386,7 @@ size_t SizeOf(TCppScope_t scope) { if (!IsComplete(scope)) return 0; + LOCK(getInterpInfo()); if (auto* RD = dyn_cast(static_cast(scope))) { ASTContext& Context = RD->getASTContext(); const ASTRecordLayout& Layout = Context.getASTRecordLayout(RD); @@ -392,8 +421,10 @@ bool IsTypedefed(TCppScope_t handle) { bool IsAbstract(TCppType_t klass) { auto* D = (clang::Decl*)klass; - if (auto* CXXRD = llvm::dyn_cast_or_null(D)) + if (auto* CXXRD = llvm::dyn_cast_or_null(D)) { + LOCK(getInterpInfo()); return CXXRD->isAbstract(); + } return false; } @@ -414,6 +445,7 @@ bool IsEnumType(TCppType_t type) { } static bool isSmartPointer(const RecordType* RT) { + LOCK(getInterpInfo()); auto IsUseCountPresent = [](const RecordDecl* Record) { ASTContext& C = Record->getASTContext(); return !Record->lookup(&C.Idents.get("use_count")).empty(); @@ -493,6 +525,7 @@ TCppType_t GetIntegerTypeFromEnumType(TCppType_t enum_type) { std::vector GetEnumConstants(TCppScope_t handle) { auto* D = (clang::Decl*)handle; + LOCK(getInterpInfo()); if (auto* ED = llvm::dyn_cast_or_null(D)) { std::vector enum_constants; for (auto* ECD : ED->enumerators()) { @@ -526,6 +559,7 @@ TCppIndex_t GetEnumConstantValue(TCppScope_t handle) { } size_t GetSizeOfType(TCppType_t type) { + LOCK(getInterpInfo()); QualType QT = QualType::getFromOpaquePtr(type); if (const TagType* TT = QT->getAs()) return SizeOf(TT->getDecl()); @@ -634,6 +668,7 @@ std::string GetQualifiedCompleteName(TCppType_t klass) { } std::vector GetUsingNamespaces(TCppScope_t scope) { + LOCK(getInterpInfo()); auto* D = (clang::Decl*)scope; if (auto* DC = llvm::dyn_cast_or_null(D)) { @@ -723,6 +758,7 @@ TCppScope_t GetScopeFromCompleteName(const std::string& name) { TCppScope_t GetNamed(const std::string& name, TCppScope_t parent /*= nullptr*/) { + LOCK(getInterpInfo()); clang::DeclContext* Within = 0; if (parent) { auto* D = (clang::Decl*)parent; @@ -758,6 +794,7 @@ TCppScope_t GetParentScope(TCppScope_t scope) { } TCppIndex_t GetNumBases(TCppScope_t klass) { + LOCK(getInterpInfo()); auto* D = (Decl*)klass; if (auto* CTSD = llvm::dyn_cast_or_null(D)) @@ -772,6 +809,7 @@ TCppIndex_t GetNumBases(TCppScope_t klass) { } TCppScope_t GetBaseClass(TCppScope_t klass, TCppIndex_t ibase) { + LOCK(getInterpInfo()); auto* D = (Decl*)klass; auto* CXXRD = llvm::dyn_cast_or_null(D); if (!CXXRD || CXXRD->getNumBases() <= ibase) @@ -811,6 +849,7 @@ bool IsSubclass(TCppScope_t derived, TCppScope_t base) { static unsigned ComputeBaseOffset(const ASTContext& Context, const CXXRecordDecl* DerivedRD, const CXXBasePath& Path) { + LOCK(getInterpInfo()); CharUnits NonVirtualOffset = CharUnits::Zero(); unsigned NonVirtualStart = 0; @@ -864,6 +903,9 @@ int64_t GetBaseClassOffset(TCppScope_t derived, TCppScope_t base) { return -1; CXXRecordDecl* DCXXRD = cast(DD); CXXRecordDecl* BCXXRD = cast(BD); + + LOCK(getInterpInfo()); + CXXBasePaths Paths(/*FindAmbiguities=*/false, /*RecordPaths=*/true, /*DetectVirtual=*/false); DCXXRD->isDerivedFrom(BCXXRD, Paths); @@ -878,6 +920,7 @@ static void GetClassDecls(TCppScope_t klass, if (!klass) return; + LOCK(getInterpInfo()); auto* D = (clang::Decl*)klass; if (auto* TD = dyn_cast(D)) @@ -935,6 +978,7 @@ void GetFunctionTemplatedDecls(TCppScope_t klass, bool HasDefaultConstructor(TCppScope_t scope) { auto* D = (clang::Decl*)scope; + LOCK(getInterpInfo()); if (auto* CXXRD = llvm::dyn_cast_or_null(D)) return CXXRD->hasDefaultConstructor(); @@ -946,6 +990,7 @@ TCppFunction_t GetDefaultConstructor(compat::Interpreter& interp, if (!HasDefaultConstructor(scope)) return nullptr; + LOCK(getInterpInfo()); auto* CXXRD = (clang::CXXRecordDecl*)scope; return interp.getCI()->getSema().LookupDefaultConstructor(CXXRD); } @@ -958,6 +1003,7 @@ TCppFunction_t GetDestructor(TCppScope_t scope) { auto* D = (clang::Decl*)scope; if (auto* CXXRD = llvm::dyn_cast_or_null(D)) { + LOCK(getInterpInfo()); getSema().ForceDeclarationOfImplicitMembers(CXXRD); return CXXRD->getDestructor(); } @@ -977,6 +1023,8 @@ std::vector GetFunctionsUsingName(TCppScope_t scope, if (!scope || name.empty()) return {}; + LOCK(getInterpInfo()); + D = GetUnderlyingScope(D); std::vector funcs; @@ -1001,6 +1049,8 @@ std::vector GetFunctionsUsingName(TCppScope_t scope, } TCppType_t GetFunctionReturnType(TCppFunction_t func) { + LOCK(getInterpInfo()); + auto* D = (clang::Decl*)func; if (auto* FD = llvm::dyn_cast_or_null(D)) { QualType Type = FD->getReturnType(); @@ -1050,8 +1100,9 @@ TCppIndex_t GetFunctionRequiredArgs(TCppConstFunction_t func) { } TCppType_t GetFunctionArgType(TCppFunction_t func, TCppIndex_t iarg) { - auto* D = (clang::Decl*)func; + LOCK(getInterpInfo()); + auto* D = (clang::Decl*)func; if (auto* FD = llvm::dyn_cast_or_null(D)) { if (iarg < FD->getNumParams()) { auto* PVD = FD->getParamDecl(iarg); @@ -1130,6 +1181,8 @@ bool ExistsFunctionTemplate(const std::string& name, TCppScope_t parent) { Within = llvm::dyn_cast(D); } + LOCK(getInterpInfo()); + auto* ND = Cpp_utils::Lookup::Named(&getSema(), name, Within); if ((intptr_t)ND == (intptr_t)0) @@ -1149,6 +1202,8 @@ void LookupConstructors(const std::string& name, TCppScope_t parent, auto* D = (Decl*)parent; if (auto* CXXRD = llvm::dyn_cast_or_null(D)) { + LOCK(getInterpInfo()); + getSema().ForceDeclarationOfImplicitMembers(CXXRD); DeclContextLookupResult Result = getSema().LookupConstructors(CXXRD); // Obtaining all constructors when we intend to lookup a method under a @@ -1166,6 +1221,8 @@ bool GetClassTemplatedMethods(const std::string& name, TCppScope_t parent, if (!D && name.empty()) return false; + LOCK(getInterpInfo()); + // Accumulate constructors LookupConstructors(name, parent, funcs); auto& S = getSema(); @@ -1207,6 +1264,8 @@ TCppFunction_t BestOverloadFunctionMatch(const std::vector& candidates, const std::vector& explicit_types, const std::vector& arg_types) { + LOCK(getInterpInfo()); + auto& S = getSema(); auto& C = S.getASTContext(); @@ -1328,6 +1387,8 @@ bool IsDestructor(TCppConstFunction_t method) { } bool IsStaticMethod(TCppConstFunction_t method) { + LOCK(getInterpInfo()); + const auto* D = static_cast(method); if (auto* CXXMD = llvm::dyn_cast_or_null(D)) { return CXXMD->isStatic(); @@ -1389,6 +1450,7 @@ TCppFuncAddr_t GetFunctionAddress(TCppFunction_t method) { } bool IsVirtualMethod(TCppFunction_t method) { + LOCK(getInterpInfo()); auto* D = (Decl*)method; if (auto* CXXMD = llvm::dyn_cast_or_null(D)) { return CXXMD->isVirtual(); @@ -1401,6 +1463,8 @@ void GetDatamembers(TCppScope_t scope, std::vector& datamembers) { auto* D = (Decl*)scope; if (auto* CXXRD = llvm::dyn_cast_or_null(D)) { + LOCK(getInterpInfo()); + getSema().ForceDeclarationOfImplicitMembers(CXXRD); if (CXXRD->hasDefinition()) CXXRD = CXXRD->getDefinition(); @@ -1446,6 +1510,8 @@ void GetStaticDatamembers(TCppScope_t scope, void GetEnumConstantDatamembers(TCppScope_t scope, std::vector& datamembers, bool include_enum_class) { + LOCK(getInterpInfo()); + std::vector EDs; GetClassDecls(scope, EDs); for (TCppScope_t i : EDs) { @@ -1467,6 +1533,7 @@ TCppScope_t LookupDatamember(const std::string& name, TCppScope_t parent) { Within = llvm::dyn_cast(D); } + LOCK(getInterpInfo()); auto* ND = Cpp_utils::Lookup::Named(&getSema(), name, Within); if (ND && ND != (clang::NamedDecl*)-1) { if (llvm::isa_and_nonnull(ND)) { @@ -1512,6 +1579,8 @@ intptr_t GetVariableOffset(compat::Interpreter& I, Decl* D, if (!D) return 0; + LOCK(getInterpInfo()); + auto& C = I.getSema().getASTContext(); if (auto* FD = llvm::dyn_cast(D)) { @@ -1643,11 +1712,7 @@ bool IsPrivateVariable(TCppScope_t var) { bool IsStaticVariable(TCppScope_t var) { auto* D = (Decl*)var; - if (llvm::isa_and_nonnull(D)) { - return true; - } - - return false; + return llvm::isa_and_nonnull(D); } bool IsConstVariable(TCppScope_t var) { @@ -1881,6 +1946,7 @@ TCppType_t GetType(const std::string& name) { if (!builtin.isNull()) return builtin.getAsOpaquePtr(); + LOCK(getInterpInfo()); auto* D = (Decl*)GetNamed(name, /* Within= */ 0); if (auto* TD = llvm::dyn_cast_or_null(D)) { return QualType(TD->getTypeForDecl(), 0).getAsOpaquePtr(); @@ -1892,6 +1958,7 @@ TCppType_t GetType(const std::string& name) { TCppType_t GetComplexType(TCppType_t type) { QualType QT = QualType::getFromOpaquePtr(type); + LOCK(getInterpInfo()); return getASTContext().getComplexType(QT).getAsOpaquePtr(); } @@ -1931,6 +1998,7 @@ void* compile_wrapper(compat::Interpreter& I, const std::string& wrapper_name, const std::string& wrapper, bool withAccessControl = true) { LLVM_DEBUG(dbgs() << "Compiling '" << wrapper_name << "'\n"); + LOCK(getInterpInfo()); return I.compileFunction(wrapper_name, wrapper, false /*ifUnique*/, withAccessControl); } @@ -1939,8 +2007,10 @@ void get_type_as_string(QualType QT, std::string& type_name, ASTContext& C, PrintingPolicy Policy) { // TODO: Implement cling desugaring from utils::AST // cling::utils::Transform::GetPartiallyDesugaredType() - if (!QT->isTypedefNameType() || QT->isBuiltinType()) + if (!QT->isTypedefNameType() || QT->isBuiltinType()) { + LOCK(getInterpInfo()); QT = QT.getDesugaredType(C); + } Policy.SuppressElaboration = true; Policy.SuppressTagKeyword = !QT->isEnumeralType(); Policy.FullyQualifiedName = true; @@ -2289,12 +2359,14 @@ void make_narg_call(const FunctionDecl* FD, const std::string& return_type, // a simple way of determining whether a viable copy constructor exists, // so check for the most common case: the trivial one, but not uniquely // available, while there is a move constructor. - - // include utility header if not already included for std::move - DeclarationName DMove = &getASTContext().Idents.get("move"); - auto result = getSema().getStdNamespace()->lookup(DMove); - if (result.empty()) - Cpp::Declare("#include "); + { + LOCK(getInterpInfo()); + // include utility header if not already included for std::move + DeclarationName DMove = &getASTContext().Idents.get("move"); + auto result = getSema().getStdNamespace()->lookup(DMove); + if (result.empty()) + Cpp::Declare("#include "); + } // move construction as needed for classes (note that this is implicit) callbuf << "std::move(*(" << type_name.c_str() << "*)args[" << i << "])"; @@ -2505,6 +2577,8 @@ void make_narg_call_with_return(compat::Interpreter& I, const FunctionDecl* FD, int get_wrapper_code(compat::Interpreter& I, const FunctionDecl* FD, std::string& wrapper_name, std::string& wrapper) { assert(FD && "generate_wrapper called without a function decl!"); + LOCK(getInterpInfo()); + ASTContext& Context = FD->getASTContext(); // // Get the class or namespace name. @@ -2925,6 +2999,8 @@ JitCall::GenericCall make_wrapper(compat::Interpreter& I, const FunctionDecl* FD) { static std::map gWrapperStore; + LOCK(getInterpInfo()); + auto R = gWrapperStore.find(FD); if (R != gWrapperStore.end()) return (JitCall::GenericCall)R->second; @@ -3017,6 +3093,8 @@ static JitCall::DestructorCall make_dtor_wrapper(compat::Interpreter& interp, static map gDtorWrapperStore; + LOCK(getInterpInfo()); + auto I = gDtorWrapperStore.find(D); if (I != gDtorWrapperStore.end()) return (JitCall::DestructorCall)I->second; @@ -3196,6 +3274,7 @@ static std::string MakeResourcesPath() { TInterp_t CreateInterpreter(const std::vector& Args /*={}*/, const std::vector& GpuArgs /*={}*/) { + std::unique_lock Lock(InterpreterStackLock); std::string MainExecutableName = sys::fs::getMainExecutable(nullptr, nullptr); std::string ResourceDir = MakeResourcesPath(); std::vector ClingArgv = {"-resource-dir", ResourceDir.c_str(), @@ -3277,6 +3356,8 @@ TInterp_t CreateInterpreter(const std::vector& Args /*={}*/, } bool DeleteInterpreter(TInterp_t I /*=nullptr*/) { + std::unique_lock Lock(InterpreterStackLock); + if (!I) { sInterpreters->pop_back(); return true; @@ -3288,11 +3369,14 @@ bool DeleteInterpreter(TInterp_t I /*=nullptr*/) { if (found == sInterpreters->end()) return false; // failure + LOCK(*found); sInterpreters->erase(found); return true; } bool ActivateInterpreter(TInterp_t I) { + std::unique_lock Lock(InterpreterStackLock); + if (!I) return false; @@ -3309,18 +3393,21 @@ bool ActivateInterpreter(TInterp_t I) { } TInterp_t GetInterpreter() { + std::unique_lock Lock(InterpreterStackLock); if (sInterpreters->empty()) return nullptr; return sInterpreters->back().Interpreter; } void UseExternalInterpreter(TInterp_t I) { + std::unique_lock Lock(InterpreterStackLock); assert(sInterpreters->empty() && "sInterpreter already in use!"); sInterpreters->emplace_back(static_cast(I), /*isOwned=*/false); } void AddSearchPath(const char* dir, bool isUser, bool prepend) { + LOCK(getInterpInfo()); getInterp().getDynamicLibraryManager()->addSearchPath(dir, isUser, prepend); } @@ -3384,7 +3471,10 @@ void DetectSystemCompilerIncludePaths(std::vector& Paths, exec(cmd.c_str(), Paths); } -void AddIncludePath(const char* dir) { getInterp().AddIncludePath(dir); } +void AddIncludePath(const char* dir) { + LOCK(getInterpInfo()); + getInterp().AddIncludePath(dir); +} void GetIncludePaths(std::vector& IncludePaths, bool withSystem, bool withFlags) { @@ -3421,10 +3511,14 @@ int Declare(compat::Interpreter& I, const char* code, bool silent) { } int Declare(const char* code, bool silent) { + LOCK(getInterpInfo()); return Declare(getInterp(), code, silent); } -int Process(const char* code) { return getInterp().process(code); } +int Process(const char* code) { + LOCK(getInterpInfo()); + return getInterp().process(code); +} intptr_t Evaluate(const char* code, bool* HadError /*=nullptr*/) { #ifdef CPPINTEROP_USE_CLING @@ -3436,6 +3530,7 @@ intptr_t Evaluate(const char* code, bool* HadError /*=nullptr*/) { if (HadError) *HadError = false; + LOCK(getInterpInfo()); auto res = getInterp().evaluate(code, V); if (res != 0) { // 0 is success if (HadError) @@ -3452,6 +3547,7 @@ std::string LookupLibrary(const char* lib_name) { } bool LoadLibrary(const char* lib_stem, bool lookup) { + LOCK(getInterpInfo()); compat::Interpreter::CompilationResult res = getInterp().loadLibrary(lib_stem, lookup); @@ -3459,11 +3555,13 @@ bool LoadLibrary(const char* lib_stem, bool lookup) { } void UnloadLibrary(const char* lib_stem) { + LOCK(getInterpInfo()); getInterp().getDynamicLibraryManager()->unloadLibrary(lib_stem); } std::string SearchLibrariesForSymbol(const char* mangled_name, bool search_system /*true*/) { + LOCK(getInterpInfo()); auto* DLM = getInterp().getDynamicLibraryManager(); return DLM->searchLibrariesForSymbol(mangled_name, search_system); } @@ -3549,16 +3647,19 @@ bool InsertOrReplaceJitSymbol(compat::Interpreter& I, bool InsertOrReplaceJitSymbol(const char* linker_mangled_name, uint64_t address) { + LOCK(getInterpInfo()); return InsertOrReplaceJitSymbol(getInterp(), linker_mangled_name, address); } std::string ObjToString(const char* type, void* obj) { + LOCK(getInterpInfo()); return getInterp().toString(type, obj); } -static Decl* InstantiateTemplate(TemplateDecl* TemplateD, - TemplateArgumentListInfo& TLI, Sema& S, - bool instantiate_body) { +Decl* InstantiateTemplate(TemplateDecl* TemplateD, + TemplateArgumentListInfo& TLI, Sema& S, + bool instantiate_body) { + LOCK(getInterpInfo()); // This is not right but we don't have a lot of options to choose from as a // template instantiation requires a valid source location. SourceLocation fakeLoc = GetValidSLoc(S); @@ -3609,6 +3710,7 @@ static Decl* InstantiateTemplate(TemplateDecl* TemplateD, Decl* InstantiateTemplate(TemplateDecl* TemplateD, ArrayRef TemplateArgs, Sema& S, bool instantiate_body) { + LOCK(getInterpInfo()); // Create a list of template arguments. TemplateArgumentListInfo TLI{}; for (auto TA : TemplateArgs) @@ -3653,12 +3755,14 @@ TCppScope_t InstantiateTemplate(TCppScope_t tmpl, const TemplateArgInfo* template_args, size_t template_args_size, bool instantiate_body) { + LOCK(getInterpInfo()); return InstantiateTemplate(getInterp(), tmpl, template_args, template_args_size, instantiate_body); } void GetClassTemplateInstantiationArgs(TCppScope_t templ_instance, std::vector& args) { + LOCK(getInterpInfo()); auto* CTSD = static_cast(templ_instance); for (const auto& TA : CTSD->getTemplateInstantiationArgs().asArray()) { switch (TA.getKind()) { @@ -3691,6 +3795,7 @@ InstantiateTemplateFunctionFromString(const char* function_template) { std::string id = "__Cppyy_GetMethTmpl_" + std::to_string(var_count++); std::string instance = "auto " + id + " = " + function_template + ";\n"; + LOCK(getInterpInfo()); if (!Cpp::Declare(instance.c_str(), /*silent=*/false)) { VarDecl* VD = (VarDecl*)Cpp::GetNamed(id, 0); DeclRefExpr* DRE = (DeclRefExpr*)VD->getInit()->IgnoreImpCasts(); @@ -3704,6 +3809,7 @@ void GetAllCppNames(TCppScope_t scope, std::set& names) { clang::DeclContext* DC; clang::DeclContext::decl_iterator decl; + LOCK(getInterpInfo()); if (auto* TD = dyn_cast_or_null(D)) { DC = clang::TagDecl::castToDeclContext(TD); decl = DC->decls_begin(); @@ -3733,6 +3839,7 @@ void GetEnums(TCppScope_t scope, std::vector& Result) { auto* DC = llvm::dyn_cast(D); + LOCK(getInterpInfo()); llvm::SmallVector DCs; DC->collectAllContexts(DCs); @@ -3749,6 +3856,7 @@ void GetEnums(TCppScope_t scope, std::vector& Result) { // FIXME: On the CPyCppyy side the receiver is of type // vector instead of vector std::vector GetDimensions(TCppType_t type) { + LOCK(getInterpInfo()); QualType Qual = QualType::getFromOpaquePtr(type); if (Qual.isNull()) return {}; @@ -3775,6 +3883,7 @@ std::vector GetDimensions(TCppType_t type) { } bool IsTypeDerivedFrom(TCppType_t derived, TCppType_t base) { + LOCK(getInterpInfo()); auto& S = getSema(); auto fakeLoc = GetValidSLoc(S); auto derivedType = clang::QualType::getFromOpaquePtr(derived); @@ -3788,6 +3897,7 @@ bool IsTypeDerivedFrom(TCppType_t derived, TCppType_t base) { std::string GetFunctionArgDefault(TCppFunction_t func, TCppIndex_t param_index) { + LOCK(getInterpInfo()); auto* D = (clang::Decl*)func; clang::ParmVarDecl* PI = nullptr; @@ -3885,6 +3995,7 @@ OperatorArity GetOperatorArity(TCppFunction_t op) { void GetOperator(TCppScope_t scope, Operator op, std::vector& operators, OperatorArity kind) { + LOCK(getInterpInfo()); Decl* D = static_cast(scope); if (auto* CXXRD = llvm::dyn_cast_or_null(D)) { auto fn = [&operators, kind, op](const RecordDecl* RD) { @@ -4070,6 +4181,7 @@ std::string EndStdStreamCapture() { void CodeComplete(std::vector& Results, const char* code, unsigned complete_line /* = 1U */, unsigned complete_column /* = 1U */) { + LOCK(getInterpInfo()); compat::codeComplete(Results, getInterp(), code, complete_line, complete_column); } From cfce81012d57104ae8d0a1be8fb7218f5713b7b1 Mon Sep 17 00:00:00 2001 From: Vipul Cariappa Date: Wed, 13 Aug 2025 13:27:37 +0200 Subject: [PATCH 2/5] using hashmap to identify Sema & AST for individual Decl We need to identify which interpreter a Decl belongs to, when using multiple interpreter. We do it by checking which `clang::ASTContext` the `clang::Decl` belongs We maintain a map: `clang::ASTContext -> Cpp::InterpreterInfo`. Using this map, be identify the correct interpreter. There are 2 usecases for this: 1. We can now lock the correct interpreter making it thread safe. 2. User of `libCppInterOp` need not set the correct active interpreter using `Cpp::ActivateInterpreter`, this information can be retrived using the map. --- lib/CppInterOp/CppInterOp.cpp | 370 ++++++++++++++--------- unittests/CppInterOp/InterpreterTest.cpp | 23 ++ 2 files changed, 248 insertions(+), 145 deletions(-) diff --git a/lib/CppInterOp/CppInterOp.cpp b/lib/CppInterOp/CppInterOp.cpp index c0f615303..8338f0cc6 100755 --- a/lib/CppInterOp/CppInterOp.cpp +++ b/lib/CppInterOp/CppInterOp.cpp @@ -70,6 +70,7 @@ #include #include #include +#include #include #include @@ -137,25 +138,80 @@ struct InterpreterInfo { InterpreterInfo& operator=(const InterpreterInfo&) = delete; }; +// NOLINTBEGIN // std::deque avoids relocations and calling the dtor of InterpreterInfo. -static llvm::ManagedStatic> sInterpreters; -static std::mutex InterpreterStackLock; +static llvm::ManagedStatic>> + sInterpreters; +static llvm::ManagedStatic< + std::unordered_map>> + sInterpreterASTMap; +static std::recursive_mutex InterpreterStackLock; +static std::recursive_mutex LLVMLock; +// NOLINTEND static InterpreterInfo& getInterpInfo() { - std::unique_lock Lock(InterpreterStackLock); + std::lock_guard Lock(InterpreterStackLock); assert(!sInterpreters->empty() && "Interpreter instance must be set before calling this!"); - return sInterpreters->back(); + return *sInterpreters->back(); +} +static InterpreterInfo& getInterpInfo(const clang::Decl* D) { + std::lock_guard Lock(InterpreterStackLock); + if (!D) + return getInterpInfo(); + if (sInterpreters->size() == 1) + return *sInterpreters->back(); + return *(*sInterpreterASTMap)[&D->getASTContext()].lock(); +} +static InterpreterInfo& getInterpInfo(const void* D) { + std::lock_guard Lock(InterpreterStackLock); + if (!D) + return getInterpInfo(); + if (sInterpreters->size() == 1) + return *sInterpreters->back(); + for (auto& item : *sInterpreterASTMap) { + if (item.first->getAllocator().identifyObject(D)) + return *item.second.lock(); + } + llvm_unreachable( + "This pointer does not belong to any interpreter instance.\n"); } static compat::Interpreter& getInterp() { - std::unique_lock Lock(InterpreterStackLock); + std::lock_guard Lock(InterpreterStackLock); assert(!sInterpreters->empty() && "Interpreter instance must be set before calling this!"); - return *sInterpreters->back().Interpreter; + return *sInterpreters->back()->Interpreter; +} +static compat::Interpreter& getInterp(const clang::Decl* D) { + std::lock_guard Lock(InterpreterStackLock); + if (!D) + return getInterp(); + if (sInterpreters->size() == 1) + return *sInterpreters->back()->Interpreter; + return *(*sInterpreterASTMap)[&D->getASTContext()].lock()->Interpreter; +} +static compat::Interpreter& getInterp(const void* D) { + return *getInterpInfo(D).Interpreter; } + static clang::Sema& getSema() { return getInterp().getCI()->getSema(); } +static clang::Sema& getSema(const clang::Decl* D) { + if (!D) + return getSema(); + return getInterpInfo(D).Interpreter->getSema(); +} +static clang::Sema& getSema(const void* D) { return getInterp(D).getSema(); } + static clang::ASTContext& getASTContext() { return getSema().getASTContext(); } +static clang::ASTContext& getASTContext(const clang::Decl* D) { + if (!D) + return getASTContext(); + return getSema(D).getASTContext(); +} +static clang::ASTContext& getASTContext(const void* D) { + return getSema(D).getASTContext(); +} static void ForceCodeGen(Decl* D, compat::Interpreter& I) { // The decl was deferred by CodeGen. Force its emission. @@ -284,28 +340,28 @@ std::string Demangle(const std::string& mangled_name) { } void EnableDebugOutput(bool value /* =true*/) { - LOCK(getInterpInfo()); + std::lock_guard Lock(LLVMLock); llvm::DebugFlag = value; } bool IsDebugOutputEnabled() { - LOCK(getInterpInfo()); + std::lock_guard Lock(LLVMLock); return llvm::DebugFlag; } static void InstantiateFunctionDefinition(Decl* D) { - LOCK(getInterpInfo()); - compat::SynthesizingCodeRAII RAII(&getInterp()); if (auto* FD = llvm::dyn_cast_or_null(D)) { - getSema().InstantiateFunctionDefinition(SourceLocation(), FD, - /*Recursive=*/true, - /*DefinitionRequired=*/true); + LOCK(getInterpInfo(FD)); + compat::SynthesizingCodeRAII RAII(&getInterp(FD)); + getSema(FD).InstantiateFunctionDefinition(SourceLocation(), FD, + /*Recursive=*/true, + /*DefinitionRequired=*/true); } } bool IsAggregate(TCppScope_t scope) { - LOCK(getInterpInfo()); Decl* D = static_cast(scope); + LOCK(getInterpInfo(D)); // Aggregates are only arrays or tag decls. if (ValueDecl* ValD = dyn_cast(D)) @@ -340,16 +396,16 @@ bool IsFunctionPointerType(TCppType_t type) { } bool IsClassPolymorphic(TCppScope_t klass) { - LOCK(getInterpInfo()); Decl* D = static_cast(klass); - if (auto* CXXRD = llvm::dyn_cast(D)) + if (auto* CXXRD = llvm::dyn_cast(D)) { + LOCK(getInterpInfo(CXXRD)); if (auto* CXXRDD = CXXRD->getDefinition()) return CXXRDD->isPolymorphic(); + } return false; } static SourceLocation GetValidSLoc(Sema& semaRef) { - LOCK(getInterpInfo()); auto& SM = semaRef.getSourceManager(); return SM.getLocForStartOfFile(SM.getMainFileID()); } @@ -361,13 +417,13 @@ bool IsComplete(TCppScope_t scope) { Decl* D = static_cast(scope); - LOCK(getInterpInfo()); + LOCK(getInterpInfo(D)); if (isa(D)) { QualType QT = QualType::getFromOpaquePtr(GetTypeFromScope(scope)); - clang::Sema& S = getSema(); + clang::Sema& S = getSema(D); SourceLocation fakeLoc = GetValidSLoc(S); #ifdef CPPINTEROP_USE_CLING - cling::Interpreter::PushTransactionRAII RAII(&getInterp()); + cling::Interpreter::PushTransactionRAII RAII(&getInterp(D)); #endif // CPPINTEROP_USE_CLING return S.isCompleteType(fakeLoc, QT); } @@ -386,8 +442,8 @@ size_t SizeOf(TCppScope_t scope) { if (!IsComplete(scope)) return 0; - LOCK(getInterpInfo()); if (auto* RD = dyn_cast(static_cast(scope))) { + LOCK(getInterpInfo(RD)); ASTContext& Context = RD->getASTContext(); const ASTRecordLayout& Layout = Context.getASTRecordLayout(RD); return Layout.getSize().getQuantity(); @@ -422,7 +478,7 @@ bool IsTypedefed(TCppScope_t handle) { bool IsAbstract(TCppType_t klass) { auto* D = (clang::Decl*)klass; if (auto* CXXRD = llvm::dyn_cast_or_null(D)) { - LOCK(getInterpInfo()); + LOCK(getInterpInfo(CXXRD)); return CXXRD->isAbstract(); } @@ -445,7 +501,6 @@ bool IsEnumType(TCppType_t type) { } static bool isSmartPointer(const RecordType* RT) { - LOCK(getInterpInfo()); auto IsUseCountPresent = [](const RecordDecl* Record) { ASTContext& C = Record->getASTContext(); return !Record->lookup(&C.Idents.get("use_count")).empty(); @@ -459,6 +514,7 @@ static bool isSmartPointer(const RecordType* RT) { }; const RecordDecl* Record = RT->getDecl(); + LOCK(getInterpInfo(Record)); if (IsUseCountPresent(Record)) return true; @@ -525,8 +581,8 @@ TCppType_t GetIntegerTypeFromEnumType(TCppType_t enum_type) { std::vector GetEnumConstants(TCppScope_t handle) { auto* D = (clang::Decl*)handle; - LOCK(getInterpInfo()); if (auto* ED = llvm::dyn_cast_or_null(D)) { + LOCK(getInterpInfo(ED)); std::vector enum_constants; for (auto* ECD : ED->enumerators()) { enum_constants.push_back((TCppScope_t)ECD); @@ -559,13 +615,12 @@ TCppIndex_t GetEnumConstantValue(TCppScope_t handle) { } size_t GetSizeOfType(TCppType_t type) { - LOCK(getInterpInfo()); QualType QT = QualType::getFromOpaquePtr(type); if (const TagType* TT = QT->getAs()) return SizeOf(TT->getDecl()); // FIXME: Can we get the size of a non-tag type? - auto TI = getSema().getASTContext().getTypeInfo(QT); + auto TI = getASTContext(type).getTypeInfo(QT); size_t TypeSize = TI.Width; return TypeSize / 8; } @@ -590,8 +645,8 @@ std::string GetName(TCppType_t klass) { } std::string GetCompleteName(TCppType_t klass) { - auto& C = getSema().getASTContext(); auto* D = (Decl*)klass; + auto& C = getSema(D).getASTContext(); PrintingPolicy Policy = C.getPrintingPolicy(); Policy.SuppressUnwrittenScope = true; @@ -641,8 +696,8 @@ std::string GetQualifiedName(TCppType_t klass) { // FIXME: Figure out how to merge with GetCompleteName. std::string GetQualifiedCompleteName(TCppType_t klass) { - auto& C = getSema().getASTContext(); auto* D = (Decl*)klass; + auto& C = getSema(D).getASTContext(); if (auto* ND = llvm::dyn_cast_or_null(D)) { if (auto* TD = llvm::dyn_cast(ND)) { @@ -668,10 +723,10 @@ std::string GetQualifiedCompleteName(TCppType_t klass) { } std::vector GetUsingNamespaces(TCppScope_t scope) { - LOCK(getInterpInfo()); auto* D = (clang::Decl*)scope; if (auto* DC = llvm::dyn_cast_or_null(D)) { + LOCK(getInterpInfo(D)); std::vector namespaces; for (auto UD : DC->using_directives()) { namespaces.push_back((TCppScope_t)UD->getNominatedNamespace()); @@ -758,15 +813,15 @@ TCppScope_t GetScopeFromCompleteName(const std::string& name) { TCppScope_t GetNamed(const std::string& name, TCppScope_t parent /*= nullptr*/) { - LOCK(getInterpInfo()); clang::DeclContext* Within = 0; + auto* D = static_cast(parent); + LOCK(getInterpInfo(D)); if (parent) { - auto* D = (clang::Decl*)parent; D = GetUnderlyingScope(D); Within = llvm::dyn_cast(D); } - auto* ND = Cpp_utils::Lookup::Named(&getSema(), name, Within); + auto* ND = Cpp_utils::Lookup::Named(&getSema(D), name, Within); if (ND && ND != (clang::NamedDecl*)-1) { return (TCppScope_t)(ND->getCanonicalDecl()); } @@ -794,13 +849,15 @@ TCppScope_t GetParentScope(TCppScope_t scope) { } TCppIndex_t GetNumBases(TCppScope_t klass) { - LOCK(getInterpInfo()); auto* D = (Decl*)klass; - if (auto* CTSD = llvm::dyn_cast_or_null(D)) + if (auto* CTSD = llvm::dyn_cast_or_null(D)) { + LOCK(getInterpInfo(CTSD)); if (!CTSD->hasDefinition()) - compat::InstantiateClassTemplateSpecialization(getInterp(), CTSD); + compat::InstantiateClassTemplateSpecialization(getInterp(CTSD), CTSD); + } if (auto* CXXRD = llvm::dyn_cast_or_null(D)) { + LOCK(getInterpInfo(CXXRD)); if (CXXRD->hasDefinition()) return CXXRD->getNumBases(); } @@ -809,11 +866,14 @@ TCppIndex_t GetNumBases(TCppScope_t klass) { } TCppScope_t GetBaseClass(TCppScope_t klass, TCppIndex_t ibase) { - LOCK(getInterpInfo()); auto* D = (Decl*)klass; auto* CXXRD = llvm::dyn_cast_or_null(D); - if (!CXXRD || CXXRD->getNumBases() <= ibase) - return 0; + if (!CXXRD) + return nullptr; + + LOCK(getInterpInfo(CXXRD)); + if (CXXRD->getNumBases() <= ibase) + return nullptr; auto type = (CXXRD->bases_begin() + ibase)->getType(); if (auto RT = type->getAs()) @@ -849,7 +909,6 @@ bool IsSubclass(TCppScope_t derived, TCppScope_t base) { static unsigned ComputeBaseOffset(const ASTContext& Context, const CXXRecordDecl* DerivedRD, const CXXBasePath& Path) { - LOCK(getInterpInfo()); CharUnits NonVirtualOffset = CharUnits::Zero(); unsigned NonVirtualStart = 0; @@ -904,14 +963,14 @@ int64_t GetBaseClassOffset(TCppScope_t derived, TCppScope_t base) { CXXRecordDecl* DCXXRD = cast(DD); CXXRecordDecl* BCXXRD = cast(BD); - LOCK(getInterpInfo()); + LOCK(getInterpInfo(DD)); CXXBasePaths Paths(/*FindAmbiguities=*/false, /*RecordPaths=*/true, /*DetectVirtual=*/false); DCXXRD->isDerivedFrom(BCXXRD, Paths); // FIXME: We might want to cache these requests as they seem expensive. - return ComputeBaseOffset(getSema().getASTContext(), DCXXRD, Paths.front()); + return ComputeBaseOffset(getSema(DD).getASTContext(), DCXXRD, Paths.front()); } template @@ -920,8 +979,8 @@ static void GetClassDecls(TCppScope_t klass, if (!klass) return; - LOCK(getInterpInfo()); auto* D = (clang::Decl*)klass; + LOCK(getInterpInfo(D)); if (auto* TD = dyn_cast(D)) D = GetScopeFromType(TD->getUnderlyingType()); @@ -931,11 +990,11 @@ static void GetClassDecls(TCppScope_t klass, auto* CXXRD = dyn_cast(D); #ifdef CPPINTEROP_USE_CLING - cling::Interpreter::PushTransactionRAII RAII(&getInterp()); + cling::Interpreter::PushTransactionRAII RAII(&getInterp(CXXRD)); #endif // CPPINTEROP_USE_CLING if (CXXRD->hasDefinition()) CXXRD = CXXRD->getDefinition(); - getSema().ForceDeclarationOfImplicitMembers(CXXRD); + getSema(CXXRD).ForceDeclarationOfImplicitMembers(CXXRD); for (Decl* DI : CXXRD->decls()) { if (auto* MD = dyn_cast(DI)) methods.push_back(MD); @@ -961,7 +1020,7 @@ static void GetClassDecls(TCppScope_t klass, // Result is appended to the decls, i.e. CXXRD, iterator // non-shadowed decl will be push_back later // methods.push_back(Result); - getSema().findInheritingConstructor(SourceLocation(), CXXCD, CUSD); + getSema(CXXRD).findInheritingConstructor(SourceLocation(), CXXCD, CUSD); } } } @@ -978,9 +1037,10 @@ void GetFunctionTemplatedDecls(TCppScope_t klass, bool HasDefaultConstructor(TCppScope_t scope) { auto* D = (clang::Decl*)scope; - LOCK(getInterpInfo()); - if (auto* CXXRD = llvm::dyn_cast_or_null(D)) + if (auto* CXXRD = llvm::dyn_cast_or_null(D)) { + LOCK(getInterpInfo(CXXRD)); return CXXRD->hasDefaultConstructor(); + } return false; } @@ -990,21 +1050,22 @@ TCppFunction_t GetDefaultConstructor(compat::Interpreter& interp, if (!HasDefaultConstructor(scope)) return nullptr; - LOCK(getInterpInfo()); auto* CXXRD = (clang::CXXRecordDecl*)scope; + LOCK(getInterpInfo(CXXRD)); return interp.getCI()->getSema().LookupDefaultConstructor(CXXRD); } TCppFunction_t GetDefaultConstructor(TCppScope_t scope) { - return GetDefaultConstructor(getInterp(), scope); + auto* CXXRD = static_cast(scope); + return GetDefaultConstructor(getInterp(CXXRD), scope); } TCppFunction_t GetDestructor(TCppScope_t scope) { auto* D = (clang::Decl*)scope; if (auto* CXXRD = llvm::dyn_cast_or_null(D)) { - LOCK(getInterpInfo()); - getSema().ForceDeclarationOfImplicitMembers(CXXRD); + LOCK(getInterpInfo(CXXRD)); + getSema(CXXRD).ForceDeclarationOfImplicitMembers(CXXRD); return CXXRD->getDestructor(); } @@ -1023,14 +1084,14 @@ std::vector GetFunctionsUsingName(TCppScope_t scope, if (!scope || name.empty()) return {}; - LOCK(getInterpInfo()); + LOCK(getInterpInfo(D)); D = GetUnderlyingScope(D); std::vector funcs; llvm::StringRef Name(name); - auto& S = getSema(); - DeclarationName DName = &getASTContext().Idents.get(name); + auto& S = getSema(D); + DeclarationName DName = &S.getASTContext().Idents.get(name); clang::LookupResult R(S, DName, SourceLocation(), Sema::LookupOrdinaryName, For_Visible_Redeclaration); @@ -1049,10 +1110,9 @@ std::vector GetFunctionsUsingName(TCppScope_t scope, } TCppType_t GetFunctionReturnType(TCppFunction_t func) { - LOCK(getInterpInfo()); - auto* D = (clang::Decl*)func; if (auto* FD = llvm::dyn_cast_or_null(D)) { + LOCK(getInterpInfo(FD)); QualType Type = FD->getReturnType(); if (Type->isUndeducedAutoType()) { bool needInstantiation = false; @@ -1071,8 +1131,10 @@ TCppType_t GetFunctionReturnType(TCppFunction_t func) { return Type.getAsOpaquePtr(); } - if (auto* FD = llvm::dyn_cast_or_null(D)) + if (auto* FD = llvm::dyn_cast_or_null(D)) { + LOCK(getInterpInfo(FD)); return (FD->getTemplatedDecl())->getReturnType().getAsOpaquePtr(); + } return 0; } @@ -1100,10 +1162,9 @@ TCppIndex_t GetFunctionRequiredArgs(TCppConstFunction_t func) { } TCppType_t GetFunctionArgType(TCppFunction_t func, TCppIndex_t iarg) { - LOCK(getInterpInfo()); - - auto* D = (clang::Decl*)func; + auto* D = static_cast(func); if (auto* FD = llvm::dyn_cast_or_null(D)) { + LOCK(getInterpInfo(FD)); if (iarg < FD->getNumParams()) { auto* PVD = FD->getParamDecl(iarg); return PVD->getOriginalType().getAsOpaquePtr(); @@ -1129,7 +1190,7 @@ std::string GetFunctionSignature(TCppFunction_t func) { std::string Signature; raw_string_ostream SS(Signature); - PrintingPolicy Policy = getASTContext().getPrintingPolicy(); + PrintingPolicy Policy = getASTContext(D).getPrintingPolicy(); // Skip printing the body Policy.TerseOutput = true; Policy.FullyQualifiedName = true; @@ -1176,14 +1237,14 @@ bool IsTemplatedFunction(TCppFunction_t func) { // the template function exists and >1 means overloads bool ExistsFunctionTemplate(const std::string& name, TCppScope_t parent) { DeclContext* Within = 0; + auto* D = static_cast(parent); if (parent) { - auto* D = (Decl*)parent; Within = llvm::dyn_cast(D); } - LOCK(getInterpInfo()); + LOCK(getInterpInfo(D)); - auto* ND = Cpp_utils::Lookup::Named(&getSema(), name, Within); + auto* ND = Cpp_utils::Lookup::Named(&getSema(D), name, Within); if ((intptr_t)ND == (intptr_t)0) return false; @@ -1202,10 +1263,10 @@ void LookupConstructors(const std::string& name, TCppScope_t parent, auto* D = (Decl*)parent; if (auto* CXXRD = llvm::dyn_cast_or_null(D)) { - LOCK(getInterpInfo()); + LOCK(getInterpInfo(CXXRD)); - getSema().ForceDeclarationOfImplicitMembers(CXXRD); - DeclContextLookupResult Result = getSema().LookupConstructors(CXXRD); + getSema(CXXRD).ForceDeclarationOfImplicitMembers(CXXRD); + DeclContextLookupResult Result = getSema(CXXRD).LookupConstructors(CXXRD); // Obtaining all constructors when we intend to lookup a method under a // scope can lead to crashes. We avoid that by accumulating constructors // only if the Decl matches the lookup name. @@ -1221,14 +1282,14 @@ bool GetClassTemplatedMethods(const std::string& name, TCppScope_t parent, if (!D && name.empty()) return false; - LOCK(getInterpInfo()); + LOCK(getInterpInfo(D)); // Accumulate constructors LookupConstructors(name, parent, funcs); - auto& S = getSema(); + auto& S = getSema(D); D = GetUnderlyingScope(D); llvm::StringRef Name(name); - DeclarationName DName = &getASTContext().Idents.get(name); + DeclarationName DName = &S.getASTContext().Idents.get(name); clang::LookupResult R(S, DName, SourceLocation(), Sema::LookupOrdinaryName, For_Visible_Redeclaration); auto* DC = clang::Decl::castToDeclContext(D); @@ -1264,13 +1325,16 @@ TCppFunction_t BestOverloadFunctionMatch(const std::vector& candidates, const std::vector& explicit_types, const std::vector& arg_types) { - LOCK(getInterpInfo()); + if (candidates.empty()) + return nullptr; + InterpreterInfo& II = getInterpInfo(static_cast(candidates[0])); + LOCK(II); - auto& S = getSema(); + auto& S = II.Interpreter->getSema(); auto& C = S.getASTContext(); #ifdef CPPINTEROP_USE_CLING - cling::Interpreter::PushTransactionRAII RAII(&getInterp()); + cling::Interpreter::PushTransactionRAII RAII(II.Interpreter); #endif // The overload resolution interfaces in Sema require a list of expressions. @@ -1387,10 +1451,9 @@ bool IsDestructor(TCppConstFunction_t method) { } bool IsStaticMethod(TCppConstFunction_t method) { - LOCK(getInterpInfo()); - const auto* D = static_cast(method); if (auto* CXXMD = llvm::dyn_cast_or_null(D)) { + LOCK(getInterpInfo(D)); return CXXMD->isStatic(); } @@ -1410,7 +1473,7 @@ TCppFuncAddr_t GetFunctionAddress(const char* mangled_name) { static TCppFuncAddr_t GetFunctionAddress(const FunctionDecl* FD) { const auto get_mangled_name = [](const FunctionDecl* FD) { - auto MangleCtxt = getASTContext().createMangleContext(); + auto* MangleCtxt = getASTContext(FD).createMangleContext(); if (!MangleCtxt->shouldMangleDeclName(FD)) { return FD->getNameInfo().getName().getAsString(); @@ -1437,6 +1500,7 @@ static TCppFuncAddr_t GetFunctionAddress(const FunctionDecl* FD) { TCppFuncAddr_t GetFunctionAddress(TCppFunction_t method) { auto* D = static_cast(method); if (auto* FD = llvm::dyn_cast_or_null(D)) { + LOCK(getInterpInfo(FD)); if ((IsTemplateInstantiationOrSpecialization(FD) || FD->getTemplatedKind() == FunctionDecl::TK_MemberSpecialization) && !FD->getDefinition()) @@ -1450,9 +1514,9 @@ TCppFuncAddr_t GetFunctionAddress(TCppFunction_t method) { } bool IsVirtualMethod(TCppFunction_t method) { - LOCK(getInterpInfo()); auto* D = (Decl*)method; if (auto* CXXMD = llvm::dyn_cast_or_null(D)) { + LOCK(getInterpInfo(CXXMD)); return CXXMD->isVirtual(); } @@ -1463,9 +1527,9 @@ void GetDatamembers(TCppScope_t scope, std::vector& datamembers) { auto* D = (Decl*)scope; if (auto* CXXRD = llvm::dyn_cast_or_null(D)) { - LOCK(getInterpInfo()); + LOCK(getInterpInfo(CXXRD)); - getSema().ForceDeclarationOfImplicitMembers(CXXRD); + getSema(CXXRD).ForceDeclarationOfImplicitMembers(CXXRD); if (CXXRD->hasDefinition()) CXXRD = CXXRD->getDefinition(); @@ -1510,10 +1574,13 @@ void GetStaticDatamembers(TCppScope_t scope, void GetEnumConstantDatamembers(TCppScope_t scope, std::vector& datamembers, bool include_enum_class) { - LOCK(getInterpInfo()); - std::vector EDs; GetClassDecls(scope, EDs); + if (EDs.empty()) + return; + + LOCK(getInterpInfo(static_cast(EDs[0]))); + for (TCppScope_t i : EDs) { auto* ED = static_cast(i); @@ -1528,13 +1595,13 @@ void GetEnumConstantDatamembers(TCppScope_t scope, TCppScope_t LookupDatamember(const std::string& name, TCppScope_t parent) { clang::DeclContext* Within = 0; + auto* D = static_cast(parent); if (parent) { - auto* D = (clang::Decl*)parent; Within = llvm::dyn_cast(D); } - LOCK(getInterpInfo()); - auto* ND = Cpp_utils::Lookup::Named(&getSema(), name, Within); + LOCK(getInterpInfo(D)); + auto* ND = Cpp_utils::Lookup::Named(&getSema(D), name, Within); if (ND && ND != (clang::NamedDecl*)-1) { if (llvm::isa_and_nonnull(ND)) { return (TCppScope_t)ND; @@ -1576,11 +1643,6 @@ TCppType_t GetVariableType(TCppScope_t var) { intptr_t GetVariableOffset(compat::Interpreter& I, Decl* D, CXXRecordDecl* BaseCXXRD) { - if (!D) - return 0; - - LOCK(getInterpInfo()); - auto& C = I.getSema().getASTContext(); if (auto* FD = llvm::dyn_cast(D)) { @@ -1655,9 +1717,9 @@ intptr_t GetVariableOffset(compat::Interpreter& I, Decl* D, if (!address) { if (!VD->hasInit()) { #ifdef CPPINTEROP_USE_CLING - cling::Interpreter::PushTransactionRAII RAII(&getInterp()); + cling::Interpreter::PushTransactionRAII RAII(&getInterp(VD)); #endif // CPPINTEROP_USE_CLING - getSema().InstantiateVariableDefinition(SourceLocation(), VD); + getSema(VD).InstantiateVariableDefinition(SourceLocation(), VD); VD = VD->getDefinition(); } if (VD->hasInit() && @@ -1688,8 +1750,11 @@ intptr_t GetVariableOffset(compat::Interpreter& I, Decl* D, intptr_t GetVariableOffset(TCppScope_t var, TCppScope_t parent) { auto* D = static_cast(var); + if (!D) + return 0; + LOCK(getInterpInfo(D)); auto* RD = llvm::dyn_cast_or_null(static_cast(parent)); - return GetVariableOffset(getInterp(), D, RD); + return GetVariableOffset(getInterp(D), D, RD); } // Check if the Access Specifier of the variable matches the provided value. @@ -1736,7 +1801,7 @@ bool IsPODType(TCppType_t type) { if (QT.isNull()) return false; - return QT.isPODType(getASTContext()); + return QT.isPODType(getASTContext(type)); } bool IsPointerType(TCppType_t type) { @@ -1768,14 +1833,16 @@ bool IsRValueReferenceType(TCppType_t type) { TCppType_t GetPointerType(TCppType_t type) { QualType QT = QualType::getFromOpaquePtr(type); - return getASTContext().getPointerType(QT).getAsOpaquePtr(); + return getASTContext() + .getPointerType(QT) + .getAsOpaquePtr(); // FIXME: which ASTContext? } TCppType_t GetReferencedType(TCppType_t type, bool rvalue) { QualType QT = QualType::getFromOpaquePtr(type); if (rvalue) - return getASTContext().getRValueReferenceType(QT).getAsOpaquePtr(); - return getASTContext().getLValueReferenceType(QT).getAsOpaquePtr(); + return getASTContext(type).getRValueReferenceType(QT).getAsOpaquePtr(); + return getASTContext(type).getLValueReferenceType(QT).getAsOpaquePtr(); } TCppType_t GetNonReferenceType(TCppType_t type) { @@ -1958,8 +2025,8 @@ TCppType_t GetType(const std::string& name) { TCppType_t GetComplexType(TCppType_t type) { QualType QT = QualType::getFromOpaquePtr(type); - LOCK(getInterpInfo()); - return getASTContext().getComplexType(QT).getAsOpaquePtr(); + LOCK(getInterpInfo(type)); + return getASTContext(type).getComplexType(QT).getAsOpaquePtr(); } TCppType_t GetTypeFromScope(TCppScope_t klass) { @@ -1967,7 +2034,7 @@ TCppType_t GetTypeFromScope(TCppScope_t klass) { return 0; auto* D = (Decl*)klass; - ASTContext& C = getASTContext(); + ASTContext& C = getASTContext(D); if (ValueDecl* VD = dyn_cast(D)) return VD->getType().getAsOpaquePtr(); @@ -1998,7 +2065,6 @@ void* compile_wrapper(compat::Interpreter& I, const std::string& wrapper_name, const std::string& wrapper, bool withAccessControl = true) { LLVM_DEBUG(dbgs() << "Compiling '" << wrapper_name << "'\n"); - LOCK(getInterpInfo()); return I.compileFunction(wrapper_name, wrapper, false /*ifUnique*/, withAccessControl); } @@ -2007,10 +2073,8 @@ void get_type_as_string(QualType QT, std::string& type_name, ASTContext& C, PrintingPolicy Policy) { // TODO: Implement cling desugaring from utils::AST // cling::utils::Transform::GetPartiallyDesugaredType() - if (!QT->isTypedefNameType() || QT->isBuiltinType()) { - LOCK(getInterpInfo()); + if (!QT->isTypedefNameType() || QT->isBuiltinType()) QT = QT.getDesugaredType(C); - } Policy.SuppressElaboration = true; Policy.SuppressTagKeyword = !QT->isEnumeralType(); Policy.FullyQualifiedName = true; @@ -2359,14 +2423,12 @@ void make_narg_call(const FunctionDecl* FD, const std::string& return_type, // a simple way of determining whether a viable copy constructor exists, // so check for the most common case: the trivial one, but not uniquely // available, while there is a move constructor. - { - LOCK(getInterpInfo()); - // include utility header if not already included for std::move - DeclarationName DMove = &getASTContext().Idents.get("move"); - auto result = getSema().getStdNamespace()->lookup(DMove); - if (result.empty()) - Cpp::Declare("#include "); - } + + // include utility header if not already included for std::move + DeclarationName DMove = &getASTContext(FD).Idents.get("move"); + auto result = getSema(FD).getStdNamespace()->lookup(DMove); + if (result.empty()) + Cpp::Declare("#include "); // move construction as needed for classes (note that this is implicit) callbuf << "std::move(*(" << type_name.c_str() << "*)args[" << i << "])"; @@ -2577,7 +2639,7 @@ void make_narg_call_with_return(compat::Interpreter& I, const FunctionDecl* FD, int get_wrapper_code(compat::Interpreter& I, const FunctionDecl* FD, std::string& wrapper_name, std::string& wrapper) { assert(FD && "generate_wrapper called without a function decl!"); - LOCK(getInterpInfo()); + LOCK(getInterpInfo(FD)); ASTContext& Context = FD->getASTContext(); // @@ -2999,7 +3061,7 @@ JitCall::GenericCall make_wrapper(compat::Interpreter& I, const FunctionDecl* FD) { static std::map gWrapperStore; - LOCK(getInterpInfo()); + LOCK(getInterpInfo(FD)); auto R = gWrapperStore.find(FD); if (R != gWrapperStore.end()) @@ -3093,7 +3155,7 @@ static JitCall::DestructorCall make_dtor_wrapper(compat::Interpreter& interp, static map gDtorWrapperStore; - LOCK(getInterpInfo()); + LOCK(getInterpInfo(D)); auto I = gDtorWrapperStore.find(D); if (I != gDtorWrapperStore.end()) @@ -3243,7 +3305,8 @@ CPPINTEROP_API JitCall MakeFunctionCallable(TInterp_t I, } CPPINTEROP_API JitCall MakeFunctionCallable(TCppConstFunction_t func) { - return MakeFunctionCallable(&getInterp(), func); + const auto* D = static_cast(func); + return MakeFunctionCallable(&getInterp(D), func); } namespace { @@ -3274,7 +3337,7 @@ static std::string MakeResourcesPath() { TInterp_t CreateInterpreter(const std::vector& Args /*={}*/, const std::vector& GpuArgs /*={}*/) { - std::unique_lock Lock(InterpreterStackLock); + std::lock_guard Lock(InterpreterStackLock); std::string MainExecutableName = sys::fs::getMainExecutable(nullptr, nullptr); std::string ResourceDir = MakeResourcesPath(); std::vector ClingArgv = {"-resource-dir", ResourceDir.c_str(), @@ -3350,39 +3413,52 @@ TInterp_t CreateInterpreter(const std::vector& Args /*={}*/, } // namespace __internal_CppInterOp )"); - sInterpreters->emplace_back(I, /*Owned=*/true); + sInterpreters->emplace_back( + std::make_shared(I, /*Owned=*/true)); + sInterpreterASTMap->insert( + {&sInterpreters->back()->Interpreter->getSema().getASTContext(), + sInterpreters->back()}); return I; } bool DeleteInterpreter(TInterp_t I /*=nullptr*/) { - std::unique_lock Lock(InterpreterStackLock); + std::lock_guard Lock(InterpreterStackLock); if (!I) { + auto foundAST = + std::find_if(sInterpreterASTMap->begin(), sInterpreterASTMap->end(), + [](const auto& Item) { + return Item.second.lock() == sInterpreters->back(); + }); + sInterpreterASTMap->erase(foundAST); sInterpreters->pop_back(); return true; } auto found = std::find_if(sInterpreters->begin(), sInterpreters->end(), - [&I](const auto& Info) { return Info.Interpreter == I; }); + [&I](const auto& Info) { return Info->Interpreter == I; }); if (found == sInterpreters->end()) return false; // failure - LOCK(*found); + auto foundAST = std::find_if( + sInterpreterASTMap->begin(), sInterpreterASTMap->end(), + [&found](const auto& Item) { return Item.second.lock() == *found; }); + sInterpreterASTMap->erase(foundAST); sInterpreters->erase(found); return true; } bool ActivateInterpreter(TInterp_t I) { - std::unique_lock Lock(InterpreterStackLock); + std::lock_guard Lock(InterpreterStackLock); if (!I) return false; auto found = std::find_if(sInterpreters->begin(), sInterpreters->end(), - [&I](const auto& Info) { return Info.Interpreter == I; }); + [&I](const auto& Info) { return Info->Interpreter == I; }); if (found == sInterpreters->end()) return false; @@ -3393,17 +3469,18 @@ bool ActivateInterpreter(TInterp_t I) { } TInterp_t GetInterpreter() { - std::unique_lock Lock(InterpreterStackLock); + std::lock_guard Lock(InterpreterStackLock); if (sInterpreters->empty()) return nullptr; - return sInterpreters->back().Interpreter; + return sInterpreters->back()->Interpreter; } void UseExternalInterpreter(TInterp_t I) { - std::unique_lock Lock(InterpreterStackLock); + std::lock_guard Lock(InterpreterStackLock); assert(sInterpreters->empty() && "sInterpreter already in use!"); - sInterpreters->emplace_back(static_cast(I), - /*isOwned=*/false); + sInterpreters->emplace_back( + std::make_shared(static_cast(I), + /*isOwned=*/false)); } void AddSearchPath(const char* dir, bool isUser, bool prepend) { @@ -3652,7 +3729,8 @@ bool InsertOrReplaceJitSymbol(const char* linker_mangled_name, } std::string ObjToString(const char* type, void* obj) { - LOCK(getInterpInfo()); + LOCK(getInterpInfo()); // FIXME: not enough information to lock the corrent + // interpreter return getInterp().toString(type, obj); } @@ -3710,7 +3788,6 @@ Decl* InstantiateTemplate(TemplateDecl* TemplateD, Decl* InstantiateTemplate(TemplateDecl* TemplateD, ArrayRef TemplateArgs, Sema& S, bool instantiate_body) { - LOCK(getInterpInfo()); // Create a list of template arguments. TemplateArgumentListInfo TLI{}; for (auto TA : TemplateArgs) @@ -3755,15 +3832,16 @@ TCppScope_t InstantiateTemplate(TCppScope_t tmpl, const TemplateArgInfo* template_args, size_t template_args_size, bool instantiate_body) { - LOCK(getInterpInfo()); - return InstantiateTemplate(getInterp(), tmpl, template_args, + auto* D = static_cast(tmpl); + LOCK(getInterpInfo(D)); + return InstantiateTemplate(getInterp(D), tmpl, template_args, template_args_size, instantiate_body); } void GetClassTemplateInstantiationArgs(TCppScope_t templ_instance, std::vector& args) { - LOCK(getInterpInfo()); auto* CTSD = static_cast(templ_instance); + LOCK(getInterpInfo(CTSD)); for (const auto& TA : CTSD->getTemplateInstantiationArgs().asArray()) { switch (TA.getKind()) { default: @@ -3809,7 +3887,7 @@ void GetAllCppNames(TCppScope_t scope, std::set& names) { clang::DeclContext* DC; clang::DeclContext::decl_iterator decl; - LOCK(getInterpInfo()); + LOCK(getInterpInfo(D)); if (auto* TD = dyn_cast_or_null(D)) { DC = clang::TagDecl::castToDeclContext(TD); decl = DC->decls_begin(); @@ -3839,7 +3917,7 @@ void GetEnums(TCppScope_t scope, std::vector& Result) { auto* DC = llvm::dyn_cast(D); - LOCK(getInterpInfo()); + LOCK(getInterpInfo(D)); llvm::SmallVector DCs; DC->collectAllContexts(DCs); @@ -3856,7 +3934,6 @@ void GetEnums(TCppScope_t scope, std::vector& Result) { // FIXME: On the CPyCppyy side the receiver is of type // vector instead of vector std::vector GetDimensions(TCppType_t type) { - LOCK(getInterpInfo()); QualType Qual = QualType::getFromOpaquePtr(type); if (Qual.isNull()) return {}; @@ -3883,24 +3960,25 @@ std::vector GetDimensions(TCppType_t type) { } bool IsTypeDerivedFrom(TCppType_t derived, TCppType_t base) { - LOCK(getInterpInfo()); - auto& S = getSema(); - auto fakeLoc = GetValidSLoc(S); auto derivedType = clang::QualType::getFromOpaquePtr(derived); auto baseType = clang::QualType::getFromOpaquePtr(base); + auto* CXXRD = baseType->getAsRecordDecl(); + LOCK(getInterpInfo(CXXRD)); + auto& S = getSema(CXXRD); + auto fakeLoc = GetValidSLoc(S); #ifdef CPPINTEROP_USE_CLING - cling::Interpreter::PushTransactionRAII RAII(&getInterp()); + cling::Interpreter::PushTransactionRAII RAII(&getInterp(CXXRD)); #endif return S.IsDerivedFrom(fakeLoc, derivedType, baseType); } std::string GetFunctionArgDefault(TCppFunction_t func, TCppIndex_t param_index) { - LOCK(getInterpInfo()); auto* D = (clang::Decl*)func; clang::ParmVarDecl* PI = nullptr; + LOCK(getInterpInfo(D)); if (auto* FD = llvm::dyn_cast_or_null(D)) PI = FD->getParamDecl(param_index); @@ -3995,8 +4073,8 @@ OperatorArity GetOperatorArity(TCppFunction_t op) { void GetOperator(TCppScope_t scope, Operator op, std::vector& operators, OperatorArity kind) { - LOCK(getInterpInfo()); Decl* D = static_cast(scope); + LOCK(getInterpInfo(D)); if (auto* CXXRD = llvm::dyn_cast_or_null(D)) { auto fn = [&operators, kind, op](const RecordDecl* RD) { ASTContext& C = RD->getASTContext(); @@ -4012,7 +4090,7 @@ void GetOperator(TCppScope_t scope, Operator op, fn(CXXRD); CXXRD->forallBases(fn); } else if (auto* DC = llvm::dyn_cast_or_null(D)) { - ASTContext& C = getSema().getASTContext(); + ASTContext& C = getSema(D).getASTContext(); DeclContextLookupResult Result = DC->lookup(C.DeclarationNames.getCXXOperatorName( (clang::OverloadedOperatorKind)op)); @@ -4061,7 +4139,8 @@ TCppObject_t Construct(compat::Interpreter& interp, TCppScope_t scope, TCppObject_t Construct(TCppScope_t scope, void* arena /*=nullptr*/, TCppIndex_t count /*=1UL*/) { - return Construct(getInterp(), scope, arena, count); + auto* D = static_cast(scope); + return Construct(getInterp(D), scope, arena, count); } bool Destruct(compat::Interpreter& interp, TCppObject_t This, const Decl* Class, @@ -4077,7 +4156,7 @@ bool Destruct(compat::Interpreter& interp, TCppObject_t This, const Decl* Class, bool Destruct(TCppObject_t This, TCppConstScope_t scope, bool withFree /*=true*/, TCppIndex_t count /*=0UL*/) { const auto* Class = static_cast(scope); - return Destruct(getInterp(), This, Class, withFree, count); + return Destruct(getInterp(Class), This, Class, withFree, count); } class StreamCaptureInfo { @@ -4181,7 +4260,8 @@ std::string EndStdStreamCapture() { void CodeComplete(std::vector& Results, const char* code, unsigned complete_line /* = 1U */, unsigned complete_column /* = 1U */) { - LOCK(getInterpInfo()); + LOCK(getInterpInfo()); // FIXME: Not enough info to lock the corrent + // interpreter compat::codeComplete(Results, getInterp(), code, complete_line, complete_column); } diff --git a/unittests/CppInterOp/InterpreterTest.cpp b/unittests/CppInterOp/InterpreterTest.cpp index e9b82ea1d..1083403a1 100644 --- a/unittests/CppInterOp/InterpreterTest.cpp +++ b/unittests/CppInterOp/InterpreterTest.cpp @@ -361,3 +361,26 @@ if (llvm::sys::RunningOnValgrind()) delete ExtInterp; #endif } + +TEST(InterpreterTest, MultipleInterpreter) { +#if CLANG_VERSION_MAJOR < 20 && defined(EMSCRIPTEN) + GTEST_SKIP() << "Test fails for Emscipten LLVM 20 builds"; +#endif + + EXPECT_TRUE(Cpp::CreateInterpreter()); + Cpp::Declare(R"( + void f() {} + )"); + Cpp::TCppScope_t f = Cpp::GetNamed("f"); + + EXPECT_TRUE(Cpp::CreateInterpreter()); + Cpp::Declare(R"( + void ff() {} + )"); + Cpp::TCppScope_t ff = Cpp::GetNamed("ff"); + + auto f_callable = Cpp::MakeFunctionCallable(f); + EXPECT_EQ(f_callable.getKind(), Cpp::JitCall::Kind::kGenericCall); + auto ff_callable = Cpp::MakeFunctionCallable(ff); + EXPECT_EQ(ff_callable.getKind(), Cpp::JitCall::Kind::kGenericCall); +} From 97bd9b87ed4175fa873b56bc81914a2e0e8b1316 Mon Sep 17 00:00:00 2001 From: Vipul Cariappa Date: Fri, 22 Aug 2025 13:26:49 +0200 Subject: [PATCH 3/5] add dev docs --- docs/DevelopersDocumentation.rst | 46 ++++++++++++++++++++++++++++++-- 1 file changed, 44 insertions(+), 2 deletions(-) diff --git a/docs/DevelopersDocumentation.rst b/docs/DevelopersDocumentation.rst index 0be22db90..e586cc5d8 100644 --- a/docs/DevelopersDocumentation.rst +++ b/docs/DevelopersDocumentation.rst @@ -423,12 +423,54 @@ library files and run pytest: python -m pip install pytest python -m pytest -sv -*********************************** +################################### CppInterOp Internal Documentation -*********************************** +################################### CppInterOp maintains an internal Doxygen documentation of its components. Internal documentation aims to capture intrinsic details and overall usage of code components. The goal of internal documentation is to make the codebase easier to understand for the new developers. Internal documentation can be visited : `here `_ + +************************************** + Multiple Interpreter & Thread-Safety +************************************** + +CppInterOp allows the user to create multiple interpreters at a time and +use those interpreters. The interpreters that are created are stored in a +stack and a map. The stack is used to enable the model where the user +wants to create a temporary interpreter and destroy it after performing a +few operations. In such a use case, the top of the stack is the only +interpreter in use at any given point in time. + +The map is used to store the mapping from :code:`clang::ASTContext` to +:code:`Cpp::InterpreterInfo`. This is required to figure out which +interpreter an object belongs to. Say the library user performs the +following operations: + +1. Create an Interpreter +2. Compile some code with variable :code:`a` +3. Create another Interpreter +4. Performs :code:`Cpp::GetVariableOffset(a)` + +In step 4, the top of the stack is an interpreter without the definition of +:code:`a`. And we cannot use it to figure out the address of :code:`a`. +The :code:`clang::Decl` passed to :code:`Cpp::GetVariableOffset` is used to +retrieve the :code:`clang::ASTContext`, using +:code:`clang::Decl::getASTContext`. We then use the map to figure out the +exact Interpreter Instance this :code:`clang::Decl` belongs to and perform +the operation. + +A shortcoming of this is that if the CppInterOp accepts a +:code:`clang::QualType` instead of :code:`clang::Decl`, then it is not +possible to get the :code:`clang::ASTContext` from the :code:`clang::QualType`. +In such cases, we iterate over the Allocator of all the Interpreters in our +stack and figure out which :code:`clang::ASTContext` allocated this +:code:`clang::QualType`. This is a very expensive operation. But there is no +alternative to this. + +For **thread-safety**, we introduce a lock for each of the interpreters we +create. And lock only that one specific interpreter when required. We also +have 2 global locks, one for LLVM, and another is used to lock operations +performed on the interpreter stack and the map itself. From cfb00a4a1cea453a5b38812cc2af9e7255e11b36 Mon Sep 17 00:00:00 2001 From: Vipul Cariappa Date: Mon, 25 Aug 2025 11:14:25 +0200 Subject: [PATCH 4/5] add `Cpp::TakeInterpreter` pops interpreter with destroying it destruction to be handled by the caller --- include/CppInterOp/CppInterOp.h | 5 ++ lib/CppInterOp/CppInterOp.cpp | 77 ++++++++++++++++++------ unittests/CppInterOp/InterpreterTest.cpp | 12 ++++ 3 files changed, 74 insertions(+), 20 deletions(-) diff --git a/include/CppInterOp/CppInterOp.h b/include/CppInterOp/CppInterOp.h index bcbe3e5e1..6f19185cc 100644 --- a/include/CppInterOp/CppInterOp.h +++ b/include/CppInterOp/CppInterOp.h @@ -702,6 +702,11 @@ CreateInterpreter(const std::vector& Args = {}, ///\returns false on failure or if \c I is not tracked in the stack. CPPINTEROP_API bool DeleteInterpreter(TInterp_t I = nullptr); +/// Take ownership of an interpreter instance. +///\param[in] I - the interpreter to be taken, if nullptr, returns the last. +///\returns nullptr on failure or if \c I is not tracked in the stack. +CPPINTEROP_API TInterp_t TakeInterpreter(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. diff --git a/lib/CppInterOp/CppInterOp.cpp b/lib/CppInterOp/CppInterOp.cpp index 8338f0cc6..b9992d534 100755 --- a/lib/CppInterOp/CppInterOp.cpp +++ b/lib/CppInterOp/CppInterOp.cpp @@ -54,6 +54,7 @@ #include "llvm/Support/CommandLine.h" #include "llvm/Support/Debug.h" #include "llvm/Support/Error.h" +#include "llvm/Support/ErrorHandling.h" #include "llvm/Support/ManagedStatic.h" #include "llvm/Support/Path.h" #include "llvm/Support/raw_ostream.h" @@ -140,10 +141,10 @@ struct InterpreterInfo { // NOLINTBEGIN // std::deque avoids relocations and calling the dtor of InterpreterInfo. -static llvm::ManagedStatic>> +static llvm::ManagedStatic>> sInterpreters; static llvm::ManagedStatic< - std::unordered_map>> + std::unordered_map> sInterpreterASTMap; static std::recursive_mutex InterpreterStackLock; static std::recursive_mutex LLVMLock; @@ -161,7 +162,7 @@ static InterpreterInfo& getInterpInfo(const clang::Decl* D) { return getInterpInfo(); if (sInterpreters->size() == 1) return *sInterpreters->back(); - return *(*sInterpreterASTMap)[&D->getASTContext()].lock(); + return *(*sInterpreterASTMap)[&D->getASTContext()]; } static InterpreterInfo& getInterpInfo(const void* D) { std::lock_guard Lock(InterpreterStackLock); @@ -171,7 +172,7 @@ static InterpreterInfo& getInterpInfo(const void* D) { return *sInterpreters->back(); for (auto& item : *sInterpreterASTMap) { if (item.first->getAllocator().identifyObject(D)) - return *item.second.lock(); + return *item.second; } llvm_unreachable( "This pointer does not belong to any interpreter instance.\n"); @@ -189,7 +190,7 @@ static compat::Interpreter& getInterp(const clang::Decl* D) { return getInterp(); if (sInterpreters->size() == 1) return *sInterpreters->back()->Interpreter; - return *(*sInterpreterASTMap)[&D->getASTContext()].lock()->Interpreter; + return *(*sInterpreterASTMap)[&D->getASTContext()]->Interpreter; } static compat::Interpreter& getInterp(const void* D) { return *getInterpInfo(D).Interpreter; @@ -3338,6 +3339,8 @@ static std::string MakeResourcesPath() { TInterp_t CreateInterpreter(const std::vector& Args /*={}*/, const std::vector& GpuArgs /*={}*/) { std::lock_guard Lock(InterpreterStackLock); + assert(sInterpreters->size() == sInterpreterASTMap->size()); + std::string MainExecutableName = sys::fs::getMainExecutable(nullptr, nullptr); std::string ResourceDir = MakeResourcesPath(); std::vector ClingArgv = {"-resource-dir", ResourceDir.c_str(), @@ -3414,42 +3417,73 @@ TInterp_t CreateInterpreter(const std::vector& Args /*={}*/, )"); sInterpreters->emplace_back( - std::make_shared(I, /*Owned=*/true)); + std::make_unique(I, /*Owned=*/true)); sInterpreterASTMap->insert( {&sInterpreters->back()->Interpreter->getSema().getASTContext(), - sInterpreters->back()}); + sInterpreters->back().get()}); + assert(sInterpreters->size() == sInterpreterASTMap->size()); return I; } +static inline auto find_interpreter_in_stack(TInterp_t I) { + return std::find_if( + sInterpreters->begin(), sInterpreters->end(), + [&I](const auto& Info) { return Info->Interpreter == I; }); +} + +static inline auto find_interpreter_in_map(InterpreterInfo* I) { + return std::find_if(sInterpreterASTMap->begin(), sInterpreterASTMap->end(), + [](const auto& Item) { + return Item.second == sInterpreters->back().get(); + }); +} + bool DeleteInterpreter(TInterp_t I /*=nullptr*/) { std::lock_guard Lock(InterpreterStackLock); + assert(sInterpreters->size() == sInterpreterASTMap->size()); if (!I) { - auto foundAST = - std::find_if(sInterpreterASTMap->begin(), sInterpreterASTMap->end(), - [](const auto& Item) { - return Item.second.lock() == sInterpreters->back(); - }); + auto foundAST = find_interpreter_in_map(sInterpreters->back().get()); sInterpreterASTMap->erase(foundAST); sInterpreters->pop_back(); return true; } - auto found = - std::find_if(sInterpreters->begin(), sInterpreters->end(), - [&I](const auto& Info) { return Info->Interpreter == I; }); + auto found = find_interpreter_in_stack(I); if (found == sInterpreters->end()) return false; // failure - auto foundAST = std::find_if( - sInterpreterASTMap->begin(), sInterpreterASTMap->end(), - [&found](const auto& Item) { return Item.second.lock() == *found; }); + auto foundAST = find_interpreter_in_map((*found).get()); sInterpreterASTMap->erase(foundAST); sInterpreters->erase(found); return true; } +TInterp_t TakeInterpreter(TInterp_t I /*=nullptr*/) { + std::lock_guard Lock(InterpreterStackLock); + assert(sInterpreters->size() == sInterpreterASTMap->size()); + + if (!I) { + auto foundAST = find_interpreter_in_map(sInterpreters->back().get()); + sInterpreterASTMap->erase(foundAST); + InterpreterInfo* res = sInterpreters->back().release(); + sInterpreters->pop_back(); + return res->Interpreter; + } + + auto found = find_interpreter_in_stack(I); + if (found == sInterpreters->end()) + return nullptr; // failure + + auto foundAST = find_interpreter_in_map((*found).get()); + sInterpreterASTMap->erase(foundAST); + InterpreterInfo* res = (*found).release(); + sInterpreters->erase(found); + assert(sInterpreters->size() == sInterpreterASTMap->size()); + return res->Interpreter; +} + bool ActivateInterpreter(TInterp_t I) { std::lock_guard Lock(InterpreterStackLock); @@ -3477,10 +3511,13 @@ TInterp_t GetInterpreter() { void UseExternalInterpreter(TInterp_t I) { std::lock_guard Lock(InterpreterStackLock); - assert(sInterpreters->empty() && "sInterpreter already in use!"); sInterpreters->emplace_back( - std::make_shared(static_cast(I), + std::make_unique(static_cast(I), /*isOwned=*/false)); + sInterpreterASTMap->insert( + {&sInterpreters->back()->Interpreter->getSema().getASTContext(), + sInterpreters->back().get()}); + assert(sInterpreters->size() == sInterpreterASTMap->size()); } void AddSearchPath(const char* dir, bool isUser, bool prepend) { diff --git a/unittests/CppInterOp/InterpreterTest.cpp b/unittests/CppInterOp/InterpreterTest.cpp index 1083403a1..53916a30c 100644 --- a/unittests/CppInterOp/InterpreterTest.cpp +++ b/unittests/CppInterOp/InterpreterTest.cpp @@ -88,9 +88,19 @@ TEST(InterpreterTest, DeleteInterpreter) { EXPECT_EQ(I3, Cpp::GetInterpreter()) << "I3 is not active"; + auto* D1 = Cpp::TakeInterpreter(); + EXPECT_EQ(D1, I3); + + Cpp::UseExternalInterpreter(D1); + EXPECT_TRUE(Cpp::DeleteInterpreter(nullptr)); EXPECT_EQ(I2, Cpp::GetInterpreter()); + auto* D2 = Cpp::TakeInterpreter(I2); + EXPECT_EQ(I2, D2); + + Cpp::UseExternalInterpreter(D2); + auto* I4 = reinterpret_cast(static_cast(~0U)); EXPECT_FALSE(Cpp::DeleteInterpreter(I4)); @@ -150,6 +160,8 @@ TEST(InterpreterTest, Process) { EXPECT_EQ(Res, CXError_Success); clang_Value_dispose(CXV); clang_Interpreter_dispose(CXI); + auto* OldI = Cpp::TakeInterpreter(); + EXPECT_EQ(OldI, I); } TEST(InterpreterTest, EmscriptenExceptionHandling) { From 8748388973a830b195355b9c845cc89c078bb1cc Mon Sep 17 00:00:00 2001 From: Vipul Cariappa Date: Wed, 27 Aug 2025 14:49:53 +0200 Subject: [PATCH 5/5] squash this commit with the previous commit fix ExternalInterpreterTest `Cpp::UseExternalInterpreter` expects `Cpp::Interpreter` (clang-repl) or `cling::Interpreter` (cling). Account for this and update the test to reflect the same. --- lib/CppInterOp/CppInterOp.cpp | 2 ++ lib/CppInterOp/CppInterOpInterpreter.h | 3 +-- unittests/CppInterOp/InterpreterTest.cpp | 23 ++++++----------------- 3 files changed, 9 insertions(+), 19 deletions(-) diff --git a/lib/CppInterOp/CppInterOp.cpp b/lib/CppInterOp/CppInterOp.cpp index b9992d534..781f8c9b5 100755 --- a/lib/CppInterOp/CppInterOp.cpp +++ b/lib/CppInterOp/CppInterOp.cpp @@ -3445,6 +3445,7 @@ bool DeleteInterpreter(TInterp_t I /*=nullptr*/) { if (!I) { auto foundAST = find_interpreter_in_map(sInterpreters->back().get()); + assert(foundAST != sInterpreterASTMap->end()); sInterpreterASTMap->erase(foundAST); sInterpreters->pop_back(); return true; @@ -3455,6 +3456,7 @@ bool DeleteInterpreter(TInterp_t I /*=nullptr*/) { return false; // failure auto foundAST = find_interpreter_in_map((*found).get()); + assert(foundAST != sInterpreterASTMap->end()); sInterpreterASTMap->erase(foundAST); sInterpreters->erase(found); return true; diff --git a/lib/CppInterOp/CppInterOpInterpreter.h b/lib/CppInterOp/CppInterOpInterpreter.h index d87eb33e1..4833c0bd5 100644 --- a/lib/CppInterOp/CppInterOpInterpreter.h +++ b/lib/CppInterOp/CppInterOpInterpreter.h @@ -140,12 +140,11 @@ namespace Cpp { /// CppInterOp Interpreter /// class Interpreter { -private: std::unique_ptr inner; +public: Interpreter(std::unique_ptr CI) : inner(std::move(CI)) {} -public: static std::unique_ptr create(int argc, const char* const* argv, const char* llvmdir = nullptr, const std::vector>& diff --git a/unittests/CppInterOp/InterpreterTest.cpp b/unittests/CppInterOp/InterpreterTest.cpp index 53916a30c..2d3dce78f 100644 --- a/unittests/CppInterOp/InterpreterTest.cpp +++ b/unittests/CppInterOp/InterpreterTest.cpp @@ -88,19 +88,9 @@ TEST(InterpreterTest, DeleteInterpreter) { EXPECT_EQ(I3, Cpp::GetInterpreter()) << "I3 is not active"; - auto* D1 = Cpp::TakeInterpreter(); - EXPECT_EQ(D1, I3); - - Cpp::UseExternalInterpreter(D1); - EXPECT_TRUE(Cpp::DeleteInterpreter(nullptr)); EXPECT_EQ(I2, Cpp::GetInterpreter()); - auto* D2 = Cpp::TakeInterpreter(I2); - EXPECT_EQ(I2, D2); - - Cpp::UseExternalInterpreter(D2); - auto* I4 = reinterpret_cast(static_cast(~0U)); EXPECT_FALSE(Cpp::DeleteInterpreter(I4)); @@ -341,7 +331,9 @@ if (llvm::sys::RunningOnValgrind()) // Create the interpreter instance. std::unique_ptr I = ExitOnErr(clang::Interpreter::create(std::move(CI))); - auto ExtInterp = I.get(); + + auto CPPI = Cpp::Interpreter(std::move(I)); + auto ExtInterp = &CPPI; #endif // CPPINTEROP_USE_REPL #ifdef CPPINTEROP_USE_CLING @@ -358,12 +350,9 @@ if (llvm::sys::RunningOnValgrind()) EXPECT_NE(ExtInterp, nullptr); -#if !defined(NDEBUG) && GTEST_HAS_DEATH_TEST -#ifndef _WIN32 // Windows seems to fail to die... - EXPECT_DEATH(Cpp::UseExternalInterpreter(ExtInterp), "sInterpreter already in use!"); -#endif // _WIN32 -#endif - EXPECT_TRUE(Cpp::GetInterpreter()) << "External Interpreter not set"; + Cpp::UseExternalInterpreter(ExtInterp); + EXPECT_EQ(ExtInterp, Cpp::GetInterpreter()); + EXPECT_EQ(ExtInterp, Cpp::TakeInterpreter(ExtInterp)); #ifndef CPPINTEROP_USE_CLING I.release();