From 19bfc075a94437e5e551896ce28f1b999fc71f5e Mon Sep 17 00:00:00 2001 From: Xusheng Date: Wed, 20 May 2026 15:02:44 -0400 Subject: [PATCH 1/8] Hold a strong reference to DebuggerController in detached worker threads The 17 `Launch`/`Attach`/`Connect`/`Go`/`Step*`/`RunTo*`/`Restart`/`Detach`/ `Quit`/`Pause` methods each spawn a detached `std::thread` that runs the corresponding `*AndWait` work, capturing `this` by reference. Nothing keeps the controller alive for the duration of the detached call, so if the controller's refcount drops to zero before the worker exits (e.g. the user closes the tab), the worker dereferences a dangling pointer. Capture a `DbgRef` by value into each lambda so the controller stays alive until the worker thread finishes. Fixes #1083. Co-Authored-By: Claude Opus 4.7 (1M context) --- core/debuggercontroller.cpp | 51 ++++++++++++++++++++++++------------- 1 file changed, 34 insertions(+), 17 deletions(-) diff --git a/core/debuggercontroller.cpp b/core/debuggercontroller.cpp index 1dd3be70..d876a4d1 100644 --- a/core/debuggercontroller.cpp +++ b/core/debuggercontroller.cpp @@ -290,7 +290,8 @@ bool DebuggerController::Launch() if (!CanStartDebgging()) return false; - std::thread([&]() { LaunchAndWait(); }).detach(); + DbgRef self = this; + std::thread([self]() { self->LaunchAndWait(); }).detach(); return true; } @@ -353,7 +354,8 @@ bool DebuggerController::Attach() if (!CanStartDebgging()) return false; - std::thread([&]() { AttachAndWait(); }).detach(); + DbgRef self = this; + std::thread([self]() { self->AttachAndWait(); }).detach(); return true; } @@ -404,7 +406,8 @@ bool DebuggerController::Connect() if (!CanStartDebgging()) return false; - std::thread([&]() { ConnectAndWait(); }).detach(); + DbgRef self = this; + std::thread([self]() { self->ConnectAndWait(); }).detach(); return true; } @@ -543,7 +546,8 @@ bool DebuggerController::Go() if (!CanResumeTarget()) return false; - std::thread([&]() { GoAndWait(); }).detach(); + DbgRef self = this; + std::thread([self]() { self->GoAndWait(); }).detach(); return true; } @@ -554,7 +558,8 @@ bool DebuggerController::GoReverse() if (!CanResumeTarget()) return false; - std::thread([&]() { GoReverseAndWait(); }).detach(); + DbgRef self = this; + std::thread([self]() { self->GoReverseAndWait(); }).detach(); return true; } @@ -817,7 +822,8 @@ bool DebuggerController::StepInto(BNFunctionGraphType il) if (!CanResumeTarget()) return false; - std::thread([&, il]() { StepIntoAndWait(il); }).detach(); + DbgRef self = this; + std::thread([self, il]() { self->StepIntoAndWait(il); }).detach(); return true; } @@ -827,7 +833,8 @@ bool DebuggerController::StepIntoReverse(BNFunctionGraphType il) if (!CanResumeTarget()) return false; - std::thread([&, il]() { StepIntoReverseAndWait(il); }).detach(); + DbgRef self = this; + std::thread([self, il]() { self->StepIntoReverseAndWait(il); }).detach(); return true; } @@ -1078,7 +1085,8 @@ bool DebuggerController::StepOver(BNFunctionGraphType il) if (!CanResumeTarget()) return false; - std::thread([&, il]() { StepOverAndWait(il); }).detach(); + DbgRef self = this; + std::thread([self, il]() { self->StepOverAndWait(il); }).detach(); return true; } @@ -1089,7 +1097,8 @@ bool DebuggerController::StepOverReverse(BNFunctionGraphType il) if (!CanResumeTarget()) return false; - std::thread([&, il]() { StepOverReverseAndWait(il); }).detach(); + DbgRef self = this; + std::thread([self, il]() { self->StepOverReverseAndWait(il); }).detach(); return true; } @@ -1184,7 +1193,8 @@ bool DebuggerController::StepReturn() if (!CanResumeTarget()) return false; - std::thread([&]() { StepReturnAndWait(); }).detach(); + DbgRef self = this; + std::thread([self]() { self->StepReturnAndWait(); }).detach(); return true; } @@ -1195,7 +1205,8 @@ bool DebuggerController::StepReturnReverse() if (!CanResumeTarget()) return false; - std::thread([&]() { StepReturnReverseAndWait(); }).detach(); + DbgRef self = this; + std::thread([self]() { self->StepReturnReverseAndWait(); }).detach(); return true; } @@ -1295,7 +1306,8 @@ bool DebuggerController::RunTo(const std::vector& remoteAddresses) if (!CanResumeTarget()) return false; - std::thread([&, remoteAddresses]() { RunToAndWait(remoteAddresses); }).detach(); + DbgRef self = this; + std::thread([self, remoteAddresses]() { self->RunToAndWait(remoteAddresses); }).detach(); return true; } @@ -1307,7 +1319,8 @@ bool DebuggerController::RunToReverse(const std::vector& remoteAddress if (!CanResumeTarget()) return false; - std::thread([&, remoteAddresses]() { RunToReverseAndWait(remoteAddresses); }).detach(); + DbgRef self = this; + std::thread([self, remoteAddresses]() { self->RunToReverseAndWait(remoteAddresses); }).detach(); return true; } @@ -1457,7 +1470,8 @@ bool DebuggerController::Restart() if (!m_state->IsConnected()) return false; - std::thread([&]() { RestartAndWait(); }).detach(); + DbgRef self = this; + std::thread([self]() { self->RestartAndWait(); }).detach(); return true; } @@ -1517,7 +1531,8 @@ void DebuggerController::Detach() if (!m_state->IsConnected()) return; - std::thread([&]() { DetachAndWait(); }).detach(); + DbgRef self = this; + std::thread([self]() { self->DetachAndWait(); }).detach(); } @@ -1550,7 +1565,8 @@ void DebuggerController::Quit() if (!m_state->IsConnected()) return; - std::thread([&]() { QuitAndWait(); }).detach(); + DbgRef self = this; + std::thread([self]() { self->QuitAndWait(); }).detach(); } @@ -1589,7 +1605,8 @@ bool DebuggerController::Pause() if (!m_state->IsConnected()) return false; - std::thread([&]() { PauseAndWait(); }).detach(); + DbgRef self = this; + std::thread([self]() { self->PauseAndWait(); }).detach(); return true; } From 3882c0df4b1eae1b54fdc5978fabee83b13a132f Mon Sep 17 00:00:00 2001 From: Xusheng Date: Wed, 20 May 2026 15:24:16 -0400 Subject: [PATCH 2/8] Route all DebuggerController operations through a per-controller worker queue Replaces the previous "spawn one std::thread per operation and detach it" pattern with a single persistent worker thread per controller plus a FIFO work queue. Every public Xxx()/XxxAndWait() method now goes through Submit(). The worker thread is owned by the controller, started in the constructor and joined in the destructor (after draining), so any task can no longer outlive the controller it touches -- the lifetime guarantee is structural rather than reliant on per-call DbgRef captures (which this PR removes). Highlights: * Submit() detects re-entrant calls from the worker itself and runs them inline, so e.g. RestartAndWaitOnWorker can call QuitAndWaitOnWorker and LaunchAndWaitOnWorker without deadlocking on the queue. * SubmitAndWait() submits a task and waits on the resulting future. Every XxxAndWait now takes an optional std::chrono::milliseconds timeout, defaulting to milliseconds::max() ("wait forever") so existing callers in ffi.cpp and uinotification.cpp keep their current behavior. If a non-infinite timeout elapses, the engine is signaled to break and we still wait for the in-flight op to settle before returning. * Pause()/PauseAndWait() are special-cased: they bypass the queue and call m_adapter->BreakInto() directly on the caller's thread. The worker is blocked inside ExecuteAdapterAndWait for whatever resume op is in flight; BreakInto unsticks it. PauseAndWait then waits for the worker to drain past the running task by submitting a no-op probe. The existing public/internal split (XxxAndWaitInternal does the actual work; the old XxxAndWait was the lock-Internal-notify wrapper) is preserved. The old wrapper is renamed to XxxAndWaitOnWorker (private) and the new public XxxAndWait(timeout) is a thin SubmitAndWait wrapper around it. This keeps the public API surface identical to today for any caller that doesn't pass a timeout. Out of scope for this PR: adapter-internal threads (DbgEngAdapter::Attach spawned thread, EngineLoop, LLDB EventListener, RSP send/receive loops). Those have their own lifetime concerns and will be handled separately. Co-Authored-By: Claude Opus 4.7 (1M context) --- core/debuggercontroller.cpp | 262 ++++++++++++++++++++++++++++-------- core/debuggercontroller.h | 134 +++++++++++++++--- 2 files changed, 321 insertions(+), 75 deletions(-) diff --git a/core/debuggercontroller.cpp b/core/debuggercontroller.cpp index d876a4d1..7dca3848 100644 --- a/core/debuggercontroller.cpp +++ b/core/debuggercontroller.cpp @@ -39,11 +39,22 @@ DebuggerController::DebuggerController(BinaryViewRef data): BinaryDataNotificati RegisterEventCallback([this](const DebuggerEvent& event) { EventHandler(event); }, "Debugger Core"); m_debuggerEventThread = std::thread([&]{ DebuggerMainThread(); }); + + m_workerShouldExit = false; + m_workerThread = std::thread([this] { WorkerThreadMain(); }); } DebuggerController::~DebuggerController() { + { + std::lock_guard lock(m_workQueueMutex); + m_workerShouldExit = true; + } + m_workQueueCv.notify_all(); + if (m_workerThread.joinable()) + m_workerThread.join(); + m_shouldExit = true; m_cv.notify_all(); if (m_debuggerEventThread.joinable()) @@ -60,6 +71,27 @@ DebuggerController::~DebuggerController() } +void DebuggerController::WorkerThreadMain() +{ + m_workerThreadId = std::this_thread::get_id(); + while (true) + { + std::function task; + { + std::unique_lock lock(m_workQueueMutex); + m_workQueueCv.wait(lock, [this] { + return m_workerShouldExit || !m_workQueue.empty(); + }); + if (m_workerShouldExit && m_workQueue.empty()) + break; + task = std::move(m_workQueue.front()); + m_workQueue.pop(); + } + task(); + } +} + + void DebuggerController::AddBreakpoint(uint64_t address) { m_state->AddBreakpoint(address); @@ -290,8 +322,7 @@ bool DebuggerController::Launch() if (!CanStartDebgging()) return false; - DbgRef self = this; - std::thread([self]() { self->LaunchAndWait(); }).detach(); + Submit([this] { LaunchAndWaitOnWorker(); }); return true; } @@ -330,7 +361,7 @@ DebugStopReason DebuggerController::LaunchAndWaitInternal() } -DebugStopReason DebuggerController::LaunchAndWait() +DebugStopReason DebuggerController::LaunchAndWaitOnWorker() { // This is an API function of the debugger. We only do these checks at the API level. if (!CanStartDebgging()) @@ -348,14 +379,19 @@ DebugStopReason DebuggerController::LaunchAndWait() } +DebugStopReason DebuggerController::LaunchAndWait(std::chrono::milliseconds timeout) +{ + return SubmitAndWait([this] { return LaunchAndWaitOnWorker(); }, timeout); +} + + bool DebuggerController::Attach() { // This is an API function of the debugger. We only do these checks at the API level. if (!CanStartDebgging()) return false; - DbgRef self = this; - std::thread([self]() { self->AttachAndWait(); }).detach(); + Submit([this] { AttachAndWaitOnWorker(); }); return true; } @@ -382,7 +418,7 @@ DebugStopReason DebuggerController::AttachAndWaitInternal() } -DebugStopReason DebuggerController::AttachAndWait() +DebugStopReason DebuggerController::AttachAndWaitOnWorker() { // This is an API function of the debugger. We only do these checks at the API level. if (!CanStartDebgging()) @@ -400,14 +436,19 @@ DebugStopReason DebuggerController::AttachAndWait() } +DebugStopReason DebuggerController::AttachAndWait(std::chrono::milliseconds timeout) +{ + return SubmitAndWait([this] { return AttachAndWaitOnWorker(); }, timeout); +} + + bool DebuggerController::Connect() { // This is an API function of the debugger. We only do these checks at the API level. if (!CanStartDebgging()) return false; - DbgRef self = this; - std::thread([self]() { self->ConnectAndWait(); }).detach(); + Submit([this] { ConnectAndWaitOnWorker(); }); return true; } @@ -434,7 +475,7 @@ DebugStopReason DebuggerController::ConnectAndWaitInternal() } -DebugStopReason DebuggerController::ConnectAndWait() +DebugStopReason DebuggerController::ConnectAndWaitOnWorker() { // This is an API function of the debugger. We only do these checks at the API level. if (!CanStartDebgging()) @@ -452,6 +493,12 @@ DebugStopReason DebuggerController::ConnectAndWait() } +DebugStopReason DebuggerController::ConnectAndWait(std::chrono::milliseconds timeout) +{ + return SubmitAndWait([this] { return ConnectAndWaitOnWorker(); }, timeout); +} + + bool DebuggerController::Execute() { std::unique_lock lock(m_targetControlMutex); @@ -546,8 +593,7 @@ bool DebuggerController::Go() if (!CanResumeTarget()) return false; - DbgRef self = this; - std::thread([self]() { self->GoAndWait(); }).detach(); + Submit([this] { GoAndWaitOnWorker(); }); return true; } @@ -558,14 +604,13 @@ bool DebuggerController::GoReverse() if (!CanResumeTarget()) return false; - DbgRef self = this; - std::thread([self]() { self->GoReverseAndWait(); }).detach(); + Submit([this] { GoReverseAndWaitOnWorker(); }); return true; } -DebugStopReason DebuggerController::GoAndWait() +DebugStopReason DebuggerController::GoAndWaitOnWorker() { // This is an API function of the debugger. We only do these checks at the API level. if (!CanResumeTarget()) @@ -582,7 +627,14 @@ DebugStopReason DebuggerController::GoAndWait() return reason; } -DebugStopReason DebuggerController::GoReverseAndWait() + +DebugStopReason DebuggerController::GoAndWait(std::chrono::milliseconds timeout) +{ + return SubmitAndWait([this] { return GoAndWaitOnWorker(); }, timeout); +} + + +DebugStopReason DebuggerController::GoReverseAndWaitOnWorker() { // This is an API function of the debugger. We only do these checks at the API level. if (!CanResumeTarget()) @@ -600,6 +652,12 @@ DebugStopReason DebuggerController::GoReverseAndWait() } +DebugStopReason DebuggerController::GoReverseAndWait(std::chrono::milliseconds timeout) +{ + return SubmitAndWait([this] { return GoReverseAndWaitOnWorker(); }, timeout); +} + + DebugStopReason DebuggerController::StepIntoIL(BNFunctionGraphType il) { switch (il) @@ -822,8 +880,7 @@ bool DebuggerController::StepInto(BNFunctionGraphType il) if (!CanResumeTarget()) return false; - DbgRef self = this; - std::thread([self, il]() { self->StepIntoAndWait(il); }).detach(); + Submit([this, il] { StepIntoAndWaitOnWorker(il); }); return true; } @@ -833,13 +890,12 @@ bool DebuggerController::StepIntoReverse(BNFunctionGraphType il) if (!CanResumeTarget()) return false; - DbgRef self = this; - std::thread([self, il]() { self->StepIntoReverseAndWait(il); }).detach(); + Submit([this, il] { StepIntoReverseAndWaitOnWorker(il); }); return true; } -DebugStopReason DebuggerController::StepIntoReverseAndWait(BNFunctionGraphType il) +DebugStopReason DebuggerController::StepIntoReverseAndWaitOnWorker(BNFunctionGraphType il) { // This is an API function of the debugger. We only do these checks at the API level. if (!CanResumeTarget()) @@ -856,7 +912,13 @@ DebugStopReason DebuggerController::StepIntoReverseAndWait(BNFunctionGraphType i return reason; } -DebugStopReason DebuggerController::StepIntoAndWait(BNFunctionGraphType il) +DebugStopReason DebuggerController::StepIntoReverseAndWait(BNFunctionGraphType il, + std::chrono::milliseconds timeout) +{ + return SubmitAndWait([this, il] { return StepIntoReverseAndWaitOnWorker(il); }, timeout); +} + +DebugStopReason DebuggerController::StepIntoAndWaitOnWorker(BNFunctionGraphType il) { // This is an API function of the debugger. We only do these checks at the API level. if (!CanResumeTarget()) @@ -873,6 +935,12 @@ DebugStopReason DebuggerController::StepIntoAndWait(BNFunctionGraphType il) return reason; } +DebugStopReason DebuggerController::StepIntoAndWait(BNFunctionGraphType il, + std::chrono::milliseconds timeout) +{ + return SubmitAndWait([this, il] { return StepIntoAndWaitOnWorker(il); }, timeout); +} + DebugStopReason DebuggerController::StepOverIL(BNFunctionGraphType il) { switch (il) @@ -1085,8 +1153,7 @@ bool DebuggerController::StepOver(BNFunctionGraphType il) if (!CanResumeTarget()) return false; - DbgRef self = this; - std::thread([self, il]() { self->StepOverAndWait(il); }).detach(); + Submit([this, il] { StepOverAndWaitOnWorker(il); }); return true; } @@ -1097,14 +1164,13 @@ bool DebuggerController::StepOverReverse(BNFunctionGraphType il) if (!CanResumeTarget()) return false; - DbgRef self = this; - std::thread([self, il]() { self->StepOverReverseAndWait(il); }).detach(); + Submit([this, il] { StepOverReverseAndWaitOnWorker(il); }); return true; } -DebugStopReason DebuggerController::StepOverAndWait(BNFunctionGraphType il) +DebugStopReason DebuggerController::StepOverAndWaitOnWorker(BNFunctionGraphType il) { // This is an API function of the debugger. We only do these checks at the API level. if (!CanResumeTarget()) @@ -1122,7 +1188,14 @@ DebugStopReason DebuggerController::StepOverAndWait(BNFunctionGraphType il) } -DebugStopReason DebuggerController::StepOverReverseAndWait(BNFunctionGraphType il) +DebugStopReason DebuggerController::StepOverAndWait(BNFunctionGraphType il, + std::chrono::milliseconds timeout) +{ + return SubmitAndWait([this, il] { return StepOverAndWaitOnWorker(il); }, timeout); +} + + +DebugStopReason DebuggerController::StepOverReverseAndWaitOnWorker(BNFunctionGraphType il) { // This is an API function of the debugger. We only do these checks at the API level. if (!CanResumeTarget()) @@ -1140,6 +1213,13 @@ DebugStopReason DebuggerController::StepOverReverseAndWait(BNFunctionGraphType i } +DebugStopReason DebuggerController::StepOverReverseAndWait(BNFunctionGraphType il, + std::chrono::milliseconds timeout) +{ + return SubmitAndWait([this, il] { return StepOverReverseAndWaitOnWorker(il); }, timeout); +} + + DebugStopReason DebuggerController::EmulateStepReturnAndWait() { uint64_t address = m_state->IP(); @@ -1193,8 +1273,7 @@ bool DebuggerController::StepReturn() if (!CanResumeTarget()) return false; - DbgRef self = this; - std::thread([self]() { self->StepReturnAndWait(); }).detach(); + Submit([this] { StepReturnAndWaitOnWorker(); }); return true; } @@ -1205,14 +1284,13 @@ bool DebuggerController::StepReturnReverse() if (!CanResumeTarget()) return false; - DbgRef self = this; - std::thread([self]() { self->StepReturnReverseAndWait(); }).detach(); + Submit([this] { StepReturnReverseAndWaitOnWorker(); }); return true; } -DebugStopReason DebuggerController::StepReturnAndWait() +DebugStopReason DebuggerController::StepReturnAndWaitOnWorker() { // This is an API function of the debugger. We only do these checks at the API level. if (!CanResumeTarget()) @@ -1230,7 +1308,13 @@ DebugStopReason DebuggerController::StepReturnAndWait() } -DebugStopReason DebuggerController::StepReturnReverseAndWait() +DebugStopReason DebuggerController::StepReturnAndWait(std::chrono::milliseconds timeout) +{ + return SubmitAndWait([this] { return StepReturnAndWaitOnWorker(); }, timeout); +} + + +DebugStopReason DebuggerController::StepReturnReverseAndWaitOnWorker() { // This is an API function of the debugger. We only do these checks at the API level. if (!CanResumeTarget()) @@ -1248,6 +1332,12 @@ DebugStopReason DebuggerController::StepReturnReverseAndWait() } +DebugStopReason DebuggerController::StepReturnReverseAndWait(std::chrono::milliseconds timeout) +{ + return SubmitAndWait([this] { return StepReturnReverseAndWaitOnWorker(); }, timeout); +} + + DebugStopReason DebuggerController::RunToAndWaitInternal(const std::vector& remoteAddresses) { m_userRequestedBreak = false; @@ -1306,8 +1396,7 @@ bool DebuggerController::RunTo(const std::vector& remoteAddresses) if (!CanResumeTarget()) return false; - DbgRef self = this; - std::thread([self, remoteAddresses]() { self->RunToAndWait(remoteAddresses); }).detach(); + Submit([this, remoteAddresses] { RunToAndWaitOnWorker(remoteAddresses); }); return true; } @@ -1319,14 +1408,13 @@ bool DebuggerController::RunToReverse(const std::vector& remoteAddress if (!CanResumeTarget()) return false; - DbgRef self = this; - std::thread([self, remoteAddresses]() { self->RunToReverseAndWait(remoteAddresses); }).detach(); + Submit([this, remoteAddresses] { RunToReverseAndWaitOnWorker(remoteAddresses); }); return true; } -DebugStopReason DebuggerController::RunToAndWait(const std::vector& remoteAddresses) +DebugStopReason DebuggerController::RunToAndWaitOnWorker(const std::vector& remoteAddresses) { // This is an API function of the debugger. We only do these checks at the API level. if (!CanResumeTarget()) @@ -1344,7 +1432,15 @@ DebugStopReason DebuggerController::RunToAndWait(const std::vector& re } -DebugStopReason DebuggerController::RunToReverseAndWait(const std::vector& remoteAddresses) +DebugStopReason DebuggerController::RunToAndWait(const std::vector& remoteAddresses, + std::chrono::milliseconds timeout) +{ + return SubmitAndWait( + [this, remoteAddresses] { return RunToAndWaitOnWorker(remoteAddresses); }, timeout); +} + + +DebugStopReason DebuggerController::RunToReverseAndWaitOnWorker(const std::vector& remoteAddresses) { // This is an API function of the debugger. We only do these checks at the API level. if (!CanResumeTarget()) @@ -1362,6 +1458,14 @@ DebugStopReason DebuggerController::RunToReverseAndWait(const std::vector& remoteAddresses, + std::chrono::milliseconds timeout) +{ + return SubmitAndWait( + [this, remoteAddresses] { return RunToReverseAndWaitOnWorker(remoteAddresses); }, timeout); +} + + bool DebuggerController::CreateDebuggerBinaryView() { BinaryViewRef data = GetData(); @@ -1470,19 +1574,26 @@ bool DebuggerController::Restart() if (!m_state->IsConnected()) return false; - DbgRef self = this; - std::thread([self]() { self->RestartAndWait(); }).detach(); + Submit([this] { RestartAndWaitOnWorker(); }); return true; } -DebugStopReason DebuggerController::RestartAndWait() +DebugStopReason DebuggerController::RestartAndWaitOnWorker() { if (!m_state->IsConnected()) return InvalidStatusOrOperation; - QuitAndWait(); - return LaunchAndWait(); + // Bypass the public sync wrappers; we are already on the worker and want to + // run these inline without re-entering Submit. + QuitAndWaitOnWorker(); + return LaunchAndWaitOnWorker(); +} + + +DebugStopReason DebuggerController::RestartAndWait(std::chrono::milliseconds timeout) +{ + return SubmitAndWait([this] { return RestartAndWaitOnWorker(); }, timeout); } @@ -1531,12 +1642,11 @@ void DebuggerController::Detach() if (!m_state->IsConnected()) return; - DbgRef self = this; - std::thread([self]() { self->DetachAndWait(); }).detach(); + Submit([this] { DetachAndWaitOnWorker(); }); } -void DebuggerController::DetachAndWait() +void DebuggerController::DetachAndWaitOnWorker() { bool locked = false; if (m_targetControlMutex.try_lock()) @@ -1560,17 +1670,22 @@ void DebuggerController::DetachAndWait() } +void DebuggerController::DetachAndWait(std::chrono::milliseconds timeout) +{ + SubmitAndWait([this] { DetachAndWaitOnWorker(); }, timeout); +} + + void DebuggerController::Quit() { if (!m_state->IsConnected()) return; - DbgRef self = this; - std::thread([self]() { self->QuitAndWait(); }).detach(); + Submit([this] { QuitAndWaitOnWorker(); }); } -void DebuggerController::QuitAndWait() +void DebuggerController::QuitAndWaitOnWorker() { bool locked = false; if (m_targetControlMutex.try_lock()) @@ -1585,7 +1700,11 @@ void DebuggerController::QuitAndWait() if (m_state->IsRunning()) { - // We must pause the target if it is currently running, at least for DbgEngAdapter + // We must pause the target if it is currently running, at least for DbgEngAdapter. + // In the queue model the worker is the only thread running adapter operations, + // so reaching this branch implies a re-entrant call (e.g. Restart). PauseAndWait + // dispatches BreakInto out-of-band; the running op (if any) will settle and we + // proceed to issue the Quit below. PauseAndWait(); } @@ -1600,13 +1719,25 @@ void DebuggerController::QuitAndWait() } +void DebuggerController::QuitAndWait(std::chrono::milliseconds timeout) +{ + SubmitAndWait([this] { QuitAndWaitOnWorker(); }, timeout); +} + + bool DebuggerController::Pause() { if (!m_state->IsConnected()) return false; - DbgRef self = this; - std::thread([self]() { self->PauseAndWait(); }).detach(); + // Out-of-band: signal the engine to break on the caller's thread. The worker + // is presumed to be blocked inside ExecuteAdapterAndWait for whatever op is + // in flight (Go/Step/RunTo/etc.); when the engine receives the break it will + // report a stop, the worker's op will return, and its OnWorker wrapper will + // call NotifyStopped. We do not queue any work for the worker here. + m_userRequestedBreak = true; + if (m_adapter) + m_adapter->BreakInto(); return true; } @@ -1619,15 +1750,30 @@ DebugStopReason DebuggerController::PauseAndWaitInternal() } -DebugStopReason DebuggerController::PauseAndWait() +DebugStopReason DebuggerController::PauseAndWait(std::chrono::milliseconds timeout) { if (!m_state->IsConnected()) return InvalidStatusOrOperation; - auto reason = PauseAndWaitInternal(); - if ((reason != ProcessExited) && (reason != InternalError)) - NotifyStopped(reason); - return reason; + m_userRequestedBreak = true; + if (m_adapter) + m_adapter->BreakInto(); + + // Wait for the currently-running worker task (if any) to finish processing + // the break. Submitting a no-op gives us a future that resolves once the + // worker drains past whatever was in flight at the time of the break. + auto fut = Submit([] {}); + if (timeout == std::chrono::milliseconds::max()) + { + fut.wait(); + } + else if (fut.wait_for(timeout) != std::future_status::ready) + { + // BreakInto has already been signaled; there's nothing else to do. + return InternalError; + } + + return DebugStopReason::UserRequestedBreak; } diff --git a/core/debuggercontroller.h b/core/debuggercontroller.h index f298d902..3a44f59a 100644 --- a/core/debuggercontroller.h +++ b/core/debuggercontroller.h @@ -168,6 +168,27 @@ namespace BinaryNinjaDebugger { DebugStopReason RunToAndWaitInternal(const std::vector &remoteAddresses); DebugStopReason RunToReverseAndWaitInternal(const std::vector &remoteAddresses); + // Worker-thread bodies. Each runs on m_workerThread (via Submit) and performs the + // existing lock-Internal-notify wrapper. The public `XxxAndWait(timeout)` methods + // below submit one of these and wait on the resulting future. + DebugStopReason LaunchAndWaitOnWorker(); + DebugStopReason AttachAndWaitOnWorker(); + DebugStopReason ConnectAndWaitOnWorker(); + DebugStopReason GoAndWaitOnWorker(); + DebugStopReason GoReverseAndWaitOnWorker(); + DebugStopReason StepIntoAndWaitOnWorker(BNFunctionGraphType il); + DebugStopReason StepIntoReverseAndWaitOnWorker(BNFunctionGraphType il); + DebugStopReason StepOverAndWaitOnWorker(BNFunctionGraphType il); + DebugStopReason StepOverReverseAndWaitOnWorker(BNFunctionGraphType il); + DebugStopReason StepReturnAndWaitOnWorker(); + DebugStopReason StepReturnReverseAndWaitOnWorker(); + DebugStopReason RunToAndWaitOnWorker(const std::vector& remoteAddresses); + DebugStopReason RunToReverseAndWaitOnWorker(const std::vector& remoteAddresses); + DebugStopReason RestartAndWaitOnWorker(); + void DetachAndWaitOnWorker(); + void QuitAndWaitOnWorker(); + DebugStopReason PauseAndWaitOnWorker(); + // Whether we can start debugging, e.g., launch/attach/connec to a target bool CanStartDebgging(); // Whether we can resume the execution of the target, including stepping. @@ -201,6 +222,64 @@ namespace BinaryNinjaDebugger { std::thread m_debuggerEventThread; void DebuggerMainThread(); + // Worker queue: serializes all controller operations on a single thread. + // Replaces the per-op `std::thread(...).detach()` pattern. Tasks submitted from any + // thread run in order on m_workerThread; lifetime is owned and joined in the destructor. + // If Submit is called from the worker thread itself, the task runs inline to avoid + // deadlock when an operation needs to invoke another (e.g. Restart calls Quit + Launch). + std::thread m_workerThread; + std::thread::id m_workerThreadId; + std::mutex m_workQueueMutex; + std::condition_variable m_workQueueCv; + std::queue> m_workQueue; + std::atomic_bool m_workerShouldExit; + void WorkerThreadMain(); + + template + auto Submit(F&& f) -> std::future> + { + using R = std::invoke_result_t; + auto task = std::make_shared>(std::forward(f)); + auto future = task->get_future(); + + if (std::this_thread::get_id() == m_workerThreadId) + { + // Re-entrant call from the worker thread itself. Run inline so an outer + // operation can invoke an inner one without deadlocking on the queue. + (*task)(); + return future; + } + + { + std::lock_guard lock(m_workQueueMutex); + if (m_workerShouldExit) + return future; // future is left unset; caller's get() will throw broken_promise + m_workQueue.push([task]() { (*task)(); }); + } + m_workQueueCv.notify_one(); + return future; + } + + // Submit a worker task and block the caller until the task completes (or the timeout + // elapses, in which case the engine is signaled to break and we still wait for the + // in-flight op to settle before returning). A timeout of milliseconds::max() means + // "wait forever" and bypasses wait_for entirely (avoids overflow inside the stdlib). + template + auto SubmitAndWait(F&& f, std::chrono::milliseconds timeout) + -> std::invoke_result_t + { + auto fut = Submit(std::forward(f)); + if (timeout != std::chrono::milliseconds::max()) + { + if (fut.wait_for(timeout) != std::future_status::ready) + { + if (m_adapter) + m_adapter->BreakInto(); + } + } + return fut.get(); + } + std::unique_ptr m_uiCallbacks; uint64_t m_oldViewBase, m_newViewBase; @@ -351,23 +430,44 @@ namespace BinaryNinjaDebugger { DebugStopReason ExecuteAdapterAndWait(const DebugAdapterOperation operation); // Synchronous APIs - DebugStopReason LaunchAndWait(); - DebugStopReason GoAndWait(); - DebugStopReason GoReverseAndWait(); - DebugStopReason AttachAndWait(); - DebugStopReason RestartAndWait(); - DebugStopReason ConnectAndWait(); - DebugStopReason StepIntoAndWait(BNFunctionGraphType il = NormalFunctionGraph); - DebugStopReason StepIntoReverseAndWait(BNFunctionGraphType il = NormalFunctionGraph); - DebugStopReason StepOverAndWait(BNFunctionGraphType il = NormalFunctionGraph); - DebugStopReason StepOverReverseAndWait(BNFunctionGraphType il); - DebugStopReason StepReturnAndWait(); - DebugStopReason StepReturnReverseAndWait(); - DebugStopReason RunToAndWait(const std::vector& remoteAddresses); - DebugStopReason RunToReverseAndWait(const std::vector& remoteAddresses); - DebugStopReason PauseAndWait(); - void DetachAndWait(); - void QuitAndWait(); + // Synchronous APIs. They submit the operation to the worker thread and block the + // caller until it completes (or the optional timeout elapses, in which case the + // engine is signaled to break and the call returns once the in-flight op settles). + // Default timeout is "wait forever" so existing callers do not need to change. + DebugStopReason LaunchAndWait( + std::chrono::milliseconds timeout = std::chrono::milliseconds::max()); + DebugStopReason GoAndWait( + std::chrono::milliseconds timeout = std::chrono::milliseconds::max()); + DebugStopReason GoReverseAndWait( + std::chrono::milliseconds timeout = std::chrono::milliseconds::max()); + DebugStopReason AttachAndWait( + std::chrono::milliseconds timeout = std::chrono::milliseconds::max()); + DebugStopReason RestartAndWait( + std::chrono::milliseconds timeout = std::chrono::milliseconds::max()); + DebugStopReason ConnectAndWait( + std::chrono::milliseconds timeout = std::chrono::milliseconds::max()); + DebugStopReason StepIntoAndWait(BNFunctionGraphType il = NormalFunctionGraph, + std::chrono::milliseconds timeout = std::chrono::milliseconds::max()); + DebugStopReason StepIntoReverseAndWait(BNFunctionGraphType il = NormalFunctionGraph, + std::chrono::milliseconds timeout = std::chrono::milliseconds::max()); + DebugStopReason StepOverAndWait(BNFunctionGraphType il = NormalFunctionGraph, + std::chrono::milliseconds timeout = std::chrono::milliseconds::max()); + DebugStopReason StepOverReverseAndWait(BNFunctionGraphType il, + std::chrono::milliseconds timeout = std::chrono::milliseconds::max()); + DebugStopReason StepReturnAndWait( + std::chrono::milliseconds timeout = std::chrono::milliseconds::max()); + DebugStopReason StepReturnReverseAndWait( + std::chrono::milliseconds timeout = std::chrono::milliseconds::max()); + DebugStopReason RunToAndWait(const std::vector& remoteAddresses, + std::chrono::milliseconds timeout = std::chrono::milliseconds::max()); + DebugStopReason RunToReverseAndWait(const std::vector& remoteAddresses, + std::chrono::milliseconds timeout = std::chrono::milliseconds::max()); + DebugStopReason PauseAndWait( + std::chrono::milliseconds timeout = std::chrono::milliseconds::max()); + void DetachAndWait( + std::chrono::milliseconds timeout = std::chrono::milliseconds::max()); + void QuitAndWait( + std::chrono::milliseconds timeout = std::chrono::milliseconds::max()); // getters DebugAdapter* GetAdapter() { return m_adapter; } From b0619e79892a2ab21770f988ce1802854ca523fd Mon Sep 17 00:00:00 2001 From: Xusheng Date: Wed, 20 May 2026 16:40:36 -0400 Subject: [PATCH 3/8] Route adapter stops through an internal channel; remove dispatcher's stop machinery Adapter stops are now an internal signal from the adapter thread to the worker, not events on the public dispatcher queue. PostDebuggerEvent intercepts AdapterStoppedEventType and delivers the stop reason to a new mutex+condvar pair (m_adapterStopMutex / m_adapterStopCv / m_adapterStopPending). The worker consumes them via two paths: 1. In-flight ops: ExecuteAdapterAndWait now claims the channel for its entire duration (m_inAdapterWait = true), kicks off the adapter operation, then loops on WaitForAdapterStop. The conditional- breakpoint check moves here -- on a false condition we silently resume via m_adapter->Go() and wait again, without releasing the channel. This eliminates the old "drive m_adapter->Go() from the dispatcher thread" hack and the m_suppressResumeEvent flag that papered over it. 2. Spontaneous stops: when the user types e.g. `si` directly into the LLDB REPL while no controller op is in flight, m_inAdapterWait is false, so PostDebuggerEvent queues HandleSpontaneousAdapterStop on the worker -- which updates caches and calls NotifyStopped, the same effect as the dispatcher synthesis the old code did at debuggercontroller.cpp:2279. Target-exit / detach events continue through the dispatcher queue (they're user-visible) but ALSO signal the adapter-stop channel so an in-flight WaitForAdapterStop unblocks when the target dies. The dispatcher (DebuggerMainThread) no longer has any AdapterStoppedEventType-specific code paths. m_lastAdapterStopEventConsumed and m_suppressResumeEvent are removed; the temporary RegisterEventCallback/Semaphore pattern inside ExecuteAdapterAndWait is replaced by the direct WaitForAdapterStop wait. AdapterStoppedEventType remains in the public enum so adapters keep posting it the same way -- the change is purely in how the controller routes it once it arrives at PostDebuggerEvent. Adapter implementations need no changes. Stacked on the worker-queue refactor (this PR / #1087). Implements #1089. Co-Authored-By: Claude Opus 4.7 (1M context) --- core/debuggercontroller.cpp | 248 ++++++++++++++++++++++-------------- core/debuggercontroller.h | 21 ++- 2 files changed, 167 insertions(+), 102 deletions(-) diff --git a/core/debuggercontroller.cpp b/core/debuggercontroller.cpp index 7dca3848..397bb73a 100644 --- a/core/debuggercontroller.cpp +++ b/core/debuggercontroller.cpp @@ -52,6 +52,8 @@ DebuggerController::~DebuggerController() m_workerShouldExit = true; } m_workQueueCv.notify_all(); + // Wake any in-flight WaitForAdapterStop so the worker can observe shutdown. + m_adapterStopCv.notify_all(); if (m_workerThread.joinable()) m_workerThread.join(); @@ -2159,11 +2161,47 @@ bool DebuggerController::RemoveEventCallbackInternal(size_t index) void DebuggerController::PostDebuggerEvent(const DebuggerEvent& event) { - // During conditional breakpoint auto-resume, suppress the ResumeEventType that adapters - // post inside Go(). The target is already considered running by the UI, and posting this - // event from the dispatcher thread would trigger a re-entrant warning. - if (m_suppressResumeEvent && event.type == ResumeEventType) + // Adapter stops are an internal signal to the worker, not a user-facing event. + // Route them to the adapter-stop channel and skip the public dispatcher queue. + if (event.type == AdapterStoppedEventType) + { + DebugStopReason reason = event.data.targetStoppedData.reason; + bool inWait; + { + std::lock_guard lk(m_adapterStopMutex); + inWait = m_inAdapterWait; + if (inWait) + m_adapterStopPending = reason; + } + if (inWait) + { + m_adapterStopCv.notify_all(); + } + else + { + // No controller op is in flight — the adapter stopped on its own (e.g. + // the user typed `si` directly into the LLDB REPL). Queue a handler on + // the worker to update caches and synthesize a TargetStoppedEvent. + Submit([this, reason] { HandleSpontaneousAdapterStop(reason); }); + } return; + } + + // Target-exit / detach are user-facing events that ALSO need to unblock any + // in-flight WaitForAdapterStop (the engine isn't going to issue a separate stop). + if (event.type == TargetExitedEventType || event.type == DetachedEventType) + { + bool inWait; + { + std::lock_guard lk(m_adapterStopMutex); + inWait = m_inAdapterWait; + if (inWait) + m_adapterStopPending = ProcessExited; + } + if (inWait) + m_adapterStopCv.notify_all(); + // Fall through: still goes through the public dispatcher queue. + } auto pending = std::make_shared(); pending->event = event; @@ -2189,6 +2227,67 @@ void DebuggerController::PostDebuggerEvent(const DebuggerEvent& event) } +DebugStopReason DebuggerController::WaitForAdapterStop() +{ + std::unique_lock lk(m_adapterStopMutex); + m_adapterStopCv.wait(lk, [this] { + return m_adapterStopPending.has_value() || m_workerShouldExit; + }); + if (m_workerShouldExit && !m_adapterStopPending.has_value()) + return InternalError; + DebugStopReason reason = *m_adapterStopPending; + m_adapterStopPending = std::nullopt; + return reason; +} + + +bool DebuggerController::ShouldSilentResumeAfterStop() +{ + // Only breakpoint stops are candidates for silent resume on a false condition. + // Step operations always surface, even if they land on a breakpoint. + bool isStepOperation = (m_lastOperation == DebugAdapterStepInto) + || (m_lastOperation == DebugAdapterStepOver) + || (m_lastOperation == DebugAdapterStepReturn) + || (m_lastOperation == DebugAdapterStepIntoReverse) + || (m_lastOperation == DebugAdapterStepOverReverse) + || (m_lastOperation == DebugAdapterStepReturnReverse); + if (isStepOperation) + return false; + + m_state->SetConnectionStatus(DebugAdapterConnectedStatus); + m_state->SetExecutionStatus(DebugAdapterPausedStatus); + m_state->MarkDirty(); + m_state->UpdateCaches(); + AddRegisterValuesToExpressionParser(); + AddModuleValuesToExpressionParser(); + + uint64_t ip = m_state->IP(); + if (!m_state->GetBreakpoints()->ContainsAbsolute(ip)) + return false; + if (m_userRequestedBreak) + return false; + if (EvaluateBreakpointCondition(ip)) + return false; + + return true; +} + + +void DebuggerController::HandleSpontaneousAdapterStop(DebugStopReason reason) +{ + // The adapter reported a stop with no controller op in flight. This is the + // case the dispatcher previously synthesized a TargetStoppedEvent for at + // `debuggercontroller.cpp:2279` in the pre-refactor code. + m_state->SetConnectionStatus(DebugAdapterConnectedStatus); + m_state->SetExecutionStatus(DebugAdapterPausedStatus); + m_state->MarkDirty(); + m_state->UpdateCaches(); + AddRegisterValuesToExpressionParser(); + AddModuleValuesToExpressionParser(); + NotifyStopped(reason); +} + + void DebuggerController::DebuggerMainThread() { m_shouldExit = false; @@ -2212,49 +2311,11 @@ void DebuggerController::DebuggerMainThread() callbackLock.unlock(); auto event = current->event; - if (event.type == AdapterStoppedEventType) - m_lastAdapterStopEventConsumed = false; - if (event.type == AdapterStoppedEventType && - event.data.targetStoppedData.reason == Breakpoint) - { - // update the caches so registers are available for condition evaluation - m_state->SetConnectionStatus(DebugAdapterConnectedStatus); - m_state->SetExecutionStatus(DebugAdapterPausedStatus); - m_state->MarkDirty(); - m_state->UpdateCaches(); - AddRegisterValuesToExpressionParser(); - AddModuleValuesToExpressionParser(); - - // skip conditional breakpoint evaluation for step operations - when the user explicitly - // steps onto a breakpoint, they expect to stop there regardless of the condition. - bool isStepOperation = (m_lastOperation == DebugAdapterStepInto) - || (m_lastOperation == DebugAdapterStepOver) - || (m_lastOperation == DebugAdapterStepReturn) - || (m_lastOperation == DebugAdapterStepIntoReverse) - || (m_lastOperation == DebugAdapterStepOverReverse) - || (m_lastOperation == DebugAdapterStepReturnReverse); - - if (uint64_t ip = m_state->IP(); - !isStepOperation && m_state->GetBreakpoints()->ContainsAbsolute(ip)) - { - if (!EvaluateBreakpointCondition(ip) && !m_userRequestedBreak) - { - m_lastAdapterStopEventConsumed = true; - current->done.set_value(); - // Using m_adapter->Go() directly instead of Go() to avoid mutex deadlock - // since we're already inside ExecuteAdapterAndWait's event processing. - // Suppress the ResumeEventType that some adapters post synchronously inside - // Go() — the UI already considers the target running, and posting from the - // dispatcher thread would be unexpected. - m_suppressResumeEvent = true; - m_adapter->Go(); - m_suppressResumeEvent = false; - m_state->SetExecutionStatus(DebugAdapterRunningStatus); - continue; - } - } - } + // AdapterStoppedEventType no longer reaches the dispatcher: PostDebuggerEvent + // intercepts it and routes the reason to the worker's adapter-stop channel. + // Conditional-breakpoint silent-resume and spontaneous-stop synthesis now live + // in ExecuteAdapterAndWait / HandleSpontaneousAdapterStop on the worker. DebuggerEvent eventToSend = event; if ((eventToSend.type == TargetStoppedEventType) && !m_initialBreakpointSeen) @@ -2273,29 +2334,6 @@ void DebuggerController::DebuggerMainThread() cb.function(eventToSend); } - // If the current event is an AdapterStoppedEvent, and it is not consumed by any callback, then the adapter - // stop is not caused by the debugger core. This can happen when the user run a "ni" command directly. - // Notify a target stop reason in this case. - if (event.type == AdapterStoppedEventType && !m_lastAdapterStopEventConsumed) - { - DebuggerEvent stopEvent = event; - stopEvent.type = TargetStoppedEventType; - if (!m_initialBreakpointSeen) - { - m_initialBreakpointSeen = true; - stopEvent.data.targetStoppedData.reason = InitialBreakpoint; - } - for (const DebuggerEventCallback& cb : eventCallbacks) - { - std::unique_lock callbackLock2(m_callbackMutex); - if (m_disabledCallbacks.find(cb.index) != m_disabledCallbacks.end()) - continue; - - callbackLock2.unlock(); - cb.function(stopEvent); - } - } - CleanUpDisabledEvent(); current->done.set_value(); } @@ -2857,30 +2895,17 @@ DebugStopReason DebuggerController::ExecuteAdapterAndWait(const DebugAdapterOper } } - Semaphore sem; - DebugStopReason reason = UnknownReason; - size_t callback = RegisterEventCallback( - [&](const DebuggerEvent& event) { - switch (event.type) - { - case AdapterStoppedEventType: - reason = event.data.targetStoppedData.reason; - sem.Release(); - break; - // It is a little awkward to add two cases for these events, but we must take them into account, - // since after we resume the target, the target can either or exit. - case TargetExitedEventType: - case DetachedEventType: - // There is no DebugStopReason for "detach", so we use ProcessExited for now - reason = ProcessExited; - sem.Release(); - break; - default: - break; - } - m_lastAdapterStopEventConsumed = true; - }, - "WaitForAdapterStop"); + // Claim the adapter-stop channel for the duration of this call. Any AdapterStoppedEvent + // posted by the adapter from now until we clear m_inAdapterWait is delivered to + // WaitForAdapterStop below, not treated as spontaneous. We hold this across the + // entire silent-resume loop so that an adapter stop between iterations (after we + // kick off m_adapter->Go() for a false breakpoint condition) is still consumed + // by us, not synthesized as a spontaneous stop. + { + std::lock_guard lk(m_adapterStopMutex); + m_inAdapterWait = true; + m_adapterStopPending = std::nullopt; + } m_lastOperation = operation; @@ -2952,12 +2977,41 @@ DebugStopReason DebuggerController::ExecuteAdapterAndWait(const DebugAdapterOper ok = true; } - if (ok) - sem.Wait(); - else + DebugStopReason reason = UnknownReason; + if (!ok) + { reason = InternalError; + } + else + { + // Loop: wait for the adapter to stop. If the stop is a breakpoint whose + // condition evaluates to false (and the user didn't explicitly step or + // request a break), silently resume and wait again. Otherwise return. + while (true) + { + reason = WaitForAdapterStop(); + if (reason == ProcessExited || reason == InternalError) + break; + if (reason == Breakpoint && ShouldSilentResumeAfterStop()) + { + m_state->SetExecutionStatus(DebugAdapterRunningStatus); + if (!m_adapter || !m_adapter->Go()) + { + reason = InternalError; + break; + } + continue; + } + break; + } + } + + { + std::lock_guard lk(m_adapterStopMutex); + m_inAdapterWait = false; + m_adapterStopPending = std::nullopt; + } - RemoveEventCallback(callback); if ((operation != DebugAdapterPause) && (operation != DebugAdapterQuit) && (operation != DebugAdapterDetach)) m_adapterMutex.unlock(); else diff --git a/core/debuggercontroller.h b/core/debuggercontroller.h index 3a44f59a..32ba4491 100644 --- a/core/debuggercontroller.h +++ b/core/debuggercontroller.h @@ -22,6 +22,7 @@ limitations under the License. #include #include #include +#include #include #include "ffi_global.h" #include "refcountobject.h" @@ -119,11 +120,21 @@ namespace BinaryNinjaDebugger { bool m_userRequestedBreak = false; DebugAdapterOperation m_lastOperation = DebugAdapterGo; - bool m_lastAdapterStopEventConsumed = true; - - // When true, ResumeEventType events are suppressed in PostDebuggerEvent. - // Used during conditional breakpoint auto-resume to avoid posting events from the dispatcher thread. - bool m_suppressResumeEvent = false; + // Adapter-stop channel: internal signal from the adapter thread to the worker. + // AdapterStoppedEventType posted via PostDebuggerEvent is intercepted and routed + // here rather than dispatched through the public event queue. WaitForAdapterStop + // blocks on m_adapterStopCv until either an adapter stop arrives or shutdown is + // requested. m_inAdapterWait is true for the entire duration of an in-flight + // ExecuteAdapterAndWait call (including the silent-resume loop between iterations + // for conditional breakpoints) so that any stop during that window is consumed + // by WaitForAdapterStop and not treated as spontaneous. + std::mutex m_adapterStopMutex; + std::condition_variable m_adapterStopCv; + std::optional m_adapterStopPending; + bool m_inAdapterWait = false; + DebugStopReason WaitForAdapterStop(); + void HandleSpontaneousAdapterStop(DebugStopReason reason); + bool ShouldSilentResumeAfterStop(); bool m_inputFileLoaded = false; bool m_initialBreakpointSeen = false; From 0326f86b67ceaeb77d481407641f4c8e6df82d2b Mon Sep 17 00:00:00 2001 From: Xusheng Date: Wed, 20 May 2026 17:03:40 -0400 Subject: [PATCH 4/8] Make Restart/Quit/Detach interrupt the running target Two bugs in how interrupting ops (Restart/Quit/Detach) interact with the worker queue: 1. The public Restart/Quit/Detach methods just Submit'd their work to the queue. If a resume op (Go/Step/RunTo) was in flight, the worker was blocked inside WaitForAdapterStop -- the queued Restart/Quit/Detach would sit indefinitely with nothing to interrupt the running target. To the user, clicking Restart while running looked like a no-op. Fix: these methods now call RequestInterrupt() (a small helper that sets m_userRequestedBreak and invokes m_adapter->BreakInto()) on the caller's thread before queueing the task -- same pattern as Pause(). The BreakInto unsticks the in-flight op's WaitForAdapterStop, that op drains, and the queued Restart/Quit/Detach runs next. Applied to both the async public methods and their *AndWait(timeout) variants. 2. QuitAndWaitOnWorker, when it observed IsRunning() inside a re-entrant call (e.g. from Restart), called the public PauseAndWait() -- which does BreakInto + Submit([]{}).wait(). On the worker thread Submit detects re-entry and runs the empty lambda inline, so the wait was a no-op. We then issued Quit on a still-running target, which DbgEng in particular doesn't handle well. Fix: switch that call to PauseAndWaitInternal(), which routes through ExecuteAdapterAndWait(Pause) and actually waits for the engine via the adapter-stop channel. With (1) in place this branch should rarely fire, but it's the correct defensive behavior. Also pulled the BreakInto + m_userRequestedBreak pair out of Pause and PauseAndWait into the shared RequestInterrupt() helper. Co-Authored-By: Claude Opus 4.7 (1M context) --- core/debuggercontroller.cpp | 50 ++++++++++++++++++++++++++++--------- core/debuggercontroller.h | 6 +++++ 2 files changed, 44 insertions(+), 12 deletions(-) diff --git a/core/debuggercontroller.cpp b/core/debuggercontroller.cpp index 397bb73a..09c5cf2b 100644 --- a/core/debuggercontroller.cpp +++ b/core/debuggercontroller.cpp @@ -1576,6 +1576,10 @@ bool DebuggerController::Restart() if (!m_state->IsConnected()) return false; + // Interrupt any in-flight resume op so the queued Restart can actually run. + // Without this, if the target is running the worker is blocked in WaitForAdapterStop + // and Restart would sit in the queue indefinitely. + RequestInterrupt(); Submit([this] { RestartAndWaitOnWorker(); }); return true; } @@ -1595,6 +1599,10 @@ DebugStopReason DebuggerController::RestartAndWaitOnWorker() DebugStopReason DebuggerController::RestartAndWait(std::chrono::milliseconds timeout) { + if (!m_state->IsConnected()) + return InvalidStatusOrOperation; + + RequestInterrupt(); return SubmitAndWait([this] { return RestartAndWaitOnWorker(); }, timeout); } @@ -1644,6 +1652,8 @@ void DebuggerController::Detach() if (!m_state->IsConnected()) return; + // Interrupt any in-flight resume op (see Restart for rationale). + RequestInterrupt(); Submit([this] { DetachAndWaitOnWorker(); }); } @@ -1674,6 +1684,10 @@ void DebuggerController::DetachAndWaitOnWorker() void DebuggerController::DetachAndWait(std::chrono::milliseconds timeout) { + if (!m_state->IsConnected()) + return; + + RequestInterrupt(); SubmitAndWait([this] { DetachAndWaitOnWorker(); }, timeout); } @@ -1683,6 +1697,8 @@ void DebuggerController::Quit() if (!m_state->IsConnected()) return; + // Interrupt any in-flight resume op (see Restart for rationale). + RequestInterrupt(); Submit([this] { QuitAndWaitOnWorker(); }); } @@ -1703,11 +1719,12 @@ void DebuggerController::QuitAndWaitOnWorker() if (m_state->IsRunning()) { // We must pause the target if it is currently running, at least for DbgEngAdapter. - // In the queue model the worker is the only thread running adapter operations, - // so reaching this branch implies a re-entrant call (e.g. Restart). PauseAndWait - // dispatches BreakInto out-of-band; the running op (if any) will settle and we - // proceed to issue the Quit below. - PauseAndWait(); + // Call PauseAndWaitInternal (not the public PauseAndWait) so we go through + // ExecuteAdapterAndWait(Pause) and actually wait for the engine to stop via the + // adapter-stop channel. The public PauseAndWait would re-enter Submit inline here + // (we are on the worker) and return without waiting, leaving the engine still + // running when we issue Quit below. + PauseAndWaitInternal(); } // TODO: return whether the operation is successful @@ -1723,6 +1740,10 @@ void DebuggerController::QuitAndWaitOnWorker() void DebuggerController::QuitAndWait(std::chrono::milliseconds timeout) { + if (!m_state->IsConnected()) + return; + + RequestInterrupt(); SubmitAndWait([this] { QuitAndWaitOnWorker(); }, timeout); } @@ -1737,10 +1758,7 @@ bool DebuggerController::Pause() // in flight (Go/Step/RunTo/etc.); when the engine receives the break it will // report a stop, the worker's op will return, and its OnWorker wrapper will // call NotifyStopped. We do not queue any work for the worker here. - m_userRequestedBreak = true; - if (m_adapter) - m_adapter->BreakInto(); - + RequestInterrupt(); return true; } @@ -1757,9 +1775,7 @@ DebugStopReason DebuggerController::PauseAndWait(std::chrono::milliseconds timeo if (!m_state->IsConnected()) return InvalidStatusOrOperation; - m_userRequestedBreak = true; - if (m_adapter) - m_adapter->BreakInto(); + RequestInterrupt(); // Wait for the currently-running worker task (if any) to finish processing // the break. Submitting a no-op gives us a future that resolves once the @@ -2288,6 +2304,16 @@ void DebuggerController::HandleSpontaneousAdapterStop(DebugStopReason reason) } +void DebuggerController::RequestInterrupt() +{ + // Set the flag first so any in-flight op's silent-resume / NotifyStopped checks + // observe the break request before the engine reports the resulting stop. + m_userRequestedBreak = true; + if (m_adapter) + m_adapter->BreakInto(); +} + + void DebuggerController::DebuggerMainThread() { m_shouldExit = false; diff --git a/core/debuggercontroller.h b/core/debuggercontroller.h index 32ba4491..a8ff62a3 100644 --- a/core/debuggercontroller.h +++ b/core/debuggercontroller.h @@ -136,6 +136,12 @@ namespace BinaryNinjaDebugger { void HandleSpontaneousAdapterStop(DebugStopReason reason); bool ShouldSilentResumeAfterStop(); + // Out-of-band: signal the engine to break and mark the break as user-requested. + // Called from Pause/Restart/Quit/Detach on the caller's thread before queueing + // the actual operation, so the worker's in-flight resume op (if any) gets + // interrupted and the queued task can proceed. + void RequestInterrupt(); + bool m_inputFileLoaded = false; bool m_initialBreakpointSeen = false; From 3a44260f2e806f5bcc222cec0d3a6c89f3eefa10 Mon Sep 17 00:00:00 2001 From: Xusheng Date: Thu, 21 May 2026 15:58:29 -0400 Subject: [PATCH 5/8] Move state mutations from EventHandler onto the event-posting thread The controller used to subscribe to its own broadcast: when something happened, it posted a DebuggerEvent to its own dispatcher, the dispatcher pulled the event back out, handed it to EventHandler (running on m_debuggerEventThread), and EventHandler mutated m_state. That roundtrip is pure overhead for what is structurally just a "controller updates its own state" operation, and it created a race: the worker could observe stale m_state after WaitForAdapterStop returned but before the dispatcher had gotten around to running EventHandler. The previous commit (now reverted) patched that race by deferring the worker-wake until after the dispatcher; this commit removes the roundtrip entirely. Renames EventHandler -> ApplyOwnStateForEvent (same body) and calls it inline from PostDebuggerEvent, before the event is enqueued for the dispatcher and before the adapter-stop channel is signaled. Drops the RegisterEventCallback("Debugger Core") registration in the ctor. The dispatcher now only fans out to genuinely-external consumers (UI widgets, plugins, scripting providers); the controller's own state is updated synchronously by whichever thread posted the event. This also means the adhoc fix in d4973e0 (deferring the channel signal until the dispatcher had updated state) is no longer needed -- the signal in PostDebuggerEvent can fire immediately after ApplyOwnStateForEvent, because the state is already current. That commit is reset out of this branch. Thread-safety notes: m_state's connection/execution status fields are simple assignments. The other touched fields (m_lastIP, m_currentIP, m_exitCode, caches via UpdateCaches, etc.) used to be mutated on the single dispatcher thread; they're now mutated on whichever thread posts each event. In practice the engine produces one event at a time so concurrent mutation doesn't arise, but if it ever does a mutex is trivial to add. Deferred m_accessor deletion still uses the detached-thread trick so we don't self-join if the worker is the one calling. Co-Authored-By: Claude Opus 4.7 (1M context) --- core/debuggercontroller.cpp | 48 ++++++++++++++++++++++++++----------- core/debuggercontroller.h | 6 ++++- 2 files changed, 39 insertions(+), 15 deletions(-) diff --git a/core/debuggercontroller.cpp b/core/debuggercontroller.cpp index 09c5cf2b..c308f547 100644 --- a/core/debuggercontroller.cpp +++ b/core/debuggercontroller.cpp @@ -36,7 +36,6 @@ DebuggerController::DebuggerController(BinaryViewRef data): BinaryDataNotificati m_state = new DebuggerState(data, this); m_adapter = nullptr; m_shouldAnnotateStackVariable = Settings::Instance()->Get("debugger.stackVariableAnnotations"); - RegisterEventCallback([this](const DebuggerEvent& event) { EventHandler(event); }, "Debugger Core"); m_debuggerEventThread = std::thread([&]{ DebuggerMainThread(); }); @@ -2023,8 +2022,24 @@ void DebuggerController::Destroy() } -// This is the central hub of event dispatch. All events first arrive here and then get dispatched based on the content -void DebuggerController::EventHandler(const DebuggerEvent& event) +// The controller's own state mutations for each event type. Called inline from +// PostDebuggerEvent (on whichever thread posted the event) BEFORE the event is +// enqueued for the dispatcher. Previously this body lived in EventHandler running +// on the dispatcher thread; that created a race where the worker could observe +// stale m_state after WaitForAdapterStop returned but before the dispatcher had +// gotten around to running EventHandler. Now the controller's state is updated +// happen-before the broadcast, and the dispatcher only fans out to external +// (UI, plugin, scripting) consumers. +// +// Thread-safety: m_state's connection/execution status fields are simple atomic-ish +// assignments. The other touched fields (m_lastIP, m_currentIP, m_exitCode, the +// caches updated via UpdateCaches) are mutated under the worker's serialization +// for ops the worker generates (TargetStopped, LaunchFailure, etc.) and under +// the adapter event thread for spontaneous events (TargetExited, RegisterChanged, +// ActiveThreadChanged). Concurrent mutation is unlikely in practice because the +// engine produces one event at a time, but if it becomes a problem the mutex is +// trivial to add. +void DebuggerController::ApplyOwnStateForEvent(const DebuggerEvent& event) { switch (event.type) { @@ -2056,15 +2071,11 @@ void DebuggerController::EventHandler(const DebuggerEvent& event) if (m_accessor) { - // Defer deletion to a detached thread. The accessor holds a DbgRef, - // and if it is the last reference, deleting it here (on the event thread) would trigger - // ~DebuggerController which calls m_debuggerEventThread.join() -- deadlocking/crashing - // because we ARE the event thread. - // - // This can happen when Destroy() races with event processing: EventHandler sets - // ConnectionStatus to NotConnected (line above), and another thread observes this, - // calls Destroy() which removes the global array ref, making the accessor's DbgRef - // the last reference to the controller. + // Defer deletion to a detached thread. The accessor holds a DbgRef; + // if it's the last reference, deleting it here would trigger ~DebuggerController which + // calls m_workerThread.join() and m_debuggerEventThread.join(). If we happen to be on + // either of those threads (e.g. the worker just finished Quit and called us), the join + // would deadlock. A detached thread sidesteps that regardless of who called us. auto* accessor = m_accessor; m_accessor = nullptr; std::thread([accessor]() { delete accessor; }).detach(); @@ -2203,8 +2214,17 @@ void DebuggerController::PostDebuggerEvent(const DebuggerEvent& event) return; } - // Target-exit / detach are user-facing events that ALSO need to unblock any - // in-flight WaitForAdapterStop (the engine isn't going to issue a separate stop). + // Apply the controller's own state mutations synchronously, before this event + // reaches anyone else. Previously this happened in EventHandler running on the + // dispatcher thread, which created a race: the worker could observe stale + // m_state after WaitForAdapterStop returned but before EventHandler had run. + // Doing the mutations here means the broadcast is purely informational to + // external consumers; the controller's own state is already consistent. + ApplyOwnStateForEvent(event); + + // Target-exit / detach also unblock any in-flight WaitForAdapterStop -- the + // engine isn't going to issue a separate AdapterStoppedEvent. We do this AFTER + // ApplyOwnStateForEvent so the worker wakes to a fully-updated m_state. if (event.type == TargetExitedEventType || event.type == DetachedEventType) { bool inWait; diff --git a/core/debuggercontroller.h b/core/debuggercontroller.h index a8ff62a3..52dc688b 100644 --- a/core/debuggercontroller.h +++ b/core/debuggercontroller.h @@ -152,7 +152,11 @@ namespace BinaryNinjaDebugger { bool m_shouldAnnotateStackVariable = false; - void EventHandler(const DebuggerEvent& event); + // Apply the controller's own state mutations for each event type. Called inline + // from PostDebuggerEvent before the event is enqueued for the dispatcher, so + // m_state is consistent before any external consumer (or the worker waking from + // WaitForAdapterStop) observes the change. + void ApplyOwnStateForEvent(const DebuggerEvent& event); void UpdateStackVariables(); void AddRegisterValuesToExpressionParser(); void AddModuleValuesToExpressionParser(); From 6f2861e9145442befd987a90063b677d6baa580d Mon Sep 17 00:00:00 2001 From: Xusheng Date: Thu, 21 May 2026 16:29:15 -0400 Subject: [PATCH 6/8] Drop m_adapterMutex/m_adapterMutex2; make m_adapter atomic The two adapter mutexes were the synchronization mechanism for an older model where multiple threads (spawned per operation) could call into m_adapter concurrently. mutex1 covered Go/Step/Launch/etc.; mutex2 was separate so Pause/Quit/Detach could acquire their lock without blocking on a long-running resume op. With the worker queue, ExecuteAdapterAndWait runs exclusively on m_workerThread, so try_lock() always succeeded immediately and the mutexes did nothing. The new Pause path doesn't go through ExecuteAdapterAndWait at all -- it calls m_adapter->BreakInto() out-of-band from RequestInterrupt -- so there is no longer any "concurrent" adapter access that the mutexes were protecting against. Replaces them with a `assert(this_thread::get_id() == m_workerThreadId)` at the top of ExecuteAdapterAndWait to document and enforce the actual invariant. Separately addresses the m_adapter pointer-access race that the old mutexes never actually fixed: m_adapter is written from the worker (CreateDebugAdapter) and read from arbitrary threads (RequestInterrupt's BreakInto path). Made m_adapter `std::atomic` so the cross-thread read/write pair is race-free under C++20's memory model. All ~50 internal `m_adapter->...` sites become `m_adapter.load()->...`; null-checks and pointer-typed assignments keep working unchanged via std::atomic's implicit conversion. RequestInterrupt and SubmitAndWait's break-on-timeout path now snapshot the pointer with one acquire load to avoid load+call races. The pointed-to adapter object's lifetime is still guaranteed externally: the adapter is destroyed in ~DebuggerState (which runs inside ~DebuggerController after both worker threads have joined), and DbgRef holders on the controller prevent its destruction during cross-thread Pause/Restart/Quit/Detach calls. Co-Authored-By: Claude Opus 4.7 (1M context) --- core/debuggercontroller.cpp | 139 +++++++++++++++++------------------- core/debuggercontroller.h | 23 +++--- 2 files changed, 76 insertions(+), 86 deletions(-) diff --git a/core/debuggercontroller.cpp b/core/debuggercontroller.cpp index c308f547..0dcf051d 100644 --- a/core/debuggercontroller.cpp +++ b/core/debuggercontroller.cpp @@ -15,6 +15,8 @@ limitations under the License. */ #include "debuggercontroller.h" +#include +#include #include #include #include "lowlevelilinstruction.h" @@ -513,7 +515,7 @@ bool DebuggerController::Execute() std::replace(filePath.begin(), filePath.end(), '/', '\\'); #endif - return m_adapter->ExecuteWithArgs( + return m_adapter.load()->ExecuteWithArgs( filePath, m_state->GetCommandLineArguments(), m_state->GetWorkingDirectory(), configs); } @@ -540,7 +542,7 @@ bool DebuggerController::CreateDebugAdapter() return false; } - if (!m_adapter->Init()) + if (!m_adapter.load()->Init()) { LogWarn("Failed to init an adapter of type %s", m_state->GetAdapterType().c_str()); return false; @@ -550,12 +552,12 @@ bool DebuggerController::CreateDebugAdapter() m_state->SetAdapter(m_adapter); // This is a very hacky way to let the adapter have a handle to the controller, and then be able to access the // Binary View object - m_adapter->SetController(this); + m_adapter.load()->SetController(this); ApplyBreakpoints(); // Forward the DebuggerEvent from the adapters to the controller - m_adapter->SetEventCallback([this](const DebuggerEvent& event) { PostDebuggerEvent(event); }); + m_adapter.load()->SetEventCallback([this](const DebuggerEvent& event) { PostDebuggerEvent(event); }); return true; } @@ -1347,7 +1349,7 @@ DebugStopReason DebuggerController::RunToAndWaitInternal(const std::vectorGetBreakpoints()->ContainsAbsolute(remoteAddress)) { - m_adapter->AddBreakpoint(remoteAddress); + m_adapter.load()->AddBreakpoint(remoteAddress); } } @@ -1357,7 +1359,7 @@ DebugStopReason DebuggerController::RunToAndWaitInternal(const std::vectorGetBreakpoints()->ContainsAbsolute(remoteAddress)) { - m_adapter->RemoveBreakpoint(remoteAddress); + m_adapter.load()->RemoveBreakpoint(remoteAddress); } } @@ -1373,7 +1375,7 @@ DebugStopReason DebuggerController::RunToReverseAndWaitInternal(const std::vecto { if (!m_state->GetBreakpoints()->ContainsAbsolute(remoteAddress)) { - m_adapter->AddBreakpoint(remoteAddress); + m_adapter.load()->AddBreakpoint(remoteAddress); } } @@ -1383,7 +1385,7 @@ DebugStopReason DebuggerController::RunToReverseAndWaitInternal(const std::vecto { if (!m_state->GetBreakpoints()->ContainsAbsolute(remoteAddress)) { - m_adapter->RemoveBreakpoint(remoteAddress); + m_adapter.load()->RemoveBreakpoint(remoteAddress); } } @@ -1615,7 +1617,7 @@ bool DebuggerController::ConnectToDebugServer() if (!CreateDebugAdapter()) return false; - bool ok = m_adapter->ConnectToDebugServer(m_state->GetRemoteHost(), m_state->GetRemotePort()); + bool ok = m_adapter.load()->ConnectToDebugServer(m_state->GetRemoteHost(), m_state->GetRemotePort()); if (!ok) LogWarn("Failed to connect to the debug server"); else @@ -1630,7 +1632,7 @@ bool DebuggerController::DisconnectDebugServer() if (!m_state->IsConnectedToDebugServer()) return true; - bool ok = m_adapter->DisconnectDebugServer(); + bool ok = m_adapter.load()->DisconnectDebugServer(); if (!ok) LogWarn("Failed to disconnect from the debug server"); else @@ -1825,7 +1827,7 @@ DebugStopReason DebuggerController::EmulateStepOverAndWait() return InternalError; size_t size = remoteArch->GetMaxInstructionLength(); - DataBuffer buffer = m_adapter->ReadMemory(remoteIP, size); + DataBuffer buffer = m_adapter.load()->ReadMemory(remoteIP, size); size_t bytesRead = buffer.GetLength(); Ref ilFunc = new LowLevelILFunction(remoteArch, nullptr); @@ -1870,7 +1872,7 @@ DebugStopReason DebuggerController::StepOverAndWaitInternal() { m_userRequestedBreak = false; - if (m_adapter && m_adapter->SupportFeature(DebugAdapterSupportStepOver)) + if (m_adapter && m_adapter.load()->SupportFeature(DebugAdapterSupportStepOver)) { return ExecuteAdapterAndWait(DebugAdapterStepOver); } @@ -1885,7 +1887,7 @@ DebugStopReason DebuggerController::StepOverReverseAndWaitInternal() { m_userRequestedBreak = false; - if (m_adapter && m_adapter->SupportFeature(DebugAdapterSupportStepOverReverse)) + if (m_adapter && m_adapter.load()->SupportFeature(DebugAdapterSupportStepOverReverse)) { return ExecuteAdapterAndWait(DebugAdapterStepOverReverse); } @@ -2329,8 +2331,12 @@ void DebuggerController::RequestInterrupt() // Set the flag first so any in-flight op's silent-resume / NotifyStopped checks // observe the break request before the engine reports the resulting stop. m_userRequestedBreak = true; - if (m_adapter) - m_adapter->BreakInto(); + // Snapshot the adapter pointer with a single atomic load so we don't race between + // the null check and the call. The pointed-to object is kept alive by the caller's + // DbgRef on the controller (the adapter is destroyed only inside ~DebuggerState, + // which runs inside ~DebuggerController after both worker threads have joined). + if (DebugAdapter* adapter = m_adapter.load(std::memory_order_acquire)) + adapter->BreakInto(); } @@ -2473,7 +2479,7 @@ std::vector DebuggerController::GetProcessList() return {}; } - return m_adapter->GetProcessList(); + return m_adapter.load()->GetProcessList(); } @@ -2529,7 +2535,7 @@ uint32_t DebuggerController::GetActivePID() { if (!m_adapter) return 0; - return m_adapter->GetActivePID(); + return m_adapter.load()->GetActivePID(); } @@ -2537,7 +2543,7 @@ void DebuggerController::WriteStdIn(const std::string message) { if (m_adapter && m_state->IsRunning()) { - m_adapter->WriteStdin(message); + m_adapter.load()->WriteStdin(message); } else { @@ -2562,7 +2568,7 @@ std::string DebuggerController::InvokeBackendCommand(const std::string& cmd) else m_lastCommand = cmdToSend; - return m_adapter->InvokeBackendCommand(cmdToSend); + return m_adapter.load()->InvokeBackendCommand(cmdToSend); } return "Error: invalid adapter\n"; @@ -2916,36 +2922,24 @@ DebugStopReason DebuggerController::StopReason() const if (!m_adapter) return UnknownReason; - return m_adapter->StopReason(); + return m_adapter.load()->StopReason(); } DebugStopReason DebuggerController::ExecuteAdapterAndWait(const DebugAdapterOperation operation) { - // Due to the nature of the wait, this mutex should NOT be allowed to be locked recursively. - // If this is a pause operation, do not try to lock the mutex -- it is mostly likely held by another thread - if ((operation != DebugAdapterPause) && (operation != DebugAdapterQuit) && (operation != DebugAdapterDetach)) - { - if (!m_adapterMutex.try_lock()) - { - LogWarn("Cannot obtain mutex1 for debug adapter, operation: %d", operation); - return InternalError; - } - } - else - { - if (!m_adapterMutex2.try_lock()) - { - LogWarn("Cannot obtain mutex2 for debug adapter, operation: %d", operation); - return InternalError; - } - } + // Invariant: ExecuteAdapterAndWait only ever runs on m_workerThread. The worker queue + // serializes all adapter operations, so the previous m_adapterMutex / m_adapterMutex2 + // pair (which guarded against concurrent adapter access from multiple spawned threads) + // is no longer needed. The new Pause path bypasses this method entirely and calls + // m_adapter.load()->BreakInto() out-of-band; everything else funnels through here on the worker. + assert(std::this_thread::get_id() == m_workerThreadId); // Claim the adapter-stop channel for the duration of this call. Any AdapterStoppedEvent // posted by the adapter from now until we clear m_inAdapterWait is delivered to // WaitForAdapterStop below, not treated as spontaneous. We hold this across the // entire silent-resume loop so that an adapter stop between iterations (after we - // kick off m_adapter->Go() for a false breakpoint condition) is still consumed + // kick off m_adapter.load()->Go() for a false breakpoint condition) is still consumed // by us, not synthesized as a spontaneous stop. { std::lock_guard lk(m_adapterStopMutex); @@ -2960,46 +2954,46 @@ DebugStopReason DebuggerController::ExecuteAdapterAndWait(const DebugAdapterOper switch (operation) { case DebugAdapterGo: - resumeOK = m_adapter->Go(); + resumeOK = m_adapter.load()->Go(); break; case DebugAdapterGoReverse: - resumeOK = m_adapter->GoReverse(); + resumeOK = m_adapter.load()->GoReverse(); break; case DebugAdapterStepInto: - resumeOK = m_adapter->StepInto(); + resumeOK = m_adapter.load()->StepInto(); break; case DebugAdapterStepIntoReverse: - resumeOK = m_adapter->StepIntoReverse(); + resumeOK = m_adapter.load()->StepIntoReverse(); break; case DebugAdapterStepOver: - resumeOK = m_adapter->StepOver(); + resumeOK = m_adapter.load()->StepOver(); break; case DebugAdapterStepOverReverse: - resumeOK = m_adapter->StepOverReverse(); + resumeOK = m_adapter.load()->StepOverReverse(); break; case DebugAdapterStepReturn: - resumeOK = m_adapter->StepReturn(); + resumeOK = m_adapter.load()->StepReturn(); break; case DebugAdapterStepReturnReverse: - resumeOK = m_adapter->StepReturnReverse(); + resumeOK = m_adapter.load()->StepReturnReverse(); break; case DebugAdapterPause: - operationRequested = m_adapter->BreakInto(); + operationRequested = m_adapter.load()->BreakInto(); break; case DebugAdapterQuit: - operationRequested = m_adapter->Quit(); + operationRequested = m_adapter.load()->Quit(); break; case DebugAdapterDetach: - operationRequested = m_adapter->Detach(); + operationRequested = m_adapter.load()->Detach(); break; case DebugAdapterLaunch: resumeOK = Execute(); break; case DebugAdapterAttach: - resumeOK = m_adapter->Attach(m_state->GetPIDAttach()); + resumeOK = m_adapter.load()->Attach(m_state->GetPIDAttach()); break; case DebugAdapterConnect: - resumeOK = m_adapter->Connect(m_state->GetRemoteHost(), m_state->GetRemotePort()); + resumeOK = m_adapter.load()->Connect(m_state->GetRemoteHost(), m_state->GetRemotePort()); break; default: break; @@ -3041,7 +3035,7 @@ DebugStopReason DebuggerController::ExecuteAdapterAndWait(const DebugAdapterOper if (reason == Breakpoint && ShouldSilentResumeAfterStop()) { m_state->SetExecutionStatus(DebugAdapterRunningStatus); - if (!m_adapter || !m_adapter->Go()) + if (!m_adapter || !m_adapter.load()->Go()) { reason = InternalError; break; @@ -3058,11 +3052,6 @@ DebugStopReason DebuggerController::ExecuteAdapterAndWait(const DebugAdapterOper m_adapterStopPending = std::nullopt; } - if ((operation != DebugAdapterPause) && (operation != DebugAdapterQuit) && (operation != DebugAdapterDetach)) - m_adapterMutex.unlock(); - else - m_adapterMutex2.unlock(); - return reason; } @@ -3078,7 +3067,7 @@ Ref DebuggerController::GetAdapterProperty(const std::string& name) return nullptr; } - return m_adapter->GetProperty(name); + return m_adapter.load()->GetProperty(name); } @@ -3094,7 +3083,7 @@ bool DebuggerController::SetAdapterProperty( return false; } - return m_adapter->SetProperty(name, value); + return m_adapter.load()->SetProperty(name, value); } @@ -3350,7 +3339,7 @@ bool DebuggerController::IsTTD() { if(!m_adapter) return false; - return m_adapter->SupportFeature(DebugAdapterSupportTTD); + return m_adapter.load()->SupportFeature(DebugAdapterSupportTTD); } @@ -3366,7 +3355,7 @@ std::vector DebuggerController::GetTTDMemoryAccessForAddress(uin if (m_adapter) { - events = m_adapter->GetTTDMemoryAccessForAddress(startAddress, endAddress, accessType); + events = m_adapter.load()->GetTTDMemoryAccessForAddress(startAddress, endAddress, accessType); } return events; @@ -3384,7 +3373,7 @@ std::vector DebuggerController::GetTTDMemory if (m_adapter) { - events = m_adapter->GetTTDMemoryAccessForPositionRange(startAddress, endAddress, accessType, startTime, endTime); + events = m_adapter.load()->GetTTDMemoryAccessForPositionRange(startAddress, endAddress, accessType, startTime, endTime); } return events; @@ -3400,7 +3389,7 @@ std::vector DebuggerController::GetTTDCallsForSymbols(const std::s return events; } - return m_adapter->GetTTDCallsForSymbols(symbols, startReturnAddress, endReturnAddress); + return m_adapter.load()->GetTTDCallsForSymbols(symbols, startReturnAddress, endReturnAddress); } @@ -3414,7 +3403,7 @@ std::vector DebuggerController::GetTTDEvents(TTDEventType eventType) return events; } - return m_adapter->GetTTDEvents(eventType); + return m_adapter.load()->GetTTDEvents(eventType); } @@ -3428,16 +3417,16 @@ std::vector DebuggerController::GetAllTTDEvents() return events; } - return m_adapter->GetAllTTDEvents(); + return m_adapter.load()->GetAllTTDEvents(); } void DebuggerController::RecordTTDPosition() { - if (!m_adapter || !m_adapter->SupportFeature(DebugAdapterSupportTTD) || m_suppressTTDPositionRecording) + if (!m_adapter || !m_adapter.load()->SupportFeature(DebugAdapterSupportTTD) || m_suppressTTDPositionRecording) return; - TTDPosition position = m_adapter->GetCurrentTTDPosition(); + TTDPosition position = m_adapter.load()->GetCurrentTTDPosition(); if (position.sequence == 0 && position.step == 0) return; @@ -3486,13 +3475,13 @@ bool DebuggerController::TTDNavigateForward() bool DebuggerController::CanTTDNavigateBack() const { - return m_adapter && m_adapter->SupportFeature(DebugAdapterSupportTTD) && m_ttdPositionHistoryIndex > 0; + return m_adapter && m_adapter.load()->SupportFeature(DebugAdapterSupportTTD) && m_ttdPositionHistoryIndex > 0; } bool DebuggerController::CanTTDNavigateForward() const { - return m_adapter && m_adapter->SupportFeature(DebugAdapterSupportTTD) + return m_adapter && m_adapter.load()->SupportFeature(DebugAdapterSupportTTD) && m_ttdPositionHistoryIndex >= 0 && m_ttdPositionHistoryIndex < static_cast(m_ttdPositionHistory.size()) - 1; } @@ -3517,7 +3506,7 @@ TTDPosition DebuggerController::GetCurrentTTDPosition() if (m_adapter) { - position = m_adapter->GetCurrentTTDPosition(); + position = m_adapter.load()->GetCurrentTTDPosition(); } return position; @@ -3533,7 +3522,7 @@ bool DebuggerController::SetTTDPosition(const TTDPosition& position) if (m_adapter) { - return m_adapter->SetTTDPosition(position); + return m_adapter.load()->SetTTDPosition(position); } return false; @@ -3550,7 +3539,7 @@ std::pair DebuggerController::GetTTDNextMemoryAccess(uint6 if (m_adapter) { - return m_adapter->GetTTDNextMemoryAccess(address, size, accessType); + return m_adapter.load()->GetTTDNextMemoryAccess(address, size, accessType); } return {false, TTDMemoryEvent()}; @@ -3567,7 +3556,7 @@ std::pair DebuggerController::GetTTDPrevMemoryAccess(uint6 if (m_adapter) { - return m_adapter->GetTTDPrevMemoryAccess(address, size, accessType); + return m_adapter.load()->GetTTDPrevMemoryAccess(address, size, accessType); } return {false, TTDMemoryEvent()}; @@ -5036,7 +5025,7 @@ Ref DebuggerController::GetAdapterSettings() if (!m_adapter) return nullptr; - return m_adapter->GetAdapterSettings(); + return m_adapter.load()->GetAdapterSettings(); } diff --git a/core/debuggercontroller.h b/core/debuggercontroller.h index 52dc688b..d3620de5 100644 --- a/core/debuggercontroller.h +++ b/core/debuggercontroller.h @@ -18,6 +18,7 @@ limitations under the License. #include "binaryninjaapi.h" #include "debuggerstate.h" #include "debuggerevent.h" +#include #include #include #include @@ -77,7 +78,13 @@ namespace BinaryNinjaDebugger { }; private: - DebugAdapter* m_adapter; + // m_adapter is the active debug adapter for this controller. Written exclusively + // from the worker thread (in CreateDebugAdapter); read both from the worker + // (the bulk of references inside ExecuteAdapterAndWait and adapter ops) and + // from arbitrary caller threads (RequestInterrupt's out-of-band BreakInto). + // Made atomic so cross-thread reads are race-free; the pointed-to adapter + // object's lifetime is guaranteed by the caller holding a DbgRef. + std::atomic m_adapter; DebuggerState* m_state; FileMetadataRef m_file; BinaryViewRef m_data; @@ -99,17 +106,11 @@ namespace BinaryNinjaDebugger { std::mutex m_callbackMutex; std::set m_disabledCallbacks; - // m_adapterMutex is a low-level mutex that protects the adapter access. It cannot be locked recursively. // m_targetControlMutex is a high-level mutex that prevents two threads from controlling the debugger at the - // same time - std::mutex m_adapterMutex; + // same time. With the worker queue serializing adapter operations on a single thread, this is largely + // redundant; kept in place for now to minimize behavioral churn. std::recursive_mutex m_targetControlMutex; - // m_adapterMutex2 is similar to m_adapterMutex, but it is used to protect only the Pause/Quit/Detach operation - // These operations cannot be protected by m_adapterMutex, since if the user resume the target and it remains - // running, we need the ability to pause or kill the target - std::mutex m_adapterMutex2; - uint64_t m_lastIP = 0; uint64_t m_currentIP = 0; @@ -294,8 +295,8 @@ namespace BinaryNinjaDebugger { { if (fut.wait_for(timeout) != std::future_status::ready) { - if (m_adapter) - m_adapter->BreakInto(); + if (DebugAdapter* adapter = m_adapter.load(std::memory_order_acquire)) + adapter->BreakInto(); } } return fut.get(); From ef736af48c3283a977380fc4c7112a955ff5028d Mon Sep 17 00:00:00 2001 From: Xusheng Date: Thu, 21 May 2026 16:41:24 -0400 Subject: [PATCH 7/8] Revert atomic m_adapter (not pulling its weight); keep the snapshot pattern The std::atomic conversion in the previous commit was half-hygiene: it eliminated a torn-read / reorder race on the pointer bits, but the meaningful failure mode (calling into a destroyed adapter object) is not protected by pointer-level atomicity at all. That protection comes from external lifetime guarantees -- the adapter is destroyed only inside ~DebuggerState, which runs inside ~DebuggerController after both worker threads have joined, and cross-thread callers hold a DbgRef on the controller during their call. Given that, the atomic was paying ~50 ".load()" calls' worth of cosmetic noise for a corner-case race (Pause during Launch) that the UI doesn't even expose. Reverts m_adapter back to a plain DebugAdapter* and the ~50 .load() sites back to ordinary arrow access. The mutex removal from the same commit (m_adapterMutex / m_adapterMutex2 -> assert(this_thread == worker)) stays; that one is unambiguously correct. Retains the cross-thread snapshot pattern in RequestInterrupt and SubmitAndWait's timeout-break path -- "if (DebugAdapter* adapter = m_adapter) adapter->BreakInto()" -- so the null check and the call see the same pointer value even without atomicity. Good hygiene regardless. The real structural fix (refcount the DebugAdapter so the type system enforces the lifetime guarantee that DbgRef currently provides by convention) is filed as a follow-up issue. Co-Authored-By: Claude Opus 4.7 (1M context) --- core/debuggercontroller.cpp | 113 ++++++++++++++++++------------------ core/debuggercontroller.h | 17 ++++-- 2 files changed, 68 insertions(+), 62 deletions(-) diff --git a/core/debuggercontroller.cpp b/core/debuggercontroller.cpp index 0dcf051d..02031e76 100644 --- a/core/debuggercontroller.cpp +++ b/core/debuggercontroller.cpp @@ -15,7 +15,6 @@ limitations under the License. */ #include "debuggercontroller.h" -#include #include #include #include @@ -515,7 +514,7 @@ bool DebuggerController::Execute() std::replace(filePath.begin(), filePath.end(), '/', '\\'); #endif - return m_adapter.load()->ExecuteWithArgs( + return m_adapter->ExecuteWithArgs( filePath, m_state->GetCommandLineArguments(), m_state->GetWorkingDirectory(), configs); } @@ -542,7 +541,7 @@ bool DebuggerController::CreateDebugAdapter() return false; } - if (!m_adapter.load()->Init()) + if (!m_adapter->Init()) { LogWarn("Failed to init an adapter of type %s", m_state->GetAdapterType().c_str()); return false; @@ -552,12 +551,12 @@ bool DebuggerController::CreateDebugAdapter() m_state->SetAdapter(m_adapter); // This is a very hacky way to let the adapter have a handle to the controller, and then be able to access the // Binary View object - m_adapter.load()->SetController(this); + m_adapter->SetController(this); ApplyBreakpoints(); // Forward the DebuggerEvent from the adapters to the controller - m_adapter.load()->SetEventCallback([this](const DebuggerEvent& event) { PostDebuggerEvent(event); }); + m_adapter->SetEventCallback([this](const DebuggerEvent& event) { PostDebuggerEvent(event); }); return true; } @@ -1349,7 +1348,7 @@ DebugStopReason DebuggerController::RunToAndWaitInternal(const std::vectorGetBreakpoints()->ContainsAbsolute(remoteAddress)) { - m_adapter.load()->AddBreakpoint(remoteAddress); + m_adapter->AddBreakpoint(remoteAddress); } } @@ -1359,7 +1358,7 @@ DebugStopReason DebuggerController::RunToAndWaitInternal(const std::vectorGetBreakpoints()->ContainsAbsolute(remoteAddress)) { - m_adapter.load()->RemoveBreakpoint(remoteAddress); + m_adapter->RemoveBreakpoint(remoteAddress); } } @@ -1375,7 +1374,7 @@ DebugStopReason DebuggerController::RunToReverseAndWaitInternal(const std::vecto { if (!m_state->GetBreakpoints()->ContainsAbsolute(remoteAddress)) { - m_adapter.load()->AddBreakpoint(remoteAddress); + m_adapter->AddBreakpoint(remoteAddress); } } @@ -1385,7 +1384,7 @@ DebugStopReason DebuggerController::RunToReverseAndWaitInternal(const std::vecto { if (!m_state->GetBreakpoints()->ContainsAbsolute(remoteAddress)) { - m_adapter.load()->RemoveBreakpoint(remoteAddress); + m_adapter->RemoveBreakpoint(remoteAddress); } } @@ -1617,7 +1616,7 @@ bool DebuggerController::ConnectToDebugServer() if (!CreateDebugAdapter()) return false; - bool ok = m_adapter.load()->ConnectToDebugServer(m_state->GetRemoteHost(), m_state->GetRemotePort()); + bool ok = m_adapter->ConnectToDebugServer(m_state->GetRemoteHost(), m_state->GetRemotePort()); if (!ok) LogWarn("Failed to connect to the debug server"); else @@ -1632,7 +1631,7 @@ bool DebuggerController::DisconnectDebugServer() if (!m_state->IsConnectedToDebugServer()) return true; - bool ok = m_adapter.load()->DisconnectDebugServer(); + bool ok = m_adapter->DisconnectDebugServer(); if (!ok) LogWarn("Failed to disconnect from the debug server"); else @@ -1827,7 +1826,7 @@ DebugStopReason DebuggerController::EmulateStepOverAndWait() return InternalError; size_t size = remoteArch->GetMaxInstructionLength(); - DataBuffer buffer = m_adapter.load()->ReadMemory(remoteIP, size); + DataBuffer buffer = m_adapter->ReadMemory(remoteIP, size); size_t bytesRead = buffer.GetLength(); Ref ilFunc = new LowLevelILFunction(remoteArch, nullptr); @@ -1872,7 +1871,7 @@ DebugStopReason DebuggerController::StepOverAndWaitInternal() { m_userRequestedBreak = false; - if (m_adapter && m_adapter.load()->SupportFeature(DebugAdapterSupportStepOver)) + if (m_adapter && m_adapter->SupportFeature(DebugAdapterSupportStepOver)) { return ExecuteAdapterAndWait(DebugAdapterStepOver); } @@ -1887,7 +1886,7 @@ DebugStopReason DebuggerController::StepOverReverseAndWaitInternal() { m_userRequestedBreak = false; - if (m_adapter && m_adapter.load()->SupportFeature(DebugAdapterSupportStepOverReverse)) + if (m_adapter && m_adapter->SupportFeature(DebugAdapterSupportStepOverReverse)) { return ExecuteAdapterAndWait(DebugAdapterStepOverReverse); } @@ -2331,11 +2330,11 @@ void DebuggerController::RequestInterrupt() // Set the flag first so any in-flight op's silent-resume / NotifyStopped checks // observe the break request before the engine reports the resulting stop. m_userRequestedBreak = true; - // Snapshot the adapter pointer with a single atomic load so we don't race between - // the null check and the call. The pointed-to object is kept alive by the caller's - // DbgRef on the controller (the adapter is destroyed only inside ~DebuggerState, - // which runs inside ~DebuggerController after both worker threads have joined). - if (DebugAdapter* adapter = m_adapter.load(std::memory_order_acquire)) + // Snapshot the adapter pointer once so the null check and the call see the same + // value. The pointed-to object is kept alive by the caller's DbgRef on the + // controller (the adapter is destroyed only inside ~DebuggerState, which runs + // inside ~DebuggerController after both worker threads have joined). + if (DebugAdapter* adapter = m_adapter) adapter->BreakInto(); } @@ -2479,7 +2478,7 @@ std::vector DebuggerController::GetProcessList() return {}; } - return m_adapter.load()->GetProcessList(); + return m_adapter->GetProcessList(); } @@ -2535,7 +2534,7 @@ uint32_t DebuggerController::GetActivePID() { if (!m_adapter) return 0; - return m_adapter.load()->GetActivePID(); + return m_adapter->GetActivePID(); } @@ -2543,7 +2542,7 @@ void DebuggerController::WriteStdIn(const std::string message) { if (m_adapter && m_state->IsRunning()) { - m_adapter.load()->WriteStdin(message); + m_adapter->WriteStdin(message); } else { @@ -2568,7 +2567,7 @@ std::string DebuggerController::InvokeBackendCommand(const std::string& cmd) else m_lastCommand = cmdToSend; - return m_adapter.load()->InvokeBackendCommand(cmdToSend); + return m_adapter->InvokeBackendCommand(cmdToSend); } return "Error: invalid adapter\n"; @@ -2922,7 +2921,7 @@ DebugStopReason DebuggerController::StopReason() const if (!m_adapter) return UnknownReason; - return m_adapter.load()->StopReason(); + return m_adapter->StopReason(); } @@ -2932,14 +2931,14 @@ DebugStopReason DebuggerController::ExecuteAdapterAndWait(const DebugAdapterOper // serializes all adapter operations, so the previous m_adapterMutex / m_adapterMutex2 // pair (which guarded against concurrent adapter access from multiple spawned threads) // is no longer needed. The new Pause path bypasses this method entirely and calls - // m_adapter.load()->BreakInto() out-of-band; everything else funnels through here on the worker. + // m_adapter->BreakInto() out-of-band; everything else funnels through here on the worker. assert(std::this_thread::get_id() == m_workerThreadId); // Claim the adapter-stop channel for the duration of this call. Any AdapterStoppedEvent // posted by the adapter from now until we clear m_inAdapterWait is delivered to // WaitForAdapterStop below, not treated as spontaneous. We hold this across the // entire silent-resume loop so that an adapter stop between iterations (after we - // kick off m_adapter.load()->Go() for a false breakpoint condition) is still consumed + // kick off m_adapter->Go() for a false breakpoint condition) is still consumed // by us, not synthesized as a spontaneous stop. { std::lock_guard lk(m_adapterStopMutex); @@ -2954,46 +2953,46 @@ DebugStopReason DebuggerController::ExecuteAdapterAndWait(const DebugAdapterOper switch (operation) { case DebugAdapterGo: - resumeOK = m_adapter.load()->Go(); + resumeOK = m_adapter->Go(); break; case DebugAdapterGoReverse: - resumeOK = m_adapter.load()->GoReverse(); + resumeOK = m_adapter->GoReverse(); break; case DebugAdapterStepInto: - resumeOK = m_adapter.load()->StepInto(); + resumeOK = m_adapter->StepInto(); break; case DebugAdapterStepIntoReverse: - resumeOK = m_adapter.load()->StepIntoReverse(); + resumeOK = m_adapter->StepIntoReverse(); break; case DebugAdapterStepOver: - resumeOK = m_adapter.load()->StepOver(); + resumeOK = m_adapter->StepOver(); break; case DebugAdapterStepOverReverse: - resumeOK = m_adapter.load()->StepOverReverse(); + resumeOK = m_adapter->StepOverReverse(); break; case DebugAdapterStepReturn: - resumeOK = m_adapter.load()->StepReturn(); + resumeOK = m_adapter->StepReturn(); break; case DebugAdapterStepReturnReverse: - resumeOK = m_adapter.load()->StepReturnReverse(); + resumeOK = m_adapter->StepReturnReverse(); break; case DebugAdapterPause: - operationRequested = m_adapter.load()->BreakInto(); + operationRequested = m_adapter->BreakInto(); break; case DebugAdapterQuit: - operationRequested = m_adapter.load()->Quit(); + operationRequested = m_adapter->Quit(); break; case DebugAdapterDetach: - operationRequested = m_adapter.load()->Detach(); + operationRequested = m_adapter->Detach(); break; case DebugAdapterLaunch: resumeOK = Execute(); break; case DebugAdapterAttach: - resumeOK = m_adapter.load()->Attach(m_state->GetPIDAttach()); + resumeOK = m_adapter->Attach(m_state->GetPIDAttach()); break; case DebugAdapterConnect: - resumeOK = m_adapter.load()->Connect(m_state->GetRemoteHost(), m_state->GetRemotePort()); + resumeOK = m_adapter->Connect(m_state->GetRemoteHost(), m_state->GetRemotePort()); break; default: break; @@ -3035,7 +3034,7 @@ DebugStopReason DebuggerController::ExecuteAdapterAndWait(const DebugAdapterOper if (reason == Breakpoint && ShouldSilentResumeAfterStop()) { m_state->SetExecutionStatus(DebugAdapterRunningStatus); - if (!m_adapter || !m_adapter.load()->Go()) + if (!m_adapter || !m_adapter->Go()) { reason = InternalError; break; @@ -3067,7 +3066,7 @@ Ref DebuggerController::GetAdapterProperty(const std::string& name) return nullptr; } - return m_adapter.load()->GetProperty(name); + return m_adapter->GetProperty(name); } @@ -3083,7 +3082,7 @@ bool DebuggerController::SetAdapterProperty( return false; } - return m_adapter.load()->SetProperty(name, value); + return m_adapter->SetProperty(name, value); } @@ -3339,7 +3338,7 @@ bool DebuggerController::IsTTD() { if(!m_adapter) return false; - return m_adapter.load()->SupportFeature(DebugAdapterSupportTTD); + return m_adapter->SupportFeature(DebugAdapterSupportTTD); } @@ -3355,7 +3354,7 @@ std::vector DebuggerController::GetTTDMemoryAccessForAddress(uin if (m_adapter) { - events = m_adapter.load()->GetTTDMemoryAccessForAddress(startAddress, endAddress, accessType); + events = m_adapter->GetTTDMemoryAccessForAddress(startAddress, endAddress, accessType); } return events; @@ -3373,7 +3372,7 @@ std::vector DebuggerController::GetTTDMemory if (m_adapter) { - events = m_adapter.load()->GetTTDMemoryAccessForPositionRange(startAddress, endAddress, accessType, startTime, endTime); + events = m_adapter->GetTTDMemoryAccessForPositionRange(startAddress, endAddress, accessType, startTime, endTime); } return events; @@ -3389,7 +3388,7 @@ std::vector DebuggerController::GetTTDCallsForSymbols(const std::s return events; } - return m_adapter.load()->GetTTDCallsForSymbols(symbols, startReturnAddress, endReturnAddress); + return m_adapter->GetTTDCallsForSymbols(symbols, startReturnAddress, endReturnAddress); } @@ -3403,7 +3402,7 @@ std::vector DebuggerController::GetTTDEvents(TTDEventType eventType) return events; } - return m_adapter.load()->GetTTDEvents(eventType); + return m_adapter->GetTTDEvents(eventType); } @@ -3417,16 +3416,16 @@ std::vector DebuggerController::GetAllTTDEvents() return events; } - return m_adapter.load()->GetAllTTDEvents(); + return m_adapter->GetAllTTDEvents(); } void DebuggerController::RecordTTDPosition() { - if (!m_adapter || !m_adapter.load()->SupportFeature(DebugAdapterSupportTTD) || m_suppressTTDPositionRecording) + if (!m_adapter || !m_adapter->SupportFeature(DebugAdapterSupportTTD) || m_suppressTTDPositionRecording) return; - TTDPosition position = m_adapter.load()->GetCurrentTTDPosition(); + TTDPosition position = m_adapter->GetCurrentTTDPosition(); if (position.sequence == 0 && position.step == 0) return; @@ -3475,13 +3474,13 @@ bool DebuggerController::TTDNavigateForward() bool DebuggerController::CanTTDNavigateBack() const { - return m_adapter && m_adapter.load()->SupportFeature(DebugAdapterSupportTTD) && m_ttdPositionHistoryIndex > 0; + return m_adapter && m_adapter->SupportFeature(DebugAdapterSupportTTD) && m_ttdPositionHistoryIndex > 0; } bool DebuggerController::CanTTDNavigateForward() const { - return m_adapter && m_adapter.load()->SupportFeature(DebugAdapterSupportTTD) + return m_adapter && m_adapter->SupportFeature(DebugAdapterSupportTTD) && m_ttdPositionHistoryIndex >= 0 && m_ttdPositionHistoryIndex < static_cast(m_ttdPositionHistory.size()) - 1; } @@ -3506,7 +3505,7 @@ TTDPosition DebuggerController::GetCurrentTTDPosition() if (m_adapter) { - position = m_adapter.load()->GetCurrentTTDPosition(); + position = m_adapter->GetCurrentTTDPosition(); } return position; @@ -3522,7 +3521,7 @@ bool DebuggerController::SetTTDPosition(const TTDPosition& position) if (m_adapter) { - return m_adapter.load()->SetTTDPosition(position); + return m_adapter->SetTTDPosition(position); } return false; @@ -3539,7 +3538,7 @@ std::pair DebuggerController::GetTTDNextMemoryAccess(uint6 if (m_adapter) { - return m_adapter.load()->GetTTDNextMemoryAccess(address, size, accessType); + return m_adapter->GetTTDNextMemoryAccess(address, size, accessType); } return {false, TTDMemoryEvent()}; @@ -3556,7 +3555,7 @@ std::pair DebuggerController::GetTTDPrevMemoryAccess(uint6 if (m_adapter) { - return m_adapter.load()->GetTTDPrevMemoryAccess(address, size, accessType); + return m_adapter->GetTTDPrevMemoryAccess(address, size, accessType); } return {false, TTDMemoryEvent()}; @@ -5025,7 +5024,7 @@ Ref DebuggerController::GetAdapterSettings() if (!m_adapter) return nullptr; - return m_adapter.load()->GetAdapterSettings(); + return m_adapter->GetAdapterSettings(); } diff --git a/core/debuggercontroller.h b/core/debuggercontroller.h index d3620de5..10100b40 100644 --- a/core/debuggercontroller.h +++ b/core/debuggercontroller.h @@ -18,7 +18,6 @@ limitations under the License. #include "binaryninjaapi.h" #include "debuggerstate.h" #include "debuggerevent.h" -#include #include #include #include @@ -82,9 +81,14 @@ namespace BinaryNinjaDebugger { // from the worker thread (in CreateDebugAdapter); read both from the worker // (the bulk of references inside ExecuteAdapterAndWait and adapter ops) and // from arbitrary caller threads (RequestInterrupt's out-of-band BreakInto). - // Made atomic so cross-thread reads are race-free; the pointed-to adapter - // object's lifetime is guaranteed by the caller holding a DbgRef. - std::atomic m_adapter; + // + // The pointed-to adapter object's lifetime is guaranteed externally: it is + // destroyed only inside ~DebuggerState, which runs inside ~DebuggerController + // after both worker threads have joined. Cross-thread callers hold a DbgRef + // on the controller during their call, so the adapter cannot be destroyed + // out from under them. See the "Refcount the DebugAdapter" follow-up issue + // for the structural fix that would make this guarantee enforced by the type. + DebugAdapter* m_adapter; DebuggerState* m_state; FileMetadataRef m_file; BinaryViewRef m_data; @@ -295,7 +299,10 @@ namespace BinaryNinjaDebugger { { if (fut.wait_for(timeout) != std::future_status::ready) { - if (DebugAdapter* adapter = m_adapter.load(std::memory_order_acquire)) + // Snapshot the pointer once so the null check and the call see the + // same value. Lifetime of the pointee is guaranteed by the caller's + // DbgRef on the controller. + if (DebugAdapter* adapter = m_adapter) adapter->BreakInto(); } } From 13d80849f59ca8777ec84a00447b4fd520736388 Mon Sep 17 00:00:00 2001 From: Xusheng Date: Mon, 25 May 2026 11:03:37 -0400 Subject: [PATCH 8/8] Notify the stop even when the break was user-requested When the user interrupts a freely-running target (Pause, or the interrupt that Restart/Quit/Detach issue), the target stops but the controller state was never updated -- the UI kept showing "running" even though the process had halted. Cause: the worker op (GoAndWaitOnWorker etc.) guarded its NotifyStopped call with !m_userRequestedBreak. That guard is a leftover from the pre-refactor model, where the Pause path itself ran ExecuteAdapterAndWait(Pause) and called NotifyStopped, so the resume op deliberately deferred to it to avoid a double notify. In the new model Pause is out-of-band (RequestInterrupt just sets the flag and calls BreakInto) and does NOT notify, so the worker op is the only place left that can report the stop -- but it was skipping it precisely when a break had been requested. Drop the !m_userRequestedBreak condition from the 14 NotifyStopped sites so the worker op always reports a genuine stop. m_userRequestedBreak is still used by ShouldSilentResumeAfterStop to suppress conditional-breakpoint auto-resume when the user explicitly broke. This also restores the pre-refactor behavior for Quit/Restart-while-running, where the old QuitAndWait called PauseAndWait (which notified an intermediate stop) before quitting. Co-Authored-By: Claude Opus 4.7 (1M context) --- core/debuggercontroller.cpp | 34 +++++++++++++++++++--------------- 1 file changed, 19 insertions(+), 15 deletions(-) diff --git a/core/debuggercontroller.cpp b/core/debuggercontroller.cpp index 02031e76..bd39e7ae 100644 --- a/core/debuggercontroller.cpp +++ b/core/debuggercontroller.cpp @@ -373,7 +373,7 @@ DebugStopReason DebuggerController::LaunchAndWaitOnWorker() return InternalError; auto reason = LaunchAndWaitInternal(); - if (!m_userRequestedBreak && (reason != ProcessExited) && (reason != InternalError)) + if ((reason != ProcessExited) && (reason != InternalError)) NotifyStopped(reason); m_targetControlMutex.unlock(); @@ -430,7 +430,7 @@ DebugStopReason DebuggerController::AttachAndWaitOnWorker() return InternalError; auto reason = AttachAndWaitInternal(); - if (!m_userRequestedBreak && (reason != ProcessExited) && (reason != InternalError)) + if ((reason != ProcessExited) && (reason != InternalError)) NotifyStopped(reason); m_targetControlMutex.unlock(); @@ -487,7 +487,7 @@ DebugStopReason DebuggerController::ConnectAndWaitOnWorker() return InternalError; auto reason = ConnectAndWaitInternal(); - if (!m_userRequestedBreak && (reason != ProcessExited) && (reason != InternalError)) + if ((reason != ProcessExited) && (reason != InternalError)) NotifyStopped(reason); m_targetControlMutex.unlock(); @@ -622,7 +622,7 @@ DebugStopReason DebuggerController::GoAndWaitOnWorker() return InternalError; auto reason = GoAndWaitInternal(); - if (!m_userRequestedBreak && (reason != ProcessExited) && (reason != InternalError)) + if ((reason != ProcessExited) && (reason != InternalError)) NotifyStopped(reason); m_targetControlMutex.unlock(); @@ -646,7 +646,7 @@ DebugStopReason DebuggerController::GoReverseAndWaitOnWorker() return InternalError; auto reason = GoReverseAndWaitInternal(); - if (!m_userRequestedBreak && (reason != ProcessExited) && (reason != InternalError)) + if ((reason != ProcessExited) && (reason != InternalError)) NotifyStopped(reason); m_targetControlMutex.unlock(); @@ -907,7 +907,7 @@ DebugStopReason DebuggerController::StepIntoReverseAndWaitOnWorker(BNFunctionGra return InternalError; auto reason = StepIntoReverseIL(il); - if (!m_userRequestedBreak && (reason != ProcessExited) && (reason != InternalError)) + if ((reason != ProcessExited) && (reason != InternalError)) NotifyStopped(reason); m_targetControlMutex.unlock(); @@ -930,7 +930,7 @@ DebugStopReason DebuggerController::StepIntoAndWaitOnWorker(BNFunctionGraphType return InternalError; auto reason = StepIntoIL(il); - if (!m_userRequestedBreak && (reason != ProcessExited) && (reason != InternalError)) + if ((reason != ProcessExited) && (reason != InternalError)) NotifyStopped(reason); m_targetControlMutex.unlock(); @@ -1182,7 +1182,7 @@ DebugStopReason DebuggerController::StepOverAndWaitOnWorker(BNFunctionGraphType return InternalError; auto reason = StepOverIL(il); - if (!m_userRequestedBreak && (reason != ProcessExited) && (reason != InternalError)) + if ((reason != ProcessExited) && (reason != InternalError)) NotifyStopped(reason); m_targetControlMutex.unlock(); @@ -1207,7 +1207,7 @@ DebugStopReason DebuggerController::StepOverReverseAndWaitOnWorker(BNFunctionGra return InternalError; auto reason = StepOverReverseIL(il); - if (!m_userRequestedBreak && (reason != ProcessExited) && (reason != InternalError)) + if ((reason != ProcessExited) && (reason != InternalError)) NotifyStopped(reason); m_targetControlMutex.unlock(); @@ -1302,7 +1302,7 @@ DebugStopReason DebuggerController::StepReturnAndWaitOnWorker() return InternalError; auto reason = StepReturnAndWaitInternal(); - if (!m_userRequestedBreak && (reason != ProcessExited) && (reason != InternalError)) + if ((reason != ProcessExited) && (reason != InternalError)) NotifyStopped(reason); m_targetControlMutex.unlock(); @@ -1326,7 +1326,7 @@ DebugStopReason DebuggerController::StepReturnReverseAndWaitOnWorker() return InternalError; auto reason = StepReturnReverseAndWaitInternal(); - if (!m_userRequestedBreak && (reason != ProcessExited) && (reason != InternalError)) + if ((reason != ProcessExited) && (reason != InternalError)) NotifyStopped(reason); m_targetControlMutex.unlock(); @@ -1426,7 +1426,7 @@ DebugStopReason DebuggerController::RunToAndWaitOnWorker(const std::vector