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

[Pedro Alves <pedro@palves.net>]

Hi!

On 2022-06-21 1:07 p.m., Eli Zaretskii wrote:
>> From: Pedro Alves <pedro@palves.net>
>> Date: Mon, 20 Jun 2022 23:54:19 +0100
>>
>> 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.
> 
> Thanks.
> 
>>    ** GDBserver is now supported on LoongArch GNU/Linux.
>>  
>> +* New remote packets
>> +
>> +clone stop reason
>> +  Indicates that a clone system call was executed.
> 
> I'm confused: what is the relation between the "stop reason" part and
> the description saying that "a clone system call was executed"?  The
> gdb.texinfo description only mentions "clone" as the packet name,
> without the other 2 words.  What am I missing?

A "stop reason" is a remote-protocol defined term, which should be familiar
to remote stub maintainers.  See here:

 @item
 If @var{n} is a recognized @dfn{stop reason}, it describes a more
 specific event that stopped the target.  The currently defined stop
 reasons are listed below.  The @var{aa} should be @samp{05}, the trap
 signal.  At most one stop reason should be present.

So that new line in NEWS is saying that we're added a new "stop reason",
called "clone". When the remote target reports it, it means that the
program being debugged execute a clone system call.

This is exactly how e.g. the "fork" and "vfork" stop reasons (and others) were
mentioned in NEWS back when they were introduced:

~~~
fork stop reason
  Indicates that a fork system call was executed.

vfork stop reason
  Indicates that a vfork system call was executed.
~~~


> 
>> +@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.  Refer to @ref{thread-id syntax}
>> +for the format of the @var{thread-id} field.  This packet is only
>> +applicable to targets that support clone events.
> 
> The text refers to @var{r} and @var{thread-id}, but they are not
> present on the @item line that describes the packet itself.  Where are
> those parameters used in this case.

Hmm.  @var{r} is described further up.  This is all within the description
of the T stop reply, which mentions:

@item T @var{AA} @var{n1}:@var{r1};@var{n2}:@var{r2};@dots{}
...
Each @samp{@var{n}:@var{r}} pair is interpreted as follows:
...
@item
If @var{n} is a recognized @dfn{stop reason}, it describes a more
specific event that stopped the target.  The currently defined stop
reasons are listed below. 
...


So for the "clone" case in question, "clone" is the recognized stop reason,
and in the n:r pair, r is the thread ID of the child.


However, I agree that @var{thread-id} appears a bit out of the blue.  Note this
is the exact same wording as in the existing fork and vfork stop reasons, which
I just copied.  E.g.:

 @cindex fork events, remote reply
 @item fork
 The packet indicates that @code{fork} was called, and @var{r}
 is the thread ID of the new child process.  Refer to
 @ref{thread-id syntax} for the format of the @var{thread-id}
 field. (...)

A bit further above, we say:

 @item
 If @var{n} is @samp{thread}, then @var{r} is the @var{thread-id} of
 the stopped thread, as specified in @ref{thread-id syntax}.

so how about we use the same wording for fork/vfork?

Like so (against current master):

~~~~~~~~~~~~~~~
diff --git c/gdb/doc/gdb.texinfo w/gdb/doc/gdb.texinfo
index 7a4e337d15b..79b2fccb501 100644
--- c/gdb/doc/gdb.texinfo
+++ w/gdb/doc/gdb.texinfo
@@ -41655,11 +41655,10 @@ apply.
 
 @cindex fork events, remote reply
 @item fork
-The packet indicates that @code{fork} was called, and @var{r}
-is the thread ID of the new child process.  Refer to
-@ref{thread-id syntax} for the format of the @var{thread-id}
-field.  This packet is only applicable to targets that support
-fork events.
+The packet indicates that @code{fork} was called, and @var{r} is the
+@var{thread-id} of the new child process, as specified in
+@ref{thread-id syntax}.  This packet is only applicable to targets
+that support fork events.
 
 This packet should not be sent by default; older @value{GDBN} versions
 did not support it.  @value{GDBN} requests it, by supplying an
@@ -41669,11 +41668,10 @@ indicating support.
 
 @cindex vfork events, remote reply
 @item vfork
-The packet indicates that @code{vfork} was called, and @var{r}
-is the thread ID of the new child process. Refer to
-@ref{thread-id syntax} for the format of the @var{thread-id}
-field.  This packet is only applicable to targets that support
-vfork events.
+The packet indicates that @code{vfork} was called, and @var{r} is the
+@var{thread-id} of the new child process, as specified in
+@ref{thread-id syntax}.  This packet is only applicable to targets
+that support vfork events.
 
 This packet should not be sent by default; older @value{GDBN} versions
 did not support it.  @value{GDBN} requests it, by supplying an
~~~~~~~~~~~~~~~

I'd push this as a separate patch right away, if OK.

> 
>> +@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 rightmost options with a matching
>> +@var{thread-id} are applied.
> 
> "Rightmost" means here "the last in the list"?  If so, perhaps it's
> worth saying that explicitly to avoid possible confusion.

I can change it, but I'm curious what the confusion could be?  I used
rightmost here to contrast with the vCont description, which uses "leftmost".

> 
>> +@item
>> +Without clone events (on systems where threads are spawned via a clone
>> +system call), if the single-stepped thread spawns a new clone child
>> +(i.e., it executes a clone system call), and:
>> +
>> +@itemize @minus
>> +@item
>> +the breakpoint is stepped-over in-line, the spawned thread incorrectly
>> +runs free while the breakpoint being stepped over is not inserted,
>> +thus the spawned thread may potentially run past the breakpoint
>> +without stopping for it.  By enabling @code{GDB_TO_CLONE}, the new
>> +cloned thread is halted before it executes any instruction;
>> +
>> +@item
>> +if alternativelly displaced (out-of-line) stepping is used, the
>> +spawned thread starts running at the out-of-line PC, leading to
>> +undefined behavior, usually crashes or data corruption.  By enabling
>> +@code{GDB_TO_CLONE}, the new cloned thread is halted before it
>> +executes any instruction, and @value{GDBN} adjusts its PC before
>> +resuming its execution.
>> +@end itemize
> 
> This reads awkwardly, because you say "and:", and start each @item of
> the following itemized list with a lower-case letter, but each @item
> includes more than one sentence.  How about the following rephrasing:
> 
>     For example, @value{GDBN} enables the @code{GDB_TO_EXIT} and
>     @code{GDB_TO_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_TO_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_TO_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;
> 
>     @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
> 

Thank you, that is perfect, I'll take it.