Re: [PATCH 3/6] Add new vDefaultInferiorFd packet

[Eli Zaretskii <eliz@gnu.org>]

> From: Alexandra Hájková <ahajkova@redhat.com>
> Cc: ahajkova@redhat.com
> Date: Fri, 17 Nov 2023 12:18:37 +0100
> 
> Add a new DefaultInferiorFd feature and the corresponding packet.
> This feature allows GDB to send, to GDBserver, the file descriptor
> numbers of the terminal to which GDB is connected. The inferior is
> then started connected to the same terminal as GDB. This allows the
> inferior run by local GDBserver to read from GDB's STDIN and write
> its output to GDB's STOUT/ERR the same way as native target.
> ---
>  gdb/doc/gdb.texinfo |  32 +++++++++++
>  gdb/remote.c        |  28 ++++++++++
>  gdbserver/server.cc | 132 +++++++++++++++++++++++++++++++++++++++++++-
>  gdbserver/server.h  |  12 ++++
>  4 files changed, 201 insertions(+), 3 deletions(-)

Thanks.  I have some comments:

> +When GDBserver is run locally using stdio for example
> +@code{target remote | gdbserver - inferior}.
> +Inferior's STDIN is closed which means the inferior can't read any input.

This should be a single sentence, and it currently lacks some
mandatory punctuation and uses sub-optimal markup.  Here's a
suggestion to fix that:

  When GDBserver is run locally using stdio, for example, with
  @w{@kbd{target remote | gdbserver - @var{inferior}}}, the inferior's
  @code{stdin} is closed, which means the inferior can't read any
  input.

> +If GDBserver is run locally using stdio and supports the @xref{default inferior
> +file descriptors} feature, GDBserver will start inferior connected to the same
> +terminal as @value{GDBN} which enables the inferior to read from STDIN and
> +write to the STDOUT/ERR in the same fashion native target does.

Suggest to reword like this:

  If GDBserver supports the default inferior file descriptors feature
  (@pxref{default inferior file descriptors}), it will start the
  inferior connected to the same terminal as @value{GDBN}, which
  enables the inferior to read from @code{stdin} and write to
  @code{stdout} and @code{stderr}, in the same way native targets do.

> +@item vDefaultInferior;@var{fd0};@var{fd1};@var{fd2}
> +@item vDefaultInferiorFd

The second @item should be @itemx.

> +@anchor{default inferior file descriptors}
> +@cindex @samp{vDefaultInferiorFd} packet

Please place the @anchor and the @cindex lines before the @item lines,
so that following the index link will show the @item lines, not only
the text below them.

> +@value{GDBN} would preserve the numbers of STDOUT/STDIN/STDERR file descriptors.
                                              ^^^^^^^^^^^^^^^^^^^
@code{stdin}/@code{stdout}/@code{stderr}

And I don't think I understand why we need to mention that GDB
preserves these file descriptors.  is this important forf using GDB
and gdbserver in these cases?

> +Preserved numbers of the file descriptors are then sent to GDBserver.

Same question about this sentence: is it important, and of so, why?

> +If GDBserver is run locally and will accept the numbers of the file
> +descriptors, it will start the inferior connected to the same STDIN/OUT/ERR
> +@value{GDBN} is connected to. This allows the inferior run under the local GDBserver
> +to read from the STDIN and write to STOUT/ERR. This makes a remote target (which is
> +run locally) to behave similarly to the native target.

I'm confused here: does the above passage describe an alternative way
of starting the inferior?  If so, how is that method useful, and how
can the user control the behavior?

Also, please leave two spaces, not one, between sentences, per our
conventions.

> +This feature does not support setting inferior tty.

What does this mean in practice?  Which commands and/or packets are
not supported, and what does it mean for GDB user in practice?

Reviewed-By: Eli Zaretskii <eliz@gnu.org>