From: Andrew Burgess <aburgess@redhat.com>
To: John Baldwin <jhb@freebsd.org>
Cc: gdb-patches@sourceware.org
Subject: Re: [PATCH v2 04/13] linux-nat: Don't enable async mode at the end of linux_nat_target::resume.
Date: Fri, 3 Dec 2021 10:43:34 +0000 [thread overview]
Message-ID: <20211203104334.GM2662946@redhat.com> (raw)
In-Reply-To: <93bccc79-4d39-671c-eeb5-3591dec3cdb4@FreeBSD.org>
* John Baldwin <jhb@FreeBSD.org> [2021-12-02 17:21:27 -0800]:
> On 12/2/21 3:19 AM, Andrew Burgess wrote:
> > * 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 :)
>
> I will include it, and I will merge the commit I had made to remove the
> explicit target_async(1) calls in the record targets into this commit
> so that the detailed commit log covers all of them.
>
> I noticed one additional call that could perhaps be elided which is the
> call to target_async(1) near the end of remote::resume in the remote
> target. Unlike the Linux native and record targets, this call is not
> at the very end of the function, but I don't think it will break anything
> to run the relevant code before enabling async mode rather than after:
>
> /* We are about to start executing the inferior, let's register it
> with the event loop. NOTE: this is the one place where all the
> execution commands end up. We could alternatively do this in each
> of the execution commands in infcmd.c. */
> /* FIXME: ezannoni 1999-09-28: We may need to move this out of here
> into infcmd.c in order to allow inferior function calls to work
> NOT asynchronously. */
> if (target_can_async_p ())
> target_async (1);
>
> /* We've just told the target to resume. The remote server will
> wait for the inferior to stop, and then send a stop reply. In
> the mean time, we can't start another command/query ourselves
> because the stub wouldn't be ready to process it. This applies
> only to the base all-stop protocol, however. In non-stop (which
> only supports vCont), the stub replies with an "OK", and is
> immediate able to process further serial input. */
> if (!target_is_non_stop_p ())
> rs->waiting_for_stop_reply = 1;
>
> This would also in theory remove the FIXME comment, though I don't know
> that this change is fully realizing the goal of that FIXME (it's also
> over 20 years old though). If you think it makes sense to elide the
> call in the remote target I can fold this into this commit as well.
> I can also just leave the remote target as-is.
I agree that this call can be safely removed.
Thanks,
Andrew
next prev parent reply other threads:[~2021-12-03 10:43 UTC|newest]
Thread overview: 52+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-08-03 18:49 [PATCH v2 00/13] FreeBSD target async mode and related refactoring John Baldwin
2021-08-03 18:49 ` [PATCH v2 01/13] gdbsupport: Add an event-pipe class John Baldwin
2021-10-11 21:39 ` Lancelot SIX
2021-11-24 13:04 ` Andrew Burgess
2021-08-03 18:49 ` [PATCH v2 02/13] gdb linux-nat: Convert linux_nat_event_pipe to the event_pipe class John Baldwin
2021-11-24 13:07 ` Andrew Burgess
2021-08-03 18:49 ` [PATCH v2 03/13] gdbserver linux-low: Convert linux_event_pipe " John Baldwin
2021-11-24 13:10 ` Andrew Burgess
2021-08-03 18:49 ` [PATCH v2 04/13] linux-nat: Don't enable async mode at the end of linux_nat_target::resume John Baldwin
2021-11-24 13:24 ` Andrew Burgess
2021-11-24 16:03 ` John Baldwin
2021-12-01 12:02 ` Andrew Burgess
2021-12-01 20:03 ` John Baldwin
2021-12-02 11:19 ` Andrew Burgess
2021-12-03 1:21 ` John Baldwin
2021-12-03 10:43 ` Andrew Burgess [this message]
2021-12-07 18:47 ` Tom Tromey
2021-08-03 18:49 ` [PATCH v2 05/13] do_target_wait_1: Clear TARGET_WNOHANG if the target isn't async John Baldwin
2021-11-24 13:26 ` Andrew Burgess
2021-08-03 18:49 ` [PATCH v2 06/13] inf-ptrace: Raise an internal_error if waitpid() fails John Baldwin
2021-11-24 14:16 ` Andrew Burgess
2021-11-24 20:16 ` Simon Marchi
2021-11-24 22:00 ` John Baldwin
2021-11-25 10:30 ` Andrew Burgess
2021-11-25 16:38 ` John Baldwin
2021-12-01 12:04 ` Andrew Burgess
2021-12-03 18:12 ` John Baldwin
2021-12-06 15:04 ` Andrew Burgess
2021-08-03 18:49 ` [PATCH v2 07/13] inf-ptrace: Support async targets in inf_ptrace_target::wait John Baldwin
2021-11-24 14:31 ` Andrew Burgess
2021-11-24 16:05 ` John Baldwin
2021-08-03 18:49 ` [PATCH v2 08/13] fbsd-nat: Implement async target support John Baldwin
2021-11-24 15:00 ` Andrew Burgess
2021-11-24 22:17 ` John Baldwin
2021-08-03 18:49 ` [PATCH v2 09/13] fbsd-nat: Include ptrace operation in error messages John Baldwin
2021-11-24 15:02 ` Andrew Burgess
2021-08-03 18:49 ` [PATCH v2 10/13] fbsd-nat: Switch fbsd_nat_target::resume entry debug message from lwp to nat John Baldwin
2021-11-24 15:05 ` Andrew Burgess
2021-11-24 22:19 ` John Baldwin
2021-08-03 18:49 ` [PATCH v2 11/13] fbsd-nat: Return NULL rather than failing thread_alive John Baldwin
2021-11-24 15:12 ` Andrew Burgess
2021-11-24 21:29 ` John Baldwin
2021-12-01 12:15 ` Andrew Burgess
2021-08-03 18:49 ` [PATCH v2 12/13] Enable async mode in the target in attach_cmd John Baldwin
2021-11-24 15:16 ` Andrew Burgess
2021-11-24 22:42 ` John Baldwin
2021-12-01 12:21 ` Andrew Burgess
2021-08-03 18:50 ` [PATCH v2 13/13] inf-ptrace: Add an event_pipe to be used for async mode in subclasses John Baldwin
2021-11-24 15:30 ` Andrew Burgess
2021-11-25 0:14 ` John Baldwin
2021-11-25 0:31 ` John Baldwin
2021-10-06 15:43 ` [PING] [PATCH v2 00/13] FreeBSD target async mode and related refactoring John Baldwin
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20211203104334.GM2662946@redhat.com \
--to=aburgess@redhat.com \
--cc=gdb-patches@sourceware.org \
--cc=jhb@freebsd.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).