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

  This commit introduces two new functions to stop and restart target
processes that shared code can use and that clients must implement.
It also changes some shared code to use these functions.

The changes in this patch replace the target_stop, target_wait
and target_resume parts of this patch I posted on August 1:
https://sourceware.org/ml/gdb-patches/2014-08/msg00014.html
The remainder of that patch is in patch 2 of this series.

gdb/ChangeLog:

	* target/target.h (target_stop_ptid, target_continue_ptid):
	Declare.
	* target.c (target_stop_ptid, target_continue_ptid): New
	functions.
	* common/agent.c [!GDBSERVER]: Don't include infrun.h.
	(agent_run_command): Always use target_stop_ptid and
	target_continue_ptid.

gdb/gdbserver/ChangeLog:

	* target.c (target_stop_ptid, target_continue_ptid): New
	functions.
---
 gdb/ChangeLog           |   10 ++++++++++
 gdb/common/agent.c      |   37 ++-----------------------------------
 gdb/gdbserver/ChangeLog |    5 +++++
 gdb/gdbserver/target.c  |   32 ++++++++++++++++++++++++++++++++
 gdb/target.c            |   25 +++++++++++++++++++++++++
 gdb/target/target.h     |   12 ++++++++++++
 6 files changed, 86 insertions(+), 35 deletions(-)
  

On 08/29/2014 02:51 PM, Gary Benson wrote:
> This commit introduces two new functions to stop and restart target
> processes that shared code can use and that clients must implement.
> It also changes some shared code to use these functions.
> 
> The changes in this patch replace the target_stop, target_wait
> and target_resume parts of this patch I posted on August 1:
> https://sourceware.org/ml/gdb-patches/2014-08/msg00014.html
> The remainder of that patch is in patch 2 of this series.
> 
> gdb/ChangeLog:
> 
> 	* target/target.h (target_stop_ptid, target_continue_ptid):
> 	Declare.
> 	* target.c (target_stop_ptid, target_continue_ptid): New
> 	functions.
> 	* common/agent.c [!GDBSERVER]: Don't include infrun.h.
> 	(agent_run_command): Always use target_stop_ptid and
> 	target_continue_ptid.
> 
> gdb/gdbserver/ChangeLog:
> 
> 	* target.c (target_stop_ptid, target_continue_ptid): New
> 	functions.

OK, with ...

> +/* Make target stop in a continuable fashion.  (For instance, under
> +   Unix, this should act like SIGSTOP).  This function must be
> +   provided by the client.  This function is normally used by GUIs
> +   to implement a stop button.  */

... Please drop the last sentence referring to GUIs.

> +
> +extern void target_stop_ptid (ptid_t ptid);

Thanks,
Pedro Alves
  

Gary Benson writes:
 > This commit introduces two new functions to stop and restart target
 > processes that shared code can use and that clients must implement.
 > It also changes some shared code to use these functions.
 > 
 > The changes in this patch replace the target_stop, target_wait
 > and target_resume parts of this patch I posted on August 1:
 > https://sourceware.org/ml/gdb-patches/2014-08/msg00014.html
 > The remainder of that patch is in patch 2 of this series.
 > 
 > gdb/ChangeLog:
 > 
 > 	* target/target.h (target_stop_ptid, target_continue_ptid):
 > 	Declare.
 > 	* target.c (target_stop_ptid, target_continue_ptid): New
 > 	functions.
 > 	* common/agent.c [!GDBSERVER]: Don't include infrun.h.
 > 	(agent_run_command): Always use target_stop_ptid and
 > 	target_continue_ptid.
 > 
 > gdb/gdbserver/ChangeLog:
 > 
 > 	* target.c (target_stop_ptid, target_continue_ptid): New
 > 	functions.
 > [...]
 > diff --git a/gdb/target.c b/gdb/target.c
 > index 711e7cb..339b1d1 100644
 > --- a/gdb/target.c
 > +++ b/gdb/target.c
 > @@ -3027,6 +3027,31 @@ target_stop (ptid_t ptid)
 >    (*current_target.to_stop) (&current_target, ptid);
 >  }
 >  
 > [...]
 > +/* See target/target.h.  */
 > +
 > +void
 > +target_continue_ptid (ptid_t ptid)
 > +{
 > +  target_resume (ptid, 0, GDB_SIGNAL_0);
 > +}
 > +

Hi.
How come GDB_SIGNAL_0 is used here?
Maybe it's correct, but it's not immediately clear.

The reason I ask is because there are two ways to "continue"
the inferior:
1) resume it where it left off, and if it stopped because
   of a signal then forward on that signal (assuming the
   signal is not "nopass") (GDB_SIGNAL_DEFAULT).
2) Either inject a new signal (GDB_SIGNAL_FOO) or cancel out a previously
   queued signal (GDB_SIGNAL_0).

GDB_SIGNAL_0 is used to resume the target and discarding
any signal that it may have stopped for.
GDB_SIGNAL_DEFAULT is used for (1).

I realize the comments for target_resume say to not pass
GDB_SIGNAL_DEFAULT to it.  But the name "target_continue_ptid"
with no option to choose between (1) and (2)
says to me "do what GDB_SIGNAL_DEFAULT" does.

 > +/* Restart a target that was previously stopped by target_stop_ptid.
 > +   This function must be provided by the client.  */
 > +
 > +extern void target_continue_ptid (ptid_t ptid);
 > +
  

Doug Evans wrote:
> Gary Benson writes:
> > This commit introduces two new functions to stop and restart
> > target processes that shared code can use and that clients must
> > implement.  It also changes some shared code to use these
> > functions.
> [...]
> > +/* See target/target.h.  */
> > +
> > +void
> > +target_continue_ptid (ptid_t ptid)
> > +{
> > +  target_resume (ptid, 0, GDB_SIGNAL_0);
> > +}
> 
> How come GDB_SIGNAL_0 is used here?
> Maybe it's correct, but it's not immediately clear.
> 
> The reason I ask is because there are two ways to "continue"
> the inferior:
> 1) resume it where it left off, and if it stopped because
>    of a signal then forward on that signal (assuming the
>    signal is not "nopass") (GDB_SIGNAL_DEFAULT).
> 2) Either inject a new signal (GDB_SIGNAL_FOO) or cancel out
>    a previously queued signal (GDB_SIGNAL_0).
> 
> GDB_SIGNAL_0 is used to resume the target and discarding
> any signal that it may have stopped for.
> GDB_SIGNAL_DEFAULT is used for (1).
> 
> I realize the comments for target_resume say to not pass
> GDB_SIGNAL_DEFAULT to it.  But the name "target_continue_ptid"
> with no option to choose between (1) and (2)
> says to me "do what GDB_SIGNAL_DEFAULT" does.

I don't know the answer to this, I just moved the code from one
place to the next :)  Possibly it's because (I think) under the
hood target_stop_ptid sends a SIGSTOP to the inferior, so you
don't want to restart it with that signal queued.

The comment for target_continue_ptid says:

> > +/* Restart a target that was previously stopped by target_stop_ptid.
> > +   This function must be provided by the client.  */

That implies to me "don't use this for targets not previously
stopped by target_stop_ptid".

Maybe someone more familiar with this code could elaborate?

Thanks,
Gary
  

On 09/11/2014 11:26 AM, Gary Benson wrote:
> Doug Evans wrote:
>> Gary Benson writes:
>>> This commit introduces two new functions to stop and restart
>>> target processes that shared code can use and that clients must
>>> implement.  It also changes some shared code to use these
>>> functions.
>> [...]
>>> +/* See target/target.h.  */
>>> +
>>> +void
>>> +target_continue_ptid (ptid_t ptid)
>>> +{
>>> +  target_resume (ptid, 0, GDB_SIGNAL_0);
>>> +}
>>
>> How come GDB_SIGNAL_0 is used here?
>> Maybe it's correct, but it's not immediately clear.
>>
>> The reason I ask is because there are two ways to "continue"
>> the inferior:
>> 1) resume it where it left off, and if it stopped because
>>    of a signal then forward on that signal (assuming the
>>    signal is not "nopass") (GDB_SIGNAL_DEFAULT).
>> 2) Either inject a new signal (GDB_SIGNAL_FOO) or cancel out
>>    a previously queued signal (GDB_SIGNAL_0).
>>
>> GDB_SIGNAL_0 is used to resume the target and discarding
>> any signal that it may have stopped for.
>> GDB_SIGNAL_DEFAULT is used for (1).
>>
>> I realize the comments for target_resume say to not pass
>> GDB_SIGNAL_DEFAULT to it.  But the name "target_continue_ptid"
>> with no option to choose between (1) and (2)
>> says to me "do what GDB_SIGNAL_DEFAULT" does.
> 
> I don't know the answer to this, I just moved the code from one
> place to the next :)  Possibly it's because (I think) under the
> hood target_stop_ptid sends a SIGSTOP to the inferior, so you
> don't want to restart it with that signal queued.

No, that's not it.  The SIGSTOP is an internal detail of the
target implementation.  It's reported to the target_wait caller
as GDB_SIGNAL_0 (meaning, the thread stopped for no reason
other than that GDB wanted it to stop).

> The comment for target_continue_ptid says:
> 
>>> +/* Restart a target that was previously stopped by target_stop_ptid.
>>> +   This function must be provided by the client.  */
> 
> That implies to me "don't use this for targets not previously
> stopped by target_stop_ptid".
> 
> Maybe someone more familiar with this code could elaborate?

I guess that'd be me...

In the case that target_stop_ptid returns anything else other
than GDB_SIGNAL_0, resuming with GDB_SIGNAL_0, GDB_SIGNAL_DEFAULT or
the signal that the thread had stopped for would all be wrong.
If we got a synchronous SIGSEGV, for instance, we'd want to abort
the agent call and return error.  We'd likely disable access to the
agent from there on, as it's messed up.  If instead we got an
asynchronous signal, we'd want to hold on to it, and requeue it
later, once the agent command is done with.

[As an example showing this same complication being handled
similarly, see See enqueue_one_deferred_signal and
collecting_fast_tracepoint in gdbserver/linux-low.c.  This
handles the case of getting signals when they arrive while
a thread is in a fast tracepoint jump pad.    That's a lower
level though.]

But, we have to look at where/how this is presently being used
to understand why the current code got away from handling
that complication.

The thread that we're interacting with blocks all signals.
See gdbserver/tracepoint.c:

static void
gdb_agent_init (void)
{
  int res;
  pthread_t thread;
  sigset_t new_mask;
  sigset_t orig_mask;

  /* We want the helper thread to be as transparent as possible, so
     have it inherit an all-signals-blocked mask.  */

  sigfillset (&new_mask);
  res = pthread_sigmask (SIG_SETMASK, &new_mask, &orig_mask);
  if (res)

and, we run commands with all breakpoints lifted:

/* Ask the in-process agent to run a command.  Since we don't want to
   have to handle the IPA hitting breakpoints while running the
   command, we pause all threads, remove all breakpoints, and then set
   the helper thread re-running.  We communicate with the helper
   thread by means of direct memory xfering, and a socket for
   synchronization.  */

static int
run_inferior_command (char *cmd, int len)
{
  int err = -1;
  int pid = ptid_get_pid (current_ptid);

  trace_debug ("run_inferior_command: running: %s", cmd);

  pause_all (0);
  uninsert_all_breakpoints ();

  err = agent_run_command (pid, (const char *) cmd, len);



If we want to reuse target_stop_ptid for other cases in the
future, we'll likely make it return the stop waitstatus then.

For now, I think just documenting target_continue_ptid as
resuming with no signal is good enough.

Thanks,
Pedro Alves
  

On 08/29/2014 02:51 PM, Gary Benson wrote:


> +/* See target/target.h.  */
> +
> +void
> +target_stop_ptid (ptid_t ptid)
> +{
> +  struct target_waitstatus status;
> +  int was_non_stop = non_stop;
> +
> +  non_stop = 1;
> +  target_stop (ptid);
> +
> +  memset (&status, 0, sizeof (status));
> +  target_wait (ptid, &status, 0);
> +
> +  non_stop = was_non_stop;
> +}

One thing that was bugging me was that given that the names
of target_stop and target_stop_ptid are so similar and that
they have the same signature is ripe for confusion.

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.  Would you mind renaming this to target_stop_wait
or some such?  And then add an explicit "and wait for it to stop"
or some such to the function's description.

Thanks,
Pedro Alves
  

Pedro Alves writes:
 > On 09/11/2014 11:26 AM, Gary Benson wrote:
 > > Doug Evans wrote:
 > >> Gary Benson writes:
 > >>> This commit introduces two new functions to stop and restart
 > >>> target processes that shared code can use and that clients must
 > >>> implement.  It also changes some shared code to use these
 > >>> functions.
 > >> [...]
 > >>> +/* See target/target.h.  */
 > >>> +
 > >>> +void
 > >>> +target_continue_ptid (ptid_t ptid)
 > >>> +{
 > >>> +  target_resume (ptid, 0, GDB_SIGNAL_0);
 > >>> +}
 > >>
 > >> How come GDB_SIGNAL_0 is used here?
 > >> Maybe it's correct, but it's not immediately clear.
 > >>
 > >> The reason I ask is because there are two ways to "continue"
 > >> the inferior:
 > >> 1) resume it where it left off, and if it stopped because
 > >>    of a signal then forward on that signal (assuming the
 > >>    signal is not "nopass") (GDB_SIGNAL_DEFAULT).
 > >> 2) Either inject a new signal (GDB_SIGNAL_FOO) or cancel out
 > >>    a previously queued signal (GDB_SIGNAL_0).
 > >>
 > >> GDB_SIGNAL_0 is used to resume the target and discarding
 > >> any signal that it may have stopped for.
 > >> GDB_SIGNAL_DEFAULT is used for (1).
 > >>
 > >> I realize the comments for target_resume say to not pass
 > >> GDB_SIGNAL_DEFAULT to it.  But the name "target_continue_ptid"
 > >> with no option to choose between (1) and (2)
 > >> says to me "do what GDB_SIGNAL_DEFAULT" does.
 >
 > [...]
 > 
 > For now, I think just documenting target_continue_ptid as
 > resuming with no signal is good enough.

That may be sufficient for me to make this patch checkin-able,
but before then I'd like to understand how and where
this function will be used from gdb.

btw, how about target_continue_with_no_signal (ptid_t ptid) ?
  

Pedro Alves writes:
 > On 08/29/2014 02:51 PM, Gary Benson wrote:
 > 
 > > +/* See target/target.h.  */
 > > +
 > > +void
 > > +target_stop_ptid (ptid_t ptid)
 > > +{
 > > +  struct target_waitstatus status;
 > > +  int was_non_stop = non_stop;
 > > +
 > > +  non_stop = 1;
 > > +  target_stop (ptid);
 > > +
 > > +  memset (&status, 0, sizeof (status));
 > > +  target_wait (ptid, &status, 0);
 > > +
 > > +  non_stop = was_non_stop;
 > > +}
 > 
 > One thing that was bugging me was that given that the names
 > of target_stop and target_stop_ptid are so similar and that
 > they have the same signature is ripe for confusion.
 > 
 > 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.  Would you mind renaming this to target_stop_wait
 > or some such?  And then add an explicit "and wait for it to stop"
 > or some such to the function's description.

target_stop_and_wait would be a significantly more readable to me.
["stop_wait" makes me pause too long to figure out what was meant]

Or if a plain reading of "target_stop" implies waiting,
then rename target_stop to target_stop_no_wait or target_stop_async.
and use "target_stop" for "target stop and wait".

Do we want a convention for marking async vs sync routines?
e.g. append every async routine that is otherwise ambiguous with _async?
Or if gdb is becoming generally async, turn it around and
append _sync to every sync routine that is otherwise ambiguous.
[Or even allow both in certain instances.]
That way we'd no longer have to have individual discussions
whenever the topic comes up.
  

On 09/12/2014 05:53 PM, Doug Evans wrote:

> That may be sufficient for me to make this patch checkin-able,
> but before then I'd like to understand how and where
> this function will be used from gdb.

Can you clarify?  I don't have any plan to use this
elsewhere myself.  All we're doing is factoring out
the current use behind a common function so that both gdb
and gdbserver can implement it their own way.

> btw, how about target_continue_with_no_signal (ptid_t ptid) ?

Fine with me.

Thanks,
Pedro Alves
  

Pedro Alves writes:
 > On 09/12/2014 05:53 PM, Doug Evans wrote:
 > 
 > > That may be sufficient for me to make this patch checkin-able,
 > > but before then I'd like to understand how and where
 > > this function will be used from gdb.
 > 
 > Can you clarify?  I don't have any plan to use this
 > elsewhere myself.  All we're doing is factoring out
 > the current use behind a common function so that both gdb
 > and gdbserver can implement it their own way.

I don't see the function being used in gdb by this patch set.
So, absent further info, the change to gdb/target.c is just adding
dead code.  I'm assuming that's not the case (*1), but until I
understand how it's going to be used in gdb it's not clear to
me whether code that uses it will be confusing to read.

(*1): I could have missed its use.
But even if it's dead code today, I don't mind it
in this particular case.

 > > btw, how about target_continue_with_no_signal (ptid_t ptid) ?
 > 
 > Fine with me.

In which case I'm happy.
Code that uses that name can be read unambiguously
(to me anyway).
  

On 09/12/2014 06:38 PM, Doug Evans wrote:
> Pedro Alves writes:
>  > On 09/12/2014 05:53 PM, Doug Evans wrote:
>  > 
>  > > That may be sufficient for me to make this patch checkin-able,
>  > > but before then I'd like to understand how and where
>  > > this function will be used from gdb.
>  > 
>  > Can you clarify?  I don't have any plan to use this
>  > elsewhere myself.  All we're doing is factoring out
>  > the current use behind a common function so that both gdb
>  > and gdbserver can implement it their own way.
> 
> I don't see the function being used in gdb by this patch set.
> So, absent further info, the change to gdb/target.c is just adding
> dead code.  I'm assuming that's not the case (*1), but until I
> understand how it's going to be used in gdb it's not clear to
> me whether code that uses it will be confusing to read.

It's used in common/agent.c, which is used by gdb as well.

 linux-nat.c:  agent_run_command (pid, s, strlen (s) + 1);
 linux-nat.c:      agent_run_command (pid, s, strlen (s) + 1);

It's because it's used in gdb that the current code, before
Gary's patch has the #ifdef GDBSERVER #else /* gdb code */ bits.
He's just moving that code around.

Thanks,
Pedro Alves
  

Pedro Alves writes:
 > On 09/12/2014 06:38 PM, Doug Evans wrote:
 > > Pedro Alves writes:
 > >  > On 09/12/2014 05:53 PM, Doug Evans wrote:
 > >  > 
 > >  > > That may be sufficient for me to make this patch checkin-able,
 > >  > > but before then I'd like to understand how and where
 > >  > > this function will be used from gdb.
 > >  > 
 > >  > Can you clarify?  I don't have any plan to use this
 > >  > elsewhere myself.  All we're doing is factoring out
 > >  > the current use behind a common function so that both gdb
 > >  > and gdbserver can implement it their own way.
 > > 
 > > I don't see the function being used in gdb by this patch set.
 > > So, absent further info, the change to gdb/target.c is just adding
 > > dead code.  I'm assuming that's not the case (*1), but until I
 > > understand how it's going to be used in gdb it's not clear to
 > > me whether code that uses it will be confusing to read.
 > 
 > It's used in common/agent.c, which is used by gdb as well.
 > 
 >  linux-nat.c:  agent_run_command (pid, s, strlen (s) + 1);
 >  linux-nat.c:      agent_run_command (pid, s, strlen (s) + 1);
 > 
 > It's because it's used in gdb that the current code, before
 > Gary's patch has the #ifdef GDBSERVER #else /* gdb code */ bits.
 > He's just moving that code around.

Ah.
I'm liking target_continue_with_no_signal even more.
[assuming one wants to leave the signature as is]

btw, I noticed this 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);

And I remembered you saying:

 > 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]

The function comment for target_stop says nothing one way or the other
about whether it's asynchronous which doesn't help.
  

On 09/12/2014 07:08 PM, Doug Evans wrote:

> And I remembered you saying:
> 
>  > 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.)

Thanks,
Pedro Alves
  

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:
>
>> And I remembered you saying:
>>
>>  > 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.)

Heh.
Data point that target_stop should be named target_stop_async?
1/2 :-)

[I'd like to point out that all I had to do was read the code (glance
is more apt) to see the bug (given that your comment about target_stop
was still fresh in my mind).  Unambiguous/clear function names are
worth at least some effort.]

btw, out-of-tree agent, and not gdbserver's agent?
That's odd.  Where would I find this agent?
  

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?

Thanks,
Gary
  

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.
  

On Mon, Sep 15, 2014 at 9:00 AM, Doug Evans <dje@google.com> wrote:
> [...]
> 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

Bleah.  s/async/synchronous/

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

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
  

On 09/12/2014 07:29 PM, Doug Evans wrote:

> btw, out-of-tree agent, and not gdbserver's agent?
> That's odd.  Where would I find this agent?

I don't have a link handy; I'd have to do a web search.  Yao was
working on it.  Try looking for the history around the patch
that made common/agent.c work in gdb.

Thanks,
Pedro Alves
  

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
  

On 09/16/2014 10:49 AM, Gary Benson wrote:

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

No, the "resume" interface can "continue" and "step" (and on gdbserver,
stop/interrupt too), but target_continue_ptid _only_ continues.

Thanks,
Pedro Alves
  

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.

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

> 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.
[I've gotten target_stop's asyncness burned into memory,
but I can't help but worry about the next reader.]
  

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, 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.
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).  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.

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.

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

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

Thanks,
Pedro Alves
  

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.
  

On 09/17/2014 07:20 PM, Doug Evans wrote:

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

OK.  I was reading it like "a convention where all async functions
ended with _async or _no_wait" would be applied throughout.  I could
see instead that restricted to cases where we have two variants -- I
guess that's where my understanding was.

> re: "more important things":
> Eh?

I'm sorry about that.  I clearly overreacted...

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

OK.  I'll be sure to ask for clarification earlier too, as I seem to
have misunderstood what you were actually suggesting.

Thanks,
Pedro Alves
  

On Fri, Sep 19, 2014 at 9:50 AM, Pedro Alves <palves@redhat.com> wrote:
> On 09/17/2014 07:20 PM, Doug Evans wrote:
>
>>>> 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).
>
> OK.  I was reading it like "a convention where all async functions
> ended with _async or _no_wait" would be applied throughout.  I could
> see instead that restricted to cases where we have two variants -- I
> guess that's where my understanding was.

I did write "... that is otherwise ambiguous".
ref: https://sourceware.org/ml/gdb-patches/2014-09/msg00440.html