Re: [PATCH] Fix reverse stepping multiple contiguous PC ranges over the line table.

[Bruno Larsen <blarsen@redhat.com>]

On 02/05/2023 17:40, Carl Love wrote:
> Bruno:
>
> On Tue, 2023-05-02 at 16:15 +0200, Bruno Larsen wrote:
>
> I had intended everything from here down as part of the commit message.
> It is admittedly a lot.  It could be cut down my making the first part
> could be moved to the mailing list context.  The commit message would
> then start with the two scenarios the patch fixes.
>>> ---------------------------------------------------------------
>>> Fix reverse stepping multiple contiguous PC ranges over the line
>>> table.
>>>
>>> There are a couple of scenarios where the GDB reverse-step and
>>> reverse-next
>>> commands do not work correctly.  The first scenario consists of
>>> multiple
>>> assignment statements on the same line.  A patch was proposed to
>>> address the
>>> issue by Luis Machado and briefly discussed on the mailing list in
>>> Feb 2021.
>>>
>>> https://sourceware.org/pipermail/gdb-patches/2021-February/175678.html
>>>
>>> The discussion was revived by Carl Love with regards to fixing the
>>> same
>>> issue on PowerPC.
>>>
>>> https://sourceware.org/pipermail/gdb-patches/2022-March/186463.html
>>>
>>> The patch was not completed and has been on Carl's to do list for
>>> some time.
>>>
>>> Discussion of a patch to change how the reverse-step and reverse-
>>> next
>>> commands submitted by Carl Love was started in thread:
>>>
>>> https://sourceware.org/pipermail/gdb-patches/2023-January/195563.html
>>>
>>> The patch was withdrawn as it was pointed out the proposed patch
>>> would
>>> change the intended behavior of the commands as documented in the
>>> GDB
>>> manual.  However, it was pointed out by Pedro Alves <
>>> pedro@palves.net>
>>> that the reverse-step and reverse-next commands do not work when
>>> there
>>> are multiple function calls on the same line. This is a second
>>> scenario
>>> where the commands do not work correctly.
>>>
>>> The following patch is an extended version of the original patch by
>>> Luis Machado to fix the issues in scenario 1 to also address the
>>> issues in
>>> scenario 2.
>>>
> Move the above to the mailing list
>
> Start here with the commit message
>
>>> --------------------------------------------------------
>>>
>>> Scenario 1 issue description by Luis Machado:
>>>
>>> When running GDB's testsuite on aarch64-linux/Ubuntu 20.04 (also
>>> spotted on
>>> the ppc backend), I noticed some failures in gdb.reverse/solib-
>>> precsave.exp
>>> and gdb.reverse/solib-reverse.exp.
>>>
>>> The failure happens around the following code:
>>>
>>> 38  b[1] = shr2(17);          /* middle part two */
>>> 40  b[0] = 6;   b[1] = 9;     /* generic statement, end part two */
>>> 42  shr1 ("message 1\n");     /* shr1 one */
>>>
>>> Normal execution:
>>>
>>> - step from line 38 will land on line 40.
>>> - step from line 40 will land on line 42.
>>>
>>> Reverse execution:
>>> - step from line 42 will land on line 40.
>>> - step from line 40 will land on line 40.
>>> - step from line 40 will land on line 38.
>>>
>>> The problem here is that line 40 contains two contiguous but
>>> distinct
>>> PC ranges in the line table, like so:
>>>
>>> Line 40 - [0x7ec ~ 0x7f4]
>>> Line 40 - [0x7f4 ~ 0x7fc]
>>>
>>> The two distinct ranges are generated because GCC started
>>> outputting source
>>> column information, which GDB doesn't take into account at the
>>> moment.
>>>
>>> When stepping forward from line 40, we skip both of these ranges
>>> and land on
>>> line 42. When stepping backward from line 42, we stop at the start
>>> PC of the
>>> second (or first, going backwards) range of line 40.
>>>
>>> This happens because we have this check in
>>> infrun.c:process_event_stop_test:
>>>
>>>           /* When stepping backward, stop at beginning of line range
>>>              (unless it's the function entry point, in which case
>>>              keep going back to the call point).  */
>>>           CORE_ADDR stop_pc = ecs->event_thread->stop_pc ();
>>>           if (stop_pc == ecs->event_thread->control.step_range_start
>>>            && stop_pc != ecs->stop_func_start
>>>            && execution_direction == EXEC_REVERSE)
>>>             end_stepping_range (ecs);
>>>           else
>>>             keep_going (ecs);
>>>
>>> Since we've reached ecs->event_thread->control.step_range_start, we
>>> stop
>>> stepping backwards.
>>>
>>> The right thing to do is to look for adjacent PC ranges for the
>>> same line,
>>> until we notice a line change. Then we take that as the start PC of
>>> the
>>> range.
>>>
>>> Another solution I thought about is to merge the contiguous ranges
>>> when
>>> we are reading the line tables. Though I'm not sure if we really
>>> want to
>>> process that data as opposed to keeping it as the compiler created,
>>> and
>>> then working around that.
>>>
>>> The test case gdb.reverse/map-to-same-line.exp is added to test the
>>> fix
>>> for the issues in scenario 1.
>>>
>>> ---------------------------------------------------------
>>>
>>> Scenario 2 issue described by Pedro Alves:
>>>
>>> The following explanation of the issue was taken from the gdb
>>> mailing list
>>> discussion of the withdrawn patch to change the behavior of the
>>> reverse-step
>>> and reverse-next commands.  Specifically, message from Pedro Alves
>>> <pedro@palves.net> where he demonstrates the issue where you have
>>> multiple
>>> function calls on the same source code line:
>>>
>>> https://sourceware.org/pipermail/gdb-patches/2023-January/196110.html
>>>
>>> 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      }
>>>
>>> Compiled with:
>>>
>>>    $ gcc reverse.c -o reverse -g3 -O0
>>>    $ gcc -v
>>>    ...
>>>    gcc version 11.3.0 (Ubuntu 11.3.0-1ubuntu1~22.04)
>>>
>>> Now let's debug it with target record, using current gdb git master
>>> (f3d8ae90b236),
>>> without your patch:
>>>
>>>    $ gdb ~/reverse
>>>    GNU gdb (GDB) 14.0.50.20230124-git
>>>    ...
>>>    Reading symbols from /home/pedro/reverse...
>>>    (gdb) start
>>>    Temporary breakpoint 1 at 0x1147: file reverse.c, line 11.
>>>    Starting program: /home/pedro/reverse
>>>    [Thread debugging using libthread_db enabled]
>>>    Using host libthread_db library "/lib/x86_64-linux-
>>> gnu/libthread_db.so.1".
>>>
>>>    Temporary breakpoint 1, main () at reverse.c:11
>>>    11        func1 (); func2 ();
>>>    (gdb) record
>>>
>>>    (gdb) disassemble /s
>>>    Dump of assembler code for function main:
>>>    reverse.c:
>>>    10      {
>>>       0x000055555555513f <+0>:     endbr64
>>>       0x0000555555555143 <+4>:     push   %rbp
>>>       0x0000555555555144 <+5>:     mov    %rsp,%rbp
>>>
>>>    11        func1 (); func2 ();
>>>    => 0x0000555555555147 <+8>:     mov    $0x0,%eax
>>>       0x000055555555514c <+13>:    call   0x555555555129 <func1>
>>>       0x0000555555555151 <+18>:    mov    $0x0,%eax
>>>       0x0000555555555156 <+23>:    call   0x555555555134 <func2>
>>>       0x000055555555515b <+28>:    mov    $0x0,%eax
>>>
>>>    12      }
>>>       0x0000555555555160 <+33>:    pop    %rbp
>>>       0x0000555555555161 <+34>:    ret
>>>    End of assembler dump.
>>>
>>>    (gdb) n
>>>    12      }
>>>
>>> So far so good, a "next" stepped over the whole of line 11 and
>>> stopped at line 12.
>>>
>>> Let's confirm where we are now:
>>>
>>>    (gdb) disassemble /s
>>>    Dump of assembler code for function main:
>>>    reverse.c:
>>>    10      {
>>>       0x000055555555513f <+0>:     endbr64
>>>       0x0000555555555143 <+4>:     push   %rbp
>>>       0x0000555555555144 <+5>:     mov    %rsp,%rbp
>>>
>>>    11        func1 (); func2 ();
>>>       0x0000555555555147 <+8>:     mov    $0x0,%eax
>>>       0x000055555555514c <+13>:    call   0x555555555129 <func1>
>>>       0x0000555555555151 <+18>:    mov    $0x0,%eax
>>>       0x0000555555555156 <+23>:    call   0x555555555134 <func2>
>>>       0x000055555555515b <+28>:    mov    $0x0,%eax
>>>
>>>    12      }
>>>    => 0x0000555555555160 <+33>:    pop    %rbp
>>>       0x0000555555555161 <+34>:    ret
>>>    End of assembler dump.
>>>
>>> Good, we're at the first instruction of line 12.
>>>
>>> Now let's undo the "next", with "reverse-next":
>>>
>>>    (gdb) reverse-next
>>>    11        func1 (); func2 ();
>>>
>>> Seemingly stopped at line 11.  Let's see exactly where:
>>>
>>>    (gdb) disassemble /s
>>>    Dump of assembler code for function main:
>>>    reverse.c:
>>>    10      {
>>>       0x000055555555513f <+0>:     endbr64
>>>       0x0000555555555143 <+4>:     push   %rbp
>>>       0x0000555555555144 <+5>:     mov    %rsp,%rbp
>>>
>>>    11        func1 (); func2 ();
>>>       0x0000555555555147 <+8>:     mov    $0x0,%eax
>>>       0x000055555555514c <+13>:    call   0x555555555129 <func1>
>>>    => 0x0000555555555151 <+18>:    mov    $0x0,%eax
>>>       0x0000555555555156 <+23>:    call   0x555555555134 <func2>
>>>       0x000055555555515b <+28>:    mov    $0x0,%eax
>>>
>>>    12      }
>>>       0x0000555555555160 <+33>:    pop    %rbp
>>>       0x0000555555555161 <+34>:    ret
>>>    End of assembler dump.
>>>    (gdb)
>>>
>>> And lo, we stopped in the middle of line 11!  That is a bug, we
>>> should have
>>> stepped back all the way to the beginning of the line.  The
>>> "reverse-next"
>>> should have fully undone the prior "next" command.
>>>
>>> The test cases gdb.reverse/func-map-to-same-line-no-colum-info.exp
>>> and
>>> gdb.reverse/func-map-to-same-line.exp were added to test the fix
>>> for scenario
>>> 2 when the binary was compiled with and without line table
>>> information.
>>>
>>> bug:
>>> https://sourceware.org/bugzilla/show_bug.cgi?id=28426
>>>   
>>>
>>> Co-authored-by: Luis Machado <luis.machado@arm.com>
>>> Co-authored-by: Carl Love <cel@us.ibm.com>
> End of commit message
>> Hey Carl,
>>
>> Thanks for working on this. I'm wondering which parts will be part
>> of
>> the final commit messages and which is just context for the mailing
>> list, so some clarity would be nice, but that is not a huge deal.
>>
>> I wanted to test this change, but it doesn't apply anymore on
>> master,
>> and `git apply --3way` can't figure out how to do it. Which commit
>> did
>> you use as base (or alternatively, can you rebase it)?
> OK, let me rebase it and repost.  Let me know if you think the
> suggested commit message is still too much.  Thanks.
Hey sorry, this part was my bad, no need to rebase. I'll look at the 
commit message in a bit


-- 
Cheers,
Bruno

>
>                    Carl