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

[Andrew Burgess]

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

Thanks,
Andrew