[0/6] Add vDefaultInferiorFd feature

  Currently, when GDBserver is run locally using stdio, the inferior
is unable to read from STDIN so we can't give it any input.
The main motivation to address this issue is to use GDB together
with Valgrind, using vgdb --multi feature which allows to run
Valgrind from inside GDB. Valgrind then acts as a locally run
GDBserver that uses stdio.

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.



Alexandra Hájková (6):
  gdb.server/non-existing-program.exp: Use gdbserver_start.
  gdb/ser-pipe.c: Duplicate the file descriptors
  Add new vDefaultInferiorFd packet
  gdbserver/linux-low.cc: Connect the inferior to the terminal
  remote.c: Add terminal handling functions
  Add defaultinf.exp test to the testsuite

 gdb/doc/gdb.texinfo                           |  32 +++++
 gdb/remote.c                                  |  83 +++++++++++
 gdb/ser-pipe.c                                |  25 ++++
 gdb/serial.c                                  |   4 +
 gdb/serial.h                                  |   4 +
 gdb/testsuite/gdb.server/defaultinf.c         |  39 ++++++
 gdb/testsuite/gdb.server/defaultinf.exp       |  59 ++++++++
 .../gdb.server/non-existing-program.exp       |  54 ++-----
 gdb/testsuite/lib/gdbserver-support.exp       |  62 +++++---
 gdbserver/linux-low.cc                        |  32 ++++-
 gdbserver/server.cc                           | 132 +++++++++++++++++-
 gdbserver/server.h                            |  12 ++
 12 files changed, 476 insertions(+), 62 deletions(-)
 create mode 100644 gdb/testsuite/gdb.server/defaultinf.c
 create mode 100644 gdb/testsuite/gdb.server/defaultinf.exp
  

Ping

On Fri, Nov 17, 2023 at 12:18 PM Alexandra Hájková <ahajkova@redhat.com>
wrote:

> Currently, when GDBserver is run locally using stdio, the inferior
> is unable to read from STDIN so we can't give it any input.
> The main motivation to address this issue is to use GDB together
> with Valgrind, using vgdb --multi feature which allows to run
> Valgrind from inside GDB. Valgrind then acts as a locally run
> GDBserver that uses stdio.
>
> 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.
>
>
>
> Alexandra Hájková (6):
>   gdb.server/non-existing-program.exp: Use gdbserver_start.
>   gdb/ser-pipe.c: Duplicate the file descriptors
>   Add new vDefaultInferiorFd packet
>   gdbserver/linux-low.cc: Connect the inferior to the terminal
>   remote.c: Add terminal handling functions
>   Add defaultinf.exp test to the testsuite
>
>  gdb/doc/gdb.texinfo                           |  32 +++++
>  gdb/remote.c                                  |  83 +++++++++++
>  gdb/ser-pipe.c                                |  25 ++++
>  gdb/serial.c                                  |   4 +
>  gdb/serial.h                                  |   4 +
>  gdb/testsuite/gdb.server/defaultinf.c         |  39 ++++++
>  gdb/testsuite/gdb.server/defaultinf.exp       |  59 ++++++++
>  .../gdb.server/non-existing-program.exp       |  54 ++-----
>  gdb/testsuite/lib/gdbserver-support.exp       |  62 +++++---
>  gdbserver/linux-low.cc                        |  32 ++++-
>  gdbserver/server.cc                           | 132 +++++++++++++++++-
>  gdbserver/server.h                            |  12 ++
>  12 files changed, 476 insertions(+), 62 deletions(-)
>  create mode 100644 gdb/testsuite/gdb.server/defaultinf.c
>  create mode 100644 gdb/testsuite/gdb.server/defaultinf.exp
>
> --
> 2.41.0
>
>
  

>>>>> "Alexandra" == Alexandra Hájková <ahajkova@redhat.com> writes:

FWIW I tend to think Pedro ought to review this, since he's got the most
up-to-date experience with terminal handling, etc; or at least more so
than I do.

I do have a few comments on the implementation, but before that, I
wanted to ask a bit about the overall approach.

Alexandra> Currently, when GDBserver is run locally using stdio, the inferior
Alexandra> is unable to read from STDIN so we can't give it any input.

This idea in general seems fine to me (pending Pedro's input).
It's also in line with, and probably needed by, the idea of moving gdb
to a "gdbserver-only" model:

https://sourceware.org/gdb/wiki/LocalRemoteFeatureParity

It's never been super clear to me if gdbserver-only is a real goal or
just something we talk about idly.  I've been on the fence about it
myself, though more recently I tend to like the idea, simply because it
means less work -- I've written a number of patches now that needed work
on both gdb and gdbserver, and this project would halve that kind of
effort.

Alexandra> Add a new DefaultInferiorFd feature and the corresponding packet.

One question I had is - why a new packet?  A new packet seems somewhat
weird, in that it's only valid pretty early during startup, it seems.

Another approach might be to have a different way to specify the
connection fd to the remote, like a command-line option naming the fd to
use for RSP traffic.

Tom
  

Tom Tromey <tom@tromey.com> writes:

>>>>>> "Alexandra" == Alexandra Hájková <ahajkova@redhat.com> writes:
>
> FWIW I tend to think Pedro ought to review this, since he's got the most
> up-to-date experience with terminal handling, etc; or at least more so
> than I do.
>
> I do have a few comments on the implementation, but before that, I
> wanted to ask a bit about the overall approach.
>
> Alexandra> Currently, when GDBserver is run locally using stdio, the inferior
> Alexandra> is unable to read from STDIN so we can't give it any input.
>
> This idea in general seems fine to me (pending Pedro's input).
> It's also in line with, and probably needed by, the idea of moving gdb
> to a "gdbserver-only" model:
>
> https://sourceware.org/gdb/wiki/LocalRemoteFeatureParity
>
> It's never been super clear to me if gdbserver-only is a real goal or
> just something we talk about idly.  I've been on the fence about it
> myself, though more recently I tend to like the idea, simply because it
> means less work -- I've written a number of patches now that needed work
> on both gdb and gdbserver, and this project would halve that kind of
> effort.

I wouldn't describe it as the only thing I'm working on, but this is,
lets say, my background goal, part of my 10 year plan :)

I suspect my motivation is the same as yours -- the code duplication
between GDB and gdbserver is annoying.  And even if we managed some
super code refactor, and managed to share 100% of the native handling
code between GDB and gdbserver, I suspect simply having the remote
interface in-between would introduce its only differences.  Better, I
think, to have just one way of doing things.

Honestly, I suspect I may never get there, there are just too many
distractions, but I'm hoping to work on closing the gap between native
and remote over the next couple of years.  Then, maybe, who knows...

Anyway, I just thought I'd register my very real interest in working
towards a remote-only setup.

Thanks,
Andrew




>
> Alexandra> Add a new DefaultInferiorFd feature and the corresponding packet.
>
> One question I had is - why a new packet?  A new packet seems somewhat
> weird, in that it's only valid pretty early during startup, it seems.
>
> Another approach might be to have a different way to specify the
> connection fd to the remote, like a command-line option naming the fd to
> use for RSP traffic.
>
> Tom
  

>
> Alexandra> Add a new DefaultInferiorFd feature and the corresponding
> packet.
>
> One question I had is - why a new packet?  A new packet seems somewhat
> weird, in that it's only valid pretty early during startup, it seems.
>
> Another approach might be to have a different way to specify the
> connection fd to the remote, like a command-line option naming the fd to
> use for RSP traffic.
>

Are you imagining something like "target remote | gdbserver --once RSP_FD
...." ?
And GDB would replace RSP_FD with the actual file descriptor to use?
I agree that's a good idea but both  approaches have pros and cons.
You are correct that a command line approach is better because it
avoids adding a new packet and the whole FD switching business. But adding
the new
packet approach makes it easier for the users. It's possible to run GDB to
then run
 Valgrind from inside by using simply

target extended-remote | vgdb --multi

I hope this command will be replaced with an even simpler " target
valgrind" at
some point.
If we wanted to use the feature with GDBserver, I think, it's always more
user-friendly
 when the user does not have to set any additional command-line options.

Thanks,
Alexandra
  

>>>>> "Alexandra" == Alexandra Petlanova Hajkova <ahajkova@redhat.com> writes:

>  Another approach might be to have a different way to specify the
>  connection fd to the remote, like a command-line option naming the fd to
>  use for RSP traffic.

Alexandra> Are you imagining something like "target remote | gdbserver --once RSP_FD ...." ?
Alexandra> And GDB would replace RSP_FD with the actual file descriptor to use?

Yeah.

Alexandra> avoids adding a new packet and the whole FD switching business. But adding the new 
Alexandra> packet approach makes it easier for the users. It's possible to run GDB to then run
Alexandra>  Valgrind from inside by using simply

Alexandra> target extended-remote | vgdb --multi

Alexandra> I hope this command will be replaced with an even simpler " target valgrind" at
Alexandra> some point.

Part of the idea would be to hide the new file descriptor handling
behind the "target valgrind" facade.  That is, the implementation of
"target valgrind" would handle setting up the command line arguments to
vgdb.

Tom
  

Tom Tromey <tom@tromey.com> writes:

>>>>>> "Alexandra" == Alexandra Petlanova Hajkova <ahajkova@redhat.com> writes:
>
>>  Another approach might be to have a different way to specify the
>>  connection fd to the remote, like a command-line option naming the fd to
>>  use for RSP traffic.
>
> Alexandra> Are you imagining something like "target remote | gdbserver --once RSP_FD ...." ?
> Alexandra> And GDB would replace RSP_FD with the actual file descriptor to use?
>
> Yeah.

There is one problem I see with this approach, maybe a bit of an edge
case, but, what if the application wanted to use the text RSP_FD
elsewhere in it's command line.  For example, a user does:

  (gdb) target remote | gdbserver --once RSP_FD /tmp/myprog

And everything is great, GDB replaces RSP_FD with the descriptor to use
for RSP traffic, and that's fine.

Or a user does:

  (gdb) target remote | gdbserver --once /tmp/myprog

And GDB doesn't spot RSP_FD, so doesn't redirect the RSP traffic, and
just continues to use stdin/stdout as it does right now.

But what about the poor user who needs to do this:

  (gdb) target remote | gdbserver --once /tmp/myprog RSP_FD

that is the 'RSP_FD' is an actual argument string to pass to 'myprog'!
Unlikely maybe, but not impossible.  Anything that requires GDB to
understand the command that appears after the `|` suffers from the
possibility that GDB might get it wrong.

>
> Alexandra> avoids adding a new packet and the whole FD switching business. But adding the new 
> Alexandra> packet approach makes it easier for the users. It's possible to run GDB to then run
> Alexandra>  Valgrind from inside by using simply
>
> Alexandra> target extended-remote | vgdb --multi
>
> Alexandra> I hope this command will be replaced with an even simpler " target valgrind" at
> Alexandra> some point.
>
> Part of the idea would be to hide the new file descriptor handling
> behind the "target valgrind" facade.  That is, the implementation of
> "target valgrind" would handle setting up the command line arguments to
> vgdb.

Even writing a Python wrapper doesn't really solve the above problem, it
just shifts it from GDB into the Python code.

And (I think) ideally we wouldn't rely on users having to write a Python
wrapper in order to use this feature with their own tools, so it would
be nice if this feature was usable from pure GDB.

Maybe it's just enough to add an on/off control setting, and have GDB
announce when it's performed the command line replacements.  Then
(hopefully) users will know that if they don't want this feature they
could turn this off...

Just my thoughts.

Thanks,
Andrew
  

I sent some reviews, but I wanted to reiterate this bit:

Tom> FWIW I tend to think Pedro ought to review this, since he's got the most
Tom> up-to-date experience with terminal handling, etc; or at least more so
Tom> than I do.

In particular I wonder if palves/tty-always-separate-session helps solve
this in a different way.

Tom