diff --git a/core/debuggercontroller.cpp b/core/debuggercontroller.cpp index 1dd3be70..397bb73a 100644 --- a/core/debuggercontroller.cpp +++ b/core/debuggercontroller.cpp @@ -39,11 +39,24 @@ 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(); + // Wake any in-flight WaitForAdapterStop so the worker can observe shutdown. + m_adapterStopCv.notify_all(); + if (m_workerThread.joinable()) + m_workerThread.join(); + m_shouldExit = true; m_cv.notify_all(); if (m_debuggerEventThread.joinable()) @@ -60,6 +73,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,7 +324,7 @@ bool DebuggerController::Launch() if (!CanStartDebgging()) return false; - std::thread([&]() { LaunchAndWait(); }).detach(); + Submit([this] { LaunchAndWaitOnWorker(); }); return true; } @@ -329,7 +363,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()) @@ -347,13 +381,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; - std::thread([&]() { AttachAndWait(); }).detach(); + Submit([this] { AttachAndWaitOnWorker(); }); return true; } @@ -380,7 +420,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()) @@ -398,13 +438,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; - std::thread([&]() { ConnectAndWait(); }).detach(); + Submit([this] { ConnectAndWaitOnWorker(); }); return true; } @@ -431,7 +477,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()) @@ -449,6 +495,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); @@ -543,7 +595,7 @@ bool DebuggerController::Go() if (!CanResumeTarget()) return false; - std::thread([&]() { GoAndWait(); }).detach(); + Submit([this] { GoAndWaitOnWorker(); }); return true; } @@ -554,13 +606,13 @@ bool DebuggerController::GoReverse() if (!CanResumeTarget()) return false; - std::thread([&]() { 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()) @@ -577,7 +629,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()) @@ -595,6 +654,12 @@ DebugStopReason DebuggerController::GoReverseAndWait() } +DebugStopReason DebuggerController::GoReverseAndWait(std::chrono::milliseconds timeout) +{ + return SubmitAndWait([this] { return GoReverseAndWaitOnWorker(); }, timeout); +} + + DebugStopReason DebuggerController::StepIntoIL(BNFunctionGraphType il) { switch (il) @@ -817,7 +882,7 @@ bool DebuggerController::StepInto(BNFunctionGraphType il) if (!CanResumeTarget()) return false; - std::thread([&, il]() { StepIntoAndWait(il); }).detach(); + Submit([this, il] { StepIntoAndWaitOnWorker(il); }); return true; } @@ -827,12 +892,12 @@ bool DebuggerController::StepIntoReverse(BNFunctionGraphType il) if (!CanResumeTarget()) return false; - std::thread([&, il]() { 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()) @@ -849,7 +914,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()) @@ -866,6 +937,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) @@ -1078,7 +1155,7 @@ bool DebuggerController::StepOver(BNFunctionGraphType il) if (!CanResumeTarget()) return false; - std::thread([&, il]() { StepOverAndWait(il); }).detach(); + Submit([this, il] { StepOverAndWaitOnWorker(il); }); return true; } @@ -1089,13 +1166,13 @@ bool DebuggerController::StepOverReverse(BNFunctionGraphType il) if (!CanResumeTarget()) return false; - std::thread([&, il]() { 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()) @@ -1113,7 +1190,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()) @@ -1131,6 +1215,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(); @@ -1184,7 +1275,7 @@ bool DebuggerController::StepReturn() if (!CanResumeTarget()) return false; - std::thread([&]() { StepReturnAndWait(); }).detach(); + Submit([this] { StepReturnAndWaitOnWorker(); }); return true; } @@ -1195,13 +1286,13 @@ bool DebuggerController::StepReturnReverse() if (!CanResumeTarget()) return false; - std::thread([&]() { 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()) @@ -1219,7 +1310,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()) @@ -1237,6 +1334,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; @@ -1295,7 +1398,7 @@ bool DebuggerController::RunTo(const std::vector& remoteAddresses) if (!CanResumeTarget()) return false; - std::thread([&, remoteAddresses]() { RunToAndWait(remoteAddresses); }).detach(); + Submit([this, remoteAddresses] { RunToAndWaitOnWorker(remoteAddresses); }); return true; } @@ -1307,13 +1410,13 @@ bool DebuggerController::RunToReverse(const std::vector& remoteAddress if (!CanResumeTarget()) return false; - std::thread([&, remoteAddresses]() { 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()) @@ -1331,7 +1434,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()) @@ -1349,6 +1460,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(); @@ -1457,18 +1576,26 @@ bool DebuggerController::Restart() if (!m_state->IsConnected()) return false; - std::thread([&]() { 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); } @@ -1517,11 +1644,11 @@ void DebuggerController::Detach() if (!m_state->IsConnected()) return; - std::thread([&]() { DetachAndWait(); }).detach(); + Submit([this] { DetachAndWaitOnWorker(); }); } -void DebuggerController::DetachAndWait() +void DebuggerController::DetachAndWaitOnWorker() { bool locked = false; if (m_targetControlMutex.try_lock()) @@ -1545,16 +1672,22 @@ void DebuggerController::DetachAndWait() } +void DebuggerController::DetachAndWait(std::chrono::milliseconds timeout) +{ + SubmitAndWait([this] { DetachAndWaitOnWorker(); }, timeout); +} + + void DebuggerController::Quit() { if (!m_state->IsConnected()) return; - std::thread([&]() { QuitAndWait(); }).detach(); + Submit([this] { QuitAndWaitOnWorker(); }); } -void DebuggerController::QuitAndWait() +void DebuggerController::QuitAndWaitOnWorker() { bool locked = false; if (m_targetControlMutex.try_lock()) @@ -1569,7 +1702,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(); } @@ -1584,12 +1721,25 @@ void DebuggerController::QuitAndWait() } +void DebuggerController::QuitAndWait(std::chrono::milliseconds timeout) +{ + SubmitAndWait([this] { QuitAndWaitOnWorker(); }, timeout); +} + + bool DebuggerController::Pause() { if (!m_state->IsConnected()) return false; - std::thread([&]() { 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; } @@ -1602,15 +1752,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; } @@ -1996,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; @@ -2026,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; @@ -2049,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) @@ -2110,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(); } @@ -2694,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; @@ -2789,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 f298d902..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; @@ -168,6 +179,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 +233,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 +441,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; }