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

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

[Simon Marchi]

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
   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
   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.

   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?
     - Do we ever need to do a non-blocking wait on a target that does
       not support async?

   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?

   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.

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)
 	{
 	  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 ())
+    gdb_assert ((options & TARGET_WNOHANG) == 0);
+
   return current_top_target ()->wait (ptid, status, options);
 }
 
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"
+
+if { ![runto_main] } {
+    fail "can't run to main"
+    return
+}
+
+gdb_continue_to_end
-- 
2.28.0

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

[Pedro Alves]

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
> 

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

[Simon Marchi]

On 2020-10-12 10:48 a.m., Pedro Alves wrote:
>> 2. the infrun async handler is marked in prepare_to_wait, to immediatly
>
> immediatly -> immediately

Fixed.

>> 2. the infrun async handler is marked in prepare_to_wait, to immediatly
>
> Ditto.

Fixed.

>> 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.

Makes sense.

>>      - 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.

Ok.  I would see it as a nice simplification for implementing target to
only have to implement one model, so that async targets don't have to
think about a way to block (which can be implemented as a nested event
loop like stop_all_threads does).  But indeed that's not necessary.

>>    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.

Ok.

>>
>>    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.

Hmm, right, with the assert in target_wait, it should never happen.  And
if we make it valid to do non-blocking polls of non-async targets, as
you described we could do for Windows above, then it could become a
valid case I suppose.

>> 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?

Yes, since infrun won't ask linux-nat to TARGET_WNOHANG when
target_async_permitted is false.  I'll remove it, since it can never
happen with the assert in target_wait.  If anything, we could put an
assert here as a double-check, if you think it could help.

>> 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.

I prefer the version where we don't hide the uses of current_whatever,
instead going slowly towards passing them as parameters instead.

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

Yes, done.

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

Done.

I'll send a v2 shortly.

I was wondering, do you see a better way to fix this than what I did in
do_target_wait_1, forcing TARGET_WNOHANG to off?  It doesn't feel right
to have the caller ask for something and just do something else.  But I
couldn't think of anything better (that's also suitable for the release
branch, at least).

Simon

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

[Pedro Alves]

On 10/13/20 4:06 PM, Simon Marchi wrote:
> On 2020-10-12 10:48 a.m., Pedro Alves wrote:

>> I'd rather go without this hunk.  Would it still fix things?
> 
> Yes, since infrun won't ask linux-nat to TARGET_WNOHANG when
> target_async_permitted is false.  I'll remove it, since it can never
> happen with the assert in target_wait.  If anything, we could put an
> assert here as a double-check, if you think it could help.

I don't think it could help.

> I'll send a v2 shortly.
> 
> I was wondering, do you see a better way to fix this than what I did in
> do_target_wait_1, forcing TARGET_WNOHANG to off?  It doesn't feel right
> to have the caller ask for something and just do something else.  But I
> couldn't think of anything better (that's also suitable for the release
> branch, at least).

I don't -- you can't do it at the caller because there you don't know
which inferior/target will be called.

We could just change the semantics of the do_target_wait slightly, like:

 static bool
 do_target_wait (ptid_t wait_ptid, execution_control_state *ecs,
-		target_wait_flags options)
+		bool dont_block_if_possible)
 {

That doesn't seem that much better to me, though.

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

[Simon Marchi]

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

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

[Simon Marchi]

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
> 

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

[Pedro Alves]

On 10/13/20 4:31 PM, Simon Marchi via Gdb-patches wrote:

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

Note the save_vars scope only needs to be around prepare_for_testing.

Anyway, LGTM.

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

[Simon Marchi]

On 2020-10-13 11:44 a.m., Pedro Alves wrote:
> On 10/13/20 4:31 PM, Simon Marchi via Gdb-patches wrote:
> 
>>> +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
>>> +}
> 
> Note the save_vars scope only needs to be around prepare_for_testing.

Ok, I changed it to avoid unnecessary indentation for the rest of the test.

> Anyway, LGTM.

Thanks, I'll push with that change.

Simon