From 311a355505bbf02a4df87befbb22d0ea4b678302 Mon Sep 17 00:00:00 2001 From: Itamar Date: Sat, 15 Aug 2020 10:58:11 +0300 Subject: [PATCH] HackStudio: Change singlestepping logic in the debugger Previously, we did source-level singlestepping by inserting a breakpoint at every source line and continued execution until we hit a breakpoint. We did this because we used to not generate source locations debug info for library code, and it allowed us to not single step through lots of library code to get to the next source line (which is super slow). Since we now do generate source locations debug info for libraries (-g1), we can improve the way we implement source level stepping by stepping at the assembly level until we reach a different source code location. --- DevTools/HackStudio/Debugger/Debugger.cpp | 61 +++++++++++++---------- DevTools/HackStudio/Debugger/Debugger.h | 16 ++++++ 2 files changed, 52 insertions(+), 25 deletions(-) diff --git a/DevTools/HackStudio/Debugger/Debugger.cpp b/DevTools/HackStudio/Debugger/Debugger.cpp index cb325e2b275..2c3f319ec3d 100644 --- a/DevTools/HackStudio/Debugger/Debugger.cpp +++ b/DevTools/HackStudio/Debugger/Debugger.cpp @@ -74,8 +74,13 @@ void Debugger::on_breakpoint_change(const String& file, size_t line, BreakpointC return; auto address = session->debug_info().get_instruction_from_source(position.file_path, position.line_number); - if (!address.has_value()) + if (!address.has_value()) { + dbg() << "Warning: couldn't get instruction address from source"; + // TODO: Currently, the GUI will indicate that a breakpoint was inserted/remove at this line, + // regardless of whether we actually succeeded to insert it. (For example a breakpoint on a comment, or an include statemanet). + // We should indicate failure via a return value from this function, and not update the breakpoint GUI if we fail. return; + } if (change_type == BreakpointChange::Added) { bool success = session->insert_breakpoint(reinterpret_cast(address.value())); @@ -88,7 +93,9 @@ void Debugger::on_breakpoint_change(const String& file, size_t line, BreakpointC DebugInfo::SourcePosition Debugger::create_source_position(const String& file, size_t line) { - return { String::format("./%s", file.characters()), line + 1 }; + if (!file.starts_with('/') && !file.starts_with("./")) + return { String::format("./%s", file.characters()), line + 1 }; + return { file, line + 1 }; } int Debugger::start_static() @@ -118,8 +125,7 @@ void Debugger::start() int Debugger::debugger_loop() { - bool in_single_step_mode = false; - Vector temporary_breakpoints; + DebuggingState state; m_debug_session->run([&](DebugSession::DebugBreakReason reason, Optional optional_regs) { if (reason == DebugSession::DebugBreakReason::Exited) { @@ -130,12 +136,14 @@ int Debugger::debugger_loop() ASSERT(optional_regs.has_value()); const PtraceRegisters& regs = optional_regs.value(); - if (in_single_step_mode) { - for (auto address : temporary_breakpoints) { - m_debug_session->remove_breakpoint(address); + auto source_position = m_debug_session->debug_info().get_source_position(regs.eip); + if (state.get() == Debugger::DebuggingState::SingleStepping) { + ASSERT(source_position.has_value()); + if (state.should_stop_single_stepping(source_position.value())) { + state.set_normal(); + } else { + return DebugSession::DebugDecision::SingleStep; } - temporary_breakpoints.clear(); - in_single_step_mode = false; } auto control_passed_to_user = m_on_stopped_callback(regs); @@ -155,25 +163,28 @@ int Debugger::debugger_loop() } if (m_continue_type == ContinueType::SourceSingleStep) { - // A possible method for source level single stepping is to single step - // in assembly level, until the current instruction's source position has changed. - // However, since we do not currently generate debug symbols for library code, - // we may have to single-step over lots of library code instructions until we get back to our code, - // which is very slow. - // So the current method is to insert a temporary breakpoint at every known statement in our source code, - // continue execution, and remove the temporary breakpoints once we hit the first breakpoint. - m_debug_session->debug_info().for_each_source_position([&](DebugInfo::SourcePosition position) { - auto address = (void*)position.address_of_first_statement; - if ((u32)address != regs.eip && !m_debug_session->breakpoint_exists(address)) { - m_debug_session->insert_breakpoint(address); - temporary_breakpoints.append(address); - } - }); - in_single_step_mode = true; - return DebugSession::DebugDecision::Continue; + state.set_single_stepping(source_position.value()); + return DebugSession::DebugDecision::SingleStep; } ASSERT_NOT_REACHED(); }); m_debug_session.clear(); return 0; } + +void Debugger::DebuggingState::set_normal() +{ + m_state = State::Normal; + m_original_source_position.clear(); +} + +void Debugger::DebuggingState::set_single_stepping(DebugInfo::SourcePosition original_source_position) +{ + m_state = State::SingleStepping; + m_original_source_position = original_source_position; +} +bool Debugger::DebuggingState::should_stop_single_stepping(const DebugInfo::SourcePosition& current_source_position) const +{ + ASSERT(m_state == State::SingleStepping); + return m_original_source_position.value() != current_source_position; +} diff --git a/DevTools/HackStudio/Debugger/Debugger.h b/DevTools/HackStudio/Debugger/Debugger.h index 451269746a9..6de8c03d979 100644 --- a/DevTools/HackStudio/Debugger/Debugger.h +++ b/DevTools/HackStudio/Debugger/Debugger.h @@ -70,6 +70,22 @@ public: void reset_breakpoints() { m_breakpoints.clear(); } private: + class DebuggingState { + public: + enum State { + Normal, + SingleStepping, + }; + State get() const { return m_state; } + void set_normal(); + void set_single_stepping(DebugInfo::SourcePosition original_source_position); + bool should_stop_single_stepping(const DebugInfo::SourcePosition& current_source_position) const; + + private: + State m_state { Normal }; + Optional m_original_source_position; // The source position at which we started the current single step + }; + explicit Debugger( Function on_stop_callback, Function on_continue_callback,