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-24 2:23 p.m., Bruno Larsen wrote:
> On 24/01/2023 15:08, Pedro Alves wrote:
>> On 2023-01-23 9:13 p.m., Carl Love wrote:
>>> Pedro:
>>>
>>> On Mon, 2023-01-23 at 19:17 +0000, Pedro Alves wrote:
>>>>> Currently on X86, when executing the finish command in reverse, gdb
>>>>> does a
>>>>> single step from the first instruction in the callee to get back to
>>>>> the
>>>>> caller.  GDB stops on the last instruction in the source code line
>>>>> where
>>>>> the call was made.  When stopped at the last instruction of the
>>>>> source code
>>>>> line, a reverse next or step command will stop at the first
>>>>> instruction
>>>>> of the same source code line thus requiring two step/next commands
>>>>> to
>>>>> reach the previous source code line.  It should only require one
>>>>> step/next
>>>>> command to reach the previous source code line.
>>>>>
>>>>> By contrast, a reverse next or step command from the first line in
>>>>> a
>>>>> function stops at the first instruction in the source code line
>>>>> where the
>>>>> call was made.
>>>> I'd think this was on purpose.  Note that next/step/reverse-
>>>> {next/step} are line-oriented
>>>> stepping commands, they step/next until the previous/next line.
>>>> While "finish" is described
>>>> as undoing the _function call_.
>>>>
>>>> The manual says:
>>>>
>>>>   reverse-finish
>>>>   Just as the finish command takes you to the point where the current
>>>> function returns,
>>>>   reverse-finish takes you to the point where it was called. Instead
>>>> of ending up at the end of
>>>>   the current function invocation, you end up at the beginning.
>>>>
>>>> Say you have a line with multiple statements involving multiple
>>>> function calls.
>>>> The simplest would be:
>>>>
>>>>    func1 ();  func2 ();
>>>>
>>>> Say you'd stopped inside 'func2'.  If you do finish there, in current
>>>> master gdb
>>>> stops at the call to 'func2', and you can then decide to reverse step
>>>> into 'func1'.
>>> I don't think you followed the issue.
>> Totally possible!
>>
>>> So, if you are in func2 and do a reverse-finish, without the patch gdb
>>> stops on the last instruction for the line that calls func2.
>> Right.
>>
>>> Now if
>>> you issue a reverse-step, you stop at the first instruction for the
>>> call to func2, i.e. you are still on the same source code line.
>> Wait.  That right there sounds bogus.  The source line looks like:
>>
>>     func1 ();  func2 ();
>>
>> so stepping backwards over that line should always stop at the first
>> instruction of the line, not in the middle.  Let's simplify this.
>>
>> Here's the full source code of my example:
>>
>> (gdb) list 1
>> 1       void func1 ()
>> 2       {
>> 3       }
>> 4
>> 5       void func2 ()
>> 6       {
>> 7       }
>> 8
>> 9       int main ()
>> 10      {
>> 11        func1 (); func2 ();
>> 12      }
>>
> I think you are describing a different issue to what Carl is trying to solve. Using your example code, I have the following debugging session:

No, it's the exact same.  Please re-read.

> 
> $ ./gdb -q reverse
> Reading symbols from reverse...
> (gdb) start
> Temporary breakpoint 1 at 0x401118: file t.c, line 8.
> Starting program: /home/blarsen/Documents/build/gdb/reverse
> [Thread debugging using libthread_db enabled]
> Using host libthread_db library "/lib64/libthread_db.so.1".
> 
> Temporary breakpoint 1, main () at t.c:8
> 8           func1(); func2();
> (gdb) record
> (gdb) n
> 9       }
> (gdb) rs
> func2 () at t.c:5
> 5       }
> (gdb) reverse-finish
> Run back to call of #0  func2 () at t.c:5
> 0x0000000000401127 in main () at t.c:8
> 8           func1(); func2();
> (gdb) rs
> 8           func1(); func2();
> (gdb)
> func1 () at t.c:2
> 2       }
> (gdb)
> 
> Notice how there were two "reverse-step" commands needed after the "reverse-finish" used to exit func2. 

Yes.  And the reason you need two "reverse-step" is because there are two line ranges for line 8, and reverse-step stops
at the beginning of the second range instead of at the beginning of the first.  It's the exact same as with my simpler
example of just doing "next" -> "reverse-next", which I used as a simpler example to explain the problem.

> What Carl is proposing we do is make it so GDB only needs one command.

I understand.  However, I am saying that that is papering over the actual problem, _and_ it only works in the situation
where you ended up with two line entries with is-stmt for the same line.  Note how the patch breaks clang, and gcc with
-gno-column-info...

> If I compare the $pc of where GDB is stopped and the linetable, we get:
> 
> (gdb) print $pc
> $2 = (void (*)()) 0x401127 <main+19>
> (gdb) maint info line-table
> objfile: /home/blarsen/Documents/downstream_build/gdb/reverse ((struct objfile *) 0x10f7750)
> compunit_symtab: t.c ((struct compunit_symtab *) 0x116c330)
> symtab: /home/blarsen/Documents/downstream_build/gdb/t.c ((struct symtab *) 0x116c3b0)
> linetable: ((struct linetable *) 0x11aec80):
> INDEX  LINE   ADDRESS            IS-STMT PROLOGUE-END
> 0      1      0x0000000000401106 Y
> 1      2      0x000000000040110a Y
> 2      4      0x000000000040110d Y
> 3      5      0x0000000000401111 Y
> 4      7      0x0000000000401114 Y
> 5      8      0x0000000000401118 Y
> 6      8      0x0000000000401122 Y
> 7      9      0x0000000000401131 Y
> 8      END    0x0000000000401133 Y
> 
> We can see that GDB shouldn't even be able to stop at that $pc, it isn't an is_stmt. 

reverse-finish is not supposed to step backwards until it reaches is_stmt.  Doing so makes it
step backwards too much, as I've shown in my previous example.

> We should have stopped at 0x401122, which is where the first reverse-step stops:

No...

> 
> (gdb) rs
> 8           f1(); f2();
> (gdb) p $pc
> $4 = (void (*)()) 0x401122 <main+14>

... no because in the case where you don't have column debug info (or with clang), there won't be
an is-stmt entry for the f2 call/column, there will only be one single line entry for the whole of
line 8, so gdb would step back too much.

> 
> So not only are we needing an extra command to re-sync with user expectations, we are in an instruction where the inferior state might be all over the place.
> 

What does that mean, the state might be all over the place?  The DWARF should describe the locations of all variables accurately at all instructions.