[PATCH v2 41/47] Windows gdb: Watchpoints while running (internal vs external stops)

Pedro Alves
2025-05-19T13:34:12Z

Teach the Windows target to temporarily pause all threads when we
change the debug registers for a watchpoint. Implements the same
logic as Linux uses:

~~~
/* (...) if threads are running when the
mirror changes, a temporary and transparent stop on all threads
is forced so they can get their copy of the debug registers
updated on re-resume. (...) */
~~~

On Linux, we send each thread a SIGSTOP to step them. On Windows,
SuspendThread itself doesn't cause any asynchronous debug event to be
reported. However, we've implemented windows_nat_target::stop such
that it uses SuspendThread, and then queues a pending GDB_SIGNAL_0
stop on the thread. That results in a user-visible stop, while here
we want a non-user-visible stop. So what we do is re-use that
windows_nat_target::stop stopping mechanism, but add an external vs
internal stopping kind distinction. An internal stop results in
windows_nat_target::wait immediately re-resuming the thread.

Note we don't make the debug registers poking code SuspendThread ->
write debug registers -> ContinueThread itself, because SuspendThread
is actually asynchronous and may take a bit to stop the thread (a
following GetThreadContext blocks until the thread is actually
suspended), and, there will be several debug register writes when a
watchpoint is set, because we have to set all of DR0, DR1, DR2, DR3,
and DR7. Defering the actualy writes to ::wait avoids a bunch of
SuspendThread/ResumeThread sequences, so in principle should be
faster.

Change-Id: I39c2492c7aac06d23ef8f287f4afe3747b7bc53f
---
gdb/nat/windows-nat.h | 27 ++++++++--
gdb/windows-nat.c | 118 +++++++++++++++++++++++++++++++++++-------
2 files changed, 121 insertions(+), 24 deletions(-)

diff --git a/gdb/nat/windows-nat.h b/gdb/nat/windows-nat.h
index fe377fcde34..568c7c46a51 100644
--- a/gdb/nat/windows-nat.h
+++ b/gdb/nat/windows-nat.h
@@ -40,6 +40,24 @@ namespace windows_nat

struct windows_process_info;

+/* The reason for explicitly stopping a thread. Note the enumerators
+ are ordered such that when comparing two stopping_kind's numerical
+ value, the highest should prevail. */
+enum stopping_kind
+ {
+ /* Not really stopping the thread. */
+ SK_NOT_STOPPING = 0,
+
+ /* We're stopping the thread for internal reasons, the stop should
+ not be reported as an event to the core. */
+ SK_INTERNAL = 1,
+
+ /* We're stopping the thread for external reasons, meaning, the
+ core/user asked us to stop the thread, so we must report a stop
+ event to the core. */
+ SK_EXTERNAL = 2,
+ };
+
/* Thread information structure used to track extra information about
each thread. */
struct windows_thread_info
@@ -123,9 +141,10 @@ struct windows_thread_info
int suspended = 0;

/* This flag indicates whether we are explicitly stopping this
- thread in response to a target_stop request. This allows
- distinguishing between threads that are explicitly stopped by the
- debugger and threads that are stopped due to other reasons.
+ thread in response to a target_stop request or for
+ backend-internal reasons. This allows distinguishing between
+ threads that are explicitly stopped by the debugger and threads
+ that are stopped due to other reasons.

Typically, when we want to stop a thread, we suspend it, enqueue
a pending GDB_SIGNAL_0 stop status on the thread, and then set
@@ -134,7 +153,7 @@ struct windows_thread_info
already has an event to report. In such case, we simply set the
'stopping' flag without suspending the thread or enqueueing a
pending stop. See stop_one_thread. */
- bool stopping = false;
+ stopping_kind stopping = SK_NOT_STOPPING;

/* Info about a potential pending stop.

diff --git a/gdb/windows-nat.c b/gdb/windows-nat.c
index 4f7828beb36..71ef26501de 100644
--- a/gdb/windows-nat.c
+++ b/gdb/windows-nat.c
@@ -304,6 +304,10 @@ enum windows_continue_flag
all-stop mode. This flag indicates that windows_continue
should call ContinueDebugEvent even in non-stop mode. */
WCONT_CONTINUE_DEBUG_EVENT = 4,
+
+ /* Skip calling ContinueDebugEvent even in all-stop mode. This is
+ the default in non-stop mode. */
+ WCONT_DONT_CONTINUE_DEBUG_EVENT = 8,
};

DEF_ENUM_FLAGS_TYPE (windows_continue_flag, windows_continue_flags);
@@ -572,6 +576,8 @@ struct windows_nat_target final : public x86_nat_target<inf_child_target>
return serial_event_fd (m_wait_event);
}

+ void debug_registers_changed_all_threads ();
+
private:

windows_thread_info *add_thread (ptid_t ptid, HANDLE h, void *tlb,
@@ -579,7 +585,8 @@ struct windows_nat_target final : public x86_nat_target<inf_child_target>
void delete_thread (ptid_t ptid, DWORD exit_code, bool main_thread_p);
DWORD fake_create_process (const DEBUG_EVENT &current_event);

- void stop_one_thread (windows_thread_info *th);
+ void stop_one_thread (windows_thread_info *th,
+ enum stopping_kind stopping_kind);

DWORD continue_status_for_event_detaching
(const DEBUG_EVENT &event, size_t *reply_later_events_left = nullptr);
@@ -1369,7 +1376,7 @@ windows_per_inferior::handle_output_debug_string
a pending event. It will be picked up by
windows_nat_target::wait. */
th->suspend ();
- th->stopping = true;
+ th->stopping = SK_EXTERNAL;
th->last_event = {};
th->pending_status.set_stopped (gotasig);

@@ -1629,7 +1636,7 @@ windows_per_inferior::continue_one_thread (windows_thread_info *th,
});

th->resume ();
- th->stopping = false;
+ th->stopping = SK_NOT_STOPPING;
th->last_sig = GDB_SIGNAL_0;
}

@@ -1704,8 +1711,19 @@ windows_nat_target::windows_continue (DWORD continue_status, int id,
#endif
}

- if (!target_is_non_stop_p ()
- || (cont_flags & WCONT_CONTINUE_DEBUG_EVENT) != 0)
+ /* WCONT_DONT_CONTINUE_DEBUG_EVENT and WCONT_CONTINUE_DEBUG_EVENT
+ can't both be enabled at the same time. */
+ gdb_assert ((cont_flags & WCONT_DONT_CONTINUE_DEBUG_EVENT) == 0
+ || (cont_flags & WCONT_CONTINUE_DEBUG_EVENT) == 0);
+
+ bool continue_debug_event;
+ if ((cont_flags & WCONT_CONTINUE_DEBUG_EVENT) != 0)
+ continue_debug_event = true;
+ else if ((cont_flags & WCONT_DONT_CONTINUE_DEBUG_EVENT) != 0)
+ continue_debug_event = false;
+ else
+ continue_debug_event = !target_is_non_stop_p ();
+ if (continue_debug_event)
{
DEBUG_EVENTS ("windows_continue -> continue_last_debug_event");
continue_last_debug_event_main_thread
@@ -1914,11 +1932,13 @@ windows_nat_target::interrupt ()
"Press Ctrl-c in the program console."));
}

-/* Stop thread TH. This leaves a GDB_SIGNAL_0 pending in the thread,
- which is later consumed by windows_nat_target::wait. */
+/* Stop thread TH, for STOPPING_KIND reason. This leaves a
+ GDB_SIGNAL_0 pending in the thread, which is later consumed by
+ windows_nat_target::wait. */

void
-windows_nat_target::stop_one_thread (windows_thread_info *th)
+windows_nat_target::stop_one_thread (windows_thread_info *th,
+ enum stopping_kind stopping_kind)
{
ptid_t thr_ptid (windows_process.process_id, th->tid);

@@ -1934,12 +1954,18 @@ windows_nat_target::stop_one_thread (windows_thread_info *th)
#ifdef __CYGWIN__
else if (th->suspended
&& th->signaled_thread != nullptr
- && th->pending_status.kind () == TARGET_WAITKIND_IGNORE)
+ && th->pending_status.kind () == TARGET_WAITKIND_IGNORE
+ /* If doing an internal stop to update debug registers,
+ then just leave the "sig" thread suspended. Otherwise
+ windows_nat_target::wait would incorrectly break the
+ signaled_thread lock when it later processes the pending
+ stop and calls windows_continue on this thread. */
+ && stopping_kind == SK_EXTERNAL)
{
DEBUG_EVENTS ("explict stop for \"sig\" thread %s held for signal",
thr_ptid.to_string ().c_str ());

- th->stopping = true;
+ th->stopping = stopping_kind;
th->pending_status.set_stopped (GDB_SIGNAL_0);
th->last_event = {};
serial_event_set (m_wait_event);
@@ -1953,7 +1979,9 @@ windows_nat_target::stop_one_thread (windows_thread_info *th)
thr_ptid.to_string ().c_str (),
th->suspended, th->stopping);

- th->stopping = true;
+ /* Upgrade stopping. */
+ if (stopping_kind > th->stopping)
+ th->stopping = stopping_kind;
}
else
{
@@ -1968,14 +1996,20 @@ windows_nat_target::stop_one_thread (windows_thread_info *th)
{
DEBUG_EVENTS ("suspension of %s failed, expect thread exit event",
thr_ptid.to_string ().c_str ());
+ if (stopping_kind > th->stopping)
+ th->stopping = stopping_kind;
return;
}

gdb_assert (th->suspended == 1);

- th->stopping = true;
- th->pending_status.set_stopped (GDB_SIGNAL_0);
- th->last_event = {};
+ if (stopping_kind > th->stopping)
+ {
+ th->stopping = stopping_kind;
+ th->pending_status.set_stopped (GDB_SIGNAL_0);
+ th->last_event = {};
+ }
+
serial_event_set (m_wait_event);
}
}
@@ -1988,7 +2022,7 @@ windows_nat_target::stop (ptid_t ptid)
for (thread_info *thr : all_non_exited_threads (this))
{
if (thr->ptid.matches (ptid))
- stop_one_thread (as_windows_thread_info (thr));
+ stop_one_thread (as_windows_thread_info (thr), SK_EXTERNAL);
}
}

@@ -2493,6 +2527,17 @@ windows_nat_target::wait (ptid_t ptid, struct target_waitstatus *ourstatus,
{
windows_thread_info *th = windows_process.find_thread (result);

+ /* If this thread was temporarily stopped just so we
+ could update its debug registers on the next
+ resumption, do it now. */
+ if (th->stopping == SK_INTERNAL)
+ {
+ gdb_assert (fake);
+ windows_continue (DBG_CONTINUE, th->tid,
+ WCONT_DONT_CONTINUE_DEBUG_EVENT);
+ continue;
+ }
+
th->stopped_at_software_breakpoint = false;
if (current_event.dwDebugEventCode
== EXCEPTION_DEBUG_EVENT
@@ -2539,7 +2584,7 @@ windows_nat_target::wait (ptid_t ptid, struct target_waitstatus *ourstatus,
for (auto *thr : all_windows_threads ())
thr->suspend ();

- th->stopping = false;
+ th->stopping = SK_NOT_STOPPING;
}

/* If something came out, assume there may be more. This is
@@ -4158,6 +4203,41 @@ Use \"%ps\" or \"%ps\" command to load executable/libraries directly."),
}
}

+/* For each thread, set the debug_registers_changed flag, and
+ temporarily stop it so we can update its debug registers. */
+
+void
+windows_nat_target::debug_registers_changed_all_threads ()
+{
+ for (auto *th : all_windows_threads ())
+ {
+ th->debug_registers_changed = true;
+
+ /* Note we don't SuspendThread => update debug regs =>
+ ResumeThread, because SuspendThread is actually asynchronous
+ (and GetThreadContext blocks until the thread really
+ suspends), and doing that for all threads may take a bit.
+ Also, the core does one call per DR register update, so that
+ would result in a lot of suspend-resumes. So instead, we
+ suspend the thread if it wasn't already suspended, and queue
+ a pending stop to be handled by windows_nat_target::wait.
+ This means we only stop each thread once, and, we don't block
+ waiting for each individual thread stop. */
+ stop_one_thread (th, SK_INTERNAL);
+ }
+}
+
+/* Trampoline helper to get at the
+ windows_nat_target::debug_registers_changed_all_threads method in
+ the native target. */
+
+static void
+debug_registers_changed_all_threads ()
+{
+ auto *win_tgt = static_cast<windows_nat_target *> (get_native_target ());
+ win_tgt->debug_registers_changed_all_threads ();
+}
+
/* Hardware watchpoint support, adapted from go32-nat.c code. */

/* Pass the address ADDR to the inferior in the I'th debug register.
@@ -4169,8 +4249,7 @@ windows_set_dr (int i, CORE_ADDR addr)
if (i < 0 || i > 3)
internal_error (_("Invalid register %d in windows_set_dr.\n"), i);

- for (auto *th : all_windows_threads ())
- th->debug_registers_changed = true;
+ debug_registers_changed_all_threads ();
}

/* Pass the value VAL to the inferior in the DR7 debug control
@@ -4179,8 +4258,7 @@ windows_set_dr (int i, CORE_ADDR addr)
static void
windows_set_dr7 (unsigned long val)
{
- for (auto *th : all_windows_threads ())
- th->debug_registers_changed = true;
+ debug_registers_changed_all_threads ();
}

/* Get the value of debug register I from the inferior. */
--
2.49.0