[PATCH 0/2] GDBserver: Pass process_info pointers to target methods

[PATCH 0/2] GDBserver: Pass process_info pointers to target methods

[Pedro Alves]

While working on the multi-forks.exp regression fix, I noticed how for
some of the gdbserver target_ops methods, the core of gdbserver starts
from a process_info pointer, passes down the process's pid, and then
the method implementations need to find the process from the pid
again.  This series adjusts the target_ops interfaces to pass the
process_info pointer down directly instead.

Pedro Alves (2):
  GDBserver: Pass process_info pointer to target_detach and target_join
  GDBserver: Pass process_info pointer to target_kill

 gdb/gdbserver/linux-low.c | 26 ++++++++------------------
 gdb/gdbserver/lynx-low.c  | 20 +++++---------------
 gdb/gdbserver/nto-low.c   |  6 ++++--
 gdb/gdbserver/server.c    | 18 +++++++++---------
 gdb/gdbserver/spu-low.c   | 18 ++++++------------
 gdb/gdbserver/target.c    |  6 +++---
 gdb/gdbserver/target.h    | 23 ++++++++++++-----------
 gdb/gdbserver/win32-low.c | 25 ++++++++++---------------
 8 files changed, 57 insertions(+), 85 deletions(-)

-- 
2.14.4

[PATCH 2/2] GDBserver: Pass process_info pointer to target_kill

[Pedro Alves]

We start from a process_info pointer, pass down process->pid, and
then the target_kill implementations need to find the process from the
pid again.  Pass the process_info pointer down directly instead.

gdb/gdbserver/ChangeLog:
yyyy-mm-dd  Pedro Alves  <palves@redhat.com>

	* linux-low.c (linux_kill): Change parameter to process_info
	pointer instead of pid.  Adjust.
	* lynx-low.c (lynx_kill): Likewise.
	* nto-low.c (nto_kill): Likewise.
	* spu-low.c (spu_kill): Likewise.
	* win32-low.c (win32_kill): Likewise.
	* server.c (handle_v_kill, kill_inferior_callback)
	(detach_or_kill_for_exit): Adjust.
	* target.c (kill_inferior): Change parameter to process_info
	pointer instead of pid.  Adjust.
	* target.h (struct target_ops) <kill>: Change parameter to
	process_info pointer instead of pid.  Adjust all implementations
	and callers.
	(kill_inferior): Likewise.
---
 gdb/gdbserver/linux-low.c | 11 +++--------
 gdb/gdbserver/lynx-low.c  |  9 ++-------
 gdb/gdbserver/nto-low.c   |  4 +++-
 gdb/gdbserver/server.c    | 12 ++++++------
 gdb/gdbserver/spu-low.c   |  6 ++----
 gdb/gdbserver/target.c    |  6 +++---
 gdb/gdbserver/target.h    |  6 +++---
 gdb/gdbserver/win32-low.c | 11 +++--------
 8 files changed, 25 insertions(+), 40 deletions(-)

diff --git a/gdb/gdbserver/linux-low.c b/gdb/gdbserver/linux-low.c
index dfa7fbaa49..984464fcc2 100644
--- a/gdb/gdbserver/linux-low.c
+++ b/gdb/gdbserver/linux-low.c
@@ -1385,14 +1385,9 @@ kill_one_lwp_callback (thread_info *thread, int pid)
 }
 
 static int
-linux_kill (int pid)
+linux_kill (process_info *process)
 {
-  struct process_info *process;
-  struct lwp_info *lwp;
-
-  process = find_process_pid (pid);
-  if (process == NULL)
-    return -1;
+  int pid = process->pid;
 
   /* If we're killing a running inferior, make sure it is stopped
      first, as PTRACE_KILL will not work otherwise.  */
@@ -1405,7 +1400,7 @@ linux_kill (int pid)
 
   /* See the comment in linux_kill_one_lwp.  We did not kill the first
      thread in the list, so do so now.  */
-  lwp = find_lwp_pid (ptid_t (pid));
+  lwp_info *lwp = find_lwp_pid (ptid_t (pid));
 
   if (lwp == NULL)
     {
diff --git a/gdb/gdbserver/lynx-low.c b/gdb/gdbserver/lynx-low.c
index 902a70386b..6c5933bc47 100644
--- a/gdb/gdbserver/lynx-low.c
+++ b/gdb/gdbserver/lynx-low.c
@@ -522,15 +522,10 @@ lynx_wait (ptid_t ptid, struct target_waitstatus *status, int options)
 /* Implement the kill target_ops method.  */
 
 static int
-lynx_kill (int pid)
+lynx_kill (process_info *process)
 {
-  ptid_t ptid = lynx_ptid_t (pid, 0);
+  ptid_t ptid = lynx_ptid_t (process->pid, 0);
   struct target_waitstatus status;
-  struct process_info *process;
-
-  process = find_process_pid (pid);
-  if (process == NULL)
-    return -1;
 
   lynx_ptrace (PTRACE_KILL, ptid, 0, 0, 0);
   lynx_wait (ptid, &status, 0);
diff --git a/gdb/gdbserver/nto-low.c b/gdb/gdbserver/nto-low.c
index 46e96449a1..098678e70b 100644
--- a/gdb/gdbserver/nto-low.c
+++ b/gdb/gdbserver/nto-low.c
@@ -397,8 +397,10 @@ nto_attach (unsigned long pid)
 /* Send signal to process PID.  */
 
 static int
-nto_kill (int pid)
+nto_kill (process_info *proc)
 {
+  int pid = proc->pid;
+
   TRACE ("%s %d\n", __func__, pid);
   kill (pid, SIGKILL);
   do_detach ();
diff --git a/gdb/gdbserver/server.c b/gdb/gdbserver/server.c
index b35015b7bf..540f643607 100644
--- a/gdb/gdbserver/server.c
+++ b/gdb/gdbserver/server.c
@@ -3080,7 +3080,10 @@ handle_v_kill (char *own_buf)
     pid = strtol (p, NULL, 16);
   else
     pid = signal_pid;
-  if (pid != 0 && kill_inferior (pid) == 0)
+
+  process_info *proc = find_process_pid (pid);
+
+  if (proc != nullptr && kill_inferior (proc) == 0)
     {
       cs.last_status.kind = TARGET_WAITKIND_SIGNALLED;
       cs.last_status.value.sig = GDB_SIGNAL_KILL;
@@ -3481,10 +3484,7 @@ gdbserver_show_disableable (FILE *stream)
 static void
 kill_inferior_callback (process_info *process)
 {
-  int pid = process->pid;
-
-  kill_inferior (pid);
-  discard_queued_stop_replies (ptid_t (pid));
+  discard_queued_stop_replies (ptid_t (process->pid));
 }
 
 /* Call this when exiting gdbserver with possible inferiors that need
@@ -3528,7 +3528,7 @@ detach_or_kill_for_exit (void)
     if (process->attached)
       detach_inferior (process);
     else
-      kill_inferior (pid);
+      kill_inferior (process);
 
     discard_queued_stop_replies (ptid_t (pid));
   });
diff --git a/gdb/gdbserver/spu-low.c b/gdb/gdbserver/spu-low.c
index c2a8734f68..83a31a203d 100644
--- a/gdb/gdbserver/spu-low.c
+++ b/gdb/gdbserver/spu-low.c
@@ -326,12 +326,10 @@ spu_attach (unsigned long  pid)
 
 /* Kill the inferior process.  */
 static int
-spu_kill (int pid)
+spu_kill (process_info *process)
 {
   int status, ret;
-  struct process_info *process = find_process_pid (pid);
-  if (process == NULL)
-    return -1;
+  int pid = process->pid;
 
   ptrace (PTRACE_KILL, pid, 0, 0);
 
diff --git a/gdb/gdbserver/target.c b/gdb/gdbserver/target.c
index 00379bfdc0..886e6cfb5c 100644
--- a/gdb/gdbserver/target.c
+++ b/gdb/gdbserver/target.c
@@ -332,11 +332,11 @@ target_pid_to_str (ptid_t ptid)
 }
 
 int
-kill_inferior (int pid)
+kill_inferior (process_info *proc)
 {
-  gdb_agent_about_to_close (pid);
+  gdb_agent_about_to_close (proc->pid);
 
-  return (*the_target->kill) (pid);
+  return (*the_target->kill) (proc);
 }
 
 /* Target can do hardware single step.  */
diff --git a/gdb/gdbserver/target.h b/gdb/gdbserver/target.h
index 7f59485580..fce54e05ad 100644
--- a/gdb/gdbserver/target.h
+++ b/gdb/gdbserver/target.h
@@ -90,9 +90,9 @@ struct target_ops
 
   int (*attach) (unsigned long pid);
 
-  /* Kill inferior PID.  Return -1 on failure, and 0 on success.  */
+  /* Kill process PROC.  Return -1 on failure, and 0 on success.  */
 
-  int (*kill) (int pid);
+  int (*kill) (process_info *proc);
 
   /* Detach from process PROC.  Return -1 on failure, and 0 on
      success.  */
@@ -497,7 +497,7 @@ void set_target_ops (struct target_ops *);
 #define myattach(pid) \
   (*the_target->attach) (pid)
 
-int kill_inferior (int);
+int kill_inferior (process_info *proc);
 
 #define target_supports_fork_events() \
   (the_target->supports_fork_events ? \
diff --git a/gdb/gdbserver/win32-low.c b/gdb/gdbserver/win32-low.c
index 17729a83cc..765391dd47 100644
--- a/gdb/gdbserver/win32-low.c
+++ b/gdb/gdbserver/win32-low.c
@@ -805,15 +805,11 @@ win32_clear_inferiors (void)
   clear_inferiors ();
 }
 
-/* Kill all inferiors.  */
+/* Implementation of target_ops::kill.  */
+
 static int
-win32_kill (int pid)
+win32_kill (process_info *process)
 {
-  struct process_info *process;
-
-  if (current_process_handle == NULL)
-    return -1;
-
   TerminateProcess (current_process_handle, 0);
   for (;;)
     {
@@ -829,7 +825,6 @@ win32_kill (int pid)
 
   win32_clear_inferiors ();
 
-  process = find_process_pid (pid);
   remove_process (process);
   return 0;
 }
-- 
2.14.4

[PATCH 1/2] GDBserver: Pass process_info pointer to target_detach and target_join

[Pedro Alves]

We start from a process_info pointer, pass down process->pid, and then
the target_detach and target_join implementations need to find the
process from the pid again.  Pass the process_info pointer down
directly instead.

gdb/gdbserver/ChangeLog:
yyyy-mm-dd  Pedro Alves  <palves@redhat.com>

	* linux-low.c (linux_detach, linux_join): Change parameter to
	process_info pointer instead of pid.  Adjust.
	* lynx-low.c (lynx_detach, lynx_join): Likewise.
	* nto-low.c (nto_detach): Likewise.
	* spu-low.c (spu_detach, spu_join): Likewise.
	* win32-low.c (win32_detach, win32_join): Likewise.
	* server.c (handle_detach, detach_or_kill_for_exit): Adjust.
	* target.h (struct target_ops) <detach, join>: Change parameter to
	process_info pointer instead of pid.  Adjust all implementations
	and callers.
	(detach_inferior, join_inferior): Rename 'pid' parameter to
	'proc'.
---
 gdb/gdbserver/linux-low.c | 15 +++++----------
 gdb/gdbserver/lynx-low.c  | 11 +++--------
 gdb/gdbserver/nto-low.c   |  2 +-
 gdb/gdbserver/server.c    |  6 +++---
 gdb/gdbserver/spu-low.c   | 12 ++++--------
 gdb/gdbserver/target.h    | 17 +++++++++--------
 gdb/gdbserver/win32-low.c | 14 +++++++-------
 7 files changed, 32 insertions(+), 45 deletions(-)

diff --git a/gdb/gdbserver/linux-low.c b/gdb/gdbserver/linux-low.c
index 7c396ae366..dfa7fbaa49 100644
--- a/gdb/gdbserver/linux-low.c
+++ b/gdb/gdbserver/linux-low.c
@@ -1608,15 +1608,10 @@ linux_detach_lwp_callback (thread_info *thread)
 }
 
 static int
-linux_detach (int pid)
+linux_detach (process_info *process)
 {
-  struct process_info *process;
   struct lwp_info *main_lwp;
 
-  process = find_process_pid (pid);
-  if (process == NULL)
-    return -1;
-
   /* As there's a step over already in progress, let it finish first,
      otherwise nesting a stabilize_threads operation on top gets real
      messy.  */
@@ -1638,9 +1633,9 @@ linux_detach (int pid)
   /* Detach from the clone lwps first.  If the thread group exits just
      while we're detaching, we must reap the clone lwps before we're
      able to reap the leader.  */
-  for_each_thread (pid, linux_detach_lwp_callback);
+  for_each_thread (process->pid, linux_detach_lwp_callback);
 
-  main_lwp = find_lwp_pid (ptid_t (pid));
+  main_lwp = find_lwp_pid (ptid_t (process->pid));
   linux_detach_one_lwp (main_lwp);
 
   the_target->mourn (process);
@@ -1680,12 +1675,12 @@ linux_mourn (struct process_info *process)
 }
 
 static void
-linux_join (int pid)
+linux_join (process_info *proc)
 {
   int status, ret;
 
   do {
-    ret = my_waitpid (pid, &status, 0);
+    ret = my_waitpid (proc->pid, &status, 0);
     if (WIFEXITED (status) || WIFSIGNALED (status))
       break;
   } while (ret != -1 || errno != ECHILD);
diff --git a/gdb/gdbserver/lynx-low.c b/gdb/gdbserver/lynx-low.c
index b68bbeaf20..902a70386b 100644
--- a/gdb/gdbserver/lynx-low.c
+++ b/gdb/gdbserver/lynx-low.c
@@ -541,14 +541,9 @@ lynx_kill (int pid)
 /* Implement the detach target_ops method.  */
 
 static int
-lynx_detach (int pid)
+lynx_detach (process_info *process)
 {
-  ptid_t ptid = lynx_ptid_t (pid, 0);
-  struct process_info *process;
-
-  process = find_process_pid (pid);
-  if (process == NULL)
-    return -1;
+  ptid_t ptid = lynx_ptid_t (process->pid, 0);
 
   lynx_ptrace (PTRACE_DETACH, ptid, 0, 0, 0);
   the_target->mourn (process);
@@ -572,7 +567,7 @@ lynx_mourn (struct process_info *proc)
 /* Implement the join target_ops method.  */
 
 static void
-lynx_join (int pid)
+lynx_join (process_info *proc)
 {
   /* The PTRACE_DETACH is sufficient to detach from the process.
      So no need to do anything extra.  */
diff --git a/gdb/gdbserver/nto-low.c b/gdb/gdbserver/nto-low.c
index 236e555714..46e96449a1 100644
--- a/gdb/gdbserver/nto-low.c
+++ b/gdb/gdbserver/nto-low.c
@@ -408,7 +408,7 @@ nto_kill (int pid)
 /* Detach from process PID.  */
 
 static int
-nto_detach (int pid)
+nto_detach (process_info *proc)
 {
   TRACE ("%s %d\n", __func__, pid);
   do_detach ();
diff --git a/gdb/gdbserver/server.c b/gdb/gdbserver/server.c
index bf6302ba2d..b35015b7bf 100644
--- a/gdb/gdbserver/server.c
+++ b/gdb/gdbserver/server.c
@@ -1255,7 +1255,7 @@ handle_detach (char *own_buf)
 
   fprintf (stderr, "Detaching from process %d\n", process->pid);
   stop_tracing ();
-  if (detach_inferior (process->pid) != 0)
+  if (detach_inferior (process) != 0)
     write_enn (own_buf);
   else
     {
@@ -1281,7 +1281,7 @@ handle_detach (char *own_buf)
 	  /* If we are attached, then we can exit.  Otherwise, we
 	     need to hang around doing nothing, until the child is
 	     gone.  */
-	  join_inferior (process->pid);
+	  join_inferior (process);
 	  exit (0);
 	}
     }
@@ -3526,7 +3526,7 @@ detach_or_kill_for_exit (void)
     int pid = process->pid;
 
     if (process->attached)
-      detach_inferior (pid);
+      detach_inferior (process);
     else
       kill_inferior (pid);
 
diff --git a/gdb/gdbserver/spu-low.c b/gdb/gdbserver/spu-low.c
index e9fc6e7beb..c2a8734f68 100644
--- a/gdb/gdbserver/spu-low.c
+++ b/gdb/gdbserver/spu-low.c
@@ -348,13 +348,9 @@ spu_kill (int pid)
 
 /* Detach from inferior process.  */
 static int
-spu_detach (int pid)
+spu_detach (process_info *process)
 {
-  struct process_info *process = find_process_pid (pid);
-  if (process == NULL)
-    return -1;
-
-  ptrace (PTRACE_DETACH, pid, 0, 0);
+  ptrace (PTRACE_DETACH, process->pid, 0, 0);
 
   clear_inferiors ();
   remove_process (process);
@@ -368,12 +364,12 @@ spu_mourn (struct process_info *process)
 }
 
 static void
-spu_join (int pid)
+spu_join (process_info *proc)
 {
   int status, ret;
 
   do {
-    ret = waitpid (pid, &status, 0);
+    ret = waitpid (proc->pid, &status, 0);
     if (WIFEXITED (status) || WIFSIGNALED (status))
       break;
   } while (ret != -1 || errno != ECHILD);
diff --git a/gdb/gdbserver/target.h b/gdb/gdbserver/target.h
index 25accd2207..7f59485580 100644
--- a/gdb/gdbserver/target.h
+++ b/gdb/gdbserver/target.h
@@ -94,17 +94,18 @@ struct target_ops
 
   int (*kill) (int pid);
 
-  /* Detach from inferior PID. Return -1 on failure, and 0 on
+  /* Detach from process PROC.  Return -1 on failure, and 0 on
      success.  */
 
-  int (*detach) (int pid);
+  int (*detach) (process_info *proc);
 
   /* The inferior process has died.  Do what is right.  */
 
   void (*mourn) (struct process_info *proc);
 
-  /* Wait for inferior PID to exit.  */
-  void (*join) (int pid);
+  /* Wait for process PROC to exit.  */
+
+  void (*join) (process_info *proc);
 
   /* Return 1 iff the thread with process ID PID is alive.  */
 
@@ -517,8 +518,8 @@ int kill_inferior (int);
 	(*the_target->handle_new_gdb_connection) ();	 \
     } while (0)
 
-#define detach_inferior(pid) \
-  (*the_target->detach) (pid)
+#define detach_inferior(proc) \
+  (*the_target->detach) (proc)
 
 #define mythread_alive(pid) \
   (*the_target->thread_alive) (pid)
@@ -529,8 +530,8 @@ int kill_inferior (int);
 #define store_inferior_registers(regcache, regno) \
   (*the_target->store_registers) (regcache, regno)
 
-#define join_inferior(pid) \
-  (*the_target->join) (pid)
+#define join_inferior(proc) \
+  (*the_target->join) (proc)
 
 #define target_supports_non_stop() \
   (the_target->supports_non_stop ? (*the_target->supports_non_stop ) () : 0)
diff --git a/gdb/gdbserver/win32-low.c b/gdb/gdbserver/win32-low.c
index 8a20972bd4..17729a83cc 100644
--- a/gdb/gdbserver/win32-low.c
+++ b/gdb/gdbserver/win32-low.c
@@ -834,11 +834,11 @@ win32_kill (int pid)
   return 0;
 }
 
-/* Detach from inferior PID.  */
+/* Implementation of target_ops::detach.  */
+
 static int
-win32_detach (int pid)
+win32_detach (process_info *process)
 {
-  struct process_info *process;
   winapi_DebugActiveProcessStop DebugActiveProcessStop = NULL;
   winapi_DebugSetProcessKillOnExit DebugSetProcessKillOnExit = NULL;
 #ifdef _WIN32_WCE
@@ -865,7 +865,6 @@ win32_detach (int pid)
     return -1;
 
   DebugSetProcessKillOnExit (FALSE);
-  process = find_process_pid (pid);
   remove_process (process);
 
   win32_clear_inferiors ();
@@ -878,11 +877,12 @@ win32_mourn (struct process_info *process)
   remove_process (process);
 }
 
-/* Wait for inferiors to end.  */
+/* Implementation of target_ops::join.  */
+
 static void
-win32_join (int pid)
+win32_join (process_info *proc)
 {
-  HANDLE h = OpenProcess (PROCESS_ALL_ACCESS, FALSE, pid);
+  HANDLE h = OpenProcess (PROCESS_ALL_ACCESS, FALSE, proc->pid);
   if (h != NULL)
     {
       WaitForSingleObject (h, INFINITE);
-- 
2.14.4

Re: [PATCH 1/2] GDBserver: Pass process_info pointer to target_detach and target_join

[Simon Marchi]

On 2018-07-12 06:25 PM, Pedro Alves wrote:
> diff --git a/gdb/gdbserver/nto-low.c b/gdb/gdbserver/nto-low.c
> index 236e555714..46e96449a1 100644
> --- a/gdb/gdbserver/nto-low.c
> +++ b/gdb/gdbserver/nto-low.c
> @@ -408,7 +408,7 @@ nto_kill (int pid)
>  /* Detach from process PID.  */
>  
>  static int
> -nto_detach (int pid)
> +nto_detach (process_info *proc)
>  {
>    TRACE ("%s %d\n", __func__, pid);

This last line should be updated to use proc->pid.

Otherwise, LGTM.

Simon

Re: [PATCH 2/2] GDBserver: Pass process_info pointer to target_kill

[Simon Marchi]

On 2018-07-12 06:25 PM, Pedro Alves wrote:
> @@ -3481,10 +3484,7 @@ gdbserver_show_disableable (FILE *stream)
>  static void
>  kill_inferior_callback (process_info *process)
>  {
> -  int pid = process->pid;
> -
> -  kill_inferior (pid);
> -  discard_queued_stop_replies (ptid_t (pid));
> +  discard_queued_stop_replies (ptid_t (process->pid));
>  }

Hmm this doesn't call kill_inferior anymore, I guess that's not intended?

Otherwise, LGTM.

Simon

Re: [PATCH 1/2] GDBserver: Pass process_info pointer to target_detach and target_join

[Pedro Alves]

On 07/13/2018 04:01 AM, Simon Marchi wrote:
> On 2018-07-12 06:25 PM, Pedro Alves wrote:
>> diff --git a/gdb/gdbserver/nto-low.c b/gdb/gdbserver/nto-low.c
>> index 236e555714..46e96449a1 100644
>> --- a/gdb/gdbserver/nto-low.c
>> +++ b/gdb/gdbserver/nto-low.c
>> @@ -408,7 +408,7 @@ nto_kill (int pid)
>>  /* Detach from process PID.  */
>>  
>>  static int
>> -nto_detach (int pid)
>> +nto_detach (process_info *proc)
>>  {
>>    TRACE ("%s %d\n", __func__, pid);
> 
> This last line should be updated to use proc->pid.
> 
> Otherwise, LGTM.

Whoops, fixed that and pushed.

Thanks,
Pedro Alves

Re: [PATCH 2/2] GDBserver: Pass process_info pointer to target_kill

[Pedro Alves]

On 07/13/2018 04:06 AM, Simon Marchi wrote:
> On 2018-07-12 06:25 PM, Pedro Alves wrote:
>> @@ -3481,10 +3484,7 @@ gdbserver_show_disableable (FILE *stream)
>>  static void
>>  kill_inferior_callback (process_info *process)
>>  {
>> -  int pid = process->pid;
>> -
>> -  kill_inferior (pid);
>> -  discard_queued_stop_replies (ptid_t (pid));
>> +  discard_queued_stop_replies (ptid_t (process->pid));
>>  }
> 
> Hmm this doesn't call kill_inferior anymore, I guess that's not intended?

Aw, certainly not intended.  Thanks for noticing.

> Otherwise, LGTM.

I've restored the call and pushed.

Thanks,
Pedro Alves