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

[Carl Love <cel@us.ibm.com>]

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.

                  Carl 
>