[v3,gdbserver] Imply --once if connecting via stdio

  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. There is now a second disconnect at the end of the proc,
with a tighter-than-normal timeout to catch if the command hangs as
it used to.

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 | 13 ++++++++++++-
 gdbserver/server.cc                      |  4 ++++
 2 files changed, 16 insertions(+), 1 deletion(-)
  

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. There is now a second disconnect at the end of the proc,
> with a tighter-than-normal timeout to catch if the command hangs as
> it used to.

Thanks for the updates.

Approved-By: Andrew Burgess <aburgess@redhat.com>

thanks,
Andrew


>
> 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 | 13 ++++++++++++-
>  gdbserver/server.cc                      |  4 ++++
>  2 files changed, 16 insertions(+), 1 deletion(-)
>
> diff --git a/gdb/testsuite/gdb.server/server-pipe.exp b/gdb/testsuite/gdb.server/server-pipe.exp
> index b369759b98a..3d73c2e7ce6 100644
> --- a/gdb/testsuite/gdb.server/server-pipe.exp
> +++ b/gdb/testsuite/gdb.server/server-pipe.exp
> @@ -49,11 +49,12 @@ 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" ".*"
> +    gdb_test "disconnect" ".*" "disconnect before running the test"
>  
>      gdb_test "target ${target} | ${::gdbserver} - ${::binfile}" ".*" \
>  	"start gdbserver using pipe syntax"
> @@ -68,6 +69,16 @@ 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.
> +    # Here we set the timeout to a short value to see if GDB will hang in an
> +    # attempt to reconnect with the now closed STDIN. For this test to be
> +    # useful the new temporary timeout MUST be shorter than PIPE_CLOSE_TIMEOUT
> +    # defined in gdb/ser-pipe.c (5 seconds at the time of writing).
> +    set prev_timeout $timeout
> +    set timeout 2
> +    gdb_test "disconnect" ".*" "disconnect and test for hang"
> +    set timeout $prev_timeout
>  }
>  
>  save_vars { GDBFLAGS } {
> diff --git a/gdbserver/server.cc b/gdbserver/server.cc
> index 87b2a267721..e55486caa91 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
  

Hi,

On Fri, Jul 19, 2024 at 02:40:34PM +0100, Andrew Burgess wrote:
> 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. There is now a second disconnect at the end of the proc,
> > with a tighter-than-normal timeout to catch if the command hangs as
> > it used to.
> 
> Thanks for the updates.
> 
> Approved-By: Andrew Burgess <aburgess@redhat.com>

I see this was approved, but never applied.  Could it please be
applied.

It resolves an issue on the buildbot where a full run of check-gdb
"hangs" with a spinning gdbserver and so always times out after 20
minutes.

The buildbots currently use a nasty workaround where it will just
killall gdbserver after a test run. Which might accidentially kill the
wrong gdbserver.

Thanks,

Mark
  

Maybe I'm the one who's out of the loop on the procedures. If I'm the 
one supposed to push it to master I'm afraid I have no idea how to do 
that. Could Andrew either take the lead from here or send me a guide on 
what to do if that's the case?

On 8/12/2024 5:55 PM, Mark Wielaard wrote:
> Hi,
>
> On Fri, Jul 19, 2024 at 02:40:34PM +0100, Andrew Burgess wrote:
>> 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. There is now a second disconnect at the end of the proc,
>>> with a tighter-than-normal timeout to catch if the command hangs as
>>> it used to.
>> Thanks for the updates.
>>
>> Approved-By: Andrew Burgess <aburgess@redhat.com>
> I see this was approved, but never applied.  Could it please be
> applied.
>
> It resolves an issue on the buildbot where a full run of check-gdb
> "hangs" with a spinning gdbserver and so always times out after 20
> minutes.
>
> The buildbots currently use a nasty workaround where it will just
> killall gdbserver after a test run. Which might accidentially kill the
> wrong gdbserver.
>
> Thanks,
>
> Mark
  

Hi,

On Tue, Aug 13, 2024 at 04:26:17PM -0300, William Ferreira wrote:
> Maybe I'm the one who's out of the loop on the procedures. If I'm
> the one supposed to push it to master I'm afraid I have no idea how
> to do that. Could Andrew either take the lead from here or send me a
> guide on what to do if that's the case?

So, what are the next steps to get this patch in?
It would be really helpful for the autobuilders/testers.

Thanks,

Mark

> On 8/12/2024 5:55 PM, Mark Wielaard wrote:
> >Hi,
> >
> >On Fri, Jul 19, 2024 at 02:40:34PM +0100, Andrew Burgess wrote:
> >>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. There is now a second disconnect at the end of the proc,
> >>>with a tighter-than-normal timeout to catch if the command hangs as
> >>>it used to.
> >>Thanks for the updates.
> >>
> >>Approved-By: Andrew Burgess <aburgess@redhat.com>
> >I see this was approved, but never applied.  Could it please be
> >applied.
> >
> >It resolves an issue on the buildbot where a full run of check-gdb
> >"hangs" with a spinning gdbserver and so always times out after 20
> >minutes.
> >
> >The buildbots currently use a nasty workaround where it will just
> >killall gdbserver after a test run. Which might accidentially kill the
> >wrong gdbserver.
> >
> >Thanks,
> >
> >Mark
  

Mark Wielaard <mark@klomp.org> writes:

> Hi,
>
> On Tue, Aug 13, 2024 at 04:26:17PM -0300, William Ferreira wrote:
>> Maybe I'm the one who's out of the loop on the procedures. If I'm
>> the one supposed to push it to master I'm afraid I have no idea how
>> to do that. Could Andrew either take the lead from here or send me a
>> guide on what to do if that's the case?
>
> So, what are the next steps to get this patch in?
> It would be really helpful for the autobuilders/testers.

Sorry all.  I thought I'd already pushed this patch.

Anyway, I've gone ahead and pushed this now.

Sorry for the delay.

Thanks,
Andrew