Re: [PATCH] Updated, fix reverse stepping multiple contiguous PC ranges

[Bruno Larsen <blarsen@redhat.com>]

Carl:

On 3/9/22 17:52, Carl Love wrote:
> Bruno:
> 
> On Wed, 2022-03-09 at 09:23 -0300, Bruno Larsen wrote:
>>>> Thanks for looking at this. Since I don't test on aarch64 often,
>>>> I am
>>>> not sure if I see regressions or racy testcases, but it does fix
>>>> the
>>>> issue you mentioned, and there doesn't seem to be regressions on
>>>> x86_64 hardware. I have a few nits, but the main feedback is:
>>>> could
>>>> you add a testcase for this, using the dwarf assembler and
>>>> manually creating contiguous PC ranges, so we can confirm that
>>>> this is not
>>>> regressed in the future on any hardware?
>>>>
>>>> Also, I can't approve a patch, but with the testcase this patch
>>>> is
>>>> mostly ok by me
>>>>
>>>
>>> I have not writen anything in dwarf assembler in the past.  Off the
>>> top
>>> of my head, don't know how to create such a test case.  It does
>>> seem
>>> that there are the two testcases  gdb.reverse/solib-precsave.exp
>>> and
>>> gdb.reverse/solib-reverse.exp which the patch fixes.  I guess the
>>> dwarf
>>> assembly test would be similar to the C level code.
>>>
>>> Do you have an example of how to write dwarf assembly or a pointer
>>> to a
>>> tutorial on writting dwarf?
>>
>> I have recently worked on gdb.base/until-trailing-insns.exp, that
>> uses the dwarf assembler quite a bit to create a line table. I am not
>> sure how one would go about making contiguous ranges, but maybe you
>> can find something in gdb/testsuite/lib/dwarf.exp to help you.
> 
> I have studied until-trailing-insns.exp, dwarf.exp and the DWARF
> Debugging Informat Format Version 5 document.  I really don't see how
> to write the test case you desire.
> 
> The issue is gdb is supposed to step in reverse one statement at a
> time.  The two testcases that the patch fix have two statements on the
> same line.  Specifically from gdb.reverse/solib-reverse.c it has the
> source code line:  b[0] = 6;   b[1] = 9;.  It is this line where the
> reverse step fails to stop in between the two statements.
> 
> I tried
> dumping the DWARF info for a very simple program with two statements on
> the same line as in the two test cases that fail to see what GCC
> generates.  The test program is:
> 
> int main() {
>    int i,j;
>    i = 1; j=3;
>    printf("i=%d, j=%d\n", i, j);
> }
> 
> gcc -g  -O0 test2.c -o test2
> objdump -g -W -S -d test2  > test2.dump
> 
> The interesting dump information is:
> 
>   int main() {
>   7fc:   02 00 4c 3c     addis   r2,r12,2
>   800:   04 77 42 38     addi    r2,r2,30468
>   804:   a6 02 08 7c     mflr    r0
>   808:   10 00 01 f8     std     r0,16(r1)
>   80c:   f8 ff e1 fb     std     r31,-8(r1)
>   810:   81 ff 21 f8     stdu    r1,-128(r1)
>   814:   78 0b 3f 7c     mr      r31,r1
>    int i,j;
>    i = 1; j=3;
>   818:   01 00 20 39     li      r9,1                    // Store i = 1
>   81c:   68 00 3f 91     stw     r9,104(r31)
>   820:   03 00 20 39     li      r9,3                    // Store j = 3
>   824:   6c 00 3f 91     stw     r9,108(r31)
>    printf("i=%d, j=%d\n", i, j);
>   
> Line Number Statements:
>    [0x00000040]  Set column to 12
>    [0x00000042]  Extended opcode 2: set Address to 0x7fc
>    [0x0000004d]  Special opcode 10: advance Address by 0 to 0x7fc and Line by 5 to 6
>    [0x0000004e]  Set column to 5
>    [0x00000050]  Special opcode 105: advance Address by 28 to 0x818 and Line by 2 to 8
>    [0x00000051]  Set column to 11
>    [0x00000053]  Special opcode 33: advance Address by 8 to 0x820 and Line by 0 to 8
>    [0x00000054]  Set column to 3
>    [0x00000056]  Special opcode 34: advance Address by 8 to 0x828 and Line by 1 to 9
>    [0x00000057]  Set column to 1
>    [0x00000059]  Special opcode 146: advance Address by 40 to 0x850 and Line by 1 to 10
>    [0x0000005a]  Advance PC by 36 to 0x874
>    [0x0000005c]  Extended opcode 1: End of Sequence
> 
> 
> Here I can see that "[0x00000053]  Special opcode 33: advance Address
> by 8 to 0x820 and Line by 0 to 8"  refers to address 0x820 in the
> binary which is the assembly statement for j=3.  Also, it says advance
> Line by 0, i.e. it is the same line number as the previous statement
> i=1.
> 
> The line entries in the line table, i.e. Line Number, are different.
> 
> In your message, you said to create a test case that has "continuous PC
> ranges."  I don't see where/how that would be reflected in the DWARF
> info as dumped above?  I don't see GCC generating information/code with
> continuous PC ranges.  I have not found anything that would enable me
> to do that.  Each assembly statement has a unique PC value, but the
> DWARF info does track the source Line number separately.


The testcase I had in mind could be something simple like this:

test.c:
1 int main (){
2     asm ("main_label: .globl main_label");
3     int i,j;
4     asm ("i_assignment: .globl i_assignment");
5     i = 0; /* TAG: assignment line */
6     asm ("j_assignment: .globl j_assignment");
7     j = 0;
8     return 0;
9 }

test.exp:
...
dwarf::assemble{
...
     {DW_LNE_set_address i_assignment}
     {line [gdb_get_line_number "TAG: assignment line"]}
     {DW_LNS_copy}
     {DW_LNE_set_address j_assignment}
     {line [gdb_get_line_number "TAG: assignment line"]}
     {DW_LNS_copy}
...
}

To explain what is going on: We use the asm() labels to identify PC addresses for the assignment of variables. We use the comment /* TAG ... */ to identify the line we care about. And then we pretend that those 2 PC addresses came from the same line. Even though they didn't in the actual .c, GDB will think they did because of the dwarf information, guaranteeing that we will continue to test this for regressions. (side note: main_label needs to be there for the dwarf assembler to work correctly.)

If I understand the problem correctly, this test and dwarf assembler snippet should be able to reproduce the bug, but I am also no DWARF expert, so I may be missing something as well.

> 
> Clearly GCC is generating DWARF that specifies the statements are in
> the same line but with different PC values.  I have not found anything
> that would help me "making contiguous ranges" that you refere to.  I
> really don't see how a new DWARF test is going to be different or
> better then the existing C tests?  Perhaps you could explain more as to
> the value of this new test and why the existing two C code tests which
> the patch fixes are not sufficient to detect a future gdb regression?
> Sorry, I just don't see how to create the test you propose and at this
> point, and I am not seeing the value in it.  I don't claim to be an
> expert on DWARF so it is entirely possible I am missing something here.

The value comming from a test like this is that we can have the aarch64 behavior on any CPU architecture. This will give us more test coverage, as I can't easily test on aarch64 hardware for instance, and I imagine a lot of people may just be unable to test it. Also,it is possible that GCC will change this behavior in the future for those specific tests, and we unknowingly stop testing for this regression.

> 
> Thanks for your feedback and help on this.
> 
>                        Carl
> 
> 
> 


-- 
Cheers!
Bruno Larsen