Fix Thread-Safety #721
Fix Thread-Safety #721
Conversation
having static locals can be risky and we might want to move them into the InterpreterInfo class
Vipul-Cariappa
requested review from
vgvassilev,
aaronj0,
anutosh491 and
mcbarton
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| if (IsSomeThreadWriting == 0) | ||
| return; | ||
| IsSomeThreadWritingMutex.unlock(); | ||
| std::this_thread::sleep_for(0ms); // give control to other threads |
There was a problem hiding this comment.
warning: no header providing "std::operator""ms" is directly included [misc-include-cleaner]
lib/CppInterOp/CppInterOp.cpp:41:
- #if CLANG_VERSION_MAJOR >= 19
+ #include <bits/chrono.h>
+ #if CLANG_VERSION_MAJOR >= 19| if (IsSomeThreadWriting == 0) | ||
| return; | ||
| IsSomeThreadWritingMutex.unlock(); | ||
| std::this_thread::sleep_for(0ms); // give control to other threads |
There was a problem hiding this comment.
warning: no header providing "std::this_thread::sleep_for" is directly included [misc-include-cleaner]
lib/CppInterOp/CppInterOp.cpp:41:
- #if CLANG_VERSION_MAJOR >= 19
+ #include <thread>
+ #if CLANG_VERSION_MAJOR >= 19| // return getSema(D).getASTContext(); | ||
| // } | ||
|
|
||
| int Declare(compat::Interpreter& I, const char* code, bool silent); |
There was a problem hiding this comment.
warning: function 'Declare' can be made static or moved into an anonymous namespace to enforce internal linkage [misc-use-internal-linkage]
| int Declare(compat::Interpreter& I, const char* code, bool silent); | |
| static int Declare(compat::Interpreter& I, const char* code, bool silent); |
There was a problem hiding this comment.
Cannot do this, Ref:
CppInterOp/lib/CppInterOp/CXCppInterOp.cpp
Line 327 in 0aa859b
lib/CppInterOp/CppInterOp.cpp
Outdated
| } | ||
|
|
||
| static void InstantiateFunctionDefinition(Decl* D) { | ||
| static void InstantiateFunctionDefinition(Decl* D, Interpreter& I) { |
There was a problem hiding this comment.
warning: no header providing "Cpp::Interpreter" is directly included [misc-include-cleaner]
lib/CppInterOp/CppInterOp.cpp:12:
+ #include "CppInterOpInterpreter.h"| auto* Within = llvm::dyn_cast<clang::DeclContext>(D); | ||
|
|
||
| auto* ND = Cpp_utils::Lookup::Named(&getSema(D), name, Within); | ||
| auto* ND = Cpp_utils::Lookup::Named(&I.getSema(), name, Within); |
There was a problem hiding this comment.
warning: no header providing "Cpp::utils::Lookup::Named" is directly included [misc-include-cleaner]
auto* ND = Cpp_utils::Lookup::Named(&I.getSema(), name, Within);
^
lib/CppInterOp/CppInterOp.cpp
Outdated
| } | ||
|
|
||
| TCppIndex_t GetNumBases(TCppScope_t klass) { | ||
| auto* D = (Decl*)klass; |
There was a problem hiding this comment.
warning: do not use C-style cast to convert between unrelated types [cppcoreguidelines-pro-type-cstyle-cast]
auto* D = (Decl*)klass;
^
lib/CppInterOp/CppInterOp.cpp
Outdated
|
|
||
| TCppType_t GetTypeFromScope(TCppScope_t klass) { | ||
| if (!klass) | ||
| return 0; |
There was a problem hiding this comment.
warning: use nullptr [modernize-use-nullptr]
| return 0; | |
| return nullptr; |
lib/CppInterOp/CppInterOp.cpp
Outdated
| if (!klass) | ||
| return 0; | ||
|
|
||
| auto* D = (Decl*)klass; |
There was a problem hiding this comment.
warning: do not use C-style cast to convert between unrelated types [cppcoreguidelines-pro-type-cstyle-cast]
auto* D = (Decl*)klass;
^
lib/CppInterOp/CppInterOp.cpp
Outdated
| // encompassed in an anonymous namespace as follows. | ||
| namespace { | ||
| static unsigned long long gWrapperSerial = 0LL; | ||
| static std::atomic<unsigned long long> gWrapperSerial = 0LL; |
There was a problem hiding this comment.
warning: 'gWrapperSerial' is a static definition in anonymous namespace; static is redundant here [readability-static-definition-in-anonymous-namespace]
| static std::atomic<unsigned long long> gWrapperSerial = 0LL; | |
| std::atomic<unsigned long long> gWrapperSerial = 0LL; |
lib/CppInterOp/CppInterOp.cpp
Outdated
| // encompassed in an anonymous namespace as follows. | ||
| namespace { | ||
| static unsigned long long gWrapperSerial = 0LL; | ||
| static std::atomic<unsigned long long> gWrapperSerial = 0LL; |
There was a problem hiding this comment.
warning: no header providing "std::atomic" is directly included [misc-include-cleaner]
lib/CppInterOp/CppInterOp.cpp:41:
- #if CLANG_VERSION_MAJOR >= 19
+ #include <atomic>
+ #if CLANG_VERSION_MAJOR >= 19
lib/CppInterOp/CppInterOp.cpp
Outdated
| // encompassed in an anonymous namespace as follows. | ||
| namespace { | ||
| static unsigned long long gWrapperSerial = 0LL; | ||
| static std::atomic<unsigned long long> gWrapperSerial = 0LL; |
There was a problem hiding this comment.
warning: variable 'gWrapperSerial' is non-const and globally accessible, consider making it const [cppcoreguidelines-avoid-non-const-global-variables]
static std::atomic<unsigned long long> gWrapperSerial = 0LL;
^
lib/CppInterOp/CppInterOp.cpp
Outdated
| std::lock_guard<std::mutex> Lock(gWrapperStoreMutex); | ||
| auto R = gWrapperStore.find(FD); | ||
| if (R != gWrapperStore.end()) | ||
| return (JitCall::GenericCall)R->second; |
There was a problem hiding this comment.
warning: do not use C-style cast to convert between unrelated types [cppcoreguidelines-pro-type-cstyle-cast]
return (JitCall::GenericCall)R->second;
^
lib/CppInterOp/CppInterOp.cpp
Outdated
| std::lock_guard<std::mutex> Lock(gDtorWrapperStoreMutex); | ||
| auto I = gDtorWrapperStore.find(D); | ||
| if (I != gDtorWrapperStore.end()) | ||
| return (JitCall::DestructorCall)I->second; |
There was a problem hiding this comment.
warning: do not use C-style cast to convert between unrelated types [cppcoreguidelines-pro-type-cstyle-cast]
return (JitCall::DestructorCall)I->second;
^
lib/CppInterOp/CppInterOp.cpp
Outdated
| if (!Cpp::Declare(instance.c_str(), /*silent=*/false, interp)) { | ||
| auto* VD = static_cast<VarDecl*>(Cpp::GetNamed(id, nullptr, interp)); | ||
| if (!Cpp::Declare(*IF, instance.c_str(), /*silent=*/false)) { | ||
| auto* VD = static_cast<VarDecl*>(Cpp::GetNamed(*IF, id, nullptr)); |
There was a problem hiding this comment.
warning: do not use static_cast to downcast from a base to a derived class; use dynamic_cast instead [cppcoreguidelines-pro-type-static-cast-downcast]
| auto* VD = static_cast<VarDecl*>(Cpp::GetNamed(*IF, id, nullptr)); | |
| auto* VD = dynamic_cast<VarDecl*>(Cpp::GetNamed(*IF, id, nullptr)); |
| llvm::InitializeAllTargets(); | ||
| llvm::InitializeAllTargetMCs(); | ||
| llvm::InitializeAllAsmPrinters(); | ||
| static std::once_flag call_once_flag; |
There was a problem hiding this comment.
warning: no header providing "std::once_flag" is directly included [misc-include-cleaner]
lib/CppInterOp/CppInterOpInterpreter.h:25:
- #if CLANG_VERSION_MAJOR >= 19
+ #include <mutex>
+ #if CLANG_VERSION_MAJOR >= 19| llvm::InitializeAllTargetMCs(); | ||
| llvm::InitializeAllAsmPrinters(); | ||
| static std::once_flag call_once_flag; | ||
| std::call_once(call_once_flag, []() { |
There was a problem hiding this comment.
warning: no header providing "std::call_once" is directly included [misc-include-cleaner]
std::call_once(call_once_flag, []() {
^| const clang::CompilerInstance* getCI() const { return getCompilerInstance(); } | ||
|
|
||
| clang::Sema& getSema() const { return getCI()->getSema(); } | ||
| clang::ASTContext& getASTContext() const { return getSema().getASTContext(); } |
There was a problem hiding this comment.
warning: function 'getASTContext' should be marked [[nodiscard]] [modernize-use-nodiscard]
| clang::ASTContext& getASTContext() const { return getSema().getASTContext(); } | |
| [[nodiscard]] clang::ASTContext& getASTContext() const { return getSema().getASTContext(); } |
| #include "gtest/gtest.h" | ||
|
|
||
| #include <chrono> | ||
| #include <thread> |
There was a problem hiding this comment.
warning: included header chrono is not used directly [misc-include-cleaner]
| #include <thread> | |
| #include <thread> |
|
|
||
| #include <chrono> | ||
| #include <thread> | ||
|
|
There was a problem hiding this comment.
warning: included header thread is not used directly [misc-include-cleaner]
| using namespace clang; | ||
|
|
||
| TEST(ScopeReflectionTest, IsEnumScope) { | ||
| void ScopeReflectionTest_IsEnumScope() { |
There was a problem hiding this comment.
warning: function 'ScopeReflectionTest_IsEnumScope' can be made static or moved into an anonymous namespace to enforce internal linkage [misc-use-internal-linkage]
| void ScopeReflectionTest_IsEnumScope() { | |
| static void ScopeReflectionTest_IsEnumScope() { |
| #include <atomic> | ||
| #include <cassert> | ||
| #include <chrono> | ||
| #include <cstddef> |
There was a problem hiding this comment.
warning: included header chrono is not used directly [misc-include-cleaner]
| #include <cstddef> | |
| #include <cstddef> |
| if (IsSomeThreadWriting == 0) | ||
| return; | ||
| IsSomeThreadWritingMutex.unlock(); | ||
| std::this_thread::sleep_for(0ms); // give control to other threads |
There was a problem hiding this comment.
warning: no header providing "std::operator""ms" is directly included [misc-include-cleaner]
lib/CppInterOp/CppInterOp.cpp:42:
- #if CLANG_VERSION_MAJOR >= 19
+ #include <bits/chrono.h>
+ #if CLANG_VERSION_MAJOR >= 19
lib/CppInterOp/CppInterOp.cpp
Outdated
| // encompassed in an anonymous namespace as follows. | ||
| namespace { | ||
| static unsigned long long gWrapperSerial = 0LL; | ||
| std::atomic<unsigned long long> gWrapperSerial = 0LL; |
There was a problem hiding this comment.
warning: variable 'gWrapperSerial' is non-const and globally accessible, consider making it const [cppcoreguidelines-avoid-non-const-global-variables]
std::atomic<unsigned long long> gWrapperSerial = 0LL;
^| } | ||
|
|
||
| TEST(ScopeReflectionTest, IsEnumConstant) { | ||
| void ScopeReflectionTest_IsEnumConstant() { |
There was a problem hiding this comment.
warning: function 'ScopeReflectionTest_IsEnumConstant' can be made static or moved into an anonymous namespace to enforce internal linkage [misc-use-internal-linkage]
| void ScopeReflectionTest_IsEnumConstant() { | |
| static void ScopeReflectionTest_IsEnumConstant() { |
| } | ||
|
|
||
| TEST(EnumReflectionTest, IsEnumType) { | ||
| void EnumReflectionTest_IsEnumType() { |
There was a problem hiding this comment.
warning: function 'EnumReflectionTest_IsEnumType' can be made static or moved into an anonymous namespace to enforce internal linkage [misc-use-internal-linkage]
| void EnumReflectionTest_IsEnumType() { | |
| static void EnumReflectionTest_IsEnumType() { |
| } | ||
|
|
||
| TEST(EnumReflectionTest, GetIntegerTypeFromEnumScope) { | ||
| void EnumReflectionTest_GetIntegerTypeFromEnumScope() { |
There was a problem hiding this comment.
warning: function 'EnumReflectionTest_GetIntegerTypeFromEnumScope' can be made static or moved into an anonymous namespace to enforce internal linkage [misc-use-internal-linkage]
| void EnumReflectionTest_GetIntegerTypeFromEnumScope() { | |
| static void EnumReflectionTest_GetIntegerTypeFromEnumScope() { |
| } | ||
|
|
||
| TEST(EnumReflectionTest, GetIntegerTypeFromEnumType) { | ||
| void EnumReflectionTest_GetIntegerTypeFromEnumType() { |
There was a problem hiding this comment.
warning: function 'EnumReflectionTest_GetIntegerTypeFromEnumType' can be made static or moved into an anonymous namespace to enforce internal linkage [misc-use-internal-linkage]
| void EnumReflectionTest_GetIntegerTypeFromEnumType() { | |
| static void EnumReflectionTest_GetIntegerTypeFromEnumType() { |
| } | ||
|
|
||
| TEST(EnumReflectionTest, GetEnumConstants) { | ||
| void EnumReflectionTest_GetEnumConstants() { |
There was a problem hiding this comment.
warning: function 'EnumReflectionTest_GetEnumConstants' can be made static or moved into an anonymous namespace to enforce internal linkage [misc-use-internal-linkage]
| void EnumReflectionTest_GetEnumConstants() { | |
| static void EnumReflectionTest_GetEnumConstants() { |
| } | ||
|
|
||
| TEST(EnumReflectionTest, GetEnumConstantType) { | ||
| void EnumReflectionTest_GetEnumConstantType() { |
There was a problem hiding this comment.
warning: function 'EnumReflectionTest_GetEnumConstantType' can be made static or moved into an anonymous namespace to enforce internal linkage [misc-use-internal-linkage]
| void EnumReflectionTest_GetEnumConstantType() { | |
| static void EnumReflectionTest_GetEnumConstantType() { |
| } | ||
|
|
||
| TEST(EnumReflectionTest, GetEnumConstantValue) { | ||
| void EnumReflectionTest_GetEnumConstantValue() { |
There was a problem hiding this comment.
warning: function 'EnumReflectionTest_GetEnumConstantValue' can be made static or moved into an anonymous namespace to enforce internal linkage [misc-use-internal-linkage]
| void EnumReflectionTest_GetEnumConstantValue() { | |
| static void EnumReflectionTest_GetEnumConstantValue() { |
| InterpreterInfo(const InterpreterInfo&) = delete; | ||
| InterpreterInfo& operator=(const InterpreterInfo&) = delete; | ||
|
|
||
| clang::Sema& getSema() const { return Interpreter->getSema(); } |
There was a problem hiding this comment.
warning: function 'getSema' should be marked [[nodiscard]] [modernize-use-nodiscard]
| clang::Sema& getSema() const { return Interpreter->getSema(); } | |
| [[nodiscard]] clang::Sema& getSema() const { return Interpreter->getSema(); } |
| InterpreterInfo& operator=(const InterpreterInfo&) = delete; | ||
|
|
||
| clang::Sema& getSema() const { return Interpreter->getSema(); } | ||
| clang::ASTContext& getASTContext() const { |
There was a problem hiding this comment.
warning: function 'getASTContext' should be marked [[nodiscard]] [modernize-use-nodiscard]
| clang::ASTContext& getASTContext() const { | |
| [[nodiscard]] clang::ASTContext& getASTContext() const { |
| auto* Within = llvm::dyn_cast<clang::DeclContext>(D); | ||
|
|
||
| auto* ND = Cpp_utils::Lookup::Named(&getSema(D), name, Within); | ||
| auto* ND = Cpp_utils::Lookup::Named(&I.getSema(), name, Within); |
There was a problem hiding this comment.
warning: no header providing "Cpp::utils::Lookup::Named" is directly included [misc-include-cleaner]
lib/CppInterOp/CppInterOp.cpp:12:
+ #include "CppInterOpInterpreter.h"| // encompassed in an anonymous namespace as follows. | ||
| namespace { | ||
| static unsigned long long gWrapperSerial = 0LL; | ||
| std::atomic<unsigned long long> gWrapperSerial = |
There was a problem hiding this comment.
warning: variable 'gWrapperSerial' is non-const and globally accessible, consider making it const [cppcoreguidelines-avoid-non-const-global-variables]
std::atomic<unsigned long long> gWrapperSerial =
^| std::lock_guard<std::mutex> Lock(gWrapperStoreMutex); | ||
| auto R = gWrapperStore.find(FD); | ||
| if (R != gWrapperStore.end()) | ||
| return (JitCall::GenericCall) |
There was a problem hiding this comment.
warning: do not use C-style cast to convert between unrelated types [cppcoreguidelines-pro-type-cstyle-cast]
return (JitCall::GenericCall)
^| std::lock_guard<std::mutex> Lock(gDtorWrapperStoreMutex); | ||
| auto I = gDtorWrapperStore.find(D); | ||
| if (I != gDtorWrapperStore.end()) | ||
| return (JitCall::DestructorCall) |
There was a problem hiding this comment.
warning: do not use C-style cast to convert between unrelated types [cppcoreguidelines-pro-type-cstyle-cast]
return (JitCall::DestructorCall)
^| } | ||
|
|
||
| TEST(EnumReflectionTest, EnumReflectionTest) { | ||
| std::vector<std::pair<const char*, void (*)()>> fns = { |
There was a problem hiding this comment.
warning: no header providing "std::pair" is directly included [misc-include-cleaner]
unittests/CppInterOp/EnumReflectionTest.cpp:7:
+ #include <utility>| using namespace clang; | ||
|
|
||
| TEST(FunctionReflectionTest, GetClassMethods) { | ||
| static Cpp::TInterp_t I = nullptr; |
There was a problem hiding this comment.
warning: variable 'I' is non-const and globally accessible, consider making it const [cppcoreguidelines-avoid-non-const-global-variables]
static Cpp::TInterp_t I = nullptr;
^| )"; | ||
|
|
||
| GetAllTopLevelDecls(code, Decls); | ||
| Cpp::TInterp_t I1 = GetAllTopLevelDecls(code, Decls); |
There was a problem hiding this comment.
warning: Value stored to 'I1' during its initialization is never read [clang-analyzer-deadcode.DeadStores]
Cpp::TInterp_t I1 = GetAllTopLevelDecls(code, Decls);
^Additional context
unittests/CppInterOp/FunctionReflectionTest.cpp:53: Value stored to 'I1' during its initialization is never read
Cpp::TInterp_t I1 = GetAllTopLevelDecls(code, Decls);
^| EXPECT_FALSE(Cpp::Process(code.c_str(), I)); | ||
| const char* str = "std::make_unique<int,int>"; | ||
| auto* Instance1 = (Decl*)Cpp::InstantiateTemplateFunctionFromString(str); | ||
| auto* Instance1 = (Decl*)Cpp::InstantiateTemplateFunctionFromString(str, I); |
There was a problem hiding this comment.
warning: do not use C-style cast to convert between unrelated types [cppcoreguidelines-pro-type-cstyle-cast]
auto* Instance1 = (Decl*)Cpp::InstantiateTemplateFunctionFromString(str, I);
^
This PR fixes the problems with the old implementation.
We now globally lock all the interpreters when we iterate over all the interpreters' bump allocators to resolve which interpreter a
QualTypebelongs to. Similarly, we also lock all the interpreters to resolve the interpreter from aDecl.I have also adopted most of the tests to run in parallel, hopefully detecting failures early.