Re: [PATCH][gdb/symtab] Fix line-table end-of-sequence sorting

[Tom de Vries <tdevries@suse.de>]

On 06-06-2020 08:51, Andrew Burgess wrote:
> * Tom de Vries <tdevries@suse.de> [2020-06-06 01:44:42 +0200]:
> 
>> [ was: Re: [PATCH 2/3] gdb: Don't reorder line table entries too much
>> when sorting. ]
>>
>> On 05-06-2020 18:00, Tom de Vries wrote:
>>> On 05-06-2020 16:49, Tom de Vries wrote:
>>>> On 23-12-2019 02:51, Andrew Burgess wrote:
>>>>> I had to make a small adjustment in find_pc_sect_line in order to
>>>>> correctly find the previous line in the line table.  In some line
>>>>> tables I was seeing an actual line entry and an end of sequence marker
>>>>> at the same address, before this commit these would reorder to move
>>>>> the end of sequence marker before the line entry (end of sequence has
>>>>> line number 0).  Now the end of sequence marker remains in its correct
>>>>> location, and in order to find a previous line we should step backward
>>>>> over any end of sequence markers.
>>>>>
>>>>> As an example, the binary:
>>>>>   gdb/testsuite/outputs/gdb.dwarf2/dw2-ranges-func/dw2-ranges-func-lo-cold
>>>>>
>>>>> Has this line table before the patch:
>>>>>
>>>>>   INDEX    LINE ADDRESS
>>>>>   0          48 0x0000000000400487
>>>>>   1         END 0x000000000040048e
>>>>>   2          52 0x000000000040048e
>>>>>   3          54 0x0000000000400492
>>>>>   4          56 0x0000000000400497
>>>>>   5         END 0x000000000040049a
>>>>>   6          62 0x000000000040049a
>>>>>   7         END 0x00000000004004a1
>>>>>   8          66 0x00000000004004a1
>>>>>   9          68 0x00000000004004a5
>>>>>   10         70 0x00000000004004aa
>>>>>   11         72 0x00000000004004b9
>>>>>   12        END 0x00000000004004bc
>>>>>   13         76 0x00000000004004bc
>>>>>   14         78 0x00000000004004c0
>>>>>   15         80 0x00000000004004c5
>>>>>   16        END 0x00000000004004cc
>>>>>
>>>>> And after this patch:
>>>>>
>>>>>   INDEX    LINE ADDRESS
>>>>>   0          48 0x0000000000400487
>>>>>   1          52 0x000000000040048e
>>>>>   2         END 0x000000000040048e
>>>>>   3          54 0x0000000000400492
>>>>>   4          56 0x0000000000400497
>>>>>   5         END 0x000000000040049a
>>>>>   6          62 0x000000000040049a
>>>>>   7          66 0x00000000004004a1
>>>>>   8         END 0x00000000004004a1
>>>>>   9          68 0x00000000004004a5
>>>>>   10         70 0x00000000004004aa
>>>>>   11         72 0x00000000004004b9
>>>>>   12        END 0x00000000004004bc
>>>>>   13         76 0x00000000004004bc
>>>>>   14         78 0x00000000004004c0
>>>>>   15         80 0x00000000004004c5
>>>>>   16        END 0x00000000004004cc
>>>>>
>>>>> When calling find_pc_sect_line with the address 0x000000000040048e, in
>>>>> both cases we find entry #3, we then try to find the previous entry,
>>>>> which originally found this entry '2         52 0x000000000040048e',
>>>>> after the patch it finds '2         END 0x000000000040048e', which
>>>>> cases the lookup to fail.
>>>>>
>>>>> By skipping the END marker after this patch we get back to the correct
>>>>> entry, which is now #1: '1          52 0x000000000040048e', and
>>>>> everything works again.
>>>>
>>>> I start to suspect that you have been working around an incorrect line
>>>> table.
>>>>
>>>> Consider this bit:
>>>> ...
>>>>    0          48 0x0000000000400487
>>>>    1          52 0x000000000040048e
>>>>    2         END 0x000000000040048e
>>>> ...
>>>>
>>>> The end marker marks the address one past the end of the sequence.
>>>> Therefore, it makes no sense to have an entry in the sequence with the
>>>> same address as the end marker.
>>>>
>>>> [ dwarf doc:
>>>>
>>>> end_sequence:
>>>>
>>>> A boolean indicating that the current address is that of the first byte
>>>> after the end of a sequence of target machine instructions. end_sequence
>>>> terminates a sequence of lines; therefore other information in the same
>>>> row is not meaningful.
>>>>
>>>> DW_LNE_end_sequence:
>>>>
>>>> The DW_LNE_end_sequence opcode takes no operands. It sets the
>>>> end_sequence register of the state machine to “true” and appends a row
>>>> to the matrix using the current values of the state-machine registers.
>>>> Then it resets the registers to the initial values specified above (see
>>>> Section 6.2.2). Every line number program sequence must end with a
>>>> DW_LNE_end_sequence instruction which creates a row whose address is
>>>> that of the byte after the last target machine instruction of the sequence.
>>>>
>>>> ]
>>>>
>>>> The incorrect entry is generated by this dwarf assembler sequence:
>>>> ...
>>>>                 {DW_LNS_copy}
>>>>                 {DW_LNE_end_sequence}
>>>> ...
>>>>
>>>> I think we should probably fix the dwarf assembly test-cases.
>>>>
>>>> If we want to handle this in gdb, the thing that seems most logical to
>>>> me is to ignore this kind of entries.
>>>
>>> Hmm, that seems to be done already, in buildsym_compunit::record_line.
>>>
>>> Anyway, I was looking at the line table for
>>> gdb.dwarf2/dw2-ranges-base.exp, and got a line table with subsequent end
>>> markers:
>>> ...
>>> INDEX  LINE   ADDRESS            IS-STMT
>>> 0      31     0x00000000004004a7 Y
>>> 1      21     0x00000000004004ae Y
>>> 2      END    0x00000000004004ae Y
>>> 3      11     0x00000000004004ba Y
>>> 4      END    0x00000000004004ba Y
>>> 5      END    0x00000000004004c6 Y
>>> ...
>>>
>>> By using this patch:
>>> ...
>>> diff --git a/gdb/buildsym.c b/gdb/buildsym.c
>>> index 33bf6523e9..76f0b54ff6 100644
>>> --- a/gdb/buildsym.c
>>> +++ b/gdb/buildsym.c
>>> @@ -943,6 +943,10 @@ buildsym_compunit::end_symtab_with_blockvector
>>> (struct block *static_block,
>>>             = [] (const linetable_entry &ln1,
>>>                   const linetable_entry &ln2) -> bool
>>>               {
>>> +               if (ln1.pc == ln2.pc
>>> +                   && ((ln1.line == 0) != (ln2.line == 0)))
>>> +                 return ln1.line == 0 ? true : false;
> 
> I will take a look at this patch properly as soon as I can, but just
> spotted this pet peeve of mine, please just write:
> 
>   return ln1.line == 0;

Ack, thanks, will update that.

Btw, I've retested this patch in combination with reverting the
find_pc_sect_line part of "gdb: Don't reorder line table entries too
much when sorting" and did not run into any test fails.

Thanks,
- Tom