From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from simark.ca (simark.ca [158.69.221.121]) by sourceware.org (Postfix) with ESMTPS id ED4C3383A379 for ; Thu, 21 Jul 2022 19:35:59 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org ED4C3383A379 Received: from [10.0.0.11] (unknown [192.222.157.6]) (using TLSv1.3 with cipher TLS_AES_128_GCM_SHA256 (128/128 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits)) (No client certificate requested) by simark.ca (Postfix) with ESMTPSA id 51F0D1E13B; Thu, 21 Jul 2022 15:28:36 -0400 (EDT) Message-ID: Date: Thu, 21 Jul 2022 15:28:26 -0400 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101 Thunderbird/91.11.0 Subject: Re: [PATCH v2 00/29] Step over thread clone and thread exit Content-Language: en-US To: Pedro Alves , gdb-patches@sourceware.org References: <20220713222433.374898-1-pedro@palves.net> From: Simon Marchi In-Reply-To: <20220713222433.374898-1-pedro@palves.net> Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit X-Spam-Status: No, score=-5.1 required=5.0 tests=BAYES_00, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, NICE_REPLY_A, SPF_HELO_PASS, 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.29 Precedence: list List-Id: Gdb-patches mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 21 Jul 2022 19:36:02 -0000 On 2022-07-13 18:24, Pedro Alves wrote: > Here's v2 of the series I previously posted here: > > https://sourceware.org/pipermail/gdb-patches/2022-June/190181.html > > New in v2: > > - One patch of v1 made it to master, one was dropped, and there are > a few new patches. > > - GDB now clears the (QThreadOptions) thread options of all threads > that are resumed, not just the current thread, in all-stop. > > - GDB now coalesces QThreadOptions packets, similarly to how vCont > packets are aggregated. Consequently, target_set_thread_options > disappeared. The need for this was exposed by the previous bullet > point, as in the v1 implementation, setting & clearing the thread > options of all threads led to two QThreadOptions packet per > thread, one when resuming, and another when stopping. Now there's > only one packet per high level, like the vCont packets. > > - That exposed a few bugs in gdbserver's implementation of > QThreadOptions, now fixed. > > - It also exposed that making fork/vfork/clone threads inherit their > QThreadOptions options from the parent (modeling > PTRACE_SETOPTIONS), was a mistake. By not inheriting, GDB can > send e.g., "QThreadOptions;0;1:TID" without worrying about threads > it doesn't know about yet. Patches that were copying thread > options from parent to child are now simpler, as they no longer do > that. > > - GDB now also avoids sending repeated QThreadEvents packets. > > - The printing the number of unexpect cores in gdb.sum caught that > one testcase was passing cleanly, with same number of PASSes as > before, but, GDB was crashing during teardown. There's a new > patch to fix the crash, which was a latent problem: > > [PATCH v2 03/29] gdb/linux: Delete all other LWPs immediately on ptrace exec event > > - The documentation patches in v1 where already reviewed, iterated > on, and largely approved. The documentation changes in v2 are the > result of those reviews/discussions. > > - Patch #14 is new: > > [PATCH v2 14/29] gdbserver/linux-low.cc: Ignore event_ptid if TARGET_WAITKIND_IGNORE > > This fixes a latent issue that gdbserver testing ran into. > > - The last 3 patches of the series are also new: > > [PATCH v2 27/29] inferior::clear_thread_list always silent > [PATCH v2 28/29] Centralize "[Thread ...exited]" notifications > [PATCH v2 29/29] Cancel execution command on thread exit, when stepping, nexting, etc. > > The first two of those are preparatory patches for the last one. > That patch is the last missing piece necessary to make step over > thread exit work nicely on the yet-unsubmitted AMGPU port, but > it's also a nice improvement for normal CPU debugging too, IMO. > > - Probably some minor details throughout the series that I forgot to > take notes of. > > Here's the series description, updated for v2: > > This is a new series that replaces two different series from last > year. > > The first is this series Simon and I wrote, here: > > [PATCH 00/10] Step over thread exit (PR gdb/27338) > https://sourceware.org/pipermail/gdb-patches/2021-July/180567.html > > The other is a series that coincidentally, back then, Andrew posted at > about the same time, and that addressed problems in kind of the mirror > scenario. His patch series was about stepping over clone (creating > new threads), instead of stepping over thread exit: > > [PATCH 0/3] Stepping over clone syscall > https://sourceware.org/pipermail/gdb-patches/2021-June/180517.html > > My & Simon's solution back then involved adding a new contract between > GDB and GDBserver -- if a thread is single stepping, and it exits, the > server was supposed to report back the thread's exit to GDB. One of > the motivations for this approach was to be able to control the > enablement of thread exit events per thread, to avoid creating > thread-exit event traffic unnecessarily, as done by > target_thread_events()/QThreadEvents. > > Andrew's solution envolves using the QThreadEvents mechanism, which > tells the server to report thread create and thread exit events for > all threads. This would conflict with the desire to avoid unnecessary > traffic in the step over thread exit series. > > The step over clone fixes back then also weren't yet fully complete, > as Andrew's series only addressed inline step overs. Fixing displaced > stepping over clone syscall would still remain broken. > > This new series fixes all of stepping over thread exit and clone, for > both of displaced stepping and inline step overs. It: > > - Merges both Andrew's and my/Simon's series, and then reworks both > parts in different ways. > > - Introduces clone events at the GDB core and remote protocol level. > > - Gets rid of the idea of "reporting thread exit if thread is > single-stepping", replaces it by a new mechanism GDB can use to > explicitly enable thread clone and/or thread exit events, and other > events in the future. The old mechanism also only worked when the > remote server supported hardware single-stepping. This new approach > has an advantage of also working on software single-step targets. > > - Uses the new clone events to fix displaced stepping over clone > syscalls too. > > - Addresses an issue that Andrew alluded to in his series, and that > coincidentally, we/AMD also ran into with AMDGPU debugging -- > currently, with "set scheduler-locking on", if you step over a > function that spawns a thread, that thread runs free, for a bit at > least, and then may stop or not, basically in an unspecified manner. > > - Addresses Simon's review comments on the original "Step over thread > exit" series referenced above. > > - Centralizes "[Thread ...exited]" notifications in core code. > > - Cancels next/step/until/etc. commands on thread exit event, like so: > > (gdb) n > [Thread 0x7ffff7d89700 (LWP 3961883) exited] > Command aborted, thread exited. > (gdb) > > There are documentation changes in the following patches: > > [PATCH 21/29] Don't resume new threads if scheduler-locking is in effect > [PATCH 26/29] Document remote clone events, and QThreadOptions packet > > ... which as mentione above, were already discussed in v1, and updated > accordingly. > > I'm aware that Tankut also has patches addressing issues around > reading registers of already-exited processes, but I haven't looked at > them in any detail yet. So I guess patch #23 ("Ignore failure to read > PC when resuming") may end up changing or be replaced by Tankut's. > > Tested on x86-64 Ubuntu 20.04, native and gdbserver. > > Andrew Burgess (1): > Add test for stepping over clone syscall > > Pedro Alves (26): > displaced step: pass down target_waitstatus instead of gdb_signal > linux-nat: introduce pending_status_str > gdb/linux: Delete all other LWPs immediately on ptrace exec event > Step over clone syscall w/ breakpoint, TARGET_WAITKIND_THREAD_CLONED > Support clone events in the remote protocol > Avoid duplicate QThreadEvents packets > Thread options & clone events (core + remote) > Thread options & clone events (native Linux) > Thread options & clone events (Linux GDBserver) > gdbserver: Hide and don't detach pending clone children > Remove gdb/19675 kfails (displaced stepping + clone) > all-stop/synchronous RSP support thread-exit events > gdbserver/linux-low.cc: Ignore event_ptid if TARGET_WAITKIND_IGNORE > Introduce GDB_TO_EXIT thread option, fix step-over-thread-exit > Implement GDB_TO_EXIT support for Linux GDBserver > Implement GDB_TO_EXIT support for native Linux > stop_all_threads: (re-)enable async before waiting for stops > gdbserver: Queue no-resumed event after thread exit > Don't resume new threads if scheduler-locking is in effect > Report thread exit event for leader if reporting thread exit events > Ignore failure to read PC when resuming > gdb/testsuite/lib/my-syscalls.S: Refactor new SYSCALL macro > Document remote clone events, and QThreadOptions packet > inferior::clear_thread_list always silent > Centralize "[Thread ...exited]" notifications > Cancel execution command on thread exit, when stepping, nexting, etc. > > Simon Marchi (2): > gdb: clear step over information on thread exit (PR gdb/27338) > Testcases for stepping over thread exit syscall (PR gdb/27338) > > gdb/NEWS | 26 + > gdb/annotate.c | 4 +- > gdb/breakpoint.c | 4 +- > gdb/displaced-stepping.c | 18 +- > gdb/displaced-stepping.h | 2 +- > gdb/doc/gdb.texinfo | 130 +++- > gdb/fbsd-nat.c | 3 - > gdb/gdbarch-components.py | 6 +- > gdb/gdbarch-gen.h | 10 +- > gdb/gdbarch.c | 4 +- > gdb/gdbthread.h | 38 +- > gdb/inferior.c | 12 +- > gdb/inferior.h | 7 +- > gdb/infrun.c | 589 +++++++++++++++--- > gdb/linux-nat.c | 381 ++++++----- > gdb/linux-nat.h | 4 + > gdb/linux-tdep.c | 5 +- > gdb/linux-tdep.h | 2 +- > gdb/mi/mi-interp.c | 8 +- > gdb/netbsd-nat.c | 4 - > gdb/observable.h | 11 +- > gdb/procfs.c | 6 - > gdb/python/py-inferior.c | 4 +- > gdb/remote.c | 251 +++++++- > gdb/target-debug.h | 2 + > gdb/target-delegates.c | 52 ++ > gdb/target.c | 17 + > gdb/target.h | 10 + > gdb/target/target.h | 14 + > gdb/target/waitstatus.c | 1 + > gdb/target/waitstatus.h | 20 +- > gdb/testsuite/gdb.base/step-over-syscall.exp | 44 +- > .../gdb.threads/schedlock-new-thread.c | 46 ++ > .../gdb.threads/schedlock-new-thread.exp | 63 ++ > gdb/testsuite/gdb.threads/step-over-exec.exp | 6 + > ...-over-thread-exit-while-stop-all-threads.c | 77 +++ > ...ver-thread-exit-while-stop-all-threads.exp | 69 ++ > .../gdb.threads/step-over-thread-exit.c | 52 ++ > .../gdb.threads/step-over-thread-exit.exp | 130 ++++ > gdb/testsuite/gdb.threads/stepi-over-clone.c | 90 +++ > .../gdb.threads/stepi-over-clone.exp | 392 ++++++++++++ > gdb/testsuite/lib/my-syscalls.S | 54 +- > gdb/testsuite/lib/my-syscalls.h | 5 + > gdb/thread.c | 70 ++- > gdb/windows-nat.c | 16 +- > gdbserver/gdbthread.h | 3 + > gdbserver/linux-low.cc | 401 +++++++----- > gdbserver/linux-low.h | 56 +- > gdbserver/remote-utils.cc | 26 +- > gdbserver/server.cc | 158 ++++- > gdbserver/target.cc | 15 +- > gdbserver/target.h | 30 +- > 52 files changed, 2848 insertions(+), 600 deletions(-) > create mode 100644 gdb/testsuite/gdb.threads/schedlock-new-thread.c > create mode 100644 gdb/testsuite/gdb.threads/schedlock-new-thread.exp > create mode 100644 gdb/testsuite/gdb.threads/step-over-thread-exit-while-stop-all-threads.c > create mode 100644 gdb/testsuite/gdb.threads/step-over-thread-exit-while-stop-all-threads.exp > create mode 100644 gdb/testsuite/gdb.threads/step-over-thread-exit.c > create mode 100644 gdb/testsuite/gdb.threads/step-over-thread-exit.exp > create mode 100644 gdb/testsuite/gdb.threads/stepi-over-clone.c > create mode 100644 gdb/testsuite/gdb.threads/stepi-over-clone.exp > > > base-commit: 9779607aff84cad92d8800290dce4eb17c17ce12 Hi Pedro, I went over the series and sent my comments, nothing major. I must admit that I read some of the patches in diagonal, because they touch code that is quite complex. Trying to run the complex conditions in my head makes my brain overflow. So for a lot of this, I trust that if the tests pass, it must be good (or at least better than the previous state). Simon