Re: [PATCH v2] gdb: don't pass TARGET_WNOHANG to targets that can't async (PR 26642)

[Simon Marchi via Gdb-patches <gdb-patches@sourceware.org>]

Sorry, I forgot the "v2" in the title.

On 2020-10-13 11:27 a.m., Simon Marchi wrote:
> New in v2:
> 
> - omit change in linux-nat.c
> - set target-async off using command-line flag in the test
> - use target local variable in target_wait
> 
> --8<---
> 
> Debugging with "maintenance set target-async off" on Linux has been
> broken since 5b6d1e4fa4f ("Multi-target support").
> 
> The issue is easy to reproduce:
> 
>     $ ./gdb -q --data-directory=data-directory -nx ./test
>     Reading symbols from ./test...
>     (gdb) maintenance set target-async off
>     (gdb) start
>     Temporary breakpoint 1 at 0x1151: file test.c, line 5.
>     Starting program: /home/simark/build/binutils-gdb/gdb/test
> 
> ... and it hangs there.
> 
> The difference between pre-5b6d1e4fa4f and 5b6d1e4fa4f is that
> fetch_inferior_event now calls target_wait with TARGET_WNOHANG for
> non-async-capable targets, whereas it didn't before.
> 
> For non-async-capable targets, this is how it's expected to work when
> resuming execution:
> 
> 1. we call resume
> 2. the infrun async handler is marked in prepare_to_wait, to immediately
>    wake up the event loop when we get back to it
> 3. fetch_inferior_event calls the target's wait method without
>    TARGET_WNOHANG, effectively blocking until the target has something
>    to report
> 
> However, since we call the target's wait method with TARGET_WNOHANG,
> this happens:
> 
> 1. we call resume
> 2. the infrun async handler is marked in prepare_to_wait, to immediately
>    wake up the event loop when we get back to it
> 3. fetch_inferior_event calls the target's wait method with
>    TARGET_WNOHANG, the target has nothing to report yet
> 4. we go back to blocking on the event loop
> 5. SIGCHLD finally arrives, but the event loop is not woken up, because
>    we are not in async mode.  Normally, we should have been stuck in
>    waitpid the SIGCHLD would have unblocked us.
> 
> We end up in this situation because these two necessary conditions are
> met:
> 
> 1. GDB uses the TARGET_WNOHANG option with a target that can't do async.
>    I don't think this makes sense.  I mean, it's technically possible,
>    the doc for TARGET_WNOHANG is:
> 
>   /* Return immediately if there's no event already queued.  If this
>      options is not requested, target_wait blocks waiting for an
>      event.  */
>   TARGET_WNOHANG = 1,
> 
>    ... which isn't in itself necessarily incompatible with synchronous
>    targets.  It could be possible for a target to support non-blocking
>    polls, while not having a way to asynchronously wake up the event
>    loop, which is also necessary to support async.  But as of today,
>    we don't expect GDB and sync targets to work this way.
> 
> 2. The linux-nat target, even in the mode where it emulates a
>    synchronous target (with "maintenance set target-async off") respects
>    TARGET_WNOHANG.  Other non-async targets, such as windows_nat_target,
>    simply don't check / support TARGET_WNOHANG, so their wait method is
>    always blocking.
> 
> Fix the first issue by avoiding using TARGET_WNOHANG on non-async
> targets, in do_target_wait_1.  Add an assert in target_wait to verify it
> doesn't happen.
> 
> The new test gdb.base/maint-target-async-off.exp is a simple test that
> just tries running to main and then to the end of the program, with
> "maintenance set target-async off".
> 
> gdb/ChangeLog:
> 
> 	PR gdb/26642
> 	* infrun.c (do_target_wait_1): Clear TARGET_WNOHANG if the
> 	target can't do async.
> 	* target.c (target_wait): Assert that we don't pass
> 	TARGET_WNOHANG to a target that can't async.
> 
> gdb/testsuite/ChangeLog:
> 
> 	PR gdb/26642
> 	* gdb.base/maint-target-async-off.c: New test.
> 	* gdb.base/maint-target-async-off.exp: New test.
> 
> Change-Id: I69ad3a14598863d21338a8c4e78700a58ce7ad86
> ---
>  gdb/infrun.c                                  |  5 +++
>  gdb/target.c                                  |  7 +++-
>  .../gdb.base/maint-target-async-off.c         | 22 ++++++++++
>  .../gdb.base/maint-target-async-off.exp       | 41 +++++++++++++++++++
>  4 files changed, 74 insertions(+), 1 deletion(-)
>  create mode 100644 gdb/testsuite/gdb.base/maint-target-async-off.c
>  create mode 100644 gdb/testsuite/gdb.base/maint-target-async-off.exp
> 
> diff --git a/gdb/infrun.c b/gdb/infrun.c
> index d552fb3adb26..8ae39a2877b3 100644
> --- a/gdb/infrun.c
> +++ b/gdb/infrun.c
> @@ -3533,6 +3533,11 @@ do_target_wait_1 (inferior *inf, ptid_t ptid,
>  
>    /* But if we don't find one, we'll have to wait.  */
>  
> +  /* We can't ask a non-async target to do a non-blocking wait, so this will be
> +     a blocking wait.  */
> +  if (!target_can_async_p ())
> +    options &= ~TARGET_WNOHANG;
> +
>    if (deprecated_target_wait_hook)
>      event_ptid = deprecated_target_wait_hook (ptid, status, options);
>    else
> diff --git a/gdb/target.c b/gdb/target.c
> index 531858a3333f..a111ea3c3336 100644
> --- a/gdb/target.c
> +++ b/gdb/target.c
> @@ -1992,7 +1992,12 @@ ptid_t
>  target_wait (ptid_t ptid, struct target_waitstatus *status,
>  	     target_wait_flags options)
>  {
> -  return current_top_target ()->wait (ptid, status, options);
> +  target_ops *target = current_top_target ();
> +
> +  if (!target->can_async_p ())
> +    gdb_assert ((options & TARGET_WNOHANG) == 0);
> +
> +  return target->wait (ptid, status, options);
>  }
>  
>  /* See target.h.  */
> diff --git a/gdb/testsuite/gdb.base/maint-target-async-off.c b/gdb/testsuite/gdb.base/maint-target-async-off.c
> new file mode 100644
> index 000000000000..9d7b2f1a4c28
> --- /dev/null
> +++ b/gdb/testsuite/gdb.base/maint-target-async-off.c
> @@ -0,0 +1,22 @@
> +/* This testcase is part of GDB, the GNU debugger.
> +
> +   Copyright 2020 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/>.  */
> +
> +int
> +main (void)
> +{
> +  return 0;
> +}
> diff --git a/gdb/testsuite/gdb.base/maint-target-async-off.exp b/gdb/testsuite/gdb.base/maint-target-async-off.exp
> new file mode 100644
> index 000000000000..dc205f533c53
> --- /dev/null
> +++ b/gdb/testsuite/gdb.base/maint-target-async-off.exp
> @@ -0,0 +1,41 @@
> +# Copyright 2020 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/>.
> +
> +# Verify that debugging with "maintenance target-async off" works somewhat.  At
> +# least running to main and to the end of the program.
> +
> +standard_testfile
> +
> +save_vars { GDBFLAGS } {
> +    # Enable target-async off this way, because with board
> +    # native-extended-gdbserver, the remote target is already open when
> +    # returning from prepare_for_testing, and that would be too late to toggle
> +    # it.
> +    append GDBFLAGS { -ex "maintenance set target-async off" }
> +
> +    if { [prepare_for_testing "failed to prepare" ${testfile} ${srcfile}] } {
> +	return
> +    }
> +
> +    # Make sure our command-line flag worked.
> +    gdb_test "maintenance show target-async" "Controlling the inferior in asynchronous mode is off\\."
> +
> +    if { ![runto_main] } {
> +	fail "can't run to main"
> +	return
> +    }
> +
> +    gdb_continue_to_end
> +}
> -- 
> 2.28.0
>