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

[Doug Evans <dje@google.com>]

On Wed, Sep 17, 2014 at 4:30 AM, Pedro Alves <palves@redhat.com> wrote:
> On 09/16/2014 10:18 PM, Doug Evans wrote:
>> On Tue, Sep 16, 2014 at 3:36 AM, Pedro Alves <palves@redhat.com> wrote:
>>> 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).
>>
>> Are you asking about target_xfer_partial as it exists today,
>> or a hypothetical version where it is used to request the transfer
>> but the transfer happens asynchronously? [Or both? 1/2 :-)]
>> I think it's more the latter, but I'll answer for both.
>>
>> When I think of a "read memory" operation, absent any info specifying
>> sync/async, I'd expect that the request completes before control is
>> returned to the caller.  If it didn't wait for the request to complete that
>> would be unexpected.  Therefore I'd leave the name of
>> target_xfer_partial as it exists today alone.
>> That same reasoning to me says a hypothetical "memory read"
>> request that was asynchronous should have something in its
>> name that says to the reader "Hey, this is async!  Heads up!".
>>
>>> What about target_resume?  I'm not sure renaming that would be useful,
>>> for example.
>>
>> I thought about target_resume.  It was an semi-interesting case
>> that immediately popped into my head at the time.
>> And then I tried to think how the typical reader would interpret it.
>> I'm not a typical reader, but I think(!) people would expect it to be
>> asynchronous in the sense that the inferior is resumed and
>> control returns to gdb.  IOW target_resume doesn't also wait
>> for the inferior to stop after it has been resumed.
>> Therefore I see no need to rename it (say to target_resume_no_wait).
>>
>>> 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.
>>
>> I think these things can be approached on a by-demand basis.
>> All I'm suggesting(!) [and it is just a suggestion] is having a convention
>> for when the topic comes up.
>
> That's the thing.  You've now shown that you've given some thought over
> a couple cases,

It's not clear to me what your point is here.
I thought of target_resume earlier, and I hardly spent any time on it.
I wouldn't have expected anyone else to either.

I thought of target_xfer_partial when you mentioned it.

> but when you first raised this, you didn't, which puts us
> in a position of having to stop and go through the same exercises and
> looking for patterns, checking what might or not make sense, etc.

Let's assume for the sake of discussion I *had* also thought of
target_xfer_partial earlier.  Mentioning those two cases would
have made any difference to the time being put into this discussion?
Wow, noted for next time.

> There's no way to tell whether the suggestion makes sense without that
> info; IOW, the suggestion was too vague and ends up being
> counterproductive (in the sense that we spend a bunch of time on it
> instead of moving forward with other more important things).

re: "more important things":
Eh?
The topic at hand is readable code and avoiding bugs,
and yet if I go through gdb-patches@ for even the last week
what will I find?  The standard bits of hacking, bug fixing,
and administrivia (e.g., vxworks cleanup).

Characterizing this as spending time on less important things bothers me.
gdb has several problems I wish people had spent a bit more time on.
[OTOH, I'm not suggesting spending *too* much time on this.
That should go without saying though.]

> Please
> don't take me wrong.  I'm in "how to communicate better mode".  I would
> have liked to have been able to reply quicker to these emails, but I
> just don't know how -- fyi, today's and yesterday's emails on this
> subject took me a few hours.

If the situation were reversed and I didn't have time at that moment,
I'd have just said it's an interesting idea but let's table this discussion
until we can collect more info.

> I guess my thing is that I'm not particularly fond of appending
> _async/_sync/_no_wait to target methods names throughout.  Maybe
> we'll find even better names as we move along.
>
>>> 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.
>>
>> Whether or not a function is a target method is orthogonal to
>> what its name suggests to the reader about what it does
>> and how it behaves.
>
> I was just trying to say that we could call target_stop_ptid
> pause_thread or something else without "target_" in the name even,
> though I'm not suggesting that.

Ah, thanks for clearing that up ...
Still, it's a bit tangential to the discussion at hand.
No worries though.

>>> 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.
>>
>> This is a step in the right direction, and if that is all people are
>> comfortable with right now that's fine by me.
>
> Thanks.  Let's go forward with that, so the common/ work can
> continue, and continue discussing the naming in parallel.

FAOD, I was not suggesting blocking the common/ work for this.
I'd expect some parallelization.

>> [I've gotten target_stop's asyncness burned into memory,
>> but I can't help but worry about the next reader.]
>
> We could even pick IMO more natural terms than _async or _no_wait,
> e.g., target_request_stop.

As long as it solves the problem in a reasonable way
and we're consistent I think that'd be ok.