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

[Pedro Alves <pedro@palves.net>]

Hi Kevin,

On 2024-10-31 03:04, Kevin Buettner wrote:
> On Wed, 23 Oct 2024 21:10:53 +0100
> Pedro Alves <pedro@palves.net> wrote:

>> 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).
> 

Thanks!

>> 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.

The '*' and '+' in the same column compact form was really confusing to me
when I tried using this in an earlier version.  I really think it makes
sense to keep it in a separate column.  

We shouldn't put "(active)" in the "frame" column (I mean, the column that is equivalent
to the "Frame" column in "info threads"), as an active fork has a real frame to print,
of course.

Putting "(active)" where "A" is being put looks like too much wasted space to me:

*  1.0 (active)  process 439463 at 0x7ffff7ce578a, file ../sysdeps/unix/sysv/linux/clock_nanosleep.c, line 78
   1.1           process 439827 at 0x7ffff7ce578a, file ../sysdeps/unix/sysv/linux/clock_nanosleep.c, line 78
   2.0           process 439463 at 0x7ffff7ce578a, file ../sysdeps/unix/sysv/linux/clock_nanosleep.c, line 78
   2.1 (active)  process 439827 at 0x7ffff7ce578a, file ../sysdeps/unix/sysv/linux/clock_nanosleep.c, line 78

In downstream rocgdb (for AMDGPU), we have a new "info lanes" command with a "State" column, and one of the
states is A (for active), which is I guess why "A" and state column looked so obvious to me.  E.g.:

(gdb) info lanes 
  Id   State Target Id                              Frame 
* 0    A     AMDGPU Lane 1:1:1:1/0 (0,0,0)[0,0,0]   bit_extract_kernel (C_d=0x7fffe3a00000, A_d=0x7fffe8400000, N=1000000)
    at /home/pedro/rocm/bit_extract/bit_extract.cpp:62
  1    A     AMDGPU Lane 1:1:1:1/1 (0,0,0)[1,0,0]   bit_extract_kernel (C_d=0x7fffe3a00000, A_d=0x7fffe8400000, N=1000000)
    at /home/pedro/rocm/bit_extract/bit_extract.cpp:62
...


> 
> But perhaps this is needless bike-shedding.  If you really want it to
> be "(running)", I'll change it.  

Well, you have to change _something_.  If the fork is running, then there is no frame to print, so
if you don't print (running), what else could you print, other than leaving it blank, like:

*  1.0 A   process 439463 
   1.1     process 439827 at 0x7ffff7ce578a, file ../sysdeps/unix/sysv/linux/clock_nanosleep.c, line 78

I now think that:

*  1.0 A  process 439463 (running)
   1.1    process 439827 at 0x7ffff7ce578a, file ../sysdeps/unix/sysv/linux/clock_nanosleep.c, line 78

... is better than:

*  1.0 AR process 439463
   1.1    process 439827 at 0x7ffff7ce578a, file ../sysdeps/unix/sysv/linux/clock_nanosleep.c, line 78

... because:

 #1 - it's what you get with "info threads" as well, and consistency is good.
 #2 - running vs not-running is not really a property of the fork wrt to checkpointing, unlike active/not-active.

> But do give some thought about whether the state character "A" still makes sense.

Yeah, if there aren't other obvious states, we could also consider other representations.
Like, make it a yes/no column?  This only works nicely if we add a table header row, but we
should do that anyhow.  E.g.:

   Id  Active  Target Id       Frame 
*  1.0 y       process 439463  at 0x7ffff7ce578a, file ../sysdeps/unix/sysv/linux/clock_nanosleep.c, line 78
   1.1 n       process 439827  at 0x7ffff7ce578a, file ../sysdeps/unix/sysv/linux/clock_nanosleep.c, line 78
   2.0 n       process 484476  at 0x7ffff7ce578a, file ../sysdeps/unix/sysv/linux/clock_nanosleep.c, line 78
   2.1 y       process 454312  at 0x7ffff7ce578a, file ../sysdeps/unix/sysv/linux/clock_nanosleep.c, line 78

This is probably the most gdb-natural output?

It's also useful/interesting to consider how all these different options would be exposed to MI.
(Why that was never done, I don't know.  I find it strange, especially since we know of frontends
like ARM DDT which use checkpoints.  But I think they relied on annotations instead of MI.  But I
hope they moved along by now.)
Representing the "active" state in MI in the same "column" as the selected thread more obviously
doesn't work there, as the "selected" item is not normally a property of a row.  E.g., for threads
listing, we get a current-thread-id="1" attribute, separate from the thread list:

 (gdb) interpreter-exec mi "-thread-info"
 ^done,threads=[{id="1",target-id="Thread 0xf7fbf600 (LWP 255870)",name="step",frame={level="0",addr="0x08048385",func="main",args=[],file="step.c",fullname="/home/pedro/pedro/gdb/tests/step.c",line="27",arch="i386"},state="stopped",core="2"}],current-thread-id="1"

An MI "-checkpoints-info" command would most probably want to do the same, and add an active=y/n attribute.

Pedro Alves