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

classic Classic list List threaded Threaded
5 messages Options
Reply | Threaded
Open this post in threaded view
|

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

Tom de Vries
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  <[hidden email]>

        PR symtab/26243
        * dwarf2/read.c (lnp_state_machine::record_line): Ignore zero line
        entries.

gdb/testsuite/ChangeLog:

2020-07-16  Tom de Vries  <[hidden email]>

        PR symtab/26243
        * gdb.dwarf2/dw2-line-number-zero.c: New test.
        * gdb.dwarf2/dw2-line-number-zero.exp: New file.

---
 gdb/dwarf2/read.c                                 |   5 +-
 gdb/testsuite/gdb.dwarf2/dw2-line-number-zero.c   |  62 +++++++++
 gdb/testsuite/gdb.dwarf2/dw2-line-number-zero.exp | 162 ++++++++++++++++++++++
 3 files changed, 227 insertions(+), 2 deletions(-)

diff --git a/gdb/dwarf2/read.c b/gdb/dwarf2/read.c
index 558fad74f8..93f522c1f1 100644
--- a/gdb/dwarf2/read.c
+++ b/gdb/dwarf2/read.c
@@ -20300,8 +20300,9 @@ lnp_state_machine::record_line (bool end_sequence)
   bool file_changed
     = m_last_subfile != m_cu->get_builder ()->get_current_subfile ();
   bool ignore_this_line
-    = (file_changed && !end_sequence && m_last_address == m_address
-       && !m_is_stmt && m_stmt_at_address);
+           = ((file_changed && !end_sequence && m_last_address == m_address
+               && !m_is_stmt && m_stmt_at_address)
+              || (!end_sequence && m_line == 0));
 
   if ((file_changed && !ignore_this_line) || end_sequence)
     {
diff --git a/gdb/testsuite/gdb.dwarf2/dw2-line-number-zero.c b/gdb/testsuite/gdb.dwarf2/dw2-line-number-zero.c
new file mode 100644
index 0000000000..15b37a6676
--- /dev/null
+++ b/gdb/testsuite/gdb.dwarf2/dw2-line-number-zero.c
@@ -0,0 +1,62 @@
+/*
+   Copyright 2020 Free Software Foundation, Inc.
+
+   This program is free software; you can redistribute it and/or modify
+   it under the terms of the GNU General Public License as published by
+   the Free Software Foundation; either version 3 of the License, or
+   (at your option) any later version.
+
+   This program is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+   GNU General Public License for more details.
+
+   You should have received a copy of the GNU General Public License
+   along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
+
+void
+foo (int x)
+{
+
+}
+
+void
+bar1 (void)
+{
+  asm ("bar1_label: .globl bar1_label");
+  foo (1);
+  asm ("bar1_label_2: .globl bar1_label_2");
+  foo (2);
+  asm ("bar1_label_3: .globl bar1_label_3");
+  foo (3);
+  asm ("bar1_label_4: .globl bar1_label_4");
+  foo (4);
+  asm ("bar1_label_5: .globl bar1_label_5");
+}
+
+void
+bar2 (void)
+{
+  asm ("bar2_label: .globl bar2_label");
+  foo (1);
+  asm ("bar2_label_2: .globl bar2_label_2");
+  foo (2);
+  asm ("bar2_label_3: .globl bar2_label_3");
+  foo (3);
+  asm ("bar2_label_4: .globl bar2_label_4");
+  foo (4);
+  asm ("bar2_label_5: .globl bar2_label_5");
+}
+
+int
+main (void)
+{
+  asm ("main_label: .globl main_label");
+
+  bar1 ();
+
+  bar2 ();
+
+  return 0;
+}
+
diff --git a/gdb/testsuite/gdb.dwarf2/dw2-line-number-zero.exp b/gdb/testsuite/gdb.dwarf2/dw2-line-number-zero.exp
new file mode 100644
index 0000000000..e322f77708
--- /dev/null
+++ b/gdb/testsuite/gdb.dwarf2/dw2-line-number-zero.exp
@@ -0,0 +1,162 @@
+# Copyright 2020 Free Software Foundation, Inc.
+
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 3 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see <http://www.gnu.org/licenses/>.
+load_lib dwarf.exp
+
+# This test can only be run on targets which support DWARF-2 and use gas.
+if {![dwarf2_support]} {
+    verbose "Skipping dw2-line-number-zero test."
+    return 0
+}
+
+# The .c files use __attribute__.
+if [get_compiler_info] {
+    return -1
+}
+if !$gcc_compiled {
+    verbose "Skipping dw2-line-number-zero test."
+    return 0
+}
+
+standard_testfile .c dw2-line-number-zero-dw.S
+
+set asm_file [standard_output_file $srcfile2]
+Dwarf::assemble $asm_file {
+    global srcdir subdir srcfile srcfile2
+    declare_labels Llines
+
+    # Find start address and length for our functions.
+    set main_func \
+ [function_range main [list ${srcdir}/${subdir}/$srcfile]]
+    set main_start [lindex $main_func 0]
+    set main_length [lindex $main_func 1]
+
+    set bar1_func \
+ [function_range bar1 [list ${srcdir}/${subdir}/$srcfile]]
+    set bar1_start [lindex $bar1_func 0]
+    set bar1_length [lindex $bar1_func 1]
+
+    set bar2_func \
+ [function_range bar2 [list ${srcdir}/${subdir}/$srcfile]]
+    set bar2_start [lindex $bar2_func 0]
+    set bar2_length [lindex $bar2_func 1]
+
+    cu {} {
+ compile_unit {
+    {language @DW_LANG_C}
+    {name dw2-line-number-zero.c}
+    {stmt_list $Llines DW_FORM_sec_offset}
+ } {
+    subprogram {
+ {external 1 flag}
+ {name main}
+ {low_pc $main_start addr}
+ {high_pc "$main_start + $main_length" addr}
+    }
+    subprogram {
+ {external 1 flag}
+ {name bar1}
+ {low_pc $bar1_start addr}
+ {high_pc "$bar1_start + $bar1_length" addr}
+    }
+    subprogram {
+ {external 1 flag}
+ {name bar2}
+ {low_pc $bar2_start addr}
+ {high_pc "$bar2_start + $main_length" addr}
+    }
+ }
+    }
+
+    lines {version 2} Llines {
+ include_dir "${srcdir}/${subdir}"
+ file_name "$srcfile" 1
+
+ program {
+    {DW_LNE_set_address bar1_label}
+    {line 27}
+    {DW_LNS_copy}
+
+    {DW_LNE_set_address bar1_label_2}
+    {line 29}
+    {DW_LNS_copy}
+
+    {DW_LNE_set_address bar1_label_3}
+    {line 0}
+    {DW_LNS_copy}
+
+    {DW_LNE_set_address bar1_label_4}
+    {line 33}
+    {DW_LNS_copy}
+
+    {DW_LNE_set_address bar1_label_5}
+    {DW_LNE_end_sequence}
+
+
+    {DW_LNE_set_address bar2_label}
+    {line 41}
+    {DW_LNS_copy}
+
+    {DW_LNE_set_address bar2_label_2}
+    {line 43}
+    {DW_LNS_copy}
+
+    {DW_LNE_set_address bar2_label_3}
+    {line 0}
+    {DW_LNS_negate_stmt}
+    {DW_LNS_copy}
+    {DW_LNS_negate_stmt}
+
+    {DW_LNE_set_address bar2_label_4}
+    {line 47}
+    {DW_LNS_copy}
+
+    {DW_LNE_set_address bar2_label_5}
+    {DW_LNE_end_sequence}
+ }
+    }
+}
+
+if { [prepare_for_testing "failed to prepare" ${testfile} \
+  [list $srcfile $asm_file] {nodebug}] } {
+    return -1
+}
+
+if ![runto_main] {
+    return -1
+}
+
+gdb_breakpoint "bar1"
+gdb_continue_to_breakpoint "bar1" "\[^\r\n\]*:27\r\n.*"
+
+gdb_test "n" "foo \\(2\\);" "bar1, 1st next"
+gdb_test "n" "foo \\(4\\);" "bar1, 2nd next"
+
+gdb_breakpoint "bar2"
+gdb_continue_to_breakpoint "bar2" "\[^\r\n\]*:41\r\n.*"
+
+gdb_test "n" "foo \\(2\\);" "bar2, 1st next"
+gdb_test "n" "foo \\(4\\);" "bar2, 2nd next"
+
+if ![runto_main] {
+    return -1
+}
+
+gdb_breakpoint "bar1_label_3"
+setup_kfail "gdb/nnnnn" *-*-*
+gdb_continue_to_breakpoint "bar1_label_3" "bar1 \\(\\)"
+
+gdb_breakpoint "bar2_label_3"
+setup_kfail "gdb/nnnnn" *-*-*
+gdb_continue_to_breakpoint "bar2_label_3" "bar2 \\(\\)"
Reply | Threaded
Open this post in threaded view
|

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

Simon Marchi-4
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  <[hidden email]>
>
> PR symtab/26243
> * dwarf2/read.c (lnp_state_machine::record_line): Ignore zero line
> entries.
>
> gdb/testsuite/ChangeLog:
>
> 2020-07-16  Tom de Vries  <[hidden email]>
>
> 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.

Simon

[1] https://sourceware.org/pipermail/gdb-patches/2020-July/170588.html
[2] https://sourceware.org/pipermail/gdb-patches/2020-July/170645.html
Reply | Threaded
Open this post in threaded view
|

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

Pedro Alves-2
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  <[hidden email]>
>>
>> PR symtab/26243
>> * dwarf2/read.c (lnp_state_machine::record_line): Ignore zero line
>> entries.
>>
>> gdb/testsuite/ChangeLog:
>>
>> 2020-07-16  Tom de Vries  <[hidden email]>
>>
>> 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.

Thanks,
Pedro Alves
Reply | Threaded
Open this post in threaded view
|

Re: [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  <[hidden email]>
>>>
>>> PR symtab/26243
>>> * dwarf2/read.c (lnp_state_machine::record_line): Ignore zero line
>>> entries.
>>>
>>> gdb/testsuite/ChangeLog:
>>>
>>> 2020-07-16  Tom de Vries  <[hidden email]>
>>>
>>> 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
Reply | Threaded
Open this post in threaded view
|

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

Simon Marchi-4
On 2020-07-24 6:32 p.m., Tom de Vries wrote:
> 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

Ok, thanks to all.  So the plan is to revert this once the branch is cut and
apply the various patches (Pedro's and Andrew's, possibly more) necessary to
fix each spot not handling line == 0 well.

Simon