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

[Pedro Alves <pedro@palves.net>]

On 2022-07-11 5:09 p.m., Eli Zaretskii wrote:
>> Cc: gdb-patches@sourceware.org
>> From: Pedro Alves <pedro@palves.net>
>> Date: Mon, 11 Jul 2022 16:19:57 +0100
>>
>>>> +* 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:
> 
> I see.  It's highly confusing, IMO, although we use it elsewhere in
> NEWS.  I would suggest at least
> 
>   'clone', a stop reason
> 
> or
> 
>   'clone' (a stop reason)
> 
> or even
> 
>   new stop reason: 'clone'

I'll go with the last one.

> 
> (Yes, for the rest of similar entries as well.  No, I don't insist.)
> 
>> fork stop reason
>>   Indicates that a fork system call was executed.
>>
>> vfork stop reason
>>   Indicates that a vfork system call was executed.
> 
> Just for the record: you do realize that using 3 nouns, 2 of which can
> also be verbs, in a row, without any context creates very ambiguous
> text?  "clone stop reason" could be interpreted as "clone the stop
> reason" (i.e. the stop reason should be cloned) or "reason for
> stopping the clone" or "stop reason of the clone" and probably a few
> others.

There is context.  It's a section about new packets, and "stop reason"
is a familiar term.  And the sentence just below removed any ambiguity, IMO.
Anyhow, I can change it, no problem.

> 
>>  @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}.
> 
> "thread-id" should not be in @var here, since it is not referenced
> anywhere.  The text about fork events that you cite above gets that
> right, and the new text should so the same.

The text for fork events uses "@var{thread-id}" too, in the
"Refer to @ref{thread-id syntax} for the format of the @var{thread-id} field."
sentence.  The new text was just copying that.

How about this to fix the preexisting issues?

~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

From 77f4d421d311c12360d5051cc7047a51cf8e8cc4 Mon Sep 17 00:00:00 2001
From: Pedro Alves <pedro@palves.net>
Date: Mon, 11 Jul 2022 16:05:00 +0100
Subject: [PATCH] Fix non-existent "@var{thread-id}" in stop reply descriptions

In the description stop replies, where the "thread" register is
explained, and where the fork and vfork stop reasons are detailed,
there are references to "@var{thread-id}", but such variable does not
actually exist.  This commit fixes it.

Change-Id: If679944aaf15f6f64aabe506339a9e7667864cab
---
 gdb/doc/gdb.texinfo | 20 +++++++++-----------
 1 file changed, 9 insertions(+), 11 deletions(-)

diff --git a/gdb/doc/gdb.texinfo b/gdb/doc/gdb.texinfo
index 7a4e337d15b..5250709818e 100644
--- a/gdb/doc/gdb.texinfo
+++ b/gdb/doc/gdb.texinfo
@@ -41578,7 +41578,7 @@ series of bytes in target byte order, with each byte given by a
 two-digit hex number.
 
 @item
-If @var{n} is @samp{thread}, then @var{r} is the @var{thread-id} of
+If @var{n} is @samp{thread}, then @var{r} is the thread ID of
 the stopped thread, as specified in @ref{thread-id syntax}.
 
 @item
@@ -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
+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
+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

base-commit: 53a7a7e17c5d21b7b182ddf6bd8bfc092af196f5
-- 
2.36.0

~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~


> 
>>>> +@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".
> 
> I don't think "left" and "right" are well defined in this context, so
> they sound strange (especially to someone who reads RTL text every
> day).  Why not "first" and "last"?

Would a RTL reader really be confused with "right"?  "Right" is always
on the "right", even for RTL, no?  I'd think it's actually "first" and "last" that
would be confusing to RTL readers, as they could misinterpret "last" as being
on the left, being used to start reading from the right.  Anyhow, 
I'll (begrudgingly) change it.

> 
>>>     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.
> 
> But please capitalize the text of each @item (I forgot to do that in
> the last two).

OK.

> 
> Thanks.
>