Re: [PATCH 2/2 ] PowerPC: fix for gdb.reverse/finish-precsave.exp and gdb.reverse/finish-reverse.exp

[Bruno Larsen <blarsen@redhat.com>]

On 28/03/2023 17:38, Simon Marchi wrote:
> On 3/28/23 11:17, Carl Love wrote:
>> Simon:
>>
>> On Mon, 2023-03-27 at 21:19 -0400, Simon Marchi wrote:
>>> To confirm the theory, I added a:
>>>
>>>     gdb_test "set range-stepping off"
>>>
>>> somewhere near the beginning of your test, and it makes the test pass
>>> with native-gdbserver.
>>>
>>> If we want recording to work the same with process targets using
>>> range-stepping and those that don't, maybe GDB should avoid using
>>> range-stepping when the record-full target is in effect?
>> We could just update the test to include gdb_test "set range-stepping
>> off" with a brief explanation of the issue. This would have a minimal
>> impact on performance of this test and no performance degradation for
>> any other tests.  It sounds like this would fix the issue for just this
>> test.
>>
>> It sounds like disabling range-stepping when record-full is enabled
>> would be the more general fix for the issue.  Not sure if there are
>> other tests where this issue occurs.  Doing the more general fix would
>> probably have some performance impact on the other tests that need to
>> use record-full. I can't really say how much of an impact it would be.
> It depends on what the goal is.  In general, we try to minimize the
> differences in behavior when debugging native vs debugging remote.  So,
> if we say that it's a bug that the record-full target doesn't see all
> the intermediary steps, then putting "set range-stepping off" in that
> test would just be papering over the bug.  I think the correct thing to
> do would be to fix GDB.  And yes there will be a performance impact when
> using remote debugging + record-full, but if that's what's needed to get
> correct behavior...
>
> Some ideas to implement this:
>
>   - Add a target method supports_range_stepping, record-full would
>     implement it and return false.  The default would be true.
>   - record-full's resume method could clean the resumed thread's
>     may_range_step flag.
>
> I'm open to other ideas.  Note that we don't want to disable range
> stepping for other record implementations (only btrace, currently), I
> don't think that one is affected by the problem (the hardware should
> record all intermediary steps in any case).
>
> Simon
>
Sorry for possibly necromancing this thread, but I decided to look into 
gdb.reverse failures when testing with clang and the same issue occurs 
as with gdbserver. I decided to take a look and re-read old messages and 
what Pedro actually said in that thread is a bit confusing, because 
there were multiple intertwined issues being discussed. Looking at this 
email 
(https://sourceware.org/pipermail/gdb-patches/2023-January/196130.html) 
he does mention that GDB should not stop in the same line when a 
reverse-step or reverse-next is used. Because of that, I believe that 
the behavior that the test expects is actually incorrect, and we should 
try and fix this.

Looking at the state of the program when it is compiled with GCC we get:

➜  gdb ./gdb -q 
testsuite/outputs/gdb.reverse/finish-reverse-next/finish-reverse-next 
-ex start -ex record
Reading symbols from 
testsuite/outputs/gdb.reverse/finish-reverse-next/finish-reverse-next...
Temporary breakpoint 1 at 0x401176: file 
/home/blarsen/Documents/fsf_build/gdb/testsuite/../../../binutils-gdb/gdb/testsuite/gdb.revers3/finish-reverse-next.c, 
line 76.
Starting program: 
/home/blarsen/Documents/fsf_build/gdb/testsuite/outputs/gdb.reverse/finish-reverse-next/finish-reverse-next
[Thread debugging using libthread_db enabled]
Using host libthread_db library "/lib64/libthread_db.so.1".

Temporary breakpoint 1, main (argc=1, argv=0x7fffffffde78)
     at 
/home/blarsen/Documents/fsf_build/gdb/testsuite/../../../binutils-gdb/gdb/testsuite/gdb.reverse/finish-reverse-next.c:76
76        int (*funp) (int, int) = &function1;
(gdb) until 83
main (argc=1, argv=0x7fffffffde78)
     at 
/home/blarsen/Documents/fsf_build/gdb/testsuite/../../../binutils-gdb/gdb/testsuite/gdb.reverse/finish-reverse-next.c:83
83        function1 (a, b);   // CALL VIA LEP
(gdb) n
86        a = 10;
(gdb) rs
function1 (a=1, b=5)
     at 
/home/blarsen/Documents/fsf_build/gdb/testsuite/../../../binutils-gdb/gdb/testsuite/gdb.reverse/finish-reverse-next.c:70
70      }
(gdb) reverse-finish
Run back to call of #0  function1 (a=1, b=5)
     at 
/home/blarsen/Documents/fsf_build/gdb/testsuite/../../../binutils-gdb/gdb/testsuite/gdb.reverse/finish-reverse-next.c:70
0x0000000000401196 in main (argc=1, argv=0x7fffffffde78)
     at 
/home/blarsen/Documents/fsf_build/gdb/testsuite/../../../binutils-gdb/gdb/testsuite/gdb.reverse/finish-reverse-next.c:83
83        function1 (a, b);   // CALL VIA LEP
(gdb) maint info line-table
             (snip)
15     81     0x0000000000401185 0x0000000000401185 Y
16     83     0x000000000040118c 0x000000000040118c Y
17     86     0x000000000040119b 0x000000000040119b Y
(gdb) disas /s
Dump of assembler code for function main:
       (snip)
83        function1 (a, b);   // CALL VIA LEP
    0x000000000040118c <+37>:    mov    -0x10(%rbp),%edx
    0x000000000040118f <+40>:    mov    -0xc(%rbp),%eax
    0x0000000000401192 <+43>:    mov    %edx,%esi
    0x0000000000401194 <+45>:    mov    %eax,%edi
=> 0x0000000000401196 <+47>:    call   0x40112c <function1>

We can see that we have stopped at the right instruction, but it isn't 
mapped to any line number directly. If we reverse-next from there:

82
83        function1 (a, b);   // CALL VIA LEP
=> 0x000000000040118c <+37>:    mov    -0x10(%rbp),%edx
    0x000000000040118f <+40>:    mov    -0xc(%rbp),%eax
    0x0000000000401192 <+43>:    mov    %edx,%esi
    0x0000000000401194 <+45>:    mov    %eax,%edi
    0x0000000000401196 <+47>:    call   0x40112c <function1>

We at at an address that _is_ mapped in the line table. So my guess is 
that the code setting up the reverse-next or reverse-step is failing to 
figure out our current line (83) so cant properly setup a stepping 
range, GDB would reverse step a single instruction, but since that 
leaves us in a place that is not marked IS_STMT, GDB will continue to 
step until we hit an IS_STMT location, and we end up stopping at the 
same line twice.

For completeness sake, here's what the Clang session looks like:
(gdb) reverse-finish
Run back to call of #0  0x000000000040117b in function1 (a=1, b=5) at 
/home/blarsen/Documents/fsf_build/gdb/testsuite/../../../binutils-gdb/gdb/testsuite/gdb.reverse/finish-reverse-next.c:69
0x00000000004011c8 in main (argc=1, argv=0x7fffffffde78) at 
/home/blarsen/Documents/fsf_build/gdb/testsuite/../../../binutils-gdb/gdb/testsuite/gdb.reverse/finish-reverse-next.c:83
83        function1 (a, b);   // CALL VIA LEP
(gdb) maint info line-table
          (snip)
22     80     0x00000000004011b4 0x00000000004011b4 Y
23     81     0x00000000004011bb 0x00000000004011bb Y
24     83     0x00000000004011c2 0x00000000004011c2 Y
25     83     0x00000000004011c5 0x00000000004011c5
26     83     0x00000000004011c8 0x00000000004011c8
27     86     0x00000000004011cd 0x00000000004011cd Y
(gdb) disas /s
          (snip)
81        b = 5;
    0x00000000004011bb <+43>:    movl   $0x5,-0x18(%rbp)

82
83        function1 (a, b);   // CALL VIA LEP
    0x00000000004011c2 <+50>:    mov    -0x14(%rbp),%edi
    0x00000000004011c5 <+53>:    mov    -0x18(%rbp),%esi
=> 0x00000000004011c8 <+56>:    call   0x401140 <function1>
(gdb) rn
81        b = 5;

Basically, the best way to fix this solution is to get reverse-next and 
reverse-step to properly figure out the stepping range and not have a 
reverse-next that ends up in the same line

-- 
Cheers,
Bruno