Re: [PATCH v2 00/29] Step over thread clone and thread exit

[Simon Marchi <simark@simark.ca>]

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