* Re: [PATCH 2/3] gdb: Don't reorder line table entries too much when sorting.
@ 2019-12-25 11:19 Bernd Edlinger
2019-12-26 22:17 ` Andrew Burgess
0 siblings, 1 reply; 8+ messages in thread
From: Bernd Edlinger @ 2019-12-25 11:19 UTC (permalink / raw)
To: Andrew Burgess, gdb-patches
Hi,
when I tried this patch, git am says this:
+foreach p $patterns {
+ gdb_test "step" "/\\* $p \\*/" \
+ "step to '$p'"
+}
+
--
2.14.5
Applying: gdb: Don't reorder line table entries too much when sorting.
/home/ed/gnu/binutils-gdb/.git/rebase-apply/patch:294: new blank line at EOF.
+
warning: 1 line adds whitespace errors.
I just was curious to try this patch series,
since this has some overlap with a patch I posted here:
https://sourceware.org/ml/gdb-patches/2019-11/msg00792.html
we both want to add end_sequence here:
> @@ -21330,7 +21331,8 @@ lnp_state_machine::record_line (bool end_sequence)
> else if (m_op_index == 0 || end_sequence)
> {
> fe->included_p = 1;
> - if (m_record_lines_p && (producer_is_codewarrior (m_cu) || m_is_stmt))
> + if (m_record_lines_p
> + && (producer_is_codewarrior (m_cu) || m_is_stmt || end_sequence))
> {
> if (m_last_subfile != m_cu->get_builder ()->get_current_subfile ()
> || end_sequence)
I was not sure, if m_is_stmt is ever false when end_sequence is true,
but considered that to be safer this way too.
Does that actually happen?
Bernd.
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 2/3] gdb: Don't reorder line table entries too much when sorting. 2019-12-25 11:19 [PATCH 2/3] gdb: Don't reorder line table entries too much when sorting Bernd Edlinger @ 2019-12-26 22:17 ` Andrew Burgess 2019-12-28 11:09 ` Bernd Edlinger 0 siblings, 1 reply; 8+ messages in thread From: Andrew Burgess @ 2019-12-26 22:17 UTC (permalink / raw) To: Bernd Edlinger; +Cc: gdb-patches * Bernd Edlinger <bernd.edlinger@hotmail.de> [2019-12-25 11:19:50 +0000]: > Hi, > > when I tried this patch, git am says this: > > +foreach p $patterns { > + gdb_test "step" "/\\* $p \\*/" \ > + "step to '$p'" > +} > + > -- > 2.14.5 > > Applying: gdb: Don't reorder line table entries too much when sorting. > /home/ed/gnu/binutils-gdb/.git/rebase-apply/patch:294: new blank line at EOF. > + > warning: 1 line adds whitespace errors. Thanks, I fixed two whitespace errors in this patch. > > > I just was curious to try this patch series, > since this has some overlap with a patch I posted here: > https://sourceware.org/ml/gdb-patches/2019-11/msg00792.html > > we both want to add end_sequence here: > > @@ -21330,7 +21331,8 @@ lnp_state_machine::record_line (bool end_sequence) > > else if (m_op_index == 0 || end_sequence) > > { > > fe->included_p = 1; > > - if (m_record_lines_p && (producer_is_codewarrior (m_cu) || m_is_stmt)) > > + if (m_record_lines_p > > + && (producer_is_codewarrior (m_cu) || m_is_stmt || end_sequence)) > > { > > if (m_last_subfile != m_cu->get_builder ()->get_current_subfile () > > || end_sequence) > > I was not sure, if m_is_stmt is ever false when end_sequence is true, > but considered that to be safer this way too. > Does that actually happen? Honestly, I didn't check. Like you it seemed to make more sense to leave things as they are unless I have a good reason to change them. Does how I wrote the test for this help you create a test for you patch at all? If I can help in any way I'd be happy to try. Thanks, Andrew ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 2/3] gdb: Don't reorder line table entries too much when sorting. 2019-12-26 22:17 ` Andrew Burgess @ 2019-12-28 11:09 ` Bernd Edlinger 0 siblings, 0 replies; 8+ messages in thread From: Bernd Edlinger @ 2019-12-28 11:09 UTC (permalink / raw) To: Andrew Burgess; +Cc: gdb-patches On 12/26/19 11:17 PM, Andrew Burgess wrote: > * Bernd Edlinger <bernd.edlinger@hotmail.de> [2019-12-25 11:19:50 +0000]: > >> Hi, >> >> when I tried this patch, git am says this: >> >> +foreach p $patterns { >> + gdb_test "step" "/\\* $p \\*/" \ >> + "step to '$p'" >> +} >> + >> -- >> 2.14.5 >> >> Applying: gdb: Don't reorder line table entries too much when sorting. >> /home/ed/gnu/binutils-gdb/.git/rebase-apply/patch:294: new blank line at EOF. >> + >> warning: 1 line adds whitespace errors. > > Thanks, I fixed two whitespace errors in this patch. > >> >> >> I just was curious to try this patch series, >> since this has some overlap with a patch I posted here: >> https://sourceware.org/ml/gdb-patches/2019-11/msg00792.html >> >> we both want to add end_sequence here: >>> @@ -21330,7 +21331,8 @@ lnp_state_machine::record_line (bool end_sequence) >>> else if (m_op_index == 0 || end_sequence) >>> { >>> fe->included_p = 1; >>> - if (m_record_lines_p && (producer_is_codewarrior (m_cu) || m_is_stmt)) >>> + if (m_record_lines_p >>> + && (producer_is_codewarrior (m_cu) || m_is_stmt || end_sequence)) >>> { >>> if (m_last_subfile != m_cu->get_builder ()->get_current_subfile () >>> || end_sequence) >> >> I was not sure, if m_is_stmt is ever false when end_sequence is true, >> but considered that to be safer this way too. >> Does that actually happen? > > Honestly, I didn't check. Like you it seemed to make more sense to > leave things as they are unless I have a good reason to change them. > Neither did I, but when I add an assertion there it is obvious that it happens rarely with gcc-4.8 but very often with gcc-10. > Does how I wrote the test for this help you create a test for you > patch at all? If I can help in any way I'd be happy to try. > Wow, that's a really impressive work... I was able to write a test for the step over inline issue, but it is dependent on the gcc version, gcc-8 or newer generate debug info where the problem exists, but test itself passes with all gcc versions as far as I can tell, it does just not prove much for old gcc versions. I posted everyting again here: https://sourceware.org/ml/gdb-patches/2019-12/msg01052.html In case you want to see. Thanks Bernd. ^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH 0/3] Improve inline frame debug experience @ 2019-12-23 1:51 Andrew Burgess 2019-12-23 1:51 ` [PATCH 2/3] gdb: Don't reorder line table entries too much when sorting Andrew Burgess 0 siblings, 1 reply; 8+ messages in thread From: Andrew Burgess @ 2019-12-23 1:51 UTC (permalink / raw) To: gdb-patches; +Cc: Andrew Burgess This series replaces 3 patches from gerrit: https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/525 https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/526 https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/616 I've done nothing but rebase and retest. All feedback welcome. Thanks, Andrew --- Andrew Burgess (3): gdb: Include end of sequence markers in the line table gdb: Don't reorder line table entries too much when sorting. gdb: Better frame tracking for inline frames gdb/ChangeLog | 31 ++ gdb/buildsym.c | 42 +-- gdb/dwarf2read.c | 8 +- gdb/frame.c | 9 +- gdb/inline-frame.c | 30 +- gdb/symmisc.c | 8 +- gdb/symtab.c | 7 +- gdb/testsuite/ChangeLog | 15 + gdb/testsuite/gdb.base/maint.exp | 20 +- gdb/testsuite/gdb.dwarf2/dw2-inline-many-frames.c | 158 +++++++++ .../gdb.dwarf2/dw2-inline-many-frames.exp | 379 +++++++++++++++++++++ gdb/testsuite/gdb.dwarf2/dw2-inline-stepping.c | 45 +++ gdb/testsuite/gdb.dwarf2/dw2-inline-stepping.exp | 147 ++++++++ gdb/testsuite/gdb.dwarf2/dw2-ranges-base.exp | 22 ++ 14 files changed, 870 insertions(+), 51 deletions(-) create mode 100644 gdb/testsuite/gdb.dwarf2/dw2-inline-many-frames.c create mode 100644 gdb/testsuite/gdb.dwarf2/dw2-inline-many-frames.exp create mode 100644 gdb/testsuite/gdb.dwarf2/dw2-inline-stepping.c create mode 100644 gdb/testsuite/gdb.dwarf2/dw2-inline-stepping.exp -- 2.14.5 ^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH 2/3] gdb: Don't reorder line table entries too much when sorting. 2019-12-23 1:51 [PATCH 0/3] Improve inline frame debug experience Andrew Burgess @ 2019-12-23 1:51 ` Andrew Burgess 2020-01-24 17:40 ` Tom Tromey 2020-06-05 14:49 ` Tom de Vries 0 siblings, 2 replies; 8+ messages in thread From: Andrew Burgess @ 2019-12-23 1:51 UTC (permalink / raw) To: gdb-patches; +Cc: Andrew Burgess Don't reorder line table entries for the same address when sorting the line table, maintain the compiler given line order. Usually this will reflect the order in which lines are conceptually encountered at a given address. Consider this example: /* 1 */ volatile int global_var; /* 2 */ int __attribute__ ((noinline)) /* 3 */ bar () /* 4 */ { /* 5 */ return global_var; /* 6 */ } /* 7 */ static inline int __attribute__ ((always_inline)) /* 8 */ foo () /* 9 */ { /* 10 */ return bar (); /* 11 */ } /* 12 */ int /* 13 */ main () /* 14 */ { /* 15 */ global_var = 0; /* 16 */ return foo (); /* 17 */ } GCC 10 currently generates a line table like this (as shown by objdump): CU: ./test.c: File name Line number Starting address test.c 4 0x4004b0 test.c 5 0x4004b0 test.c 6 0x4004b6 test.c 6 0x4004b7 test.c 14 0x4003b0 test.c 15 0x4003b0 test.c 16 0x4003ba test.c 10 0x4003ba test.c 10 0x4003c1 The interesting entries are those for lines 16 and 10 at address 0x4003ba, these represent the call to foo and the inlined body of foo. With the current line table sorting GDB builds the line table like this (as shown by 'maintenance info line-table'): INDEX LINE ADDRESS 0 14 0x00000000004003b0 1 15 0x00000000004003b0 2 10 0x00000000004003ba 3 16 0x00000000004003ba 4 END 0x00000000004003c1 5 4 0x00000000004004b0 6 5 0x00000000004004b0 7 END 0x00000000004004b7 Notice that entries 2 and 3 for lines 10 and 16 are now in a different order to the line table as given by the compiler. With this patch applied the order is now: INDEX LINE ADDRESS 0 14 0x00000000004003b0 1 15 0x00000000004003b0 2 16 0x00000000004003ba 3 10 0x00000000004003ba 4 END 0x00000000004003c1 5 4 0x00000000004004b0 6 5 0x00000000004004b0 7 END 0x00000000004004b7 Notice that entries 2 and 3 are now in their original order again. The consequence of the incorrect ordering is that when stepping through inlined functions GDB will display the wrong line for the inner most frame. Here's a GDB session before this patch is applied: Starting program: /home/andrew/tmp/inline/test Temporary breakpoint 1, main () at test.c:15 15 /* 15 */ global_var = 0; (gdb) step 16 /* 16 */ return foo (); (gdb) step foo () at test.c:16 16 /* 16 */ return foo (); (gdb) step bar () at test.c:5 5 /* 5 */ return global_var; The step from line 15 to 16 was fine, but the next step should have taken us to line 10, instead we are left at line 16. The final step to line 5 is as expected. With this patch applied the session goes better: Starting program: /home/andrew/tmp/inline/test Temporary breakpoint 1, main () at test.c:15 15 /* 15 */ global_var = 0; (gdb) step 16 /* 16 */ return foo (); (gdb) step foo () at test.c:10 10 /* 10 */ return bar (); (gdb) step bar () at test.c:5 5 /* 5 */ return global_var; We now visit the lines as 15, 16, 10, 5 as we would like. The reason for this issue is that the inline frame unwinder is detecting that foo is inlined in main. When we stop at the shared address 0x4003ba the inline frame unwinder first shows us the outer frame, this information is extracted from the DWARF's DW_TAG_inlined_subroutine entries and passed via GDB's block data. When we step again the inlined frame unwinder moves us up the call stack to the inner most frame at which point the frame is displayed as normal, with the location for the address being looked up in the line table. As GDB uses the last line table entry for an address as "the" line to report for that address it is critical that GDB maintain the order of the line table entries. In the first case, by reordering the line table we report the wrong location. 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. gdb/ChangeLog: * buildsym.c (lte_is_less_than): Delete. (buildsym_compunit::end_symtab_with_blockvector): Create local lambda function to sort line table entries, and use std::stable_sort instead of std::sort. * symtab.c (find_pc_sect_line): Skip backward over end of sequence markers when looking for a previous line. gdb/testsuite/ChangeLog: * gdb.dwarf2/dw2-inline-stepping.c: New file. * gdb.dwarf2/dw2-inline-stepping.exp: New file. Change-Id: Ia0309494be4cfd9dcc554f30209477f5f040b21b --- gdb/ChangeLog | 9 ++ gdb/buildsym.c | 42 +++---- gdb/symtab.c | 7 +- gdb/testsuite/ChangeLog | 5 + gdb/testsuite/gdb.dwarf2/dw2-inline-stepping.c | 45 +++++++ gdb/testsuite/gdb.dwarf2/dw2-inline-stepping.exp | 147 +++++++++++++++++++++++ 6 files changed, 230 insertions(+), 25 deletions(-) create mode 100644 gdb/testsuite/gdb.dwarf2/dw2-inline-stepping.c create mode 100644 gdb/testsuite/gdb.dwarf2/dw2-inline-stepping.exp diff --git a/gdb/buildsym.c b/gdb/buildsym.c index 79f83057634..2d131d07a4d 100644 --- a/gdb/buildsym.c +++ b/gdb/buildsym.c @@ -725,23 +725,6 @@ buildsym_compunit::record_line (struct subfile *subfile, int line, e->pc = pc; } -/* Needed in order to sort line tables from IBM xcoff files. Sigh! */ - -static bool -lte_is_less_than (const linetable_entry &ln1, const linetable_entry &ln2) -{ - /* Note: this code does not assume that CORE_ADDRs can fit in ints. - Please keep it that way. */ - if (ln1.pc < ln2.pc) - return true; - - if (ln1.pc > ln2.pc) - return false; - - /* If pc equal, sort by line. I'm not sure whether this is optimum - behavior (see comment at struct linetable in symtab.h). */ - return ln1.line < ln2.line; -} \f /* Subroutine of end_symtab to simplify it. Look for a subfile that matches the main source file's basename. If there is only one, and @@ -959,14 +942,25 @@ buildsym_compunit::end_symtab_with_blockvector (struct block *static_block, linetablesize = sizeof (struct linetable) + subfile->line_vector->nitems * sizeof (struct linetable_entry); - /* Like the pending blocks, the line table may be - scrambled in reordered executables. Sort it if - OBJF_REORDERED is true. */ + const auto lte_is_less_than + = [] (const linetable_entry &ln1, + const linetable_entry &ln2)->bool + { + /* Note: this code does not assume that CORE_ADDRs can fit + in ints. Please keep it that way. */ + return (ln1.pc < ln2.pc); + }; + + /* Like the pending blocks, the line table may be scrambled in + reordered executables. Sort it if OBJF_REORDERED is true. It + is important to preserve the order of lines at the same + address, as this maintains the inline function caller/callee + relationships, this is why std::stable_sort is used. */ if (m_objfile->flags & OBJF_REORDERED) - std::sort (subfile->line_vector->item, - subfile->line_vector->item - + subfile->line_vector->nitems, - lte_is_less_than); + std::stable_sort (subfile->line_vector->item, + subfile->line_vector->item + + subfile->line_vector->nitems, + lte_is_less_than); } /* Allocate a symbol table if necessary. */ diff --git a/gdb/symtab.c b/gdb/symtab.c index 26551372cbb..ee149b7642a 100644 --- a/gdb/symtab.c +++ b/gdb/symtab.c @@ -3219,7 +3219,12 @@ find_pc_sect_line (CORE_ADDR pc, struct obj_section *section, int notcurrent) struct linetable_entry *last = item + len; item = std::upper_bound (first, last, pc, pc_compare); if (item != first) - prev = item - 1; /* Found a matching item. */ + { + /* Found a matching item. Skip backwards over any end of + sequence markers. */ + for (prev = item - 1; prev->line == 0 && prev != first; prev--) + /* Nothing. */; + } /* At this point, prev points at the line whose start addr is <= pc, and item points at the next line. If we ran off the end of the linetable diff --git a/gdb/testsuite/gdb.dwarf2/dw2-inline-stepping.c b/gdb/testsuite/gdb.dwarf2/dw2-inline-stepping.c new file mode 100644 index 00000000000..41a89370c33 --- /dev/null +++ b/gdb/testsuite/gdb.dwarf2/dw2-inline-stepping.c @@ -0,0 +1,45 @@ +/* Copyright 2019 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/>. */ + +/* This test relies on foo being inlined into main and bar not being + inlined. The test is checking GDB's behaviour as we single step from + main through foo and into bar. */ + +volatile int global_var; + +int __attribute__ ((noinline)) +bar () +{ /* bar prologue */ + asm ("bar_label: .globl bar_label"); + return global_var; /* bar return global_var */ +} /* bar end */ + +static inline int __attribute__ ((always_inline)) +foo () +{ /* foo prologue */ + return bar (); /* foo call bar */ +} /* foo end */ + +int +main () +{ /* main prologue */ + int ans; + asm ("main_label: .globl main_label"); + global_var = 0; /* main set global_var */ + asm ("main_label2: .globl main_label2"); + ans = foo (); /* main call foo */ + asm ("main_label3: .globl main_label3"); + return ans; +} /* main end */ diff --git a/gdb/testsuite/gdb.dwarf2/dw2-inline-stepping.exp b/gdb/testsuite/gdb.dwarf2/dw2-inline-stepping.exp new file mode 100644 index 00000000000..64da28acec5 --- /dev/null +++ b/gdb/testsuite/gdb.dwarf2/dw2-inline-stepping.exp @@ -0,0 +1,147 @@ +# Copyright 2019 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/>. + +# This test shows the importance of not corrupting the order of line +# table information. When multiple lines are given for the same +# address the compiler usually lists these in the order in which we +# would expect to encounter them. When stepping through nested inline +# frames the last line given for an address is assumed by GDB to be +# the most inner frame, and this is what GDB displays. +# +# If we corrupt the order of the line table entries then GDB will +# display the wrong line as being the inner most frame. + +load_lib dwarf.exp + +# This test can only be run on targets which support DWARF-2 and use gas. +if {![dwarf2_support]} { + return 0 +} + +# The .c files use __attribute__. +if [get_compiler_info] { + return -1 +} +if !$gcc_compiled { + return 0 +} + +standard_testfile dw2-inline-stepping.c dw2-inline-stepping.S + +set asm_file [standard_output_file $srcfile2] +Dwarf::assemble $asm_file { + global srcdir subdir srcfile srcfile2 + declare_labels ranges_label lines_label foo_prog + + lassign [function_range main [list ${srcdir}/${subdir}/$srcfile]] \ + main_start main_len + set main_end "$main_start + $main_len" + lassign [function_range bar [list ${srcdir}/${subdir}/$srcfile]] \ + bar_start bar_len + set bar_end "$bar_start + $bar_len" + + set call_line [gdb_get_line_number "main call foo"] + + cu {} { + compile_unit { + {language @DW_LANG_C} + {name dw2-inline-stepping.c} + {low_pc 0 addr} + {stmt_list ${lines_label} DW_FORM_sec_offset} + {ranges ${ranges_label} DW_FORM_sec_offset} + } { + subprogram { + {external 1 flag} + {name bar} + {low_pc $bar_start addr} + {high_pc "$bar_start + $bar_len" addr} + } + foo_prog: subprogram { + {name foo} + {inline 3 data1} + } + subprogram { + {external 1 flag} + {name main} + {low_pc $main_start addr} + {high_pc "$main_start + $main_len" addr} + } { + inlined_subroutine { + {abstract_origin %$foo_prog} + {low_pc main_label2 addr} + {high_pc main_label3 addr} + {call_file 1 data1} + {call_line $call_line data1} + } + } + } + } + + lines {version 2} lines_label { + include_dir "${srcdir}/${subdir}" + file_name "$srcfile" 1 + + program { + {DW_LNE_set_address $main_start} + {DW_LNS_advance_line [expr [gdb_get_line_number "main prologue"] - 1]} + {DW_LNS_copy} + {DW_LNE_set_address main_label} + {DW_LNS_advance_line [expr [gdb_get_line_number "main set global_var"] - [gdb_get_line_number "main prologue"]]} + {DW_LNS_copy} + {DW_LNE_set_address main_label2} + {DW_LNS_advance_line [expr [gdb_get_line_number "main call foo"] - [gdb_get_line_number "main set global_var"]]} + {DW_LNS_copy} + {DW_LNE_set_address main_label2} + {DW_LNS_advance_line [expr [gdb_get_line_number "foo call bar"] - [gdb_get_line_number "main call foo"]]} + {DW_LNS_copy} + {DW_LNE_set_address $main_end} + {DW_LNE_end_sequence} + + {DW_LNE_set_address $bar_start} + {DW_LNS_advance_line [expr [gdb_get_line_number "bar prologue"] - 1]} + {DW_LNS_copy} + {DW_LNE_set_address bar_label} + {DW_LNS_advance_line [expr [gdb_get_line_number "bar return global_var"] - [gdb_get_line_number "bar prologue"]]} + {DW_LNS_copy} + {DW_LNE_set_address $bar_end} + {DW_LNE_end_sequence} + } + } + + ranges {is_64 [is_64_target]} { + ranges_label: sequence { + {range {${main_start}} ${main_end}} + {range {${bar_start}} ${bar_end}} + } + } +} + +if { [prepare_for_testing "failed to prepare" ${testfile} \ + [list $srcfile $asm_file] {nodebug}] } { + return -1 +} + +if ![runto_main] { + return -1 +} + +set patterns [list "main call foo" \ + "foo call bar" \ + "bar return global_var"] +foreach p $patterns { + gdb_test "step" "/\\* $p \\*/" \ + "step to '$p'" +} + -- 2.14.5 ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 2/3] gdb: Don't reorder line table entries too much when sorting. 2019-12-23 1:51 ` [PATCH 2/3] gdb: Don't reorder line table entries too much when sorting Andrew Burgess @ 2020-01-24 17:40 ` Tom Tromey 2020-06-05 6:10 ` Tom de Vries 2020-06-05 14:49 ` Tom de Vries 1 sibling, 1 reply; 8+ messages in thread From: Tom Tromey @ 2020-01-24 17:40 UTC (permalink / raw) To: Andrew Burgess; +Cc: gdb-patches >>>>> "Andrew" == Andrew Burgess <andrew.burgess@embecosm.com> writes: Andrew> Don't reorder line table entries for the same address when sorting the Andrew> line table, maintain the compiler given line order. Usually this will Andrew> reflect the order in which lines are conceptually encountered at a Andrew> given address. Thanks for the long explanation and the patch. I had a couple minor nits. Andrew> - /* Like the pending blocks, the line table may be Andrew> - scrambled in reordered executables. Sort it if Andrew> - OBJF_REORDERED is true. */ Andrew> + const auto lte_is_less_than Andrew> + = [] (const linetable_entry &ln1, Andrew> + const linetable_entry &ln2)->bool I'd put spaces around the "->". Andrew> + { Andrew> + /* Note: this code does not assume that CORE_ADDRs can fit Andrew> + in ints. Please keep it that way. */ Andrew> + return (ln1.pc < ln2.pc); I don't think this comment adds anything any more. IMO it can just be dropped. Tom ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 2/3] gdb: Don't reorder line table entries too much when sorting. 2020-01-24 17:40 ` Tom Tromey @ 2020-06-05 6:10 ` Tom de Vries 0 siblings, 0 replies; 8+ messages in thread From: Tom de Vries @ 2020-06-05 6:10 UTC (permalink / raw) To: Tom Tromey, Andrew Burgess; +Cc: gdb-patches On 24-01-2020 18:35, Tom Tromey wrote: >>>>>> "Andrew" == Andrew Burgess <andrew.burgess@embecosm.com> writes: > > Andrew> Don't reorder line table entries for the same address when sorting the > Andrew> line table, maintain the compiler given line order. Usually this will > Andrew> reflect the order in which lines are conceptually encountered at a > Andrew> given address. > > Thanks for the long explanation and the patch. > > I had a couple minor nits. > > Andrew> - /* Like the pending blocks, the line table may be > Andrew> - scrambled in reordered executables. Sort it if > Andrew> - OBJF_REORDERED is true. */ > Andrew> + const auto lte_is_less_than > Andrew> + = [] (const linetable_entry &ln1, > Andrew> + const linetable_entry &ln2)->bool > > I'd put spaces around the "->". > > > Andrew> + { > Andrew> + /* Note: this code does not assume that CORE_ADDRs can fit > Andrew> + in ints. Please keep it that way. */ > Andrew> + return (ln1.pc < ln2.pc); > > I don't think this comment adds anything any more. IMO it can just be > dropped. This commit caused the following regressions with target board readnow: ... +FAIL: gdb.ada/bp_c_mixed_case.exp: break <NoDebugMixedCaseFunc> +FAIL: gdb.arch/amd64-invalid-stack-top.exp: first backtrace, with error message +FAIL: gdb.arch/amd64-invalid-stack-top.exp: second backtrace, with error message +FAIL: gdb.arch/amd64-invalid-stack-top.exp: check mi -stack-list-frames command, first time +FAIL: gdb.arch/amd64-invalid-stack-top.exp: check mi -stack-list-frames command, second time +FAIL: gdb.arch/i386-bp_permanent.exp: single-step past permanent breakpoint +FAIL: gdb.base/gnu-ifunc.exp: resolver_attr=0: resolver_debug=0: final_debug=0: step +FAIL: gdb.base/gnu-ifunc.exp: resolver_attr=0: resolver_debug=0: final_debug=0: set-break: after resolving: break final +FAIL: gdb.base/gnu-ifunc.exp: resolver_attr=0: resolver_debug=0: final_debug=0: set-break: after resolving: break gnu_ifunc +FAIL: gdb.base/gnu-ifunc.exp: resolver_attr=0: resolver_debug=0: final_debug=0: set-break: after resolving: info breakpoints +FAIL: gdb.base/gnu-ifunc.exp: resolver_attr=0: resolver_debug=1: final_debug=0: step +FAIL: gdb.base/gnu-ifunc.exp: resolver_attr=0: resolver_debug=1: final_debug=0: set-break: after resolving: break final +FAIL: gdb.base/gnu-ifunc.exp: resolver_attr=0: resolver_debug=1: final_debug=0: set-break: after resolving: break gnu_ifunc +FAIL: gdb.base/gnu-ifunc.exp: resolver_attr=0: resolver_debug=1: final_debug=0: set-break: after resolving: info breakpoints +FAIL: gdb.base/gnu-ifunc.exp: resolver_attr=1: resolver_debug=0: final_debug=0: step +FAIL: gdb.base/gnu-ifunc.exp: resolver_attr=1: resolver_debug=0: final_debug=0: set-break: after resolving: break final +FAIL: gdb.base/gnu-ifunc.exp: resolver_attr=1: resolver_debug=0: final_debug=0: set-break: after resolving: break gnu_ifunc +FAIL: gdb.base/gnu-ifunc.exp: resolver_attr=1: resolver_debug=0: final_debug=0: set-break: after resolving: info breakpoints +FAIL: gdb.base/gnu-ifunc.exp: resolver_attr=1: resolver_debug=1: final_debug=0: step +FAIL: gdb.base/gnu-ifunc.exp: resolver_attr=1: resolver_debug=1: final_debug=0: set-break: after resolving: break final +FAIL: gdb.base/gnu-ifunc.exp: resolver_attr=1: resolver_debug=1: final_debug=0: set-break: after resolving: break gnu_ifunc +FAIL: gdb.base/gnu-ifunc.exp: resolver_attr=1: resolver_debug=1: final_debug=0: set-break: after resolving: info breakpoints +FAIL: gdb.base/solib-weak.exp: run to breakpoint - lib1 nodebug, lib2 nodebug, lib1 first +FAIL: gdb.base/solib-weak.exp: run to breakpoint - lib1 nodebug, lib2 nodebug, lib2 first +FAIL: gdb.base/solib-weak.exp: run to breakpoint - lib1 nodebug, lib2 debug, lib1 first +FAIL: gdb.base/solib-weak.exp: run to breakpoint - lib1 debug, lib2 nodebug, lib2 first +FAIL: gdb.base/step-symless.exp: step +FAIL: gdb.base/until-nodebug.exp: until 1 +FAIL: gdb.base/until-nodebug.exp: until 2 (the program exited) +FAIL: gdb.btrace/unknown_functions.exp: flat +FAIL: gdb.btrace/unknown_functions.exp: indented +FAIL: gdb.cp/minsym-fallback.exp: break C::f() +FAIL: gdb.cp/minsym-fallback.exp: break C::operator()() +FAIL: gdb.reverse/singlejmp-reverse.exp: reverse-step +FAIL: gdb.reverse/singlejmp-reverse.exp: reverse-next ... As well as the following progressions: ... +PASS: gdb.gdb/complaints.exp: breakpoint in captured_command_loop +PASS: gdb.gdb/python-interrupts.exp: breakpoint in captured_command_loop +PASS: gdb.gdb/python-selftest.exp: breakpoint in captured_command_loop +PASS: gdb.gdb/selftest.exp: breakpoint in captured_main ... The first regression is noted in PR25858 - "[readnow] FAIL: gdb.ada/bp_c_mixed_case.exp: break <NoDebugMixedCaseFunc>" ( https://sourceware.org/bugzilla/show_bug.cgi?id=25858 ). Thanks, - Tom ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 2/3] gdb: Don't reorder line table entries too much when sorting. 2019-12-23 1:51 ` [PATCH 2/3] gdb: Don't reorder line table entries too much when sorting Andrew Burgess 2020-01-24 17:40 ` Tom Tromey @ 2020-06-05 14:49 ` Tom de Vries 2020-06-05 16:00 ` Tom de Vries 1 sibling, 1 reply; 8+ messages in thread From: Tom de Vries @ 2020-06-05 14:49 UTC (permalink / raw) To: Andrew Burgess, gdb-patches 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. Note btw that the opposite does make sense: and end marker can have the same address as a following start-of-sequence entry. Thanks, - Tom ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 2/3] gdb: Don't reorder line table entries too much when sorting. 2020-06-05 14:49 ` Tom de Vries @ 2020-06-05 16:00 ` Tom de Vries 0 siblings, 0 replies; 8+ messages in thread From: Tom de Vries @ 2020-06-05 16:00 UTC (permalink / raw) To: Andrew Burgess, gdb-patches 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; + return (ln1.pc < ln2.pc); }; ... I get the expected: ... INDEX LINE ADDRESS IS-STMT 0 31 0x00000000004004a7 Y 1 END 0x00000000004004ae Y 2 21 0x00000000004004ae Y 3 END 0x00000000004004ba Y 4 11 0x00000000004004ba Y 5 END 0x00000000004004c6 Y ... Thanks, - Tom ^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2020-06-05 16:00 UTC | newest] Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2019-12-25 11:19 [PATCH 2/3] gdb: Don't reorder line table entries too much when sorting Bernd Edlinger 2019-12-26 22:17 ` Andrew Burgess 2019-12-28 11:09 ` Bernd Edlinger -- strict thread matches above, loose matches on Subject: below -- 2019-12-23 1:51 [PATCH 0/3] Improve inline frame debug experience Andrew Burgess 2019-12-23 1:51 ` [PATCH 2/3] gdb: Don't reorder line table entries too much when sorting Andrew Burgess 2020-01-24 17:40 ` Tom Tromey 2020-06-05 6:10 ` Tom de Vries 2020-06-05 14:49 ` Tom de Vries 2020-06-05 16:00 ` Tom de Vries
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).