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

Bruno Larsen via Gdb-patches
2023-05-02T15:42:54Z

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