[binutils-gdb] gdb: preserve `|` in connection details string

[binutils-gdb] gdb: preserve `|` in connection details string

[Andrew Burgess]

https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;h=cced7cacecad104fff03e94814a3fca1d304b16f

commit cced7cacecad104fff03e94814a3fca1d304b16f
Author: Andrew Burgess <aburgess@redhat.com>
Date:   Wed Jan 12 17:13:38 2022 +0000

    gdb: preserve `|` in connection details string
    
    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.

Diff:
---
 gdb/ser-pipe.c                           |  6 +++
 gdb/serial.c                             |  7 +--
 gdb/testsuite/gdb.server/server-pipe.exp | 80 ++++++++++++++++++++++++++++++++
 3 files changed, 87 insertions(+), 6 deletions(-)

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