From 5c494eefd64cfaa4a265c864287b0d7f6ecd60e8 Mon Sep 17 00:00:00 2001 From: Itamar Date: Fri, 21 Aug 2020 16:48:42 +0300 Subject: [PATCH] HackStudio: Implement "Step Over" debugging action The "Step Over" action continues execution without stepping into instructions in subsequent function calls. --- DevTools/HackStudio/Debugger/Debugger.cpp | 34 +++++++++++++++++++---- DevTools/HackStudio/Debugger/Debugger.h | 2 ++ DevTools/HackStudio/main.cpp | 4 --- Libraries/LibDebug/DebugInfo.cpp | 31 +++++++++++++++++++-- Libraries/LibDebug/DebugInfo.h | 6 +++- Libraries/LibDebug/DebugSession.cpp | 2 ++ Libraries/LibDebug/DebugSession.h | 9 +++++- 7 files changed, 75 insertions(+), 13 deletions(-) diff --git a/DevTools/HackStudio/Debugger/Debugger.cpp b/DevTools/HackStudio/Debugger/Debugger.cpp index 0438f3cc417..49280741cd6 100644 --- a/DevTools/HackStudio/Debugger/Debugger.cpp +++ b/DevTools/HackStudio/Debugger/Debugger.cpp @@ -205,6 +205,7 @@ bool Debugger::DebuggingState::should_stop_single_stepping(const DebugInfo::Sour void Debugger::remove_temporary_breakpoints() { for (auto breakpoint_address : m_state.temporary_breakpoints()) { + ASSERT(m_debug_session->breakpoint_exists((void*)breakpoint_address)); bool rc = m_debug_session->remove_breakpoint((void*)breakpoint_address); ASSERT(rc); } @@ -221,18 +222,41 @@ void Debugger::DebuggingState::add_temporary_breakpoint(u32 address) } void Debugger::do_step_out(const PtraceRegisters& regs) +{ + // To step out, we simply insert a temporary breakpoint at the + // instruction the current function returns to, and continue + // execution until we hit that instruction (or some other breakpoint). + insert_temporary_breakpoint_at_return_address(regs); +} + +void Debugger::do_step_over(const PtraceRegisters& regs) +{ + // To step over, we insert a temporary breakpoint at each line in the current function, + // as well as at the current function's return point, and continue execution. + auto current_function = m_debug_session->debug_info().get_containing_function(regs.eip); + ASSERT(current_function.has_value()); + auto lines_in_current_function = m_debug_session->debug_info().source_lines_in_scope(current_function.value()); + for (const auto& line : lines_in_current_function) { + insert_temporary_breakpoint(line.address_of_first_statement); + } + insert_temporary_breakpoint_at_return_address(regs); +} + +void Debugger::insert_temporary_breakpoint_at_return_address(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); + insert_temporary_breakpoint(return_address); } -void Debugger::do_step_over(const PtraceRegisters&) +void Debugger::insert_temporary_breakpoint(FlatPtr address) { - // TODO: Implement + if (m_debug_session->breakpoint_exists((void*)address)) + return; + bool success = m_debug_session->insert_breakpoint(reinterpret_cast(address)); + ASSERT(success); + m_state.add_temporary_breakpoint(address); } } diff --git a/DevTools/HackStudio/Debugger/Debugger.h b/DevTools/HackStudio/Debugger/Debugger.h index c1fe6b32c97..bcd496dda99 100644 --- a/DevTools/HackStudio/Debugger/Debugger.h +++ b/DevTools/HackStudio/Debugger/Debugger.h @@ -113,6 +113,8 @@ private: void remove_temporary_breakpoints(); void do_step_out(const PtraceRegisters&); void do_step_over(const PtraceRegisters&); + void insert_temporary_breakpoint(FlatPtr address); + void insert_temporary_breakpoint_at_return_address(const PtraceRegisters&); OwnPtr m_debug_session; DebuggingState m_state; diff --git a/DevTools/HackStudio/main.cpp b/DevTools/HackStudio/main.cpp index 989e57558e5..5de2cfc823d 100644 --- a/DevTools/HackStudio/main.cpp +++ b/DevTools/HackStudio/main.cpp @@ -614,8 +614,6 @@ int main_impl(int argc, char** argv) RefPtr current_editor_in_execution; Debugger::initialize( [&](const PtraceRegisters& regs) { - dbg() << "Program stopped"; - ASSERT(Debugger::the().session()); const auto& debug_session = *Debugger::the().session(); auto source_position = debug_session.debug_info().get_source_position(regs.eip); @@ -639,7 +637,6 @@ int main_impl(int argc, char** argv) return Debugger::HasControlPassedToUser::Yes; }, [&]() { - dbg() << "Program continued"; Core::EventLoop::main().post_event(*g_window, make([&](auto&) { debug_info_widget.set_debug_actions_enabled(false); if (current_editor_in_execution) { @@ -649,7 +646,6 @@ int main_impl(int argc, char** argv) Core::EventLoop::wake(); }, [&]() { - dbg() << "Program exited"; Core::EventLoop::main().post_event(*g_window, make([&](auto&) { debug_info_widget.program_stopped(); hide_action_tabs(); diff --git a/Libraries/LibDebug/DebugInfo.cpp b/Libraries/LibDebug/DebugInfo.cpp index cd46ab1c476..f8b13d25b5b 100644 --- a/Libraries/LibDebug/DebugInfo.cpp +++ b/Libraries/LibDebug/DebugInfo.cpp @@ -138,7 +138,7 @@ Optional DebugInfo::get_source_position(u32 target_ad // TODO: We can do a binray search here for (size_t i = 0; i < m_sorted_lines.size() - 1; ++i) { if (m_sorted_lines[i + 1].address > target_address) { - return Optional({ m_sorted_lines[i].file, m_sorted_lines[i].line, m_sorted_lines[i].address }); + return SourcePosition::from_line_info(m_sorted_lines[i]); } } return {}; @@ -300,11 +300,38 @@ OwnPtr DebugInfo::create_variable_info(const Dwarf::DIE } String DebugInfo::name_of_containing_function(u32 address) const +{ + auto function = get_containing_function(address); + if (!function.has_value()) + return {}; + return function.value().name; +} + +Optional DebugInfo::get_containing_function(u32 address) const { for (const auto& scope : m_scopes) { if (!scope.is_function || address < scope.address_low || address >= scope.address_high) continue; - return scope.name; + return scope; } return {}; } + +Vector DebugInfo::source_lines_in_scope(const VariablesScope& scope) const +{ + Vector source_lines; + for (const auto& line : m_sorted_lines) { + if (line.address < scope.address_low) + continue; + + if (line.address >= scope.address_high) + break; + source_lines.append(SourcePosition::from_line_info(line)); + } + return source_lines; +} + +DebugInfo::SourcePosition DebugInfo::SourcePosition::from_line_info(const LineProgram::LineInfo& line) +{ + return { line.file, line.line, line.address }; +} diff --git a/Libraries/LibDebug/DebugInfo.h b/Libraries/LibDebug/DebugInfo.h index de9de2ddfff..9208dea102f 100644 --- a/Libraries/LibDebug/DebugInfo.h +++ b/Libraries/LibDebug/DebugInfo.h @@ -47,6 +47,8 @@ public: bool operator==(const SourcePosition& other) const { return file_path == other.file_path && line_number == other.line_number; } bool operator!=(const SourcePosition& other) const { return !(*this == other); } + + static SourcePosition from_line_info(const LineProgram::LineInfo&); }; struct VariableInfo { @@ -80,7 +82,7 @@ public: bool is_function { false }; String name; u32 address_low { 0 }; - u32 address_high { 0 }; + u32 address_high { 0 }; // Non-inclusive - the lowest address after address_low that's not in this scope Vector dies_of_variables; }; @@ -104,6 +106,8 @@ public: } String name_of_containing_function(u32 address) const; + Vector source_lines_in_scope(const VariablesScope&) const; + Optional get_containing_function(u32 address) const; private: void prepare_variable_scopes(); diff --git a/Libraries/LibDebug/DebugSession.cpp b/Libraries/LibDebug/DebugSession.cpp index 242108761a5..2c6394de1eb 100644 --- a/Libraries/LibDebug/DebugSession.cpp +++ b/Libraries/LibDebug/DebugSession.cpp @@ -175,6 +175,8 @@ bool DebugSession::enable_breakpoint(void* address) auto breakpoint = m_breakpoints.get(address); ASSERT(breakpoint.has_value()); + ASSERT(breakpoint.value().state == BreakPointState::Disabled); + if (!poke(reinterpret_cast(breakpoint.value().address), (breakpoint.value().original_first_word & ~(uint32_t)0xff) | BREAKPOINT_INSTRUCTION)) return false; diff --git a/Libraries/LibDebug/DebugSession.h b/Libraries/LibDebug/DebugSession.h index 84a87922c26..e11f93c9441 100644 --- a/Libraries/LibDebug/DebugSession.h +++ b/Libraries/LibDebug/DebugSession.h @@ -207,10 +207,17 @@ void DebugSession::run(Callback callback) // Re-enable the breakpoint if it wasn't removed by the user if (current_breakpoint.has_value() && m_breakpoints.contains(current_breakpoint.value().address)) { - // The current breakpoint was removed in order to make it transparrent to the user. + // The current breakpoint was removed to make it transparrent to the user. // We now want to re-enable it - the code execution flow could hit it again. // To re-enable the breakpoint, we first perform a single step and execute the // instruction of the breakpoint, and then redo the INT3 patch in its first byte. + + // If the user manually inserted a breakpoint at were we breaked at originally, + // we need to disable that breakpoint because we want to singlestep over it to execute the + // instruction we breaked on (we re-enable it again later anyways). + if (m_breakpoints.contains(current_breakpoint.value().address) && m_breakpoints.get(current_breakpoint.value().address).value().state == BreakPointState::Enabled) { + disable_breakpoint(current_breakpoint.value().address); + } auto stopped_address = single_step(); enable_breakpoint(current_breakpoint.value().address); did_single_step = true;