Skip to content

Commit

Permalink
[ORC][MachO] Fix deferred action handling during MachOPlatform bootst…
Browse files Browse the repository at this point in the history
…rap.

DeferredAAs should only capture bootstrap actions, but after 30b73ed it was
capturing all actions, including those from other plugins. This is problematic
as other plugins may introduce actions that need to run before the platform
actions (e.g. on arm64e we need pointer signing to run before we access any
global pointers in the graph).

Note that this effectively undoes 30b73ed, which was a buggy attempt to
synchronize writes to the DeferredAAs vector. This patch fixes that issue the
obvious way by locking the bootstrap mutex while accessing the DeferredAAs
vector.

No testcase yet: So far I've only seen this fail during bootstrap of arm64e
JIT'd programs.
  • Loading branch information
lhames committed Jan 10, 2025
1 parent 99d2ff5 commit e8cc4d2
Show file tree
Hide file tree
Showing 2 changed files with 37 additions and 43 deletions.
4 changes: 1 addition & 3 deletions llvm/include/llvm/ExecutionEngine/Orc/MachOPlatform.h
Original file line number Diff line number Diff line change
Expand Up @@ -240,7 +240,6 @@ class MachOPlatform : public Platform {
};
using JITSymTabVector = SmallVector<SymbolTablePair>;

Error bootstrapPipelineStart(jitlink::LinkGraph &G);
Error bootstrapPipelineRecordRuntimeFunctions(jitlink::LinkGraph &G);
Error bootstrapPipelineEnd(jitlink::LinkGraph &G);

Expand Down Expand Up @@ -368,11 +367,10 @@ class MachOPlatform : public Platform {
DenseMap<JITDylib *, SymbolLookupSet> RegisteredInitSymbols;

std::mutex PlatformMutex;
BootstrapInfo *Bootstrap = nullptr;
DenseMap<JITDylib *, ExecutorAddr> JITDylibToHeaderAddr;
DenseMap<ExecutorAddr, JITDylib *> HeaderAddrToJITDylib;
DenseMap<JITDylib *, uint64_t> JITDylibToPThreadKey;

std::atomic<BootstrapInfo *> Bootstrap;
};

// Generates a MachO header.
Expand Down
76 changes: 36 additions & 40 deletions llvm/lib/ExecutionEngine/Orc/MachOPlatform.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -799,17 +799,21 @@ void MachOPlatform::MachOPlatformPlugin::modifyPassConfig(

using namespace jitlink;

bool InBootstrapPhase =
&MR.getTargetJITDylib() == &MP.PlatformJD && MP.Bootstrap;
bool InBootstrapPhase = false;

if (LLVM_UNLIKELY(&MR.getTargetJITDylib() == &MP.PlatformJD)) {
std::lock_guard<std::mutex> Lock(MP.PlatformMutex);
if (MP.Bootstrap) {
InBootstrapPhase = true;
++MP.Bootstrap->ActiveGraphs;
}
}

// If we're in the bootstrap phase then increment the active graphs.
if (InBootstrapPhase) {
Config.PrePrunePasses.push_back(
[this](LinkGraph &G) { return bootstrapPipelineStart(G); });
if (LLVM_UNLIKELY(InBootstrapPhase))
Config.PostAllocationPasses.push_back([this](LinkGraph &G) {
return bootstrapPipelineRecordRuntimeFunctions(G);
});
}

// --- Handle Initializers ---
if (auto InitSymbol = MR.getInitializerSymbol()) {
Expand Down Expand Up @@ -872,19 +876,11 @@ void MachOPlatform::MachOPlatformPlugin::modifyPassConfig(
[this](LinkGraph &G) { return bootstrapPipelineEnd(G); });
}

Error MachOPlatform::MachOPlatformPlugin::bootstrapPipelineStart(
jitlink::LinkGraph &G) {
// Increment the active graphs count in BootstrapInfo.
std::lock_guard<std::mutex> Lock(MP.Bootstrap.load()->Mutex);
++MP.Bootstrap.load()->ActiveGraphs;
return Error::success();
}

Error MachOPlatform::MachOPlatformPlugin::
bootstrapPipelineRecordRuntimeFunctions(jitlink::LinkGraph &G) {
// Record bootstrap function names.
std::pair<StringRef, ExecutorAddr *> RuntimeSymbols[] = {
{*MP.MachOHeaderStartSymbol, &MP.Bootstrap.load()->MachOHeaderAddr},
{*MP.MachOHeaderStartSymbol, &MP.Bootstrap->MachOHeaderAddr},
{*MP.PlatformBootstrap.Name, &MP.PlatformBootstrap.Addr},
{*MP.PlatformShutdown.Name, &MP.PlatformShutdown.Addr},
{*MP.RegisterJITDylib.Name, &MP.RegisterJITDylib.Addr},
Expand Down Expand Up @@ -924,30 +920,22 @@ Error MachOPlatform::MachOPlatformPlugin::
// If this graph defines the macho header symbol then create the internal
// mapping between it and PlatformJD.
std::lock_guard<std::mutex> Lock(MP.PlatformMutex);
MP.JITDylibToHeaderAddr[&MP.PlatformJD] =
MP.Bootstrap.load()->MachOHeaderAddr;
MP.HeaderAddrToJITDylib[MP.Bootstrap.load()->MachOHeaderAddr] =
&MP.PlatformJD;
MP.JITDylibToHeaderAddr[&MP.PlatformJD] = MP.Bootstrap->MachOHeaderAddr;
MP.HeaderAddrToJITDylib[MP.Bootstrap->MachOHeaderAddr] = &MP.PlatformJD;
}

return Error::success();
}

Error MachOPlatform::MachOPlatformPlugin::bootstrapPipelineEnd(
jitlink::LinkGraph &G) {
std::lock_guard<std::mutex> Lock(MP.Bootstrap.load()->Mutex);
assert(MP.Bootstrap && "DeferredAAs reset before bootstrap completed");

// Transfer any allocation actions to DeferredAAs.
std::move(G.allocActions().begin(), G.allocActions().end(),
std::back_inserter(MP.Bootstrap.load()->DeferredAAs));
G.allocActions().clear();
std::lock_guard<std::mutex> Lock(MP.Bootstrap->Mutex);

--MP.Bootstrap.load()->ActiveGraphs;
--MP.Bootstrap->ActiveGraphs;
// Notify Bootstrap->CV while holding the mutex because the mutex is
// also keeping Bootstrap->CV alive.
if (MP.Bootstrap.load()->ActiveGraphs == 0)
MP.Bootstrap.load()->CV.notify_all();
if (MP.Bootstrap->ActiveGraphs == 0)
MP.Bootstrap->CV.notify_all();
return Error::success();
}

Expand Down Expand Up @@ -1412,15 +1400,23 @@ Error MachOPlatform::MachOPlatformPlugin::registerObjectPlatformSections(
assert(I->second && "Null header registered for JD");
HeaderAddr = I->second;
}
G.allocActions().push_back(
{cantFail(
WrapperFunctionCall::Create<SPSRegisterObjectPlatformSectionsArgs>(
MP.RegisterObjectPlatformSections.Addr, HeaderAddr, UnwindInfo,
MachOPlatformSecs)),
cantFail(
WrapperFunctionCall::Create<SPSRegisterObjectPlatformSectionsArgs>(
MP.DeregisterObjectPlatformSections.Addr, HeaderAddr,
UnwindInfo, MachOPlatformSecs))});

AllocActionCallPair AllocActions = {
cantFail(
WrapperFunctionCall::Create<SPSRegisterObjectPlatformSectionsArgs>(
MP.RegisterObjectPlatformSections.Addr, HeaderAddr, UnwindInfo,
MachOPlatformSecs)),
cantFail(
WrapperFunctionCall::Create<SPSRegisterObjectPlatformSectionsArgs>(
MP.DeregisterObjectPlatformSections.Addr, HeaderAddr,
UnwindInfo, MachOPlatformSecs))};

if (LLVM_LIKELY(!InBootstrapPhase))
G.allocActions().push_back(std::move(AllocActions));
else {
std::lock_guard<std::mutex> Lock(MP.Bootstrap->Mutex);
MP.Bootstrap->DeferredAAs.push_back(std::move(AllocActions));
}
}

return Error::success();
Expand Down Expand Up @@ -1701,8 +1697,8 @@ Error MachOPlatform::MachOPlatformPlugin::addSymbolTableRegistration(
// If we're in the bootstrap phase then just record these symbols in the
// bootstrap object and then bail out -- registration will be attached to
// the bootstrap graph.
std::lock_guard<std::mutex> Lock(MP.Bootstrap.load()->Mutex);
auto &SymTab = MP.Bootstrap.load()->SymTab;
std::lock_guard<std::mutex> Lock(MP.Bootstrap->Mutex);
auto &SymTab = MP.Bootstrap->SymTab;
for (auto &[OriginalSymbol, NameSym] : JITSymTabInfo)
SymTab.push_back({NameSym->getAddress(), OriginalSymbol->getAddress(),
flagsForSymbol(*OriginalSymbol)});
Expand Down

0 comments on commit e8cc4d2

Please sign in to comment.