]> git.ipfire.org Git - thirdparty/binutils-gdb.git/commitdiff
gdb: only update m_last_subfile after writing a line table entry
authorAndrew Burgess <aburgess@redhat.com>
Sat, 25 Jan 2025 13:00:12 +0000 (13:00 +0000)
committerAndrew Burgess <aburgess@redhat.com>
Fri, 14 Feb 2025 16:43:21 +0000 (16:43 +0000)
While working on another patch which changes how we parse the line
DWARF line tables I noticed what I think is a minor bug in how we
process the line tables.

What I noticed is that my new line table parser was adding more END
markers into the parsed table than GDB's current approach.  This
difference was observed when processing the debug information for
libstdc++.

Here is the line table from the new test, this is a reasonable
reproduction of the problem case that I observed in the actual debug
line table:

  Contents of the .debug_line section:

  dw2-skipped-line-entries-1.c:
  File name                        Line number    Starting address    View    Stmt
  dw2-skipped-line-entries-1.c             101            0x40110a               x

  /tmp/dw2-skipped-line-entries-2.c:
  dw2-skipped-line-entries-2.c             201            0x401114               x

  /tmp/dw2-skipped-line-entries-3.c:
  dw2-skipped-line-entries-3.c             301            0x40111e               x

  /tmp/dw2-skipped-line-entries-1.c:
  dw2-skipped-line-entries-1.c             102            0x401128               x
  dw2-skipped-line-entries-1.c             103            0x401128               x
  dw2-skipped-line-entries-1.c             104            0x401128               x

  /tmp/dw2-skipped-line-entries-2.c:
  dw2-skipped-line-entries-2.c             211            0x401128

  /tmp/dw2-skipped-line-entries-3.c:
  dw2-skipped-line-entries-3.c             311            0x401132

  /tmp/dw2-skipped-line-entries-1.c:
  dw2-skipped-line-entries-1.c             104            0x40113c
  dw2-skipped-line-entries-1.c             105            0x401146               x
  dw2-skipped-line-entries-1.c               -            0x401150

The problem is caused by the entry for line 211.  Notice that this
entry is at the same address as the previous entries.  Further, the
entry for 211 is a non-statement entry, while the previous entries are
statement entries.

As the entry for line 211 is a non-statement entry, and the previous
entries at that address are statement entries in a different symtab,
it is thought that it is better to prefer the earlier entries (in
dw2-skipped-line-entries-1.c), and so the entry for line 211 will be
discarded.

As GDB parses the line table it switches between the 3 symtabs (based
on source filename) adding the relevant entries to each symtab.
Additionally, as GDB switches symtabs, it adds an END entry to the
previous symtab.

The problem then is that, for the line 211 entry, this is the only
entry in dw2-skipped-line-entries-2.c before we switch symtab again.
But the line 211 entry is discarded.  This means that GDB switches
from dw2-skipped-line-entries-1.c to dw2-skipped-line-entries-2.c, and
then on to dw2-skipped-line-entries-3.c without ever adding an entry
to dw2-skipped-line-entries-2.c.

And here then is the bug.  GDB updates its idea of the previous symtab
not when an entry is written into a symtab, but every time we change
symtab.

In this case, when we switch to dw2-skipped-line-entries-3.c we add
the END marker to dw2-skipped-line-entries-2.c, even though no entries
were written to dw2-skipped-line-entries-2.c.  At the same time, no
END marker is ever written into dw2-skipped-line-entries-1.c as the
dw2-skipped-line-entries-2.c entry (for line 211) was discarded.

Here is the 'maint info line-table' for dw2-skipped-line-entries-1.c
before this patch:

  INDEX  LINE   REL-ADDRESS        UNREL-ADDRESS      IS-STMT PROLOGUE-END EPILOGUE-BEGIN
  0      101    0x000000000040110a 0x000000000040110a Y
  1      END    0x0000000000401114 0x0000000000401114 Y
  2      102    0x0000000000401128 0x0000000000401128 Y
  3      103    0x0000000000401128 0x0000000000401128 Y
  4      104    0x0000000000401128 0x0000000000401128 Y
  5      104    0x000000000040113c 0x000000000040113c
  6      105    0x0000000000401146 0x0000000000401146 Y
  7      END    0x0000000000401150 0x0000000000401150 Y

And after this patch:

  INDEX  LINE   REL-ADDRESS        UNREL-ADDRESS      IS-STMT PROLOGUE-END EPILOGUE-BEGIN
  0      101    0x000000000040110a 0x000000000040110a Y
  1      END    0x0000000000401114 0x0000000000401114 Y
  2      102    0x0000000000401128 0x0000000000401128 Y
  3      103    0x0000000000401128 0x0000000000401128 Y
  4      104    0x0000000000401128 0x0000000000401128 Y
  5      END    0x0000000000401132 0x0000000000401132 Y
  6      104    0x000000000040113c 0x000000000040113c
  7      105    0x0000000000401146 0x0000000000401146 Y
  8      END    0x0000000000401150 0x0000000000401150 Y

Notice that we gained an extra entry, the END marker that was added at
position #5 in the table.

Now, does this matter?  I cannot find any bugs that trigger because of
this behaviour.

So why fix it?  First, the current behaviour is inconsistent, as we
switch symtabs, we usually get an END marker in the previous symtab.
But occasionally we don't.  I don't like things that are inconsistent
for no good reason.  And second, as I said, I want to change the line
table parsing.  To do this I want to check that my new parser creates
an identical table to the current parser.  But my new parser naturally
"fixes" this inconsistency, so I have two choices, do extra work to
make my new parser bug-compatible with the current one, or fix the
current one.  I'd prefer to just fix the current line table parser.

There's a test that includes the above example and checks that the END
markers are put in the correct place.  But as I said, I've not been
able to trigger any negative behaviour from the current solution, so
there's no test that exposes any broken behaviour.

Approved-By: Tom Tromey <tom@tromey.com>
gdb/dwarf2/read.c
gdb/testsuite/gdb.dwarf2/dw2-skipped-line-entries.c [new file with mode: 0644]
gdb/testsuite/gdb.dwarf2/dw2-skipped-line-entries.exp [new file with mode: 0644]

index be33beac0f113a585af75cfb2f2a9b821fe72771..6bd9c4ddd70f92e381b6fdba34539ca20113a8ec 100644 (file)
@@ -17707,7 +17707,6 @@ lnp_state_machine::handle_set_file (file_name_index file)
     dwarf2_debug_line_missing_file_complaint ();
   else
     {
-      m_last_subfile = m_cu->get_builder ()->get_current_subfile ();
       m_line_has_non_zero_discriminator = m_discriminator != 0;
       dwarf2_start_subfile (m_cu, *fe, *m_line_header);
     }
@@ -17896,9 +17895,10 @@ lnp_state_machine::record_line (bool end_sequence)
                                   builder->get_current_subfile (),
                                   m_line, m_address, lte_flags,
                                   m_currently_recording_lines ? m_cu : nullptr);
+
+             m_last_subfile = m_cu->get_builder ()->get_current_subfile ();
+             m_last_line = m_line;
            }
-         m_last_subfile = m_cu->get_builder ()->get_current_subfile ();
-         m_last_line = m_line;
        }
     }
 
diff --git a/gdb/testsuite/gdb.dwarf2/dw2-skipped-line-entries.c b/gdb/testsuite/gdb.dwarf2/dw2-skipped-line-entries.c
new file mode 100644 (file)
index 0000000..d99d873
--- /dev/null
@@ -0,0 +1,50 @@
+/* Copyright 2025 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/>.  */
+
+/* Used to insert labels with which we can build a fake line table.  */
+#define LL(N) asm ("line_label_" #N ": .globl line_label_" #N)
+
+volatile int var;
+volatile int bar;
+
+/* Generate some code to take up some space.  */
+#define FILLER do { \
+    var = 99;      \
+} while (0)
+
+int
+main ()
+{
+  asm ("main_label: .globl main_label");
+  LL (1);
+  FILLER;
+  LL (2);
+  FILLER;
+  LL (3);
+  FILLER;
+  LL (4);
+  FILLER;
+  LL (5);
+  FILLER;
+  LL (6);
+  FILLER;
+  LL (7);
+  FILLER;
+  LL (8);
+  FILLER;
+  LL (9);
+  FILLER;
+  return 0;
+}
diff --git a/gdb/testsuite/gdb.dwarf2/dw2-skipped-line-entries.exp b/gdb/testsuite/gdb.dwarf2/dw2-skipped-line-entries.exp
new file mode 100644 (file)
index 0000000..2c2b5ff
--- /dev/null
@@ -0,0 +1,211 @@
+# Copyright 2025 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/>.
+
+# Check that END markers are placed into the correct symtabs line
+# table.  Of particular interest is the case where we switch symtabs
+# in order to add a line, but then decide that we shouldn't actually
+# record the new line because the new line is a non-statement line at
+# the same address as a previous statement line in a different symtab.
+#
+# If we then switch symtabs again we used to add the END marker to the
+# wrong symtab.
+#
+# For a (possibly) better explanation, see the comment on the line
+# table in the DWARF assembler block below.
+
+load_lib dwarf.exp
+
+# This test can only be run on targets which support DWARF-2 and use gas.
+require dwarf2_support
+
+# The -1.c, -2.c, and -3.c source files don't actually exist.  We just
+# want the filenames so we can use them in the generated DWARF.
+standard_testfile .c -dw.S -1.c -2.c -3.c
+
+set asm_file [standard_output_file $srcfile2]
+Dwarf::assemble $asm_file {
+    declare_labels lines_label
+
+    cu {} {
+       compile_unit {
+           {language @DW_LANG_C}
+           {name $::srcfile}
+           {low_pc 0 addr}
+           {stmt_list ${lines_label} DW_FORM_sec_offset}
+       } {
+           subprogram {
+               {external 1 flag}
+               {MACRO_AT_func {main}}
+           }
+       }
+    }
+
+    lines {version 2 default_is_stmt 1} lines_label {
+       include_dir "${::srcdir}/${::subdir}"
+       file_name "$::srcfile3" 1
+       file_name "$::srcfile4" 1
+       file_name "$::srcfile5" 1
+
+       # Generate the following line table:
+       #
+       # Index | Address | Symtab | Line | Statment
+       # ------|---------|--------|------------
+       #  0    |  A1     | F1     | 101  | X
+       #  1    |  A2     | F2     | 201  | X
+       #  2    |  A3     | F3     | 301  | X
+       #  3    |  A4     | F1     | 102  | X
+       #  4    |  A4     | F1     | 103  | X
+       #  5    |  A4     | F1     | 104  | X
+       #  6    |  A4     | F2     | 211  |
+       #  7    |  A5     | F3     | 311  |
+       #  8    |  A6     | F1     | 104  |
+       #  9    |  A7     | F1     | 105  | X
+       #  10   |  A8     | F1     | END  |
+       #
+       # GDB switches from F1 to F2 in order to add the entry at
+       # index 6, however the entry at index 6 is a non-statement
+       # line at the same address as entries 3, 4, and 5.
+       #
+       # If GDB then added an END marker in F1 at A4 in order to add
+       # entry 6 in F2, then END marker in F1 would effectively
+       # prevent the use of entries 3, 4, and 5.
+       #
+       # To avoid this, GDB discards entry 6.
+       #
+       # When GDB then switches to F3 to add entry 7, which is at a
+       # different address, GDB finally gets to add an END marker to
+       # the previous symtab's line table.
+       #
+       # This END marker should go into F1, that is, after all, the
+       # last symtab that had line table entries added too it.
+       #
+       # At one point, a bug in GDB meant that we added the END
+       # marker into F2, but we never actually added any entries to
+       # F2, so the END marker was redundant.
+
+       program {
+           DW_LNE_set_address line_label_1
+           DW_LNS_advance_line 100
+           DW_LNS_copy
+
+           DW_LNS_set_file 2
+           DW_LNE_set_address line_label_2
+           DW_LNS_advance_line 100
+           DW_LNS_copy
+
+           DW_LNS_set_file 3
+           DW_LNE_set_address line_label_3
+           DW_LNS_advance_line 100
+           DW_LNS_copy
+
+           DW_LNS_set_file 1
+           DW_LNE_set_address line_label_4
+           DW_LNS_advance_line -199
+           DW_LNS_copy
+
+           DW_LNE_set_address line_label_4
+           DW_LNS_advance_line 1
+           DW_LNS_copy
+
+           DW_LNE_set_address line_label_4
+           DW_LNS_advance_line 1
+           DW_LNS_copy
+
+           DW_LNS_set_file 2
+           DW_LNE_set_address line_label_4
+           DW_LNS_advance_line 107
+           DW_LNS_negate_stmt
+           DW_LNS_copy
+
+           DW_LNS_set_file 3
+           DW_LNE_set_address line_label_5
+           DW_LNS_advance_line 100
+           DW_LNS_copy
+
+           DW_LNS_set_file 1
+           DW_LNE_set_address line_label_6
+           DW_LNS_advance_line -207
+           DW_LNS_copy
+
+           DW_LNE_set_address line_label_7
+           DW_LNS_advance_line 1
+           DW_LNS_negate_stmt
+           DW_LNS_copy
+
+           DW_LNE_set_address line_label_8
+           DW_LNE_end_sequence
+       }
+    }
+}
+
+if { [prepare_for_testing "failed to prepare" ${testfile} \
+         [list $srcfile $asm_file] {nodebug}] } {
+    return -1
+}
+
+if { ![runto_main] } {
+    return -1
+}
+
+# Check the END markers using 'maint info line-table'.
+# EXPECTED_SYMTAB is the basename of the source file being checked.
+# TEST_SPEC is a list of lists.  Each inner list if a pair made of a
+# line table index number and a string, the name of a variable in the
+# parent scope that holds the address we expect the END marker to
+# appear at.
+proc check_end_markers { expected_symtab test_spec } {
+    foreach spec $test_spec {
+       set index [lindex $spec 0]
+       set varname [lindex $spec 1]
+
+       upvar 1 $varname upvar_addr
+       set addr [string range $upvar_addr 2 end]
+
+       verbose -log "APB: index: $index, addr: $addr"
+
+       set symtab ""
+       set saw_line false
+       gdb_test_multiple "maint info line-table" \
+           "check END marker at index $index in $expected_symtab" {
+           -re "^symtab: \[^\r\n\]+/($::testfile-$::decimal.c) \\(\\(\[^\r\n\]+\r\n" {
+               set symtab $expect_out(1,string)
+               exp_continue
+           }
+           -re "^${index}\\s+END\\s+0x0*$addr\\s+$::hex\\s+Y\\s*\r\n" {
+               if { $symtab eq $expected_symtab } {
+                   set saw_line true
+               }
+               exp_continue
+           }
+           -re "^$::gdb_prompt $" {
+               gdb_assert $saw_line $gdb_test_name
+           }
+           -re "^\[^\r\n\]*\r\n" {
+               exp_continue
+           }
+       }
+    }
+}
+
+set addr_2 [get_hexadecimal_valueof "&line_label_2" "UNKNOWN"]
+set addr_3 [get_hexadecimal_valueof "&line_label_3" "UNKNOWN"]
+set addr_4 [get_hexadecimal_valueof "&line_label_4" "UNKNOWN"]
+set addr_5 [get_hexadecimal_valueof "&line_label_5" "UNKNOWN"]
+set addr_6 [get_hexadecimal_valueof "&line_label_6" "UNKNOWN"]
+set addr_8 [get_hexadecimal_valueof "&line_label_8" "UNKNOWN"]
+
+check_end_markers "$testfile-1.c" { { 1 addr_2 } { 5 addr_5} { 8 addr_8 } }
+check_end_markers "$testfile-2.c" { { 1 addr_3 } }
+check_end_markers "$testfile-3.c" { { 1 addr_4 } { 3 addr_6} }