This is the mail archive of the
gdb-patches@sourceware.org
mailing list for the GDB project.
Re: [PATCH 3/9 v7] Introduce target_{stop,continue}_ptid
- From: Gary Benson <gbenson at redhat dot com>
- To: Doug Evans <dje at google dot com>
- Cc: Pedro Alves <palves at redhat dot com>, gdb-patches <gdb-patches at sourceware dot org>
- Date: Tue, 16 Sep 2014 10:49:14 +0100
- Subject: Re: [PATCH 3/9 v7] Introduce target_{stop,continue}_ptid
- Authentication-results: sourceware.org; auth=none
- References: <5412DEB5 dot 6020706 at redhat dot com> <21523 dot 9502 dot 168492 dot 803068 at ruffy2 dot mtv dot corp dot google dot com> <54132B55 dot 9000108 at redhat dot com> <21523 dot 12189 dot 134570 dot 770432 at ruffy2 dot mtv dot corp dot google dot com> <5413305B dot 6020402 at redhat dot com> <21523 dot 13993 dot 986533 dot 615240 at ruffy2 dot mtv dot corp dot google dot com> <54133939 dot 70801 at redhat dot com> <CADPb22RsGv_Do1SztOK4Bse99e5yL_hmnrOHCU8OhNGkFyxGDQ at mail dot gmail dot com> <20140915100736 dot GA13503 at blade dot nx> <CADPb22Qs-Xi7DV+8OV32ao8KUw6OB-8F3wbKB3+Fypd7Rjd64A at mail dot gmail dot com>
Doug Evans wrote:
> On Mon, Sep 15, 2014 at 3:07 AM, Gary Benson <gbenson@redhat.com> wrote:
> > Doug Evans wrote:
> > > On Fri, Sep 12, 2014 at 11:19 AM, Pedro Alves <palves@redhat.com> wrote:
> > > > On 09/12/2014 07:08 PM, Doug Evans wrote:
> > > > > Pedro Alves wrote:
> > > > > > I just now noticed the elephant in the room -- target_stop
> > > > > > is asynchronous, doesn't wait for a stop, while and
> > > > > > target_stop_ptid is synchronous. [...]
> > > > >
> > > > > If the above code is right, I think a clarifying comment is
> > > > > required somewhere. It's odd that one can call
> > > > > agent_run_command when the inferior may or may not be stopped
> > > > > yet. [Or is there a bug here? - if I'm reading the gdbserver
> > > > > version correctly it first waits for the inferior to stop]
> > > >
> > > > It's a bug.
> > > >
> > > > (Note that the GDB side interfaces with an out-of-tree
> > > > agent, not GDBserver's agent. I don't know the status of
> > > > that agent.)
> > >
> > > Data point that target_stop should be named target_stop_async?
> >
> > Ok, can I get a summary of this thread, I'm struggling to follow it.
> >
> > a) What should the functions be called:
> > - target_stop_async / target_stop_wait
> > - target_continue_async / target_continue_no_signal
> > - something else?
> >
> > b) Is there a bug here I need to address?
>
> At this point I think we're still in discussion mode, there are no
> conclusions/agreements yet, except for the agreement to use
> target_continue_with_no_signal instead of target_continue_ptid.
>
> To advance the discussion,
> the async case is the subtle case IMO. Evidently (and I'm just going
> by what I see, there may be more data here) someone (*1) looked at the
> name "target_stop" and thought it was async (which is probably what
> I'd do). The function comment doesn't specify. One could argue it's
> sufficient to just fix the function comment, but if we're going to
> have a mix of similar functions, some of which are async and some
> sync, then IMO we should also make the difference stand out in the
> code where it's read. I'd be happy with a convention where all async
> functions ended with _async or _no_wait (the latter reads better to
> me), but I'm guessing I'd be happy with a different convention as
> well.
>
> FAOD,
> there is a bug, but it's not one you specifically need to address.
> I pointed it out because it's a data point that contributes to the discussion.
>
> (*1): I've looked at git log and blame. I'm speaking generically here
> because I think this is unlikely to be a one-off kind of issue. Plus I
> can well imagine me making a similar mistake too. Plus the bug got
> through code review.
Ok, so what I think you're saying is:
a) The target_stop method in GDB should be renamed target_stop_async
b) The common code target_stop_ptid should be renamed... target_stop ?
c) The bug is that in this code in linux-nat.c:
/* Pause all */
target_stop (ptid);
memcpy (s, "qTfSTM", sizeof ("qTfSTM"));
s[sizeof ("qTfSTM")] = 0;
agent_run_command (pid, s, strlen (s) + 1);
the "target_stop" should actually be a call to whatever
target_stop_ptid is renamed to.
One final thing--and I may now be going over something that has been
discussed before--could target_continue_ptid be better renamed as
target_resume_no_signal? It seems to sit better alongside GDB's
target_resume.
Thanks,
Gary
--
http://gbenson.net/
- References:
- Re: [PATCH 3/9 v7] Introduce target_{stop,continue}_ptid
- Re: [PATCH 3/9 v7] Introduce target_{stop,continue}_ptid
- Re: [PATCH 3/9 v7] Introduce target_{stop,continue}_ptid
- Re: [PATCH 3/9 v7] Introduce target_{stop,continue}_ptid
- Re: [PATCH 3/9 v7] Introduce target_{stop,continue}_ptid
- Re: [PATCH 3/9 v7] Introduce target_{stop,continue}_ptid
- Re: [PATCH 3/9 v7] Introduce target_{stop,continue}_ptid
- Re: [PATCH 3/9 v7] Introduce target_{stop,continue}_ptid
- Re: [PATCH 3/9 v7] Introduce target_{stop,continue}_ptid
- Re: [PATCH 3/9 v7] Introduce target_{stop,continue}_ptid