From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from simark.ca by simark.ca with LMTP id wHReCFQzK2g2NyoAWB0awg (envelope-from ) for ; Mon, 19 May 2025 09:34:12 -0400 Received: by simark.ca (Postfix, from userid 112) id 1EC471E11E; Mon, 19 May 2025 09:34:12 -0400 (EDT) X-Spam-Checker-Version: SpamAssassin 4.0.1 (2024-03-25) on simark.ca X-Spam-Level: X-Spam-Status: No, score=-9.0 required=5.0 tests=ARC_SIGNED,ARC_VALID,BAYES_00, MAILING_LIST_MULTI,RCVD_IN_DNSWL_MED,RCVD_IN_VALIDITY_CERTIFIED, RCVD_IN_VALIDITY_RPBL,RCVD_IN_VALIDITY_SAFE autolearn=ham autolearn_force=no version=4.0.1 Received: from server2.sourceware.org (server2.sourceware.org [8.43.85.97]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature ECDSA (prime256v1) server-digest SHA256) (No client certificate requested) by simark.ca (Postfix) with ESMTPS id 776E71E11B for ; Mon, 19 May 2025 09:34:10 -0400 (EDT) Received: from server2.sourceware.org (localhost [IPv6:::1]) by sourceware.org (Postfix) with ESMTP id 03CA23858410 for ; Mon, 19 May 2025 13:34:09 +0000 (GMT) DKIM-Filter: OpenDKIM Filter v2.11.0 sourceware.org 03CA23858410 Received: from mail-wm1-f50.google.com (mail-wm1-f50.google.com [209.85.128.50]) by sourceware.org (Postfix) with ESMTPS id 120E63858D26 for ; Mon, 19 May 2025 13:25:27 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org 120E63858D26 Authentication-Results: sourceware.org; dmarc=none (p=none dis=none) header.from=palves.net Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=gmail.com ARC-Filter: OpenARC Filter v1.0.0 sourceware.org 120E63858D26 Authentication-Results: server2.sourceware.org; arc=none smtp.remote-ip=209.85.128.50 ARC-Seal: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1747661127; cv=none; b=LNCoQSgNNae5Nf67mys5lKTf1IPlarfgr5NdnNrABST7mxCCY+VvT0eCrvZtSSkKH5/51d/Srd3N8ZYDR682Y0NQi5hPbyzXRCRjLnp1wJir/LBxC86A95xMu1uTgUjRplMXdi+zeH2cQNRs/QCHK/MJXq+O4PKpazA6RLLcA4k= ARC-Message-Signature: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1747661127; c=relaxed/simple; bh=yiSPHJoCuS25nTNVER9LswKn4NyMgH/KeSu9i28GtzY=; h=From:To:Subject:Date:Message-ID:MIME-Version; b=RDTQLvJjGTxtoYCNa0VMP/MXbOHvFEUYqcdsI+aw94X8j7xw0eKI1/lmTnwkgAR4v0niVsp3ZBveRZ3+YMW6KG11t00USoWNHTnt6yqWtrrunrjUAzvlLl24FV1jJahB3ZMLCL9d2J5568M4+nxpJVoQ07qE1/OU+r1b6q7tfDE= ARC-Authentication-Results: i=1; server2.sourceware.org DKIM-Filter: OpenDKIM Filter v2.11.0 sourceware.org 120E63858D26 Received: by mail-wm1-f50.google.com with SMTP id 5b1f17b1804b1-43d04dc73b7so47973275e9.3 for ; Mon, 19 May 2025 06:25:27 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1747661126; x=1748265926; h=content-transfer-encoding:mime-version:references:in-reply-to :message-id:date:subject:to:from:x-gm-message-state:from:to:cc :subject:date:message-id:reply-to; bh=CP4NZZsApc+ZmB2z/mqjszjPj1LwqQWQnO1nOK48Tdo=; b=IWtyyJYhI5+pEu0Fd3upR7Rc709iSD5N9+ziDbfZz5ZtagaNRcDIBluOl1op1QIdZw MiaNsLc1VxF8g82BWc123De+ODGyJEAbi1neTGQee+YD0+iXiHCr34hhw8agWY8oAsse WoAyyEPbP3KaslPx89amSaw4gOxRyP4d3UyT2dwHOjhZYWtuVvMrMx1z6qt03ockgCb7 tE0p1Tw3ZEPZ9h4dMoRZzOhNuSpia6UNSNm04z22qg+EH4mrIJKH0rAjshM/RpmNX5mW FsTn17AX8PJFEUS7WEgBy3nU/RDUqnSkoYbQ2Q3HwWGIgelA0nnoxhTxKtB6arw+MhUM CDmw== X-Gm-Message-State: AOJu0YwOwHnR+LRjFhUQGUYoWNvUIuB8L0n5q0v1GXPbq480TX6rmB6p fxgBNLf18jM+OdXxBHGVYynzOe+WvVuLuqKlFKOcNSYfYLCC5SbxnWCpbj7brx0i X-Gm-Gg: ASbGncspK2sidjPQtqY3VEi51+wIN6kUz2T+re1oUGQpvz0FlQ4L8l1UQbq/5Pmbs3e 5MRHg4ex6SFf8GAHDHc0t9iwMOJqCoUgZcxcMOhqps3x5O4UF8i/X5hwol+xCT+xtOdBGJxVXdw EKspZzKCwWONZwmKI5/fQToot+XQMYkL5IVDy2Q7Zh7S4TbYbJigfJnGfVrP7ay9s+xhINLWfz+ 67TJgBMIdcPTtMCh/CfVMZVkL4HoLzs0poH7NbrJCHocAe395WQmwLSnRHmdTdnvg8JAj+tSQfG 0R0GqzV/CUHg8XLXXiX3lt7HiT37hfm8NPJ99gHaKxGWoGK/vJ8= X-Google-Smtp-Source: AGHT+IFWklplTxvsKUDUKbqaz2igVH4GuHukPFzX5GVYpYhW+RbRPiFunzU+idvMyO2b8TgN+a9/xA== X-Received: by 2002:a05:600c:4e0e:b0:442:faa3:fadb with SMTP id 5b1f17b1804b1-442fd608293mr141074265e9.2.1747661125391; Mon, 19 May 2025 06:25:25 -0700 (PDT) Received: from localhost ([2001:8a0:4fe9:b400:8d90:6f0d:36bf:32df]) by smtp.gmail.com with UTF8SMTPSA id ffacd0b85a97d-3a3698c504csm7841022f8f.59.2025.05.19.06.25.24 for (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Mon, 19 May 2025 06:25:24 -0700 (PDT) From: Pedro Alves To: gdb-patches@sourceware.org Subject: [PATCH v2 41/47] Windows gdb: Watchpoints while running (internal vs external stops) Date: Mon, 19 May 2025 14:23:02 +0100 Message-ID: <20250519132308.3553663-42-pedro@palves.net> X-Mailer: git-send-email 2.49.0 In-Reply-To: <20250519132308.3553663-1-pedro@palves.net> References: <20250519132308.3553663-1-pedro@palves.net> MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-BeenThere: gdb-patches@sourceware.org X-Mailman-Version: 2.1.30 Precedence: list List-Id: Gdb-patches mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: gdb-patches-bounces~public-inbox=simark.ca@sourceware.org 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 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 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); 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 (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