[PATCH 1/2] Use target_continue{,_no_signal} instead of target_resume

[PATCH 1/2] Use target_continue{,_no_signal} instead of target_resume

[Sergio Durigan Junior]

This commit implements a new function, target_continue, on top of the
target_resume function.  Then, it replaces all calls to target_resume
by calls to target_continue or to the already existing
target_continue_no_signal.

This is one of the (many) necessary steps needed to consolidate the
target interface between GDB and gdbserver.  In particular, I am
interested in the impact this change will have on the unification of
the fork_inferior function (which I have been working on).

Tested on the BuildBot, no regressions introduced.

gdb/gdbserver/ChangeLog:
2016-08-29  Sergio Durigan Junior  <sergiodj@redhat.com>

	* server.c (start_inferior): New variable 'ptid'.  Replace calls
	to the_target->resume by target_continue{,_no_signal}, depending
	on the case.
	* target.c (target_stop_and_wait): Call target_continue_no_signal
	instead of the_target->resume.
	(target_continue): New function.

gdb/ChangeLog:
2016-08-29  Sergio Durigan Junior  <sergiodj@redhat.com>

	* fork-child.c (startup_inferior): Replace calls to target_resume
	by target_continue{,_no_signal}, depending on the case.
	* linux-nat.c (cleanup_target_stop): Call
	target_continue_no_signal instead of target_resume.
	* procfs.c (procfs_wait): Likewise.
	* target.c (target_continue): New function.
	* target/target.h (target_continue): New prototype.
---
 gdb/fork-child.c       |  4 ++--
 gdb/gdbserver/server.c | 25 +++++++------------------
 gdb/gdbserver/target.c | 19 ++++++++++++++-----
 gdb/linux-nat.c        |  2 +-
 gdb/procfs.c           |  4 ++--
 gdb/target.c           |  8 ++++++++
 gdb/target/target.h    |  6 ++++++
 7 files changed, 40 insertions(+), 28 deletions(-)

diff --git a/gdb/fork-child.c b/gdb/fork-child.c
index 6856cf6..3e29a1f 100644
--- a/gdb/fork-child.c
+++ b/gdb/fork-child.c
@@ -514,7 +514,7 @@ startup_inferior (int ntraps)
       if (resume_signal != GDB_SIGNAL_TRAP)
 	{
 	  /* Let shell child handle its own signals in its own way.  */
-	  target_resume (resume_ptid, 0, resume_signal);
+	  target_continue (resume_ptid, resume_signal);
 	}
       else
 	{
@@ -540,7 +540,7 @@ startup_inferior (int ntraps)
 	    break;
 
 	  /* Just make it go on.  */
-	  target_resume (resume_ptid, 0, GDB_SIGNAL_0);
+	  target_continue_no_signal (resume_ptid);
 	}
     }
 
diff --git a/gdb/gdbserver/server.c b/gdb/gdbserver/server.c
index 6fbd61d..9c06443 100644
--- a/gdb/gdbserver/server.c
+++ b/gdb/gdbserver/server.c
@@ -258,12 +258,7 @@ start_inferior (char **argv)
 
   if (wrapper_argv != NULL)
     {
-      struct thread_resume resume_info;
-
-      memset (&resume_info, 0, sizeof (resume_info));
-      resume_info.thread = pid_to_ptid (signal_pid);
-      resume_info.kind = resume_continue;
-      resume_info.sig = 0;
+      ptid_t ptid = pid_to_ptid (signal_pid);
 
       last_ptid = mywait (pid_to_ptid (signal_pid), &last_status, 0, 0);
 
@@ -271,7 +266,7 @@ start_inferior (char **argv)
 	{
 	  do
 	    {
-	      (*the_target->resume) (&resume_info, 1);
+	      target_continue_no_signal (ptid);
 
 	      last_ptid = mywait (pid_to_ptid (signal_pid), &last_status, 0, 0);
 	      if (last_status.kind != TARGET_WAITKIND_STOPPED)
@@ -3929,7 +3924,6 @@ process_serial_event (void)
 
       if ((tracing && disconnected_tracing) || any_persistent_commands ())
 	{
-	  struct thread_resume resume_info;
 	  struct process_info *process = find_process_pid (pid);
 
 	  if (process == NULL)
@@ -3965,10 +3959,7 @@ process_serial_event (void)
 	  process->gdb_detached = 1;
 
 	  /* Detaching implicitly resumes all threads.  */
-	  resume_info.thread = minus_one_ptid;
-	  resume_info.kind = resume_continue;
-	  resume_info.sig = 0;
-	  (*the_target->resume) (&resume_info, 1);
+	  target_continue_no_signal (minus_one_ptid);
 
 	  write_ok (own_buf);
 	  break; /* from switch/case */
@@ -4428,7 +4419,7 @@ handle_target_event (int err, gdb_client_data client_data)
 	      /* A thread stopped with a signal, but gdb isn't
 		 connected to handle it.  Pass it down to the
 		 inferior, as if it wasn't being traced.  */
-	      struct thread_resume resume_info;
+	      enum gdb_signal signal;
 
 	      if (debug_threads)
 		debug_printf ("GDB not connected; forwarding event %d for"
@@ -4436,13 +4427,11 @@ handle_target_event (int err, gdb_client_data client_data)
 			      (int) last_status.kind,
 			      target_pid_to_str (last_ptid));
 
-	      resume_info.thread = last_ptid;
-	      resume_info.kind = resume_continue;
 	      if (last_status.kind == TARGET_WAITKIND_STOPPED)
-		resume_info.sig = gdb_signal_to_host (last_status.value.sig);
+		signal = last_status.value.sig;
 	      else
-		resume_info.sig = 0;
-	      (*the_target->resume) (&resume_info, 1);
+		signal = GDB_SIGNAL_0;
+	      target_continue (last_ptid, signal);
 	    }
 	}
       else
diff --git a/gdb/gdbserver/target.c b/gdb/gdbserver/target.c
index 42d22b4..8435dc2 100644
--- a/gdb/gdbserver/target.c
+++ b/gdb/gdbserver/target.c
@@ -248,12 +248,8 @@ target_stop_and_wait (ptid_t ptid)
 {
   struct target_waitstatus status;
   int was_non_stop = non_stop;
-  struct thread_resume resume_info;
 
-  resume_info.thread = ptid;
-  resume_info.kind = resume_stop;
-  resume_info.sig = GDB_SIGNAL_0;
-  (*the_target->resume) (&resume_info, 1);
+  target_continue_no_signal (ptid);
 
   non_stop = 1;
   mywait (ptid, &status, 0, 0);
@@ -273,6 +269,19 @@ target_continue_no_signal (ptid_t ptid)
   (*the_target->resume) (&resume_info, 1);
 }
 
+/* See target/target.h.  */
+
+void
+target_continue (ptid_t ptid, enum gdb_signal signal)
+{
+  struct thread_resume resume_info;
+
+  resume_info.thread = ptid;
+  resume_info.kind = resume_continue;
+  resume_info.sig = gdb_signal_to_host (signal);
+  (*the_target->resume) (&resume_info, 1);
+}
+
 int
 start_non_stop (int nonstop)
 {
diff --git a/gdb/linux-nat.c b/gdb/linux-nat.c
index 5d5efa0..7410f8e 100644
--- a/gdb/linux-nat.c
+++ b/gdb/linux-nat.c
@@ -4289,7 +4289,7 @@ cleanup_target_stop (void *arg)
   gdb_assert (arg != NULL);
 
   /* Unpause all */
-  target_resume (*ptid, 0, GDB_SIGNAL_0);
+  target_continue_no_signal (*ptid);
 }
 
 static VEC(static_tracepoint_marker_p) *
diff --git a/gdb/procfs.c b/gdb/procfs.c
index 67b424f..0e0641e 100644
--- a/gdb/procfs.c
+++ b/gdb/procfs.c
@@ -3716,7 +3716,7 @@ wait_again:
 		    else
 		      {
 			/* How to keep going without returning to wfi: */
-			target_resume (ptid, 0, GDB_SIGNAL_0);
+			target_continue_no_signal (ptid);
 			goto wait_again;
 		      }
 		  }
@@ -3742,7 +3742,7 @@ wait_again:
 		    /* This is an internal event and should be transparent
 		       to wfi, so resume the execution and wait again.	See
 		       comment in procfs_init_inferior() for more details.  */
-		    target_resume (ptid, 0, GDB_SIGNAL_0);
+		    target_continue_no_signal (ptid);
 		    goto wait_again;
 		  }
 #endif
diff --git a/gdb/target.c b/gdb/target.c
index 13e3cdb..87c9681 100644
--- a/gdb/target.c
+++ b/gdb/target.c
@@ -3449,6 +3449,14 @@ target_continue_no_signal (ptid_t ptid)
   target_resume (ptid, 0, GDB_SIGNAL_0);
 }
 
+/* See target/target.h.  */
+
+void
+target_continue (ptid_t ptid, enum gdb_signal signal)
+{
+  target_resume (ptid, 0, signal);
+}
+
 /* Concatenate ELEM to LIST, a comma separate list, and return the
    result.  The LIST incoming argument is released.  */
 
diff --git a/gdb/target/target.h b/gdb/target/target.h
index 76af732..1bfa1b0 100644
--- a/gdb/target/target.h
+++ b/gdb/target/target.h
@@ -70,4 +70,10 @@ extern void target_stop_and_wait (ptid_t ptid);
 
 extern void target_continue_no_signal (ptid_t ptid);
 
+/* Restart a target previously stopped by target_stop_and_wait.
+   SIGNAL is delivered to the target.  This function must be provided
+   by the client.  */
+
+extern void target_continue (ptid_t ptid, enum gdb_signal signal);
+
 #endif /* TARGET_COMMON_H */
-- 
2.4.3

[PATCH 0/2] Consolidate target_{wait,resume} calls between GDB and gdbserver

[Sergio Durigan Junior]

Hi,

This simple patch series proposes a consolidation of the calls to
target_{wait,resume} functions between GDB and gdbserver.  This is a
necessary and helpful step in the direction that we're heading, and is
particularly important for my work on the unification of fork_inferior.

The first patch implements a new function, target_continue, on top of
target_resume (or the_target->resume, depending on where you're
looking), and replaces all calls to target_resume (or
the_target->resume) by either target_continue or
target_continue_no_signal (this last one has been a GDB citizen for
some time).  Note that I haven't touched infrun.c's
"do_target_resume", which is a wrapper around target_resume and does
many other things.

The second patch, which is even simpler, is meant to pay a technical
debt on gdbserver: the lack of the "target_wait" function.  So there
it is now, properly implemented, even though everyone else will still
use mywait because it accounts for many aspects that the regular
target_wait doesn't.  This is actually another place for future
improvement: make the two versions of target_wait identical API-wise,
so that sharing code is easier.

Both patches do not introduce regressions (I've tested them using the
Try server feature on BuildBot, BTW).

OK to apply?

Thanks,

[PATCH 2/2] Share target_wait prototype between GDB and gdbserver

[Sergio Durigan Junior]

This commit moves the target_wait prototype from the GDB-specific
target.h header to the common target/target.h header.  Then, it
creates a compatible implementation of target_wait on gdbserver using
the_target->wait, and adjusts the (only) caller (mywait function).

Pretty straightforward, no regressions introduced.

gdb/gdbserver/ChangeLog:
2016-08-29  Sergio Durigan Junior  <sergiodj@redhat.com>

	* target.c (mywait): Call target_wait instead of
	the_target->wait.
	(target_wait): New function.

gdb/ChangeLog:
2016-08-29  Sergio Durigan Junior  <sergiodj@redhat.com>

	* target.c (target_wait): Mention that the function's prototype
	can be found at target/target.h.
	* target.h (target_wait): Move prototype from here...
	* target/target.h (target_wait): ... to here.
---
 gdb/gdbserver/target.c | 10 +++++++++-
 gdb/target.c           |  2 ++
 gdb/target.h           | 12 ------------
 gdb/target/target.h    | 12 ++++++++++++
 4 files changed, 23 insertions(+), 13 deletions(-)

diff --git a/gdb/gdbserver/target.c b/gdb/gdbserver/target.c
index 8435dc2..053629c 100644
--- a/gdb/gdbserver/target.c
+++ b/gdb/gdbserver/target.c
@@ -211,7 +211,7 @@ mywait (ptid_t ptid, struct target_waitstatus *ourstatus, int options,
   if (connected_wait)
     server_waiting = 1;
 
-  ret = (*the_target->wait) (ptid, ourstatus, options);
+  ret = target_wait (ptid, ourstatus, options);
 
   /* We don't expose _LOADED events to gdbserver core.  See the
      `dlls_changed' global.  */
@@ -258,6 +258,14 @@ target_stop_and_wait (ptid_t ptid)
 
 /* See target/target.h.  */
 
+ptid_t
+target_wait (ptid_t ptid, struct target_waitstatus *status, int options)
+{
+  return (*the_target->wait) (ptid, status, options);
+}
+
+/* See target/target.h.  */
+
 void
 target_continue_no_signal (ptid_t ptid)
 {
diff --git a/gdb/target.c b/gdb/target.c
index 87c9681..bca25bd 100644
--- a/gdb/target.c
+++ b/gdb/target.c
@@ -2283,6 +2283,8 @@ target_disconnect (const char *args, int from_tty)
   current_target.to_disconnect (&current_target, args, from_tty);
 }
 
+/* See target/target.h.  */
+
 ptid_t
 target_wait (ptid_t ptid, struct target_waitstatus *status, int options)
 {
diff --git a/gdb/target.h b/gdb/target.h
index 9506e04..2af405a 100644
--- a/gdb/target.h
+++ b/gdb/target.h
@@ -1341,18 +1341,6 @@ extern void target_disconnect (const char *, int);
 
 extern void target_resume (ptid_t ptid, int step, enum gdb_signal signal);
 
-/* Wait for process pid to do something.  PTID = -1 to wait for any
-   pid to do something.  Return pid of child, or -1 in case of error;
-   store status through argument pointer STATUS.  Note that it is
-   _NOT_ OK to throw_exception() out of target_wait() without popping
-   the debugging target from the stack; GDB isn't prepared to get back
-   to the prompt with a debugging target but without the frame cache,
-   stop_pc, etc., set up.  OPTIONS is a bitwise OR of TARGET_W*
-   options.  */
-
-extern ptid_t target_wait (ptid_t ptid, struct target_waitstatus *status,
-			   int options);
-
 /* The default target_ops::to_wait implementation.  */
 
 extern ptid_t default_target_wait (struct target_ops *ops,
diff --git a/gdb/target/target.h b/gdb/target/target.h
index 1bfa1b0..a95fbe4 100644
--- a/gdb/target/target.h
+++ b/gdb/target/target.h
@@ -76,4 +76,16 @@ extern void target_continue_no_signal (ptid_t ptid);
 
 extern void target_continue (ptid_t ptid, enum gdb_signal signal);
 
+/* Wait for process pid to do something.  PTID = -1 to wait for any
+   pid to do something.  Return pid of child, or -1 in case of error;
+   store status through argument pointer STATUS.  Note that it is
+   _NOT_ OK to throw_exception() out of target_wait() without popping
+   the debugging target from the stack; GDB isn't prepared to get back
+   to the prompt with a debugging target but without the frame cache,
+   stop_pc, etc., set up.  OPTIONS is a bitwise OR of TARGET_W*
+   options.  */
+
+extern ptid_t target_wait (ptid_t ptid, struct target_waitstatus *status,
+			   int options);
+
 #endif /* TARGET_COMMON_H */
-- 
2.4.3

Re: [PATCH 1/2] Use target_continue{,_no_signal} instead of target_resume

[Pedro Alves]

On 08/29/2016 05:11 AM, Sergio Durigan Junior wrote:

> diff --git a/gdb/target/target.h b/gdb/target/target.h
> index 76af732..1bfa1b0 100644
> --- a/gdb/target/target.h
> +++ b/gdb/target/target.h
> @@ -70,4 +70,10 @@ extern void target_stop_and_wait (ptid_t ptid);
>  
>  extern void target_continue_no_signal (ptid_t ptid);
>  
> +/* Restart a target previously stopped by target_stop_and_wait.

Given you're using this function in other contexts, this sentence
doesn't sound right.  Likewise, the existing comment on top of
target_continue_no_signal is no longer accurate.

Otherwise LGTM.

> +   SIGNAL is delivered to the target.  This function must be provided
> +   by the client.  */
> +
> +extern void target_continue (ptid_t ptid, enum gdb_signal signal);
> +
>  #endif /* TARGET_COMMON_H */
> 

Thanks,
Pedro Alves

Re: [PATCH 2/2] Share target_wait prototype between GDB and gdbserver

[Pedro Alves]

On 08/29/2016 05:11 AM, Sergio Durigan Junior wrote:

> --- a/gdb/target.h
> +++ b/gdb/target.h
> @@ -1341,18 +1341,6 @@ extern void target_disconnect (const char *, int);
>  
>  extern void target_resume (ptid_t ptid, int step, enum gdb_signal signal);
>  
> -/* Wait for process pid to do something.  PTID = -1 to wait for any
> -   pid to do something.  Return pid of child, or -1 in case of error;
> -   store status through argument pointer STATUS.  Note that it is
> -   _NOT_ OK to throw_exception() out of target_wait() without popping
> -   the debugging target from the stack; GDB isn't prepared to get back
> -   to the prompt with a debugging target but without the frame cache,
> -   stop_pc, etc., set up.  OPTIONS is a bitwise OR of TARGET_W*
> -   options.  */
> -
> -extern ptid_t target_wait (ptid_t ptid, struct target_waitstatus *status,
> -			   int options);
> -

Please leave a breadcrumb comment here, like was done for
target_read_memory, for example:

/* For target_read_memory see target/target.h.  */

Thanks,
Pedro Alves

Re: [PATCH 2/2] Share target_wait prototype between GDB and gdbserver

[Pedro Alves]

On 08/31/2016 11:29 AM, Pedro Alves wrote:

> Please leave a breadcrumb comment here, like was done for
> target_read_memory, for example:
> 
> /* For target_read_memory see target/target.h.  */
> 

Hit send too soon.  Meant to say:

OK with that change.

Thanks,
Pedro Alves

Re: [PATCH 1/2] Use target_continue{,_no_signal} instead of target_resume

[Sergio Durigan Junior]

On Wednesday, August 31 2016, Pedro Alves wrote:

> On 08/29/2016 05:11 AM, Sergio Durigan Junior wrote:
>
>> diff --git a/gdb/target/target.h b/gdb/target/target.h
>> index 76af732..1bfa1b0 100644
>> --- a/gdb/target/target.h
>> +++ b/gdb/target/target.h
>> @@ -70,4 +70,10 @@ extern void target_stop_and_wait (ptid_t ptid);
>>  
>>  extern void target_continue_no_signal (ptid_t ptid);
>>  
>> +/* Restart a target previously stopped by target_stop_and_wait.
>
> Given you're using this function in other contexts, this sentence
> doesn't sound right.  Likewise, the existing comment on top of
> target_continue_no_signal is no longer accurate.

Thanks, fixed and pushed.

  049a857091cff98371b5688140832a3cf767153c

-- 
Sergio
GPG key ID: 237A 54B1 0287 28BF 00EF  31F4 D0EB 7628 65FC 5E36
Please send encrypted e-mail if possible
http://sergiodj.net/

Re: [PATCH 2/2] Share target_wait prototype between GDB and gdbserver

[Sergio Durigan Junior]

On Wednesday, August 31 2016, Pedro Alves wrote:

> On 08/29/2016 05:11 AM, Sergio Durigan Junior wrote:
>
>> --- a/gdb/target.h
>> +++ b/gdb/target.h
>> @@ -1341,18 +1341,6 @@ extern void target_disconnect (const char *, int);
>>  
>>  extern void target_resume (ptid_t ptid, int step, enum gdb_signal signal);
>>  
>> -/* Wait for process pid to do something.  PTID = -1 to wait for any
>> -   pid to do something.  Return pid of child, or -1 in case of error;
>> -   store status through argument pointer STATUS.  Note that it is
>> -   _NOT_ OK to throw_exception() out of target_wait() without popping
>> -   the debugging target from the stack; GDB isn't prepared to get back
>> -   to the prompt with a debugging target but without the frame cache,
>> -   stop_pc, etc., set up.  OPTIONS is a bitwise OR of TARGET_W*
>> -   options.  */
>> -
>> -extern ptid_t target_wait (ptid_t ptid, struct target_waitstatus *status,
>> -			   int options);
>> -
>
> Please leave a breadcrumb comment here, like was done for
> target_read_memory, for example:
>
> /* For target_read_memory see target/target.h.  */

Fixed and pushed:

  f2b9e3dfd4bc3c5149496fdbeaa5f0907220685f

Thanks,

-- 
Sergio
GPG key ID: 237A 54B1 0287 28BF 00EF  31F4 D0EB 7628 65FC 5E36
Please send encrypted e-mail if possible
http://sergiodj.net/