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

[Pedro Alves <pedro@palves.net>]

On 2023-01-25 2:11 p.m., Ulrich Weigand wrote:
> Pedro Alves <pedro@palves.net> wrote:
> 
>>> 11        func1 (argc); func2 (argc);
>>>    0x000055555555515f <+15>:    mov    -0x4(%rbp),%edi
>>>    0x0000555555555162 <+18>:    call   0x555555555130 <func1>
>>>    0x0000555555555167 <+23>:    mov    -0x4(%rbp),%edi
>>> => 0x000055555555516a <+26>:    call   0x555555555140 <func2>
>>>
>>> 12      }
>>>    0x000055555555516f <+31>:    xor    %eax,%eax
>>>    0x0000555555555171 <+33>:    add    $0x10,%rsp
>>>    0x0000555555555175 <+37>:    pop    %rbp
>>>    0x0000555555555176 <+38>:    ret
>>> End of assembler dump.
>>> (gdb) reverse-step
>>> func1 (i=1) at reverse.c:3
>>> 3       }
>>> (gdb)
>>>
>>> We can see that GDB stopped on the call instruction instead. So a user that finished from func1 or reverse-finished from func2 may see different inferior states.
> 
>> Seeing different inferior states is expected, as finish and reverse-finish are not the exact mirror of one another, like step
>> and reverse-step are.  The exact reversal of finish would be the equivalent of being stopped at a function call return insn after
>> a "finish" (and the command could only be used while stopped there), and stepping back into the function up until the point
>> where you had typed "finish" and stopping there.  Obviously impossible to implement.  So we made "reverse-finish" do something
>> sensible, such as stepping backwards up until the call site.
> 
> Hi Pedro,
> 
> I certainly agree with you that reverse-step needs to be fixed,
> and this explains at least part of the problems we were seeing.
> 
> However, I do think there is still another issue specific to
> reverse-finish, along the lines Bruno pointed out.  And in fact,
> I do think that there is (or at least should be!) a well-defined
> symmetry between finish and reverse-finish:
> 
> The effect of "finish" is: skip until the end of the current
> function and then do a "step".  (Semantically equivalently,
> do a series of "next" until you leave the current function.)

But that this is not actually true -- this is not how finish is implemented.

Instead, we unwind to the caller frame, and put a breakpoint at that
frame'c PC, and run freely to that.  A frame's PC is defined as the address
of the instruction that is executed once the callee returns.

OTOH, a (forward) step at the return line of a function steps until a different
line is reached, and in the case we step out of a function, once it reaches the
caller it notices we ended up in the middle of a line so it continues stepping until
the next line.

Note that finish vs step difference visible here:

(gdb) list 1
1       int func1 (int i)
2       {
3         return i + 1;
4       }
5
6       int func2 (int i)
7       {
8         return i + 1;
9       }
10
11      int main (int argc, char **argv)
12      {
13        func1 (func2 (argc));
14        return 0;
15      }
(gdb) 

(gdb) b func2
Breakpoint 1 at 0x1147: file finish.c, line 8.
(gdb) r
Starting program: /home/pedro/finish 
[Thread debugging using libthread_db enabled]
Using host libthread_db library "/lib/x86_64-linux-gnu/libthread_db.so.1".

Breakpoint 1, func2 (i=1) at finish.c:8
8         return i + 1;
(gdb) finish
Run till exit from #0  func2 (i=1) at finish.c:8
0x000055555555516c in main (argc=1, argv=0x7fffffffdcb8) at finish.c:13    <<< stopped in the middle of the line, as indicated by address on the left
13        func1 (func2 (argc));
Value returned is $1 = 2                                                   <<< at this location we can use ABI knowledge to retrieve the return value
(gdb) p $pc
$2 = (void (*)()) 0x55555555516c <main+29>                                 <<< matches the "0x000055555555516c in main" above, of course

(gdb) r
The program being debugged has been started already.
Start it from the beginning? (y or n) y
Starting program: /home/pedro/finish 
[Thread debugging using libthread_db enabled]
Using host libthread_db library "/lib/x86_64-linux-gnu/libthread_db.so.1".

Breakpoint 1, func2 (i=1) at finish.c:8
8         return i + 1;
(gdb) step
9       }                                                                  <<< stopped a line bounary, and stepped over the func1 call as well!
(gdb) p $pc
$3 = (void (*)()) 0x55555555514d <func2+17>


> 
> Similarly, the effect of "reverse-finish" *should* be: reverse-skip
> until the beginning of the current function and then do a reverse-step.
> (Or semantically equivalent, do a series of "reverse-next" until you
> leave the current function.)

I disagree, because that reverse-step would step backwards too much.  Instead of stepping back to the
call of the function, it would step back further, as reverse-step will try to stop at the
beginning of a line.  By doing that, you'll potentially step backwards more statements that
exist in the same line.  Like:

  a = 1; b = 2; func();

stepping back from inside func should stop at the first instruction of that whole line.
But finishing backwards should stop at the call to func(), without considering line
boundaries, just like forward finish does not.

From the manual:

   "Like the step command, reverse-step will only stop at the beginning of a source line. It “un-executes” the previously executed source line."

> 
> However, the actual implementation of reverse-finish is subtly but
> significantly different: it reverse-skips until the beginning of the
> current function and then does a reverse-stepi (not reverse-step).
> 
> This has the at least surprising, but IMO even incorrect, effect
> that when on the first line of a function, "reverse-finish" ends
> up in a different place than "reverse-step".  I believe they 
> ought to end up in the same place.

I disagree.

Pedro Alves

> 
> 
> This was one of the primary intentions behind Carl's patch: change
> reverse-finish so it does a reverse-step instead of a reverse-stepi.
> (Of course, for this to be useful, reverse-step itself needs to work
> correctly - which it doesn't right now as you point out.)
> 
> Bye,
> Ulrich
>