* [PATCH] Fix an issue with the gdb step-over aka. "n" command @ 2019-11-24 12:17 Bernd Edlinger 2019-12-01 20:47 ` [PING] " Bernd Edlinger 2019-12-15 1:25 ` Simon Marchi 0 siblings, 2 replies; 14+ messages in thread From: Bernd Edlinger @ 2019-11-24 12:17 UTC (permalink / raw) To: gdb-patches [-- Attachment #1: Type: text/plain, Size: 2328 bytes --] Hi, this fixes an issue with the gdb step-over aka. "n" command. Apologies, the motivation for this patch was from sub-optimal debug experience using some gcc code involving inlined functions, and initially I tried to convince gcc folks that it is in fact a gcc bug, but... It can be seen when you debug an optimized stage-3 cc1 it does not affect -O0 code, though. Note: you can use "gcc -S hello.c -wrapper gdb,--args" to invoke cc1 with debugger attached. This example debug session will explain the effect. (gdb) b get_alias_set Breakpoint 5 at 0xa099f0: file ../../gcc-trunk/gcc/alias.c, line 837. (gdb) r Breakpoint 5, get_alias_set (t=t@entry=0x7ffff7ff7ab0) at ../../gcc-trunk/gcc/alias.c:837 837 if (t == error_mark_node (gdb) n 839 && (TREE_TYPE (t) == 0 || TREE_TYPE (t) == error_mark_node))) (gdb) n 3382 return __t; <-- now we have a problem: wrong line info here (gdb) bt #0 get_alias_set (t=t@entry=0x7ffff7ff7ab0) at ../../gcc-trunk/gcc/tree.h:3382 #1 0x0000000000b25dfe in set_mem_attributes_minus_bitpos (ref=0x7ffff746f990, t=0x7ffff7ff7ab0, objectp=1, bitpos=...) at ../../gcc-trunk/gcc/emit-rtl.c:1957 #2 0x0000000001137a55 in make_decl_rtl (decl=0x7ffff7ff7ab0) at ../../gcc-trunk/gcc/varasm.c:1518 #3 0x000000000113b6e8 in assemble_variable (decl=0x7ffff7ff7ab0, top_level=<optimized out>, at_end=<optimized out>, dont_output_data=0) at ../../gcc-trunk/gcc/varasm.c:2246 #4 0x000000000113f0ea in varpool_node::assemble_decl (this=0x7ffff745b000) at ../../gcc-trunk/gcc/varpool.c:584 #5 0x000000000113fa17 in varpool_node::assemble_decl (this=0x7ffff745b000) at ../../gcc-trunk/gcc/varpool.c:750 The reason for this is a line number information that is exactly at the end of the inlined function, but there is no code at that place, only variable values (views) are declared there. Unfortunately the next instruction is again in the main program, but due to -gstatement-frontiers those do not have the is_stmt and are completely ignored by gdb at the moment. This patch fixes the effect by rewriting the is_stmt attribute of the next location info under certain conditions. I have no idea how to write a test case for this since it happens only in optimized code, and only under very special conditions. Thanks Bernd. [-- Warning: decoded text below may be mangled, UTF-8 assumed --] [-- Attachment #2: 0001-Fix-an-issue-with-the-gdb-step-over-aka.-n-command.patch --] [-- Type: text/x-patch; name="0001-Fix-an-issue-with-the-gdb-step-over-aka.-n-command.patch", Size: 2269 bytes --] From 18017c39f9598dcd8e3204afd624afd2ac723301 Mon Sep 17 00:00:00 2001 From: Bernd Edlinger <bernd.edlinger@hotmail.de> Date: Sat, 23 Nov 2019 20:59:25 +0100 Subject: [PATCH] Fix an issue with the gdb step-over aka. "n" command When debugging optimized code with inlined functions built with -gstatement-frontiers debug info, it may happen that an inline function has a line location exactly at the end of a block which has the is_stmt attribute set, followed by a location info which is at the call site but has the is_stmt attribute cleared and is therefore ignored. The visual effect is that "n" does sometimes not step over the inlined function but instead jumps to the last line of the inline function, but the inline function has already returned, hence the line number information is inconsistent at this point. This patch detects a is_stmt location info followed by a location info in a different file at the same address, but without is_stmt location set, and forces the second location info to replace the previous one. --- gdb/dwarf2read.c | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/gdb/dwarf2read.c b/gdb/dwarf2read.c index 1ca801c..a7c4c8e 100644 --- a/gdb/dwarf2read.c +++ b/gdb/dwarf2read.c @@ -20915,6 +20915,7 @@ private: example, when discriminators are present. PR 17276. */ unsigned int m_last_line = 0; bool m_line_has_non_zero_discriminator = false; + CORE_ADDR m_last_address = (CORE_ADDR) -1; }; void @@ -21097,7 +21098,10 @@ 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 + || (m_last_subfile != m_cu->get_builder ()->get_current_subfile () + && m_last_address == m_address))) { if (m_last_subfile != m_cu->get_builder ()->get_current_subfile () || end_sequence) @@ -21120,6 +21124,7 @@ lnp_state_machine::record_line (bool end_sequence) } m_last_subfile = m_cu->get_builder ()->get_current_subfile (); m_last_line = m_line; + m_last_address = m_address; } } } -- 1.9.1 ^ permalink raw reply [flat|nested] 14+ messages in thread
* [PING] [PATCH] Fix an issue with the gdb step-over aka. "n" command 2019-11-24 12:17 [PATCH] Fix an issue with the gdb step-over aka. "n" command Bernd Edlinger @ 2019-12-01 20:47 ` Bernd Edlinger 2019-12-14 13:52 ` [PING**2] " Bernd Edlinger 2019-12-15 1:25 ` Simon Marchi 1 sibling, 1 reply; 14+ messages in thread From: Bernd Edlinger @ 2019-12-01 20:47 UTC (permalink / raw) To: gdb-patches Ping... On 11/24/19 1:17 PM, Bernd Edlinger wrote: > Hi, > > this fixes an issue with the gdb step-over aka. "n" command. > > Apologies, the motivation for this patch was from sub-optimal > debug experience using some gcc code involving inlined functions, > and initially I tried to convince gcc folks that it is in fact a > gcc bug, but... > > It can be seen when you debug an optimized stage-3 cc1 > it does not affect -O0 code, though. > > Note: you can use "gcc -S hello.c -wrapper gdb,--args" to invoke cc1 with > debugger attached. > > This example debug session will explain the effect. > > (gdb) b get_alias_set > Breakpoint 5 at 0xa099f0: file ../../gcc-trunk/gcc/alias.c, line 837. > (gdb) r > Breakpoint 5, get_alias_set (t=t@entry=0x7ffff7ff7ab0) at ../../gcc-trunk/gcc/alias.c:837 > 837 if (t == error_mark_node > (gdb) n > 839 && (TREE_TYPE (t) == 0 || TREE_TYPE (t) == error_mark_node))) > (gdb) n > 3382 return __t; <-- now we have a problem: wrong line info here > (gdb) bt > #0 get_alias_set (t=t@entry=0x7ffff7ff7ab0) at ../../gcc-trunk/gcc/tree.h:3382 > #1 0x0000000000b25dfe in set_mem_attributes_minus_bitpos (ref=0x7ffff746f990, t=0x7ffff7ff7ab0, objectp=1, bitpos=...) > at ../../gcc-trunk/gcc/emit-rtl.c:1957 > #2 0x0000000001137a55 in make_decl_rtl (decl=0x7ffff7ff7ab0) at ../../gcc-trunk/gcc/varasm.c:1518 > #3 0x000000000113b6e8 in assemble_variable (decl=0x7ffff7ff7ab0, top_level=<optimized out>, at_end=<optimized out>, > dont_output_data=0) at ../../gcc-trunk/gcc/varasm.c:2246 > #4 0x000000000113f0ea in varpool_node::assemble_decl (this=0x7ffff745b000) at ../../gcc-trunk/gcc/varpool.c:584 > #5 0x000000000113fa17 in varpool_node::assemble_decl (this=0x7ffff745b000) at ../../gcc-trunk/gcc/varpool.c:750 > > The reason for this is a line number information that is exactly at > the end of the inlined function, but there is no code at that place, > only variable values (views) are declared there. Unfortunately > the next instruction is again in the main program, but due to -gstatement-frontiers > those do not have the is_stmt and are completely ignored by gdb at the moment. > > > This patch fixes the effect by rewriting the is_stmt attribute of the next > location info under certain conditions. > > I have no idea how to write a test case for this since it happens only in optimized code, > and only under very special conditions. > > > Thanks > Bernd. > ^ permalink raw reply [flat|nested] 14+ messages in thread
* [PING**2] [PATCH] Fix an issue with the gdb step-over aka. "n" command 2019-12-01 20:47 ` [PING] " Bernd Edlinger @ 2019-12-14 13:52 ` Bernd Edlinger 2019-12-30 22:12 ` Andrew Burgess 2020-01-06 8:14 ` [PING**3] " Bernd Edlinger 0 siblings, 2 replies; 14+ messages in thread From: Bernd Edlinger @ 2019-12-14 13:52 UTC (permalink / raw) To: gdb-patches Ping... I'm pinging for this patch here: https://sourceware.org/ml/gdb-patches/2019-11/msg00792.html Thanks Bernd. On 12/1/19 9:47 PM, Bernd Edlinger wrote: > Ping... > > > On 11/24/19 1:17 PM, Bernd Edlinger wrote: >> Hi, >> >> this fixes an issue with the gdb step-over aka. "n" command. >> >> Apologies, the motivation for this patch was from sub-optimal >> debug experience using some gcc code involving inlined functions, >> and initially I tried to convince gcc folks that it is in fact a >> gcc bug, but... >> >> It can be seen when you debug an optimized stage-3 cc1 >> it does not affect -O0 code, though. >> >> Note: you can use "gcc -S hello.c -wrapper gdb,--args" to invoke cc1 with >> debugger attached. >> >> This example debug session will explain the effect. >> >> (gdb) b get_alias_set >> Breakpoint 5 at 0xa099f0: file ../../gcc-trunk/gcc/alias.c, line 837. >> (gdb) r >> Breakpoint 5, get_alias_set (t=t@entry=0x7ffff7ff7ab0) at ../../gcc-trunk/gcc/alias.c:837 >> 837 if (t == error_mark_node >> (gdb) n >> 839 && (TREE_TYPE (t) == 0 || TREE_TYPE (t) == error_mark_node))) >> (gdb) n >> 3382 return __t; <-- now we have a problem: wrong line info here >> (gdb) bt >> #0 get_alias_set (t=t@entry=0x7ffff7ff7ab0) at ../../gcc-trunk/gcc/tree.h:3382 >> #1 0x0000000000b25dfe in set_mem_attributes_minus_bitpos (ref=0x7ffff746f990, t=0x7ffff7ff7ab0, objectp=1, bitpos=...) >> at ../../gcc-trunk/gcc/emit-rtl.c:1957 >> #2 0x0000000001137a55 in make_decl_rtl (decl=0x7ffff7ff7ab0) at ../../gcc-trunk/gcc/varasm.c:1518 >> #3 0x000000000113b6e8 in assemble_variable (decl=0x7ffff7ff7ab0, top_level=<optimized out>, at_end=<optimized out>, >> dont_output_data=0) at ../../gcc-trunk/gcc/varasm.c:2246 >> #4 0x000000000113f0ea in varpool_node::assemble_decl (this=0x7ffff745b000) at ../../gcc-trunk/gcc/varpool.c:584 >> #5 0x000000000113fa17 in varpool_node::assemble_decl (this=0x7ffff745b000) at ../../gcc-trunk/gcc/varpool.c:750 >> >> The reason for this is a line number information that is exactly at >> the end of the inlined function, but there is no code at that place, >> only variable values (views) are declared there. Unfortunately >> the next instruction is again in the main program, but due to -gstatement-frontiers >> those do not have the is_stmt and are completely ignored by gdb at the moment. >> >> >> This patch fixes the effect by rewriting the is_stmt attribute of the next >> location info under certain conditions. >> >> I have no idea how to write a test case for this since it happens only in optimized code, >> and only under very special conditions. >> >> >> Thanks >> Bernd. >> ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PING**2] [PATCH] Fix an issue with the gdb step-over aka. "n" command 2019-12-14 13:52 ` [PING**2] " Bernd Edlinger @ 2019-12-30 22:12 ` Andrew Burgess 2020-01-01 9:40 ` Bernd Edlinger 2020-01-06 8:14 ` [PING**3] " Bernd Edlinger 1 sibling, 1 reply; 14+ messages in thread From: Andrew Burgess @ 2019-12-30 22:12 UTC (permalink / raw) To: Bernd Edlinger; +Cc: gdb-patches, Simon Marchi I've taken a look through this patch, and I think that we should consider adding real support for the line table is_stmt attribute to GDB as an alternative. I had a crack at doing this and it fixes the test case as you originally describe it, with the inline function defined in the header file, and the caller in the .cc file - however it still doesn't fix the case where the inline function is in the same source file as the caller, however, I'm convinced this is due to incorrect DWARF being generated by GCC rather than a fault of GDB. As a test case I'm using next-inline2.cc, which I've included below, I compile this using GCC 9.2.0 though I've also tested 10.xx, and 8.3.0 which are no different. I also tried 7.4.0 which is different, and I'll discuss below. The interesting part of the source file is as follows, with line numbers on the left for reference: 28: inline tree * 29: tree_check (tree *t, int i) 30: { 31: if (t->x != i) 32: abort(); 33: tree *x = t; 34: return x; 35: } 36: 37: int __attribute__((noinline, noclone)) 38: get_alias_set (tree *t) 39: { 40: if (t != NULL 41: && TREE_TYPE (t).z != 1 42: && TREE_TYPE (t).z != 2 43: && TREE_TYPE (t).z != 3) 44: return 0; 45: return 1; 46: } I compile the test with 'g++ -O2 -g3 -o next-inline2 next-inline2.cc', and the decoded line table (or part of it) looks like: Contents of the .debug_line section: CU: ./next-inline2.cc: File name Line number Starting address View Stmt next-inline2.cc 39 0x400540 x next-inline2.cc 40 0x400540 1 x next-inline2.cc 43 0x400540 2 next-inline2.cc 31 0x400545 x next-inline2.cc 39 0x400545 1 next-inline2.cc 31 0x400549 next-inline2.cc 31 0x40054b next-inline2.cc 33 0x400553 x next-inline2.cc 34 0x400553 1 x next-inline2.cc 41 0x400553 2 next-inline2.cc 41 0x400556 next-inline2.cc 31 0x40055b x next-inline2.cc 31 0x40055b 1 next-inline2.cc 31 0x40055d next-inline2.cc 33 0x400565 x next-inline2.cc 34 0x400565 1 x next-inline2.cc 42 0x400565 2 next-inline2.cc 42 0x400568 The other interesting thing we need to understand is where the inline subroutines are, in the source code above the inline subroutines are hidden behind the TREE_TYPE macro. Looking at the DWARF we see 3 instances of DW_TAG_inlined_subroutine, they are: <2><8e8>: Abbrev Number: 39 (DW_TAG_inlined_subroutine) <8e9> DW_AT_abstract_origin: <0x9a2> <8ed> DW_AT_entry_pc : 0x400545 <8f5> DW_AT_ranges : 0x30 <8f9> DW_AT_call_file : 1 <8fa> DW_AT_call_line : 41 <8fb> DW_AT_call_column : 10 <8fc> DW_AT_sibling : <0x92f> <2><92f>: Abbrev Number: 43 (DW_TAG_inlined_subroutine) <930> DW_AT_abstract_origin: <0x9a2> <934> DW_AT_low_pc : 0x40055b <93c> DW_AT_high_pc : 0xa <944> DW_AT_call_file : 1 <945> DW_AT_call_line : 42 <946> DW_AT_call_column : 10 <947> DW_AT_sibling : <0x967> <2><967>: Abbrev Number: 44 (DW_TAG_inlined_subroutine) <968> DW_AT_abstract_origin: <0x9a2> <96c> DW_AT_low_pc : 0x40056d <974> DW_AT_high_pc : 0xa <97c> DW_AT_call_file : 1 <97d> DW_AT_call_line : 43 <97e> DW_AT_call_column : 10 Notice in all cases we reference abstract origin 0x9a2, this is the same subroutine inlined 3 times. Also notice the difference with how the address ranges are defined. In the last two cases the address ranges for the inlined subroutines start at 0x40055b and 0x40056d and continue for 10 bytes. In the first case however, the address range is in the .debug_ranges table, and looks like this: 00000030 0000000000400545 0000000000400545 (start == end) 00000030 0000000000400549 0000000000400553 00000030 0000000000400430 0000000000400435 00000030 <End of list> Here we have the first clue that something has gone wrong, the 'start == end' warning tells us that there's at the very least a dummy entry n in the ranges table (could happen with relaxation maybe, or possibly linker garbage collection, but its still a warning sign I think). So the next job is to try and merge all this information into one table and see what we can learn. In the following table I use the following short-hand: 'In' column indicates a range of addresses covered by an inline function range. 'Ln' column contains line table information, where: + A number e.g. '39', indicates line 39 is specified to be at this address in the line table, and is a statement, that is a good place to insert a breakpoint. + An underscore and number, e.g. '_43' indicates line 43 is specified to be at this address in the line table, but this is not a statement, that is, this is not a good place to put a breakpoint if you want to stop at this line. + A colon and number, e.g. ':43' indicates we infer that line 43 is here based on the last entry that was in the line table, but there is no direct entry for this address in the line table. 'Address' and 'Disassembly Text' are just from the disassembly output. | In | Ln | Address | Disassembly Text | |----+------------+----------------------------+-----------------------------------------------| | | 39,40,_43 | 0x0000000000400540 <+0>: | test %rdi,%rdi | | | :43 | 0x0000000000400543 <+3>: | je 0x400590 <get_alias_set(tree*)+80> | | | 31, _39 | 0x0000000000400545 <+5>: | sub $0x8,%rsp | | X | _31 | 0x0000000000400549 <+9>: | mov (%rdi),%eax | | X | _31 | 0x000000000040054b <+11>: | test %eax,%eax | | X | :31 | 0x000000000040054d <+13>: | jne 0x400430<_Z13get_alias_setP4tree.cold> | | | 33,34,_41 | 0x0000000000400553 <+19>: | mov 0x4(%rdi),%eax | | | _41 | 0x0000000000400556 <+22>: | cmp $0x1,%eax | | | :41 | 0x0000000000400559 <+25>: | je 0x400583 <get_alias_set(tree*)+67> | | X | 31,_31 | 0x000000000040055b <+27>: | mov (%rdi),%eax | | X | _31 | 0x000000000040055d <+29>: | test %eax,%eax | | X | :31 | 0x000000000040055f <+31>: | jne 0x400430<_Z13get_alias_setP4tree.cold> | | | 33, 34,_42 | 0x0000000000400565 <+37>: | mov 0x4(%rdi),%eax | | | | 0x0000000000400568 <+40>: | cmp $0x2,%eax | | | | 0x000000000040056b <+43>: | je 0x4005a0 <get_alias_set(tree*)+96> | | X | | 0x000000000040056d <+45>: | mov (%rdi),%eax | | X | | 0x000000000040056f <+47>: | test %eax,%eax | | X | | 0x0000000000400571 <+49>: | jne 0x400430<_Z13get_alias_setP4tree.cold> | | | | 0x0000000000400577 <+55>: | mov 0x4(%rdi),%eax | | | | 0x000000000040057a <+58>: | cmp $0x3,%eax | | | | 0x000000000040057d <+61>: | sete %al | My claim is this, in well structured DWARF and entries for lines 31, 32 33, or 34 should be inside an area marked as belonging to the inline subroutine, or at a minimum, should not be marked as a statement and be outside an inlined region. Take for example address 0x400545, this is a statement line for 31, and a non-statement line for 39, but is NOT in an inline subroutine. The DWARF is (I claim) clearly indicating that address 0x400545 comes from line 31, and is in the function get_alias_set. By looking at the source code we can clearly see this is not correct. There are other inconsistencies, like 0x400553, which claims to be line 33 and 34, yet is outside the inlined range. I'm hoping that someone can double check my analysis of the above and call me out on anything I've gotten wrong. I wont run the full analysis again, but I mentioned previously that I tried GCC 7.4.0. The debug information generated here was not as sophisticated, but nor was it (I think) so wrong. The debug range for the first inlined subroutine entry makes much more sense, there's no longer the silly 'start == end' thing going on, and the line table is much simpler. Consequently the debug experience, though not perfect, is much better than on any of the later compiler - and this is with my is_stmt supporting GDB. I've included below the is_stmt support patch as I currently have it, but please remember, this is a very quick hack just so anyone interested can test it. I'm going to try and clean it up and repost it maybe tomorrow if I have time. Thanks, Andrew -- ## START: next-inline2.cc ## /* This testcase is part of GDB, the GNU debugger. 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/>. */ #include <stdlib.h> struct tree { volatile int x; volatile int z; }; #define TREE_TYPE(NODE) (*tree_check (NODE, 0)) inline tree * tree_check (tree *t, int i) { if (t->x != i) abort(); tree *x = t; return x; } int __attribute__((noinline, noclone)) get_alias_set (tree *t) { if (t != NULL && TREE_TYPE (t).z != 1 && TREE_TYPE (t).z != 2 && TREE_TYPE (t).z != 3) return 0; return 1; } tree xx; int main() { get_alias_set (&xx); abort (); } ## END ## ## START: is_stmt patch ## diff --git a/gdb/buildsym-legacy.c b/gdb/buildsym-legacy.c index 91f8eb869b7..1a1a856ea37 100644 --- a/gdb/buildsym-legacy.c +++ b/gdb/buildsym-legacy.c @@ -252,7 +252,7 @@ void record_line (struct subfile *subfile, int line, CORE_ADDR pc) { gdb_assert (buildsym_compunit != nullptr); - buildsym_compunit->record_line (subfile, line, pc); + buildsym_compunit->record_line (subfile, line, pc, 1); } /* Start a new symtab for a new source file in OBJFILE. Called, for example, diff --git a/gdb/buildsym.c b/gdb/buildsym.c index df745080817..e6b7e0ede2a 100644 --- a/gdb/buildsym.c +++ b/gdb/buildsym.c @@ -666,7 +666,7 @@ buildsym_compunit::pop_subfile () void buildsym_compunit::record_line (struct subfile *subfile, int line, - CORE_ADDR pc) + CORE_ADDR pc, bool is_stmt) { struct linetable_entry *e; @@ -687,6 +687,17 @@ buildsym_compunit::record_line (struct subfile *subfile, int line, m_have_line_numbers = true; } + /* If we have a duplicate for the previous entry then ignore the new + entry, except, if the new entry is setting the is_stmt flag, then + ensure the previous entry respects the new setting. */ + e = subfile->line_vector->item + subfile->line_vector->nitems - 1; + if (e->line == line && e->pc == pc) + { + if (is_stmt && !e->is_stmt) + e->is_stmt = 1; + return; + } + if (subfile->line_vector->nitems + 1 >= subfile->line_vector_length) { subfile->line_vector_length *= 2; @@ -722,6 +733,7 @@ buildsym_compunit::record_line (struct subfile *subfile, int line, e = subfile->line_vector->item + subfile->line_vector->nitems++; e->line = line; + e->is_stmt = is_stmt ? 1 : 0; e->pc = pc; } diff --git a/gdb/buildsym.h b/gdb/buildsym.h index 193832fc37a..176c56dcbbb 100644 --- a/gdb/buildsym.h +++ b/gdb/buildsym.h @@ -187,7 +187,8 @@ struct buildsym_compunit const char *pop_subfile (); - void record_line (struct subfile *subfile, int line, CORE_ADDR pc); + void record_line (struct subfile *subfile, int line, CORE_ADDR pc, + bool is_stmt); struct compunit_symtab *get_compunit_symtab () { diff --git a/gdb/disasm.c b/gdb/disasm.c index 164939b5882..4eb1360de08 100644 --- a/gdb/disasm.c +++ b/gdb/disasm.c @@ -376,6 +376,9 @@ do_mixed_source_and_assembly_deprecated if (le[i].line == le[i + 1].line && le[i].pc == le[i + 1].pc) continue; /* Ignore duplicates. */ + if (le[i].is_stmt == 0) + continue; /* Ignore non-statements. */ + /* Skip any end-of-function markers. */ if (le[i].line == 0) continue; diff --git a/gdb/dwarf2read.c b/gdb/dwarf2read.c index 819d37c9b8e..96d9763147c 100644 --- a/gdb/dwarf2read.c +++ b/gdb/dwarf2read.c @@ -21267,7 +21267,7 @@ dwarf_record_line_p (struct dwarf2_cu *cu, static void dwarf_record_line_1 (struct gdbarch *gdbarch, struct subfile *subfile, - unsigned int line, CORE_ADDR address, + unsigned int line, CORE_ADDR address, bool is_stmt, struct dwarf2_cu *cu) { CORE_ADDR addr = gdbarch_addr_bits_remove (gdbarch, address); @@ -21281,7 +21281,7 @@ dwarf_record_line_1 (struct gdbarch *gdbarch, struct subfile *subfile, } if (cu != nullptr) - cu->get_builder ()->record_line (subfile, line, addr); + cu->get_builder ()->record_line (subfile, line, addr, is_stmt); } /* Subroutine of dwarf_decode_lines_1 to simplify it. @@ -21304,7 +21304,7 @@ dwarf_finish_line (struct gdbarch *gdbarch, struct subfile *subfile, paddress (gdbarch, address)); } - dwarf_record_line_1 (gdbarch, subfile, 0, address, cu); + dwarf_record_line_1 (gdbarch, subfile, 0, address, 1, cu); } void @@ -21330,7 +21330,7 @@ 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) { if (m_last_subfile != m_cu->get_builder ()->get_current_subfile () || end_sequence) @@ -21341,6 +21341,8 @@ lnp_state_machine::record_line (bool end_sequence) if (!end_sequence) { + bool is_stmt = producer_is_codewarrior (m_cu) || m_is_stmt; + if (dwarf_record_line_p (m_cu, m_line, m_last_line, m_line_has_non_zero_discriminator, m_last_subfile)) @@ -21348,7 +21350,7 @@ lnp_state_machine::record_line (bool end_sequence) buildsym_compunit *builder = m_cu->get_builder (); dwarf_record_line_1 (m_gdbarch, builder->get_current_subfile (), - m_line, m_address, + m_line, m_address, is_stmt, m_currently_recording_lines ? m_cu : nullptr); } m_last_subfile = m_cu->get_builder ()->get_current_subfile (); diff --git a/gdb/inline-frame.c b/gdb/inline-frame.c index 3b4848b3a45..ef2661702f0 100644 --- a/gdb/inline-frame.c +++ b/gdb/inline-frame.c @@ -464,7 +464,7 @@ void _initialize_inline_frame (void) { add_cmd ("inline-frames", class_maintenance, maint_info_inline_frames_cmd, - _("Info about inline frames at the current location"), + _("Info about inline frames at the current location."), &maintenanceinfolist); } diff --git a/gdb/jit.c b/gdb/jit.c index 2cfea4561b3..316c440e013 100644 --- a/gdb/jit.c +++ b/gdb/jit.c @@ -578,6 +578,7 @@ jit_symtab_line_mapping_add_impl (struct gdb_symbol_callbacks *cb, { stab->linetable->item[i].pc = (CORE_ADDR) map[i].pc; stab->linetable->item[i].line = map[i].line; + stab->linetable->item[i].is_stmt = 1; } } diff --git a/gdb/record-btrace.c b/gdb/record-btrace.c index 459d0da8402..6e45a993645 100644 --- a/gdb/record-btrace.c +++ b/gdb/record-btrace.c @@ -717,7 +717,16 @@ btrace_find_line_range (CORE_ADDR pc) range = btrace_mk_line_range (symtab, 0, 0); for (i = 0; i < nlines - 1; i++) { - if ((lines[i].pc == pc) && (lines[i].line != 0)) + /* The test of is_stmt here was added when the is_stmt field was + introduced to the 'struct linetable_entry' structure. This + ensured that this loop maintained the same behaviour as before we + introduced is_stmt. That said, it might be that we would be + better off not checking is_stmt here, this would lead to us + possibly adding more line numbers to the range. At the time this + change was made I was unsure how to test this so chose to go with + maintaining the existing experience. */ + if ((lines[i].pc == pc) && (lines[i].line != 0) + && (lines[i].is_stmt == 1)) range = btrace_line_range_add (range, lines[i].line); } diff --git a/gdb/symmisc.c b/gdb/symmisc.c index 69d035dc843..b173c7aaca1 100644 --- a/gdb/symmisc.c +++ b/gdb/symmisc.c @@ -306,6 +306,8 @@ dump_symtab_1 (struct symtab *symtab, struct ui_file *outfile) { fprintf_filtered (outfile, " line %d at ", l->item[i].line); fputs_filtered (paddress (gdbarch, l->item[i].pc), outfile); + if (l->item[i].is_stmt) + fprintf_filtered (outfile, "\t(stmt)"); fprintf_filtered (outfile, "\n"); } } @@ -992,16 +994,17 @@ maintenance_print_one_line_table (struct symtab *symtab, void *data) /* Leave space for 6 digits of index and line number. After that the tables will just not format as well. */ - printf_filtered (_("%-6s %6s %s\n"), - _("INDEX"), _("LINE"), _("ADDRESS")); + printf_filtered (_("%-6s %6s %s %s\n"), + _("INDEX"), _("LINE"), _("ADDRESS"), _("IS-STMT")); for (i = 0; i < linetable->nitems; ++i) { struct linetable_entry *item; item = &linetable->item [i]; - printf_filtered (_("%-6d %6d %s\n"), i, item->line, - core_addr_to_string (item->pc)); + printf_filtered (_("%-6d %6d %s%s\n"), i, item->line, + core_addr_to_string (item->pc), + (item->is_stmt ? " Y" : "")); } } diff --git a/gdb/symtab.c b/gdb/symtab.c index 099e92070a8..5f90bad30f8 100644 --- a/gdb/symtab.c +++ b/gdb/symtab.c @@ -3236,6 +3236,16 @@ find_pc_sect_line (CORE_ADDR pc, struct obj_section *section, int notcurrent) best = prev; best_symtab = iter_s; + if (!best->is_stmt) + { + struct linetable_entry *tmp = best; + while (tmp > first && (tmp - 1)->pc == tmp->pc + && (tmp - 1)->line != 0 && !tmp->is_stmt) + --tmp; + if (tmp->is_stmt) + best = tmp; + } + /* Discard BEST_END if it's before the PC of the current BEST. */ if (best_end <= best->pc) best_end = 0; @@ -3434,7 +3444,8 @@ find_pcs_for_symtab_line (struct symtab *symtab, int line, { struct linetable_entry *item = &SYMTAB_LINETABLE (symtab)->item[idx]; - if (*best_item == NULL || item->line < (*best_item)->line) + if (*best_item == NULL + || (item->line < (*best_item)->line && item->is_stmt)) *best_item = item; break; @@ -3545,6 +3556,10 @@ find_line_common (struct linetable *l, int lineno, { struct linetable_entry *item = &(l->item[i]); + /* Ignore non-statements. */ + if (!item->is_stmt) + continue; + if (item->line == lineno) { /* Return the first (lowest address) entry which matches. */ diff --git a/gdb/symtab.h b/gdb/symtab.h index a1682969355..89eb9ad5bcc 100644 --- a/gdb/symtab.h +++ b/gdb/symtab.h @@ -1277,7 +1277,13 @@ struct rust_vtable_symbol : public symbol struct linetable_entry { + /* The line number for this entry. */ int line; + + /* True if this PC is a good location to place a breakpoint for LINE. */ + int is_stmt : 1; + + /* The address for this entry. */ CORE_ADDR pc; }; diff --git a/gdb/xcoffread.c b/gdb/xcoffread.c index eaa77fd4915..9028ff4cb9b 100644 --- a/gdb/xcoffread.c +++ b/gdb/xcoffread.c @@ -431,6 +431,9 @@ arrange_linetable (struct linetable *oldLineTb) for (function_count = 0, ii = 0; ii < oldLineTb->nitems; ++ii) { + if (oldLineTb->item[ii].is_stmt == 0) + continue; + if (oldLineTb->item[ii].line == 0) { /* Function entry found. */ if (function_count >= fentry_size) @@ -441,6 +444,7 @@ arrange_linetable (struct linetable *oldLineTb) fentry_size * sizeof (struct linetable_entry)); } fentry[function_count].line = ii; + fentry[function_count].is_stmt = 1; fentry[function_count].pc = oldLineTb->item[ii].pc; ++function_count; ## END: is_stmt patch ## ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PING**2] [PATCH] Fix an issue with the gdb step-over aka. "n" command 2019-12-30 22:12 ` Andrew Burgess @ 2020-01-01 9:40 ` Bernd Edlinger 0 siblings, 0 replies; 14+ messages in thread From: Bernd Edlinger @ 2020-01-01 9:40 UTC (permalink / raw) To: Andrew Burgess; +Cc: gdb-patches, Simon Marchi On 12/30/19 11:12 PM, Andrew Burgess wrote: > I've taken a look through this patch, and I think that we should > consider adding real support for the line table is_stmt attribute to > GDB as an alternative. > > I had a crack at doing this and it fixes the test case as you > originally describe it, with the inline function defined in the header > file, and the caller in the .cc file - however it still doesn't fix > the case where the inline function is in the same source file as the > caller, however, I'm convinced this is due to incorrect DWARF being > generated by GCC rather than a fault of GDB. > Okay, the way how this fixes the original test case is similar to how my patch works: This code in buildsym_compunit::record_line which eliminates previous is_stmt locations at the same pc: /* Normally, we treat lines as unsorted. But the end of sequence marker is special. We sort line markers at the same PC by line number, so end of sequence markers (which have line == 0) appear first. This is right if the marker ends the previous function, and there is no padding before the next function. But it is wrong if the previous line was empty and we are now marking a switch to a different subfile. We must leave the end of sequence marker at the end of this group of lines, not sort the empty line to after the marker. The easiest way to accomplish this is to delete any empty lines from our table, if they are followed by end of sequence markers. All we lose is the ability to set breakpoints at some lines which contain no instructions anyway. */ if (line == 0 && subfile->line_vector->nitems > 0) { e = subfile->line_vector->item + subfile->line_vector->nitems - 1; while (subfile->line_vector->nitems > 0 && e->pc == pc) { e--; subfile->line_vector->nitems--; } } This is now invoked by: if (m_record_lines_p) { if (m_last_subfile != m_cu->get_builder ()->get_current_subfile () || end_sequence) { dwarf_finish_line (m_gdbarch, m_last_subfile, m_address, m_currently_recording_lines ? m_cu : nullptr); } previously it was guarded by && (producer_is_codewarrior (m_cu) || m_is_stmt)) That explains why that appears to be fixed, but in a way it is just a bug that record_lines does this. Especially when !is_stmt locations follow, it is probably wrong to completely remove the only is_stmt from the line table. > As a test case I'm using next-inline2.cc, which I've included below, I > compile this using GCC 9.2.0 though I've also tested 10.xx, and 8.3.0 > which are no different. I also tried 7.4.0 which is different, and > I'll discuss below. > > The interesting part of the source file is as follows, with line > numbers on the left for reference: > > 28: inline tree * > 29: tree_check (tree *t, int i) > 30: { > 31: if (t->x != i) > 32: abort(); > 33: tree *x = t; > 34: return x; > 35: } > 36: > 37: int __attribute__((noinline, noclone)) > 38: get_alias_set (tree *t) > 39: { > 40: if (t != NULL > 41: && TREE_TYPE (t).z != 1 > 42: && TREE_TYPE (t).z != 2 > 43: && TREE_TYPE (t).z != 3) > 44: return 0; > 45: return 1; > 46: } > > I compile the test with 'g++ -O2 -g3 -o next-inline2 next-inline2.cc', > and the decoded line table (or part of it) looks like: > > Contents of the .debug_line section: > > CU: ./next-inline2.cc: > File name Line number Starting address View Stmt > next-inline2.cc 39 0x400540 x > next-inline2.cc 40 0x400540 1 x > next-inline2.cc 43 0x400540 2 > next-inline2.cc 31 0x400545 x > next-inline2.cc 39 0x400545 1 > next-inline2.cc 31 0x400549 > next-inline2.cc 31 0x40054b > next-inline2.cc 33 0x400553 x > next-inline2.cc 34 0x400553 1 x > next-inline2.cc 41 0x400553 2 > next-inline2.cc 41 0x400556 > next-inline2.cc 31 0x40055b x > next-inline2.cc 31 0x40055b 1 > next-inline2.cc 31 0x40055d > next-inline2.cc 33 0x400565 x > next-inline2.cc 34 0x400565 1 x > next-inline2.cc 42 0x400565 2 > next-inline2.cc 42 0x400568 > > The other interesting thing we need to understand is where the inline > subroutines are, in the source code above the inline subroutines are > hidden behind the TREE_TYPE macro. Looking at the DWARF we see 3 > instances of DW_TAG_inlined_subroutine, they are: > > <2><8e8>: Abbrev Number: 39 (DW_TAG_inlined_subroutine) > <8e9> DW_AT_abstract_origin: <0x9a2> > <8ed> DW_AT_entry_pc : 0x400545 > <8f5> DW_AT_ranges : 0x30 > <8f9> DW_AT_call_file : 1 > <8fa> DW_AT_call_line : 41 > <8fb> DW_AT_call_column : 10 > <8fc> DW_AT_sibling : <0x92f> > > <2><92f>: Abbrev Number: 43 (DW_TAG_inlined_subroutine) > <930> DW_AT_abstract_origin: <0x9a2> > <934> DW_AT_low_pc : 0x40055b > <93c> DW_AT_high_pc : 0xa > <944> DW_AT_call_file : 1 > <945> DW_AT_call_line : 42 > <946> DW_AT_call_column : 10 > <947> DW_AT_sibling : <0x967> > > <2><967>: Abbrev Number: 44 (DW_TAG_inlined_subroutine) > <968> DW_AT_abstract_origin: <0x9a2> > <96c> DW_AT_low_pc : 0x40056d > <974> DW_AT_high_pc : 0xa > <97c> DW_AT_call_file : 1 > <97d> DW_AT_call_line : 43 > <97e> DW_AT_call_column : 10 > > Notice in all cases we reference abstract origin 0x9a2, this is the > same subroutine inlined 3 times. Also notice the difference with how > the address ranges are defined. In the last two cases the address > ranges for the inlined subroutines start at 0x40055b and 0x40056d and > continue for 10 bytes. In the first case however, the address range > is in the .debug_ranges table, and looks like this: > > 00000030 0000000000400545 0000000000400545 (start == end) > 00000030 0000000000400549 0000000000400553 > 00000030 0000000000400430 0000000000400435 > 00000030 <End of list> > > Here we have the first clue that something has gone wrong, the 'start > == end' warning tells us that there's at the very least a dummy entry > n in the ranges table (could happen with relaxation maybe, or possibly > linker garbage collection, but its still a warning sign I think). > > So the next job is to try and merge all this information into one > table and see what we can learn. In the following table I use the > following short-hand: > > 'In' column indicates a range of addresses covered by an inline > function range. > > 'Ln' column contains line table information, where: > + A number e.g. '39', indicates line 39 is specified to be > at this address in the line table, and is a statement, that is a > good place to insert a breakpoint. > + An underscore and number, e.g. '_43' indicates line 43 is > specified to be at this address in the line table, but this is > not a statement, that is, this is not a good place to put a > breakpoint if you want to stop at this line. > + A colon and number, e.g. ':43' indicates we infer that line 43 > is here based on the last entry that was in the line table, > but there is no direct entry for this address in the line table. > > 'Address' and 'Disassembly Text' are just from the disassembly output. > > | In | Ln | Address | Disassembly Text | > |----+------------+----------------------------+-----------------------------------------------| > | | 39,40,_43 | 0x0000000000400540 <+0>: | test %rdi,%rdi | > | | :43 | 0x0000000000400543 <+3>: | je 0x400590 <get_alias_set(tree*)+80> | > | | 31, _39 | 0x0000000000400545 <+5>: | sub $0x8,%rsp | > | X | _31 | 0x0000000000400549 <+9>: | mov (%rdi),%eax | > | X | _31 | 0x000000000040054b <+11>: | test %eax,%eax | > | X | :31 | 0x000000000040054d <+13>: | jne 0x400430<_Z13get_alias_setP4tree.cold> | > | | 33,34,_41 | 0x0000000000400553 <+19>: | mov 0x4(%rdi),%eax | > | | _41 | 0x0000000000400556 <+22>: | cmp $0x1,%eax | > | | :41 | 0x0000000000400559 <+25>: | je 0x400583 <get_alias_set(tree*)+67> | > | X | 31,_31 | 0x000000000040055b <+27>: | mov (%rdi),%eax | > | X | _31 | 0x000000000040055d <+29>: | test %eax,%eax | > | X | :31 | 0x000000000040055f <+31>: | jne 0x400430<_Z13get_alias_setP4tree.cold> | > | | 33, 34,_42 | 0x0000000000400565 <+37>: | mov 0x4(%rdi),%eax | > | | | 0x0000000000400568 <+40>: | cmp $0x2,%eax | > | | | 0x000000000040056b <+43>: | je 0x4005a0 <get_alias_set(tree*)+96> | > | X | | 0x000000000040056d <+45>: | mov (%rdi),%eax | > | X | | 0x000000000040056f <+47>: | test %eax,%eax | > | X | | 0x0000000000400571 <+49>: | jne 0x400430<_Z13get_alias_setP4tree.cold> | > | | | 0x0000000000400577 <+55>: | mov 0x4(%rdi),%eax | > | | | 0x000000000040057a <+58>: | cmp $0x3,%eax | > | | | 0x000000000040057d <+61>: | sete %al | > > > My claim is this, in well structured DWARF and entries for lines 31, > 32 33, or 34 should be inside an area marked as belonging to the > inline subroutine, or at a minimum, should not be marked as a > statement and be outside an inlined region. > > Take for example address 0x400545, this is a statement line for 31, > and a non-statement line for 39, but is NOT in an inline subroutine. > The DWARF is (I claim) clearly indicating that address 0x400545 comes > from line 31, and is in the function get_alias_set. By looking at the > source code we can clearly see this is not correct. > > There are other inconsistencies, like 0x400553, which claims to be > line 33 and 34, yet is outside the inlined range. > > I'm hoping that someone can double check my analysis of the above and > call me out on anything I've gotten wrong. > Yes I agree, with your analysis, however I believe that is not a problem of the implementation but of the specification. Maybe it helps to get the background if you look at my attempt of fixing this as a gcc bug: https://gcc.gnu.org/ml/gcc-patches/2019-10/msg01459.html and Alexandre Oliva's response here: https://gcc.gnu.org/ml/gcc-patches/2019-11/msg01771.html So this is a problem in the design of the dwarf line info which has stmt-type line info at the end of a inlined subroutine. I think the problem appears always at the end of the inline range, where it is ambiguous if the line number is part of the subroutine or not. It is less ambiguous if the subroutine is in a header file, which is fortunately the common case. What I think gcc is trying to tell us here is the location view of certain variables at the end of the subroutine. Note I added this statement on purpose, because I knew that already: tree *x = t; return x; So this introduces a new name, that does require zero instructions. But it inherits the is_stmt property of the unoptimized code, also as desinged. And the location view seems to tell us the value of x at this very spot. I am however not sure if it is good to make "s" stop at every location that is not a statement barrier, I think these might be good to have in a source-annotation in the disassembler view. BTW: if I use the gcc patch from the posting above, it fixes next-inline.cc and next-inline2.cc at once, but not this one: ## START: next-inline3.cc ## /* This testcase is part of GDB, the GNU debugger. 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/>. */ #include <stdlib.h> struct tree { volatile int x; volatile int z; }; #define TREE_TYPE(NODE) (*tree_check (NODE, 0)) inline tree * tree_check (tree *t, int i); int __attribute__((noinline, noclone)) get_alias_set (tree *t) { if (t != NULL && TREE_TYPE (t).z != 1 && TREE_TYPE (t).z != 2 && TREE_TYPE (t).z != 3) return 0; return 1; } tree xx; inline tree * tree_check (tree *t, int i) { if (t->x != i) abort(); tree *x = t; return x; } int main() { get_alias_set (&xx); abort (); } ## END ## however, together with your other patch about the stable sorting of the line numbers it would fix next-inline3.cc as well. I tried your patch against the original problem, that is debugging in gcc. $ gcc -S next-inline2.cc -wrapper gdb,--args GNU gdb (GDB) 10.0.50.20191231-git (gdb) b get_alias_set Breakpoint 1 at 0xb2d8e0: file ../../gcc-trunk/gcc/alias.c, line 837. (gdb) r Starting program: /home/ed/gnu/install/libexec/gcc/x86_64-pc-linux-gnu/10.0.0/cc1plus -quiet -imultiarch x86_64-linux-gnu -D_GNU_SOURCE next-inline2.cc -quiet -dumpbase next-inline2.cc -mtune=generic -march=x86-64 -auxbase next-inline2 -o next-inline2.s Breakpoint 1, get_alias_set (t=t@entry=0x7ffff74bd7e0) at ../../gcc-trunk/gcc/alias.c:837 837 if (t == error_mark_node (gdb) n 829 { (gdb) n 838 || (! TYPE_P (t) (gdb) n 839 && (TREE_TYPE (t) == 0 || TREE_TYPE (t) == error_mark_node))) (gdb) n 851 STRIP_NOPS (t); (gdb) n 852 set = lang_hooks.get_alias_set (t); note the execution jumps back to line 829, that is code from the prologue which is subject to code motion, I wanted to avoid that, which would affect pretty much every function. I tried to break at main and the code just twice to the prologue: (gdb) b main Breakpoint 2 at 0x893e10: main. (2 locations) (gdb) r The program being debugged has been started already. Start it from the beginning? (y or n) y Starting program: /home/ed/gnu/install/libexec/gcc/x86_64-pc-linux-gnu/10.0.0/cc1plus -quiet -imultiarch x86_64-linux-gnu -D_GNU_SOURCE next-inline2.cc -quiet -dumpbase next-inline2.cc -mtune=generic -march=x86-64 -auxbase next-inline2 -o next-inline2.s Breakpoint 2, main (argc=15, argv=0x7fffffffdd78) at ../../gcc-trunk/gcc/main.c:36 36 toplev toplev (NULL, /* external_timer */ (gdb) n 37 true /* init_signals */); (gdb) n 35 { (gdb) n 37 true /* init_signals */); (gdb) n 35 { (gdb) n 37 true /* init_signals */); (gdb) n 39 return toplev.main (argc, argv); I think this patch could be a base for a slightly better heuristic, that does not depend on the inline function to be in a different file, but uses end-PC values from the inlined_subroutine ranges. I could imagine that if a is_stmt location is at the same PC value where a subroutine range ends, and it is followed by one or more !is_stmt location at the same PC then the last of these should replace the is_stmt which is likely part if the inline routine. I believe the real fix would be to annotate the line number program with a reference to the inlined subroutine that the line number refers to. Thanks Bernd. > I wont run the full analysis again, but I mentioned previously that I > tried GCC 7.4.0. The debug information generated here was not as > sophisticated, but nor was it (I think) so wrong. The debug range for > the first inlined subroutine entry makes much more sense, there's no > longer the silly 'start == end' thing going on, and the line table is > much simpler. Consequently the debug experience, though not perfect, > is much better than on any of the later compiler - and this is with my > is_stmt supporting GDB. > > I've included below the is_stmt support patch as I currently have it, > but please remember, this is a very quick hack just so anyone > interested can test it. I'm going to try and clean it up and repost > it maybe tomorrow if I have time. > > Thanks, > Andrew > > -- > > ## START: next-inline2.cc ## > > /* This testcase is part of GDB, the GNU debugger. > > 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/>. */ > > #include <stdlib.h> > > struct tree > { > volatile int x; > volatile int z; > }; > > #define TREE_TYPE(NODE) (*tree_check (NODE, 0)) > > inline tree * > tree_check (tree *t, int i) > { > if (t->x != i) > abort(); > tree *x = t; > return x; > } > > int __attribute__((noinline, noclone)) > get_alias_set (tree *t) > { > if (t != NULL > && TREE_TYPE (t).z != 1 > && TREE_TYPE (t).z != 2 > && TREE_TYPE (t).z != 3) > return 0; > return 1; > } > > tree xx; > > int main() > { > get_alias_set (&xx); > abort (); > } > > ## END ## > > ## START: is_stmt patch ## > > diff --git a/gdb/buildsym-legacy.c b/gdb/buildsym-legacy.c > index 91f8eb869b7..1a1a856ea37 100644 > --- a/gdb/buildsym-legacy.c > +++ b/gdb/buildsym-legacy.c > @@ -252,7 +252,7 @@ void > record_line (struct subfile *subfile, int line, CORE_ADDR pc) > { > gdb_assert (buildsym_compunit != nullptr); > - buildsym_compunit->record_line (subfile, line, pc); > + buildsym_compunit->record_line (subfile, line, pc, 1); > } > > /* Start a new symtab for a new source file in OBJFILE. Called, for example, > diff --git a/gdb/buildsym.c b/gdb/buildsym.c > index df745080817..e6b7e0ede2a 100644 > --- a/gdb/buildsym.c > +++ b/gdb/buildsym.c > @@ -666,7 +666,7 @@ buildsym_compunit::pop_subfile () > > void > buildsym_compunit::record_line (struct subfile *subfile, int line, > - CORE_ADDR pc) > + CORE_ADDR pc, bool is_stmt) > { > struct linetable_entry *e; > > @@ -687,6 +687,17 @@ buildsym_compunit::record_line (struct subfile *subfile, int line, > m_have_line_numbers = true; > } > > + /* If we have a duplicate for the previous entry then ignore the new > + entry, except, if the new entry is setting the is_stmt flag, then > + ensure the previous entry respects the new setting. */ > + e = subfile->line_vector->item + subfile->line_vector->nitems - 1; > + if (e->line == line && e->pc == pc) > + { > + if (is_stmt && !e->is_stmt) > + e->is_stmt = 1; > + return; > + } > + > if (subfile->line_vector->nitems + 1 >= subfile->line_vector_length) > { > subfile->line_vector_length *= 2; > @@ -722,6 +733,7 @@ buildsym_compunit::record_line (struct subfile *subfile, int line, > > e = subfile->line_vector->item + subfile->line_vector->nitems++; > e->line = line; > + e->is_stmt = is_stmt ? 1 : 0; > e->pc = pc; > } > > diff --git a/gdb/buildsym.h b/gdb/buildsym.h > index 193832fc37a..176c56dcbbb 100644 > --- a/gdb/buildsym.h > +++ b/gdb/buildsym.h > @@ -187,7 +187,8 @@ struct buildsym_compunit > > const char *pop_subfile (); > > - void record_line (struct subfile *subfile, int line, CORE_ADDR pc); > + void record_line (struct subfile *subfile, int line, CORE_ADDR pc, > + bool is_stmt); > > struct compunit_symtab *get_compunit_symtab () > { > diff --git a/gdb/disasm.c b/gdb/disasm.c > index 164939b5882..4eb1360de08 100644 > --- a/gdb/disasm.c > +++ b/gdb/disasm.c > @@ -376,6 +376,9 @@ do_mixed_source_and_assembly_deprecated > if (le[i].line == le[i + 1].line && le[i].pc == le[i + 1].pc) > continue; /* Ignore duplicates. */ > > + if (le[i].is_stmt == 0) > + continue; /* Ignore non-statements. */ > + > /* Skip any end-of-function markers. */ > if (le[i].line == 0) > continue; > diff --git a/gdb/dwarf2read.c b/gdb/dwarf2read.c > index 819d37c9b8e..96d9763147c 100644 > --- a/gdb/dwarf2read.c > +++ b/gdb/dwarf2read.c > @@ -21267,7 +21267,7 @@ dwarf_record_line_p (struct dwarf2_cu *cu, > > static void > dwarf_record_line_1 (struct gdbarch *gdbarch, struct subfile *subfile, > - unsigned int line, CORE_ADDR address, > + unsigned int line, CORE_ADDR address, bool is_stmt, > struct dwarf2_cu *cu) > { > CORE_ADDR addr = gdbarch_addr_bits_remove (gdbarch, address); > @@ -21281,7 +21281,7 @@ dwarf_record_line_1 (struct gdbarch *gdbarch, struct subfile *subfile, > } > > if (cu != nullptr) > - cu->get_builder ()->record_line (subfile, line, addr); > + cu->get_builder ()->record_line (subfile, line, addr, is_stmt); > } > > /* Subroutine of dwarf_decode_lines_1 to simplify it. > @@ -21304,7 +21304,7 @@ dwarf_finish_line (struct gdbarch *gdbarch, struct subfile *subfile, > paddress (gdbarch, address)); > } > > - dwarf_record_line_1 (gdbarch, subfile, 0, address, cu); > + dwarf_record_line_1 (gdbarch, subfile, 0, address, 1, cu); > } > > void > @@ -21330,7 +21330,7 @@ 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) > { > if (m_last_subfile != m_cu->get_builder ()->get_current_subfile () > || end_sequence) > @@ -21341,6 +21341,8 @@ lnp_state_machine::record_line (bool end_sequence) > > if (!end_sequence) > { > + bool is_stmt = producer_is_codewarrior (m_cu) || m_is_stmt; > + > if (dwarf_record_line_p (m_cu, m_line, m_last_line, > m_line_has_non_zero_discriminator, > m_last_subfile)) > @@ -21348,7 +21350,7 @@ lnp_state_machine::record_line (bool end_sequence) > buildsym_compunit *builder = m_cu->get_builder (); > dwarf_record_line_1 (m_gdbarch, > builder->get_current_subfile (), > - m_line, m_address, > + m_line, m_address, is_stmt, > m_currently_recording_lines ? m_cu : nullptr); > } > m_last_subfile = m_cu->get_builder ()->get_current_subfile (); > diff --git a/gdb/inline-frame.c b/gdb/inline-frame.c > index 3b4848b3a45..ef2661702f0 100644 > --- a/gdb/inline-frame.c > +++ b/gdb/inline-frame.c > @@ -464,7 +464,7 @@ void > _initialize_inline_frame (void) > { > add_cmd ("inline-frames", class_maintenance, maint_info_inline_frames_cmd, > - _("Info about inline frames at the current location"), > + _("Info about inline frames at the current location."), > &maintenanceinfolist); > } > > diff --git a/gdb/jit.c b/gdb/jit.c > index 2cfea4561b3..316c440e013 100644 > --- a/gdb/jit.c > +++ b/gdb/jit.c > @@ -578,6 +578,7 @@ jit_symtab_line_mapping_add_impl (struct gdb_symbol_callbacks *cb, > { > stab->linetable->item[i].pc = (CORE_ADDR) map[i].pc; > stab->linetable->item[i].line = map[i].line; > + stab->linetable->item[i].is_stmt = 1; > } > } > > diff --git a/gdb/record-btrace.c b/gdb/record-btrace.c > index 459d0da8402..6e45a993645 100644 > --- a/gdb/record-btrace.c > +++ b/gdb/record-btrace.c > @@ -717,7 +717,16 @@ btrace_find_line_range (CORE_ADDR pc) > range = btrace_mk_line_range (symtab, 0, 0); > for (i = 0; i < nlines - 1; i++) > { > - if ((lines[i].pc == pc) && (lines[i].line != 0)) > + /* The test of is_stmt here was added when the is_stmt field was > + introduced to the 'struct linetable_entry' structure. This > + ensured that this loop maintained the same behaviour as before we > + introduced is_stmt. That said, it might be that we would be > + better off not checking is_stmt here, this would lead to us > + possibly adding more line numbers to the range. At the time this > + change was made I was unsure how to test this so chose to go with > + maintaining the existing experience. */ > + if ((lines[i].pc == pc) && (lines[i].line != 0) > + && (lines[i].is_stmt == 1)) this condition can never be true, since is_stmt is either 0, or -1. > range = btrace_line_range_add (range, lines[i].line); > } > > diff --git a/gdb/symmisc.c b/gdb/symmisc.c > index 69d035dc843..b173c7aaca1 100644 > --- a/gdb/symmisc.c > +++ b/gdb/symmisc.c > @@ -306,6 +306,8 @@ dump_symtab_1 (struct symtab *symtab, struct ui_file *outfile) > { > fprintf_filtered (outfile, " line %d at ", l->item[i].line); > fputs_filtered (paddress (gdbarch, l->item[i].pc), outfile); > + if (l->item[i].is_stmt) > + fprintf_filtered (outfile, "\t(stmt)"); > fprintf_filtered (outfile, "\n"); > } > } > @@ -992,16 +994,17 @@ maintenance_print_one_line_table (struct symtab *symtab, void *data) > > /* Leave space for 6 digits of index and line number. After that the > tables will just not format as well. */ > - printf_filtered (_("%-6s %6s %s\n"), > - _("INDEX"), _("LINE"), _("ADDRESS")); > + printf_filtered (_("%-6s %6s %s %s\n"), > + _("INDEX"), _("LINE"), _("ADDRESS"), _("IS-STMT")); > > for (i = 0; i < linetable->nitems; ++i) > { > struct linetable_entry *item; > > item = &linetable->item [i]; > - printf_filtered (_("%-6d %6d %s\n"), i, item->line, > - core_addr_to_string (item->pc)); > + printf_filtered (_("%-6d %6d %s%s\n"), i, item->line, > + core_addr_to_string (item->pc), > + (item->is_stmt ? " Y" : "")); > } > } > > diff --git a/gdb/symtab.c b/gdb/symtab.c > index 099e92070a8..5f90bad30f8 100644 > --- a/gdb/symtab.c > +++ b/gdb/symtab.c > @@ -3236,6 +3236,16 @@ find_pc_sect_line (CORE_ADDR pc, struct obj_section *section, int notcurrent) > best = prev; > best_symtab = iter_s; > > + if (!best->is_stmt) > + { > + struct linetable_entry *tmp = best; > + while (tmp > first && (tmp - 1)->pc == tmp->pc > + && (tmp - 1)->line != 0 && !tmp->is_stmt) > + --tmp; > + if (tmp->is_stmt) > + best = tmp; > + } > + > /* Discard BEST_END if it's before the PC of the current BEST. */ > if (best_end <= best->pc) > best_end = 0; > @@ -3434,7 +3444,8 @@ find_pcs_for_symtab_line (struct symtab *symtab, int line, > { > struct linetable_entry *item = &SYMTAB_LINETABLE (symtab)->item[idx]; > > - if (*best_item == NULL || item->line < (*best_item)->line) > + if (*best_item == NULL > + || (item->line < (*best_item)->line && item->is_stmt)) > *best_item = item; > > break; > @@ -3545,6 +3556,10 @@ find_line_common (struct linetable *l, int lineno, > { > struct linetable_entry *item = &(l->item[i]); > > + /* Ignore non-statements. */ > + if (!item->is_stmt) > + continue; > + > if (item->line == lineno) > { > /* Return the first (lowest address) entry which matches. */ > diff --git a/gdb/symtab.h b/gdb/symtab.h > index a1682969355..89eb9ad5bcc 100644 > --- a/gdb/symtab.h > +++ b/gdb/symtab.h > @@ -1277,7 +1277,13 @@ struct rust_vtable_symbol : public symbol > > struct linetable_entry > { > + /* The line number for this entry. */ > int line; > + > + /* True if this PC is a good location to place a breakpoint for LINE. */ > + int is_stmt : 1; I think this should be unsinged int is_stmt : 1, or bool. > + > + /* The address for this entry. */ > CORE_ADDR pc; > }; > > diff --git a/gdb/xcoffread.c b/gdb/xcoffread.c > index eaa77fd4915..9028ff4cb9b 100644 > --- a/gdb/xcoffread.c > +++ b/gdb/xcoffread.c > @@ -431,6 +431,9 @@ arrange_linetable (struct linetable *oldLineTb) > > for (function_count = 0, ii = 0; ii < oldLineTb->nitems; ++ii) > { > + if (oldLineTb->item[ii].is_stmt == 0) > + continue; > + > if (oldLineTb->item[ii].line == 0) > { /* Function entry found. */ > if (function_count >= fentry_size) > @@ -441,6 +444,7 @@ arrange_linetable (struct linetable *oldLineTb) > fentry_size * sizeof (struct linetable_entry)); > } > fentry[function_count].line = ii; > + fentry[function_count].is_stmt = 1; > fentry[function_count].pc = oldLineTb->item[ii].pc; > ++function_count; > > ## END: is_stmt patch ## > ^ permalink raw reply [flat|nested] 14+ messages in thread
* [PING**3] [PATCH] Fix an issue with the gdb step-over aka. "n" command 2019-12-14 13:52 ` [PING**2] " Bernd Edlinger 2019-12-30 22:12 ` Andrew Burgess @ 2020-01-06 8:14 ` Bernd Edlinger 2020-01-06 22:09 ` Andrew Burgess 1 sibling, 1 reply; 14+ messages in thread From: Bernd Edlinger @ 2020-01-06 8:14 UTC (permalink / raw) To: gdb-patches, Simon Marchi, Andrew Burgess Hi, I'd like to ping for this patch again. The latest version (including testcase + changelog) can be found here: https://sourceware.org/ml/gdb-patches/2019-12/msg01052.html Thanks Bernd. On 12/14/19 2:52 PM, Bernd Edlinger wrote: > Ping... > > I'm pinging for this patch here: > https://sourceware.org/ml/gdb-patches/2019-11/msg00792.html > > Thanks > Bernd. > > On 12/1/19 9:47 PM, Bernd Edlinger wrote: >> Ping... >> >> >> On 11/24/19 1:17 PM, Bernd Edlinger wrote: >>> Hi, >>> >>> this fixes an issue with the gdb step-over aka. "n" command. >>> >>> Apologies, the motivation for this patch was from sub-optimal >>> debug experience using some gcc code involving inlined functions, >>> and initially I tried to convince gcc folks that it is in fact a >>> gcc bug, but... >>> >>> It can be seen when you debug an optimized stage-3 cc1 >>> it does not affect -O0 code, though. >>> >>> Note: you can use "gcc -S hello.c -wrapper gdb,--args" to invoke cc1 with >>> debugger attached. >>> >>> This example debug session will explain the effect. >>> >>> (gdb) b get_alias_set >>> Breakpoint 5 at 0xa099f0: file ../../gcc-trunk/gcc/alias.c, line 837. >>> (gdb) r >>> Breakpoint 5, get_alias_set (t=t@entry=0x7ffff7ff7ab0) at ../../gcc-trunk/gcc/alias.c:837 >>> 837 if (t == error_mark_node >>> (gdb) n >>> 839 && (TREE_TYPE (t) == 0 || TREE_TYPE (t) == error_mark_node))) >>> (gdb) n >>> 3382 return __t; <-- now we have a problem: wrong line info here >>> (gdb) bt >>> #0 get_alias_set (t=t@entry=0x7ffff7ff7ab0) at ../../gcc-trunk/gcc/tree.h:3382 >>> #1 0x0000000000b25dfe in set_mem_attributes_minus_bitpos (ref=0x7ffff746f990, t=0x7ffff7ff7ab0, objectp=1, bitpos=...) >>> at ../../gcc-trunk/gcc/emit-rtl.c:1957 >>> #2 0x0000000001137a55 in make_decl_rtl (decl=0x7ffff7ff7ab0) at ../../gcc-trunk/gcc/varasm.c:1518 >>> #3 0x000000000113b6e8 in assemble_variable (decl=0x7ffff7ff7ab0, top_level=<optimized out>, at_end=<optimized out>, >>> dont_output_data=0) at ../../gcc-trunk/gcc/varasm.c:2246 >>> #4 0x000000000113f0ea in varpool_node::assemble_decl (this=0x7ffff745b000) at ../../gcc-trunk/gcc/varpool.c:584 >>> #5 0x000000000113fa17 in varpool_node::assemble_decl (this=0x7ffff745b000) at ../../gcc-trunk/gcc/varpool.c:750 >>> >>> The reason for this is a line number information that is exactly at >>> the end of the inlined function, but there is no code at that place, >>> only variable values (views) are declared there. Unfortunately >>> the next instruction is again in the main program, but due to -gstatement-frontiers >>> those do not have the is_stmt and are completely ignored by gdb at the moment. >>> >>> >>> This patch fixes the effect by rewriting the is_stmt attribute of the next >>> location info under certain conditions. >>> >>> I have no idea how to write a test case for this since it happens only in optimized code, >>> and only under very special conditions. >>> >>> >>> Thanks >>> Bernd. >>> ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PING**3] [PATCH] Fix an issue with the gdb step-over aka. "n" command 2020-01-06 8:14 ` [PING**3] " Bernd Edlinger @ 2020-01-06 22:09 ` Andrew Burgess 2020-01-07 15:15 ` Bernd Edlinger 0 siblings, 1 reply; 14+ messages in thread From: Andrew Burgess @ 2020-01-06 22:09 UTC (permalink / raw) To: Bernd Edlinger; +Cc: gdb-patches, Simon Marchi * Bernd Edlinger <bernd.edlinger@hotmail.de> [2020-01-06 08:14:37 +0000]: > Hi, > > I'd like to ping for this patch again. > The latest version (including testcase + changelog) can be found here: > https://sourceware.org/ml/gdb-patches/2019-12/msg01052.html I think we should investigate tracking the line table is_stmt property better, as I suggested in another mail in this thread. I see you've provided some feedback to the patch I posted, I just haven't had time to look at it yet, but will try to get to it as soon as I can. Thanks, Andrew > > Thanks > Bernd. > > On 12/14/19 2:52 PM, Bernd Edlinger wrote: > > Ping... > > > > I'm pinging for this patch here: > > https://sourceware.org/ml/gdb-patches/2019-11/msg00792.html > > > > Thanks > > Bernd. > > > > On 12/1/19 9:47 PM, Bernd Edlinger wrote: > >> Ping... > >> > >> > >> On 11/24/19 1:17 PM, Bernd Edlinger wrote: > >>> Hi, > >>> > >>> this fixes an issue with the gdb step-over aka. "n" command. > >>> > >>> Apologies, the motivation for this patch was from sub-optimal > >>> debug experience using some gcc code involving inlined functions, > >>> and initially I tried to convince gcc folks that it is in fact a > >>> gcc bug, but... > >>> > >>> It can be seen when you debug an optimized stage-3 cc1 > >>> it does not affect -O0 code, though. > >>> > >>> Note: you can use "gcc -S hello.c -wrapper gdb,--args" to invoke cc1 with > >>> debugger attached. > >>> > >>> This example debug session will explain the effect. > >>> > >>> (gdb) b get_alias_set > >>> Breakpoint 5 at 0xa099f0: file ../../gcc-trunk/gcc/alias.c, line 837. > >>> (gdb) r > >>> Breakpoint 5, get_alias_set (t=t@entry=0x7ffff7ff7ab0) at ../../gcc-trunk/gcc/alias.c:837 > >>> 837 if (t == error_mark_node > >>> (gdb) n > >>> 839 && (TREE_TYPE (t) == 0 || TREE_TYPE (t) == error_mark_node))) > >>> (gdb) n > >>> 3382 return __t; <-- now we have a problem: wrong line info here > >>> (gdb) bt > >>> #0 get_alias_set (t=t@entry=0x7ffff7ff7ab0) at ../../gcc-trunk/gcc/tree.h:3382 > >>> #1 0x0000000000b25dfe in set_mem_attributes_minus_bitpos (ref=0x7ffff746f990, t=0x7ffff7ff7ab0, objectp=1, bitpos=...) > >>> at ../../gcc-trunk/gcc/emit-rtl.c:1957 > >>> #2 0x0000000001137a55 in make_decl_rtl (decl=0x7ffff7ff7ab0) at ../../gcc-trunk/gcc/varasm.c:1518 > >>> #3 0x000000000113b6e8 in assemble_variable (decl=0x7ffff7ff7ab0, top_level=<optimized out>, at_end=<optimized out>, > >>> dont_output_data=0) at ../../gcc-trunk/gcc/varasm.c:2246 > >>> #4 0x000000000113f0ea in varpool_node::assemble_decl (this=0x7ffff745b000) at ../../gcc-trunk/gcc/varpool.c:584 > >>> #5 0x000000000113fa17 in varpool_node::assemble_decl (this=0x7ffff745b000) at ../../gcc-trunk/gcc/varpool.c:750 > >>> > >>> The reason for this is a line number information that is exactly at > >>> the end of the inlined function, but there is no code at that place, > >>> only variable values (views) are declared there. Unfortunately > >>> the next instruction is again in the main program, but due to -gstatement-frontiers > >>> those do not have the is_stmt and are completely ignored by gdb at the moment. > >>> > >>> > >>> This patch fixes the effect by rewriting the is_stmt attribute of the next > >>> location info under certain conditions. > >>> > >>> I have no idea how to write a test case for this since it happens only in optimized code, > >>> and only under very special conditions. > >>> > >>> > >>> Thanks > >>> Bernd. > >>> ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PING**3] [PATCH] Fix an issue with the gdb step-over aka. "n" command 2020-01-06 22:09 ` Andrew Burgess @ 2020-01-07 15:15 ` Bernd Edlinger 0 siblings, 0 replies; 14+ messages in thread From: Bernd Edlinger @ 2020-01-07 15:15 UTC (permalink / raw) To: Andrew Burgess; +Cc: gdb-patches, Simon Marchi On 1/6/20 11:09 PM, Andrew Burgess wrote: > * Bernd Edlinger <bernd.edlinger@hotmail.de> [2020-01-06 08:14:37 +0000]: > >> Hi, >> >> I'd like to ping for this patch again. >> The latest version (including testcase + changelog) can be found here: >> https://sourceware.org/ml/gdb-patches/2019-12/msg01052.html > > I think we should investigate tracking the line table is_stmt property > better, as I suggested in another mail in this thread. > Sure, no problem. I am fully aware that this is just a workaround. > I see you've provided some feedback to the patch I posted, I just > haven't had time to look at it yet, but will try to get to it as soon > as I can. > Thanks, one additional thought I had about your patch: I believe that we should also look at implementing the variable-location-views. I mean adding a view number to the record_line function. That would mean adding PC.view in the line table. That could in theory be used to find out if a location view is inside a subroutine or in the main program, even if the PC is the same, using the view number it would be possible to find that out. That would allow us to step to the return statement of the inline function if we are stepping in the inline function and to step over the whole inline if we want to. The only part that is really missing for that is the view number in addition to the PC where the inline function ends. Note there is already a view number where the inline starts: <2><918>: Abbrev Number: 43 (DW_TAG_inlined_subroutine) <919> DW_AT_abstract_origin: <0x9b5> <91d> DW_AT_entry_pc : 0x40117b <925> DW_AT_GNU_entry_view: 0 <926> DW_AT_low_pc : 0x40117b <92e> DW_AT_high_pc : 0xa <936> DW_AT_call_file : 1 <937> DW_AT_call_line : 24 <938> DW_AT_call_column : 10 <939> DW_AT_sibling : <0x965> conceptually the subroutine starta at DW_AT_entry_pc/DW_AT_GNU_entry_view and ends at DW_AT_high_pc/<end view number>, but the end view number is not yet specified. That IMHO is what should be added here. Bernd. ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] Fix an issue with the gdb step-over aka. "n" command 2019-11-24 12:17 [PATCH] Fix an issue with the gdb step-over aka. "n" command Bernd Edlinger 2019-12-01 20:47 ` [PING] " Bernd Edlinger @ 2019-12-15 1:25 ` Simon Marchi 2019-12-15 8:39 ` Bernd Edlinger 1 sibling, 1 reply; 14+ messages in thread From: Simon Marchi @ 2019-12-15 1:25 UTC (permalink / raw) To: Bernd Edlinger, gdb-patches On 2019-11-24 7:17 a.m., Bernd Edlinger wrote: > Hi, > > this fixes an issue with the gdb step-over aka. "n" command. > > Apologies, the motivation for this patch was from sub-optimal > debug experience using some gcc code involving inlined functions, > and initially I tried to convince gcc folks that it is in fact a > gcc bug, but... > > It can be seen when you debug an optimized stage-3 cc1 > it does not affect -O0 code, though. > > Note: you can use "gcc -S hello.c -wrapper gdb,--args" to invoke cc1 with > debugger attached. > > This example debug session will explain the effect. > > (gdb) b get_alias_set > Breakpoint 5 at 0xa099f0: file ../../gcc-trunk/gcc/alias.c, line 837. > (gdb) r > Breakpoint 5, get_alias_set (t=t@entry=0x7ffff7ff7ab0) at ../../gcc-trunk/gcc/alias.c:837 > 837 if (t == error_mark_node > (gdb) n > 839 && (TREE_TYPE (t) == 0 || TREE_TYPE (t) == error_mark_node))) > (gdb) n > 3382 return __t; <-- now we have a problem: wrong line info here > (gdb) bt > #0 get_alias_set (t=t@entry=0x7ffff7ff7ab0) at ../../gcc-trunk/gcc/tree.h:3382 > #1 0x0000000000b25dfe in set_mem_attributes_minus_bitpos (ref=0x7ffff746f990, t=0x7ffff7ff7ab0, objectp=1, bitpos=...) > at ../../gcc-trunk/gcc/emit-rtl.c:1957 > #2 0x0000000001137a55 in make_decl_rtl (decl=0x7ffff7ff7ab0) at ../../gcc-trunk/gcc/varasm.c:1518 > #3 0x000000000113b6e8 in assemble_variable (decl=0x7ffff7ff7ab0, top_level=<optimized out>, at_end=<optimized out>, > dont_output_data=0) at ../../gcc-trunk/gcc/varasm.c:2246 > #4 0x000000000113f0ea in varpool_node::assemble_decl (this=0x7ffff745b000) at ../../gcc-trunk/gcc/varpool.c:584 > #5 0x000000000113fa17 in varpool_node::assemble_decl (this=0x7ffff745b000) at ../../gcc-trunk/gcc/varpool.c:750 I have a hard time understanding what is going wrong and what we should see instead. I think it would help if you showed what's $pc at every place where you are stopping, as well as the output for readelf --debug-dump=decodedline for those regions. It would also help if you provided the same session without and with your patch applied, so we could see the difference. Simon ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] Fix an issue with the gdb step-over aka. "n" command 2019-12-15 1:25 ` Simon Marchi @ 2019-12-15 8:39 ` Bernd Edlinger 2019-12-19 22:53 ` Bernd Edlinger 0 siblings, 1 reply; 14+ messages in thread From: Bernd Edlinger @ 2019-12-15 8:39 UTC (permalink / raw) To: Simon Marchi, gdb-patches On 12/15/19 2:25 AM, Simon Marchi wrote: > On 2019-11-24 7:17 a.m., Bernd Edlinger wrote: >> Hi, >> >> this fixes an issue with the gdb step-over aka. "n" command. >> >> Apologies, the motivation for this patch was from sub-optimal >> debug experience using some gcc code involving inlined functions, >> and initially I tried to convince gcc folks that it is in fact a >> gcc bug, but... >> >> It can be seen when you debug an optimized stage-3 cc1 >> it does not affect -O0 code, though. >> >> Note: you can use "gcc -S hello.c -wrapper gdb,--args" to invoke cc1 with >> debugger attached. >> >> This example debug session will explain the effect. >> >> (gdb) b get_alias_set >> Breakpoint 5 at 0xa099f0: file ../../gcc-trunk/gcc/alias.c, line 837. >> (gdb) r >> Breakpoint 5, get_alias_set (t=t@entry=0x7ffff7ff7ab0) at ../../gcc-trunk/gcc/alias.c:837 >> 837 if (t == error_mark_node >> (gdb) n >> 839 && (TREE_TYPE (t) == 0 || TREE_TYPE (t) == error_mark_node))) >> (gdb) n >> 3382 return __t; <-- now we have a problem: wrong line info here >> (gdb) bt >> #0 get_alias_set (t=t@entry=0x7ffff7ff7ab0) at ../../gcc-trunk/gcc/tree.h:3382 >> #1 0x0000000000b25dfe in set_mem_attributes_minus_bitpos (ref=0x7ffff746f990, t=0x7ffff7ff7ab0, objectp=1, bitpos=...) >> at ../../gcc-trunk/gcc/emit-rtl.c:1957 >> #2 0x0000000001137a55 in make_decl_rtl (decl=0x7ffff7ff7ab0) at ../../gcc-trunk/gcc/varasm.c:1518 >> #3 0x000000000113b6e8 in assemble_variable (decl=0x7ffff7ff7ab0, top_level=<optimized out>, at_end=<optimized out>, >> dont_output_data=0) at ../../gcc-trunk/gcc/varasm.c:2246 >> #4 0x000000000113f0ea in varpool_node::assemble_decl (this=0x7ffff745b000) at ../../gcc-trunk/gcc/varpool.c:584 >> #5 0x000000000113fa17 in varpool_node::assemble_decl (this=0x7ffff745b000) at ../../gcc-trunk/gcc/varpool.c:750 > > I have a hard time understanding what is going wrong and what we should see > instead. I think it would help if you showed what's $pc at every place where > you are stopping, as well as the output for readelf --debug-dump=decodedline > for those regions. It would also help if you provided the same session without > and with your patch applied, so we could see the difference. > Hi Simon, the issue is there is a line-info from the inline function right at the end of the inline superblock without any code just variable tacking info there. Maybe it helps to get the background if you look at my attempt of fixing this as a gcc bug: https://gcc.gnu.org/ml/gcc-patches/2019-10/msg01459.html and Alexandre Oliva's response here: https://gcc.gnu.org/ml/gcc-patches/2019-11/msg01771.html So this is a problem in the design of the dwarf line info which has stmt-type line info at the end of a inlined subroutine. Those fall outside the inline block, therefore the step over stops at the line where the inline function ends, and for gdb it just appears as if this line was in the calling subroutine, which happens since the line info does not have any connection to the inlined subroutine range info. Contents of the .debug_info section: <2><4f686>: Abbrev Number: 12 (DW_TAG_inlined_subroutine) <4f687> DW_AT_abstract_origin: <0x53d4e> <4f68b> DW_AT_entry_pc : 0x7280 <4f693> DW_AT_GNU_entry_view: 1 <4f695> DW_AT_ranges : 0xb480 <4f699> DW_AT_call_file : 8 <- alias.c <4f69a> DW_AT_call_line : 839 <4f69c> DW_AT_call_column : 8 <4f69d> DW_AT_sibling : <0x4f717> The File Name Table (offset 0x253): 8 2 0 0 alias.c 10 2 0 0 tree.h Contents of the .debug_ranges section: 0000b480 0000000000007280 0000000000007291 0000b480 0000000000002764 000000000000277e 0000b480 <End of list> The problem is at pc=0x7291 in the Line Number Section: Line Number Statements: [0x00008826] Special opcode 61: advance Address by 4 to 0x7284 and Line by 0 to 3380 [0x00008827] Set is_stmt to 1 [0x00008828] Special opcode 189: advance Address by 13 to 0x7291 and Line by 2 to 3382 (*) [0x00008829] Set is_stmt to 0 (**) [0x0000882a] Copy (view 1) [0x0000882b] Set File Name to entry 8 in the File Name Table <- back to alias.c [0x0000882d] Set column to 8 [0x0000882f] Advance Line by -2543 to 839 [0x00008832] Copy (view 2) [0x00008833] Set column to 27 [0x00008835] Special opcode 61: advance Address by 4 to 0x7295 and Line by 0 to 839 [0x00008836] Set column to 3 [0x00008838] Set is_stmt to 1 <-- next line info counts: alias.c:847 [0x00008839] Special opcode 153: advance Address by 10 to 0x729f and Line by 8 to 847 [0x0000883a] Set column to 7 (*) this line is tree.h:3382, but the program counter is *not* within the subroutine, but exactly at the first instruction *after* the subroutine according to the debug_ranges. What makes it worse, is that (**) makes gdb ignore the new location info alias.c:839, which means, normally the n command would have continued to pc=0x729f, which is at alias.c:847. What this patch does, is a heuristic, that means when the last satement line number (*) contains no code, and is followed by a non-statment line number in another file, then pretend the non-statement (**) was actually a stmt-type line number. By adding the end of sequence marker here this code in buildsym.c cancels the last line number in the inline file: /* Normally, we treat lines as unsorted. But the end of sequence marker is special. We sort line markers at the same PC by line number, so end of sequence markers (which have line == 0) appear first. This is right if the marker ends the previous function, and there is no padding before the next function. But it is wrong if the previous line was empty and we are now marking a switch to a different subfile. We must leave the end of sequence marker at the end of this group of lines, not sort the empty line to after the marker. The easiest way to accomplish this is to delete any empty lines from our table, if they are followed by end of sequence markers. All we lose is the ability to set breakpoints at some lines which contain no instructions anyway. */ if (line == 0 && subfile->line_vector->nitems > 0) { e = subfile->line_vector->item + subfile->line_vector->nitems - 1; while (subfile->line_vector->nitems > 0 && e->pc == pc) { e--; subfile->line_vector->nitems--; } } (That's where I discovered the line number 65535 issue BTW) Bernd. ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] Fix an issue with the gdb step-over aka. "n" command 2019-12-15 8:39 ` Bernd Edlinger @ 2019-12-19 22:53 ` Bernd Edlinger 2019-12-20 6:13 ` Simon Marchi 0 siblings, 1 reply; 14+ messages in thread From: Bernd Edlinger @ 2019-12-19 22:53 UTC (permalink / raw) To: Simon Marchi, gdb-patches; +Cc: Tom Tromey On 12/15/19 9:39 AM, Bernd Edlinger wrote: > On 12/15/19 2:25 AM, Simon Marchi wrote: >> On 2019-11-24 7:17 a.m., Bernd Edlinger wrote: >>> Hi, >>> >>> this fixes an issue with the gdb step-over aka. "n" command. >>> >>> Apologies, the motivation for this patch was from sub-optimal >>> debug experience using some gcc code involving inlined functions, >>> and initially I tried to convince gcc folks that it is in fact a >>> gcc bug, but... >>> >>> It can be seen when you debug an optimized stage-3 cc1 >>> it does not affect -O0 code, though. >>> >>> Note: you can use "gcc -S hello.c -wrapper gdb,--args" to invoke cc1 with >>> debugger attached. >>> >>> This example debug session will explain the effect. >>> >>> (gdb) b get_alias_set >>> Breakpoint 5 at 0xa099f0: file ../../gcc-trunk/gcc/alias.c, line 837. >>> (gdb) r >>> Breakpoint 5, get_alias_set (t=t@entry=0x7ffff7ff7ab0) at ../../gcc-trunk/gcc/alias.c:837 >>> 837 if (t == error_mark_node >>> (gdb) n >>> 839 && (TREE_TYPE (t) == 0 || TREE_TYPE (t) == error_mark_node))) >>> (gdb) n >>> 3382 return __t; <-- now we have a problem: wrong line info here >>> (gdb) bt >>> #0 get_alias_set (t=t@entry=0x7ffff7ff7ab0) at ../../gcc-trunk/gcc/tree.h:3382 >>> #1 0x0000000000b25dfe in set_mem_attributes_minus_bitpos (ref=0x7ffff746f990, t=0x7ffff7ff7ab0, objectp=1, bitpos=...) >>> at ../../gcc-trunk/gcc/emit-rtl.c:1957 >>> #2 0x0000000001137a55 in make_decl_rtl (decl=0x7ffff7ff7ab0) at ../../gcc-trunk/gcc/varasm.c:1518 >>> #3 0x000000000113b6e8 in assemble_variable (decl=0x7ffff7ff7ab0, top_level=<optimized out>, at_end=<optimized out>, >>> dont_output_data=0) at ../../gcc-trunk/gcc/varasm.c:2246 >>> #4 0x000000000113f0ea in varpool_node::assemble_decl (this=0x7ffff745b000) at ../../gcc-trunk/gcc/varpool.c:584 >>> #5 0x000000000113fa17 in varpool_node::assemble_decl (this=0x7ffff745b000) at ../../gcc-trunk/gcc/varpool.c:750 >> >> I have a hard time understanding what is going wrong and what we should see >> instead. I think it would help if you showed what's $pc at every place where >> you are stopping, as well as the output for readelf --debug-dump=decodedline >> for those regions. It would also help if you provided the same session without >> and with your patch applied, so we could see the difference. >> > > Hi Simon, > > the issue is there is a line-info from the inline function right at the end of the > inline superblock without any code just variable tacking info there. > > Maybe it helps to get the background if you look at my attempt of fixing this as > a gcc bug: > > https://gcc.gnu.org/ml/gcc-patches/2019-10/msg01459.html > > and Alexandre Oliva's response here: > > https://gcc.gnu.org/ml/gcc-patches/2019-11/msg01771.html > > > So this is a problem in the design of the dwarf line info which > has stmt-type line info at the end of a inlined subroutine. > > Those fall outside the inline block, therefore the step over stops > at the line where the inline function ends, and for gdb it just appears > as if this line was in the calling subroutine, which happens since > the line info does not have any connection to the inlined subroutine > range info. > > Contents of the .debug_info section: > > <2><4f686>: Abbrev Number: 12 (DW_TAG_inlined_subroutine) > <4f687> DW_AT_abstract_origin: <0x53d4e> > <4f68b> DW_AT_entry_pc : 0x7280 > <4f693> DW_AT_GNU_entry_view: 1 > <4f695> DW_AT_ranges : 0xb480 > <4f699> DW_AT_call_file : 8 <- alias.c > <4f69a> DW_AT_call_line : 839 > <4f69c> DW_AT_call_column : 8 > <4f69d> DW_AT_sibling : <0x4f717> > > The File Name Table (offset 0x253): > 8 2 0 0 alias.c > 10 2 0 0 tree.h > > Contents of the .debug_ranges section: > > 0000b480 0000000000007280 0000000000007291 > 0000b480 0000000000002764 000000000000277e > 0000b480 <End of list> > > The problem is at pc=0x7291 in the Line Number Section: > > Line Number Statements: > > [0x00008826] Special opcode 61: advance Address by 4 to 0x7284 and Line by 0 to 3380 > [0x00008827] Set is_stmt to 1 > [0x00008828] Special opcode 189: advance Address by 13 to 0x7291 and Line by 2 to 3382 (*) > [0x00008829] Set is_stmt to 0 (**) > [0x0000882a] Copy (view 1) > [0x0000882b] Set File Name to entry 8 in the File Name Table <- back to alias.c > [0x0000882d] Set column to 8 > [0x0000882f] Advance Line by -2543 to 839 > [0x00008832] Copy (view 2) > [0x00008833] Set column to 27 > [0x00008835] Special opcode 61: advance Address by 4 to 0x7295 and Line by 0 to 839 > [0x00008836] Set column to 3 > [0x00008838] Set is_stmt to 1 <-- next line info counts: alias.c:847 > [0x00008839] Special opcode 153: advance Address by 10 to 0x729f and Line by 8 to 847 > [0x0000883a] Set column to 7 > > (*) this line is tree.h:3382, but the program counter is *not* within the subroutine, > but exactly at the first instruction *after* the subroutine according to the debug_ranges. > > What makes it worse, is that (**) makes gdb ignore the new location info alias.c:839, > which means, normally the n command would have continued to pc=0x729f, which is at alias.c:847. > > > What this patch does, is a heuristic, that means when the last satement line number (*) > contains no code, and is followed by a non-statment line number in another file, then > pretend the non-statement (**) was actually a stmt-type line number. By adding the > end of sequence marker here this code in buildsym.c cancels the last line number in the > inline file: > > /* Normally, we treat lines as unsorted. But the end of sequence > marker is special. We sort line markers at the same PC by line > number, so end of sequence markers (which have line == 0) appear > first. This is right if the marker ends the previous function, > and there is no padding before the next function. But it is > wrong if the previous line was empty and we are now marking a > switch to a different subfile. We must leave the end of sequence > marker at the end of this group of lines, not sort the empty line > to after the marker. The easiest way to accomplish this is to > delete any empty lines from our table, if they are followed by > end of sequence markers. All we lose is the ability to set > breakpoints at some lines which contain no instructions > anyway. */ > if (line == 0 && subfile->line_vector->nitems > 0) > { > e = subfile->line_vector->item + subfile->line_vector->nitems - 1; > while (subfile->line_vector->nitems > 0 && e->pc == pc) > { > e--; > subfile->line_vector->nitems--; > } > } > > > (That's where I discovered the line number 65535 issue BTW) > > > Bernd. > Does this explanation make sense? Maybe it helps, but I have been able to come up with a test case. It uses C++ and needs to be compiled with -Og or higher, with recent gcc versions, the next command steps multiple times into the header file, where the inline function is, that we ought to step over. Since I already know that in optimized build stepping may or may not stop in the opening brace, dependent on the gcc version, I merged the { with the first statement, to make the test case more stable although normally that would be avoided, but having any test case at all might be worth it. Would that be okay as a test case, it appears to work with gcc 4.8 (where the test case works also unpatched gdb) and gcc-Version 10.0.0 20191123, where passed only with patched gdb) rom a9bf22888b4655d104c7956894a96a542bcbdd57 Mon Sep 17 00:00:00 2001 From: Bernd Edlinger <bernd.edlinger@hotmail.de> Date: Thu, 19 Dec 2019 23:41:37 +0100 Subject: [PATCH] Add a test case for step over inline functions gdb/testsuite: 2019-12-19 Bernd Edlinger <bernd.edlinger@hotmail.de> * gdb.cp/next-inline.exp: New file. * gdb.cp/next-inline.cc: New file. * gdb.cp/next-inline.h: New file. --- gdb/testsuite/gdb.cp/next-inline.cc | 34 +++++++++++++++++++++++++++++++ gdb/testsuite/gdb.cp/next-inline.exp | 39 ++++++++++++++++++++++++++++++++++++ gdb/testsuite/gdb.cp/next-inline.h | 34 +++++++++++++++++++++++++++++++ 3 files changed, 107 insertions(+) create mode 100644 gdb/testsuite/gdb.cp/next-inline.cc create mode 100644 gdb/testsuite/gdb.cp/next-inline.exp create mode 100644 gdb/testsuite/gdb.cp/next-inline.h diff --git a/gdb/testsuite/gdb.cp/next-inline.cc b/gdb/testsuite/gdb.cp/next-inline.cc new file mode 100644 index 0000000..dcf5ab9 --- /dev/null +++ b/gdb/testsuite/gdb.cp/next-inline.cc @@ -0,0 +1,34 @@ +/* This testcase is part of GDB, the GNU debugger. + + 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/>. */ + +#include "next-inline.h" + +int __attribute__((noinline, noclone)) +get_alias_set (tree *t) +{ if (t != NULL + && TREE_TYPE (t).z != 1 + && TREE_TYPE (t).z != 2 + && TREE_TYPE (t).z != 3) + return 0; + return 1; +} + +tree xx; +int main() +{ get_alias_set(&xx); + abort(); +} diff --git a/gdb/testsuite/gdb.cp/next-inline.exp b/gdb/testsuite/gdb.cp/next-inline.exp new file mode 100644 index 0000000..6badc8c --- /dev/null +++ b/gdb/testsuite/gdb.cp/next-inline.exp @@ -0,0 +1,39 @@ +# 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/>. + +standard_testfile + +if { [prepare_for_testing "failed to prepare" "next-inline" \ + {next-inline.cc} \ + {debug nowarnings optimize=-O2}] } { + return -1 +} + +if ![runto_main] { + fail "can't run to main" + return +} + +gdb_test "bt" "\\s*\\#0\\s+main.*" "in main" +gdb_test "step" ".*" "step into get_alias_set" +gdb_test "bt" "\\s*\\#0\\s+\[^\r\]*next-inline\\.cc:.*" "not in inline" +gdb_test "next" ".*" "next step 1" +gdb_test "bt" "\\s*\\#0\\s+\[^\r\]*next-inline\\.cc:.*" "not in inline 1" +gdb_test "next" ".*" "next step 2" +gdb_test "bt" "\\s*\\#0\\s+\[^\r\]*next-inline\\.cc:.*" "not in inline 2" +gdb_test "next" ".*" "next step 3" +gdb_test "bt" "\\s*\\#0\\s+\[^\r\]*next-inline\\.cc:.*" "not in inline 3" +gdb_test "next" ".*" "next step 4" +gdb_test "bt" "\\s*\\#0\\s+\[^\r\]*next-inline\\.cc:.*" "not in inline 4" diff --git a/gdb/testsuite/gdb.cp/next-inline.h b/gdb/testsuite/gdb.cp/next-inline.h new file mode 100644 index 0000000..99fb1b2 --- /dev/null +++ b/gdb/testsuite/gdb.cp/next-inline.h @@ -0,0 +1,34 @@ +/* This testcase is part of GDB, the GNU debugger. + + 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/>. */ + +#include <stdlib.h> + +struct tree{ + volatile int x; + volatile int z; +}; + +#define TREE_TYPE(NODE) (*tree_check (NODE, 0)) + +inline tree * +tree_check (tree *t, int i) +{ + if (t->x != i) + abort(); + tree *x = t; + return x; +} -- 1.9.1 Thanks Bernd. ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] Fix an issue with the gdb step-over aka. "n" command 2019-12-19 22:53 ` Bernd Edlinger @ 2019-12-20 6:13 ` Simon Marchi 2019-12-20 19:57 ` Bernd Edlinger 2019-12-28 8:40 ` Bernd Edlinger 0 siblings, 2 replies; 14+ messages in thread From: Simon Marchi @ 2019-12-20 6:13 UTC (permalink / raw) To: Bernd Edlinger, gdb-patches; +Cc: Tom Tromey Hi Bernd, On 2019-12-19 5:53 p.m., Bernd Edlinger wrote: > Does this explanation make sense? Yes. Well I think so, I have to admit this is a bit over my head, there are a lot of pieces to put together to have a good understanding of the problem. I just did a first read, I'll sleep on it and come back to it later. Thanks for the small reproducer, this is extremely valuable. I think it will be a good idea to integrate it as a test case in the test suite. In your patch to dwarf2read.c, I was a bit surprised to see: m_last_subfile != m_cu->get_builder ()->get_current_subfile () So your fix only works if the inlined subroutine comes from another file? If I move the tree_check function in next-inline.cc, the fix no longer applies, and we get the broken behavior. From your previous email, I understand that this is expected. I guess that if both are in the same file, we can't detect this situation using the same technique. I also read about location views, since that's what Alexandre referred to. It sounds like it's a magic solution that will allow GDB to do the right thing in this kind of situation. If that's indeed the case, then it might be good to start exploring this path. I'd like to have a better understanding of how this will help GDB make a smarter "next", and what kind of effort is needed to make GDB use it. My understanding is that location views allow having an address mapped to multiple source locations. For example, here's the problematic address in the next-inline test case I've compiled: ./next-inline.h:[++] next-inline.h 28 0x1175 x next-inline.h 30 0x1175 1 x ./next-inline.cc:[++] next-inline.cc 22 0x1175 2 Today, when I ask GDB "which source line does this address correspond to", it gives me one answer. Does this mean that GDB will now say that 0x1175 corresponds to - next-inline.h:28 - next-inline.h:30 - next-inline.cc:22 all at the same time? Is one of these source locations more important than the others? If execution happens to stop exactly at this address, which location do we present to the user? And to come back the problem at hand, how does this help GDB make a smarter "next"? Btw, I stumbled on a bug with the TUI source display. It might be caused by this patch, or it might be that this patch uncovers it. When I do these actions: - Start GDB with the next-inline test file (from this patch) - Enable the TUI - Type "start" - Type "s" - Type "n" twice The TUI source display wrongfully jumps to the header file, line 24. When I type "frame", it says I'm stopped at next-line.cc:24. So it is showing the right line number of the wrong file. Thanks, Simon ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] Fix an issue with the gdb step-over aka. "n" command 2019-12-20 6:13 ` Simon Marchi @ 2019-12-20 19:57 ` Bernd Edlinger 2019-12-28 8:40 ` Bernd Edlinger 1 sibling, 0 replies; 14+ messages in thread From: Bernd Edlinger @ 2019-12-20 19:57 UTC (permalink / raw) To: Simon Marchi, gdb-patches; +Cc: Tom Tromey [-- Attachment #1: Type: text/plain, Size: 4081 bytes --] On 12/20/19 7:13 AM, Simon Marchi wrote: > Hi Bernd, > > On 2019-12-19 5:53 p.m., Bernd Edlinger wrote: >> Does this explanation make sense? > > Yes. Well I think so, I have to admit this is a bit over my head, > there are a lot of pieces to put together to have a good understanding > of the problem. I just did a first read, I'll sleep on it and come > back to it later. > > Thanks for the small reproducer, this is extremely valuable. I think it > will be a good idea to integrate it as a test case in the test suite. > > In your patch to dwarf2read.c, I was a bit surprised to see: > > m_last_subfile != m_cu->get_builder ()->get_current_subfile () > > So your fix only works if the inlined subroutine comes from another file? If > I move the tree_check function in next-inline.cc, the fix no longer applies, > and we get the broken behavior. From your previous email, I understand that > this is expected. I guess that if both are in the same file, we can't detect > this situation using the same technique. Yes, when the inline function is not in a header file that will not help. But it solves 90% of the problem with a simple and obvious heuristic. To attack the rest of the problem we would need to know the PCs where inlined subroutines and each corresponding range infos do end, but that data is only available long after the line info is parsed. > > I also read about location views, since that's what Alexandre referred to. It > sounds like it's a magic solution that will allow GDB to do the right thing in > this kind of situation. If that's indeed the case, then it might be good to start > exploring this path. I'd like to have a better understanding of how this will help > GDB make a smarter "next", and what kind of effort is needed to make GDB use it. My > understanding is that location views allow having an address mapped to multiple > source locations. For example, here's the problematic address in the next-inline > test case I've compiled: > > ./next-inline.h:[++] > next-inline.h 28 0x1175 x > next-inline.h 30 0x1175 1 x > > ./next-inline.cc:[++] > next-inline.cc 22 0x1175 2 > I think the main problem here, is that from the line numbers alone it is not clear which of these location infos is in the subroutine and which is in the caller. The only link is the program address which is ambiguous at the end of the inline block. So my impression is that we need a connection between the location views and the inlined subroutine info. > Today, when I ask GDB "which source line does this address correspond to", it gives me > one answer. Does this mean that GDB will now say that 0x1175 corresponds to > > - next-inline.h:28 > - next-inline.h:30 > - next-inline.cc:22 > > all at the same time? Is one of these source locations more important than the others? > If execution happens to stop exactly at this address, which location do we present to > the user? > No idea. That will likely be confusing. > And to come back the problem at hand, how does this help GDB make a smarter "next"? > > Btw, I stumbled on a bug with the TUI source display. It might be caused by this patch, > or it might be that this patch uncovers it. > > When I do these actions: > > - Start GDB with the next-inline test file (from this patch) > - Enable the TUI > - Type "start" > - Type "s" > - Type "n" twice > > The TUI source display wrongfully jumps to the header file, line 24. > When I type "frame", it says I'm stopped at next-line.cc:24. So it > is showing the right line number of the wrong file. > Ah, yes. That is already preexistent. Consider the attached idea for a test case. I have no idea yet how to make a working test case out if it. But I can fix the tui bug, it is quite easy. I will send a patch for that in a moment. Thanks Bernd. [-- Warning: decoded text below may be mangled, UTF-8 assumed --] [-- Attachment #2: step.c --] [-- Type: text/x-csrc; name="step.c", Size: 917 bytes --] /* This testcase is part of GDB, the GNU debugger. 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/>. */ #include <stdlib.h> __attribute__((noinline, noclone)) static void bar () { } #include "step.h" int main () { int x; x = 0; foo (); abort (); return x; } [-- Warning: decoded text below may be mangled, UTF-8 assumed --] [-- Attachment #3: step.h --] [-- Type: text/x-chdr; name="step.h", Size: 827 bytes --] /* This testcase is part of GDB, the GNU debugger. 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/>. */ __attribute__((__always_inline__)) static inline int foo (void) { bar (); } ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] Fix an issue with the gdb step-over aka. "n" command 2019-12-20 6:13 ` Simon Marchi 2019-12-20 19:57 ` Bernd Edlinger @ 2019-12-28 8:40 ` Bernd Edlinger 1 sibling, 0 replies; 14+ messages in thread From: Bernd Edlinger @ 2019-12-28 8:40 UTC (permalink / raw) To: Simon Marchi, gdb-patches; +Cc: Tom Tromey [-- Attachment #1: Type: text/plain, Size: 3754 bytes --] On 12/20/19 7:13 AM, Simon Marchi wrote: > Hi Bernd, > > On 2019-12-19 5:53 p.m., Bernd Edlinger wrote: >> Does this explanation make sense? > > Yes. Well I think so, I have to admit this is a bit over my head, > there are a lot of pieces to put together to have a good understanding > of the problem. I just did a first read, I'll sleep on it and come > back to it later. > > Thanks for the small reproducer, this is extremely valuable. I think it > will be a good idea to integrate it as a test case in the test suite. > > In your patch to dwarf2read.c, I was a bit surprised to see: > > m_last_subfile != m_cu->get_builder ()->get_current_subfile () > > So your fix only works if the inlined subroutine comes from another file? If > I move the tree_check function in next-inline.cc, the fix no longer applies, > and we get the broken behavior. From your previous email, I understand that > this is expected. I guess that if both are in the same file, we can't detect > this situation using the same technique. > > I also read about location views, since that's what Alexandre referred to. It > sounds like it's a magic solution that will allow GDB to do the right thing in > this kind of situation. If that's indeed the case, then it might be good to start > exploring this path. I'd like to have a better understanding of how this will help > GDB make a smarter "next", and what kind of effort is needed to make GDB use it. My > understanding is that location views allow having an address mapped to multiple > source locations. For example, here's the problematic address in the next-inline > test case I've compiled: > > ./next-inline.h:[++] > next-inline.h 28 0x1175 x > next-inline.h 30 0x1175 1 x > > ./next-inline.cc:[++] > next-inline.cc 22 0x1175 2 > > Today, when I ask GDB "which source line does this address correspond to", it gives me > one answer. Does this mean that GDB will now say that 0x1175 corresponds to > > - next-inline.h:28 > - next-inline.h:30 > - next-inline.cc:22 > > all at the same time? Is one of these source locations more important than the others? > If execution happens to stop exactly at this address, which location do we present to > the user? > > And to come back the problem at hand, how does this help GDB make a smarter "next"? > > Btw, I stumbled on a bug with the TUI source display. It might be caused by this patch, > or it might be that this patch uncovers it. > > When I do these actions: > > - Start GDB with the next-inline test file (from this patch) > - Enable the TUI > - Type "start" > - Type "s" > - Type "n" twice > > The TUI source display wrongfully jumps to the header file, line 24. > When I type "frame", it says I'm stopped at next-line.cc:24. So it > is showing the right line number of the wrong file. > I think meanwhile the display bug in the TUI window is fixed (by one of Tom Tromey's recent patches), could you please check again? I post both parts of the patch again for your reference here, and add a ChangeLog which I forgot previously: gdb: 2019-12-28 Bernd Edlinger <bernd.edlinger@hotmail.de> * dwarf2read.c (lnp_state_machine): New member m_last_address. (lnp_state_machine::record_line): After a file changes or end sequence always assume a statement boundary. gdb/testsuite: 2019-12-28 Bernd Edlinger <bernd.edlinger@hotmail.de> * gdb.cp/next-inline.exp: New file. * gdb.cp/next-inline.cc: New file. * gdb.cp/next-inline.h: New file. Thanks Bernd. [-- Warning: decoded text below may be mangled, UTF-8 assumed --] [-- Attachment #2: 0001-Fix-an-issue-with-the-gdb-step-over-aka.-n-command.patch --] [-- Type: text/x-patch; name="0001-Fix-an-issue-with-the-gdb-step-over-aka.-n-command.patch", Size: 2273 bytes --] From 63c0053d0c2bf9ec8ffbcb06712fe8aa243c32fc Mon Sep 17 00:00:00 2001 From: Bernd Edlinger <bernd.edlinger@hotmail.de> Date: Sat, 23 Nov 2019 20:59:25 +0100 Subject: [PATCH 1/2] Fix an issue with the gdb step-over aka. "n" command When debugging optimized code with inlined functions built with -gstatement-frontiers debug info, it may happen that an inline function has a line location exactly at the end of a block which has the is_stmt attribute set, followed by a location info which is at the call site but has the is_stmt attribute cleared and is therefore ignored. The visual effect is that "n" does sometimes not step over the inlined function but instead jumps to the last line of the inline function, but the inline function has already returned, hence the line number information is inconsistent at this point. This patch detects a is_stmt location info followed by a location info in a different file at the same address, but without is_stmt location set, and forces the second location info to replace the previous one. --- gdb/dwarf2read.c | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/gdb/dwarf2read.c b/gdb/dwarf2read.c index 819d37c..2f9a181 100644 --- a/gdb/dwarf2read.c +++ b/gdb/dwarf2read.c @@ -21148,6 +21148,7 @@ private: example, when discriminators are present. PR 17276. */ unsigned int m_last_line = 0; bool m_line_has_non_zero_discriminator = false; + CORE_ADDR m_last_address = (CORE_ADDR) -1; }; void @@ -21330,7 +21331,10 @@ 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 + || (m_last_subfile != m_cu->get_builder ()->get_current_subfile () + && m_last_address == m_address))) { if (m_last_subfile != m_cu->get_builder ()->get_current_subfile () || end_sequence) @@ -21353,6 +21357,7 @@ lnp_state_machine::record_line (bool end_sequence) } m_last_subfile = m_cu->get_builder ()->get_current_subfile (); m_last_line = m_line; + m_last_address = m_address; } } } -- 1.9.1 [-- Warning: decoded text below may be mangled, UTF-8 assumed --] [-- Attachment #3: 0002-Add-a-test-case-for-step-over-inline-functions.patch --] [-- Type: text/x-patch; name="0002-Add-a-test-case-for-step-over-inline-functions.patch", Size: 4912 bytes --] From 95ba4780488b53104ea51cc0702f99a9a800984b Mon Sep 17 00:00:00 2001 From: Bernd Edlinger <bernd.edlinger@hotmail.de> Date: Thu, 19 Dec 2019 23:41:37 +0100 Subject: [PATCH 2/2] Add a test case for step over inline functions --- gdb/testsuite/gdb.cp/next-inline.cc | 34 +++++++++++++++++++++++++++++++ gdb/testsuite/gdb.cp/next-inline.exp | 39 ++++++++++++++++++++++++++++++++++++ gdb/testsuite/gdb.cp/next-inline.h | 34 +++++++++++++++++++++++++++++++ 3 files changed, 107 insertions(+) create mode 100644 gdb/testsuite/gdb.cp/next-inline.cc create mode 100644 gdb/testsuite/gdb.cp/next-inline.exp create mode 100644 gdb/testsuite/gdb.cp/next-inline.h diff --git a/gdb/testsuite/gdb.cp/next-inline.cc b/gdb/testsuite/gdb.cp/next-inline.cc new file mode 100644 index 0000000..dcf5ab9 --- /dev/null +++ b/gdb/testsuite/gdb.cp/next-inline.cc @@ -0,0 +1,34 @@ +/* This testcase is part of GDB, the GNU debugger. + + 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/>. */ + +#include "next-inline.h" + +int __attribute__((noinline, noclone)) +get_alias_set (tree *t) +{ if (t != NULL + && TREE_TYPE (t).z != 1 + && TREE_TYPE (t).z != 2 + && TREE_TYPE (t).z != 3) + return 0; + return 1; +} + +tree xx; +int main() +{ get_alias_set(&xx); + abort(); +} diff --git a/gdb/testsuite/gdb.cp/next-inline.exp b/gdb/testsuite/gdb.cp/next-inline.exp new file mode 100644 index 0000000..6badc8c --- /dev/null +++ b/gdb/testsuite/gdb.cp/next-inline.exp @@ -0,0 +1,39 @@ +# 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/>. + +standard_testfile + +if { [prepare_for_testing "failed to prepare" "next-inline" \ + {next-inline.cc} \ + {debug nowarnings optimize=-O2}] } { + return -1 +} + +if ![runto_main] { + fail "can't run to main" + return +} + +gdb_test "bt" "\\s*\\#0\\s+main.*" "in main" +gdb_test "step" ".*" "step into get_alias_set" +gdb_test "bt" "\\s*\\#0\\s+\[^\r\]*next-inline\\.cc:.*" "not in inline" +gdb_test "next" ".*" "next step 1" +gdb_test "bt" "\\s*\\#0\\s+\[^\r\]*next-inline\\.cc:.*" "not in inline 1" +gdb_test "next" ".*" "next step 2" +gdb_test "bt" "\\s*\\#0\\s+\[^\r\]*next-inline\\.cc:.*" "not in inline 2" +gdb_test "next" ".*" "next step 3" +gdb_test "bt" "\\s*\\#0\\s+\[^\r\]*next-inline\\.cc:.*" "not in inline 3" +gdb_test "next" ".*" "next step 4" +gdb_test "bt" "\\s*\\#0\\s+\[^\r\]*next-inline\\.cc:.*" "not in inline 4" diff --git a/gdb/testsuite/gdb.cp/next-inline.h b/gdb/testsuite/gdb.cp/next-inline.h new file mode 100644 index 0000000..99fb1b2 --- /dev/null +++ b/gdb/testsuite/gdb.cp/next-inline.h @@ -0,0 +1,34 @@ +/* This testcase is part of GDB, the GNU debugger. + + 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/>. */ + +#include <stdlib.h> + +struct tree{ + volatile int x; + volatile int z; +}; + +#define TREE_TYPE(NODE) (*tree_check (NODE, 0)) + +inline tree * +tree_check (tree *t, int i) +{ + if (t->x != i) + abort(); + tree *x = t; + return x; +} -- 1.9.1 ^ permalink raw reply [flat|nested] 14+ messages in thread
end of thread, other threads:[~2020-01-07 15:15 UTC | newest] Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2019-11-24 12:17 [PATCH] Fix an issue with the gdb step-over aka. "n" command Bernd Edlinger 2019-12-01 20:47 ` [PING] " Bernd Edlinger 2019-12-14 13:52 ` [PING**2] " Bernd Edlinger 2019-12-30 22:12 ` Andrew Burgess 2020-01-01 9:40 ` Bernd Edlinger 2020-01-06 8:14 ` [PING**3] " Bernd Edlinger 2020-01-06 22:09 ` Andrew Burgess 2020-01-07 15:15 ` Bernd Edlinger 2019-12-15 1:25 ` Simon Marchi 2019-12-15 8:39 ` Bernd Edlinger 2019-12-19 22:53 ` Bernd Edlinger 2019-12-20 6:13 ` Simon Marchi 2019-12-20 19:57 ` Bernd Edlinger 2019-12-28 8:40 ` Bernd Edlinger
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).