[PATCH v2 07/47] Windows gdb: Eliminate global current_process.dr[8] global

Pedro Alves
2025-05-19T13:25:37Z

current_process.dr needs to be per-thread for non-stop. Actually, it
doesn't even need to exist at all. We have x86_debug_reg_state
recording intent, and then the
cygwin_get_dr/cygwin_get_dr6/cygwin_get_dr7 functions are registered
as x86_dr_low_type vector functions, so they should return the current
value in the inferior's registers. See this comment in x86-dregs.c:

~~~
/* In non-stop/async, threads can be running while we change the
global dr_mirror (and friends). Say, we set a watchpoint, and
let threads resume. Now, say you delete the watchpoint, or
add/remove watchpoints such that dr_mirror changes while threads
are running. On targets that support non-stop,
inserting/deleting watchpoints updates the global dr_mirror only.
It does not update the real thread's debug registers; that's only
done prior to resume. Instead, 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. Now, say, a thread hit a watchpoint before
having been updated with the new dr_mirror contents, and we
haven't yet handled the corresponding SIGTRAP. If we trusted
dr_mirror below, we'd mistake the real trapped address (from the
last time we had updated debug registers in the thread) with
whatever was currently in dr_mirror. So to fix this, dr_mirror
always represents intention, what we _want_ threads to have in
debug registers. To get at the address and cause of the trap, we
need to read the state the thread still has in its debug
registers.

In sum, always get the current debug register values the current
thread has, instead of trusting the global mirror. If the thread
was running when we last changed watchpoints, the mirror no
longer represents what was set in this thread's debug
registers. */
~~~

This patch makes the Windows native target follow that model as well.

I don't understand why would windows_nat_target::resume want to call
SetThreadContext itself. That duplicates things as it is currently
worrying about setting the debug registers as well. windows_continue
also does that, and windows_nat_target::resume always calls it. So
this patch simplifies windows_nat_target::resume too.

Tromey pointed out that gdb/2388 mentioned in the code being removed
was moved to https://sourceware.org/bugzilla/show_bug.cgi?id=9493 in
the bugzilla migration. I tried the reproducer mentioned there, and
it still works correctly.

Change-Id: Id762d0faa7d5e788402f2ff5adad5352447a7526
---
gdb/nat/windows-nat.h | 1 +
gdb/windows-nat.c | 92 ++++++++++++++++++------------------------
gdbserver/win32-low.cc | 1 +
3 files changed, 42 insertions(+), 52 deletions(-)

diff --git a/gdb/nat/windows-nat.h b/gdb/nat/windows-nat.h
index 2f50742c810..806bd352705 100644
--- a/gdb/nat/windows-nat.h
+++ b/gdb/nat/windows-nat.h
@@ -150,6 +150,7 @@ struct windows_process_info
{
/* The process handle */
HANDLE handle = 0;
+ DWORD process_id = 0;
DWORD main_thread_id = 0;
enum gdb_signal last_sig = GDB_SIGNAL_0;

diff --git a/gdb/windows-nat.c b/gdb/windows-nat.c
index 092cb699a4a..fbe36102e2e 100644
--- a/gdb/windows-nat.c
+++ b/gdb/windows-nat.c
@@ -100,8 +100,6 @@ struct windows_per_inferior : public windows_process_info
void handle_unload_dll () override;
bool handle_access_violation (const EXCEPTION_RECORD *rec) override;

- uintptr_t dr[8] {};
-
int windows_initialization_done = 0;

std::vector<std::unique_ptr<windows_thread_info>> thread_list;
@@ -721,18 +719,6 @@ windows_nat_target::fetch_registers (struct regcache *regcache, int r)
{
context->ContextFlags = WindowsContext<decltype(context)>::all;
CHECK (get_thread_context (th->h, context));
- /* Copy dr values from that thread.
- But only if there were not modified since last stop.
- PR gdb/2388 */
- if (!th->debug_registers_changed)
- {
- windows_process.dr[0] = context->Dr0;
- windows_process.dr[1] = context->Dr1;
- windows_process.dr[2] = context->Dr2;
- windows_process.dr[3] = context->Dr3;
- windows_process.dr[6] = context->Dr6;
- windows_process.dr[7] = context->Dr7;
- }
});

th->reload_context = false;
@@ -1224,18 +1210,21 @@ windows_nat_target::windows_continue (DWORD continue_status, int id,
for (auto &th : windows_process.thread_list)
if (id == -1 || id == (int) th->tid)
{
+ struct x86_debug_reg_state *state
+ = x86_debug_reg_state (windows_process.process_id);
+
windows_process.with_context (th.get (), [&] (auto *context)
{
if (th->debug_registers_changed)
{
context->ContextFlags
|= WindowsContext<decltype(context)>::debug;
- context->Dr0 = windows_process.dr[0];
- context->Dr1 = windows_process.dr[1];
- context->Dr2 = windows_process.dr[2];
- context->Dr3 = windows_process.dr[3];
+ context->Dr0 = state->dr_mirror[0];
+ context->Dr1 = state->dr_mirror[1];
+ context->Dr2 = state->dr_mirror[2];
+ context->Dr3 = state->dr_mirror[3];
context->Dr6 = DR6_CLEAR_VALUE;
- context->Dr7 = windows_process.dr[7];
+ context->Dr7 = state->dr_control_mirror;
th->debug_registers_changed = false;
}
if (context->ContextFlags)
@@ -1374,22 +1363,6 @@ windows_nat_target::resume (ptid_t ptid, int step, enum gdb_signal sig)
fetch_registers (regcache, gdbarch_ps_regnum (gdbarch));
context->EFlags |= FLAG_TRACE_BIT;
}
-
- if (context->ContextFlags)
- {
- if (th->debug_registers_changed)
- {
- context->Dr0 = windows_process.dr[0];
- context->Dr1 = windows_process.dr[1];
- context->Dr2 = windows_process.dr[2];
- context->Dr3 = windows_process.dr[3];
- context->Dr6 = DR6_CLEAR_VALUE;
- context->Dr7 = windows_process.dr[7];
- th->debug_registers_changed = false;
- }
- CHECK (set_thread_context (th->h, context));
- context->ContextFlags = 0;
- }
});
}

@@ -1770,19 +1743,15 @@ windows_nat_target::wait (ptid_t ptid, struct target_waitstatus *ourstatus,
void
windows_nat_target::do_initial_windows_stuff (DWORD pid, bool attaching)
{
- int i;
struct inferior *inf;

windows_process.last_sig = GDB_SIGNAL_0;
windows_process.open_process_used = 0;
- for (i = 0;
- i < sizeof (windows_process.dr) / sizeof (windows_process.dr[0]);
- i++)
- windows_process.dr[i] = 0;
#ifdef __CYGWIN__
windows_process.cygwin_load_start = 0;
windows_process.cygwin_load_end = 0;
#endif
+ windows_process.process_id = pid;
memset (&windows_process.current_event, 0,
sizeof (windows_process.current_event));
inf = current_inferior ();
@@ -3205,7 +3174,6 @@ cygwin_set_dr (int i, CORE_ADDR addr)
{
if (i < 0 || i > 3)
internal_error (_("Invalid register %d in cygwin_set_dr.\n"), i);
- windows_process.dr[i] = addr;

for (auto &th : windows_process.thread_list)
th->debug_registers_changed = true;
@@ -3217,8 +3185,6 @@ cygwin_set_dr (int i, CORE_ADDR addr)
static void
cygwin_set_dr7 (unsigned long val)
{
- windows_process.dr[7] = (CORE_ADDR) val;
-
for (auto &th : windows_process.thread_list)
th->debug_registers_changed = true;
}
@@ -3228,26 +3194,48 @@ cygwin_set_dr7 (unsigned long val)
static CORE_ADDR
cygwin_get_dr (int i)
{
- return windows_process.dr[i];
+ windows_thread_info *th
+ = windows_process.thread_rec (inferior_ptid, DONT_INVALIDATE_CONTEXT);
+
+ return windows_process.with_context (th, [&] (auto *context) -> CORE_ADDR
+ {
+ gdb_assert (context->ContextFlags != 0);
+ switch (i)
+ {
+ case 0:
+ return context->Dr0;
+ case 1:
+ return context->Dr1;
+ case 2:
+ return context->Dr2;
+ case 3:
+ return context->Dr3;
+ case 6:
+ return context->Dr6;
+ case 7:
+ return context->Dr7;
+ };
+
+ gdb_assert_not_reached ("invalid x86 dr register number: %d", i);
+ });
}

-/* Get the value of the DR6 debug status register from the inferior.
- Here we just return the value stored in dr[6]
- by the last call to thread_rec for current_event.dwThreadId id. */
+/* Get the value of the DR6 debug status register from the
+ inferior. */
+
static unsigned long
cygwin_get_dr6 (void)
{
- return (unsigned long) windows_process.dr[6];
+ return cygwin_get_dr (6);
}

-/* Get the value of the DR7 debug status register from the inferior.
- Here we just return the value stored in dr[7] by the last call to
- thread_rec for current_event.dwThreadId id. */
+/* Get the value of the DR7 debug status register from the
+ inferior. */

static unsigned long
cygwin_get_dr7 (void)
{
- return (unsigned long) windows_process.dr[7];
+ return cygwin_get_dr (7);
}

/* Determine if the thread referenced by "ptid" is alive
diff --git a/gdbserver/win32-low.cc b/gdbserver/win32-low.cc
index 89831de9d43..61f124337a5 100644
--- a/gdbserver/win32-low.cc
+++ b/gdbserver/win32-low.cc
@@ -296,6 +296,7 @@ do_initial_child_stuff (HANDLE proch, DWORD pid, int attached)

windows_process.last_sig = GDB_SIGNAL_0;
windows_process.handle = proch;
+ windows_process.process_id = pid;
windows_process.main_thread_id = 0;

windows_process.soft_interrupt_requested = 0;
--
2.49.0