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

[Pedro Alves <pedro@palves.net>]

On 2023-06-12 13:06, Andrew Burgess via Gdb-patches wrote:
> 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.


Note that sentence was just trying to talk about the numeric value of each option
listed  below, the "0x1" in "GDB_THREAD_OPTION_CLONE (0x1)".  If there ever was an
option like "GDB_THREAD_OPTION_FOO (0x8000)", it wouldn't make sense to talk about
that number 0x8000 being represented in big or little endian, just like it
doesn't make sense to say that 1234235345345 is in big or little endian.  It's
just a number.  How integers are encoded in the protocol is a separate
question, and I don't think we should make a local decision about that here,
because hex integers appears in many places, like e.g.:

 ‘vAttach;pid’
 Attach to a new process with the specified process ID pid. The process ID is a hexadecimal integer
 identifying the process.

By "local decision" I mean, specifying how hex integers are encoded for a specific
packet.  That seems to me something that should be described once in a central place.
I actually thought we talked about it in the overview section:

 https://sourceware.org/gdb/current/onlinedocs/gdb.html/Overview.html

.. since we use unpack_varlen_hex pretty consistently.  Looks like we don't after all.
I could swear I had seen it mentioned somewhere before.  We should fix that.
We should say that we encode hex integers as strings with no 0x prefix.

I don't know what compelled me back then to explicitly mention that the values are
in hexadecimal, when it's totally obvious (they start with 0x).

I do think I was missing something in this patch, and thanks for poking about this,
which is to explicitly state that "options" is encoded as an hex integer.  

So I did this (IMO obvious) change:

 -@var{options} is the bitwise @code{OR} of the following values.  All
 -values are given in hexadecimal representation.
 +@var{options} is an hexadecimal integer specifying the enabled thread
 +options, and is the bitwise @code{OR} of the following values.

> 
>> +
>> +@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)

No, I don't think so.  It's important that this is distinct from "thread create".

We end up with two different events "cloned" and "created":

  /* The thread was cloned.  The event's ptid corresponds to the
     cloned parent.  The cloned child is held stopped at its entry
     point, and its ptid is in the event's m_child_ptid.  The target
     must not add the cloned child to GDB's thread list until
     target_ops::follow_clone() is called.  */
  TARGET_WAITKIND_THREAD_CLONED,

  /* The thread was created.  */
  TARGET_WAITKIND_THREAD_CREATED,

And likewise in the RSP:

 @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
 The packet indicates that the thread was just created.  The new thread
 is stopped until @value{GDBN} sets it running with a resumption packet
 (@pxref{vCont packet}).  This packet should not be sent by default;
 @value{GDBN} requests it with the @ref{QThreadEvents} packet.  See
 also the @samp{w} (@pxref{thread exit event}) remote reply below.  The
 @var{r} part is ignored.

"created" events are reported by the thread that was created.  There is no
way to ask a thread to enable reporting "thread created" events for itself,
because, well, the thread doesn't exist yet before the event is reported.
Hence, there's no corresponding GDB_THREAD_OPTION_CREATED option.

Clone events, OTOH are reported by the parent thread, the one that
clones itself to create the child.  The option exists to ask a specific thread
to report TARGET_WAITKIND_THREAD_CLONED / T05:clone events, hence the
matching GDB_THREAD_OPTION_CLONE name.

"clone" seems like a pretty accurate name for what happens, I don't think we
need to try to come up with a different name.  That it matches the Linux/ptrace
event name (not a coincidence, of course) is a bonus in my view, one less
thing to learn.

>> +
>> +@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?

We don't need to imagine it, it's how e.g., Windows works.
Yes, we still want to be notified about new threads in such systems, but
there we don't have thread-grained control of such events.  All we can do
is ask to be notified of all thread creations.  That's what this series does,
with code like:

      gdb_thread_options options
	= GDB_THREAD_OPTION_CLONE | GDB_THREAD_OPTION_EXIT;
      if (target_supports_set_thread_options (options))
	tp->set_thread_options (options);
      else
	target_thread_events (true);

Pedro Alves