From ad59fb7cf0068861072aabadb19749ff785a0d67 Mon Sep 17 00:00:00 2001 From: Sam Atkins Date: Thu, 11 Jan 2024 14:57:57 +0000 Subject: [PATCH] HackStudio: Make Editor ask Debugger if a breakpoint was added/removed Rather than adding/removing a breakpoint indicator, and then telling the debugger about it and hoping it works, let the debugger tell us if it succeeded and then use that to update the indicator. This prevents the user from adding breakpoints to invalid locations while the debugger is running. It also avoids a couple of scary VERIFY()s. We still allow creating breakpoints in invalid locations while the debugger is *not* running. --- .../DevTools/HackStudio/Debugger/Debugger.cpp | 56 +++++++++++-------- .../DevTools/HackStudio/Debugger/Debugger.h | 2 +- Userland/DevTools/HackStudio/Editor.cpp | 15 +++-- 3 files changed, 43 insertions(+), 30 deletions(-) diff --git a/Userland/DevTools/HackStudio/Debugger/Debugger.cpp b/Userland/DevTools/HackStudio/Debugger/Debugger.cpp index 2a5a193fce..3f22bfcea8 100644 --- a/Userland/DevTools/HackStudio/Debugger/Debugger.cpp +++ b/Userland/DevTools/HackStudio/Debugger/Debugger.cpp @@ -1,5 +1,6 @@ /* * Copyright (c) 2020, Itamar S. + * Copyright (c) 2024, Sam Atkins * * SPDX-License-Identifier: BSD-2-Clause */ @@ -48,36 +49,44 @@ Debugger::Debugger( pthread_cond_init(&m_ui_action_cond, nullptr); } -void Debugger::on_breakpoint_change(ByteString const& file, size_t line, BreakpointChange change_type) +bool Debugger::change_breakpoint(ByteString const& file, size_t line, BreakpointChange change_type) { auto position = create_source_position(file, line); - - if (change_type == BreakpointChange::Added) { - m_breakpoints.append(position); - } else { - m_breakpoints.remove_all_matching([&](Debug::DebugInfo::SourcePosition const& val) { return val == position; }); - } - auto session = Debugger::the().session(); - if (!session) - return; + if (session) { + auto address = session->get_address_from_source_position(position.file_path, position.line_number); + if (!address.has_value()) { + dbgln("Warning: couldn't get instruction address from source"); + return false; + } - auto address = session->get_address_from_source_position(position.file_path, position.line_number); - if (!address.has_value()) { - dbgln("Warning: couldn't get instruction address from source"); - // TODO: Currently, the GUI will indicate that a breakpoint was inserted/removed at this line, - // regardless of whether we actually succeeded to insert it. (For example a breakpoint on a comment, or an include statement). - // We should indicate failure via a return value from this function, and not update the breakpoint GUI if we fail. - return; + switch (change_type) { + case BreakpointChange::Added: + if (session->insert_breakpoint(address.value().address)) { + m_breakpoints.append(position); + return true; + } + break; + case BreakpointChange::Removed: + if (session->remove_breakpoint(address.value().address)) { + m_breakpoints.remove_all_matching([&](Debug::DebugInfo::SourcePosition const& val) { return val == position; }); + return true; + } + break; + } + return false; } - if (change_type == BreakpointChange::Added) { - bool success = session->insert_breakpoint(address.value().address); - VERIFY(success); - } else { - bool success = session->remove_breakpoint(address.value().address); - VERIFY(success); + // No active session, so just modify our internal list of breakpoints + switch (change_type) { + case BreakpointChange::Added: + m_breakpoints.append(position); + return true; + case BreakpointChange::Removed: + m_breakpoints.remove_all_matching([&](Debug::DebugInfo::SourcePosition const& val) { return val == position; }); + return true; } + VERIFY_NOT_REACHED(); } bool Debugger::set_execution_position(ByteString const& file, size_t line) @@ -125,6 +134,7 @@ void Debugger::start() bool success = m_debug_session->insert_breakpoint(address.value().address); VERIFY(success); } else { + // FIXME: Report the invalid breakpoint to the GUI somehow. dbgln("couldn't insert breakpoint"); } } diff --git a/Userland/DevTools/HackStudio/Debugger/Debugger.h b/Userland/DevTools/HackStudio/Debugger/Debugger.h index c6b833b005..10873a8c01 100644 --- a/Userland/DevTools/HackStudio/Debugger/Debugger.h +++ b/Userland/DevTools/HackStudio/Debugger/Debugger.h @@ -34,7 +34,7 @@ public: static bool is_initialized(); - void on_breakpoint_change(ByteString const& file, size_t line, BreakpointChange change_type); + [[nodiscard]] bool change_breakpoint(ByteString const& file, size_t line, BreakpointChange change_type); bool set_execution_position(ByteString const& file, size_t line); void set_executable_path(ByteString const& path) { m_executable_path = path; } diff --git a/Userland/DevTools/HackStudio/Editor.cpp b/Userland/DevTools/HackStudio/Editor.cpp index 043ba7fa09..14789139fb 100644 --- a/Userland/DevTools/HackStudio/Editor.cpp +++ b/Userland/DevTools/HackStudio/Editor.cpp @@ -1,6 +1,7 @@ /* * Copyright (c) 2018-2021, Andreas Kling * Copyright (c) 2018-2022, the SerenityOS developers. + * Copyright (c) 2023-2024, Sam Atkins * * SPDX-License-Identifier: BSD-2-Clause */ @@ -774,9 +775,10 @@ void Editor::set_semantic_syntax_highlighting(bool value) ErrorOr Editor::add_breakpoint(size_t line_number) { if (!breakpoint_lines().contains_slow(line_number)) { - add_gutter_indicator(m_breakpoint_indicator_id, line_number); - TRY(breakpoint_lines().try_append(line_number)); - Debugger::the().on_breakpoint_change(wrapper().filename_title(), line_number, BreakpointChange::Added); + if (Debugger::the().change_breakpoint(wrapper().filename_title(), line_number, BreakpointChange::Added)) { + add_gutter_indicator(m_breakpoint_indicator_id, line_number); + TRY(breakpoint_lines().try_append(line_number)); + } } return {}; @@ -784,9 +786,10 @@ ErrorOr Editor::add_breakpoint(size_t line_number) void Editor::remove_breakpoint(size_t line_number) { - remove_gutter_indicator(m_breakpoint_indicator_id, line_number); - breakpoint_lines().remove_first_matching([&](size_t line) { return line == line_number; }); - Debugger::the().on_breakpoint_change(wrapper().filename_title(), line_number, BreakpointChange::Removed); + if (Debugger::the().change_breakpoint(wrapper().filename_title(), line_number, BreakpointChange::Removed)) { + remove_gutter_indicator(m_breakpoint_indicator_id, line_number); + breakpoint_lines().remove_first_matching([&](size_t line) { return line == line_number; }); + } } ErrorOr Editor::update_git_diff_indicators()