Re: [PATCH 28/31] Document remote clone events, and QThreadOptions packet

[Andrew Burgess <aburgess@redhat.com>]

Pedro Alves <pedro@palves.net> writes:

> This commit documents in both manual and NEWS:
>
>  - the new remote clone event stop reply,
>  - the new QThreadOptions packet and its current defined options,
>  - the associated "set/show remote thread-events-packet" command,
>  - and the associated QThreadOptions qSupported feature.
>
> Approved-By: Eli Zaretskii <eliz@gnu.org>
> Change-Id: Ic1c8de1fefba95729bbd242969284265de42427e
> ---
>  gdb/NEWS            |  20 +++++++
>  gdb/doc/gdb.texinfo | 127 ++++++++++++++++++++++++++++++++++++++++++--
>  2 files changed, 144 insertions(+), 3 deletions(-)
>
> diff --git a/gdb/NEWS b/gdb/NEWS
> index 0aa153b83da..b1d3dd7e7d9 100644
> --- a/gdb/NEWS
> +++ b/gdb/NEWS
> @@ -160,6 +160,10 @@ set style tui-current-position [on|off]
>    Whether to style the source and assembly code highlighted by the
>    TUI's current position indicator.  The default is off.
>  
> +set remote thread-options-packet
> +show remote thread-options-packet
> +  Set/show the use of the thread options packet.
> +
>  * Changed commands
>  
>  document user-defined
> @@ -285,6 +289,22 @@ GNU/Linux/CSKY (gdbserver) csky*-*linux*
>  
>  GDB now supports floating-point on LoongArch GNU/Linux.
>  
> +* New remote packets
> +
> +New stop reason: clone
> +  Indicates that a clone system call was executed.
> +
> +QThreadOptions
> +  Enable/disable optional event reporting, on a per-thread basis.
> +  Currently supported options are GDB_THREAD_OPTION_CLONE, to enable
> +  clone event reporting, and GDB_THREAD_OPTION_EXIT to enable thread
> +  exit event reporting.
> +
> +QThreadOptions in qSupported
> +  The qSupported packet allows GDB to inform the stub it supports the
> +  QThreadOptions packet, and the qSupported response can contain the
> +  set of thread options the remote stub supports.
> +
>  *** Changes in GDB 12
>  
>  * DBX mode is deprecated, and will be removed in GDB 13
> diff --git a/gdb/doc/gdb.texinfo b/gdb/doc/gdb.texinfo
> index 5e75f32e0cd..ef62fac366f 100644
> --- a/gdb/doc/gdb.texinfo
> +++ b/gdb/doc/gdb.texinfo
> @@ -24079,6 +24079,10 @@ are:
>  @tab @code{QThreadEvents}
>  @tab Tracking thread lifetime.
>  
> +@item @code{thread-options}
> +@tab @code{QThreadOptions}
> +@tab Set thread event reporting options.
> +
>  @item @code{no-resumed-stop-reply}
>  @tab @code{no resumed thread left stop reply}
>  @tab Tracking thread lifetime.
> @@ -42110,6 +42114,17 @@ appropriate @samp{qSupported} feature (@pxref{qSupported}).  The
>  remote stub must also supply the appropriate @samp{qSupported} feature
>  indicating support.
>  
> +@cindex thread clone events, remote reply
> +@anchor{thread clone event}
> +@item clone
> +The packet indicates that @code{clone} was called, and @var{r} is the
> +thread ID of the new child thread, as specified in @ref{thread-id
> +syntax}.  This packet is only applicable to targets that support clone
> +events.
> +
> +This packet should not be sent by default; @value{GDBN} requests it
> +with the @ref{QThreadOptions} packet.
> +
>  @cindex thread create event, remote reply
>  @anchor{thread create event}
>  @item create
> @@ -42148,9 +42163,10 @@ hex strings.
>  @item w @var{AA} ; @var{tid}
>  
>  The thread exited, and @var{AA} is the exit status.  This response
> -should not be sent by default; @value{GDBN} requests it with the
> -@ref{QThreadEvents} packet.  See also @ref{thread create event} above.
> -@var{AA} is formatted as a big-endian hex string.
> +should not be sent by default; @value{GDBN} requests it with either
> +the @ref{QThreadEvents} or @ref{QThreadOptions} packets.  See also
> +@ref{thread create event} above.  @var{AA} is formatted as a
> +big-endian hex string.
>  
>  @item N
>  There are no resumed threads left in the target.  In other words, even
> @@ -42875,6 +42891,11 @@ same thread.  @value{GDBN} does not enable this feature unless the
>  stub reports that it supports it by including @samp{QThreadEvents+} in
>  its @samp{qSupported} reply.
>  
> +This packet always enables/disables event reporting for all threads of
> +all processes under control of the remote stub.  For per-thread
> +control of optional event reporting, see the @ref{QThreadOptions}
> +packet.
> +
>  Reply:
>  @table @samp
>  @item OK
> @@ -42891,6 +42912,94 @@ the stub.
>  Use of this packet is controlled by the @code{set remote thread-events}
>  command (@pxref{Remote Configuration, set remote thread-events}).
>  
> +@anchor{QThreadOptions}
> +@item QThreadOptions@r{[};@var{options}@r{[}:@var{thread-id}@r{]]}@dots{}
> +@cindex thread options, remote request
> +@cindex @samp{QThreadOptions} packet
> +
> +For each inferior thread, the last @var{options} in the list with a
> +matching @var{thread-id} are applied.  Any options previously set on a
> +thread are discarded and replaced by the new options specified.
> +Threads that do not match any @var{thread-id} retain their
> +previously-set options.  Thread IDs are specified using the syntax
> +described in @ref{thread-id syntax}.  If multiprocess extensions
> +(@pxref{multiprocess extensions}) are supported, options can be
> +specified to apply to all threads of a process by using the
> +@samp{p@var{pid}.-1} form of @var{thread-id}.  Options with no
> +@var{thread-id} apply to all threads.  Specifying no options is an
> +error.
> +
> +@var{options} is the bitwise @code{OR} of the following values.  All
> +values are given in hexadecimal representation.

Hi Pedro,

As I said in my other email, I think we should document here that 0 is a
valid value, and that this clears all the options.

Also, I think we should say: 'given in big-endian hexadecimal
representation'.

We are clear on the byte-ordering for other encoded values.  I know we
don't currently (and are probably unlikely to ever) need more than one
byte, but I figure it doesn't hurt to future proof the spec.

> +
> +@table @code
> +@item GDB_THREAD_OPTION_CLONE (0x1)
> +Report thread clone events (@pxref{thread clone event}).  This is only
> +meaningful for targets that support clone events (e.g., GNU/Linux
> +systems).

It was only while reading this that a question occurred to me.  Is CLONE
the best name for this option?  Or are we at risk of making the remote
protocol too Linux specific here?

Would it be better if we called the option GDB_THREAD_OPTION_CREATED, or
GDB_THREAD_OPTION_START for example?

I guess the name itself isn't really important, maybe what's important
is how we talk about the option, e.g. maybe we could say:

  Report thread creation events (@pxref{...}).  This is only meaningful
  for target that support creating new threads (e.g., GNU/Linux systems).

The reason I think we should ensure we don't focus on "thread clone" but
instead "thread creation" is .... (see below)


> +
> +@item GDB_THREAD_OPTION_EXIT (0x2)
> +Report thread exit events (@pxref{thread exit event}).
> +@end table
> +
> +@noindent
> +
> +For example, @value{GDBN} enables the @code{GDB_THREAD_OPTION_EXIT}
> +and @code{GDB_THREAD_OPTION_CLONE} options when single-stepping a
> +thread past a breakpoint, for the following reasons:
> +
> +@itemize @bullet
> +@item
> +If the single-stepped thread exits (e.g., it executes a thread exit
> +system call), enabling @code{GDB_THREAD_OPTION_EXIT} prevents
> +@value{GDBN} from waiting forever, not knowing that it should no
> +longer expect a stop for that same thread, and blocking other threads
> +from progressing.
> +
> +@item
> +If the single-stepped thread spawns a new clone child (i.e., it
> +executes a clone system call), enabling @code{GDB_THREAD_OPTION_CLONE}
> +halts the cloned thread before it executes any instructions, and thus
> +prevents the following problematic situations:
> +
> +@itemize @minus
> +@item
> +If the breakpoint is stepped-over in-line, the spawned thread would
> +incorrectly run free while the breakpoint being stepped over is not
> +inserted, and thus the cloned thread may potentially run past the
> +breakpoint without stopping for it;

.... this situation.  Even if we imagine an OS where thread creation
isn't done as a "clone", but instead spawns the thread into existence
with its own unique $pc value, this case would still be a problem, and
would still justify being notified about the new thread, right?

Thanks,
Andrew

> +
> +@item
> +If displaced (out-of-line) stepping is used, the cloned thread starts
> +running at the out-of-line PC, leading to undefined behavior, usually
> +crashing or corrupting data.
> +@end itemize
> +
> +@end itemize
> +
> +New threads start with thread options cleared.
> +
> +@value{GDBN} does not enable this feature unless the stub reports that
> +it supports it by including
> +@samp{QThreadOptions=@var{supported_options}} in its @samp{qSupported}
> +reply.
> +
> +Reply:
> +@table @samp
> +@item OK
> +The request succeeded.
> +
> +@item E @var{nn}
> +An error occurred.  The error number @var{nn} is given as hex digits.
> +
> +@item @w{}
> +An empty reply indicates that @samp{QThreadOptions} is not supported by
> +the stub.
> +@end table
> +
> +Use of this packet is controlled by the @code{set remote thread-options}
> +command (@pxref{Remote Configuration, set remote thread-options}).
> +
>  @item qRcmd,@var{command}
>  @cindex execute remote command, remote request
>  @cindex @samp{qRcmd} packet
> @@ -43336,6 +43445,11 @@ These are the currently defined stub features and their properties:
>  @tab @samp{-}
>  @tab No
>  
> +@item @samp{QThreadOptions}
> +@tab Yes
> +@tab @samp{-}
> +@tab No
> +
>  @item @samp{no-resumed}
>  @tab No
>  @tab @samp{-}
> @@ -43557,6 +43671,13 @@ The remote stub reports the supported actions in the reply to
>  @item QThreadEvents
>  The remote stub understands the @samp{QThreadEvents} packet.
>  
> +@item QThreadOptions=@var{supported_options}
> +The remote stub understands the @samp{QThreadOptions} packet.
> +@var{supported_options} indicates the set of thread options the remote
> +stub supports.  @var{supported_options} has the same format as the
> +@var{options} parameter of the @code{QThreadOptions} packet, described
> +at @ref{QThreadOptions}.
> +
>  @item no-resumed
>  The remote stub reports the @samp{N} stop reply.
>  
> -- 
> 2.36.0