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

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

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