Patchwork [1/4] Make "checkpoint" not rely on inferior_ptid

login
register
mail settings
Submitter Pedro Alves
Date March 5, 2019, 6:43 p.m.
Message ID <20190305184340.26768-2-palves@redhat.com>
Download mbox | patch
Permalink /patch/31721/
State New
Headers show

Comments

Pedro Alves - March 5, 2019, 6:43 p.m.
Don't rely on "inferior_ptid" deep within add_fork.  In the
multi-target branch, I'm forcing inferior_ptid to null_ptid early in
infrun event handling to make sure we inadvertently rely on the
current thread/target when we shouldn't, and that caught some bad
or unnecessary assumptions throughout.

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

	* linux-fork.c (new_fork): New, split out of ...
	(add_fork): ... this.  Return void.  Move "first fork" special
	case from here, to ...
	(checkpoint_command): ... here.
	* linux-linux.h (add_fork): Return void.
---
 gdb/linux-fork.c | 44 ++++++++++++++++++++++++++++----------------
 gdb/linux-fork.h |  2 +-
 2 files changed, 29 insertions(+), 17 deletions(-)
Tom Tromey - March 5, 2019, 7:32 p.m.
>>>>> "Pedro" == Pedro Alves <palves@redhat.com> writes:

Pedro> In the
Pedro> multi-target branch, I'm forcing inferior_ptid to null_ptid early in
Pedro> infrun event handling to make sure we inadvertently rely on the
Pedro> current thread/target when we shouldn't, and that caught some bad
Pedro> or unnecessary assumptions throughout.

This read a bit strangely to me.  Maybe instead write "to make sure we
don't inadvertently rely on the current thread/target,..."?

The patch itself seemed fine to me.  Big +1 to reducing the impact of
globals.

Tom
Pedro Alves - March 5, 2019, 7:46 p.m.
On 03/05/2019 07:32 PM, Tom Tromey wrote:
>>>>>> "Pedro" == Pedro Alves <palves@redhat.com> writes:
> 
> Pedro> In the
> Pedro> multi-target branch, I'm forcing inferior_ptid to null_ptid early in
> Pedro> infrun event handling to make sure we inadvertently rely on the
> Pedro> current thread/target when we shouldn't, and that caught some bad
> Pedro> or unnecessary assumptions throughout.
> 
> This read a bit strangely to me.  Maybe instead write "to make sure we
> don't inadvertently rely on the current thread/target,..."?
> 

Oh, yes, I missed typing the "don't".  Thanks for spotting that.

> The patch itself seemed fine to me.  Big +1 to reducing the impact of
> globals.
Thanks,
Pedro Alves

Patch

diff --git a/gdb/linux-fork.c b/gdb/linux-fork.c
index b1b390c5c6..a748deaa3a 100644
--- a/gdb/linux-fork.c
+++ b/gdb/linux-fork.c
@@ -76,29 +76,30 @@  find_last_fork (void)
   return last;
 }
 
-/* Add a fork to the internal fork list.  */
+/* Allocate a new fork.  */
 
-struct fork_info *
-add_fork (pid_t pid)
+static struct fork_info *
+new_fork (pid_t pid)
 {
   struct fork_info *fp;
 
-  if (fork_list == NULL && pid != inferior_ptid.pid ())
-    {
-      /* Special case -- if this is the first fork in the list
-	 (the list is hitherto empty), and if this new fork is
-	 NOT the current inferior_ptid, then add inferior_ptid
-	 first, as a special zeroeth fork id.  */
-      highest_fork_num = -1;
-      add_fork (inferior_ptid.pid ());	/* safe recursion */
-    }
-
   fp = XCNEW (struct fork_info);
   fp->ptid = ptid_t (pid, pid, 0);
-  fp->num = ++highest_fork_num;
+  return fp;
+}
+
+/* Add a new fork to the internal fork list.  */
+
+void
+add_fork (pid_t pid)
+{
+  struct fork_info *fp = new_fork (pid);
 
   if (fork_list == NULL)
-    fork_list = fp;
+    {
+      fork_list = fp;
+      highest_fork_num = 0;
+    }
   else
     {
       struct fork_info *last = find_last_fork ();
@@ -106,7 +107,7 @@  add_fork (pid_t pid)
       last->next = fp;
     }
 
-  return fp;
+  fp->num = ++highest_fork_num;
 }
 
 static void
@@ -760,6 +761,17 @@  checkpoint_command (const char *args, int from_tty)
 
   if (!fp)
     error (_("Failed to find new fork"));
+
+  if (fork_list->next == NULL)
+    {
+      /* Special case -- if this is the first fork in the list (the
+	 list was hitherto empty), then add inferior_ptid first, as a
+	 special zeroeth fork id.  */
+      fork_info *first = new_fork (inferior_ptid.pid ());
+      first->next = fork_list;
+      fork_list = first;
+    }
+
   fork_save_infrun_state (fp, 1);
   fp->parent_ptid = last_target_ptid;
 }
diff --git a/gdb/linux-fork.h b/gdb/linux-fork.h
index e6f130438c..f918bcb3d9 100644
--- a/gdb/linux-fork.h
+++ b/gdb/linux-fork.h
@@ -21,7 +21,7 @@ 
 #define LINUX_FORK_H
 
 struct fork_info;
-extern struct fork_info *add_fork (pid_t);
+extern void add_fork (pid_t);
 extern struct fork_info *find_fork_pid (pid_t);
 extern void linux_fork_killall (void);
 extern void linux_fork_mourn_inferior (void);