Debugger: Breakpoints now persist after being tripped

Previously, a breakpoint was removed after it was tripped.

After a breakpoint trips, we have to undo the 'int3' patch
from the instruction in order to continue the exceution.

To make a breakpoint persist, we switch to "single step" mode,
which stops the execution after a single instruction, and then we
insert the breakpoint at the previous instruction.

There is also some code that deals with an edge case where there are
breakpoints in two consecutive instructions.

2 files changed, 98 insertions, 39 deletions

diff --git a/Applications/Debugger/DebugSession.cpp b/Applications/Debugger/DebugSession.cpp
index 14e673cc5e..5519c73289 100644
--- a/Applications/Debugger/DebugSession.cpp
+++ b/Applications/Debugger/DebugSession.cpp
@@ -132,18 +132,38 @@ bool DebugSession::insert_breakpoint(void* address)
     if (!original_bytes.has_value())
         return false;
 
-    if (!poke(reinterpret_cast<u32*>(address), (original_bytes.value() & ~(uint32_t)0xff) | BREAKPOINT_INSTRUCTION))
+    BreakPoint breakpoint { address, original_bytes.value(), BreakPointState::Disabled };
+
+    m_breakpoints.set(address, breakpoint);
+
+    enable_breakpoint(breakpoint);
+
+    return true;
+}
+
+bool DebugSession::disable_breakpoint(const BreakPoint& breakpoint)
+{
+    ASSERT(m_breakpoints.contains(breakpoint.address));
+    if (!poke(reinterpret_cast<u32*>(reinterpret_cast<char*>(breakpoint.address)), breakpoint.original_first_word))
         return false;
 
-    m_breakpoints.set(address, { address, original_bytes.value() });
+    auto bp = m_breakpoints.get(breakpoint.address).value();
+    bp.state = BreakPointState::Disabled;
+    m_breakpoints.set(bp.address, bp);
     return true;
 }
 
-void DebugSession::remove_breakpoint(const BreakPoint& breakpoint)
+bool DebugSession::enable_breakpoint(const BreakPoint& breakpoint)
 {
     ASSERT(m_breakpoints.contains(breakpoint.address));
-    poke(reinterpret_cast<u32*>(reinterpret_cast<char*>(breakpoint.address)), breakpoint.original_first_word);
-    m_breakpoints.remove(breakpoint.address);
+
+    if (!poke(reinterpret_cast<u32*>(breakpoint.address), (breakpoint.original_first_word & ~(uint32_t)0xff) | BREAKPOINT_INSTRUCTION))
+        return false;
+
+    auto bp = m_breakpoints.get(breakpoint.address).value();
+    bp.state = BreakPointState::Enabled;
+    m_breakpoints.set(bp.address, bp);
+    return true;
 }
 
 PtraceRegisters DebugSession::get_registers() const
@@ -164,11 +184,6 @@ void DebugSession::set_registers(const PtraceRegisters& regs)
     }
 }
 
-Optional<DebugSession::BreakPoint> DebugSession::get_matching_breakpoint(const PtraceRegisters& regs) const
-{
-    return m_breakpoints.get(reinterpret_cast<void*>(regs.eip - 1));
-}
-
 void DebugSession::continue_debugee()
 {
     if (ptrace(PT_CONTINUE, m_debugee_pid, 0, 0) < 0) {
@@ -176,3 +191,23 @@ void DebugSession::continue_debugee()
         ASSERT_NOT_REACHED();
     }
 }
+
+void* DebugSession::single_step()
+{
+    auto regs = get_registers();
+    constexpr u32 TRAP_FLAG = 0x100;
+    regs.eflags |= TRAP_FLAG;
+    set_registers(regs);
+
+    continue_debugee();
+
+    if (waitpid(m_debugee_pid, 0, WSTOPPED) != m_debugee_pid) {
+        perror("waitpid");
+        ASSERT_NOT_REACHED();
+    }
+
+    regs = get_registers();
+    regs.eflags &= ~(TRAP_FLAG);
+    set_registers(regs);
+    return (void*)regs.eip;
+}
diff --git a/Applications/Debugger/DebugSession.h b/Applications/Debugger/DebugSession.h
index 2b683c6ecd..43488754c5 100644
--- a/Applications/Debugger/DebugSession.h
+++ b/Applications/Debugger/DebugSession.h
@@ -53,19 +53,26 @@ public:
     bool poke(u32* address, u32 data);
     Optional<u32> peek(u32* address) const;
 
+    enum class BreakPointState {
+        Enabled,
+        Disabled,
+    };
+
     struct BreakPoint {
         void* address;
         u32 original_first_word;
+        BreakPointState state;
     };
 
     bool insert_breakpoint(void* address);
-    void remove_breakpoint(const BreakPoint&);
-    Optional<BreakPoint> get_matching_breakpoint(const PtraceRegisters&) const;
+    bool disable_breakpoint(const BreakPoint&);
+    bool enable_breakpoint(const BreakPoint&);
 
     PtraceRegisters get_registers() const;
     void set_registers(const PtraceRegisters&);
 
     void continue_debugee();
+    void* single_step();
 
     template<typename Callback>
     void run(Callback callback);
@@ -100,40 +107,57 @@ private:
 template<typename Callback>
 void DebugSession::run(Callback callback)
 {
+    bool in_consecutive_breakpoint = false;
     for (;;) {
-        continue_debugee();
+        if (!in_consecutive_breakpoint) {
+            continue_debugee();
 
-        int wstatus = 0;
-        if (waitpid(m_debugee_pid, &wstatus, WSTOPPED | WEXITED) != m_debugee_pid) {
-            perror("waitpid");
-            ASSERT_NOT_REACHED();
-        }
+            int wstatus = 0;
+            if (waitpid(m_debugee_pid, &wstatus, WSTOPPED | WEXITED) != m_debugee_pid) {
+                perror("waitpid");
+                ASSERT_NOT_REACHED();
+            }
 
-        // FIXME: This check actually only checks whether the debugee
-        // Is stopped because it hit a breakpoint or not
-        if (WSTOPSIG(wstatus) != SIGTRAP) {
-            callback(DebugBreakReason::Exited, Optional<PtraceRegisters>());
-            m_is_debugee_dead = true;
-            break;
+            // FIXME: This check actually only checks whether the debugee
+            // Is stopped because it hit a breakpoint or not
+            if (WSTOPSIG(wstatus) != SIGTRAP) {
+                callback(DebugBreakReason::Exited, Optional<PtraceRegisters>());
+                m_is_debugee_dead = true;
+                break;
+            }
         }
 
         auto regs = get_registers();
+        Optional<BreakPoint> current_breakpoint;
 
-        auto current_breakpoint = get_matching_breakpoint(regs);
-        if (current_breakpoint.has_value()) {
-            // FIXME: The current implementation removes a breakpoint
-            // after the first time it has been triggered.
-            remove_breakpoint(current_breakpoint.value());
-
-            // We need to re-execute the instruction we patched for the breakpoint,
-            // so we rollback the instruction pointer to the breakpoint's address
-            regs.eip = reinterpret_cast<u32>(current_breakpoint.value().address);
-            set_registers(regs);
-            DebugDecision decision = callback(DebugBreakReason::Breakpoint, regs);
-            if (decision != DebugDecision::Continue) {
-                // FIXME: implement detach & kill
-                ASSERT_NOT_REACHED();
-            }
+        if (in_consecutive_breakpoint) {
+            current_breakpoint = m_breakpoints.get((void*)regs.eip);
+        } else {
+            current_breakpoint = m_breakpoints.get((void*)((u32)regs.eip - 1));
         }
+
+        ASSERT(current_breakpoint.has_value());
+
+        // We want to make the breakpoint transparrent to the user of the debugger
+
+        regs.eip = reinterpret_cast<u32>(current_breakpoint.value().address);
+        set_registers(regs);
+        disable_breakpoint(current_breakpoint.value());
+
+        DebugDecision decision = callback(DebugBreakReason::Breakpoint, regs);
+        if (decision != DebugDecision::Continue) {
+            // FIXME: implement detach & kill
+            ASSERT_NOT_REACHED();
+        }
+
+        // Re-enable the breakpoint
+        auto stopped_address = single_step();
+        enable_breakpoint(current_breakpoint.value());
+
+        // If there is another breakpoint after the current one,
+        // Then we are already on it (because of single_step)
+        auto breakpoint_at_next_instruction = m_breakpoints.get(stopped_address);
+        in_consecutive_breakpoint = breakpoint_at_next_instruction.has_value()
+            && breakpoint_at_next_instruction.value().state == BreakPointState::Enabled;
     }
 }