[PATCH v2] [gdbserver] Imply --once if connecting via stdio

[Andrew Burgess]

Hi William,

I spoke to Guinevere offline and I think I've been convinced that this
approach is basically OK.  I do have some things that need fixing
though, but it's pretty minor...

William Ferreira <wqferr@gmail.com> writes:

> Currently, gdbserver hangs after stdin is closed while it tries to
> write: "Remote side has terminated connection.  GDBserver will reopen
> the connection." This hang disappears if --once is also given. Since
> the stdin connection won't ever reopen if it's closed, it's safe to
> assume --once is desired.
>
> The gdb.server/server-pipe.exp test was also updated to reflect this
> change. The "clean slate" disconnect at the start of the proc was moved
> to the end of said proc, and now is encased in a tighter timeout.
>
> Co-Authored-By: Guinevere Larsen <blarsen@redhat.com>
> Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=29796
> ---
>  gdb/testsuite/gdb.server/server-pipe.exp | 11 +++++++----
>  gdbserver/server.cc                      |  4 ++++
>  2 files changed, 11 insertions(+), 4 deletions(-)
>
> diff --git a/gdb/testsuite/gdb.server/server-pipe.exp b/gdb/testsuite/gdb.server/server-pipe.exp
> index b369759b98a..d86598335a2 100644
> --- a/gdb/testsuite/gdb.server/server-pipe.exp
> +++ b/gdb/testsuite/gdb.server/server-pipe.exp
> @@ -49,12 +49,9 @@ if {[build_executable "failed to prepare" $testfile $srcfile debug]} {
>  # or "extended-remote".  Check the output of 'info connections', and
>  # the contents of the gdb.TargetConnection.details string.
>  proc do_test { target } {
> +    global timeout
>      clean_restart ${::binfile}
>  
> -    # Make sure we're disconnected, in case we're testing with an
> -    # extended-remote board, therefore already connected.
> -    gdb_test "disconnect" ".*"
> -

This block needs to stay.  When we're testing with the
native-extended-gdbserver board we'll already be connected to a
gdbserver at this point, so we need to disconnect before we can perform
the test.

As this block will result in a test called 'disconnect', as will the
test below (that you add), you'll need to give one of these a unique
name, e.g. here you could do:

   # Make sure we're disconnected, in case we're testing with an
   # extended-remote board, therefore already connected.
   gdb_test "disconnect" ".*" \
     "disconnect before running the test"


>      gdb_test "target ${target} | ${::gdbserver} - ${::binfile}" ".*" \
>  	"start gdbserver using pipe syntax"
>  
> @@ -68,6 +65,12 @@ proc do_test { target } {
>  	gdb_test_no_output "python conn = gdb.selected_inferior().connection"
>  	gdb_test "python print(conn.details)" "\| ${::gdbserver} - ${::binfile}"
>      }
> +
> +    # Make sure GDB server doesn't attempt to reconnect with a closed STDIN.
> +    set prev_timeout $timeout
> +    set timeout 2
> +    gdb_test "disconnect" ".*"
> +    set timeout $prev_timeout

Having fixed timeouts like this is not ideal.  But if we want to test
this using GDB like this then there's probably no choice.

I do think the comment should be expanded to explain two things (a) this
timeout needs to be lower than PIPE_CLOSE_TIMEOUT (from ser-pipe.c) in
order for this test to be useful, and (b) that the timeout is a
_critical_ part of the test, rather than just an adjustment to make the
test fail quicker, or to give GDB longer to run.

Just for the record as there was some confusion; the test I proposed
aimed to remove GDB from the loop completely.  As the 5 second timeout
(PIPE_CLOSE_TIMEOUT) is on the GDB side, if we run gdbserver standalone
and then close its stdin, gdbserver will actually spin forever.  This
means we can not worry about setting a two second timeout and we can
give the test much longer to run.  Guinevere has made the case that I
neither explained myself well enough, and that this is probably too
complex ... so with the fixes above I think your original approach
should be fine.

Thanks,
Andrew


>  }
>  
>  save_vars { GDBFLAGS } {
> diff --git a/gdbserver/server.cc b/gdbserver/server.cc
> index ab9a33f40fd..bc923e510c0 100644
> --- a/gdbserver/server.cc
> +++ b/gdbserver/server.cc
> @@ -4195,6 +4195,10 @@ captured_main (int argc, char *argv[])
>  	  /* "-" specifies a stdio connection and is a form of port
>  	     specification.  */
>  	  port = STDIO_CONNECTION_NAME;
> +
> +	  /* Implying --once here prevents a hang after stdin has been closed.  */
> +	  run_once = true;
> +
>  	  next_arg++;
>  	  break;
>  	}
> -- 
> 2.45.2