[llvm] [orc::MemoryMapper] use thread-local variant for thread-local operations (PR #154355)
Jameson Nash via llvm-commits
llvm-commits at lists.llvm.org
Tue Aug 19 07:56:06 PDT 2025
https://github.com/vtjnash created https://github.com/llvm/llvm-project/pull/154355
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).
(Written by Claude)
>From 486b05966fb17f7b51a43bf05487195478c690ef Mon Sep 17 00:00:00 2001
From: Jameson Nash <vtjnash at gmail.com>
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 <variant>
+
#if defined(LLVM_ON_UNIX) && !defined(__ANDROID__)
#include <fcntl.h>
#include <sys/mman.h>
@@ -92,13 +94,18 @@ void InProcessMemoryMapper::initialize(MemoryMapper::AllocInfo &AI,
std::vector<shared::WrapperFunctionCall> DeinitializeActions;
{
- std::promise<MSVCPExpected<std::vector<shared::WrapperFunctionCall>>> P;
- auto F = P.get_future();
+ std::variant<std::monostate,
+ Expected<std::vector<shared::WrapperFunctionCall>>>
+ Result;
shared::runFinalizeActions(
AI.Actions, [&](Expected<std::vector<shared::WrapperFunctionCall>> R) {
- P.set_value(std::move(R));
+ Result = std::move(R);
});
- if (auto DeinitializeActionsOrErr = F.get())
+ assert(!std::holds_alternative<std::monostate>(Result) &&
+ "InProcessMemoryMapper should run finalize actions synchronously");
+ if (auto DeinitializeActionsOrErr = std::move(
+ std::get<Expected<std::vector<shared::WrapperFunctionCall>>>(
+ Result)))
DeinitializeActions = std::move(*DeinitializeActionsOrErr);
else
return OnInitialized(DeinitializeActionsOrErr.takeError());
@@ -162,10 +169,11 @@ void InProcessMemoryMapper::release(ArrayRef<ExecutorAddr> Bases,
}
// deinitialize sub allocations
- std::promise<MSVCPError> P;
- auto F = P.get_future();
- deinitialize(AllocAddrs, [&](Error Err) { P.set_value(std::move(Err)); });
- if (Error E = F.get()) {
+ std::variant<std::monostate, Error> Result;
+ deinitialize(AllocAddrs, [&](Error Err) { Result = std::move(Err); });
+ assert(!std::holds_alternative<std::monostate>(Result) &&
+ "InProcessMemoryMapper should run deinitialize synchronously");
+ if (Error E = std::move(std::get<Error>(Result))) {
Err = joinErrors(std::move(Err), std::move(E));
}
@@ -195,10 +203,11 @@ InProcessMemoryMapper::~InProcessMemoryMapper() {
}
}
- std::promise<MSVCPError> P;
- auto F = P.get_future();
- release(ReservationAddrs, [&](Error Err) { P.set_value(std::move(Err)); });
- cantFail(F.get());
+ std::variant<std::monostate, Error> Result;
+ release(ReservationAddrs, [&](Error Err) { Result = std::move(Err); });
+ assert(!std::holds_alternative<std::monostate>(Result) &&
+ "InProcessMemoryMapper should run release synchronously");
+ cantFail(std::move(std::get<Error>(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 <future>
#include <sstream>
+#include <variant>
#if defined(LLVM_ON_UNIX)
#include <errno.h>
@@ -182,16 +182,22 @@ Expected<ExecutorAddr> ExecutorSharedMemoryMapperService::initialize(
Segment.Size);
}
- // Run finalization actions and get deinitlization action list.
+ // Run finalization actions and get deinitialization action list.
std::vector<shared::WrapperFunctionCall> DeinitializeActions;
{
- std::promise<MSVCPExpected<std::vector<shared::WrapperFunctionCall>>> P;
- auto F = P.get_future();
+ std::variant<std::monostate,
+ Expected<std::vector<shared::WrapperFunctionCall>>>
+ Result;
shared::runFinalizeActions(
FR.Actions, [&](Expected<std::vector<shared::WrapperFunctionCall>> R) {
- P.set_value(std::move(R));
+ Result = std::move(R);
});
- if (auto DeinitializeActionsOrErr = F.get())
+ assert(!std::holds_alternative<std::monostate>(Result) &&
+ "ExecutorSharedMemoryMapperService should run finalize actions "
+ "synchronously");
+ if (auto DeinitializeActionsOrErr = std::move(
+ std::get<Expected<std::vector<shared::WrapperFunctionCall>>>(
+ 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 <variant>
+
using namespace llvm;
using namespace llvm::orc;
using namespace llvm::orc::shared;
@@ -18,33 +20,37 @@ using namespace llvm::orc::shared;
namespace {
Expected<ExecutorAddrRange> reserve(MemoryMapper &M, size_t NumBytes) {
- std::promise<MSVCPExpected<ExecutorAddrRange>> P;
- auto F = P.get_future();
- M.reserve(NumBytes, [&](auto R) { P.set_value(std::move(R)); });
- return F.get();
+ std::variant<std::monostate, Expected<ExecutorAddrRange>> Result;
+ M.reserve(NumBytes, [&](auto R) { Result = std::move(R); });
+ assert(!std::holds_alternative<std::monostate>(Result) &&
+ "MemoryMapper operations should complete synchronously in tests");
+ return std::move(std::get<Expected<ExecutorAddrRange>>(Result));
}
Expected<ExecutorAddr> initialize(MemoryMapper &M,
MemoryMapper::AllocInfo &AI) {
- std::promise<MSVCPExpected<ExecutorAddr>> P;
- auto F = P.get_future();
- M.initialize(AI, [&](auto R) { P.set_value(std::move(R)); });
- return F.get();
+ std::variant<std::monostate, Expected<ExecutorAddr>> Result;
+ M.initialize(AI, [&](auto R) { Result = std::move(R); });
+ assert(!std::holds_alternative<std::monostate>(Result) &&
+ "MemoryMapper operations should complete synchronously in tests");
+ return std::move(std::get<Expected<ExecutorAddr>>(Result));
}
Error deinitialize(MemoryMapper &M,
const std::vector<ExecutorAddr> &Allocations) {
- std::promise<MSVCPError> P;
- auto F = P.get_future();
- M.deinitialize(Allocations, [&](auto R) { P.set_value(std::move(R)); });
- return F.get();
+ std::variant<std::monostate, Error> Result;
+ M.deinitialize(Allocations, [&](auto R) { Result = std::move(R); });
+ assert(!std::holds_alternative<std::monostate>(Result) &&
+ "MemoryMapper operations should complete synchronously in tests");
+ return std::move(std::get<Error>(Result));
}
Error release(MemoryMapper &M, const std::vector<ExecutorAddr> &Reservations) {
- std::promise<MSVCPError> P;
- auto F = P.get_future();
- M.release(Reservations, [&](auto R) { P.set_value(std::move(R)); });
- return F.get();
+ std::variant<std::monostate, Error> Result;
+ M.release(Reservations, [&](auto R) { Result = std::move(R); });
+ assert(!std::holds_alternative<std::monostate>(Result) &&
+ "MemoryMapper operations should complete synchronously in tests");
+ return std::move(std::get<Error>(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 <future>
+#include <variant>
using namespace llvm;
using namespace llvm::orc;
@@ -114,29 +114,29 @@ static void voidNoopAsync(unique_function<void(SPSEmpty)> SendResult) {
static WrapperFunctionResult voidNoopAsyncWrapper(const char *ArgData,
size_t ArgSize) {
- std::promise<WrapperFunctionResult> RP;
- auto RF = RP.get_future();
+ std::variant<std::monostate, WrapperFunctionResult> Result;
WrapperFunction<void()>::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<std::monostate>(Result) &&
+ "handleAsync should complete synchronously in tests");
+ return std::move(std::get<WrapperFunctionResult>(Result));
}
static WrapperFunctionResult addAsyncWrapper(const char *ArgData,
size_t ArgSize) {
- std::promise<WrapperFunctionResult> RP;
- auto RF = RP.get_future();
+ std::variant<std::monostate, WrapperFunctionResult> Result;
WrapperFunction<int32_t(int32_t, int32_t)>::handleAsync(
- ArgData, ArgSize,
- [&](WrapperFunctionResult R) { RP.set_value(std::move(R)); },
+ ArgData, ArgSize, [&](WrapperFunctionResult R) { Result = std::move(R); },
[](unique_function<void(int32_t)> SendResult, int32_t X, int32_t Y) {
SendResult(X + Y);
});
- return RF.get();
+ assert(!std::holds_alternative<std::monostate>(Result) &&
+ "handleAsync should complete synchronously in tests");
+ return std::move(std::get<WrapperFunctionResult>(Result));
}
TEST(WrapperFunctionUtilsTest, WrapperFunctionCallAndHandleAsyncVoid) {
More information about the llvm-commits
mailing list