Re: [PATCH v4 1/4] Make linux checkpoints work with multiple inferiors

[Kevin Buettner <kevinb@redhat.com>]

On Wed, 23 Oct 2024 21:10:53 +0100
Pedro Alves <pedro@palves.net> wrote:

> Trying v4 before looking at the code, I saw one behavior that
> surprised me.  That is when you have two inferiors, and checkpoints
> in only one inferior.  Like so:
> 
>  (gdb) info checkpoints 
>  No checkpoints.
>  (gdb) checkpoint 
>  Checkpoint 1: fork returned pid 66326.
>  (gdb) info checkpoints 
>  *  0 A  process 66173 at 0x555555555258, file threads.c, line 53
>     1    process 66326 at 0x555555555258, file threads.c, line 53
>  (gdb) add-inferior 
>  [New inferior 2]
>  Added inferior 2 on connection 1 (native)
> 
> Now in the following command, I was expecting to see 1.0 and 1.1
> as checkpoint numbers, since we now have two inferiors, but we only
> get the 0 and 1:
> 
>  (gdb) info checkpoints 
>  *  0 A  process 66173 at 0x555555555258, file threads.c, line 53
>     1    process 66326 at 0x555555555258, file threads.c, line 53
> 
> 
> If I switch to inferior 2, and then list checkpoints, I do get
> the fully-qualified checkpoint ids:
> 
>  (gdb) info inferiors 
>    Num  Description       Connection           Executable        
>  * 1    process 66173     1 (native)           /net/cascais.nfs/brno/pedro/gdb/tests/threads 
>    2    <null>            1 (native)                             
>  (gdb) inferior 2
>  [Switching to inferior 2 [<null>] (<noexec>)]
>  (gdb) info checkpoints 
>     1.0 A  process 66173 at 0x55555258, file threads.c, line 53
>     1.1    process 66326 at 0x55555258, file threads.c, line 53
> 
> I was expecting to see the same output when inferior 1 is the
> one selected.  Again, going back to inferior 1:
> 
>  (gdb) inferior 1
>  [Switching to inferior 1 [process 66173] (/net/cascais.nfs/brno/pedro/gdb/tests/threads)]
>  [Switching to thread 1.1 (Thread 0x7ffff7f93740 (LWP 66173))]
>  #0  main () at threads.c:53
>  53      int main() {
>  (gdb) info checkpoints 
>  *  0 A  process 66173 at 0x555555555258, file threads.c, line 53
>     1    process 66326 at 0x555555555258, file threads.c, line 53
>  (gdb) 
> 
> Now that I look at the code, I see that the predicate used to
> determine whether to show the inferior number is checking if
> there are multiple inferiors with checkpoints, so it seems
> intentional.  But FYI, coming at this with a lot of context
> from previous discussion swapped out, I found it confusing.

As you say, it was intentional, but it's easy enough to change it
to always print fully-qualified ids when there are multiple inferiors.
I'll make that change (and will update the test case).

> On the "R" state, thinking some more (since our last discussion),
> I wonder if we really need it.  In "info threads", we show that
> the thread is running in the "frame" column, like:
> 
>  (gdb) info threads 
>    Id   Target Id                                    Frame 
>  * 1    Thread 0x7ffff7f8e740 (LWP 439463) "infloop" (running)
>                                                      ^^^^^^^^^
> 
> Maybe we should just reuse the frame-printing code from "info threads".

The current linux-fork.c code prints <running>.  In the v1 series, I
had used '*' and '+' as the first character to indicate the active
forks, with '*' also indicating the active inferior.  You recommended
that I instead use a state character "A" and dispense with the '+'
indicators.  Well, since we have one state character, why not another,
hence "R".  (It makes the output more compact.) If we change "R" to
"(running)", the state character "A" seems a little odd to me; why not
say "(active)" or, instead, use a compact method of indicating active
forks as I did in my v1 patch.

But perhaps this is needless bike-shedding.  If you really want it to
be "(running)", I'll change it.  But do give some thought about whether
the state character "A" still makes sense.

> I tried to reproduce the "R" state here locally, and saw that
> it's broken, for trying to fetch registers of a running thread:
> 
>  (gdb) start
>  ...
>  (gdb) checkpoint
>  ...
>  (gdb) info checkpoints 
>  *  0 A  process 439463 at 0x7ffff7ce578a, file ../sysdeps/unix/sysv/linux/clock_nanosleep.c, line 78
>     1    process 439827 at 0x7ffff7ce578a, file ../sysdeps/unix/sysv/linux/clock_nanosleep.c, line 78
>  (gdb) c&
>  Continuing.
>  (gdb) info checkpoints 
>  *  0 AR process 439463 at Couldn't get registers: No such process.
> 
> 
> BTW, the documentation patch says that you can only see the "R" state
> in non-stop mode:
> 
>  +one which @value{GDBN} is currently debugging.  @code{R} indicates a
>  +running process; this status letter can only appear when @value{GDBN}
>  +is running in non-stop mode.  
> 
> But that's not correct.  You can also use the "c&" command in
> all-stop mode.
> 
> I noticed this because I wanted to try what it say here:
> 
>                                If neither @code{A} or @code{R} are present,
>  +this indicates a checkpoint which can be switched to using the
>  +@code{restart} command or deleted using the @code{delete checkpoint}
>  +command.
> 
> ... specifically, I wanted to try switching to a checkpoint in the "R"
> state, check that GDB behaves properly.

I'll look into the various problems that you mention above and will
also look at adding some more tests.  (I'll fix the nits for the v5
series too.)

Kevin