[PATCH 0/6] Add vDefaultInferiorFd feature

[PATCH 0/6] Add vDefaultInferiorFd feature

[Alexandra Hájková]

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

[PATCH 1/6] gdb.server/non-existing-program.exp: Use gdbserver_start.

[Alexandra Hájková]

Also modify GDBserver_start to match more messages from the server
and return them to the caller.

This test tests if GDBserver exits cleanly when GDBserver is called using
stdio with some program that does not exist. My series modifies the GDBserver
in such a way, it defers starting the inferior till certain packets arrive
when stdio is used. The result was, when GDBserver was called using stdio with
some nonexistent file, it was waiting for those packets to come but since GDB
was never attached, they would never do so and the test timed out. Running
GDBserver using stdio without planning to attach there with GDB is not a
realistic situation and what the user would normally do. The test  was
modifyied to start GDBserver with a port number instead of stdio by calling
GDBserver_start proc which was modifyied to return a message from
GDBserver.
---
 .../gdb.server/non-existing-program.exp       | 54 +++++-----------
 gdb/testsuite/lib/gdbserver-support.exp       | 62 +++++++++++++------
 2 files changed, 58 insertions(+), 58 deletions(-)

diff --git a/gdb/testsuite/gdb.server/non-existing-program.exp b/gdb/testsuite/gdb.server/non-existing-program.exp
index eea1eb1d911..99b98e1222c 100644
--- a/gdb/testsuite/gdb.server/non-existing-program.exp
+++ b/gdb/testsuite/gdb.server/non-existing-program.exp
@@ -30,44 +30,20 @@ if { $gdbserver == "" } {
     return
 }
 
-# Fire off gdbserver.  The port doesn't really matter, gdbserver tries
-# to spawn the program before opening the connection.
-set spawn_id [remote_spawn target "$gdbserver stdio non-existing-program"]
-
-set msg "gdbserver exits cleanly"
-set saw_exiting 0
-expect {
-    # This is what we get on ptrace-based targets with
-    # startup-with-shell disabled (e.g., when the SHELL variable is
-    # unset).
-    -re "stdin/stdout redirected.*gdbserver: Cannot exec non-existing-program\r\ngdbserver: Error: No such file or directory\r\n\r\nDuring startup program exited with code 127\.\r\nExiting\r\n" {
-	set saw_exiting 1
-	exp_continue
-    }
-    # Likewise, but with startup-with-shell enabled, which is the
-    # default behaviour.
-    -re "stdin/stdout redirected.*exec: non-existing-program: not found\r\nDuring startup program exited with code 127\.\r\nExiting\r\n" {
-	set saw_exiting 1
-	exp_continue
-    }
-    # This is what we get on Windows.
-    -re "Error creating process\r\n\r\nExiting\r\n" {
-	set saw_exiting 1
-	exp_continue
-    }
-    -re "A problem internal to GDBserver has been detected" {
-	fail "$msg (GDBserver internal error)"
-	wait
-    }
-    eof {
-	gdb_assert $saw_exiting $msg
-	wait
+foreach_with_prefix start_with_shell { on off } {
+    if { $start_with_shell } {
+	set arg "--startup-with-shell"
+    } else {
+	set arg "--no-startup-with-shell"
     }
-    timeout {
-	fail "$msg (timeout)"
-    }
-}
 
-# expect defaults to spawn_id in many places.  Avoid confusing any
-# following code.
-unset spawn_id
+    set msg "GDBserver exits cleanly"
+    set res [gdbserver_start $arg non-existent-file]
+    set status [lindex $res 2]
+
+    # We expect GDBserver to fail with the message indicating executable
+    # does not exist.
+    gdb_assert { $status == "During startup program exited with code 127\.\r\nExiting\r\n"
+      || $status == "Error creating process\r\n\r\nExiting\r\n"} $msg
+
+}
diff --git a/gdb/testsuite/lib/gdbserver-support.exp b/gdb/testsuite/lib/gdbserver-support.exp
index 30d94fd7eb6..aa22315ef92 100644
--- a/gdb/testsuite/lib/gdbserver-support.exp
+++ b/gdb/testsuite/lib/gdbserver-support.exp
@@ -244,7 +244,8 @@ proc gdbserver_default_get_comm_port { port } {
 # Start a gdbserver process with initial OPTIONS and trailing ARGUMENTS.
 # The port will be filled in between them automatically.
 #
-# Returns the target protocol and socket to connect to.
+# Returns the target protocol, socket to connect to and the status
+# message from the gdbserver.
 
 proc gdbserver_start { options arguments } {
     global portnum
@@ -363,33 +364,56 @@ proc gdbserver_start { options arguments } {
 	# talk to the program using GDBserver's tty instead.
 	global inferior_spawn_id
 	set inferior_spawn_id $server_spawn_id
+        set msg 0
 
 	# Wait for the server to open its TCP socket, so that GDB can connect.
 	expect {
-	    -i $server_spawn_id
-	    -timeout 120
-	    -notransfer
-	    -re "Listening on" { }
-	    -re "Can't (bind address|listen on socket): Address already in use\\.\r\n" {
-		verbose -log "Port $portnum is already in use."
-		if ![target_info exists gdb,socketport] {
-		    # Bump the port number to avoid the conflict.
-		    wait -i $expect_out(spawn_id)
-		    incr portnum
-		    continue
-		}
-	    }
-	    -re ".*: cannot resolve name: .*\r\n" {
-		error "gdbserver cannot resolve name."
-	    }
-	    timeout {
+          -i $server_spawn_id
+          -timeout 120
+          -notransfer
+          -re "Listening on" {
+            set msg $expect_out(0,string)
+	  }
+	  -re "Can't (bind address|listen on socket): Address already in use\\.\r\n" {
+	    verbose -log "Port $portnum is already in use."
+	    set msg "Port is already in use"
+            if ![target_info exists gdb,socketport] {
+              # Bump the port number to avoid the conflict.
+              wait -i $expect_out(spawn_id)
+              incr portnum
+              continue
+            }
+          }
+          -re ".*: cannot resolve name: .*\r\n" {
+            error "gdbserver cannot resolve name."
+          }
+          # Likewise, but with startup-with-shell enabled, which is the
+          # default behaviour.
+          -re "During startup program exited with code 127\.\r\nExiting\r\n" {
+            set msg $expect_out(0,string)
+            exp_continue
+          }
+          -re "Can't bind address: Address already in use\.\r\nExiting\r\n" {
+            set msg $expect_out(0,string)
+            exp_continue
+          }
+          # This is what we get on Windows.
+          -re "Error creating process\r\n\r\nExiting\r\n" {
+            set msg $expect_out(0,string)
+            exp_continue
+          }
+          -re "A problem internal to GDBserver has been detected" {
+            set msg $expect_out(0,string)
+            wait
+          }
+          timeout {
 		error "Timeout waiting for gdbserver response."
 	    }
 	}
 	break
     }
 
-    return [list $protocol [$get_remote_address $debughost $portnum]]
+    return [list $protocol [$get_remote_address $debughost $portnum] $msg]
 }
 
 # Start a gdbserver process running SERVER_EXEC, and connect GDB
-- 
2.41.0

[PATCH 2/6] gdb/ser-pipe.c: Duplicate the file descriptors

[Alexandra Hájková]

Duplicate the numbers of STDOUT/STDIN/STDERR file descriptors
GDB is connected to. Preserved numbers of the file descriptors
could be then sent to the GDBserver. If GDBserver is run locally
and will accept he numbers of the file descriptors, it can start
the inferior connected to the same STDIN/OUT/ERR, GDB is connected to.
---
 gdb/ser-pipe.c | 25 +++++++++++++++++++++++++
 gdb/serial.c   |  4 ++++
 gdb/serial.h   |  4 ++++
 3 files changed, 33 insertions(+)

diff --git a/gdb/ser-pipe.c b/gdb/ser-pipe.c
index 47ccd33cece..84253cf2e2c 100644
--- a/gdb/ser-pipe.c
+++ b/gdb/ser-pipe.c
@@ -77,6 +77,16 @@ pipe_open (struct serial *scb, const char *name)
       return -1;
     }
 
+  /* Preserve STDIN/STDOUT/STDERR so they won't be closed on
+     exec later, after we fork.  */
+  int saved_stdin = dup (STDIN_FILENO);
+  int saved_stdout = dup (STDOUT_FILENO);
+  int saved_stderr = dup (STDERR_FILENO);
+
+  scb->fds[0] = saved_stdin;
+  scb->fds[1] = saved_stdout;
+  scb->fds[2] = saved_stderr;
+
   /* Create the child process to run the command in.  Note that the
      apparent call to vfork() below *might* actually be a call to
      fork() due to the fact that autoconf will ``#define vfork fork''
@@ -90,6 +100,12 @@ pipe_open (struct serial *scb, const char *name)
       close (pdes[1]);
       close (err_pdes[0]);
       close (err_pdes[1]);
+      close (saved_stdout);
+      close (saved_stdin);
+      close (saved_stderr);
+      scb->fds[0] = -1;
+      scb->fds[1] = -1;
+      scb->fds[2] = -1;
       return -1;
     }
 
@@ -128,6 +144,10 @@ pipe_open (struct serial *scb, const char *name)
 	  close (err_pdes[1]);
 	}
 
+      mark_fd_no_cloexec (saved_stdout);
+      mark_fd_no_cloexec (saved_stdin);
+      mark_fd_no_cloexec (saved_stderr);
+
       close_most_fds ();
 
       const char *shellfile = get_shell ();
@@ -139,6 +159,11 @@ pipe_open (struct serial *scb, const char *name)
   close (pdes[1]);
   if (err_pdes[1] != -1)
     close (err_pdes[1]);
+
+  close (saved_stdout);
+  close (saved_stdin);
+  close (saved_stderr);
+
   /* :end chunk */
   state = XNEW (struct pipe_state);
   state->pid = pid;
diff --git a/gdb/serial.c b/gdb/serial.c
index 8a8bab46ead..a2bbe9a972d 100644
--- a/gdb/serial.c
+++ b/gdb/serial.c
@@ -182,6 +182,10 @@ new_serial (const struct serial_ops *ops)
 
   scb->ops = ops;
 
+  scb->fds[0] = -1;
+  scb->fds[1] = -1;
+  scb->fds[2] = -1;
+
   scb->bufp = scb->buf;
   scb->error_fd = -1;
   scb->refcnt = 1;
diff --git a/gdb/serial.h b/gdb/serial.h
index 3b861200302..113f7360a55 100644
--- a/gdb/serial.h
+++ b/gdb/serial.h
@@ -254,6 +254,10 @@ struct serial
     int async_state;		/* Async internal state.  */
     void *async_context;	/* Async event thread's context */
     serial_event_ftype *async_handler;/* Async event handler */
+
+    /* File descriptors for preserved STDIN/STDOUT/STDERR
+       to be sent to GDBserver when run locally.  */
+    int fds[3];
   };
 
 struct serial_ops
-- 
2.41.0

[PATCH 3/6] Add new vDefaultInferiorFd packet

[Alexandra Hájková]

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(-)

diff --git a/gdb/doc/gdb.texinfo b/gdb/doc/gdb.texinfo
index e4c00143fd1..69f0d0499ef 100644
--- a/gdb/doc/gdb.texinfo
+++ b/gdb/doc/gdb.texinfo
@@ -23210,6 +23210,15 @@ If you specify the program to debug on the command line, then the
 resume using commands like @kbd{step} and @kbd{continue} as with
 @code{target remote} mode.
 
+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.
+
+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.
+
 @anchor{Attaching in Types of Remote Connections}
 @item Attaching
 @strong{With target remote mode:} The @value{GDBN} command @code{attach} is
@@ -42934,6 +42943,29 @@ for success (@pxref{Stop Reply Packets})
 @cindex @samp{vStopped} packet
 @xref{Notification Packets}.
 
+@item vDefaultInferior;@var{fd0};@var{fd1};@var{fd2}
+@item vDefaultInferiorFd
+@anchor{default inferior file descriptors}
+@cindex @samp{vDefaultInferiorFd} packet
+@value{GDBN} would preserve the numbers of STDOUT/STDIN/STDERR file descriptors.
+Preserved numbers of the file descriptors are then sent to GDBserver.
+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.
+
+This packet is also available in extended mode (@pxref{extended mode}).
+This feature does not support setting inferior tty.
+
+Reply:
+@table @samp
+@item OK
+for success
+@item E.errtext
+for an error
+@end table
+
 @item X @var{addr},@var{length}:@var{XX@dots{}}
 @anchor{X packet}
 @cindex @samp{X} packet
diff --git a/gdb/remote.c b/gdb/remote.c
index ce5addade6f..27d053c367c 100644
--- a/gdb/remote.c
+++ b/gdb/remote.c
@@ -305,6 +305,10 @@ enum {
      packets and the tag violation stop replies.  */
   PACKET_memory_tagging_feature,
 
+  /* Support for connecting inferior to the same terminal as
+     GDB when running GDBserver locally.  */
+  PACKET_vDefaultInferiorFd,
+
   PACKET_MAX
 };
 
@@ -726,6 +730,11 @@ struct remote_features
   bool remote_memory_tagging_p () const
   { return packet_support (PACKET_memory_tagging_feature) == PACKET_ENABLE; }
 
+  /* Returns true if connecting inferior to the same terminal as GDB is
+     supported, false otherwise.  */
+  bool remote_fd_switch_p () const
+  { return packet_support (PACKET_vDefaultInferiorFd) == PACKET_ENABLE; }
+
   /* Reset all packets back to "unknown support".  Called when opening a
      new connection to a remote target.  */
   void reset_all_packet_configs_support ();
@@ -5010,6 +5019,18 @@ remote_target::start_remote_1 (int from_tty, int extended_p)
      which later probes to skip.  */
   remote_query_supported ();
 
+  if ((m_features.packet_support (PACKET_vDefaultInferiorFd) != PACKET_DISABLE)
+      && (rs->remote_desc->fds[0] != -1) && (rs->remote_desc->fds[1] != -1)
+      && (rs->remote_desc->fds[2] != -1))
+  {
+      xsnprintf (rs->buf.data(), rs->buf.size (), "vDefaultInferiorFd;%d;%d;%d",
+		 rs->remote_desc->fds[0], rs->remote_desc->fds[1],
+		 rs->remote_desc->fds[2]);
+
+      putpkt (rs->buf);
+      getpkt (&rs->buf, 0);
+  }
+
   /* Check vCont support and set the remote state's vCont_action_support
      attribute.  */
   remote_vcont_probe ();
@@ -5717,6 +5738,7 @@ static const struct protocol_feature remote_protocol_features[] = {
   { "no-resumed", PACKET_DISABLE, remote_supported_packet, PACKET_no_resumed },
   { "memory-tagging", PACKET_DISABLE, remote_supported_packet,
     PACKET_memory_tagging_feature },
+  { "vDefaultInferiorFd", PACKET_DISABLE, remote_supported_packet, PACKET_vDefaultInferiorFd },
 };
 
 static char *remote_support_xml;
@@ -5828,6 +5850,10 @@ remote_target::remote_query_supported ()
 	  != AUTO_BOOLEAN_FALSE)
 	remote_query_supported_append (&q, "memory-tagging+");
 
+      if (m_features.packet_set_cmd_state (PACKET_vDefaultInferiorFd)
+	  != AUTO_BOOLEAN_FALSE)
+	remote_query_supported_append (&q, "vDefaultInferiorFd+");
+
       /* Keep this one last to work around a gdbserver <= 7.10 bug in
 	 the qSupported:xmlRegisters=i386 handling.  */
       if (remote_support_xml != NULL
@@ -15984,6 +16010,8 @@ Show the maximum size of the address (in bits) in a memory packet."), NULL,
   add_packet_config_cmd (PACKET_memory_tagging_feature,
 			 "memory-tagging-feature", "memory-tagging-feature", 0);
 
+  add_packet_config_cmd (PACKET_vDefaultInferiorFd, "vDefaultInferiorFd", "vDefaultInferiorFd", 0);
+
   /* Assert that we've registered "set remote foo-packet" commands
      for all packet configs.  */
   {
diff --git a/gdbserver/server.cc b/gdbserver/server.cc
index a8e23561dcb..e69e5c125c4 100644
--- a/gdbserver/server.cc
+++ b/gdbserver/server.cc
@@ -81,6 +81,12 @@ static bool extended_protocol;
 static bool response_needed;
 static bool exit_requested;
 
+/* To be able to connect the inferior to GDB's stdin/out/err,
+   when GDBserver is run locally, we need to wait with starting
+   the inferior to have time to recieve the vDefaultInferiorFd
+   packet.  Set to true if need to postpone starting the inferior.  */
+static bool deferred_inferior_startup = false;
+
 /* --once: Exit after the first connection has closed.  */
 bool run_once;
 
@@ -171,6 +177,34 @@ get_client_state ()
   return cs;
 }
 
+volatile int multi_mode = 0;
+
+/* To be able to connect the inferior to GDB's stdin/out/err,
+   when GDBserver is run locally, we need to wait with starting
+   the inferior to have time to recieve the vDefaultInferiorFd
+   packet.
+ 
+   Can only be called when deferred_inferior_startup is true.
+   Starts the inferior and throws an error if startup fails.
+   If startup is successful then deferred_inferior_startup is
+   reset to false. Throw an error on failure.  */
+static void
+do_deferred_startup ()
+{
+  gdb_assert (deferred_inferior_startup);
+  client_state &cs = get_client_state ();
+  target_create_inferior (program_path.get (), program_args);
+
+  if (current_thread != nullptr)
+    current_process ()->dlls_changed = false;
+
+  if ((cs.last_status.kind () == TARGET_WAITKIND_EXITED
+       || cs.last_status.kind () == TARGET_WAITKIND_SIGNALLED)
+       && !multi_mode)
+      error ("No program to debug");
+
+  deferred_inferior_startup = false;
+}
 
 /* Put a stop reply to the stop reply queue.  */
 
@@ -2274,6 +2308,45 @@ supported_btrace_packets (char *buf)
   strcat (buf, ";qXfer:btrace-conf:read+");
 }
 
+/* Parse a vDefaultInferiorFd packet from pkt
+   and save the received file descriptors into
+   fds.  Return true  on success, false otherwise.
+   Set the file descriptors to -1 on failure.  */
+static bool
+parse_vdefaultinf (const char *pkt, int *fds)
+{
+  char *end;
+  int ret = true;
+  errno = 0;
+
+  if (*pkt != '\0')
+    {
+      fds[0] = (int) strtoul (pkt, &end, 10);
+      if ((pkt == end) || (*end != ';'))
+        ret = false;
+      pkt = end + 1;
+      fds[1] = (int) strtoul (pkt, &end, 10);
+      if ((pkt == end) || (*end != ';'))
+        ret = false;
+      pkt = end + 1;
+      fds[2] = (int) strtoul (pkt, &end, 10);
+      if ((pkt == end) || (*end != '\0'))
+        ret = false;
+      if (errno != 0)
+        ret = false;
+    }
+  else
+    ret = false;
+  if (!ret)
+  {
+    fds[0] = -1;
+    fds[1] = -1;
+    fds[2] = -1;
+  }
+
+  return ret;
+}
+
 /* Handle all of the extended 'q' packets.  */
 
 static void
@@ -2484,6 +2557,8 @@ handle_query (char *own_buf, int packet_len, int *new_packet_len_p)
 		  if (target_supports_memory_tagging ())
 		    cs.memory_tagging_feature = true;
 		}
+	      else if (feature == "vDefaultInferiorFd+")
+		cs.vDefaultInferiorFd_supported = 1;
 	      else
 		{
 		  /* Move the unknown features all together.  */
@@ -2613,6 +2688,17 @@ handle_query (char *own_buf, int packet_len, int *new_packet_len_p)
       if (target_supports_memory_tagging ())
 	strcat (own_buf, ";memory-tagging+");
 
+      if (cs.vDefaultInferiorFd_supported)
+      {
+         /* If GDB didn't advertise vDefaultInferiorFd then we don't mention
+            vDefaultInferiorFd in the reply.  If GDB did mention vDefaultInferiorFd
+            then we only claim support if we are started as an stdio target.  */
+          if (remote_connection_is_stdio ())
+	    strcat (own_buf, ";vDefaultInferiorFd+");
+	  else
+	    strcat (own_buf, ";vDefaultInferiorFd-");
+	}
+
       /* Reinitialize components as needed for the new connection.  */
       hostio_handle_new_gdb_connection ();
       target_handle_new_gdb_connection ();
@@ -3316,6 +3402,19 @@ handle_v_requests (char *own_buf, int packet_len, int *new_packet_len)
       return;
     }
 
+  if (startswith (own_buf, "vDefaultInferiorFd;"))
+  {
+    if (!parse_vdefaultinf (own_buf + 19, cs.fds))
+      {
+	strcpy (own_buf, "E.invalid vDefaultInferiorFd packet");
+	return;
+      }
+
+    cs.vDefaultInferiorFd_accepted = true;
+    write_ok (own_buf);
+    return;
+  }
+
   if (startswith (own_buf, "vKill;"))
     {
       if (!target_running ())
@@ -3807,7 +3906,6 @@ captured_main (int argc, char *argv[])
   char *arg_end;
   const char *port = NULL;
   char **next_arg = &argv[1];
-  volatile int multi_mode = 0;
   volatile int attach = 0;
   int was_running;
   bool selftest = false;
@@ -4056,8 +4154,27 @@ captured_main (int argc, char *argv[])
       for (i = 1; i < n; i++)
 	program_args.push_back (xstrdup (next_arg[i]));
 
-      /* Wait till we are at first instruction in program.  */
-      target_create_inferior (program_path.get (), program_args);
+      if (remote_connection_is_stdio ())
+	{
+	  /* Debugger might want to set the default inferior
+	     in/out/err file descriptors, in which case we need to
+	     defer starting the inferior until this information
+	     arrives.  */
+	  deferred_inferior_startup = true;
+          multi_mode = 1;
+
+	  /* Only used when reporting the first stop to GDB.  This
+	     should be replaced when we start the inferior, but lets
+	     be on the safe side and set this now just so we carry
+	     around a sane state.  */
+	  cs.last_status.set_exited (0);
+	  cs.last_ptid = minus_one_ptid;
+	}
+      else
+	{
+	  /* Wait till we are at first instruction in program.  */
+	  target_create_inferior (program_path.get (), program_args);
+	}
 
       /* We are now (hopefully) stopped at the first instruction of
 	 the target process.  This assumes that the target process was
@@ -4107,6 +4224,7 @@ captured_main (int argc, char *argv[])
       cs.hwbreak_feature = 0;
       cs.vCont_supported = 0;
       cs.memory_tagging_feature = false;
+      cs.vDefaultInferiorFd_supported = false;
 
       remote_open (port);
 
@@ -4295,6 +4413,14 @@ process_serial_event (void)
   response_needed = true;
 
   char ch = cs.own_buf[0];
+  if (deferred_inferior_startup)
+  {
+    /* Actually start the inferior if not a qSupported or
+       vDefaultInferiorFd packet.  */
+    if (!startswith(cs.own_buf, "qSupported")
+       && !startswith(cs.own_buf, "vDefaultInferiorFd"))
+      do_deferred_startup ();
+  }
   switch (ch)
     {
     case 'q':
diff --git a/gdbserver/server.h b/gdbserver/server.h
index 306d75d4e9b..7afdc9f6cd4 100644
--- a/gdbserver/server.h
+++ b/gdbserver/server.h
@@ -192,6 +192,18 @@ struct client_state
   /* If true, memory tagging features are supported.  */
   bool memory_tagging_feature = false;
 
+  /* If true, connecting the inferior to the terminal's
+     stdin/out/err when running GDBserver locally is supported.  */
+  bool vDefaultInferiorFd_supported = false;
+
+  /* If true, GDBserver accepted file descriptors sent by GDB
+    and connecting the inferior to the same terminal as GDB
+    when running GDBserver locally is then supported.  */
+  bool vDefaultInferiorFd_accepted = false;
+
+  /* File descriptors pointing to terminal's stdin/out/err
+     sent by GDB to GDBserver.  */
+  int fds[3];
 };
 
 client_state &get_client_state ();
-- 
2.41.0

[PATCH 4/6] gdbserver/linux-low.cc: Connect the inferior to the terminal

[Alexandra Hájková]

If GDBserver is connected to GDB via stdio and it recieved file
descriptors of the STDIN/OUT/ERR of the terminal GDB is connected to,
GDBserver will start the inferior connected to them.
---
 gdbserver/linux-low.cc | 32 +++++++++++++++++++++++++++++++-
 1 file changed, 31 insertions(+), 1 deletion(-)

diff --git a/gdbserver/linux-low.cc b/gdbserver/linux-low.cc
index f9001e2fa17..04d72d22050 100644
--- a/gdbserver/linux-low.cc
+++ b/gdbserver/linux-low.cc
@@ -952,6 +952,8 @@ linux_process_target::low_new_thread (lwp_info *info)
 static void
 linux_ptrace_fun ()
 {
+  client_state &cs = get_client_state ();
+
   if (ptrace (PTRACE_TRACEME, 0, (PTRACE_TYPE_ARG3) 0,
 	      (PTRACE_TYPE_ARG4) 0) < 0)
     trace_start_error_with_name ("ptrace");
@@ -959,10 +961,38 @@ linux_ptrace_fun ()
   if (setpgid (0, 0) < 0)
     trace_start_error_with_name ("setpgid");
 
+  /* If GDBserver is connected to GDB via stdio and it recieved the file
+     descriptors for the terminal stdin/out/err from GDB, start the inferior
+     connected to them.  */
+  if (remote_connection_is_stdio () && cs.vDefaultInferiorFd_accepted)
+  {
+    struct stat buf{};
+
+    if ((fstat(cs.fds[0], &buf) == -1)
+        || (fstat(cs.fds[1], &buf) == -1)
+        || (fstat(cs.fds[2], &buf) == -1))
+    {
+      write (2, "Fstat failed. Can't connect inferior to the terminal.\n",
+             sizeof ("Fstat failed. Can't connect inferior to the terminal.\n") - 1);
+      cs.vDefaultInferiorFd_accepted = false;
+    }
+
+    /* Dupped file descriptors will be inherited by the inferior.  */
+    if (!cs.vDefaultInferiorFd_accepted
+        || (dup2 (cs.fds[0], 0) == -1)
+        || (dup2 (cs.fds[1], 1) == -1)
+        || (dup2 (cs.fds[2], 2) == -1))
+      {
+        write (2, "Dup2 failed. Can't connect inferior to the terminal.\n",
+               sizeof ("Dup2 failed. Can't connect inferior to the terminal.\n") - 1);
+        cs.vDefaultInferiorFd_accepted = false;
+      }
+  }
+
   /* If GDBserver is connected to gdb via stdio, redirect the inferior's
      stdout to stderr so that inferior i/o doesn't corrupt the connection.
      Also, redirect stdin to /dev/null.  */
-  if (remote_connection_is_stdio ())
+  if (remote_connection_is_stdio () && !cs.vDefaultInferiorFd_accepted)
     {
       if (close (0) < 0)
 	trace_start_error_with_name ("close");
-- 
2.41.0

[PATCH 5/6] remote.c: Add terminal handling functions

[Alexandra Hájková]

Re-use terminal handling functions from inf-child.c so GDBserver can
call them. This helps local remote target to behave more like native
target.
---
 gdb/remote.c | 55 ++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 55 insertions(+)

diff --git a/gdb/remote.c b/gdb/remote.c
index 27d053c367c..68835a29244 100644
--- a/gdb/remote.c
+++ b/gdb/remote.c
@@ -607,6 +607,10 @@ class remote_state
      qSupported.  */
   gdb_thread_options supported_thread_options = 0;
 
+  /* GDBserver accepted the file descriptor numbers of the terminal
+     GDB is connected to.  */
+  bool FdS_accepted = false;
+
 private:
   /* Asynchronous signal handle registered as event loop source for
      when we have pending events ready to be passed to the core.  */
@@ -908,10 +912,18 @@ class remote_target : public process_stratum_target
 
   int can_do_single_step () override;
 
+  bool supports_terminal_ours () override;
+
+  void terminal_init () override;
+
   void terminal_inferior () override;
 
+  void terminal_save_inferior() override;
+
   void terminal_ours () override;
 
+  void terminal_ours_for_output() override;
+
   bool supports_non_stop () override;
 
   bool supports_multi_process () override;
@@ -1732,6 +1744,36 @@ remote_target::get_remote_state ()
   return &m_remote_state;
 }
 
+bool
+remote_target::supports_terminal_ours ()
+{
+  return true;
+}
+
+void
+remote_target::terminal_init ()
+{
+  struct remote_state *rs = get_remote_state ();
+  if (rs->FdS_accepted)
+    child_terminal_init (this);
+}
+
+void
+remote_target::terminal_save_inferior ()
+{
+  struct remote_state *rs = get_remote_state ();
+  if (rs->FdS_accepted)
+    child_terminal_save_inferior (this);
+}
+
+void
+remote_target::terminal_ours_for_output ()
+{
+  struct remote_state *rs = get_remote_state ();
+  if (rs->FdS_accepted)
+    child_terminal_ours_for_output (this);
+}
+
 /* Fetch the remote exec-file from the current program space.  */
 
 static const char *
@@ -5029,6 +5071,12 @@ remote_target::start_remote_1 (int from_tty, int extended_p)
 
       putpkt (rs->buf);
       getpkt (&rs->buf, 0);
+
+      if (m_features.packet_ok (rs->buf, PACKET_vDefaultInferiorFd) == PACKET_OK)
+        rs->FdS_accepted = true;
+      else
+        error (_("Remote replied unexpectedly to 'vDefaultInferiorFd': %s"),
+               rs->buf.data ());
   }
 
   /* Check vCont support and set the remote state's vCont_action_support
@@ -7558,11 +7606,18 @@ remote_target::terminal_inferior ()
   /* NOTE: At this point we could also register our selves as the
      recipient of all input.  Any characters typed could then be
      passed on down to the target.  */
+
+    struct remote_state *rs = get_remote_state ();
+    if (rs->FdS_accepted)
+        child_terminal_inferior (this);
 }
 
 void
 remote_target::terminal_ours ()
 {
+    struct remote_state *rs = get_remote_state ();
+    if (rs->FdS_accepted)
+        child_terminal_ours (this);
 }
 
 static void
-- 
2.41.0

[PATCH 6/6] Add defaultinf.exp test to the testsuite

[Alexandra Hájková]

Test the behaviour of the inferior when GDBserver is run locally
and using stdio. The inferior should be able to write to STOUT
and STDERR and read from STDIN.
---
 gdb/testsuite/gdb.server/defaultinf.c   | 39 ++++++++++++++++
 gdb/testsuite/gdb.server/defaultinf.exp | 59 +++++++++++++++++++++++++
 2 files changed, 98 insertions(+)
 create mode 100644 gdb/testsuite/gdb.server/defaultinf.c
 create mode 100644 gdb/testsuite/gdb.server/defaultinf.exp

diff --git a/gdb/testsuite/gdb.server/defaultinf.c b/gdb/testsuite/gdb.server/defaultinf.c
new file mode 100644
index 00000000000..8963c7dbabf
--- /dev/null
+++ b/gdb/testsuite/gdb.server/defaultinf.c
@@ -0,0 +1,39 @@
+/* Copyright 2023 Free Software Foundation, Inc.
+
+   This program is free software; you can redistribute it and/or modify
+   it under the terms of the GNU General Public License as published by
+   the Free Software Foundation; either version 3 of the License, or
+   (at your option) any later version.
+
+   This program is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+   GNU General Public License for more details.
+
+   You should have received a copy of the GNU General Public License
+   along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
+
+#include <stdio.h>
+#include <stdlib.h>
+
+int
+main ()
+{
+  printf ("Hello stdout!\n");
+  fprintf (stderr, "Hello stderr!\n");
+
+  char *line = NULL;
+  size_t len = 0;
+  ssize_t nread;
+
+  while ((nread = getline (&line, &len, stdin)) != -1)
+    {
+      if (line[nread - 1] == '\n')
+	line[nread - 1] = '\0';
+      printf ("Got line len %zd: '%s'\n", nread, line);
+    }
+
+  printf ("Got EOF\n");
+  free (line);
+  return 0;
+}
diff --git a/gdb/testsuite/gdb.server/defaultinf.exp b/gdb/testsuite/gdb.server/defaultinf.exp
new file mode 100644
index 00000000000..03163e150e0
--- /dev/null
+++ b/gdb/testsuite/gdb.server/defaultinf.exp
@@ -0,0 +1,59 @@
+# This testcase is part of GDB, the GNU debugger.
+
+# Copyright 2023 Free Software Foundation, Inc.
+
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 3 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see <http://www.gnu.org/licenses/>.
+
+# Test the behaviour of the inferior when GDBserver is run locally
+# and using stdio. The inferior should be able to write to STOUT
+# and STDERR and read from STDIN.
+
+require {!is_remote target} {!is_remote host}
+
+load_lib gdbserver-support.exp
+
+require allow_gdbserver_tests
+
+set gdbserver [find_gdbserver]
+if { $gdbserver == "" } {
+    unsupported "could not find GDBserver"
+    return
+}
+
+standard_testfile
+
+if {[build_executable "failed to prepare" $testfile $srcfile debug]} {
+    return -1
+}
+
+clean_restart ${testfile}
+
+gdb_test "target remote | ${::gdbserver} - [standard_output_file $testfile]" ".*" \
+    "start gdbserver using pipe syntax"
+
+  set input_msg "hello world"
+  set input_len [expr [string length ${input_msg}] + 1]
+  gdb_test_multiple "c" "Test STDOUT/ERR, STDIN" {
+    -re "Hello stdout!\r\nHello stderr!" {
+      send_gdb "${input_msg}\n"
+      exp_continue
+    }
+    -re  "${input_msg}\r\nGot line len ${input_len}: '${input_msg}'" {
+      send_gdb "\004"
+      exp_continue
+    }
+    -re "Got EOF" {
+      pass "$gdb_test_name"
+    }
+  }
-- 
2.41.0

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

[Eli Zaretskii]

> 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>

Re: [PATCH 0/6] Add vDefaultInferiorFd feature

[Alexandra Petlanova Hajkova]

[-- Attachment #1: Type: text/plain, Size: 2193 bytes --]

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
>
>

Re: [PATCH 0/6] Add vDefaultInferiorFd feature

[Tom Tromey]

>>>>> "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

Re: [PATCH 0/6] Add vDefaultInferiorFd feature

[Andrew Burgess]

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

Re: [PATCH 0/6] Add vDefaultInferiorFd feature

[Alexandra Petlanova Hajkova]

[-- Attachment #1: Type: text/plain, Size: 1202 bytes --]

>
> 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

Re: [PATCH 0/6] Add vDefaultInferiorFd feature

[Tom Tromey]

>>>>> "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

Re: [PATCH 0/6] Add vDefaultInferiorFd feature

[Andrew Burgess]

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

Re: [PATCH 2/6] gdb/ser-pipe.c: Duplicate the file descriptors

[Tom Tromey]

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

Alexandra> Duplicate the numbers of STDOUT/STDIN/STDERR file descriptors
Alexandra> GDB is connected to. Preserved numbers of the file descriptors
Alexandra> could be then sent to the GDBserver. If GDBserver is run locally
Alexandra> and will accept he numbers of the file descriptors, it can start
Alexandra> the inferior connected to the same STDIN/OUT/ERR, GDB is connected to.

Thanks for the patch.

Alexandra> +  /* Preserve STDIN/STDOUT/STDERR so they won't be closed on
Alexandra> +     exec later, after we fork.  */
Alexandra> +  int saved_stdin = dup (STDIN_FILENO);
Alexandra> +  int saved_stdout = dup (STDOUT_FILENO);
Alexandra> +  int saved_stderr = dup (STDERR_FILENO);

I wonder what should happen if any of these fail.

Alexandra> @@ -128,6 +144,10 @@ pipe_open (struct serial *scb, const char *name)
Alexandra>  	  close (err_pdes[1]);
Alexandra>  	}
 
Alexandra> +      mark_fd_no_cloexec (saved_stdout);
Alexandra> +      mark_fd_no_cloexec (saved_stdin);
Alexandra> +      mark_fd_no_cloexec (saved_stderr);
Alexandra> +

This happens in the child process, but due to vfork, it actually affects
the parent process as well.  vfork is weird.

However I think this introduces a funny bug, in that these particular
descriptors are marked as no-cloexec by number.  So, while they are
closed again in the parent:

Alexandra> +  close (saved_stdout);
Alexandra> +  close (saved_stdin);
Alexandra> +  close (saved_stderr);

... their numbers are preserved for not-closing, and so it's possible
they could be reused and inherited by accident by some future
subprocess.

Probably these closes in the parent should be paired with calls to
unmark_fd_no_cloexec.

I'd suggest hoisting the calls to mark_fd_no_cloexec near the dup()s,
too, just to make it more clear.

Tom

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

[Tom Tromey]

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

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

I think the packet name is wrong on this line.

Alexandra> +@item vDefaultInferiorFd
Alexandra> +@anchor{default inferior file descriptors}
Alexandra> +@cindex @samp{vDefaultInferiorFd} packet
Alexandra> +@value{GDBN} would preserve the numbers of STDOUT/STDIN/STDERR file descriptors.
Alexandra> +Preserved numbers of the file descriptors are then sent to GDBserver.
Alexandra> +If GDBserver is run locally and will accept the numbers of the file
Alexandra> +descriptors, it will start the inferior connected to the same STDIN/OUT/ERR
Alexandra> +@value{GDBN} is connected to. This allows the inferior run under the local GDBserver

To me, the use of "sent" here makes it sound like this is using the
(obscure) SCM_RIGHTS feature, that lets one send file descriptors over a
socket.

However, it isn't, instead it is invoking gdbserver with some extra file
descriptors already opened, and then telling the server which ones to
use.

I think the text should be reworded to make this more clear.
 
Alexandra> +  /* Returns true if connecting inferior to the same terminal as GDB is
Alexandra> +     supported, false otherwise.  */
Alexandra> +  bool remote_fd_switch_p () const
Alexandra> +  { return packet_support (PACKET_vDefaultInferiorFd) == PACKET_ENABLE; }

This seems a little odd to me in that if I set up some kind of 'nc'
bridge and use it to connect to a remote valgrind-gdbserver, it will try
to use the gdb file descriptors but that just won't make sense at all.

I guess I'm wondering if this should try to use whatever mechanism we
came up with for checking if the remote is really remote.  I don't
remember if that ended up going in or not.

Alexandra> +  if ((m_features.packet_support (PACKET_vDefaultInferiorFd) != PACKET_DISABLE)
Alexandra> +      && (rs->remote_desc->fds[0] != -1) && (rs->remote_desc->fds[1] != -1)
Alexandra> +      && (rs->remote_desc->fds[2] != -1))

These are over-parenthesized.

Alexandra> +volatile int multi_mode = 0;

I think you can drop the volatile.  I suspect that was an artifact of
the old longjmp-based exception system.

Alexandra> @@ -4295,6 +4413,14 @@ process_serial_event (void)
Alexandra>    response_needed = true;
 
Alexandra>    char ch = cs.own_buf[0];
Alexandra> +  if (deferred_inferior_startup)
Alexandra> +  {
Alexandra> +    /* Actually start the inferior if not a qSupported or
Alexandra> +       vDefaultInferiorFd packet.  */
Alexandra> +    if (!startswith(cs.own_buf, "qSupported")
Alexandra> +       && !startswith(cs.own_buf, "vDefaultInferiorFd"))
Alexandra> +      do_deferred_startup ();
Alexandra> +  }

I had to read this a few times before I felt like it made sense.

Since there is an ordering dependency here, I think that should be
documented.  That is, vDefaultInferiorFd is only accepted after
qSupported and before any other request.

Tom

Re: [PATCH 4/6] gdbserver/linux-low.cc: Connect the inferior to the terminal

[Tom Tromey]

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

Alexandra> --- a/gdbserver/linux-low.cc
Alexandra> +++ b/gdbserver/linux-low.cc
Alexandra> @@ -952,6 +952,8 @@ linux_process_target::low_new_thread (lwp_info *info)

If this feature is Linux-specific then I think there has to be a new
target method so that gdbserver doesn't advertise it on platforms where
it won't actually work.

Alexandra> +    if ((fstat(cs.fds[0], &buf) == -1)
Alexandra> +        || (fstat(cs.fds[1], &buf) == -1)
Alexandra> +        || (fstat(cs.fds[2], &buf) == -1))

These are over-parenthesized.

Alexandra> +      write (2, "Fstat failed. Can't connect inferior to the terminal.\n",
Alexandra> +             sizeof ("Fstat failed. Can't connect inferior to the terminal.\n") - 1);

This looked weird but now I see that other code here does this.

Tom

Re: [PATCH 5/6] remote.c: Add terminal handling functions

[Tom Tromey]

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

Alexandra> Re-use terminal handling functions from inf-child.c so GDBserver can
Alexandra> call them. This helps local remote target to behave more like native
Alexandra> target.

Alexandra> +  /* GDBserver accepted the file descriptor numbers of the terminal
Alexandra> +     GDB is connected to.  */
Alexandra> +  bool FdS_accepted = false;

This sort of capitalization isn't really gdb style.

Tom

Re: [PATCH 0/6] Add vDefaultInferiorFd feature

[Tom Tromey]

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