From 5449e2185894590d9bbd0c9e199f894d84c94a6b Mon Sep 17 00:00:00 2001 From: Xusheng Date: Thu, 9 Mar 2023 18:23:14 +0800 Subject: [PATCH] Clean up DebugAdapter interface. --- core/adapters/dbgengadapter.cpp | 54 ++++++++++++++++++--------------- core/adapters/dbgengadapter.h | 14 ++++----- core/adapters/lldbadapter.cpp | 46 ++++++++++++++-------------- core/adapters/lldbadapter.h | 14 ++++----- core/debugadapter.cpp | 4 +-- core/debugadapter.h | 19 +++++------- core/debuggercontroller.cpp | 14 ++++----- 7 files changed, 83 insertions(+), 82 deletions(-) diff --git a/core/adapters/dbgengadapter.cpp b/core/adapters/dbgengadapter.cpp index b2aefa1f..635e3c3c 100644 --- a/core/adapters/dbgengadapter.cpp +++ b/core/adapters/dbgengadapter.cpp @@ -559,26 +559,32 @@ bool DbgEngAdapter::DisconnectDebugServer() return ret == S_OK; } -void DbgEngAdapter::Detach() +bool DbgEngAdapter::Detach() { m_aboutToBeKilled = true; m_lastOperationIsStepInto = false; - if (this->m_debugClient) - this->m_debugClient->DetachProcesses(); + if (!this->m_debugClient) + return false; + + if (this->m_debugClient->DetachProcesses() != S_OK) + return false; m_debugClient->ExitDispatch(reinterpret_cast(m_debugClient)); + return true; } -void DbgEngAdapter::Quit() +bool DbgEngAdapter::Quit() { m_aboutToBeKilled = true; m_lastOperationIsStepInto = false; - if (this->m_debugClient) - { - HRESULT result = this->m_debugClient->TerminateProcesses(); - } + if (!this->m_debugClient) + return false; + + if (this->m_debugClient->TerminateProcesses() != S_OK) + return false; m_debugClient->ExitDispatch(reinterpret_cast(m_debugClient)); + return true; } std::vector DbgEngAdapter::GetProcessList() @@ -995,7 +1001,7 @@ std::vector DbgEngAdapter::GetModuleList() bool DbgEngAdapter::BreakInto() { if (ExecStatus() == DEBUG_STATUS_BREAK) - return InvalidStatusOrOperation; + return false; m_lastOperationIsStepInto = false; // After we call SetInterrupt(), the WaitForEvent() function will return due to a breakpoint exception @@ -1005,54 +1011,54 @@ bool DbgEngAdapter::BreakInto() return true; } -DebugStopReason DbgEngAdapter::Go() +bool DbgEngAdapter::Go() { // TODO: we should have the debugger core to detect the failure and notify the user about it. // Currently, LLDB directly notifies such errors, which needs to be changed in the future. if (ExecStatus() != DEBUG_STATUS_BREAK) - return InvalidStatusOrOperation; + return false; m_lastOperationIsStepInto = false; if (this->m_debugControl->SetExecutionStatus(DEBUG_STATUS_GO) != S_OK) - return DebugStopReason::InternalError; + return false; m_debugClient->ExitDispatch(reinterpret_cast(m_debugClient)); - return UnknownReason; + return true; } -DebugStopReason DbgEngAdapter::StepInto() +bool DbgEngAdapter::StepInto() { if (ExecStatus() != DEBUG_STATUS_BREAK) return InvalidStatusOrOperation; m_lastOperationIsStepInto = true; if (this->m_debugControl->SetExecutionStatus(DEBUG_STATUS_STEP_INTO) != S_OK) - return DebugStopReason::InternalError; + return false; m_debugClient->ExitDispatch(reinterpret_cast(m_debugClient)); - return UnknownReason; + return true; } -DebugStopReason DbgEngAdapter::StepOver() +bool DbgEngAdapter::StepOver() { if (ExecStatus() != DEBUG_STATUS_BREAK) - return InvalidStatusOrOperation; + return false; m_lastOperationIsStepInto = false; if (this->m_debugControl->SetExecutionStatus(DEBUG_STATUS_STEP_OVER) != S_OK) - return DebugStopReason::InternalError; + return false; m_debugClient->ExitDispatch(reinterpret_cast(m_debugClient)); - return UnknownReason; + return true; } -DebugStopReason DbgEngAdapter::StepReturn() +bool DbgEngAdapter::StepReturn() { if (ExecStatus() != DEBUG_STATUS_BREAK) - return InvalidStatusOrOperation; + return false; InvokeBackendCommand("gu"); - return UnknownReason; + return true; } bool DbgEngAdapter::Wait(std::chrono::milliseconds timeout) @@ -1165,7 +1171,7 @@ std::string DbgEngAdapter::InvokeBackendCommand(const std::string& command) return ""; } -std::uintptr_t DbgEngAdapter::GetInstructionOffset() +uint64_t DbgEngAdapter::GetInstructionOffset() { if (!m_debugRegisters) return -1; diff --git a/core/adapters/dbgengadapter.h b/core/adapters/dbgengadapter.h index 40caa9f9..450b70c3 100644 --- a/core/adapters/dbgengadapter.h +++ b/core/adapters/dbgengadapter.h @@ -155,8 +155,8 @@ namespace BinaryNinjaDebugger { bool ConnectToDebugServer(const std::string& server, std::uint32_t port) override; bool DisconnectDebugServer() override; - void Detach() override; - void Quit() override; + bool Detach() override; + bool Quit() override; bool Wait(std::chrono::milliseconds timeout = std::chrono::milliseconds::max()); @@ -198,13 +198,13 @@ namespace BinaryNinjaDebugger { uint64_t ExitCode() override; bool BreakInto() override; - DebugStopReason Go() override; - DebugStopReason StepInto() override; - DebugStopReason StepOver() override; - DebugStopReason StepReturn() override; + bool Go() override; + bool StepInto() override; + bool StepOver() override; + bool StepReturn() override; std::string InvokeBackendCommand(const std::string& command) override; - std::uintptr_t GetInstructionOffset() override; + uint64_t GetInstructionOffset() override; uint64_t GetStackPointer() override; bool SupportFeature(DebugAdapterCapacity feature) override; diff --git a/core/adapters/lldbadapter.cpp b/core/adapters/lldbadapter.cpp index f57ffd27..03906319 100644 --- a/core/adapters/lldbadapter.cpp +++ b/core/adapters/lldbadapter.cpp @@ -365,19 +365,19 @@ bool LldbAdapter::Connect(const std::string& server, std::uint32_t port) } -void LldbAdapter::Detach() +bool LldbAdapter::Detach() { std::unique_lock lock(m_quitingMutex); - // TODO: return if the operation succeeds SBError error = m_process.Detach(); + return error.Success(); } -void LldbAdapter::Quit() +bool LldbAdapter::Quit() { std::unique_lock lock(m_quitingMutex); - // TODO: return if the operation succeeds SBError error = m_process.Kill(); + return error.Success(); } @@ -1075,7 +1075,7 @@ bool LldbAdapter::BreakInto() } -DebugStopReason LldbAdapter::Go() +bool LldbAdapter::Go() { if (m_process.GetState() != lldb::eStateStopped) { @@ -1084,21 +1084,21 @@ DebugStopReason LldbAdapter::Go() event.data.errorData.shortError = "Go failed"; event.data.errorData.error = fmt::format("LLDB: go failed, process state is not stopped"); PostDebuggerEvent(event); - return DebugStopReason::InternalError; + return false; } #ifndef WIN32 SBError error = m_process.Continue(); if (!error.Success()) - return DebugStopReason::InternalError; + return false; #else InvokeBackendCommand("c"); #endif - return StopReason(); + return true; } -DebugStopReason LldbAdapter::StepInto() +bool LldbAdapter::StepInto() { if (m_process.GetState() != lldb::eStateStopped) { @@ -1107,7 +1107,7 @@ DebugStopReason LldbAdapter::StepInto() event.data.errorData.shortError = "step into failed"; event.data.errorData.error = fmt::format("LLDB: step into failed, process state is not stopped"); PostDebuggerEvent(event); - return DebugStopReason::InternalError; + return false; } #ifndef WIN32 @@ -1119,7 +1119,7 @@ DebugStopReason LldbAdapter::StepInto() event.data.errorData.shortError = "Step into failed"; event.data.errorData.error = fmt::format("LLDB: step into failed, invalid thread"); PostDebuggerEvent(event); - return DebugStopReason::InternalError; + return false; } SBError error; @@ -1132,16 +1132,16 @@ DebugStopReason LldbAdapter::StepInto() event.data.errorData.error = fmt::format("LLDB: step into failed {}", error.GetCString() ? error.GetCString() : ""); PostDebuggerEvent(event); - return DebugStopReason::InternalError; + return false; } #else InvokeBackendCommand("si"); #endif - return StopReason(); + return true; } -DebugStopReason LldbAdapter::StepOver() +bool LldbAdapter::StepOver() { if (m_process.GetState() != lldb::eStateStopped) { @@ -1150,7 +1150,7 @@ DebugStopReason LldbAdapter::StepOver() event.data.errorData.shortError = "Step over failed"; event.data.errorData.error = fmt::format("LLDB: step over failed, process state is not stopped"); PostDebuggerEvent(event); - return DebugStopReason::InternalError; + return false; } #ifndef WIN32 @@ -1162,7 +1162,7 @@ DebugStopReason LldbAdapter::StepOver() event.data.errorData.shortError = "Step over failed"; event.data.errorData.error = fmt::format("LLDB: step over failed, invalid thread"); PostDebuggerEvent(event); - return DebugStopReason::InternalError; + return false; } SBError error; @@ -1175,16 +1175,16 @@ DebugStopReason LldbAdapter::StepOver() event.data.errorData.error = fmt::format("LLDB: step over failed {}", error.GetCString() ? error.GetCString() : ""); PostDebuggerEvent(event); - return DebugStopReason::InternalError; + return false; } #else InvokeBackendCommand("ni"); #endif - return StopReason(); + return true; } -DebugStopReason LldbAdapter::StepReturn() +bool LldbAdapter::StepReturn() { if (m_process.GetState() != lldb::eStateStopped) { @@ -1193,7 +1193,7 @@ DebugStopReason LldbAdapter::StepReturn() event.data.errorData.shortError = "Step return failed"; event.data.errorData.error = fmt::format("LLDB: step return failed, process state is not stopped"); PostDebuggerEvent(event); - return DebugStopReason::InternalError; + return false; } // The following method, calling StepOutOfFrame(), will receive an unexpected lldb::eStateRunning event when the @@ -1223,11 +1223,11 @@ DebugStopReason LldbAdapter::StepReturn() event.data.errorData.shortError = "Step return failed"; event.data.errorData.error = fmt::format("LLDB: step return failed, {}", result); PostDebuggerEvent(event); - return DebugStopReason::InternalError; + return false; } //#endif - return StopReason(); + return true; } @@ -1251,7 +1251,7 @@ std::string LldbAdapter::InvokeBackendCommand(const std::string& command) } -uintptr_t LldbAdapter::GetInstructionOffset() +uint64_t LldbAdapter::GetInstructionOffset() { SBThread thread = m_process.GetSelectedThread(); if (!thread.IsValid()) diff --git a/core/adapters/lldbadapter.h b/core/adapters/lldbadapter.h index 34a7bc3f..af9b861e 100644 --- a/core/adapters/lldbadapter.h +++ b/core/adapters/lldbadapter.h @@ -58,9 +58,9 @@ namespace BinaryNinjaDebugger { bool Connect(const std::string& server, std::uint32_t port) override; - void Detach() override; + bool Detach() override; - void Quit() override; + bool Quit() override; std::vector GetProcessList() override; @@ -110,17 +110,17 @@ namespace BinaryNinjaDebugger { bool BreakInto() override; - DebugStopReason Go() override; + bool Go() override; - DebugStopReason StepInto() override; + bool StepInto() override; - DebugStopReason StepOver() override; + bool StepOver() override; - DebugStopReason StepReturn() override; + bool StepReturn() override; std::string InvokeBackendCommand(const std::string& command) override; - uintptr_t GetInstructionOffset() override; + uint64_t GetInstructionOffset() override; uint64_t GetStackPointer() override; diff --git a/core/debugadapter.cpp b/core/debugadapter.cpp index 716e27c2..8e4e2285 100644 --- a/core/debugadapter.cpp +++ b/core/debugadapter.cpp @@ -89,9 +89,9 @@ bool DebugModule::IsSameBaseModule(const std::string& module1, const std::string } -DebugStopReason DebugAdapter::StepReturn() +bool DebugAdapter::StepReturn() { - return OperationNotSupported; + return false; } diff --git a/core/debugadapter.h b/core/debugadapter.h index 5e487916..dacd737f 100644 --- a/core/debugadapter.h +++ b/core/debugadapter.h @@ -219,9 +219,9 @@ namespace BinaryNinjaDebugger { virtual bool DisconnectDebugServer(); - virtual void Detach() = 0; + virtual bool Detach() = 0; - virtual void Quit() = 0; + virtual bool Quit() = 0; virtual std::vector GetProcessList() = 0; @@ -243,10 +243,7 @@ namespace BinaryNinjaDebugger { virtual DebugBreakpoint AddBreakpoint(const std::uintptr_t address, unsigned long breakpoint_type = 0) = 0; - virtual DebugBreakpoint AddBreakpoint(const ModuleNameAndOffset& address, unsigned long breakpoint_type = 0) - { - return DebugBreakpoint {}; - } + virtual DebugBreakpoint AddBreakpoint(const ModuleNameAndOffset& address, unsigned long breakpoint_type = 0) = 0; virtual bool RemoveBreakpoint(const DebugBreakpoint& breakpoint) = 0; @@ -274,18 +271,18 @@ namespace BinaryNinjaDebugger { virtual bool BreakInto() = 0; - virtual DebugStopReason Go() = 0; + virtual bool Go() = 0; - virtual DebugStopReason StepInto() = 0; + virtual bool StepInto() = 0; - virtual DebugStopReason StepOver() = 0; + virtual bool StepOver() = 0; // virtual bool RunTo(std::uintptr_t address) = 0; - virtual DebugStopReason StepReturn(); + virtual bool StepReturn(); virtual std::string InvokeBackendCommand(const std::string& command) = 0; - virtual std::uintptr_t GetInstructionOffset() = 0; + virtual uint64_t GetInstructionOffset() = 0; virtual uint64_t GetStackPointer(); diff --git a/core/debuggercontroller.cpp b/core/debuggercontroller.cpp index 936b7b95..7a1da584 100644 --- a/core/debuggercontroller.cpp +++ b/core/debuggercontroller.cpp @@ -1680,22 +1680,21 @@ DebugStopReason DebuggerController::ExecuteAdapterAndWait(const DebugAdapterOper }, "WaitForAdapterStop"); - // TODO: the operations like Go() should return a bool to indicate whether it succeeds - DebugStopReason resumeReason; + bool resumeOK = false; bool operationRequested = false; switch (operation) { case DebugAdapterGo: - resumeReason = m_adapter->Go(); + resumeOK = m_adapter->Go(); break; case DebugAdapterStepInto: - resumeReason = m_adapter->StepInto(); + resumeOK = m_adapter->StepInto(); break; case DebugAdapterStepOver: - resumeReason = m_adapter->StepOver(); + resumeOK = m_adapter->StepOver(); break; case DebugAdapterStepReturn: - resumeReason = m_adapter->StepReturn(); + resumeOK = m_adapter->StepReturn(); break; case DebugAdapterPause: operationRequested = m_adapter->BreakInto(); @@ -1716,8 +1715,7 @@ DebugStopReason DebuggerController::ExecuteAdapterAndWait(const DebugAdapterOper if ((operation == DebugAdapterGo) || (operation == DebugAdapterStepInto) || (operation == DebugAdapterStepOver) || (operation == DebugAdapterStepReturn)) { - if (resumeReason != InternalError) - ok = true; + ok = resumeOK; } else if (operation == DebugAdapterPause) {