From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from rock.gnat.com (rock.gnat.com [IPv6:2620:20:4000:0:a9e:1ff:fe9b:1d1]) by sourceware.org (Postfix) with ESMTP id B9A7E3AAA048 for ; Fri, 13 Mar 2020 19:09:00 +0000 (GMT) Received: from localhost (localhost.localdomain [127.0.0.1]) by filtered-rock.gnat.com (Postfix) with ESMTP id B108B561B7; Fri, 13 Mar 2020 15:09:00 -0400 (EDT) X-Virus-Scanned: Debian amavisd-new at gnat.com Received: from rock.gnat.com ([127.0.0.1]) by localhost (rock.gnat.com [127.0.0.1]) (amavisd-new, port 10024) with LMTP id 57arvOq0Z1Kn; Fri, 13 Mar 2020 15:09:00 -0400 (EDT) Received: from murgatroyd.Home (184-96-250-69.hlrn.qwest.net [184.96.250.69]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by rock.gnat.com (Postfix) with ESMTPSA id 6CB85561B0; Fri, 13 Mar 2020 15:09:00 -0400 (EDT) From: Tom Tromey To: gdb-patches@sourceware.org Cc: Tom Tromey Subject: [PATCH v3 11/29] Handle pending stops from the Windows kernel Date: Fri, 13 Mar 2020 13:08:37 -0600 Message-Id: <20200313190855.28662-12-tromey@adacore.com> X-Mailer: git-send-email 2.21.1 In-Reply-To: <20200313190855.28662-1-tromey@adacore.com> References: <20200313190855.28662-1-tromey@adacore.com> MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Spam-Status: No, score=-25.0 required=5.0 tests=GIT_PATCH_0, GIT_PATCH_1, GIT_PATCH_2, GIT_PATCH_3, SPF_HELO_NONE, SPF_PASS autolearn=ham autolearn_force=no version=3.4.2 X-Spam-Checker-Version: SpamAssassin 3.4.2 (2018-09-13) on server2.sourceware.org X-BeenThere: gdb-patches@sourceware.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Gdb-patches mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 13 Mar 2020 19:09:01 -0000 PR gdb/22992 concerns an assertion failure in gdb when debugging a certain inferior: int finish_step_over(execution_control_state*): Assertion `ecs->event_thread->control.trap_expected' failed. Initially the investigation centered on the discovery that gdb was not suspending other threads when attempting to single-step. This oversight is corrected in this patch: now, when stepping a thread, gdb will call SuspendThread on all other threads. However, the bug persisted even after this change. In particular, WaitForDebugEvent could see a stop for a thread that was ostensibly suspended. Our theory of what is happening here is that there are actually simultaneous breakpoint hits, and the Windows kernel queues the events, causing the second stop to be reported on a suspended thread. In Windows 10 or later gdb could use the DBG_REPLY_LATER flag to ContinueDebugEvent to request that such events be re-reported later. However, relying on that did not seem advisable, so this patch instead arranges to queue such "pending" stops, and then to report them later, once the step has completed. In the PR, Pedro pointed out that it's best in this scenario to implement the stopped_by_sw_breakpoint method, so this patch does this as well. gdb/ChangeLog 2020-03-13 Tom Tromey PR gdb/22992 * windows-nat.c (current_event): Update comment. (last_wait_event, desired_stop_thread_id): New globals. (struct pending_stop): New. (pending_stops): New global. (windows_nat_target) : New methods. (windows_fetch_one_register): Add assertions. Adjust PC. (windows_continue): Handle pending stops. Suspend other threads when stepping. Use last_wait_event (wait_for_debug_event): New function. (get_windows_debug_event): Use wait_for_debug_event. Handle pending stops. Queue spurious stops. (windows_nat_target::wait): Set stopped_at_software_breakpoint. (windows_nat_target::kill): Use wait_for_debug_event. * nat/windows-nat.h (struct windows_thread_info) : New field. * nat/windows-nat.c (windows_thread_info::resume): Clear stopped_at_software_breakpoint. --- gdb/ChangeLog | 22 +++++ gdb/nat/windows-nat.c | 2 + gdb/nat/windows-nat.h | 4 + gdb/windows-nat.c | 200 +++++++++++++++++++++++++++++++++++++++--- 4 files changed, 215 insertions(+), 13 deletions(-) diff --git a/gdb/nat/windows-nat.c b/gdb/nat/windows-nat.c index a98ff421e6f..767ed8c192f 100644 --- a/gdb/nat/windows-nat.c +++ b/gdb/nat/windows-nat.c @@ -49,6 +49,8 @@ windows_thread_info::resume () { if (suspended > 0) { + stopped_at_software_breakpoint = false; + if (ResumeThread (h) == (DWORD) -1) { DWORD err = GetLastError (); diff --git a/gdb/nat/windows-nat.h b/gdb/nat/windows-nat.h index 695f801c58f..224ae5c536c 100644 --- a/gdb/nat/windows-nat.h +++ b/gdb/nat/windows-nat.h @@ -77,6 +77,10 @@ struct windows_thread_info inferior thread. */ bool reload_context = false; + /* True if this thread is currently stopped at a software + breakpoint. This is used to offset the PC when needed. */ + bool stopped_at_software_breakpoint = false; + /* The name of the thread, allocated by xmalloc. */ gdb::unique_xmalloc_ptr name; }; diff --git a/gdb/windows-nat.c b/gdb/windows-nat.c index cdc24ec7ebd..e48fa57f74a 100644 --- a/gdb/windows-nat.c +++ b/gdb/windows-nat.c @@ -249,8 +249,17 @@ static std::vector thread_list; /* The process and thread handles for the above context. */ -static DEBUG_EVENT current_event; /* The current debug event from - WaitForDebugEvent */ +/* The current debug event from WaitForDebugEvent or from a pending + stop. */ +static DEBUG_EVENT current_event; + +/* The most recent event from WaitForDebugEvent. Unlike + current_event, this is guaranteed never to come from a pending + stop. This is important because only data from the most recent + event from WaitForDebugEvent can be used when calling + ContinueDebugEvent. */ +static DEBUG_EVENT last_wait_event; + static HANDLE current_process_handle; /* Currently executing process */ static windows_thread_info *current_thread; /* Info on currently selected thread */ static EXCEPTION_RECORD siginfo_er; /* Contents of $_siginfo */ @@ -325,6 +334,37 @@ static const struct xlate_exception xlate[] = #endif /* 0 */ +/* 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. */ +static DWORD desired_stop_thread_id = -1; + +/* 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; +}; + +/* 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. */ +static std::vector pending_stops; + struct windows_nat_target final : public x86_nat_target { void close () override; @@ -343,6 +383,16 @@ struct windows_nat_target final : public x86_nat_target void fetch_registers (struct regcache *, int) override; void store_registers (struct regcache *, int) override; + bool stopped_by_sw_breakpoint () override + { + return current_thread->stopped_at_software_breakpoint; + } + + bool supports_stopped_by_sw_breakpoint () override + { + return true; + } + enum target_xfer_status xfer_partial (enum target_object object, const char *annex, gdb_byte *readbuf, @@ -613,6 +663,10 @@ windows_fetch_one_register (struct regcache *regcache, struct gdbarch *gdbarch = regcache->arch (); struct gdbarch_tdep *tdep = gdbarch_tdep (gdbarch); + gdb_assert (!gdbarch_read_pc_p (gdbarch)); + gdb_assert (gdbarch_pc_regnum (gdbarch) >= 0); + gdb_assert (!gdbarch_write_pc_p (gdbarch)); + if (r == I387_FISEG_REGNUM (tdep)) { long l = *((long *) context_offset) & 0xffff; @@ -632,7 +686,29 @@ windows_fetch_one_register (struct regcache *regcache, regcache->raw_supply (r, (char *) &l); } else - regcache->raw_supply (r, context_offset); + { + if (th->stopped_at_software_breakpoint + && r == gdbarch_pc_regnum (gdbarch)) + { + int size = register_size (gdbarch, r); + if (size == 4) + { + uint32_t value; + memcpy (&value, context_offset, size); + value -= gdbarch_decr_pc_after_break (gdbarch); + memcpy (context_offset, &value, size); + } + else + { + gdb_assert (size == 8); + uint64_t value; + memcpy (&value, context_offset, size); + value -= gdbarch_decr_pc_after_break (gdbarch); + memcpy (context_offset, &value, size); + } + } + regcache->raw_supply (r, context_offset); + } } void @@ -1450,16 +1526,36 @@ windows_continue (DWORD continue_status, int id, int killed) { BOOL res; + desired_stop_thread_id = id; + + /* 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 (("windows_continue - pending stop anticipated, " + "desired=0x%x, item=0x%x\n", + desired_stop_thread_id, item.thread_id)); + return TRUE; + } + } + DEBUG_EVENTS (("ContinueDebugEvent (cpid=%d, ctid=0x%x, %s);\n", - (unsigned) current_event.dwProcessId, - (unsigned) current_event.dwThreadId, + (unsigned) last_wait_event.dwProcessId, + (unsigned) last_wait_event.dwThreadId, continue_status == DBG_CONTINUE ? "DBG_CONTINUE" : "DBG_EXCEPTION_NOT_HANDLED")); for (windows_thread_info *th : thread_list) - if ((id == -1 || id == (int) th->tid) - && th->suspended) + if (id == -1 || id == (int) th->tid) { + if (!th->suspended) + continue; #ifdef __x86_64__ if (wow64_process) { @@ -1519,9 +1615,15 @@ windows_continue (DWORD continue_status, int id, int killed) } th->resume (); } + else + { + /* When single-stepping a specific thread, other threads must + be suspended. */ + th->suspend (); + } - res = ContinueDebugEvent (current_event.dwProcessId, - current_event.dwThreadId, + res = ContinueDebugEvent (last_wait_event.dwProcessId, + last_wait_event.dwThreadId, continue_status); if (!res) @@ -1704,6 +1806,17 @@ ctrl_c_handler (DWORD event_type) return TRUE; } +/* A wrapper for WaitForDebugEvent that sets "last_wait_event" + appropriately. */ +static BOOL +wait_for_debug_event (DEBUG_EVENT *event, DWORD timeout) +{ + BOOL result = WaitForDebugEvent (event, timeout); + if (result) + last_wait_event = *event; + return result; +} + /* Get the next event from the child. Returns a non-zero thread id if the event requires handling by WFI (or whatever). */ @@ -1717,9 +1830,36 @@ windows_nat_target::get_windows_debug_event (int pid, static windows_thread_info dummy_thread_info (0, 0, 0); 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. */ + for (auto iter = pending_stops.begin (); + iter != pending_stops.end (); + ++iter) + { + if (desired_stop_thread_id == -1 + || desired_stop_thread_id == iter->thread_id) + { + thread_id = iter->thread_id; + *ourstatus = iter->status; + current_event = iter->event; + + inferior_ptid = ptid_t (current_event.dwProcessId, thread_id, 0); + current_thread = thread_rec (thread_id, INVALIDATE_CONTEXT); + current_thread->reload_context = 1; + + DEBUG_EVENTS (("get_windows_debug_event - " + "pending stop found in 0x%x (desired=0x%x)\n", + thread_id, desired_stop_thread_id)); + + pending_stops.erase (iter); + return thread_id; + } + } + last_sig = GDB_SIGNAL_0; - if (!(debug_event = WaitForDebugEvent (¤t_event, 1000))) + if (!(debug_event = wait_for_debug_event (¤t_event, 1000))) goto out; event_count++; @@ -1903,7 +2043,27 @@ windows_nat_target::get_windows_debug_event (int pid, if (!thread_id || saw_create != 1) { - CHECK (windows_continue (continue_status, -1, 0)); + CHECK (windows_continue (continue_status, desired_stop_thread_id, 0)); + } + else if (desired_stop_thread_id != -1 && desired_stop_thread_id != thread_id) + { + /* 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)\n", + thread_id, desired_stop_thread_id)); + + if (current_event.dwDebugEventCode == EXCEPTION_DEBUG_EVENT + && (current_event.u.Exception.ExceptionRecord.ExceptionCode + == EXCEPTION_BREAKPOINT) + && windows_initialization_done) + { + th = thread_rec (thread_id, INVALIDATE_CONTEXT); + th->stopped_at_software_breakpoint = true; + } + pending_stops.push_back ({thread_id, *ourstatus, current_event}); + thread_id = 0; + CHECK (windows_continue (continue_status, desired_stop_thread_id, 0)); } else { @@ -1965,7 +2125,21 @@ windows_nat_target::wait (ptid_t ptid, struct target_waitstatus *ourstatus, SetConsoleCtrlHandler (&ctrl_c_handler, FALSE); if (retval) - return ptid_t (current_event.dwProcessId, retval, 0); + { + ptid_t result = ptid_t (current_event.dwProcessId, retval, 0); + + if (current_thread != nullptr) + { + current_thread->stopped_at_software_breakpoint = false; + if (current_event.dwDebugEventCode == EXCEPTION_DEBUG_EVENT + && (current_event.u.Exception.ExceptionRecord.ExceptionCode + == EXCEPTION_BREAKPOINT) + && windows_initialization_done) + current_thread->stopped_at_software_breakpoint = true; + } + + return result; + } else { int detach = 0; @@ -3174,7 +3348,7 @@ windows_nat_target::kill () { if (!windows_continue (DBG_CONTINUE, -1, 1)) break; - if (!WaitForDebugEvent (¤t_event, INFINITE)) + if (!wait_for_debug_event (¤t_event, INFINITE)) break; if (current_event.dwDebugEventCode == EXIT_PROCESS_DEBUG_EVENT) break; -- 2.21.1