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

[Pedro Alves <pedro@palves.net>]

On 10/1/20 3:56 AM, Simon Marchi via Gdb-patches wrote:
> 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 immediatly

immediatly -> immediately

>    wake up the event loop
> 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 immediatly

Ditto.

>    wake up the event loop
> 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.  But I don't see when it would be useful to ask a sync
>    target to do a non-blocking wait.

I could imagine it being useful to implement async mode for targets that
can poll for events, but don't have a native asynchronous "there's a new event!"
mechanism (like SIGCHLD or similar).  So GDB could poll for events
periodically, say, with a timer via the event loop.  Windows could gain
async support that way.

> 
>    Pass TARGET_WNOHANG to sync targets also poses the risk of passing
>    TARGET_WNOHANG to a target that doesn't implement it.  The caller of
>    target_wait would expect the call not to block, but the call may
>    indeed block.
> 
>    Since supporting TARGET_WNOHANG is a requirement for targets that can
>    do async (I believe), I propose (as implemented in this patch) that
>    we add an assertion in target_wait to make sure we don't ask a target
>    that can't do async to handle TARGET_WNOHANG.
> 
>    <off-topic>
>    A question in my mind is: could we bind the blocking or non-blocking
>    behavior of wait with can_async_p?  In other words:
> 
>      - Do we ever need to do a blocking wait on a target that supports
>        async?

Yes, for example in prepare_for_detach, we do a blocking wait
(via do_target_wait).  I think all such loops could likely be
converted to go via a nested event loop, which may be better for
handling user input events at the same time.  But regardless, I wouldn't
be conformable with trying can_async_p with what a specific target_wait
call wants.  Seems best to me to keep them orthogonal.

>      - Do we ever need to do a non-blocking wait on a target that does
>        not support async?

Not today, I don't think.

> 
>    So, could we make it so that if target's can_async_p method returns
>    true, its wait method is necessarily non-blocking?  And if
>    can_async_p returns false, its wait method is necessarily blocking?

I'd rather not.

> 
>    It sounds to me like it would simplify the semantic of
>    target_ops::wait a little bit.
>    </off-topic>
> 
> 2. The linux-nat target, even in the mode where it emulates a
>    synchronous target (with "maintenance set target-async off") respects
>    TARGET_WNOHANG.
> 
>    Non-async targets, such as windows_nat_target, simply don't check /
>    support TARGET_WNOHANG.  Their wait method is always blocking.  So,
>    to properly emulate a non-async target, I believe that linux-nat
>    should also ignore it when in "maintenance set target-async off"
>    mode.  Its behavior would be closer to a "true" non-async target.
> 
> The problem disappears if we simply fix either of these issues, but I
> think it wouldn't hurt to fix both.

Unless there's a need otherwise, it would seem better to me to
just fix the common code to not request a non-blocking wait out
of !async targets.  I don't think there's any good reason to complicate
(however little) linux-nat.c to handle a scenario that doesn't exist.

> 
> 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.
> 	* linux-nat.c (linux_nat_wait_1): Ignore TARGET_WNOHANG if
> 	target_async_permitted is false.
> 	* 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/linux-nat.c                               |  6 +++-
>  gdb/target.c                                  |  5 +++
>  .../gdb.base/maint-target-async-off.c         | 22 +++++++++++++
>  .../gdb.base/maint-target-async-off.exp       | 32 +++++++++++++++++++
>  5 files changed, 69 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 daf10417842f..1b88514c9cc7 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/linux-nat.c b/gdb/linux-nat.c
> index fbb538859429..1064fc4f8c72 100644
> --- a/gdb/linux-nat.c
> +++ b/gdb/linux-nat.c
> @@ -3238,7 +3238,11 @@ linux_nat_wait_1 (ptid_t ptid, struct target_waitstatus *ourstatus,
>  
>        /* No interesting event to report to the core.  */
>  
> -      if (target_options & TARGET_WNOHANG)
> +      /* If target_async_permitted is false (maintenance set target-async off
> +	 is used), pretend that we don't know about TARGET_WNOHANG and go block
> +	 in wait_for_signal.  */
> +      if (target_options & TARGET_WNOHANG
> +	  && target_async_permitted)

I'd rather go without this hunk.  Would it still fix things?

>  	{
>  	  linux_nat_debug_printf ("exit (ignore)");
>  
> diff --git a/gdb/target.c b/gdb/target.c
> index dd78a848caec..6f340678b7ca 100644
> --- a/gdb/target.c
> +++ b/gdb/target.c
> @@ -1997,6 +1997,11 @@ ptid_t
>  target_wait (ptid_t ptid, struct target_waitstatus *status,
>  	     target_wait_flags options)
>  {
> +  target_ops *target = current_top_target ();
> +
> +  if (!target->can_async_p ())

Why not call "!target_can_async_p ()" instead?  It's exactly
the same.

> +    gdb_assert ((options & TARGET_WNOHANG) == 0);
> +
>    return current_top_target ()->wait (ptid, status, options);

Perhaps you meant to adjust this to do "target->wait" instead?

>  }
>  
> 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..e77bc79a21e1
> --- /dev/null
> +++ b/gdb/testsuite/gdb.base/maint-target-async-off.exp
> @@ -0,0 +1,32 @@
> +# 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
> +
> +if { [prepare_for_testing "failed to prepare" ${testfile} ${srcfile}] } {
> +    return
> +}
> +
> +gdb_test_no_output "maintenance set target-async off"

I don't think this works correctly 
with --target_board=native-extended-gdbserver, since by the time you
get here, the remote connection is already set up.

I think the best way to handle that is to do the same we do what
non-stop testcases do:

        save_vars { GDBFLAGS } {
          append GDBFLAGS " -ex \"set non-stop $nonstop\""
          clean_restart ${executable}
        }

> +
> +if { ![runto_main] } {
> +    fail "can't run to main"
> +    return
> +}
> +
> +gdb_continue_to_end
>