From: Andrew Burgess Date: Sat, 25 Jan 2025 13:00:12 +0000 (+0000) Subject: gdb: only update m_last_subfile after writing a line table entry X-Git-Url: http://git.ipfire.org/?a=commitdiff_plain;h=17c89ffc0a5ccedc3547b9f1578f2a3630d7ef15;p=thirdparty%2Fbinutils-gdb.git gdb: only update m_last_subfile after writing a line table entry 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 --- diff --git a/gdb/dwarf2/read.c b/gdb/dwarf2/read.c index be33beac0f1..6bd9c4ddd70 100644 --- a/gdb/dwarf2/read.c +++ b/gdb/dwarf2/read.c @@ -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 index 00000000000..d99d8734b33 --- /dev/null +++ b/gdb/testsuite/gdb.dwarf2/dw2-skipped-line-entries.c @@ -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 . */ + +/* 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 index 00000000000..2c2b5ffc744 --- /dev/null +++ b/gdb/testsuite/gdb.dwarf2/dw2-skipped-line-entries.exp @@ -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 . + +# 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} }