From patchwork Tue May 7 23:42:01 2024 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Pedro Alves X-Patchwork-Id: 89683 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 A822238449C6 for ; Tue, 7 May 2024 23:45:00 +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 BD818384AB58 for ; Tue, 7 May 2024 23:42:43 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org BD818384AB58 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 BD818384AB58 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=1715125366; cv=none; b=TAiOvQtk42/oOu1sQPD2KB9ADCjPN52KGvVdx7SKFCXwY7nlng3h8Cbs/YWaPDPALtrKc6srS9212PJgWT48l4qBOrY6/kYhbx8tQiZE5qy0PWa0Po7wQ+Xgh9HGBr/RCe//TX6kbuXtBTWUmp7ny6RMmChlMoHn+hGBZLHbymw= ARC-Message-Signature: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1715125366; c=relaxed/simple; bh=P9GPbwPq3y8EzlwIoHxNfhIZI6A6iCzcYRnxtX+7DGA=; h=From:To:Subject:Date:Message-ID:MIME-Version; b=XYMgXVZqLn7r3fkn2xLT83Dc2ZnLjVsqbU43OI1bLy7FEuCYsGi16BfP3vCMgHbVgz+JfB6K9zwA/4DqZwDTQ3DfEISYATXWZxOGj70MluWsKOj0/6m1FKdk0ij2dHMUVDBQHu4ZEb80cmE0B7L1Abs0OUILE040B7XrP2CZt2M= ARC-Authentication-Results: i=1; server2.sourceware.org Received: by mail-wm1-f41.google.com with SMTP id 5b1f17b1804b1-41b5e74fa83so28503675e9.0 for ; Tue, 07 May 2024 16:42:43 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1715125362; x=1715730162; 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=1p677Zk5yIi79NadEcRl/h8x+WIcnz2ltsZSNBh0K4Y=; b=kPzH6oxYzu+aSF95SgqAKgDEfT+cfFe3TyuzdCCkVGwFZk1ph5VysgtQTsIy8eguLZ HAuNHD8GbVTy+r444AIilokSMQRd2TZ2zcF9PukzSJAKgrktPcQyj2RbvTZ6wZMVSIn0 rUnYu+Dr0jh5N7QVePe4ShLF3R1pSed6AslO0k9np+ttj1DnDs1FFxXNxVNw0nBYAfdH rweXKBklqYMgehY2Zha7FVuD3PWnsR2my2bl/U7CWvH0prDucdqfVOPzPBCK8of3UZKC VCdP1lU0NqCkwNunOavtrIG4zVi2DV7enFgHLsI5OtwZFgU0n8AAsJAZuMtVJyISdeLl XHnA== X-Gm-Message-State: AOJu0Yz1qoj12eoqxfULK3NSBNr2AF1oBN5nSFSDzozAIHeDEXB4FTNH zxxGqOtbyoYleqDpsCHGT97kqrpiiYtdNcPUvN7Kd2vLuSbmKmnVHzKsu2OP X-Google-Smtp-Source: AGHT+IElAE3SIJRclmTLpOKqPYo7VTiaf1NGSYcDwUEgWi6IvTFZX7xe01SBXBkLY4DDrqXH3G9ZCw== X-Received: by 2002:adf:b1d1:0:b0:345:bcd4:bc99 with SMTP id ffacd0b85a97d-34fca149bb2mr879139f8f.11.1715125362205; Tue, 07 May 2024 16:42:42 -0700 (PDT) Received: from localhost ([2001:8a0:f908:4900:2dd1:1a0d:2b75:dc42]) by smtp.gmail.com with UTF8SMTPSA id g20-20020a05600c311400b0041be58cdf83sm205637wmo.4.2024.05.07.16.42.41 for (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Tue, 07 May 2024 16:42:41 -0700 (PDT) From: Pedro Alves To: gdb-patches@sourceware.org Subject: [PATCH 02/34] Windows gdb: Eliminate global current_process.dr[8] global Date: Wed, 8 May 2024 00:42:01 +0100 Message-ID: <20240507234233.371123-3-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.3 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 current_process.dr needs to be per-thread for non-stop. Actually, it doesn't even need to exist at all. We have the 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. Change-Id: I2fe460341b598ad293ea60d5f702b10cefc30711 --- gdb/nat/windows-nat.h | 1 + gdb/windows-nat.c | 151 ++++++++++++++++++----------------------- gdbserver/win32-low.cc | 1 + 3 files changed, 68 insertions(+), 85 deletions(-) diff --git a/gdb/nat/windows-nat.h b/gdb/nat/windows-nat.h index f2b5d777016..3783f39051d 100644 --- a/gdb/nat/windows-nat.h +++ b/gdb/nat/windows-nat.h @@ -144,6 +144,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 5578ae250a6..0102511dc8d 100644 --- a/gdb/windows-nat.c +++ b/gdb/windows-nat.c @@ -97,8 +97,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> thread_list; @@ -730,36 +728,12 @@ windows_nat_target::fetch_registers (struct regcache *regcache, int r) { th->wow64_context.ContextFlags = CONTEXT_DEBUGGER_DR; CHECK (Wow64GetThreadContext (th->h, &th->wow64_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] = th->wow64_context.Dr0; - windows_process.dr[1] = th->wow64_context.Dr1; - windows_process.dr[2] = th->wow64_context.Dr2; - windows_process.dr[3] = th->wow64_context.Dr3; - windows_process.dr[6] = th->wow64_context.Dr6; - windows_process.dr[7] = th->wow64_context.Dr7; - } } else #endif { th->context.ContextFlags = CONTEXT_DEBUGGER_DR; CHECK (GetThreadContext (th->h, &th->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] = th->context.Dr0; - windows_process.dr[1] = th->context.Dr1; - windows_process.dr[2] = th->context.Dr2; - windows_process.dr[3] = th->context.Dr3; - windows_process.dr[6] = th->context.Dr6; - windows_process.dr[7] = th->context.Dr7; - } } th->reload_context = false; } @@ -1283,18 +1257,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); + #ifdef __x86_64__ if (windows_process.wow64_process) { if (th->debug_registers_changed) { th->wow64_context.ContextFlags |= CONTEXT_DEBUG_REGISTERS; - th->wow64_context.Dr0 = windows_process.dr[0]; - th->wow64_context.Dr1 = windows_process.dr[1]; - th->wow64_context.Dr2 = windows_process.dr[2]; - th->wow64_context.Dr3 = windows_process.dr[3]; + th->wow64_context.Dr0 = state->dr_mirror[0]; + th->wow64_context.Dr1 = state->dr_mirror[1]; + th->wow64_context.Dr2 = state->dr_mirror[2]; + th->wow64_context.Dr3 = state->dr_mirror[3]; th->wow64_context.Dr6 = DR6_CLEAR_VALUE; - th->wow64_context.Dr7 = windows_process.dr[7]; + th->wow64_context.Dr7 = state->dr_control_mirror; th->debug_registers_changed = false; } if (th->wow64_context.ContextFlags) @@ -1319,12 +1296,12 @@ windows_nat_target::windows_continue (DWORD continue_status, int id, if (th->debug_registers_changed) { th->context.ContextFlags |= CONTEXT_DEBUG_REGISTERS; - th->context.Dr0 = windows_process.dr[0]; - th->context.Dr1 = windows_process.dr[1]; - th->context.Dr2 = windows_process.dr[2]; - th->context.Dr3 = windows_process.dr[3]; + th->context.Dr0 = state->dr_mirror[0]; + th->context.Dr1 = state->dr_mirror[1]; + th->context.Dr2 = state->dr_mirror[2]; + th->context.Dr3 = state->dr_mirror[3]; th->context.Dr6 = DR6_CLEAR_VALUE; - th->context.Dr7 = windows_process.dr[7]; + th->context.Dr7 = state->dr_control_mirror; th->debug_registers_changed = false; } if (th->context.ContextFlags) @@ -1463,22 +1440,6 @@ windows_nat_target::resume (ptid_t ptid, int step, enum gdb_signal sig) fetch_registers (regcache, gdbarch_ps_regnum (gdbarch)); th->wow64_context.EFlags |= FLAG_TRACE_BIT; } - - if (th->wow64_context.ContextFlags) - { - if (th->debug_registers_changed) - { - th->wow64_context.Dr0 = windows_process.dr[0]; - th->wow64_context.Dr1 = windows_process.dr[1]; - th->wow64_context.Dr2 = windows_process.dr[2]; - th->wow64_context.Dr3 = windows_process.dr[3]; - th->wow64_context.Dr6 = DR6_CLEAR_VALUE; - th->wow64_context.Dr7 = windows_process.dr[7]; - th->debug_registers_changed = false; - } - CHECK (Wow64SetThreadContext (th->h, &th->wow64_context)); - th->wow64_context.ContextFlags = 0; - } } else #endif @@ -1491,22 +1452,6 @@ windows_nat_target::resume (ptid_t ptid, int step, enum gdb_signal sig) fetch_registers (regcache, gdbarch_ps_regnum (gdbarch)); th->context.EFlags |= FLAG_TRACE_BIT; } - - if (th->context.ContextFlags) - { - if (th->debug_registers_changed) - { - th->context.Dr0 = windows_process.dr[0]; - th->context.Dr1 = windows_process.dr[1]; - th->context.Dr2 = windows_process.dr[2]; - th->context.Dr3 = windows_process.dr[3]; - th->context.Dr6 = DR6_CLEAR_VALUE; - th->context.Dr7 = windows_process.dr[7]; - th->debug_registers_changed = false; - } - CHECK (SetThreadContext (th->h, &th->context)); - th->context.ContextFlags = 0; - } } } @@ -1887,19 +1832,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 (); @@ -3321,7 +3262,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; @@ -3333,8 +3273,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; } @@ -3344,26 +3282,69 @@ 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); + +#ifdef __x86_64__ + if (windows_process.wow64_process) + { + gdb_assert (th->wow64_context.ContextFlags != 0); + switch (i) + { + case 0: + return th->wow64_context.Dr0; + case 1: + return th->wow64_context.Dr1; + case 2: + return th->wow64_context.Dr2; + case 3: + return th->wow64_context.Dr3; + case 6: + return th->wow64_context.Dr6; + case 7: + return th->wow64_context.Dr7; + }; + } + else +#endif + { + gdb_assert (th->context.ContextFlags != 0); + switch (i) + { + case 0: + return th->context.Dr0; + case 1: + return th->context.Dr1; + case 2: + return th->context.Dr2; + case 3: + return th->context.Dr3; + case 6: + return th->context.Dr6; + case 7: + return th->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 f672e542d1b..cc314df8dbd 100644 --- a/gdbserver/win32-low.cc +++ b/gdbserver/win32-low.cc @@ -309,6 +309,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;