[PATCH 0/3] target_{stop,continue} things

[PATCH 0/3] target_{stop,continue} things

[Gary Benson]

Hi all,

This series fixes three issues raised in this thread:

  https://sourceware.org/ml/gdb-patches/2014-09/msg00338.html

Patch 1 renames target_{stop,continue}_ptid, patch 2 updates
target_stop's documentation to indicate that it is asynchronous,
and patch 3 fixes a bug in linux-nat.c where agent commands are
run on inferiors that may not have stopped.

Build and regtested on x86_64 RHEL 6.5.

Is this ok to commit?

Thanks,
Gary

--
http://gbenson.net/

[PATCH 1/3] Rename target_{stop,continue}_ptid

[Gary Benson]

This commit renames target_stop_ptid as target_stop_and_wait and
target_continue_ptid as target_continue_no_signal.  Comments are
updated to more fully describe the functions' behaviour.

gdb/ChangeLog:

	* target/target.h (target_stop_ptid): Renamed as...
	(target_stop_and_wait): New function.  Updated comment.
	All uses updated.
	(target_continue_ptid): Renamed as...
	(target_continue_no_signal): New function.  Updated comment.
	All uses updated.
---
 gdb/ChangeLog          |    9 +++++++++
 gdb/common/agent.c     |    4 ++--
 gdb/gdbserver/target.c |    4 ++--
 gdb/target.c           |    4 ++--
 gdb/target/target.h    |   16 +++++++++-------
 5 files changed, 24 insertions(+), 13 deletions(-)

diff --git a/gdb/common/agent.c b/gdb/common/agent.c
index 854a997..0c3d7a3 100644
--- a/gdb/common/agent.c
+++ b/gdb/common/agent.c
@@ -204,7 +204,7 @@ agent_run_command (int pid, const char *cmd, int len)
   DEBUG_AGENT ("agent: resumed helper thread\n");
 
   /* Resume helper thread.  */
-  target_continue_ptid (ptid);
+  target_continue_no_signal (ptid);
 
   fd = gdb_connect_sync_socket (pid);
   if (fd >= 0)
@@ -238,7 +238,7 @@ agent_run_command (int pid, const char *cmd, int len)
     {
       /* Stop thread PTID.  */
       DEBUG_AGENT ("agent: stop helper thread\n");
-      target_stop_ptid (ptid);
+      target_stop_and_wait (ptid);
     }
 
   if (fd >= 0)
diff --git a/gdb/gdbserver/target.c b/gdb/gdbserver/target.c
index c95b2f0..d63e211 100644
--- a/gdb/gdbserver/target.c
+++ b/gdb/gdbserver/target.c
@@ -137,7 +137,7 @@ mywait (ptid_t ptid, struct target_waitstatus *ourstatus, int options,
 /* See target/target.h.  */
 
 void
-target_stop_ptid (ptid_t ptid)
+target_stop_and_wait (ptid_t ptid)
 {
   struct target_waitstatus status;
   int was_non_stop = non_stop;
@@ -156,7 +156,7 @@ target_stop_ptid (ptid_t ptid)
 /* See target/target.h.  */
 
 void
-target_continue_ptid (ptid_t ptid)
+target_continue_no_signal (ptid_t ptid)
 {
   struct thread_resume resume_info;
 
diff --git a/gdb/target.c b/gdb/target.c
index 339b1d1..c1b5182 100644
--- a/gdb/target.c
+++ b/gdb/target.c
@@ -3030,7 +3030,7 @@ target_stop (ptid_t ptid)
 /* See target/target.h.  */
 
 void
-target_stop_ptid (ptid_t ptid)
+target_stop_and_wait (ptid_t ptid)
 {
   struct target_waitstatus status;
   int was_non_stop = non_stop;
@@ -3047,7 +3047,7 @@ target_stop_ptid (ptid_t ptid)
 /* See target/target.h.  */
 
 void
-target_continue_ptid (ptid_t ptid)
+target_continue_no_signal (ptid_t ptid)
 {
   target_resume (ptid, 0, GDB_SIGNAL_0);
 }
diff --git a/gdb/target/target.h b/gdb/target/target.h
index 99a7966..c8b9c80 100644
--- a/gdb/target/target.h
+++ b/gdb/target/target.h
@@ -59,15 +59,17 @@ extern int target_read_uint32 (CORE_ADDR memaddr, uint32_t *result);
 extern int target_write_memory (CORE_ADDR memaddr, const gdb_byte *myaddr,
 				ssize_t len);
 
-/* Make target stop in a continuable fashion.  (For instance, under
-   Unix, this should act like SIGSTOP).  This function must be
-   provided by the client.  */
+/* Cause the target to stop in a continuable fashion--for instance,
+   under Unix, this should act like SIGSTOP--and wait for the target
+   to be stopped before returning.  This function must be provided by
+   the client.  */
 
-extern void target_stop_ptid (ptid_t ptid);
+extern void target_stop_and_wait (ptid_t ptid);
 
-/* Restart a target that was previously stopped by target_stop_ptid.
-   This function must be provided by the client.  */
+/* Restart a target previously stopped by target_stop_and_wait.
+   No signal is delivered to the target.  This function must be
+   provided by the client.  */
 
-extern void target_continue_ptid (ptid_t ptid);
+extern void target_continue_no_signal (ptid_t ptid);
 
 #endif /* TARGET_COMMON_H */
-- 
1.7.1

Re: [PATCH 1/3] Rename target_{stop,continue}_ptid

[Pedro Alves]

On 09/18/2014 10:59 AM, Gary Benson wrote:
> This commit renames target_stop_ptid as target_stop_and_wait and
> target_continue_ptid as target_continue_no_signal.  Comments are
> updated to more fully describe the functions' behaviour.
> 
> gdb/ChangeLog:
> 
> 	* target/target.h (target_stop_ptid): Renamed as...
> 	(target_stop_and_wait): New function.  Updated comment.
> 	All uses updated.
> 	(target_continue_ptid): Renamed as...
> 	(target_continue_no_signal): New function.  Updated comment.
> 	All uses updated.

Fine with me.

> ---
>  gdb/ChangeLog          |    9 +++++++++
>  gdb/common/agent.c     |    4 ++--
>  gdb/gdbserver/target.c |    4 ++--
>  gdb/target.c           |    4 ++--
>  gdb/target/target.h    |   16 +++++++++-------
>  5 files changed, 24 insertions(+), 13 deletions(-)
> 
> diff --git a/gdb/common/agent.c b/gdb/common/agent.c
> index 854a997..0c3d7a3 100644
> --- a/gdb/common/agent.c
> +++ b/gdb/common/agent.c
> @@ -204,7 +204,7 @@ agent_run_command (int pid, const char *cmd, int len)
>    DEBUG_AGENT ("agent: resumed helper thread\n");
>  
>    /* Resume helper thread.  */
> -  target_continue_ptid (ptid);
> +  target_continue_no_signal (ptid);
>  
>    fd = gdb_connect_sync_socket (pid);
>    if (fd >= 0)
> @@ -238,7 +238,7 @@ agent_run_command (int pid, const char *cmd, int len)
>      {
>        /* Stop thread PTID.  */
>        DEBUG_AGENT ("agent: stop helper thread\n");
> -      target_stop_ptid (ptid);
> +      target_stop_and_wait (ptid);
>      }
>  
>    if (fd >= 0)
> diff --git a/gdb/gdbserver/target.c b/gdb/gdbserver/target.c
> index c95b2f0..d63e211 100644
> --- a/gdb/gdbserver/target.c
> +++ b/gdb/gdbserver/target.c
> @@ -137,7 +137,7 @@ mywait (ptid_t ptid, struct target_waitstatus *ourstatus, int options,
>  /* See target/target.h.  */
>  
>  void
> -target_stop_ptid (ptid_t ptid)
> +target_stop_and_wait (ptid_t ptid)
>  {
>    struct target_waitstatus status;
>    int was_non_stop = non_stop;
> @@ -156,7 +156,7 @@ target_stop_ptid (ptid_t ptid)
>  /* See target/target.h.  */
>  
>  void
> -target_continue_ptid (ptid_t ptid)
> +target_continue_no_signal (ptid_t ptid)
>  {
>    struct thread_resume resume_info;
>  
> diff --git a/gdb/target.c b/gdb/target.c
> index 339b1d1..c1b5182 100644
> --- a/gdb/target.c
> +++ b/gdb/target.c
> @@ -3030,7 +3030,7 @@ target_stop (ptid_t ptid)
>  /* See target/target.h.  */
>  
>  void
> -target_stop_ptid (ptid_t ptid)
> +target_stop_and_wait (ptid_t ptid)
>  {
>    struct target_waitstatus status;
>    int was_non_stop = non_stop;
> @@ -3047,7 +3047,7 @@ target_stop_ptid (ptid_t ptid)
>  /* See target/target.h.  */
>  
>  void
> -target_continue_ptid (ptid_t ptid)
> +target_continue_no_signal (ptid_t ptid)
>  {
>    target_resume (ptid, 0, GDB_SIGNAL_0);
>  }
> diff --git a/gdb/target/target.h b/gdb/target/target.h
> index 99a7966..c8b9c80 100644
> --- a/gdb/target/target.h
> +++ b/gdb/target/target.h
> @@ -59,15 +59,17 @@ extern int target_read_uint32 (CORE_ADDR memaddr, uint32_t *result);
>  extern int target_write_memory (CORE_ADDR memaddr, const gdb_byte *myaddr,
>  				ssize_t len);
>  
> -/* Make target stop in a continuable fashion.  (For instance, under
> -   Unix, this should act like SIGSTOP).  This function must be
> -   provided by the client.  */
> +/* Cause the target to stop in a continuable fashion--for instance,
> +   under Unix, this should act like SIGSTOP--and wait for the target
> +   to be stopped before returning.  This function must be provided by
> +   the client.  */
>  
> -extern void target_stop_ptid (ptid_t ptid);
> +extern void target_stop_and_wait (ptid_t ptid);
>  
> -/* Restart a target that was previously stopped by target_stop_ptid.
> -   This function must be provided by the client.  */
> +/* Restart a target previously stopped by target_stop_and_wait.
> +   No signal is delivered to the target.  This function must be
> +   provided by the client.  */
>  
> -extern void target_continue_ptid (ptid_t ptid);
> +extern void target_continue_no_signal (ptid_t ptid);
>  
>  #endif /* TARGET_COMMON_H */
> 


Thanks,
Pedro Alves

[PATCH 3/3] Wait for target to stop before running agent commands

[Gary Benson]

This commit fixes a bug on where agent_run_command is called on
GNU/Linux targets that may not have stopped yet.

gdb/ChangeLog:

	* linux-nat.c (linux_child_static_tracepoint_markers_by_strid):
	Use target_stop_and_wait in place of target_stop.
---
 gdb/ChangeLog   |    5 +++++
 gdb/linux-nat.c |    2 +-
 2 files changed, 6 insertions(+), 1 deletions(-)

diff --git a/gdb/linux-nat.c b/gdb/linux-nat.c
index 5a82d23..f2895ff 100644
--- a/gdb/linux-nat.c
+++ b/gdb/linux-nat.c
@@ -4458,7 +4458,7 @@ linux_child_static_tracepoint_markers_by_strid (struct target_ops *self,
   ptid_t ptid = ptid_build (pid, 0, 0);
 
   /* Pause all */
-  target_stop (ptid);
+  target_stop_and_wait (ptid);
 
   memcpy (s, "qTfSTM", sizeof ("qTfSTM"));
   s[sizeof ("qTfSTM")] = 0;
-- 
1.7.1

Re: [PATCH 3/3] Wait for target to stop before running agent commands

[Pedro Alves]

On 09/18/2014 10:59 AM, Gary Benson wrote:
> This commit fixes a bug on where agent_run_command is called on
> GNU/Linux targets that may not have stopped yet.
> 
> gdb/ChangeLog:
> 
> 	* linux-nat.c (linux_child_static_tracepoint_markers_by_strid):
> 	Use target_stop_and_wait in place of target_stop.
> ---
>  gdb/ChangeLog   |    5 +++++
>  gdb/linux-nat.c |    2 +-
>  2 files changed, 6 insertions(+), 1 deletions(-)
> 
> diff --git a/gdb/linux-nat.c b/gdb/linux-nat.c
> index 5a82d23..f2895ff 100644
> --- a/gdb/linux-nat.c
> +++ b/gdb/linux-nat.c
> @@ -4458,7 +4458,7 @@ linux_child_static_tracepoint_markers_by_strid (struct target_ops *self,
>    ptid_t ptid = ptid_build (pid, 0, 0);
>  
>    /* Pause all */
> -  target_stop (ptid);
> +  target_stop_and_wait (ptid);

More would be needed for fix this fully.

Unlike the comment above suggests, target_stop_and_wait only
waits for one thread to stop, not all.

See the gdbserver version (run_inferior_command) -- this is also
missing uninserting breakpoints after fully pausing the target.

If we handle this internally to linux-nat.c, then similarly
to gdbserver's version, this could use
stop_callback/stop_wait_callback to leave pending any events
that pausing might trip on.  Then we'd need to be careful to
only re-resume threads that were already resumed before, and
leave those with pending events stopped.  If we want to
use existing target methods, then we need to teach gdb's
struct thread_info layer about leaving events pending, like in:

  https://sourceware.org/ml/gdb-patches/2011-12/msg00564.html

Alternative, we can would promote target_stop_and_wait to
a real target method that handles the leaving of events pending,
like gdbserver's pause_all/unpause_all methods.

Thanks,
Pedro Alves

[PATCH 2/3] Update target_stop's documentation

[Gary Benson]

This commit updates target_stop's documentation to clarify that
it is asynchronous.

gdb/ChangeLog:

	* target.c (target_stop): Updated comment.
---
 gdb/ChangeLog |    4 ++++
 gdb/target.h  |    6 ++++--
 2 files changed, 8 insertions(+), 2 deletions(-)

diff --git a/gdb/target.h b/gdb/target.h
index 2ea7a2d..24fb757 100644
--- a/gdb/target.h
+++ b/gdb/target.h
@@ -1573,8 +1573,10 @@ extern int target_thread_alive (ptid_t ptid);
 extern void target_find_new_threads (void);
 
 /* Make target stop in a continuable fashion.  (For instance, under
-   Unix, this should act like SIGSTOP).  This function is normally
-   used by GUIs to implement a stop button.  */
+   Unix, this should act like SIGSTOP).  Note that this function is
+   asynchronous: it does not wait for the target to become stopped
+   before returning.  If this is the behaviour you want please use
+   target_stop_and_wait.  */
 
 extern void target_stop (ptid_t ptid);
 
-- 
1.7.1

Re: [PATCH 2/3] Update target_stop's documentation

[Pedro Alves]

On 09/18/2014 10:59 AM, Gary Benson wrote:
> This commit updates target_stop's documentation to clarify that
> it is asynchronous.

> +   before returning.  If this is the behaviour you want please use

US spelling: "behavior".

LGTM.

Thanks,
Pedro Alves

Re: [PATCH 2/3] Update target_stop's documentation

[Gary Benson]

Pedro Alves wrote:
> On 09/18/2014 10:59 AM, Gary Benson wrote:
> > This commit updates target_stop's documentation to clarify that
> > it is asynchronous.
> 
> > +   before returning.  If this is the behaviour you want please use
> 
> US spelling: "behavior".
> 
> LGTM.

Ok, I have fixed this and pushed the first two patches of this series.

Thanks,
Gary

-- 
http://gbenson.net/