From 7d6c04c5807753e5ad485eea6f4ce0946d4f1443 Mon Sep 17 00:00:00 2001 From: Pedro Alves Date: Tue, 7 May 2024 13:57:47 +0100 Subject: Windows gdb: Watchpoints while running (internal vs external stops) 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 | 116 +++++++++++++++++++++++++++++++++++++++++--------- 2 files changed, 119 insertions(+), 24 deletions(-) diff --git a/gdb/nat/windows-nat.h b/gdb/nat/windows-nat.h index cf6889b..98ea4ba 100644 --- a/gdb/nat/windows-nat.h +++ b/gdb/nat/windows-nat.h @@ -34,6 +34,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 @@ -117,9 +135,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 @@ -128,7 +147,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 dfb5601..494fc1e 100644 --- a/gdb/windows-nat.c +++ b/gdb/windows-nat.c @@ -261,6 +261,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); @@ -521,6 +525,8 @@ struct windows_nat_target final : public x86_nat_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, @@ -528,7 +534,8 @@ private: void delete_thread (ptid_t ptid, DWORD exit_code, bool main_thread_p); DWORD fake_create_process (const DEBUG_EVENT ¤t_event); - void stop_one_thread (windows_thread_info *th); + void stop_one_thread (windows_thread_info *th, + enum stopping_kind stopping_kind); BOOL windows_continue (DWORD continue_status, int id, windows_continue_flags cont_flags = 0); @@ -1295,7 +1302,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); @@ -1608,7 +1615,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; } @@ -1683,8 +1690,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 @@ -1879,11 +1897,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); @@ -1899,12 +1919,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); @@ -1918,7 +1944,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 { @@ -1927,9 +1955,13 @@ windows_nat_target::stop_one_thread (windows_thread_info *th) th->suspend (); gdb_assert (th->suspended); - 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); } } @@ -1943,7 +1975,7 @@ windows_nat_target::stop (ptid_t ptid) { ptid_t thr_ptid (windows_process.process_id, th->tid); if (thr_ptid.matches (ptid)) - stop_one_thread (th.get ()); + stop_one_thread (th.get (), SK_EXTERNAL); } } @@ -2370,6 +2402,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 @@ -2422,7 +2465,7 @@ windows_nat_target::wait (ptid_t ptid, struct target_waitstatus *ourstatus, for (auto &thr : windows_process.thread_list) thr->suspend (); - th->stopping = false; + th->stopping = SK_NOT_STOPPING; } /* If something came out, assume there may be more. This is @@ -3905,6 +3948,41 @@ Use \"file\" or \"dll\" 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 : windows_process.thread_list) + { + 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.get (), 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 (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. @@ -3916,8 +3994,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 : windows_process.thread_list) - th->debug_registers_changed = true; + debug_registers_changed_all_threads (); } /* Pass the value VAL to the inferior in the DR7 debug control @@ -3926,8 +4003,7 @@ windows_set_dr (int i, CORE_ADDR addr) static void windows_set_dr7 (unsigned long val) { - for (auto &th : windows_process.thread_list) - th->debug_registers_changed = true; + debug_registers_changed_all_threads (); } /* Get the value of debug register I from the inferior. */ -- cgit v1.1