Re: [PATCH v2 04/13] linux-nat: Don't enable async mode at the end of linux_nat_target::resume.

[John Baldwin <jhb@FreeBSD.org>]

On 12/1/21 4:02 AM, Andrew Burgess wrote:
> * John Baldwin <jhb@FreeBSD.org> [2021-11-24 08:03:36 -0800]:
> 
>> On 11/24/21 5:24 AM, Andrew Burgess wrote:
>>> * John Baldwin <jhb@FreeBSD.org> [2021-08-03 11:49:51 -0700]:
>>>
>>>> Commit 5b6d1e4fa4f ("Multi-target support") enabled async mode in the caller
>>>> (do_target_resume) after target::resume returns making this call
>>>> redundant.
>>>
>>> The only thing I spotted where this might result in a change of
>>> behaviour is in record-full.c in record_full_wait_1, there's this
>>> line:
>>>
>>>     ops->beneath ()->resume (ptid, step, GDB_SIGNAL_0);
>>>
>>> Doesn't this mean that if the record target is sitting on top of a
>>> Linux target, then me could potentially see a change of behaviour
>>> here?
>>>
>>> I know almost nothing about the record target, so I can't really offer
>>> any insights into whether the situation about could actually happen or
>>> not.
>>>
>>> Maybe somebody else will have some thoughts, but at a minimum, it is
>>> probably worth mentioning that there might be a change for that
>>> case.... Unless you know for sure that there isn't.
>>
>> Hmm, I was not aware of that case.  This is a change Pedro had suggested
>> and it didn't show any regressions in the test suite on a Linux/x86-64
>> VM, but it very well might be that the test suite doesn't cover
>> record?
> 
> I suspect it's not well tested.
> 
>>
>> Hmm, looking at record_full_target::resume, it enables async right after
>> calling the beneath method and it also does so before returning, so I
>> suspect it's explicit call is safe to elide for the same reason as this
>> patch.  That is, in the code today, linux_nat_target::resume enables async
>> mode, then would return to remote_full_target::resume which would enable
>> async mode (without any other statements in between), and then return
>> to do_target_resume which would enable async mode a third time (again without
>> any other code in between aside from any RAII destructors when returning
>> from the target methods).  Given, that I think this should be safe, and
>> I can add a patch to this series to remove the call from
>> remote_full_target::resume.
> 
> That's all true, but I wasn't commenting on
> record_full_target::resume, rather record_full_wait_1, which is called
> from record_full_base_target::wait.
> 
> I'd certainly be happy for your change to go in.  Having looked again,
> I don't think there's going to be any problems, looking at other
> targets, it doesn't seem like having to (re-)enable async mode is
> something we'd normally expect to do on the ::wait path...
> 
> I only pointed this out as it seems to be a case that doesn't align
> with the claims in your commit message...

Oh, humm, I see (and indeed, I had misread your earlier message).  It is
true that the commit log isn't accurate (and at the least that would
need to be amended).  One thing I don't really understand are the cases
when async is disabled such that resume needs to re-enable it vs are
the cases in resume just trying to catch an edge case with attach or
create_inferior or the like where async mode isn't enabled by some other
hook.

Ah, it seems that in wait_one if a target returns WAITKIND_NO_RESUMED,
we disable async on that target, and then the next time we want to
resume the target we re-enable async after target_resume returns.  The
other places we disable async are either temporary places around
readline, etc. or when when an inferior exits (INF_EXEC_COMPLETE)
passed to inferior_event_handle).

Given that, I do think the code in record_full_wait_1 is ok:

1) Any time ::wait() is invoked, it looks like target_async(1) should
    have already been called by the preceding do_target_resume().

2) The loop in record_full_wait_1() doesn't return to the wait_one()
    loop where target_async(0) is invoked, so async for the underlying
    target should stay enabled for the duration of the ::wait() call.

3) All the places that can invoke INF_EXEC_COMPLETE look to be in the
    gdb core itself which is "above" a target ::wait() method on the
    stack.

-- 
John Baldwin