Re: [PATCH] Introduce utility function find_inferior_ptid

[Simon Marchi <simon dot marchi at polymtl dot ca>]

On 13/12/14 09:40 AM, Joel Brobecker wrote:
>> This patch introduces find_inferior_ptid to replace the common idiom
>>
>>   find_inferior_pid (ptid_get_pid (...));
>>
>> It replaces all the instances of that idiom that I found with the new
>> function.
>>
>> The change was mostly mechanical and built-tested.
>>
>> gdb/ChangeLog:
>>
>> 	* inferior.c (find_inferior_ptid): New function.
>> 	* inferior.h (find_inferior_ptid): New declaration.
>> 	* ada-tasks.c (ada_get_task_number): Use find_inferior_ptid.
>> 	* corelow.c (core_pid_to_str): Same.
>> 	* darwin-nat.c (darwin_resume): Same.
>> 	* infrun.c (fetch_inferior_event): Same.
>> 	(get_inferior_stop_soon): Same.
>> 	(handle_inferior_event): Same.
>> 	(handle_signal_stop): Same.
>> 	* linux-nat.c (resume_lwp): Same.
>> 	(stop_wait_callback): Same.
>> 	* mi/mi-interp.c (mi_new_thread): Same.
>> 	(mi_thread_exit): Same.
>> 	* proc-service.c (ps_pglobal_lookup): Same.
>> 	* record-btrace.c (record_btrace_step_thread): Same.
>> 	* remote-sim.c (gdbsim_close_inferior): Same.
>> 	(gdbsim_resume): Same.
>> 	(gdbsim_stop): Same.
>> 	* sol2-tdep.c (sol2_core_pid_to_str): Same.
>> 	* target.c (memory_xfer_partial_1): Same.
>> 	(default_thread_address_space): Same.
>> 	* thread.c (thread_change_ptid): Same.
>> 	(switch_to_thread): Same.
>> 	(do_restore_current_thread_cleanup): Same.
> 
> Overall, this looks reasonable. A couple of comments below.
> Also, while at it, I would prefer if you tested the change.
> With the number of mechanical changes, it's really easy to
> let one bad change slip by, no matter how careful we try
> to be; testing does not cost much so let's add that as well.


Oops, it's a very good thing you made me test the patch again, because
there is an horrible, obvious mistake in:

struct inferior *
find_inferior_ptid (ptid_t ptid) {
  return find_inferior_ptid (ptid);

I actually forgot to git add/git commit --amend my latest fixes before
sending the patch, so some important changes about that were still unstaged
in my git tree. There were a few extraneous closing parenthesis left.

With v2 (below), the testsuite results look similar before/after (just
the usual small variations), although it's just for amd64 Linux, and
the change touches other targets.

>>  
>> +struct inferior *
>> +find_inferior_ptid (ptid_t ptid) {
>> +  return find_inferior_ptid (ptid);
>> +}
> 
> This function needs an introductory commend ("/* See inferior.h */").
> This is something we do systematically now, even if the function's
> documentation is present elsewhere. The one-line comment allows us
> to know that there is a command, in incidentally where it is.

Ok.

> Also, the opening curly brace needs to be on the next line. Therefore:
> 
>         /* See inferior.h.  */
> 
>         struct inferior *
>         find_inferior_ptid (ptid_t ptid)
>         {
>           return find_inferior_ptid (ptid);
>         }

Ok.

>> +/* Search function to lookup an inferior whose pid is equal to 'ptid.pid'. */
>> +extern struct inferior * find_inferior_ptid (ptid_t ptid);
> 
> No space after the '*'.

Ok.

> Pre-approved with those changes and after testing confirmed.
> 
> Thank you,

Ok, here is what I will push on Monday if nothing comes up.

>From 5756f3cd6c26a16d9e0e57b7fd8044c635d0a53d Mon Sep 17 00:00:00 2001
From: Simon Marchi <simon.marchi@ericsson.com>
Date: Sat, 13 Dec 2014 12:12:10 -0500
Subject: [PATCH] Introduce utility function find_inferior_ptid

This patch introduces find_inferior_ptid to replace the common idiom

  find_inferior_pid (ptid_get_pid (...));

It replaces all the instances of that idiom that I found with the new
function.

No significant changes before/after the patch in the regression suite
on amd64 linux.

gdb/ChangeLog:

	* inferior.c (find_inferior_ptid): New function.
	* inferior.h (find_inferior_ptid): New declaration.
	* ada-tasks.c (ada_get_task_number): Use find_inferior_ptid.
	* corelow.c (core_pid_to_str): Same.
	* darwin-nat.c (darwin_resume): Same.
	* infrun.c (fetch_inferior_event): Same.
	(get_inferior_stop_soon): Same.
	(handle_inferior_event): Same.
	(handle_signal_stop): Same.
	* linux-nat.c (resume_lwp): Same.
	(stop_wait_callback): Same.
	* mi/mi-interp.c (mi_new_thread): Same.
	(mi_thread_exit): Same.
	* proc-service.c (ps_pglobal_lookup): Same.
	* record-btrace.c (record_btrace_step_thread): Same.
	* remote-sim.c (gdbsim_close_inferior): Same.
	(gdbsim_resume): Same.
	(gdbsim_stop): Same.
	* sol2-tdep.c (sol2_core_pid_to_str): Same.
	* target.c (memory_xfer_partial_1): Same.
	(default_thread_address_space): Same.
	* thread.c (thread_change_ptid): Same.
	(switch_to_thread): Same.
	(do_restore_current_thread_cleanup): Same.
---
 gdb/ada-tasks.c     |  3 +--
 gdb/corelow.c       |  2 +-
 gdb/darwin-nat.c    |  2 +-
 gdb/inferior.c      |  8 ++++++++
 gdb/inferior.h      |  3 +++
 gdb/infrun.c        | 10 +++++-----
 gdb/linux-nat.c     |  4 ++--
 gdb/mi/mi-interp.c  |  4 ++--
 gdb/proc-service.c  |  2 +-
 gdb/record-btrace.c |  4 ++--
 gdb/remote-sim.c    |  6 +++---
 gdb/sol2-tdep.c     |  2 +-
 gdb/target.c        |  4 ++--
 gdb/thread.c        |  6 +++---
 14 files changed, 35 insertions(+), 25 deletions(-)

diff --git a/gdb/ada-tasks.c b/gdb/ada-tasks.c
index 2d5a19d..17d0338 100644
--- a/gdb/ada-tasks.c
+++ b/gdb/ada-tasks.c
@@ -292,7 +292,7 @@ int
 ada_get_task_number (ptid_t ptid)
 {
   int i;
-  struct inferior *inf = find_inferior_pid (ptid_get_pid (ptid));
+  struct inferior *inf = find_inferior_ptid (ptid);
   struct ada_tasks_inferior_data *data;

   gdb_assert (inf != NULL);
@@ -1449,4 +1449,3 @@ _initialize_tasks (void)
 Without argument, this command simply prints the current task ID"),
            &cmdlist);
 }
-
diff --git a/gdb/corelow.c b/gdb/corelow.c
index b91ad22..154b2c4 100644
--- a/gdb/corelow.c
+++ b/gdb/corelow.c
@@ -954,7 +954,7 @@ core_pid_to_str (struct target_ops *ops, ptid_t ptid)

   /* Otherwise, this isn't a "threaded" core -- use the PID field, but
      only if it isn't a fake PID.  */
-  inf = find_inferior_pid (ptid_get_pid (ptid));
+  inf = find_inferior_ptid (ptid);
   if (inf != NULL && !inf->fake_pid_p)
     return normal_pid_to_str (ptid);

diff --git a/gdb/darwin-nat.c b/gdb/darwin-nat.c
index 511f370..36b6021 100644
--- a/gdb/darwin-nat.c
+++ b/gdb/darwin-nat.c
@@ -859,7 +859,7 @@ darwin_resume (ptid_t ptid, int step, enum gdb_signal signal)
     }
   else
     {
-      struct inferior *inf = find_inferior_pid (ptid_get_pid (ptid));
+      struct inferior *inf = find_inferior_ptid (ptid);
       long tid = ptid_get_tid (ptid);

       /* Stop the inferior (should be useless).  */
diff --git a/gdb/inferior.c b/gdb/inferior.c
index 44f4560..49c479d 100644
--- a/gdb/inferior.c
+++ b/gdb/inferior.c
@@ -367,6 +367,14 @@ find_inferior_pid (int pid)
   return NULL;
 }

+/* See inferior.h */
+
+struct inferior *
+find_inferior_ptid (ptid_t ptid)
+{
+  return find_inferior_pid (ptid_get_pid (ptid));
+}
+
 /* See inferior.h.  */

 struct inferior *
diff --git a/gdb/inferior.h b/gdb/inferior.h
index 0129549..eebc034 100644
--- a/gdb/inferior.h
+++ b/gdb/inferior.h
@@ -465,6 +465,9 @@ extern int valid_gdb_inferior_id (int num);
 /* Search function to lookup an inferior by target 'pid'.  */
 extern struct inferior *find_inferior_pid (int pid);

+/* Search function to lookup an inferior whose pid is equal to 'ptid.pid'. */
+extern struct inferior *find_inferior_ptid (ptid_t ptid);
+
 /* Search function to lookup an inferior by GDB 'num'.  */
 extern struct inferior *find_inferior_id (int num);

diff --git a/gdb/infrun.c b/gdb/infrun.c
index 74f9e12..413b052 100644
--- a/gdb/infrun.c
+++ b/gdb/infrun.c
@@ -3262,7 +3262,7 @@ fetch_inferior_event (void *client_data)

   if (!ecs->wait_some_more)
     {
-      struct inferior *inf = find_inferior_pid (ptid_get_pid (ecs->ptid));
+      struct inferior *inf = find_inferior_ptid (ecs->ptid);

       delete_just_stopped_threads_infrun_breakpoints ();

@@ -3581,7 +3581,7 @@ fill_in_stop_func (struct gdbarch *gdbarch,
 static enum stop_kind
 get_inferior_stop_soon (ptid_t ptid)
 {
-  struct inferior *inf = find_inferior_pid (ptid_get_pid (ptid));
+  struct inferior *inf = find_inferior_ptid (ptid);

   gdb_assert (inf != NULL);
   return inf->control.stop_soon;
@@ -3815,7 +3815,7 @@ handle_inferior_event (struct execution_control_state *ecs)
 	}

       inferior_ptid = ecs->ptid;
-      set_current_inferior (find_inferior_pid (ptid_get_pid (ecs->ptid)));
+      set_current_inferior (find_inferior_ptid (ecs->ptid));
       set_current_program_space (current_inferior ()->pspace);
       handle_vfork_child_exec_or_exit (0);
       target_terminal_ours ();	/* Must do this before mourn anyway.  */
@@ -3899,7 +3899,7 @@ Cannot fill $_exitsignal with the correct signal number.\n"));
 	if (displaced && ptid_equal (displaced->step_ptid, ecs->ptid))
 	  {
 	    struct inferior *parent_inf
-	      = find_inferior_pid (ptid_get_pid (ecs->ptid));
+	      = find_inferior_ptid (ecs->ptid);
 	    struct regcache *child_regcache;
 	    CORE_ADDR parent_pc;

@@ -4495,7 +4495,7 @@ handle_signal_stop (struct execution_control_state *ecs)
   if (random_signal)
     {
       /* Signal not for debugging purposes.  */
-      struct inferior *inf = find_inferior_pid (ptid_get_pid (ecs->ptid));
+      struct inferior *inf = find_inferior_ptid (ecs->ptid);
       enum gdb_signal stop_signal = ecs->event_thread->suspend.stop_signal;

       if (debug_infrun)
diff --git a/gdb/linux-nat.c b/gdb/linux-nat.c
index 21797c1..29133f9 100644
--- a/gdb/linux-nat.c
+++ b/gdb/linux-nat.c
@@ -1435,7 +1435,7 @@ resume_lwp (struct lwp_info *lp, int step, enum gdb_signal signo)
 {
   if (lp->stopped)
     {
-      struct inferior *inf = find_inferior_pid (ptid_get_pid (lp->ptid));
+      struct inferior *inf = find_inferior_ptid (lp->ptid);

       if (inf->vfork_child != NULL)
 	{
@@ -2388,7 +2388,7 @@ linux_nat_set_status_is_event (struct target_ops *t,
 static int
 stop_wait_callback (struct lwp_info *lp, void *data)
 {
-  struct inferior *inf = find_inferior_pid (ptid_get_pid (lp->ptid));
+  struct inferior *inf = find_inferior_ptid (lp->ptid);

   /* If this is a vfork parent, bail out, it is not going to report
      any SIGSTOP until the vfork is done with.  */
diff --git a/gdb/mi/mi-interp.c b/gdb/mi/mi-interp.c
index 60f0666..09ff7bf 100644
--- a/gdb/mi/mi-interp.c
+++ b/gdb/mi/mi-interp.c
@@ -371,7 +371,7 @@ static void
 mi_new_thread (struct thread_info *t)
 {
   struct mi_interp *mi = top_level_interpreter_data ();
-  struct inferior *inf = find_inferior_pid (ptid_get_pid (t->ptid));
+  struct inferior *inf = find_inferior_ptid (t->ptid);

   gdb_assert (inf);

@@ -391,7 +391,7 @@ mi_thread_exit (struct thread_info *t, int silent)
   if (silent)
     return;

-  inf = find_inferior_pid (ptid_get_pid (t->ptid));
+  inf = find_inferior_ptid (t->ptid);

   mi = top_level_interpreter_data ();
   old_chain = make_cleanup_restore_target_terminal ();
diff --git a/gdb/proc-service.c b/gdb/proc-service.c
index 5819489..1dc98d8 100644
--- a/gdb/proc-service.c
+++ b/gdb/proc-service.c
@@ -114,7 +114,7 @@ ps_pglobal_lookup (gdb_ps_prochandle_t ph, const char *obj,
 {
   struct bound_minimal_symbol ms;
   struct cleanup *old_chain = save_current_program_space ();
-  struct inferior *inf = find_inferior_pid (ptid_get_pid (ph->ptid));
+  struct inferior *inf = find_inferior_ptid (ph->ptid);
   ps_err_e result;

   set_current_program_space (inf->pspace);
diff --git a/gdb/record-btrace.c b/gdb/record-btrace.c
index 65d28e3..8b98a91 100644
--- a/gdb/record-btrace.c
+++ b/gdb/record-btrace.c
@@ -1662,7 +1662,7 @@ record_btrace_step_thread (struct thread_info *tp)
       if (replay == NULL)
 	return btrace_step_no_history ();

-      inf = find_inferior_pid (ptid_get_pid (tp->ptid));
+      inf = find_inferior_ptid (tp->ptid);
       aspace = inf->aspace;

       /* Determine the end of the instruction trace.  */
@@ -1699,7 +1699,7 @@ record_btrace_step_thread (struct thread_info *tp)
       if (replay == NULL)
 	replay = record_btrace_start_replaying (tp);

-      inf = find_inferior_pid (ptid_get_pid (tp->ptid));
+      inf = find_inferior_ptid (tp->ptid);
       aspace = inf->aspace;

       for (;;)
diff --git a/gdb/remote-sim.c b/gdb/remote-sim.c
index 8901548..92c41ad 100644
--- a/gdb/remote-sim.c
+++ b/gdb/remote-sim.c
@@ -774,7 +774,7 @@ gdbsim_close_inferior (struct inferior *inf, void *arg)
 	 Thus we need to verify the existence of an inferior using the
 	 pid in question before setting inferior_ptid via
 	 switch_to_thread() or mourning the inferior.  */
-      if (find_inferior_pid (ptid_get_pid (ptid)) != NULL)
+      if (find_inferior_ptid (ptid) != NULL)
 	{
 	  switch_to_thread (ptid);
 	  generic_mourn_inferior ();
@@ -881,7 +881,7 @@ gdbsim_resume (struct target_ops *ops,
      either have multiple inferiors to resume or an error condition.  */

   if (sim_data)
-    gdbsim_resume_inferior (find_inferior_pid (ptid_get_pid (ptid)), &rd);
+    gdbsim_resume_inferior (find_inferior_ptid (ptid), &rd);
   else if (ptid_equal (ptid, minus_one_ptid))
     iterate_over_inferiors (gdbsim_resume_inferior, &rd);
   else
@@ -928,7 +928,7 @@ gdbsim_stop (struct target_ops *self, ptid_t ptid)
     }
   else
     {
-      struct inferior *inf = find_inferior_pid (ptid_get_pid (ptid));
+      struct inferior *inf = find_inferior_ptid (ptid);

       if (inf == NULL)
 	error (_("Can't stop pid %d.  No inferior found."),
diff --git a/gdb/sol2-tdep.c b/gdb/sol2-tdep.c
index bf7d6a1..a508978 100644
--- a/gdb/sol2-tdep.c
+++ b/gdb/sol2-tdep.c
@@ -60,7 +60,7 @@ sol2_core_pid_to_str (struct gdbarch *gdbarch, ptid_t ptid)
   /* GDB didn't use to put a NT_PSTATUS note in Solaris cores.  If
      that's missing, then we're dealing with a fake PID corelow.c made
      up.  */
-  inf = find_inferior_pid (ptid_get_pid (ptid));
+  inf = find_inferior_ptid (ptid);
   if (inf == NULL || inf->fake_pid_p)
     {
       xsnprintf (buf, sizeof buf, "<core>");
diff --git a/gdb/target.c b/gdb/target.c
index 7161e62..38c6c16 100644
--- a/gdb/target.c
+++ b/gdb/target.c
@@ -1173,7 +1173,7 @@ memory_xfer_partial_1 (struct target_ops *ops, enum target_object object,
     return TARGET_XFER_E_IO;

   if (!ptid_equal (inferior_ptid, null_ptid))
-    inf = find_inferior_pid (ptid_get_pid (inferior_ptid));
+    inf = find_inferior_ptid (inferior_ptid);
   else
     inf = NULL;

@@ -2649,7 +2649,7 @@ default_thread_address_space (struct target_ops *self, ptid_t ptid)
   struct inferior *inf;

   /* Fall-back to the "main" address space of the inferior.  */
-  inf = find_inferior_pid (ptid_get_pid (ptid));
+  inf = find_inferior_ptid (ptid);

   if (inf == NULL || inf->aspace == NULL)
     internal_error (__FILE__, __LINE__,
diff --git a/gdb/thread.c b/gdb/thread.c
index a3040a7..782fc70 100644
--- a/gdb/thread.c
+++ b/gdb/thread.c
@@ -747,7 +747,7 @@ thread_change_ptid (ptid_t old_ptid, ptid_t new_ptid)
   /* It can happen that what we knew as the target inferior id
      changes.  E.g, target remote may only discover the remote process
      pid after adding the inferior to GDB's list.  */
-  inf = find_inferior_pid (ptid_get_pid (old_ptid));
+  inf = find_inferior_ptid (old_ptid);
   inf->pid = ptid_get_pid (new_ptid);

   tp = find_thread_ptid (old_ptid);
@@ -1179,7 +1179,7 @@ switch_to_thread (ptid_t ptid)
     {
       struct inferior *inf;

-      inf = find_inferior_pid (ptid_get_pid (ptid));
+      inf = find_inferior_ptid (ptid);
       gdb_assert (inf != NULL);
       set_current_program_space (inf->pspace);
       set_current_inferior (inf);
@@ -1290,7 +1290,7 @@ do_restore_current_thread_cleanup (void *arg)
      then don't revert back to it, but instead simply drop back to no
      thread selected.  */
   if (tp
-      && find_inferior_pid (ptid_get_pid (tp->ptid)) != NULL)
+      && find_inferior_ptid (tp->ptid) != NULL)
     restore_current_thread (old->inferior_ptid);
   else
     {
-- 
2.1.3