From patchwork Tue May 7 23:42:32 2024 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Pedro Alves X-Patchwork-Id: 89709 Return-Path: X-Original-To: patchwork@sourceware.org Delivered-To: patchwork@sourceware.org Received: from server2.sourceware.org (localhost [IPv6:::1]) by sourceware.org (Postfix) with ESMTP id 547F5384AB6F for ; Tue, 7 May 2024 23:48:18 +0000 (GMT) X-Original-To: gdb-patches@sourceware.org Delivered-To: gdb-patches@sourceware.org Received: from mail-wm1-f41.google.com (mail-wm1-f41.google.com [209.85.128.41]) by sourceware.org (Postfix) with ESMTPS id BFC2C3858D37 for ; Tue, 7 May 2024 23:44:40 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org BFC2C3858D37 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 BFC2C3858D37 Authentication-Results: server2.sourceware.org; arc=none smtp.remote-ip=209.85.128.41 ARC-Seal: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1715125486; cv=none; b=wk0nFoeGIG93rPt21zNzcTivbxD3P9DMEN6PDS9MOF+ncPScOBfFHgrncoW4WcZEvtePXhr+UhxAXPd0i0j3UjQCU5UfWGTQMRkZbzG7M+xSJyVfKT5YixRlMNaqPuUfzoOJmv9YJrgOYyQeCiG5RrdZoOhAvd45rE8MTL2PmMA= ARC-Message-Signature: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1715125486; c=relaxed/simple; bh=9hwn77Kb3HK81nJK5vFuaveeUSf+2T6bpUs4pq5JSoE=; h=From:To:Subject:Date:Message-ID:MIME-Version; b=QbWygap2pUPIOY/eiuqcaMOuLtBm4ixJPSG2afcbebpVKoRuzQ9FGn5pJ4PMk+q1ijU7nUdEvTC3DgDGSo9mK8iHx/3UoYgoln3TdQvtkMVyH2zPtAn14wZZbZTXsMCNwkC1bvuMTeJDO+elDb1bH5tvoxDsFddQZo/2Lgee0xo= ARC-Authentication-Results: i=1; server2.sourceware.org Received: by mail-wm1-f41.google.com with SMTP id 5b1f17b1804b1-41ba1ba55ebso27659465e9.1 for ; Tue, 07 May 2024 16:44:40 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1715125479; x=1715730279; 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=sZdCOoO9dK+lcYiZ25CHd8s/o++TDtsrZHPg4YZ9IFk=; b=PceJSFJGNMOpzLreIQ1+7X+87ZRSJRNr4t//EXAOzBIhzSFA0Zw7lwVxCIUvuIGdFx ju1v1mehHaK1CROzSSlamsh+X5kLWHtr3lPn71kPLdVJEDUU0R/7Aax0CuSJkFtuIuMJ o5Y0mDlodFzr/NAQ6wqVyxJL9/zLlkuyUgqMTqfjyj4o1Jolbq161HvKNVIqbuvLzO5o nAL8OkiL6CzIOwBzl666HtuvYlgZciV+RWoXV+LpWVWn31QPGlGSFP+pIc6JRiTFO7RS Whuo4xtM0wWjbF9UF8CD0OHONDVLjDZbVWUGpy3STmykFCd6g82n2ZK+tDwCKymp+/Z8 ZbFQ== X-Gm-Message-State: AOJu0YwLgTzSiCUnZ4sjZnD+pzF3SYcrGyqSC0jsxtqX0jATdWgxC5zk NEjqC0DmVfDwgnKCLYkYHTYerBq5bT5Rhppx/cBDHURkLwBTYQtFzzLOVwdT X-Google-Smtp-Source: AGHT+IEMSC2XxaAF6UFv8F0c3ruqjczv4SNDpvtwOv6Uh//hNaK0i4O+OiDx4VTjX4YfPr3c+bVSdg== X-Received: by 2002:a05:600c:384a:b0:415:4b4b:1e28 with SMTP id 5b1f17b1804b1-41f71ec27e1mr8349205e9.20.1715125479176; Tue, 07 May 2024 16:44:39 -0700 (PDT) Received: from localhost ([2001:8a0:f908:4900:2dd1:1a0d:2b75:dc42]) by smtp.gmail.com with UTF8SMTPSA id h18-20020a056000001200b0034c78001f6asm13890353wrx.109.2024.05.07.16.44.38 for (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Tue, 07 May 2024 16:44:38 -0700 (PDT) From: Pedro Alves To: gdb-patches@sourceware.org Subject: [PATCH 33/34] Windows gdb: Watchpoints while running (internal vs external stops) Date: Wed, 8 May 2024 00:42:32 +0100 Message-ID: <20240507234233.371123-34-pedro@palves.net> X-Mailer: git-send-email 2.43.2 In-Reply-To: <20240507234233.371123-1-pedro@palves.net> References: <20240507234233.371123-1-pedro@palves.net> MIME-Version: 1.0 X-Spam-Status: No, score=-10.8 required=5.0 tests=BAYES_00, FREEMAIL_FORGED_FROMDOMAIN, FREEMAIL_FROM, GIT_PATCH_0, HEADER_FROM_DIFFERENT_DOMAINS, KAM_DMARC_STATUS, RCVD_IN_DNSWL_NONE, RCVD_IN_MSPIKE_H2, SPF_HELO_NONE, SPF_PASS, TXREP autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on server2.sourceware.org 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+patchwork=sourceware.org@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 | 116 ++++++++++++++++++++++++++++++++++-------- 2 files changed, 119 insertions(+), 24 deletions(-) diff --git a/gdb/nat/windows-nat.h b/gdb/nat/windows-nat.h index c6d0c4e98dd..8399ed7b3b7 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 a33ef868566..cb1d030d12b 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 @@ 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); BOOL windows_continue (DWORD continue_status, int id, windows_continue_flags cont_flags = 0); @@ -1294,7 +1301,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); @@ -1605,7 +1612,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; } @@ -1680,8 +1687,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 @@ -1876,11 +1894,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); @@ -1896,12 +1916,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); @@ -1915,7 +1941,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 { @@ -1924,9 +1952,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); } } @@ -1940,7 +1972,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); } } @@ -2367,6 +2399,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 @@ -2419,7 +2462,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 @@ -3902,6 +3945,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. @@ -3913,8 +3991,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 @@ -3923,8 +4000,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. */