[PATCH 0/3] [gdb] Two checkpoint fixes

[PATCH 0/3] [gdb] Two checkpoint fixes

[Tom de Vries]

While investigating PR31203 "[gdb] FAIL: gdb.base/kill-during-detach.exp: exit_p=true: checkpoint_p=true: python kill_and_detach()" I ran into two checkpoint issues.

I filed these as:
- PR31211 "[gdb] info checkpoints show incorrect info"
- PR31209 "[gdb, delete checkpoint] inferior.c:406: internal-error: find_inferior_pid: Assertion `pid != 0' failed"

This patch series contains fixes for both.

While working on PR31211, I noticed a simplification opportunity in
info_checkpoints_command, so the series starts out with this simplification.

Tested on x86_64-linux.

Tom de Vries (3):
  [gdb] Make variable printed bool in info_checkpoints_command
  [gdb] Fix info checkpoints
  [gdb] Fix assertion failure for checkpoint delete 0

 gdb/linux-fork.c                      | 33 +++++++++++++++++++++++----
 gdb/testsuite/gdb.base/checkpoint.exp | 22 +++++++++++++++---
 2 files changed, 47 insertions(+), 8 deletions(-)


base-commit: 53ba8c111774d318fa572669ba4d3b76121e102b
-- 
2.35.3

[PATCH 1/3] [gdb] Make variable printed bool in info_checkpoints_command

[Tom de Vries]

While reading info_checkpoints_command, I noticed variable printed:
...
  const fork_info *printed = NULL;
  ...
  for (const fork_info &fi : fork_list)
    {
      if (requested > 0 && fi.num != requested)
	continue;

      printed = &fi;
      ...
    }
  if (printed == NULL)
...
has pointer type, but is just used as bool.

Make this explicit by changing the variable type to bool.

Tested on x86_64-linux.
---
 gdb/linux-fork.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/gdb/linux-fork.c b/gdb/linux-fork.c
index 1430ff89fa7..177a012ec08 100644
--- a/gdb/linux-fork.c
+++ b/gdb/linux-fork.c
@@ -583,7 +583,7 @@ info_checkpoints_command (const char *arg, int from_tty)
 {
   struct gdbarch *gdbarch = get_current_arch ();
   int requested = -1;
-  const fork_info *printed = NULL;
+  bool printed = false;
 
   if (arg && *arg)
     requested = (int) parse_and_eval_long (arg);
@@ -592,8 +592,8 @@ info_checkpoints_command (const char *arg, int from_tty)
     {
       if (requested > 0 && fi.num != requested)
 	continue;
+      printed = true;
 
-      printed = &fi;
       if (fi.ptid == inferior_ptid)
 	gdb_printf ("* ");
       else
@@ -623,7 +623,8 @@ info_checkpoints_command (const char *arg, int from_tty)
 
       gdb_putc ('\n');
     }
-  if (printed == NULL)
+
+  if (!printed)
     {
       if (requested > 0)
 	gdb_printf (_("No checkpoint number %d.\n"), requested);
-- 
2.35.3

[PATCH 2/3] [gdb] Fix info checkpoints

[Tom de Vries]

Consider test-case gdb.base/checkpoint.exp.  At some point, it issues an info
checkpoints command:
...
(gdb) info checkpoints^M
* 0 process 30570 (main process) at 0x0^M
  1 process 30573 at 0x4008bb, file checkpoint.c, line 49^M
  2 process 30574 at 0x4008bb, file checkpoint.c, line 49^M
  3 process 30575 at 0x4008bb, file checkpoint.c, line 49^M
  4 process 30576 at 0x4008bb, file checkpoint.c, line 49^M
  5 process 30577 at 0x4008bb, file checkpoint.c, line 49^M
  6 process 30578 at 0x4008bb, file checkpoint.c, line 49^M
  7 process 30579 at 0x4008bb, file checkpoint.c, line 49^M
  8 process 30580 at 0x4008bb, file checkpoint.c, line 49^M
  9 process 30582 at 0x4008bb, file checkpoint.c, line 49^M
  10 process 30583 at 0x4008bb, file checkpoint.c, line 49^M
...

According to the docs, each of these (0-10) is a checkpoint.

But the pc address (as well as the file name and line number) is missing for
checkpoint 0.

Fix this by sampling the pc value for the current process in
info_checkpoints_command, such that we have instead:
...
* 0 process 30570 (main process) at 0x4008bb, file checkpoint.c, line 49^M
...

Tested on x86_64-linux.

PR gdb/31211
Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=31211
---
 gdb/linux-fork.c                      | 15 +++++++++++++--
 gdb/testsuite/gdb.base/checkpoint.exp |  8 +++++---
 2 files changed, 18 insertions(+), 5 deletions(-)

diff --git a/gdb/linux-fork.c b/gdb/linux-fork.c
index 177a012ec08..659264ab712 100644
--- a/gdb/linux-fork.c
+++ b/gdb/linux-fork.c
@@ -594,16 +594,27 @@ info_checkpoints_command (const char *arg, int from_tty)
 	continue;
       printed = true;
 
-      if (fi.ptid == inferior_ptid)
+      bool is_current = fi.ptid == inferior_ptid;
+      if (is_current)
 	gdb_printf ("* ");
       else
 	gdb_printf ("  ");
 
-      ULONGEST pc = fi.pc;
       gdb_printf ("%d %s", fi.num, target_pid_to_str (fi.ptid).c_str ());
       if (fi.num == 0)
 	gdb_printf (_(" (main process)"));
+
+      if (is_current && inferior_thread ()->state == THREAD_RUNNING)
+	{
+	  gdb_printf (_(" <running>\n"));
+	  continue;
+	}
+
       gdb_printf (_(" at "));
+      ULONGEST pc
+	= (is_current
+	   ? regcache_read_pc (get_thread_regcache (inferior_thread ()))
+	   : fi.pc);
       gdb_puts (paddress (gdbarch, pc));
 
       symtab_and_line sal = find_pc_line (pc, 0);
diff --git a/gdb/testsuite/gdb.base/checkpoint.exp b/gdb/testsuite/gdb.base/checkpoint.exp
index a4ea94354b7..9ba5b3e82ec 100644
--- a/gdb/testsuite/gdb.base/checkpoint.exp
+++ b/gdb/testsuite/gdb.base/checkpoint.exp
@@ -84,9 +84,11 @@ gdb_test "checkpoint" "" "checkpoint nine"
 gdb_test "continue 10" "breakpoint 1.*" "break1 ten"
 gdb_test "checkpoint" "" "checkpoint ten"
 
-gdb_test "info checkpoints" \
-    " 1 .* 2 .* 3 .* 4 .* 5 .* 6 .* 7 .* 8 .* 9 .* 10 .*" \
-    "info checkpoints one"
+set info_checkpoints_re ""
+for { set i 0 } { $i <= 10 } { incr i } {
+    append info_checkpoints_re " $i .* file .*"
+}
+gdb_test "info checkpoints" $info_checkpoints_re "info checkpoints one"
 
 delete_breakpoints
 gdb_breakpoint $break2_loc
-- 
2.35.3

[PATCH 3/3] [gdb] Fix assertion failure for checkpoint delete 0

[Tom de Vries]

When doing "checkpoint delete 0" we run into an assertion failure:
...
+delete checkpoint 0
inferior.c:406: internal-error: find_inferior_pid: Assertion `pid != 0' failed.
...

Fix this by handling the "pptid == null_ptid" case in
delete_checkpoint_command.

Tested on x86_64-linux.

PR gdb/31209
Bug: https://sourceware.org/bugzilla/show_bug.cgi?id
---
 gdb/linux-fork.c                      | 11 +++++++++++
 gdb/testsuite/gdb.base/checkpoint.exp | 14 ++++++++++++++
 2 files changed, 25 insertions(+)

diff --git a/gdb/linux-fork.c b/gdb/linux-fork.c
index 659264ab712..64b83e79204 100644
--- a/gdb/linux-fork.c
+++ b/gdb/linux-fork.c
@@ -537,6 +537,17 @@ Please switch to another checkpoint before deleting the current one"));
 
   delete_fork (ptid);
 
+  if (pptid == null_ptid)
+    {
+      int status;
+      /* Wait to collect the inferior's exit status.  Do not check whether
+	 this succeeds though, since we may be dealing with a process that we
+	 attached to.  Such a process will only report its exit status to its
+	 original parent.  */
+      waitpid (ptid.pid (), &status, 0);
+      return;
+    }
+
   /* If fi->parent_ptid is not a part of lwp but it's a part of checkpoint
      list, waitpid the ptid.
      If fi->parent_ptid is a part of lwp and it is stopped, waitpid the
diff --git a/gdb/testsuite/gdb.base/checkpoint.exp b/gdb/testsuite/gdb.base/checkpoint.exp
index 9ba5b3e82ec..d318fc785b8 100644
--- a/gdb/testsuite/gdb.base/checkpoint.exp
+++ b/gdb/testsuite/gdb.base/checkpoint.exp
@@ -336,3 +336,17 @@ verbose "Timeout now $timeout sec."
 #
 # Finished: cleanup
 #
+
+#
+# Now let's try to delete checkpoint 0.
+#
+
+with_test_prefix "delete checkpoint 0" {
+    clean_restart $binfile
+    runto_main
+
+    gdb_test "checkpoint" "checkpoint 1: fork returned pid $decimal\\."
+    gdb_test "restart 1" "Switching to process $decimal\r\n.*"
+    gdb_test "delete checkpoint 0" "Killed process $decimal"
+    gdb_test "info checkpoints" [string_to_regexp "No checkpoints."]
+}
-- 
2.35.3

Re: [PATCH 1/3] [gdb] Make variable printed bool in info_checkpoints_command

[Kevin Buettner]

On Mon,  8 Jan 2024 16:25:51 +0100
Tom de Vries <tdevries@suse.de> wrote:

> While reading info_checkpoints_command, I noticed variable printed:
> ...
>   const fork_info *printed = NULL;
>   ...
>   for (const fork_info &fi : fork_list)
>     {
>       if (requested > 0 && fi.num != requested)
> 	continue;
> 
>       printed = &fi;
>       ...
>     }
>   if (printed == NULL)
> ...
> has pointer type, but is just used as bool.
> 
> Make this explicit by changing the variable type to bool.
> 
> Tested on x86_64-linux.

LGTM.

Approved-By: Kevin Buettner <kevinb@redhat.com>

Re: [PATCH 2/3] [gdb] Fix info checkpoints

[Kevin Buettner]

On Mon,  8 Jan 2024 16:25:52 +0100
Tom de Vries <tdevries@suse.de> wrote:

> Consider test-case gdb.base/checkpoint.exp.  At some point, it issues an info
> checkpoints command:
> ...
> (gdb) info checkpoints^M
> * 0 process 30570 (main process) at 0x0^M
>   1 process 30573 at 0x4008bb, file checkpoint.c, line 49^M
>   2 process 30574 at 0x4008bb, file checkpoint.c, line 49^M
>   3 process 30575 at 0x4008bb, file checkpoint.c, line 49^M
>   4 process 30576 at 0x4008bb, file checkpoint.c, line 49^M
>   5 process 30577 at 0x4008bb, file checkpoint.c, line 49^M
>   6 process 30578 at 0x4008bb, file checkpoint.c, line 49^M
>   7 process 30579 at 0x4008bb, file checkpoint.c, line 49^M
>   8 process 30580 at 0x4008bb, file checkpoint.c, line 49^M
>   9 process 30582 at 0x4008bb, file checkpoint.c, line 49^M
>   10 process 30583 at 0x4008bb, file checkpoint.c, line 49^M
> ...
> 
> According to the docs, each of these (0-10) is a checkpoint.
> 
> But the pc address (as well as the file name and line number) is missing for
> checkpoint 0.
> 
> Fix this by sampling the pc value for the current process in
> info_checkpoints_command, such that we have instead:
> ...
> * 0 process 30570 (main process) at 0x4008bb, file checkpoint.c, line 49^M
> ...
> 
> Tested on x86_64-linux.
> 
> PR gdb/31211
> Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=31211

Approved-By: Kevin Buettner <kevinb@redhat.com>

Re: [PATCH 3/3] [gdb] Fix assertion failure for checkpoint delete 0

[Kevin Buettner]

On Mon,  8 Jan 2024 16:25:53 +0100
Tom de Vries <tdevries@suse.de> wrote:

> When doing "checkpoint delete 0" we run into an assertion failure:
> ...
> +delete checkpoint 0
> inferior.c:406: internal-error: find_inferior_pid: Assertion `pid != 0' failed.
> ...
> 
> Fix this by handling the "pptid == null_ptid" case in
> delete_checkpoint_command.
> 
> Tested on x86_64-linux.
> 
> PR gdb/31209
> Bug: https://sourceware.org/bugzilla/show_bug.cgi?id

The Bug URL above is missing an '=31209' at the end.

Otherwise, LGTM.

Approved-By: Kevin Buettner <kevinb@redhat.com>

Kevin

Re: [PATCH 3/3] [gdb] Fix assertion failure for checkpoint delete 0

[Tom de Vries]

On 1/10/24 06:28, Kevin Buettner wrote:
> On Mon,  8 Jan 2024 16:25:53 +0100
> Tom de Vries <tdevries@suse.de> wrote:
> 
>> When doing "checkpoint delete 0" we run into an assertion failure:
>> ...
>> +delete checkpoint 0
>> inferior.c:406: internal-error: find_inferior_pid: Assertion `pid != 0' failed.
>> ...
>>
>> Fix this by handling the "pptid == null_ptid" case in
>> delete_checkpoint_command.
>>
>> Tested on x86_64-linux.
>>
>> PR gdb/31209
>> Bug: https://sourceware.org/bugzilla/show_bug.cgi?id
> 
> The Bug URL above is missing an '=31209' at the end.
> 
> Otherwise, LGTM.
> 
> Approved-By: Kevin Buettner <kevinb@redhat.com>

Thanks for the reviews.

The linaro CI found:
...
(gdb) PASS: gdb.base/checkpoint-ns.exp: delete checkpoint 0: checkpoint
restart 1
Switching to Thread 0xfffff7ff7e60 (LWP 146669)
#0  main () at 
/home/tcwg-build/workspace/tcwg_gnu_0/abe/snapshots/gdb.git~master/gdb/testsuite/gdb.base/checkpoint.c:28
28	  char *tmp = &linebuf[0];
(gdb) FAIL: gdb.base/checkpoint-ns.exp: delete checkpoint 0: restart 1
...
so I'll commit shortly with this minor update:
...
-    gdb_test "restart 1" "Switching to process $decimal\r\n.*"
+    gdb_test "restart 1" "Switching to .*"
...

Thanks,
- Tom