From 99788e6b32863734413fe1f5d99f8907c7840f41 Mon Sep 17 00:00:00 2001 From: Itamar Date: Fri, 21 Aug 2020 12:27:15 +0300 Subject: [PATCH] HackStudio: Implement "Step Out" debugging action The "Step Out" action continues execution until the current function returns. Also, LibDebug/StackFrameUtils was introduced to eliminate the duplication of stack frame inspection logic between the "Step Out" action and the BacktraceModel. --- ...ug-single-step.png => debug-step-over.png} | Bin .../HackStudio/Debugger/BacktraceModel.cpp | 7 +- .../HackStudio/Debugger/DebugInfoWidget.cpp | 16 +++-- DevTools/HackStudio/Debugger/Debugger.cpp | 61 +++++++++++++++--- DevTools/HackStudio/Debugger/Debugger.h | 20 +++++- Libraries/LibDebug/CMakeLists.txt | 1 + Libraries/LibDebug/StackFrameUtils.cpp | 40 ++++++++++++ Libraries/LibDebug/StackFrameUtils.h | 42 ++++++++++++ 8 files changed, 171 insertions(+), 16 deletions(-) rename Base/res/icons/16x16/{debug-single-step.png => debug-step-over.png} (100%) create mode 100644 Libraries/LibDebug/StackFrameUtils.cpp create mode 100644 Libraries/LibDebug/StackFrameUtils.h diff --git a/Base/res/icons/16x16/debug-single-step.png b/Base/res/icons/16x16/debug-step-over.png similarity index 100% rename from Base/res/icons/16x16/debug-single-step.png rename to Base/res/icons/16x16/debug-step-over.png diff --git a/DevTools/HackStudio/Debugger/BacktraceModel.cpp b/DevTools/HackStudio/Debugger/BacktraceModel.cpp index 281b621ffbc..500b1a07471 100644 --- a/DevTools/HackStudio/Debugger/BacktraceModel.cpp +++ b/DevTools/HackStudio/Debugger/BacktraceModel.cpp @@ -26,6 +26,7 @@ #include "BacktraceModel.h" #include "Debugger.h" +#include namespace HackStudio { @@ -63,8 +64,10 @@ Vector BacktraceModel::create_backtrace(const DebugSe } frames.append({ name, current_instruction, current_ebp }); - current_instruction = Debugger::the().session()->peek(reinterpret_cast(current_ebp + 4)).value(); - current_ebp = Debugger::the().session()->peek(reinterpret_cast(current_ebp)).value(); + auto frame_info = StackFrameUtils::get_info(*Debugger::the().session(), current_ebp); + ASSERT(frame_info.has_value()); + current_instruction = frame_info.value().return_address; + current_ebp = frame_info.value().next_ebp; } while (current_ebp && current_instruction); return frames; } diff --git a/DevTools/HackStudio/Debugger/DebugInfoWidget.cpp b/DevTools/HackStudio/Debugger/DebugInfoWidget.cpp index ee1a2cdfc9b..c3d7ebda5ca 100644 --- a/DevTools/HackStudio/Debugger/DebugInfoWidget.cpp +++ b/DevTools/HackStudio/Debugger/DebugInfoWidget.cpp @@ -50,17 +50,25 @@ void DebugInfoWidget::init_toolbar() pthread_mutex_unlock(Debugger::the().continue_mutex()); }); - m_singlestep_action = GUI::Action::create("Single Step", Gfx::Bitmap::load_from_file("/res/icons/16x16/debug-single-step.png"), [&](auto&) { + m_singlestep_action = GUI::Action::create("Step Over", Gfx::Bitmap::load_from_file("/res/icons/16x16/debug-step-over.png"), [&](auto&) { + pthread_mutex_lock(Debugger::the().continue_mutex()); + Debugger::the().set_continue_type(Debugger::ContinueType::SourceStepOver); + pthread_cond_signal(Debugger::the().continue_cond()); + pthread_mutex_unlock(Debugger::the().continue_mutex()); + }); + + m_step_in_action = GUI::Action::create("Step In", Gfx::Bitmap::load_from_file("/res/icons/16x16/debug-step-in.png"), [&](auto&) { pthread_mutex_lock(Debugger::the().continue_mutex()); Debugger::the().set_continue_type(Debugger::ContinueType::SourceSingleStep); pthread_cond_signal(Debugger::the().continue_cond()); pthread_mutex_unlock(Debugger::the().continue_mutex()); }); - m_step_in_action = GUI::Action::create("Step In", Gfx::Bitmap::load_from_file("/res/icons/16x16/debug-step-in.png"), [&](auto&) { - }); - m_step_out_action = GUI::Action::create("Step Out", Gfx::Bitmap::load_from_file("/res/icons/16x16/debug-step-out.png"), [&](auto&) { + pthread_mutex_lock(Debugger::the().continue_mutex()); + Debugger::the().set_continue_type(Debugger::ContinueType::SourceStepOut); + pthread_cond_signal(Debugger::the().continue_cond()); + pthread_mutex_unlock(Debugger::the().continue_mutex()); }); m_toolbar->add_action(*m_continue_action); diff --git a/DevTools/HackStudio/Debugger/Debugger.cpp b/DevTools/HackStudio/Debugger/Debugger.cpp index 6e65a794d7e..0438f3cc417 100644 --- a/DevTools/HackStudio/Debugger/Debugger.cpp +++ b/DevTools/HackStudio/Debugger/Debugger.cpp @@ -25,6 +25,7 @@ */ #include "Debugger.h" +#include namespace HackStudio { @@ -127,7 +128,7 @@ void Debugger::start() int Debugger::debugger_loop() { - DebuggingState state; + ASSERT(m_debug_session); m_debug_session->run([&](DebugSession::DebugBreakReason reason, Optional optional_regs) { if (reason == DebugSession::DebugBreakReason::Exited) { @@ -135,14 +136,15 @@ int Debugger::debugger_loop() m_on_exit_callback(); return DebugSession::DebugDecision::Detach; } + remove_temporary_breakpoints(); ASSERT(optional_regs.has_value()); const PtraceRegisters& regs = optional_regs.value(); auto source_position = m_debug_session->debug_info().get_source_position(regs.eip); - if (state.get() == Debugger::DebuggingState::SingleStepping) { + if (m_state.get() == Debugger::DebuggingState::SingleStepping) { ASSERT(source_position.has_value()); - if (state.should_stop_single_stepping(source_position.value())) { - state.set_normal(); + if (m_state.should_stop_single_stepping(source_position.value())) { + m_state.set_normal(); } else { return DebugSession::DebugDecision::SingleStep; } @@ -160,13 +162,21 @@ int Debugger::debugger_loop() m_continue_type = ContinueType::Continue; } - if (m_continue_type == ContinueType::Continue) { + switch (m_continue_type) { + case ContinueType::Continue: + m_state.set_normal(); return DebugSession::DebugDecision::Continue; - } - - if (m_continue_type == ContinueType::SourceSingleStep) { - state.set_single_stepping(source_position.value()); + case ContinueType::SourceSingleStep: + m_state.set_single_stepping(source_position.value()); return DebugSession::DebugDecision::SingleStep; + case ContinueType::SourceStepOut: + m_state.set_stepping_out(); + do_step_out(regs); + return DebugSession::DebugDecision::Continue; + case ContinueType::SourceStepOver: + m_state.set_stepping_over(); + do_step_over(regs); + return DebugSession::DebugDecision::Continue; } ASSERT_NOT_REACHED(); }); @@ -192,4 +202,37 @@ bool Debugger::DebuggingState::should_stop_single_stepping(const DebugInfo::Sour return m_original_source_position.value() != current_source_position; } +void Debugger::remove_temporary_breakpoints() +{ + for (auto breakpoint_address : m_state.temporary_breakpoints()) { + bool rc = m_debug_session->remove_breakpoint((void*)breakpoint_address); + ASSERT(rc); + } + m_state.clear_temporary_breakpoints(); +} + +void Debugger::DebuggingState::clear_temporary_breakpoints() +{ + m_addresses_of_temporary_breakpoints.clear(); +} +void Debugger::DebuggingState::add_temporary_breakpoint(u32 address) +{ + m_addresses_of_temporary_breakpoints.append(address); +} + +void Debugger::do_step_out(const PtraceRegisters& regs) +{ + auto frame_info = StackFrameUtils::get_info(*m_debug_session, regs.ebp); + ASSERT(frame_info.has_value()); + u32 return_address = frame_info.value().return_address; + bool success = m_debug_session->insert_breakpoint(reinterpret_cast(return_address)); + ASSERT(success); + m_state.add_temporary_breakpoint(return_address); +} + +void Debugger::do_step_over(const PtraceRegisters&) +{ + // TODO: Implement +} + } diff --git a/DevTools/HackStudio/Debugger/Debugger.h b/DevTools/HackStudio/Debugger/Debugger.h index 1600e4ba31e..c1fe6b32c97 100644 --- a/DevTools/HackStudio/Debugger/Debugger.h +++ b/DevTools/HackStudio/Debugger/Debugger.h @@ -66,6 +66,8 @@ public: enum class ContinueType { Continue, SourceSingleStep, + SourceStepOut, + SourceStepOver, }; void set_continue_type(ContinueType type) { m_continue_type = type; } @@ -75,17 +77,27 @@ private: class DebuggingState { public: enum State { - Normal, + Normal, // Continue normally until we hit a breakpoint / program terminates SingleStepping, + SteppingOut, + SteppingOver, }; State get() const { return m_state; } + void set_normal(); void set_single_stepping(DebugInfo::SourcePosition original_source_position); + void set_stepping_out() { m_state = State::SteppingOut; } + void set_stepping_over() { m_state = State::SteppingOver; } + bool should_stop_single_stepping(const DebugInfo::SourcePosition& current_source_position) const; + void clear_temporary_breakpoints(); + void add_temporary_breakpoint(u32 address); + const Vector& temporary_breakpoints() const { return m_addresses_of_temporary_breakpoints; } private: State m_state { Normal }; Optional m_original_source_position; // The source position at which we started the current single step + Vector m_addresses_of_temporary_breakpoints; }; explicit Debugger( @@ -98,12 +110,18 @@ private: void start(); int debugger_loop(); + void remove_temporary_breakpoints(); + void do_step_out(const PtraceRegisters&); + void do_step_over(const PtraceRegisters&); + OwnPtr m_debug_session; + DebuggingState m_state; pthread_mutex_t m_continue_mutex {}; pthread_cond_t m_continue_cond {}; Vector m_breakpoints; + String m_executable_path; Function m_on_stopped_callback; diff --git a/Libraries/LibDebug/CMakeLists.txt b/Libraries/LibDebug/CMakeLists.txt index ad2f1bfb15b..f4c4277436f 100644 --- a/Libraries/LibDebug/CMakeLists.txt +++ b/Libraries/LibDebug/CMakeLists.txt @@ -7,6 +7,7 @@ set(SOURCES Dwarf/DwarfInfo.cpp Dwarf/Expression.cpp Dwarf/LineProgram.cpp + StackFrameUtils.cpp ) serenity_lib(LibDebug debug) diff --git a/Libraries/LibDebug/StackFrameUtils.cpp b/Libraries/LibDebug/StackFrameUtils.cpp new file mode 100644 index 00000000000..e85fce67542 --- /dev/null +++ b/Libraries/LibDebug/StackFrameUtils.cpp @@ -0,0 +1,40 @@ +/* + * Copyright (c) 2020, Itamar S. + * All rights reserved. + * + * Redistribution and use in source and binary forms, with or without + * modification, are permitted provided that the following conditions are met: + * + * 1. Redistributions of source code must retain the above copyright notice, this + * list of conditions and the following disclaimer. + * + * 2. Redistributions in binary form must reproduce the above copyright notice, + * this list of conditions and the following disclaimer in the documentation + * and/or other materials provided with the distribution. + * + * THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS "AS IS" + * AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE + * IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE ARE + * DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT HOLDER OR CONTRIBUTORS BE LIABLE + * FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL + * DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR + * SERVICES; LOSS OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER + * CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, + * OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE + * OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. + */ + +#include "StackFrameUtils.h" + +namespace StackFrameUtils { +Optional get_info(const DebugSession& session, FlatPtr current_ebp) +{ + auto return_address = session.peek(reinterpret_cast(current_ebp + sizeof(FlatPtr))); + auto next_ebp = session.peek(reinterpret_cast(current_ebp)); + if (!return_address.has_value() || !next_ebp.has_value()) + return {}; + + StackFrameInfo info = { return_address.value(), next_ebp.value() }; + return info; +} +} diff --git a/Libraries/LibDebug/StackFrameUtils.h b/Libraries/LibDebug/StackFrameUtils.h new file mode 100644 index 00000000000..9948ee04a75 --- /dev/null +++ b/Libraries/LibDebug/StackFrameUtils.h @@ -0,0 +1,42 @@ +/* + * Copyright (c) 2020, Itamar S. + * All rights reserved. + * + * Redistribution and use in source and binary forms, with or without + * modification, are permitted provided that the following conditions are met: + * + * 1. Redistributions of source code must retain the above copyright notice, this + * list of conditions and the following disclaimer. + * + * 2. Redistributions in binary form must reproduce the above copyright notice, + * this list of conditions and the following disclaimer in the documentation + * and/or other materials provided with the distribution. + * + * THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS "AS IS" + * AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE + * IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE ARE + * DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT HOLDER OR CONTRIBUTORS BE LIABLE + * FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL + * DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR + * SERVICES; LOSS OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER + * CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, + * OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE + * OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. + */ + +#pragma once + +#include +#include + +#include "LibDebug/DebugSession.h" + +namespace StackFrameUtils { +struct StackFrameInfo { + FlatPtr return_address; + FlatPtr next_ebp; +}; + +Optional get_info(const DebugSession&, FlatPtr current_ebp); + +}