[PATCH][gdb/symtab] Ignore zero line table entries

[Tom de Vries]

On 7/24/20 1:30 PM, Pedro Alves wrote:
> On 7/21/20 8:04 PM, Simon Marchi wrote:
>> On 2020-07-16 1:49 p.m., Tom de Vries wrote:
>>> Hi,
>>>
>>> The DWARF standard states for the line register in the line number information
>>> state machine the following:
>>> ...
>>> An unsigned integer indicating a source line number.  Lines are numbered
>>> beginning at 1.  The compiler may emit the value 0 in cases where an
>>> instruction cannot be attributed to any source line.
>>> ...
>>>
>>> So, it's possible to have a zero line number in the DWARF line table.
>>>
>>> This is currently not handled by GDB.  The zero value is read in as any other
>>> line number, but internally the zero value has a special meaning:
>>> end-of-sequence, so the line table entry ends up having a different
>>> interpretation than intended in some situations.
>>>
>>> I've created a test-case where various aspects are tested, which has these 4
>>> interesting tests.
>>>
>>> 1. Next-step through a zero-line instruction, is_stmt == 1
>>> gdb.dwarf2/dw2-line-number-zero.exp: bar1, 2nd next
>>>
>>> 2. Next-step through a zero-line instruction, is_stmt == 0
>>> gdb.dwarf2/dw2-line-number-zero.exp: bar2, 2nd next
>>>
>>> 3. Show source location at zero-line instruction, is_stmt == 1
>>> gdb.dwarf2/dw2-line-number-zero.exp: continue to breakpoint: bar1_label_3
>>>
>>> 4. Show source location at zero-line instruction, is_stmt == 0
>>> gdb.dwarf2/dw2-line-number-zero.exp: continue to breakpoint: bar2_label_3
>>>
>>> And we have the following results:
>>>
>>> 8.3.1, 9.2:
>>> ...
>>> FAIL: gdb.dwarf2/dw2-line-number-zero.exp: bar1, 2nd next
>>> PASS: gdb.dwarf2/dw2-line-number-zero.exp: bar2, 2nd next
>>> PASS: gdb.dwarf2/dw2-line-number-zero.exp: continue to breakpoint: bar1_label_3
>>> FAIL: gdb.dwarf2/dw2-line-number-zero.exp: continue to breakpoint: bar2_label_3
>>> ...
>>>
>>> commit 8c95582da8 "gdb: Add support for tracking the DWARF line table is-stmt
>>> field":
>>> ...
>>> PASS: gdb.dwarf2/dw2-line-number-zero.exp: bar1, 2nd next
>>> PASS: gdb.dwarf2/dw2-line-number-zero.exp: bar2, 2nd next
>>> FAIL: gdb.dwarf2/dw2-line-number-zero.exp: continue to breakpoint: bar1_label_3
>>> FAIL: gdb.dwarf2/dw2-line-number-zero.exp: continue to breakpoint: bar2_label_3
>>> ...
>>>
>>> commit d8cc8af6a1 "[gdb/symtab] Fix line-table end-of-sequence sorting",
>>> master:
>>> FAIL: gdb.dwarf2/dw2-line-number-zero.exp: bar1, 2nd next
>>> FAIL: gdb.dwarf2/dw2-line-number-zero.exp: bar2, 2nd next
>>> PASS: gdb.dwarf2/dw2-line-number-zero.exp: continue to breakpoint: bar1_label_3
>>> PASS: gdb.dwarf2/dw2-line-number-zero.exp: continue to breakpoint: bar2_label_3
>>> ...
>>>
>>> The regression in test 2 at commit d8cc8af6a1 was filed as PR symtab/26243,
>>> where clang emits zero line numbers.
>>>
>>> The way to fix all tests is to make sure line number zero internally doesn't
>>> clash with special meaning values, and by handling it appropriately
>>> everywhere.  That however looks too intrusive for the GDB 10 release.
>>>
>>> Instead, we decide to ensure defined behaviour for line number zero by
>>> ignoring it.  This gives us back the test results from before commit
>>> d8cc8af6a1, fixing PR26243.
>>>
>>> We mark the FAILs for tests 3 and 4 as KFAILs.  Test 4 was already failing for
>>> the 9.2 release, and we consider the regression of test 3 from gdb 9.2 to gdb
>>> 10 the cost for having defined behaviour.
>>>
>>> Build and reg-tested on x86_64-linux.
>>>
>>> Any comments?
>>>
>>> Thanks,
>>> - Tom
>>>
>>> [gdb/symtab] Ignore zero line table entries
>>>
>>> gdb/ChangeLog:
>>>
>>> 2020-07-16  Tom de Vries  <tdevries@suse.de>
>>>
>>> 	PR symtab/26243
>>> 	* dwarf2/read.c (lnp_state_machine::record_line): Ignore zero line
>>> 	entries.
>>>
>>> gdb/testsuite/ChangeLog:
>>>
>>> 2020-07-16  Tom de Vries  <tdevries@suse.de>
>>>
>>> 	PR symtab/26243
>>> 	* gdb.dwarf2/dw2-line-number-zero.c: New test.
>>> 	* gdb.dwarf2/dw2-line-number-zero.exp: New file.
>>
>> Ok, so the state as of today is that we have three patches for 26243:
>>
>> Tom's patch (this one): filter out line 0 entres at the DWARF reader level
>>
>> Andrew's patch [1]: replace internal references to line 0 (which means an end marker
>>                     in the line table) with an explicit linetable_entry::end_marker,
>>                     but otherwise don't try to handle the "real" line 0 entries coming
>>                     from the DWARF
>>
>> Pedro's patch [2]: be smarter when stepping into or from some instructions that map to
>>                    line 0
>>
>> Pedro's patch tackles one consequence of having "line 0" entries in the line table,
>> related to stepping.  But they have other consequences than stepping, like the
>> "disassemble /s" issue that I raised in response to Andrew's patch.
>>
>> So the safe bet would be: merge Tom's patch for GDB 10 and keep the others for after
>> the branch creation would.  It brings the GDB 10 behavior back to what GDB 9 was, where
>> we pretend that the instruction mapped to line 0 belongs to the line of the previous
>> instruction.
>>
>> Or, we go straight away with Andrew's patch that switches the end markers to -1,
>> Pedro's patch that makes stepping deal with "line 0" entries, and give a push for
>> GDB 10 to try to find and fix any other issues that this would cause.
>>
>> I don't really mind, but I think it would be good to clear that up first to know where
>> we are going.
> 
> As I mentioned in the other bug, I'm more inclined with going with Tom's
> approach for GDB 10.  Adding proper support for line 0 entries doesn't
> seem to bring a significant user-visible improvement, which suggests
> that there's no need to rush it.  So I've rather we fix things in master
> calmly without the pressure of having to fix whatever fallout blocking
> the release.

I've now committed my patch (with the bug in the test-case noticed by
Pedro fixed by using MACRO_AT_func) as a stopgap.  If we decide to go
with that, we're done.  If we decide to try and fix this more precisely,
we have something to revert back to in case things don't work out.

Thanks,
- Tom