From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-wr1-f41.google.com (mail-wr1-f41.google.com [209.85.221.41]) by sourceware.org (Postfix) with ESMTPS id 4D652384AB6C for ; Tue, 7 May 2024 23:43:29 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org 4D652384AB6C 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 4D652384AB6C Authentication-Results: server2.sourceware.org; arc=none smtp.remote-ip=209.85.221.41 ARC-Seal: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1715125413; cv=none; b=qlPW+bW37kpnLthXuSKz7P45v8Ot4pD9I234zPgBTC7CGw5RD0/ul/G1SXB90MWr35uM/KusWE4Q4l3ZcvzmHnrzQTFixZDq/ufphM6uKgP/Pkf4c8xFBae30CWCTjlGrV/dPgk1/NQs3nb31fgCMNPeM/4dJl9itWcEJSi5VFU= ARC-Message-Signature: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1715125413; c=relaxed/simple; bh=SzZ4GMPXiDv85kwdGBgLmG0gkA5CyVgsp7jYTp/4RRI=; h=From:To:Subject:Date:Message-ID:MIME-Version; b=sGvl42jvFys9Hmef4x+oP2jT99vYPU9+c5KZwQgsD/N4ZsFQtURmc1/haF9QN1LQ4Ad6zrP88ZLXOWAxuVeewaJ8++aKVqVcbYgjeP0UZkhuS4OoiWHgZJ/ZCzmNw3szd+D76DvrcovA8S+Uv3nhQTEqeCzQXROKzR301FpCEjE= ARC-Authentication-Results: i=1; server2.sourceware.org Received: by mail-wr1-f41.google.com with SMTP id ffacd0b85a97d-34db6a29998so2180347f8f.0 for ; Tue, 07 May 2024 16:43:29 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1715125408; x=1715730208; 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=/W40Mih2zdk96BZl7b2AKeMffwoMKbtl19yK5mtnHi0=; b=xGUVObvcsSwfufzY8T/OdcJzFJ1RqIHOPe4YQcjn5Ih+6T5jw+FJcv+ImL4+Ro0S++ 36mqPmvPsxA936XKLvo1j2XdXlCvjn5+XFYOMQMZc6VfrOFmzTa0Agi5IYBxePHe5eKY GJnqqm7ljFGJB7xTNbxBr8sbSh7wN36Hv82AC0DDXXA0aLsEx4elpke0p0TeNDXbHsp3 k38ciONSk6ujyc+10hXpDJ4C20E1wka87Yy9D3EuwQ8W0zfEc1KXV7qGjYSMsQJ6QW0T CAYGODaDva6+v1ecoWcB0MXhvPKHi2sIWGvAuHoD0+URIT5Uh1cjSBj3BS3f1iRi9Uj1 CmbQ== X-Gm-Message-State: AOJu0YxtmpU34GqwPWuGqM0SgQNh8l5plIm43UqI7OOzoYMBsRq8TYrQ yB5HZavpaPcEigJVEvC4q8CI3Yk9FBS9Z6v4BsXUI0iqA8QOhD1sEQCN/Tjx X-Google-Smtp-Source: AGHT+IGV3gEYBggaVLzNYubyTifWcKcLAJu5PUJD2fPKs8nFi/agQm4I02MSMR9zgVmo7JKZQ7w42g== X-Received: by 2002:a05:6000:a8c:b0:34d:354:b9c1 with SMTP id ffacd0b85a97d-34fca80e9b3mr803583f8f.57.1715125407761; Tue, 07 May 2024 16:43:27 -0700 (PDT) Received: from localhost ([2001:8a0:f908:4900:2dd1:1a0d:2b75:dc42]) by smtp.gmail.com with UTF8SMTPSA id q27-20020adfab1b000000b0034e24be18a1sm14030198wrc.15.2024.05.07.16.43.27 for (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Tue, 07 May 2024 16:43:27 -0700 (PDT) From: Pedro Alves To: gdb-patches@sourceware.org Subject: [PATCH 15/34] Windows gdb+gdbserver: Elim desired_stop_thread_id / rework pending_stops Date: Wed, 8 May 2024 00:42:14 +0100 Message-ID: <20240507234233.371123-16-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 Content-Transfer-Encoding: 8bit X-Spam-Status: No, score=-10.7 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 List-Id: windows_process.desired_stop_thread_id doesn't work for non-stop, as in that case every thread will have its own independent WaitForDebugEvent event. Instead, detect whether we have been reported a stop that was not supposed to be reported by simply checking whether the thread that is reporting the event is suspended. This is now easilly possible since each thread's suspend state is kept in sync with whether infrun wants the thread executing or not. windows_process.desired_stop_thread_id was also used as thread to pass to windows_continue. However, we don't really need that either. windows_continue is used to update the thread's registers, unsuspend them, and then finally call ContinueDebugEvent. In most cases, we only need the ContinueDebugEvent step, so we can convert the windows_continue calls to continue_last_debug_event_main_thread calls. The exception is when we see a thread creation event -- in that case, we need to update the debug registers of the new thread. We can use continue_one_thread for that. Since the pending stop is now stored in windows_thread_info, get_windows_debug_event needs to avoid reaching the bottom code if there's no thread associated with the event anymore (i.e., EXIT_THREAD_DEBUG_EVENT / EXIT_PROCESS_DEBUG_EVENT). I considered whether it would be possible to keep the pending_stop handling code shared in gdb/nat/windows-nat.c, in this patch and throughout the series, but I conclused that it isn't worth it, until gdbserver is taught about async and non-stop as well. The pending_stop struct will eventually be eliminated later down the series. Change-Id: Ib7c8e8d16edc0900b7c411976c5d70cf93931c1c --- gdb/nat/windows-nat.c | 51 ------------------- gdb/nat/windows-nat.h | 71 ++++++++++---------------- gdb/windows-nat.c | 111 ++++++++++++++++++++++++----------------- gdbserver/win32-low.cc | 53 ++++++++++++-------- 4 files changed, 123 insertions(+), 163 deletions(-) diff --git a/gdb/nat/windows-nat.c b/gdb/nat/windows-nat.c index b5cfad6274b..a01d7b3c0c0 100644 --- a/gdb/nat/windows-nat.c +++ b/gdb/nat/windows-nat.c @@ -666,57 +666,6 @@ windows_process_info::add_all_dlls () /* See nat/windows-nat.h. */ -bool -windows_process_info::matching_pending_stop (bool debug_events) -{ - /* If there are pending stops, and we might plausibly hit one of - them, we don't want to actually continue the inferior -- we just - want to report the stop. In this case, we just pretend to - continue. See the comment by the definition of "pending_stops" - for details on why this is needed. */ - for (const auto &item : pending_stops) - { - if (desired_stop_thread_id == -1 - || desired_stop_thread_id == item.thread_id) - { - DEBUG_EVENTS ("pending stop anticipated, desired=0x%x, item=0x%x", - desired_stop_thread_id, item.thread_id); - return true; - } - } - - return false; -} - -/* See nat/windows-nat.h. */ - -std::optional -windows_process_info::fetch_pending_stop (bool debug_events) -{ - std::optional result; - for (auto iter = pending_stops.begin (); - iter != pending_stops.end (); - ++iter) - { - if (desired_stop_thread_id == -1 - || desired_stop_thread_id == iter->thread_id) - { - result = *iter; - current_event = iter->event; - - DEBUG_EVENTS ("pending stop found in 0x%x (desired=0x%x)", - iter->thread_id, desired_stop_thread_id); - - pending_stops.erase (iter); - break; - } - } - - return result; -} - -/* See nat/windows-nat.h. */ - BOOL continue_last_debug_event (DWORD continue_status, bool debug_events) { diff --git a/gdb/nat/windows-nat.h b/gdb/nat/windows-nat.h index fdbab0fc566..c268a12fd8f 100644 --- a/gdb/nat/windows-nat.h +++ b/gdb/nat/windows-nat.h @@ -32,6 +32,21 @@ namespace windows_nat { +/* Info about a potential pending stop. Each thread holds one of + these. See "windows_thread_info::pending_stop" for more + information. */ +struct pending_stop +{ + /* The target waitstatus we computed. TARGET_WAITKIND_IGNORE if the + thread does not have a pending stop. */ + target_waitstatus status; + + /* The event. A few fields of this can be referenced after a stop, + and it seemed simplest to store the entire event. */ + DEBUG_EVENT event; +}; + + /* Thread information structure used to track extra information about each thread. */ struct windows_thread_info @@ -71,6 +86,18 @@ struct windows_thread_info was not. */ int suspended = 0; +/* Info about a potential pending stop. + + Sometimes, Windows will report a stop on a thread that has been + ostensibly suspended. We believe what happens here is that two + threads hit a breakpoint simultaneously, and the Windows kernel + queues the stop events. However, this can result in the strange + effect of trying to single step thread A -- leaving all other + threads suspended -- and then seeing a stop in thread B. To handle + this scenario, we queue all such "pending" stops here, and then + process them once the step has completed. See PR gdb/22992. */ + struct pending_stop pending_stop {}; + /* The context of the thread, including any manipulations. */ union { @@ -97,22 +124,6 @@ struct windows_thread_info gdb::unique_xmalloc_ptr name; }; - -/* A single pending stop. See "pending_stops" for more - information. */ -struct pending_stop -{ - /* The thread id. */ - DWORD thread_id; - - /* The target waitstatus we computed. */ - target_waitstatus status; - - /* The event. A few fields of this can be referenced after a stop, - and it seemed simplest to store the entire event. */ - DEBUG_EVENT event; -}; - enum handle_exception_result { HANDLE_EXCEPTION_UNHANDLED = 0, @@ -136,22 +147,6 @@ struct windows_process_info stop. */ DEBUG_EVENT current_event {}; - /* The ID of the thread for which we anticipate a stop event. - Normally this is -1, meaning we'll accept an event in any - thread. */ - DWORD desired_stop_thread_id = -1; - - /* A vector of pending stops. Sometimes, Windows will report a stop - on a thread that has been ostensibly suspended. We believe what - happens here is that two threads hit a breakpoint simultaneously, - and the Windows kernel queues the stop events. However, this can - result in the strange effect of trying to single step thread A -- - leaving all other threads suspended -- and then seeing a stop in - thread B. To handle this scenario, we queue all such "pending" - stops here, and then process them once the step has completed. See - PR gdb/22992. */ - std::vector pending_stops; - /* Contents of $_siginfo */ EXCEPTION_RECORD siginfo_er {}; @@ -217,18 +212,6 @@ struct windows_process_info void add_all_dlls (); - /* Return true if there is a pending stop matching - desired_stop_thread_id. If DEBUG_EVENTS is true, logging will be - enabled. */ - - bool matching_pending_stop (bool debug_events); - - /* See if a pending stop matches DESIRED_STOP_THREAD_ID. If so, - remove it from the list of pending stops, set 'current_event', and - return it. Otherwise, return an empty optional. */ - - std::optional fetch_pending_stop (bool debug_events); - const char *pid_to_exec_file (int); private: diff --git a/gdb/windows-nat.c b/gdb/windows-nat.c index 196f6a1f72d..888f85b1862 100644 --- a/gdb/windows-nat.c +++ b/gdb/windows-nat.c @@ -1352,15 +1352,22 @@ BOOL windows_nat_target::windows_continue (DWORD continue_status, int id, windows_continue_flags cont_flags) { - windows_process.desired_stop_thread_id = id; - - if (windows_process.matching_pending_stop (debug_events)) + for (auto &th : windows_process.thread_list) { - /* There's no need to really continue, because there's already - another event pending. However, we do need to inform the - event loop of this. */ - serial_event_set (m_wait_event); - return TRUE; + if ((id == -1 || id == (int) th->tid) + && !th->suspended + && th->pending_stop.status.kind () != TARGET_WAITKIND_IGNORE) + { + DEBUG_EVENTS ("got matching pending stop event " + "for 0x%x, not resuming", + th->tid); + + /* There's no need to really continue, because there's already + another event pending. However, we do need to inform the + event loop of this. */ + serial_event_set (m_wait_event); + return TRUE; + } } for (auto &th : windows_process.thread_list) @@ -1547,20 +1554,24 @@ windows_nat_target::get_windows_debug_event DWORD thread_id = 0; /* If there is a relevant pending stop, report it now. See the - comment by the definition of "pending_stops" for details on why - this is needed. */ - std::optional stop - = windows_process.fetch_pending_stop (debug_events); - if (stop.has_value ()) + comment by the definition of "windows_thread_info::pending_stop" + for details on why this is needed. */ + for (auto &th : windows_process.thread_list) { - thread_id = stop->thread_id; - *ourstatus = stop->status; - windows_process.current_event = stop->event; + if (!th->suspended + && th->pending_stop.status.kind () != TARGET_WAITKIND_IGNORE) + { + DEBUG_EVENTS ("reporting pending event for 0x%x", th->tid); + + thread_id = th->tid; + *ourstatus = th->pending_stop.status; + th->pending_stop.status.set_ignore (); + windows_process.current_event = th->pending_stop.event; - ptid_t ptid (windows_process.current_event.dwProcessId, thread_id); - windows_thread_info *th = windows_process.find_thread (ptid); - windows_process.invalidate_context (th); - return ptid; + ptid_t ptid (windows_process.process_id, thread_id); + windows_process.invalidate_context (th.get ()); + return ptid; + } } windows_process.last_sig = GDB_SIGNAL_0; @@ -1602,12 +1613,18 @@ windows_nat_target::get_windows_debug_event } /* Record the existence of this thread. */ thread_id = current_event->dwThreadId; - add_thread - (ptid_t (current_event->dwProcessId, current_event->dwThreadId, 0), - current_event->u.CreateThread.hThread, - current_event->u.CreateThread.lpThreadLocalBase, - false /* main_thread_p */); + { + windows_thread_info *th + = (add_thread + (ptid_t (current_event->dwProcessId, current_event->dwThreadId, 0), + current_event->u.CreateThread.hThread, + current_event->u.CreateThread.lpThreadLocalBase, + false /* main_thread_p */)); + + /* This updates debug registers if necessary. */ + windows_process.continue_one_thread (th, 0); + } break; case EXIT_THREAD_DEBUG_EVENT: @@ -1619,6 +1636,7 @@ windows_nat_target::get_windows_debug_event current_event->dwThreadId, 0), current_event->u.ExitThread.dwExitCode, false /* main_thread_p */); + thread_id = 0; break; case CREATE_PROCESS_DEBUG_EVENT: @@ -1669,8 +1687,7 @@ windows_nat_target::get_windows_debug_event ourstatus->set_exited (exit_status); else ourstatus->set_signalled (gdb_signal_from_host (exit_signal)); - - thread_id = current_event->dwThreadId; + return ptid_t (current_event->dwProcessId); } break; @@ -1760,17 +1777,22 @@ windows_nat_target::get_windows_debug_event if (!thread_id || windows_process.saw_create != 1) { - CHECK (windows_continue (continue_status, - windows_process.desired_stop_thread_id, 0)); + continue_last_debug_event_main_thread + (_("Failed to resume program execution"), continue_status); + ourstatus->set_ignore (); + return null_ptid; } - else if (windows_process.desired_stop_thread_id != -1 - && windows_process.desired_stop_thread_id != thread_id) + + const ptid_t ptid = ptid_t (current_event->dwProcessId, thread_id, 0); + windows_thread_info *th = windows_process.find_thread (ptid); + + if (th->suspended) { /* Pending stop. See the comment by the definition of "pending_stops" for details on why this is needed. */ DEBUG_EVENTS ("get_windows_debug_event - " - "unexpected stop in 0x%x (expecting 0x%x)", - thread_id, windows_process.desired_stop_thread_id); + "unexpected stop in suspended thread 0x%x", + thread_id); if (current_event->dwDebugEventCode == EXCEPTION_DEBUG_EVENT && ((current_event->u.Exception.ExceptionRecord.ExceptionCode @@ -1779,24 +1801,19 @@ windows_nat_target::get_windows_debug_event == STATUS_WX86_BREAKPOINT)) && windows_process.windows_initialization_done) { - ptid_t ptid = ptid_t (current_event->dwProcessId, thread_id, 0); - windows_thread_info *th = windows_process.find_thread (ptid); th->stopped_at_software_breakpoint = true; th->pc_adjusted = false; } - windows_process.pending_stops.push_back - ({thread_id, *ourstatus, windows_process.current_event}); - thread_id = 0; - CHECK (windows_continue (continue_status, - windows_process.desired_stop_thread_id, 0)); - } - - if (thread_id == 0) - { + th->pending_stop.status = *ourstatus; + th->pending_stop.event = *current_event; ourstatus->set_ignore (); + + continue_last_debug_event_main_thread + (_("Failed to resume program execution"), continue_status); return null_ptid; } - return ptid_t (windows_process.current_event.dwProcessId, thread_id, 0); + + return ptid; } /* Wait for interesting events to occur in the target process. */ @@ -1822,8 +1839,8 @@ windows_nat_target::wait (ptid_t ptid, struct target_waitstatus *ourstatus, if (ourstatus->kind () == TARGET_WAITKIND_SPURIOUS) { - CHECK (windows_continue (DBG_CONTINUE, - windows_process.desired_stop_thread_id, 0)); + continue_last_debug_event_main_thread + (_("Failed to resume program execution"), DBG_CONTINUE); } else if (ourstatus->kind () != TARGET_WAITKIND_IGNORE) { diff --git a/gdbserver/win32-low.cc b/gdbserver/win32-low.cc index 8dd8f21330d..8f51dd67dc2 100644 --- a/gdbserver/win32-low.cc +++ b/gdbserver/win32-low.cc @@ -427,9 +427,14 @@ continue_one_thread (thread_info *thread, int thread_id) static BOOL child_continue (DWORD continue_status, int thread_id) { - windows_process.desired_stop_thread_id = thread_id; - if (windows_process.matching_pending_stop (debug_threads)) - return TRUE; + for (thread_info *thread : all_threads) + { + auto *th = (windows_thread_info *) thread_target_data (thread); + if ((thread_id == -1 || thread_id == (int) th->tid) + && !th->suspended + && th->pending_stop.status.kind () != TARGET_WAITKIND_IGNORE) + return TRUE; + } /* The inferior will only continue after the ContinueDebugEvent call. */ @@ -1012,15 +1017,20 @@ get_child_debug_event (DWORD *continue_status, windows_process.attaching = 0; { - std::optional stop - = windows_process.fetch_pending_stop (debug_threads); - if (stop.has_value ()) + for (thread_info *thread : all_threads) { - *ourstatus = stop->status; - windows_process.current_event = stop->event; - ptid = debug_event_ptid (&windows_process.current_event); - switch_to_thread (find_thread_ptid (ptid)); - return 1; + auto *th = (windows_thread_info *) thread_target_data (thread); + + if (!th->suspended + && th->pending_stop.status.kind () != TARGET_WAITKIND_IGNORE) + { + *ourstatus = th->pending_stop.status; + th->pending_stop.status.set_ignore (); + windows_process.current_event = th->pending_stop.event; + ptid = debug_event_ptid (&windows_process.current_event); + switch_to_thread (find_thread_ptid (ptid)); + return 1; + } } /* Keep the wait time low enough for comfortable remote @@ -1112,7 +1122,7 @@ get_child_debug_event (DWORD *continue_status, else ourstatus->set_signalled (gdb_signal_from_host (exit_signal)); } - child_continue (DBG_CONTINUE, windows_process.desired_stop_thread_id); + continue_last_debug_event (DBG_CONTINUE, debug_threads); break; case LOAD_DLL_DEBUG_EVENT: @@ -1169,17 +1179,19 @@ get_child_debug_event (DWORD *continue_status, ptid = debug_event_ptid (&windows_process.current_event); - if (windows_process.desired_stop_thread_id != -1 - && windows_process.desired_stop_thread_id != ptid.lwp ()) + windows_thread_info *th = windows_process.find_thread (ptid); + + if (th != nullptr && th->suspended) { /* Pending stop. See the comment by the definition of - "pending_stops" for details on why this is needed. */ + "windows_thread_info::pending_stop" for details on why this + is needed. */ OUTMSG2 (("get_windows_debug_event - " - "unexpected stop in 0x%lx (expecting 0x%x)\n", - ptid.lwp (), windows_process.desired_stop_thread_id)); + "unexpected stop in suspended thread 0x%x\n", + th->tid)); maybe_adjust_pc (); - windows_process.pending_stops.push_back - ({(DWORD) ptid.lwp (), *ourstatus, *current_event}); + th->pending_stop.status = *ourstatus; + th->pending_stop.event = *current_event; ourstatus->set_spurious (); } else @@ -1239,8 +1251,7 @@ win32_process_target::wait (ptid_t ptid, target_waitstatus *ourstatus, [[fallthrough]]; case TARGET_WAITKIND_SPURIOUS: /* do nothing, just continue */ - child_continue (continue_status, - windows_process.desired_stop_thread_id); + continue_last_debug_event (continue_status, debug_threads); break; } } -- 2.43.2