[PATCH] gdb: preserve `|` in connection details string

[PATCH] gdb: preserve `|` in connection details string

[Andrew Burgess]

Consider this GDB session:

  $ gdb -q
  (gdb) target remote  | gdbserver - ~/tmp/hello.x
  Remote debugging using | gdbserver - ~/tmp/hello.x
  ... snip ...
  (gdb) info connections
    Num  What                              Description
  * 1    remote gdbserver - ~/tmp/hello.x  Remote target using gdb-specific protocol
  (gdb) python conn = gdb.selected_inferior().connection
  (gdb) python print(conn.details)
  gdbserver - ~/tmp/hello.x
  (gdb)

I think there are two things wrong here, first in the "What" column of
the 'info connections' output, I think the text should be:

  remote | gdbserver - ~/tmp/hello.x

to correctly show the user how the connection was established.  And in
a similar fashion, I think that the `details` string of the
gdb.TargetConnection object should be:

  | gdbserver - ~/tmp/hello.x

This commit makes this change.  Currently the '|' is detected and
removed in gdb/serial.c.  The string passed to the pipe_ops
structure (from gdb/ser-pipe.c), doesn't then, contain the `|`, this
is instead implied by the fact that it is a pipes based implementation
of the serial_ops interface.

After this commit we still detect the `|` in gdb/serial.c, but we now
store the full string (including the `|`) in the serial::name member
variable.

For pipe based serial connections, this name is only used for
displaying the two fields I mention above, and in pipe_open (from
gdb/ser-pipe.c), and in pipe_open, we now know to skip over the `|`.

The benefit I see from this change is that GDB's output now more
accurately reflects the commands used to start a target, thus making
it easier for a user to understand what is going on.
---
 gdb/ser-pipe.c                           |  6 ++
 gdb/serial.c                             |  7 +--
 gdb/testsuite/gdb.server/server-pipe.exp | 80 ++++++++++++++++++++++++
 3 files changed, 87 insertions(+), 6 deletions(-)
 create mode 100644 gdb/testsuite/gdb.server/server-pipe.exp

diff --git a/gdb/ser-pipe.c b/gdb/ser-pipe.c
index 06c3d3a174e..80bb73e0418 100644
--- a/gdb/ser-pipe.c
+++ b/gdb/ser-pipe.c
@@ -62,6 +62,12 @@ pipe_open (struct serial *scb, const char *name)
   int err_pdes[2];
   int pid;
 
+  if (*name == '|')
+    {
+      name++;
+      name = skip_spaces (name);
+    }
+
   if (gdb_socketpair_cloexec (AF_UNIX, SOCK_STREAM, 0, pdes) < 0)
     return -1;
   if (gdb_socketpair_cloexec (AF_UNIX, SOCK_STREAM, 0, err_pdes) < 0)
diff --git a/gdb/serial.c b/gdb/serial.c
index c6e3cb9bbbb..41ba753a4b8 100644
--- a/gdb/serial.c
+++ b/gdb/serial.c
@@ -201,12 +201,7 @@ serial_open (const char *name)
   const char *open_name = name;
 
   if (startswith (name, "|"))
-    {
-      ops = serial_interface_lookup ("pipe");
-      /* Discard ``|'' and any space before the command itself.  */
-      ++open_name;
-      open_name = skip_spaces (open_name);
-    }
+    ops = serial_interface_lookup ("pipe");
   /* Check for a colon, suggesting an IP address/port pair.
      Do this *after* checking for all the interesting prefixes.  We
      don't want to constrain the syntax of what can follow them.  */
diff --git a/gdb/testsuite/gdb.server/server-pipe.exp b/gdb/testsuite/gdb.server/server-pipe.exp
new file mode 100644
index 00000000000..ca2164b03e5
--- /dev/null
+++ b/gdb/testsuite/gdb.server/server-pipe.exp
@@ -0,0 +1,80 @@
+# This testcase is part of GDB, the GNU debugger.
+
+# Copyright 2022 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 starting gdbserver using the 'target remove | ....' syntax.
+# The extended-remote target type is tested too.  The test checks that
+# the `|` character can be seen in the 'info connections' output, and
+# (when Python is available) in the gdb.TargetConnection.details
+# string.
+
+# This test relies on starting gdbserver using the pipe syntax.  Not
+# sure how well this will run if part of this test is being run
+# elsewhere.
+if { [is_remote target] || [is_remote host] } {
+    return 0
+}
+
+load_lib gdbserver-support.exp
+
+standard_testfile
+
+if { [skip_gdbserver_tests] } {
+    return 0
+}
+
+set gdbserver [find_gdbserver]
+if { $gdbserver == "" } {
+    unsupported "could not find gdbserver"
+    return
+}
+
+standard_testfile normal.c
+set executable ${testfile}
+
+if {[build_executable "failed to prepare" $testfile $srcfile debug]} {
+    return -1
+}
+
+# Start GDB using remote type TARGET, which should be either "remote"
+# or "extended-remote".  Check the output of 'info connections', and
+# the contents of the gdb.TargetConnection.details string.
+proc do_test { target } {
+    clean_restart ${::binfile}
+
+    # Make sure we're disconnected, in case we're testing with an
+    # extended-remote board, therefore already connected.
+    gdb_test "disconnect" ".*"
+
+    gdb_test "target ${target} | ${::gdbserver} - ${::binfile}" ".*" \
+	"start gdbserver using pipe syntax"
+
+    gdb_breakpoint main
+    gdb_continue_to_breakpoint main
+    gdb_test "info frame" ".* in main .*"
+
+    gdb_test "info connections" "${target} \| ${::gdbserver} - ${::binfile} \[^\r\n\]+"
+
+    if { ![skip_python_tests] } {
+	gdb_test_no_output "python conn = gdb.selected_inferior().connection"
+	gdb_test "python print(conn.details)" "\| ${::gdbserver} - ${::binfile}"
+    }
+}
+
+# Test with the two remote protocol types.
+foreach_with_prefix target { remote extended-remote } {
+    do_test ${target}
+}
-- 
2.25.4

Re: [PATCH] gdb: preserve `|` in connection details string

[Tom Tromey]

>>>>> "Andrew" == Andrew Burgess via Gdb-patches <gdb-patches@sourceware.org> writes:

Andrew> This commit makes this change.  Currently the '|' is detected and
Andrew> removed in gdb/serial.c.  The string passed to the pipe_ops
Andrew> structure (from gdb/ser-pipe.c), doesn't then, contain the `|`, this
Andrew> is instead implied by the fact that it is a pipes based implementation
Andrew> of the serial_ops interface.

Andrew> After this commit we still detect the `|` in gdb/serial.c, but we now
Andrew> store the full string (including the `|`) in the serial::name member
Andrew> variable.

Looks good to me.  Thank you.

Tom

Re: [PATCH] gdb: preserve `|` in connection details string

[Andrew Burgess]

Tom Tromey <tom@tromey.com> writes:

>>>>>> "Andrew" == Andrew Burgess via Gdb-patches <gdb-patches@sourceware.org> writes:
>
> Andrew> This commit makes this change.  Currently the '|' is detected and
> Andrew> removed in gdb/serial.c.  The string passed to the pipe_ops
> Andrew> structure (from gdb/ser-pipe.c), doesn't then, contain the `|`, this
> Andrew> is instead implied by the fact that it is a pipes based implementation
> Andrew> of the serial_ops interface.
>
> Andrew> After this commit we still detect the `|` in gdb/serial.c, but we now
> Andrew> store the full string (including the `|`) in the serial::name member
> Andrew> variable.
>
> Looks good to me.  Thank you.

Thanks, pushed.

Andrew