HackStudio: Detach from debugged process before terminating

Fixes #3308

5 files changed, 62 insertions, 37 deletions

diff --git a/DevTools/HackStudio/Debugger/DebugInfoWidget.cpp b/DevTools/HackStudio/Debugger/DebugInfoWidget.cpp
index c44fba80cc..c73467d51a 100644
--- a/DevTools/HackStudio/Debugger/DebugInfoWidget.cpp
+++ b/DevTools/HackStudio/Debugger/DebugInfoWidget.cpp
@@ -46,31 +46,19 @@ namespace HackStudio {
 void DebugInfoWidget::init_toolbar()
 {
     m_continue_action = GUI::Action::create("Continue", Gfx::Bitmap::load_from_file("/res/icons/16x16/debug-continue.png"), [](auto&) {
-        pthread_mutex_lock(Debugger::the().continue_mutex());
-        Debugger::the().set_continue_type(Debugger::ContinueType::Continue);
-        pthread_cond_signal(Debugger::the().continue_cond());
-        pthread_mutex_unlock(Debugger::the().continue_mutex());
+        Debugger::the().set_requested_debugger_action(Debugger::DebuggerAction::Continue);
     });
 
     m_singlestep_action = GUI::Action::create("Step Over", { Mod_None, Key_F10 }, 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());
+        Debugger::the().set_requested_debugger_action(Debugger::DebuggerAction::SourceStepOver);
     });
 
     m_step_in_action = GUI::Action::create("Step In", { Mod_None, Key_F11 }, 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());
+        Debugger::the().set_requested_debugger_action(Debugger::DebuggerAction::SourceSingleStep);
     });
 
     m_step_out_action = GUI::Action::create("Step Out", { Mod_Shift, Key_F11 }, 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());
+        Debugger::the().set_requested_debugger_action(Debugger::DebuggerAction::SourceStepOut);
     });
 
     m_toolbar->add_action(*m_continue_action);
diff --git a/DevTools/HackStudio/Debugger/Debugger.cpp b/DevTools/HackStudio/Debugger/Debugger.cpp
index 4a23ada332..97fb0aee9b 100644
--- a/DevTools/HackStudio/Debugger/Debugger.cpp
+++ b/DevTools/HackStudio/Debugger/Debugger.cpp
@@ -58,8 +58,8 @@ Debugger::Debugger(
     , m_on_continue_callback(move(on_continue_callback))
     , m_on_exit_callback(move(on_exit_callback))
 {
-    pthread_mutex_init(&m_continue_mutex, nullptr);
-    pthread_cond_init(&m_continue_cond, nullptr);
+    pthread_mutex_init(&m_ui_action_mutex, nullptr);
+    pthread_cond_init(&m_ui_action_cond, nullptr);
 }
 
 void Debugger::on_breakpoint_change(const String& file, size_t line, BreakpointChange change_type)
@@ -153,30 +153,38 @@ int Debugger::debugger_loop()
         auto control_passed_to_user = m_on_stopped_callback(regs);
 
         if (control_passed_to_user == HasControlPassedToUser::Yes) {
-            pthread_mutex_lock(&m_continue_mutex);
-            pthread_cond_wait(&m_continue_cond, &m_continue_mutex);
-            pthread_mutex_unlock(&m_continue_mutex);
+            pthread_mutex_lock(&m_ui_action_mutex);
+            pthread_cond_wait(&m_ui_action_cond, &m_ui_action_mutex);
+            pthread_mutex_unlock(&m_ui_action_mutex);
+
+            if (m_requested_debugger_action != DebuggerAction::Exit)
+                m_on_continue_callback();
 
-            m_on_continue_callback();
         } else {
-            m_continue_type = ContinueType::Continue;
+            m_requested_debugger_action = DebuggerAction::Continue;
         }
 
-        switch (m_continue_type) {
-        case ContinueType::Continue:
+        switch (m_requested_debugger_action) {
+        case DebuggerAction::Continue:
             m_state.set_normal();
             return Debug::DebugSession::DebugDecision::Continue;
-        case ContinueType::SourceSingleStep:
+        case DebuggerAction::SourceSingleStep:
             m_state.set_single_stepping(source_position.value());
             return Debug::DebugSession::DebugDecision::SingleStep;
-        case ContinueType::SourceStepOut:
+        case DebuggerAction::SourceStepOut:
             m_state.set_stepping_out();
             do_step_out(regs);
             return Debug::DebugSession::DebugDecision::Continue;
-        case ContinueType::SourceStepOver:
+        case DebuggerAction::SourceStepOver:
             m_state.set_stepping_over();
             do_step_over(regs);
             return Debug::DebugSession::DebugDecision::Continue;
+        case DebuggerAction::Exit:
+            // NOTE: Is detaching from the debugee the best thing to do here?
+            // We could display a dialog in the UI, remind the user that there is
+            // a live debugged process, and ask whether they want to terminate/detach.
+            dbg() << "Debugger exiting";
+            return Debug::DebugSession::DebugDecision::Detach;
         }
         ASSERT_NOT_REACHED();
     });
@@ -259,4 +267,12 @@ void Debugger::insert_temporary_breakpoint(FlatPtr address)
     m_state.add_temporary_breakpoint(address);
 }
 
+void Debugger::set_requested_debugger_action(DebuggerAction action)
+{
+    pthread_mutex_lock(continue_mutex());
+    m_requested_debugger_action = action;
+    pthread_cond_signal(continue_cond());
+    pthread_mutex_unlock(continue_mutex());
+}
+
 }
diff --git a/DevTools/HackStudio/Debugger/Debugger.h b/DevTools/HackStudio/Debugger/Debugger.h
index ec7dead40e..5c38d4bbce 100644
--- a/DevTools/HackStudio/Debugger/Debugger.h
+++ b/DevTools/HackStudio/Debugger/Debugger.h
@@ -60,17 +60,18 @@ public:
     // Thread entry point
     static int start_static();
 
-    pthread_mutex_t* continue_mutex() { return &m_continue_mutex; }
-    pthread_cond_t* continue_cond() { return &m_continue_cond; }
+    pthread_mutex_t* continue_mutex() { return &m_ui_action_mutex; }
+    pthread_cond_t* continue_cond() { return &m_ui_action_cond; }
 
-    enum class ContinueType {
+    enum class DebuggerAction {
         Continue,
         SourceSingleStep,
         SourceStepOut,
         SourceStepOver,
+        Exit,
     };
 
-    void set_continue_type(ContinueType type) { m_continue_type = type; }
+    void set_requested_debugger_action(DebuggerAction);
     void reset_breakpoints() { m_breakpoints.clear(); }
 
 private:
@@ -119,8 +120,9 @@ private:
     OwnPtr<Debug::DebugSession> m_debug_session;
     DebuggingState m_state;
 
-    pthread_mutex_t m_continue_mutex {};
-    pthread_cond_t m_continue_cond {};
+    pthread_mutex_t m_ui_action_mutex {};
+    pthread_cond_t m_ui_action_cond {};
+    DebuggerAction m_requested_debugger_action { DebuggerAction::Continue };
 
     Vector<Debug::DebugInfo::SourcePosition> m_breakpoints;
 
@@ -129,8 +131,6 @@ private:
     Function<HasControlPassedToUser(const PtraceRegisters&)> m_on_stopped_callback;
     Function<void()> m_on_continue_callback;
     Function<void()> m_on_exit_callback;
-
-    ContinueType m_continue_type { ContinueType::Continue };
 };
 
 }
diff --git a/DevTools/HackStudio/HackStudioWidget.cpp b/DevTools/HackStudio/HackStudioWidget.cpp
index 6d415976f8..baff5106eb 100644
--- a/DevTools/HackStudio/HackStudioWidget.cpp
+++ b/DevTools/HackStudio/HackStudioWidget.cpp
@@ -678,9 +678,15 @@ void HackStudioWidget::create_form_editor(GUI::Widget& parent)
 
     GUI::WidgetClassRegistration::for_each([&, this](const GUI::WidgetClassRegistration& reg) {
         constexpr size_t gui_namespace_prefix_length = sizeof("GUI::") - 1;
-        auto icon_path = String::format("/res/icons/hackstudio/G%s.png", reg.class_name().substring(gui_namespace_prefix_length, reg.class_name().length() - gui_namespace_prefix_length).characters());
+        auto icon_path = String::format(
+            "/res/icons/hackstudio/G%s.png",
+            reg.class_name().substring(
+                                gui_namespace_prefix_length,
+                                reg.class_name().length() - gui_namespace_prefix_length)
+                .characters());
         if (!Core::File::exists(icon_path))
             return;
+
         auto action = GUI::Action::create_checkable(reg.class_name(), Gfx::Bitmap::load_from_file(icon_path), [&reg, this](auto&) {
             m_form_editor_widget->set_tool(make<WidgetTool>(*m_form_editor_widget, reg));
             auto widget = reg.construct();
@@ -899,4 +905,18 @@ void HackStudioWidget::initialize_menubar(GUI::MenuBar& menubar)
     create_help_menubar(menubar);
 }
 
+HackStudioWidget::~HackStudioWidget()
+{
+    if (!m_debugger_thread.is_null()) {
+        Debugger::the().set_requested_debugger_action(Debugger::DebuggerAction::Exit);
+        void* retval;
+        dbg() << "Waiting for debugger thread to terminate";
+        int rc = pthread_join(m_debugger_thread->tid(), &retval);
+        if (rc < 0) {
+            perror("pthread_join");
+            dbg() << "error joining debugger thread";
+        }
+    }
+}
+
 }
diff --git a/DevTools/HackStudio/HackStudioWidget.h b/DevTools/HackStudio/HackStudioWidget.h
index ec1f918b3d..bf219d9e1c 100644
--- a/DevTools/HackStudio/HackStudioWidget.h
+++ b/DevTools/HackStudio/HackStudioWidget.h
@@ -48,6 +48,7 @@ class HackStudioWidget : public GUI::Widget {
     C_OBJECT(HackStudioWidget)
 
 public:
+    virtual ~HackStudioWidget() override;
     void open_file(const String& filename);
 
     Vector<String> selected_file_names() const;