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

[Andrew Burgess <aburgess@redhat.com>]

* John Baldwin <jhb@FreeBSD.org> [2021-12-01 12:03:41 -0800]:

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

Thanks for the detailed analysis.  I agree that, to the best of my
understanding, your change is fine.  I think it would be worth adding
this analysis to the commit message, then if there ever is a bug in
the future, we can understand what we thought was going on :)

Thanks again!
Andrew