From 486b05966fb17f7b51a43bf05487195478c690ef Mon Sep 17 00:00:00 2001 From: Jameson Nash Date: Tue, 19 Aug 2025 14:50:14 +0000 Subject: [PATCH] [MemoryMapper] use thread-local variant for thread-local operations Clarify this code does not have access to a TaskDispatcher, and thus is not permitted use of threads, by using a more efficient single-threaded monostate variant, instead of making it appear that this code could potentially be a cause of thread-safety concerns (notably deadlock). --- llvm/lib/ExecutionEngine/Orc/MemoryMapper.cpp | 33 ++++++++++------ .../ExecutorSharedMemoryMapperService.cpp | 18 ++++++--- .../ExecutionEngine/Orc/MemoryMapperTest.cpp | 38 +++++++++++-------- .../Orc/WrapperFunctionUtilsTest.cpp | 22 +++++------ 4 files changed, 66 insertions(+), 45 deletions(-) diff --git a/llvm/lib/ExecutionEngine/Orc/MemoryMapper.cpp b/llvm/lib/ExecutionEngine/Orc/MemoryMapper.cpp index ea3b22a092437..b17d8a77b9f60 100644 --- a/llvm/lib/ExecutionEngine/Orc/MemoryMapper.cpp +++ b/llvm/lib/ExecutionEngine/Orc/MemoryMapper.cpp @@ -12,6 +12,8 @@ #include "llvm/ExecutionEngine/Orc/Shared/OrcRTBridge.h" #include "llvm/Support/WindowsError.h" +#include + #if defined(LLVM_ON_UNIX) && !defined(__ANDROID__) #include #include @@ -92,13 +94,18 @@ void InProcessMemoryMapper::initialize(MemoryMapper::AllocInfo &AI, std::vector DeinitializeActions; { - std::promise>> P; - auto F = P.get_future(); + std::variant>> + Result; shared::runFinalizeActions( AI.Actions, [&](Expected> R) { - P.set_value(std::move(R)); + Result = std::move(R); }); - if (auto DeinitializeActionsOrErr = F.get()) + assert(!std::holds_alternative(Result) && + "InProcessMemoryMapper should run finalize actions synchronously"); + if (auto DeinitializeActionsOrErr = std::move( + std::get>>( + Result))) DeinitializeActions = std::move(*DeinitializeActionsOrErr); else return OnInitialized(DeinitializeActionsOrErr.takeError()); @@ -162,10 +169,11 @@ void InProcessMemoryMapper::release(ArrayRef Bases, } // deinitialize sub allocations - std::promise P; - auto F = P.get_future(); - deinitialize(AllocAddrs, [&](Error Err) { P.set_value(std::move(Err)); }); - if (Error E = F.get()) { + std::variant Result; + deinitialize(AllocAddrs, [&](Error Err) { Result = std::move(Err); }); + assert(!std::holds_alternative(Result) && + "InProcessMemoryMapper should run deinitialize synchronously"); + if (Error E = std::move(std::get(Result))) { Err = joinErrors(std::move(Err), std::move(E)); } @@ -195,10 +203,11 @@ InProcessMemoryMapper::~InProcessMemoryMapper() { } } - std::promise P; - auto F = P.get_future(); - release(ReservationAddrs, [&](Error Err) { P.set_value(std::move(Err)); }); - cantFail(F.get()); + std::variant Result; + release(ReservationAddrs, [&](Error Err) { Result = std::move(Err); }); + assert(!std::holds_alternative(Result) && + "InProcessMemoryMapper should run release synchronously"); + cantFail(std::move(std::get(Result))); } // SharedMemoryMapper diff --git a/llvm/lib/ExecutionEngine/Orc/TargetProcess/ExecutorSharedMemoryMapperService.cpp b/llvm/lib/ExecutionEngine/Orc/TargetProcess/ExecutorSharedMemoryMapperService.cpp index 8c24b1f3f5265..6844382033e4a 100644 --- a/llvm/lib/ExecutionEngine/Orc/TargetProcess/ExecutorSharedMemoryMapperService.cpp +++ b/llvm/lib/ExecutionEngine/Orc/TargetProcess/ExecutorSharedMemoryMapperService.cpp @@ -12,8 +12,8 @@ #include "llvm/Support/MSVCErrorWorkarounds.h" #include "llvm/Support/Process.h" #include "llvm/Support/WindowsError.h" -#include #include +#include #if defined(LLVM_ON_UNIX) #include @@ -182,16 +182,22 @@ Expected ExecutorSharedMemoryMapperService::initialize( Segment.Size); } - // Run finalization actions and get deinitlization action list. + // Run finalization actions and get deinitialization action list. std::vector DeinitializeActions; { - std::promise>> P; - auto F = P.get_future(); + std::variant>> + Result; shared::runFinalizeActions( FR.Actions, [&](Expected> R) { - P.set_value(std::move(R)); + Result = std::move(R); }); - if (auto DeinitializeActionsOrErr = F.get()) + assert(!std::holds_alternative(Result) && + "ExecutorSharedMemoryMapperService should run finalize actions " + "synchronously"); + if (auto DeinitializeActionsOrErr = std::move( + std::get>>( + Result))) DeinitializeActions = std::move(*DeinitializeActionsOrErr); else return DeinitializeActionsOrErr.takeError(); diff --git a/llvm/unittests/ExecutionEngine/Orc/MemoryMapperTest.cpp b/llvm/unittests/ExecutionEngine/Orc/MemoryMapperTest.cpp index fea9eabbb465b..7c13370f497b8 100644 --- a/llvm/unittests/ExecutionEngine/Orc/MemoryMapperTest.cpp +++ b/llvm/unittests/ExecutionEngine/Orc/MemoryMapperTest.cpp @@ -11,6 +11,8 @@ #include "llvm/Testing/Support/Error.h" #include "gtest/gtest.h" +#include + using namespace llvm; using namespace llvm::orc; using namespace llvm::orc::shared; @@ -18,33 +20,37 @@ using namespace llvm::orc::shared; namespace { Expected reserve(MemoryMapper &M, size_t NumBytes) { - std::promise> P; - auto F = P.get_future(); - M.reserve(NumBytes, [&](auto R) { P.set_value(std::move(R)); }); - return F.get(); + std::variant> Result; + M.reserve(NumBytes, [&](auto R) { Result = std::move(R); }); + assert(!std::holds_alternative(Result) && + "MemoryMapper operations should complete synchronously in tests"); + return std::move(std::get>(Result)); } Expected initialize(MemoryMapper &M, MemoryMapper::AllocInfo &AI) { - std::promise> P; - auto F = P.get_future(); - M.initialize(AI, [&](auto R) { P.set_value(std::move(R)); }); - return F.get(); + std::variant> Result; + M.initialize(AI, [&](auto R) { Result = std::move(R); }); + assert(!std::holds_alternative(Result) && + "MemoryMapper operations should complete synchronously in tests"); + return std::move(std::get>(Result)); } Error deinitialize(MemoryMapper &M, const std::vector &Allocations) { - std::promise P; - auto F = P.get_future(); - M.deinitialize(Allocations, [&](auto R) { P.set_value(std::move(R)); }); - return F.get(); + std::variant Result; + M.deinitialize(Allocations, [&](auto R) { Result = std::move(R); }); + assert(!std::holds_alternative(Result) && + "MemoryMapper operations should complete synchronously in tests"); + return std::move(std::get(Result)); } Error release(MemoryMapper &M, const std::vector &Reservations) { - std::promise P; - auto F = P.get_future(); - M.release(Reservations, [&](auto R) { P.set_value(std::move(R)); }); - return F.get(); + std::variant Result; + M.release(Reservations, [&](auto R) { Result = std::move(R); }); + assert(!std::holds_alternative(Result) && + "MemoryMapper operations should complete synchronously in tests"); + return std::move(std::get(Result)); } // A basic function to be used as both initializer/deinitializer diff --git a/llvm/unittests/ExecutionEngine/Orc/WrapperFunctionUtilsTest.cpp b/llvm/unittests/ExecutionEngine/Orc/WrapperFunctionUtilsTest.cpp index 8de2412fed4d0..3d56c30146fee 100644 --- a/llvm/unittests/ExecutionEngine/Orc/WrapperFunctionUtilsTest.cpp +++ b/llvm/unittests/ExecutionEngine/Orc/WrapperFunctionUtilsTest.cpp @@ -11,7 +11,7 @@ #include "llvm/Testing/Support/Error.h" #include "gtest/gtest.h" -#include +#include using namespace llvm; using namespace llvm::orc; @@ -114,29 +114,29 @@ static void voidNoopAsync(unique_function SendResult) { static WrapperFunctionResult voidNoopAsyncWrapper(const char *ArgData, size_t ArgSize) { - std::promise RP; - auto RF = RP.get_future(); + std::variant Result; WrapperFunction::handleAsync( - ArgData, ArgSize, - [&](WrapperFunctionResult R) { RP.set_value(std::move(R)); }, + ArgData, ArgSize, [&](WrapperFunctionResult R) { Result = std::move(R); }, voidNoopAsync); - return RF.get(); + assert(!std::holds_alternative(Result) && + "handleAsync should complete synchronously in tests"); + return std::move(std::get(Result)); } static WrapperFunctionResult addAsyncWrapper(const char *ArgData, size_t ArgSize) { - std::promise RP; - auto RF = RP.get_future(); + std::variant Result; WrapperFunction::handleAsync( - ArgData, ArgSize, - [&](WrapperFunctionResult R) { RP.set_value(std::move(R)); }, + ArgData, ArgSize, [&](WrapperFunctionResult R) { Result = std::move(R); }, [](unique_function SendResult, int32_t X, int32_t Y) { SendResult(X + Y); }); - return RF.get(); + assert(!std::holds_alternative(Result) && + "handleAsync should complete synchronously in tests"); + return std::move(std::get(Result)); } TEST(WrapperFunctionUtilsTest, WrapperFunctionCallAndHandleAsyncVoid) {