Re: [PATCH 3/9 v7] Introduce target_{stop,continue}_ptid

[Pedro Alves <palves@redhat.com>]

On 09/15/2014 05:00 PM, 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.

At first blush, this looks to me like looking for a generalization / hard
rule where one might not be necessary, or otherwise, we'd need more a
better/clearer rule/constrain on which methods this would apply to.  E.g.,
would you rename target_xfer_partial?  Certainly memory accesses can
be expected to work in an asynchronous way, with read results being sent
over with asynchronous notifications (even though we don't do it like that).
What about target_resume?  I'm not sure renaming that would be useful,
for example.  So I'd like to see a comprehensive list of functions
that'd you're proposing would be affected in order to be able to
comment further or even be able to see the same patterns you're seeing.

target_stop_and_wait (the existing target_stop_ptid) isn't really a
target method, unlike target_stop, target_resume, etc. -- it's a
helper method that sits on top of target methods.  So I'd go with
clarifying target_stop's "asyncness" in its comments, name the
helper target_stop_and_wait.  I think that's already better
than the status quo.  And then we can discuss conventions and
(potentially wholesale) renaming separately.  It's not a big deal
if we have to rename the new function again.

Thanks,
Pedro Alves