[1/4] Make "checkpoint" not rely on inferior_ptid
Commit Message
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(-)
Comments
>>>>> "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
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
@@ -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;
}
@@ -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);