* [PATCH] Fix partial symbols @ 2020-11-28 18:01 Bernd Edlinger 2020-11-29 19:43 ` Andrew Burgess 0 siblings, 1 reply; 7+ messages in thread From: Bernd Edlinger @ 2020-11-28 18:01 UTC (permalink / raw) To: gdb-patches, Tom Tromey [-- Attachment #1: Type: text/plain, Size: 1419 bytes --] Hi, this fixes a regression in partial symbols. The bug was introduced with this patch: commit 529908cbd0afc2524044b7df3626c09d0bdae12d Author: Tom Tromey <tom@tromey.com> Date: Tue Sep 29 18:49:08 2020 -0600 Remove DW_UNSND This removes DW_UNSND, replacing uses with either as_unsigned or constant_value, depending primarily on whether or not the form is already known to be appropriate. gdb/ChangeLog 2020-09-29 Tom Tromey <tom@tromey.com> * dwarf2/read.c (lookup_dwo_id, get_type_unit_group) (read_file_scope, dwarf2_get_pc_bounds) (dwarf2_record_block_ranges, dwarf2_add_field, get_alignment) (read_structure_type, handle_struct_member_die) (read_enumeration_type, read_array_type, read_set_type) (read_tag_pointer_type, read_tag_reference_type) (read_subroutine_type, read_base_type, read_subrange_type) (read_full_die_1, partial_die_info::read) (partial_die_info::read, by, new_symbol) (dwarf2_const_value_data, dwarf2_const_value_attr) (dump_die_shallow, dwarf2_fetch_constant_bytes) (prepare_one_comp_unit): Update. * dwarf2/attribute.h (DW_UNSND): Remove. Due to the broken handling of DW_AT_ranges in partial_die_info::read only a subset of the complete locations is found when I set a breakpoint on an inline function. Is it OK for trunk? Thanks Bernd. [-- Warning: decoded text below may be mangled, UTF-8 assumed --] [-- Attachment #2: 0001-Fix-partial-symbols.patch --] [-- Type: text/x-patch; name="0001-Fix-partial-symbols.patch", Size: 1016 bytes --] From a2db347c00b4dac744f49a7e0d4b9689a861dc66 Mon Sep 17 00:00:00 2001 From: Bernd Edlinger <bernd.edlinger@hotmail.de> Date: Sat, 28 Nov 2020 17:29:18 +0100 Subject: [PATCH] Fix partial symbols attr.constant_value does not work for DW_FORM_rnglistx at least. Fixes: 529908cbd0a ("Remove DW_UNSND") 2020-11-28 Bernd Edlinger <bernd.edlinger@hotmail.de> * dwarf2/read.c (partial_die_info::read): Fix DW_AT_ranges. --- gdb/dwarf2/read.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/gdb/dwarf2/read.c b/gdb/dwarf2/read.c index 601a571..28244e5 100644 --- a/gdb/dwarf2/read.c +++ b/gdb/dwarf2/read.c @@ -19660,7 +19660,7 @@ struct type * /* It would be nice to reuse dwarf2_get_pc_bounds here, but that requires a full DIE, so instead we just reimplement it. */ - unsigned int ranges_offset = (attr.constant_value (0) + unsigned int ranges_offset = (attr.as_unsigned () + (need_ranges_base ? cu->ranges_base : 0)); -- 1.9.1 ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] Fix partial symbols 2020-11-28 18:01 [PATCH] Fix partial symbols Bernd Edlinger @ 2020-11-29 19:43 ` Andrew Burgess 2020-11-30 15:53 ` Bernd Edlinger 0 siblings, 1 reply; 7+ messages in thread From: Andrew Burgess @ 2020-11-29 19:43 UTC (permalink / raw) To: Bernd Edlinger; +Cc: gdb-patches, Tom Tromey * Bernd Edlinger <bernd.edlinger@hotmail.de> [2020-11-28 19:01:10 +0100]: > Hi, > > this fixes a regression in partial symbols. Is there a particular test that regressed and is fixed with this patch? It is probably worth mentioning in the commit message. If this is a regression in some functionality that is not tested in tree then it would be great to see a test added. > > The bug was introduced with this patch: > > commit 529908cbd0afc2524044b7df3626c09d0bdae12d > Author: Tom Tromey <tom@tromey.com> > Date: Tue Sep 29 18:49:08 2020 -0600 > > Remove DW_UNSND > > This removes DW_UNSND, replacing uses with either as_unsigned or > constant_value, depending primarily on whether or not the form is > already known to be appropriate. > > gdb/ChangeLog > 2020-09-29 Tom Tromey <tom@tromey.com> > > * dwarf2/read.c (lookup_dwo_id, get_type_unit_group) > (read_file_scope, dwarf2_get_pc_bounds) > (dwarf2_record_block_ranges, dwarf2_add_field, get_alignment) > (read_structure_type, handle_struct_member_die) > (read_enumeration_type, read_array_type, read_set_type) > (read_tag_pointer_type, read_tag_reference_type) > (read_subroutine_type, read_base_type, read_subrange_type) > (read_full_die_1, partial_die_info::read) > (partial_die_info::read, by, new_symbol) > (dwarf2_const_value_data, dwarf2_const_value_attr) > (dump_die_shallow, dwarf2_fetch_constant_bytes) > (prepare_one_comp_unit): Update. > * dwarf2/attribute.h (DW_UNSND): Remove. > > Due to the broken handling of DW_AT_ranges > in partial_die_info::read only a subset of the > complete locations is found when I set a breakpoint > on an inline function. This commit message is very cryptic, it assumes the reader will have knowledge of what the "broken handling of DW_AT_ranges in partial_die_info::read" is, and why this change works around this issue. A better commit message would explain what the problem is, why the current code causes issues when combined with this problem, and why the change you propose works around this issue. Maybe someone else will have more context and be able to review this change, but the clearer your commit messages the easier it is for people to review your work. Thanks, Andrew > > Is it OK for trunk? > > > Thanks > Bernd. > From a2db347c00b4dac744f49a7e0d4b9689a861dc66 Mon Sep 17 00:00:00 2001 > From: Bernd Edlinger <bernd.edlinger@hotmail.de> > Date: Sat, 28 Nov 2020 17:29:18 +0100 > Subject: [PATCH] Fix partial symbols > > attr.constant_value does not work for DW_FORM_rnglistx at least. > > Fixes: 529908cbd0a ("Remove DW_UNSND") > > 2020-11-28 Bernd Edlinger <bernd.edlinger@hotmail.de> > > * dwarf2/read.c (partial_die_info::read): Fix DW_AT_ranges. > --- > gdb/dwarf2/read.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/gdb/dwarf2/read.c b/gdb/dwarf2/read.c > index 601a571..28244e5 100644 > --- a/gdb/dwarf2/read.c > +++ b/gdb/dwarf2/read.c > @@ -19660,7 +19660,7 @@ struct type * > /* It would be nice to reuse dwarf2_get_pc_bounds here, > but that requires a full DIE, so instead we just > reimplement it. */ > - unsigned int ranges_offset = (attr.constant_value (0) > + unsigned int ranges_offset = (attr.as_unsigned () > + (need_ranges_base > ? cu->ranges_base > : 0)); > -- > 1.9.1 > ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] Fix partial symbols 2020-11-29 19:43 ` Andrew Burgess @ 2020-11-30 15:53 ` Bernd Edlinger 2020-11-30 19:26 ` [PATCH] Skip .cold functions in search_minsyms_for_name Bernd Edlinger 2020-12-18 16:23 ` [PATCHv2] Fix partial symbols Bernd Edlinger 0 siblings, 2 replies; 7+ messages in thread From: Bernd Edlinger @ 2020-11-30 15:53 UTC (permalink / raw) To: Andrew Burgess; +Cc: gdb-patches, Tom Tromey On 11/29/20 8:43 PM, Andrew Burgess wrote: > * Bernd Edlinger <bernd.edlinger@hotmail.de> [2020-11-28 19:01:10 +0100]: > >> Hi, >> >> this fixes a regression in partial symbols. > > > Is there a particular test that regressed and is fixed with this > patch? It is probably worth mentioning in the commit message. > No, I just found it by chance, since my other patch touches partial symbols, so I realized that they do not function correctly: This is what I saw while debugging gcc: $ gcc -S test.c -wrapper gdb,--args GNU gdb (GDB) 11.0.50.20201130-git Copyright (C) 2020 Free Software Foundation, Inc. License GPLv3+: GNU GPL version 3 or later <http://gnu.org/licenses/gpl.html> This is free software: you are free to change and redistribute it. There is NO WARRANTY, to the extent permitted by law. Type "show copying" and "show warranty" for details. This GDB was configured as "x86_64-pc-linux-gnu". Type "show configuration" for configuration details. For bug reporting instructions, please see: <https://www.gnu.org/software/gdb/bugs/>. Find the GDB manual and other documentation resources online at: <http://www.gnu.org/software/gdb/documentation/>. For help, type "help". Type "apropos word" to search for commands related to "word"... Reading symbols from /home/ed/gnu/install/libexec/gcc/x86_64-pc-linux-gnu/11.0.0/cc1... (gdb) b tree_class_check Breakpoint 1 at 0x67c416: tree_class_check. (3394 locations) (gdb) cle tree_class_check Deleted breakpoint 1 (gdb) b get_alias_set Breakpoint 2 at 0x67b4d1: get_alias_set. (2 locations) (gdb) cle get_alias_set Deleted breakpoint 2 (gdb) b tree_class_check Breakpoint 3 at 0x67c416: tree_class_check. (3419 locations) as you see, the number of locations of tree_class_check changes after symbols for get_alias_set are loaded. In 10.1 the numbers were stable: $ gcc -S test.c -wrapper gdb,--args GNU gdb (GDB) 10.1 Copyright (C) 2020 Free Software Foundation, Inc. License GPLv3+: GNU GPL version 3 or later <http://gnu.org/licenses/gpl.html> This is free software: you are free to change and redistribute it. There is NO WARRANTY, to the extent permitted by law. Type "show copying" and "show warranty" for details. This GDB was configured as "x86_64-pc-linux-gnu". Type "show configuration" for configuration details. For bug reporting instructions, please see: <https://www.gnu.org/software/gdb/bugs/>. Find the GDB manual and other documentation resources online at: <http://www.gnu.org/software/gdb/documentation/>. For help, type "help". Type "apropos word" to search for commands related to "word"... Reading symbols from /home/ed/gnu/install/libexec/gcc/x86_64-pc-linux-gnu/11.0.0/cc1... (gdb) b tree_class_check Breakpoint 1 at 0x67c416: tree_class_check. (7332 locations) (gdb) cle tree_class_check Deleted breakpoint 1 (gdb) b get_alias_set Breakpoint 2 at 0x955150: file ../../gcc-trunk/gcc/alias.c, line 868. (gdb) cle get_alias_set Deleted breakpoint 2 (gdb) b tree_class_check Breakpoint 3 at 0x67c416: tree_class_check. (7332 locations) after this patch the numbers are almost the same as with 10.1: $ gcc -S test.c -wrapper gdb,--args GNU gdb (GDB) 11.0.50.20201130-git Copyright (C) 2020 Free Software Foundation, Inc. License GPLv3+: GNU GPL version 3 or later <http://gnu.org/licenses/gpl.html> This is free software: you are free to change and redistribute it. There is NO WARRANTY, to the extent permitted by law. Type "show copying" and "show warranty" for details. This GDB was configured as "x86_64-pc-linux-gnu". Type "show configuration" for configuration details. For bug reporting instructions, please see: <https://www.gnu.org/software/gdb/bugs/>. Find the GDB manual and other documentation resources online at: <http://www.gnu.org/software/gdb/documentation/>. For help, type "help". Type "apropos word" to search for commands related to "word"... Reading symbols from /home/ed/gnu/install/libexec/gcc/x86_64-pc-linux-gnu/11.0.0/cc1... (gdb) b tree_class_check Breakpoint 1 at 0x67c416: tree_class_check. (7332 locations) (gdb) cle tree_class_check Deleted breakpoint 1 (gdb) b get_alias_set Breakpoint 2 at 0x67b4d1: get_alias_set. (2 locations) (gdb) cle get_alias_set Deleted breakpoint 2 (gdb) b tree_class_check Breakpoint 3 at 0x67c416: tree_class_check. (7332 locations) (gdb) cle tree_class_check Deleted breakpoint 3 Note however, that there is still a regression with the symbols of get_alias_set. There are two breakpoints, one of them is bogus. Bisection shows this patch caused that one: commit 77f2120b200be6cabbf6f610942fc1173a8df6d3 Author: Pedro Alves <pedro@palves.net> Date: Sun Sep 13 13:34:10 2020 +0100 I'll post a patch for that later. > If this is a regression in some functionality that is not tested in > tree then it would be great to see a test added. > I have the impression that it does never happen in small programs. So far I have no idea for a patch. The gdb.dwarf2/dw2-empty-pc-range.exp is probably testing something similar, a different parsing in partial symbols and normal symbols. Here we have also a parse-error that happens with the partial symbols but not in the normal symbol parser. > >> >> The bug was introduced with this patch: >> >> commit 529908cbd0afc2524044b7df3626c09d0bdae12d >> Author: Tom Tromey <tom@tromey.com> >> Date: Tue Sep 29 18:49:08 2020 -0600 >> >> Remove DW_UNSND >> >> This removes DW_UNSND, replacing uses with either as_unsigned or >> constant_value, depending primarily on whether or not the form is >> already known to be appropriate. >> >> gdb/ChangeLog >> 2020-09-29 Tom Tromey <tom@tromey.com> >> >> * dwarf2/read.c (lookup_dwo_id, get_type_unit_group) >> (read_file_scope, dwarf2_get_pc_bounds) >> (dwarf2_record_block_ranges, dwarf2_add_field, get_alignment) >> (read_structure_type, handle_struct_member_die) >> (read_enumeration_type, read_array_type, read_set_type) >> (read_tag_pointer_type, read_tag_reference_type) >> (read_subroutine_type, read_base_type, read_subrange_type) >> (read_full_die_1, partial_die_info::read) >> (partial_die_info::read, by, new_symbol) >> (dwarf2_const_value_data, dwarf2_const_value_attr) >> (dump_die_shallow, dwarf2_fetch_constant_bytes) >> (prepare_one_comp_unit): Update. >> * dwarf2/attribute.h (DW_UNSND): Remove. >> >> Due to the broken handling of DW_AT_ranges >> in partial_die_info::read only a subset of the >> complete locations is found when I set a breakpoint >> on an inline function. > > This commit message is very cryptic, it assumes the reader will have > knowledge of what the "broken handling of DW_AT_ranges in > partial_die_info::read" is, and why this change works around this > issue. > > A better commit message would explain what the problem is, why the > current code causes issues when combined with this problem, and why > the change you propose works around this issue. > Ok, I'll add this to the commit message: [PATCH] Fix partial symbols The DW_AT_ranges attribute is of type DW_FORM_sec_offset or DW_FORM_rnglistx. But the function attribute attribute::constant_value () does only handle DW_FORM_sdata, DW_FORM_implicit_const, DW_FORM_udata DW_FORM_data1, DW_FORM_data2, DW_FORM_data4, DW_FORM_data8 and returns the default value for anything else. Therefore the wrong range is parsed. Fixes: 529908cbd0a ("Remove DW_UNSND") > Maybe someone else will have more context and be able to review this > change, but the clearer your commit messages the easier it is for > people to review your work. > > Thanks, > Andrew > >> >> Is it OK for trunk? >> >> >> Thanks >> Bernd. > >> From a2db347c00b4dac744f49a7e0d4b9689a861dc66 Mon Sep 17 00:00:00 2001 >> From: Bernd Edlinger <bernd.edlinger@hotmail.de> >> Date: Sat, 28 Nov 2020 17:29:18 +0100 >> Subject: [PATCH] Fix partial symbols >> >> attr.constant_value does not work for DW_FORM_rnglistx at least. >> >> Fixes: 529908cbd0a ("Remove DW_UNSND") >> >> 2020-11-28 Bernd Edlinger <bernd.edlinger@hotmail.de> >> >> * dwarf2/read.c (partial_die_info::read): Fix DW_AT_ranges. >> --- >> gdb/dwarf2/read.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/gdb/dwarf2/read.c b/gdb/dwarf2/read.c >> index 601a571..28244e5 100644 >> --- a/gdb/dwarf2/read.c >> +++ b/gdb/dwarf2/read.c >> @@ -19660,7 +19660,7 @@ struct type * >> /* It would be nice to reuse dwarf2_get_pc_bounds here, >> but that requires a full DIE, so instead we just >> reimplement it. */ >> - unsigned int ranges_offset = (attr.constant_value (0) >> + unsigned int ranges_offset = (attr.as_unsigned () >> + (need_ranges_base >> ? cu->ranges_base >> : 0)); >> -- >> 1.9.1 >> > ^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH] Skip .cold functions in search_minsyms_for_name 2020-11-30 15:53 ` Bernd Edlinger @ 2020-11-30 19:26 ` Bernd Edlinger 2020-12-18 15:57 ` [PING] " Bernd Edlinger 2020-12-18 16:23 ` [PATCHv2] Fix partial symbols Bernd Edlinger 1 sibling, 1 reply; 7+ messages in thread From: Bernd Edlinger @ 2020-11-30 19:26 UTC (permalink / raw) To: Andrew Burgess; +Cc: gdb-patches, Tom Tromey [-- Attachment #1: Type: text/plain, Size: 923 bytes --] Hi, this fixes another regression that is cold functions were previously skipped, but that filtering was removed with 77f2120b200. However the cold functions are not the external interface, but they just contain the unlikely code paths. When a normal function entry is found, they are generally preferred over the .cold function. When a function with the same name is also available, that is preferred. The .cold function is not the external interface. Fixes 77f2120b200 ("Don't drop static function bp locations w/o debug info") gdb: 2020-11-30 Bernd Edlinger <bernd.edlinger@hotmail.de> * linespec.c (search_minsyms_for_name): Filter .cold functions when a real function with the same name is found. gdb/testsuite: 2020-11-30 Bernd Edlinger <bernd.edlinger@hotmail.de> * gdb.cp/step-and-next-inline-1.exp: New test. Tested on x86_64-pc-linux-gnu. Is it OK for trunk? Thanks Bernd. [-- Warning: decoded text below may be mangled, UTF-8 assumed --] [-- Attachment #2: 0001-Skip-.cold-functions-in-search_minsyms_for_name.patch --] [-- Type: text/x-patch; name="0001-Skip-.cold-functions-in-search_minsyms_for_name.patch", Size: 3437 bytes --] From d51a8e7740c3e8030c3ece06f117dd9312ce6ace Mon Sep 17 00:00:00 2001 From: Bernd Edlinger <bernd.edlinger@hotmail.de> Date: Mon, 30 Nov 2020 19:37:08 +0100 Subject: [PATCH] Skip .cold functions in search_minsyms_for_name When a function with the same name is also available, that is preferred. The .cold function is not the external interface. Fixes 77f2120b200 ("Don't drop static function bp locations w/o debug info") gdb: 2020-11-30 Bernd Edlinger <bernd.edlinger@hotmail.de> * linespec.c (search_minsyms_for_name): Filter .cold functions when a real function with the same name is found. gdb/testsuite: 2020-11-30 Bernd Edlinger <bernd.edlinger@hotmail.de> * gdb.cp/step-and-next-inline-1.exp: New test. --- gdb/linespec.c | 24 ++++++++++++++++++++ gdb/testsuite/gdb.cp/step-and-next-inline-1.exp | 30 +++++++++++++++++++++++++ 2 files changed, 54 insertions(+) create mode 100644 gdb/testsuite/gdb.cp/step-and-next-inline-1.exp diff --git a/gdb/linespec.c b/gdb/linespec.c index 7456095..36ccb34 100644 --- a/gdb/linespec.c +++ b/gdb/linespec.c @@ -4382,6 +4382,30 @@ class symtab_collector break; } } + else if (MSYMBOL_TYPE (item.minsym) == mst_file_text + && strlen (item.minsym->linkage_name ()) > 5) + { + size_t namelen = strlen (item.minsym->linkage_name ()) - 5; + if (memcmp (item.minsym->linkage_name () + namelen, ".cold", 5) == 0) + for (const bound_minimal_symbol &item2 : minsyms) + { + if (&item2 == &item) + continue; + + if (MSYMBOL_TYPE (item2.minsym) != mst_file_text + && MSYMBOL_TYPE (item2.minsym) != mst_text) + continue; + + if (strlen (item2.minsym->linkage_name ()) == namelen + && memcmp (item.minsym->linkage_name (), + item2.minsym->linkage_name (), namelen) != 0) + continue; + + /* found a real function with the same name as cold part. */ + skip = true; + break; + } + } if (!skip) info->result.minimal_symbols->push_back (item); diff --git a/gdb/testsuite/gdb.cp/step-and-next-inline-1.exp b/gdb/testsuite/gdb.cp/step-and-next-inline-1.exp new file mode 100644 index 0000000..f54c0c1 --- /dev/null +++ b/gdb/testsuite/gdb.cp/step-and-next-inline-1.exp @@ -0,0 +1,30 @@ +# 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/>. + +standard_testfile step-and-next-inline.cc + +if [get_compiler_info "c++"] { + unsupported "couldn't find a valid c++ compiler" + return -1 +} + +set options {c++ debug nowarnings optimize=-O2} +if { [prepare_for_testing "failed to prepare" $testfile $srcfile $options] } { + return -1 +} + +# Test that b get_alias_set sets only one breakpoint, +# thus get_alias_set.cold is filtered away. +gdb_test "b get_alias_set" ".*Breakpoint .*$srcfile, line .*" "test1" -- 1.9.1 ^ permalink raw reply [flat|nested] 7+ messages in thread
* [PING] [PATCH] Skip .cold functions in search_minsyms_for_name 2020-11-30 19:26 ` [PATCH] Skip .cold functions in search_minsyms_for_name Bernd Edlinger @ 2020-12-18 15:57 ` Bernd Edlinger 0 siblings, 0 replies; 7+ messages in thread From: Bernd Edlinger @ 2020-12-18 15:57 UTC (permalink / raw) To: Andrew Burgess; +Cc: gdb-patches, Tom Tromey Hi, I'd like to ping for this patch: https://sourceware.org/pipermail/gdb-patches/2020-November/173643.html Thanks Bernd. On 11/30/20 8:26 PM, Bernd Edlinger wrote: > Hi, > > this fixes another regression that is cold functions were > previously skipped, but that filtering was removed with > 77f2120b200. > > However the cold functions are not the external > interface, but they just contain the unlikely code paths. > When a normal function entry is found, they are generally > preferred over the .cold function. > > When a function with the same name is also available, > that is preferred. > > The .cold function is not the external interface. > > Fixes 77f2120b200 ("Don't drop static function bp locations w/o debug info") > > gdb: > 2020-11-30 Bernd Edlinger <bernd.edlinger@hotmail.de> > > * linespec.c (search_minsyms_for_name): Filter .cold functions when > a real function with the same name is found. > > gdb/testsuite: > 2020-11-30 Bernd Edlinger <bernd.edlinger@hotmail.de> > > * gdb.cp/step-and-next-inline-1.exp: New test. > > > Tested on x86_64-pc-linux-gnu. > Is it OK for trunk? > > Thanks > Bernd. > ^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCHv2] Fix partial symbols 2020-11-30 15:53 ` Bernd Edlinger 2020-11-30 19:26 ` [PATCH] Skip .cold functions in search_minsyms_for_name Bernd Edlinger @ 2020-12-18 16:23 ` Bernd Edlinger 2021-01-03 15:36 ` [PATCH v3] " Bernd Edlinger 1 sibling, 1 reply; 7+ messages in thread From: Bernd Edlinger @ 2020-12-18 16:23 UTC (permalink / raw) To: Andrew Burgess; +Cc: gdb-patches, Tom Tromey [-- Attachment #1: Type: text/plain, Size: 8347 bytes --] On 11/30/20 4:53 PM, Bernd Edlinger wrote: > On 11/29/20 8:43 PM, Andrew Burgess wrote: >> * Bernd Edlinger <bernd.edlinger@hotmail.de> [2020-11-28 19:01:10 +0100]: >> >>> Hi, >>> >>> this fixes a regression in partial symbols. >> >> >> Is there a particular test that regressed and is fixed with this >> patch? It is probably worth mentioning in the commit message. >> > > No, I just found it by chance, since my other patch touches > partial symbols, so I realized that they do not function correctly: > > This is what I saw while debugging gcc: > > $ gcc -S test.c -wrapper gdb,--args > GNU gdb (GDB) 11.0.50.20201130-git > Copyright (C) 2020 Free Software Foundation, Inc. > License GPLv3+: GNU GPL version 3 or later <http://gnu.org/licenses/gpl.html> > This is free software: you are free to change and redistribute it. > There is NO WARRANTY, to the extent permitted by law. > Type "show copying" and "show warranty" for details. > This GDB was configured as "x86_64-pc-linux-gnu". > Type "show configuration" for configuration details. > For bug reporting instructions, please see: > <https://www.gnu.org/software/gdb/bugs/>. > Find the GDB manual and other documentation resources online at: > <http://www.gnu.org/software/gdb/documentation/>. > > For help, type "help". > Type "apropos word" to search for commands related to "word"... > Reading symbols from /home/ed/gnu/install/libexec/gcc/x86_64-pc-linux-gnu/11.0.0/cc1... > (gdb) b tree_class_check > Breakpoint 1 at 0x67c416: tree_class_check. (3394 locations) > (gdb) cle tree_class_check > Deleted breakpoint 1 > (gdb) b get_alias_set > Breakpoint 2 at 0x67b4d1: get_alias_set. (2 locations) > (gdb) cle get_alias_set > Deleted breakpoint 2 > (gdb) b tree_class_check > Breakpoint 3 at 0x67c416: tree_class_check. (3419 locations) > > as you see, the number of locations of tree_class_check changes after > symbols for get_alias_set are loaded. > > In 10.1 the numbers were stable: > > $ gcc -S test.c -wrapper gdb,--args > GNU gdb (GDB) 10.1 > Copyright (C) 2020 Free Software Foundation, Inc. > License GPLv3+: GNU GPL version 3 or later <http://gnu.org/licenses/gpl.html> > This is free software: you are free to change and redistribute it. > There is NO WARRANTY, to the extent permitted by law. > Type "show copying" and "show warranty" for details. > This GDB was configured as "x86_64-pc-linux-gnu". > Type "show configuration" for configuration details. > For bug reporting instructions, please see: > <https://www.gnu.org/software/gdb/bugs/>. > Find the GDB manual and other documentation resources online at: > <http://www.gnu.org/software/gdb/documentation/>. > > For help, type "help". > Type "apropos word" to search for commands related to "word"... > Reading symbols from /home/ed/gnu/install/libexec/gcc/x86_64-pc-linux-gnu/11.0.0/cc1... > (gdb) b tree_class_check > Breakpoint 1 at 0x67c416: tree_class_check. (7332 locations) > (gdb) cle tree_class_check > Deleted breakpoint 1 > (gdb) b get_alias_set > Breakpoint 2 at 0x955150: file ../../gcc-trunk/gcc/alias.c, line 868. > (gdb) cle get_alias_set > Deleted breakpoint 2 > (gdb) b tree_class_check > Breakpoint 3 at 0x67c416: tree_class_check. (7332 locations) > > > after this patch the numbers are almost the same as with 10.1: > > $ gcc -S test.c -wrapper gdb,--args > GNU gdb (GDB) 11.0.50.20201130-git > Copyright (C) 2020 Free Software Foundation, Inc. > License GPLv3+: GNU GPL version 3 or later <http://gnu.org/licenses/gpl.html> > This is free software: you are free to change and redistribute it. > There is NO WARRANTY, to the extent permitted by law. > Type "show copying" and "show warranty" for details. > This GDB was configured as "x86_64-pc-linux-gnu". > Type "show configuration" for configuration details. > For bug reporting instructions, please see: > <https://www.gnu.org/software/gdb/bugs/>. > Find the GDB manual and other documentation resources online at: > <http://www.gnu.org/software/gdb/documentation/>. > > For help, type "help". > Type "apropos word" to search for commands related to "word"... > Reading symbols from /home/ed/gnu/install/libexec/gcc/x86_64-pc-linux-gnu/11.0.0/cc1... > (gdb) b tree_class_check > Breakpoint 1 at 0x67c416: tree_class_check. (7332 locations) > (gdb) cle tree_class_check > Deleted breakpoint 1 > (gdb) b get_alias_set > Breakpoint 2 at 0x67b4d1: get_alias_set. (2 locations) > (gdb) cle get_alias_set > Deleted breakpoint 2 > (gdb) b tree_class_check > Breakpoint 3 at 0x67c416: tree_class_check. (7332 locations) > (gdb) cle tree_class_check > Deleted breakpoint 3 > > Note however, that there is still a regression with the symbols > of get_alias_set. There are two breakpoints, one of them is bogus. > > Bisection shows this patch caused that one: > > commit 77f2120b200be6cabbf6f610942fc1173a8df6d3 > Author: Pedro Alves <pedro@palves.net> > Date: Sun Sep 13 13:34:10 2020 +0100 > > I'll post a patch for that later. > > >> If this is a regression in some functionality that is not tested in >> tree then it would be great to see a test added. >> > > I have the impression that it does never happen in small programs. > > So far I have no idea for a patch. > > The gdb.dwarf2/dw2-empty-pc-range.exp is probably testing something > similar, a different parsing in partial symbols and normal symbols. > > Here we have also a parse-error that happens with the partial symbols > but not in the normal symbol parser. > >> >>> >>> The bug was introduced with this patch: >>> >>> commit 529908cbd0afc2524044b7df3626c09d0bdae12d >>> Author: Tom Tromey <tom@tromey.com> >>> Date: Tue Sep 29 18:49:08 2020 -0600 >>> >>> Remove DW_UNSND >>> >>> This removes DW_UNSND, replacing uses with either as_unsigned or >>> constant_value, depending primarily on whether or not the form is >>> already known to be appropriate. >>> >>> gdb/ChangeLog >>> 2020-09-29 Tom Tromey <tom@tromey.com> >>> >>> * dwarf2/read.c (lookup_dwo_id, get_type_unit_group) >>> (read_file_scope, dwarf2_get_pc_bounds) >>> (dwarf2_record_block_ranges, dwarf2_add_field, get_alignment) >>> (read_structure_type, handle_struct_member_die) >>> (read_enumeration_type, read_array_type, read_set_type) >>> (read_tag_pointer_type, read_tag_reference_type) >>> (read_subroutine_type, read_base_type, read_subrange_type) >>> (read_full_die_1, partial_die_info::read) >>> (partial_die_info::read, by, new_symbol) >>> (dwarf2_const_value_data, dwarf2_const_value_attr) >>> (dump_die_shallow, dwarf2_fetch_constant_bytes) >>> (prepare_one_comp_unit): Update. >>> * dwarf2/attribute.h (DW_UNSND): Remove. >>> >>> Due to the broken handling of DW_AT_ranges >>> in partial_die_info::read only a subset of the >>> complete locations is found when I set a breakpoint >>> on an inline function. >> >> This commit message is very cryptic, it assumes the reader will have >> knowledge of what the "broken handling of DW_AT_ranges in >> partial_die_info::read" is, and why this change works around this >> issue. >> >> A better commit message would explain what the problem is, why the >> current code causes issues when combined with this problem, and why >> the change you propose works around this issue. >> > > Ok, I'll add this to the commit message: > > [PATCH] Fix partial symbols > > The DW_AT_ranges attribute is of type DW_FORM_sec_offset > or DW_FORM_rnglistx. > > But the function attribute attribute::constant_value () does only > handle DW_FORM_sdata, DW_FORM_implicit_const, DW_FORM_udata > DW_FORM_data1, DW_FORM_data2, DW_FORM_data4, DW_FORM_data8 > and returns the default value for anything else. > > Therefore the wrong range is parsed. > > Fixes: 529908cbd0a ("Remove DW_UNSND") > >> Maybe someone else will have more context and be able to review this >> change, but the clearer your commit messages the easier it is for >> people to review your work. >> >> Thanks, >> Andrew >> So, while I immediately found a test case for the issue with the cold function I have not found a test case for the partial symbols issue. It probably only happens on big numbers. I improved the commit message as requested, though. So can this patch go forward without a test case? Thanks Bernd. [-- Warning: decoded text below may be mangled, UTF-8 assumed --] [-- Attachment #2: 0001-Fix-partial-symbols.patch --] [-- Type: text/x-patch; name="0001-Fix-partial-symbols.patch", Size: 1302 bytes --] From d179030c1b8f76152e72fe3ac31789ec706b6d7f Mon Sep 17 00:00:00 2001 From: Bernd Edlinger <bernd.edlinger@hotmail.de> Date: Sat, 28 Nov 2020 17:29:18 +0100 Subject: [PATCH] Fix partial symbols The DW_AT_ranges attribute is of type DW_FORM_sec_offset or DW_FORM_rnglistx. But the function attribute attribute::constant_value () does only handle DW_FORM_sdata, DW_FORM_implicit_const, DW_FORM_udata DW_FORM_data1, DW_FORM_data2, DW_FORM_data4, DW_FORM_data8 and returns the default value for anything else. Therefore the wrong range is parsed. Fixes: 529908cbd0a ("Remove DW_UNSND") 2020-11-28 Bernd Edlinger <bernd.edlinger@hotmail.de> * dwarf2/read.c (partial_die_info::read): Fix DW_AT_ranges. --- gdb/dwarf2/read.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/gdb/dwarf2/read.c b/gdb/dwarf2/read.c index 9468b91..0d46951 100644 --- a/gdb/dwarf2/read.c +++ b/gdb/dwarf2/read.c @@ -19753,7 +19753,7 @@ struct type * /* It would be nice to reuse dwarf2_get_pc_bounds here, but that requires a full DIE, so instead we just reimplement it. */ - unsigned int ranges_offset = (attr.constant_value (0) + unsigned int ranges_offset = (attr.as_unsigned () + (need_ranges_base ? cu->ranges_base : 0)); -- 1.9.1 ^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH v3] Fix partial symbols 2020-12-18 16:23 ` [PATCHv2] Fix partial symbols Bernd Edlinger @ 2021-01-03 15:36 ` Bernd Edlinger 0 siblings, 0 replies; 7+ messages in thread From: Bernd Edlinger @ 2021-01-03 15:36 UTC (permalink / raw) To: gdb-patches; +Cc: Andrew Burgess, Tom Tromey, Simon Marchi [-- Attachment #1: Type: text/plain, Size: 299 bytes --] Hi, I finally found a test case which can be used to demonstrate the problem with the partial symbols: Before the patch there are 3 breakpoints found for "b tree_check" and after the patch there are 4 breakpoints. I have now added the test case to the otherwise unmodified patch. Thanks Bernd. [-- Warning: decoded text below may be mangled, UTF-8 assumed --] [-- Attachment #2: 0001-Fix-partial-symbols.patch --] [-- Type: text/x-patch; name="0001-Fix-partial-symbols.patch", Size: 4301 bytes --] From f46c110e4fc598d44722e82e85ca8d9d01f4680a Mon Sep 17 00:00:00 2001 From: Bernd Edlinger <bernd.edlinger@hotmail.de> Date: Sat, 28 Nov 2020 17:29:18 +0100 Subject: [PATCH] Fix partial symbols The DW_AT_ranges attribute is of type DW_FORM_sec_offset or DW_FORM_rnglistx. But the function attribute attribute::constant_value () does only handle DW_FORM_sdata, DW_FORM_implicit_const, DW_FORM_udata DW_FORM_data1, DW_FORM_data2, DW_FORM_data4, DW_FORM_data8 and returns the default value for anything else. Therefore the wrong range is parsed. Fixes: 529908cbd0a ("Remove DW_UNSND") gdb: 2021-01-03 Bernd Edlinger <bernd.edlinger@hotmail.de> * dwarf2/read.c (partial_die_info::read): Fix DW_AT_ranges. gdb/testsuite: 2021-01-03 Bernd Edlinger <bernd.edlinger@hotmail.de> * gdb.cp/step-and-next-psymtab.exp: New test. * gdb.cp/step-and-next-psymtab.cc: New test. --- gdb/dwarf2/read.c | 2 +- gdb/testsuite/gdb.cp/step-and-next-psymtab.cc | 27 ++++++++++++++++++++++ gdb/testsuite/gdb.cp/step-and-next-psymtab.exp | 31 ++++++++++++++++++++++++++ 3 files changed, 59 insertions(+), 1 deletion(-) create mode 100644 gdb/testsuite/gdb.cp/step-and-next-psymtab.cc create mode 100644 gdb/testsuite/gdb.cp/step-and-next-psymtab.exp diff --git a/gdb/dwarf2/read.c b/gdb/dwarf2/read.c index 9032186..d87d22c8 100644 --- a/gdb/dwarf2/read.c +++ b/gdb/dwarf2/read.c @@ -19805,7 +19805,7 @@ struct type * /* It would be nice to reuse dwarf2_get_pc_bounds here, but that requires a full DIE, so instead we just reimplement it. */ - unsigned int ranges_offset = (attr.constant_value (0) + unsigned int ranges_offset = (attr.as_unsigned () + (need_ranges_base ? cu->ranges_base : 0)); diff --git a/gdb/testsuite/gdb.cp/step-and-next-psymtab.cc b/gdb/testsuite/gdb.cp/step-and-next-psymtab.cc new file mode 100644 index 0000000..1bb724b --- /dev/null +++ b/gdb/testsuite/gdb.cp/step-and-next-psymtab.cc @@ -0,0 +1,27 @@ +/* This testcase is part of GDB, the GNU debugger. + + Copyright 2021 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 "step-and-next-inline.h" + +int +get_alias_set1 (tree *t) +{ + if (t != NULL + && TREE_TYPE (t).z != 1) + return 0; + return 1; +} diff --git a/gdb/testsuite/gdb.cp/step-and-next-psymtab.exp b/gdb/testsuite/gdb.cp/step-and-next-psymtab.exp new file mode 100644 index 0000000..2afc4d9 --- /dev/null +++ b/gdb/testsuite/gdb.cp/step-and-next-psymtab.exp @@ -0,0 +1,31 @@ +# Copyright 2021 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 step-and-next-inline.cc .cc + +if [get_compiler_info "c++"] { + unsupported "couldn't find a valid c++ compiler" + return -1 +} + +set options {c++ debug nowarnings optimize=-O2} + +set sources [list $srcfile $srcfile2] + +if { [prepare_for_testing "failed to prepare" $testfile $sources $options] } { + return -1 +} + +gdb_test "b tree_check" ".*Breakpoint .* \\(4 locations\\).*" "test1" -- 1.9.1 ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2021-01-03 15:36 UTC | newest] Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-11-28 18:01 [PATCH] Fix partial symbols Bernd Edlinger 2020-11-29 19:43 ` Andrew Burgess 2020-11-30 15:53 ` Bernd Edlinger 2020-11-30 19:26 ` [PATCH] Skip .cold functions in search_minsyms_for_name Bernd Edlinger 2020-12-18 15:57 ` [PING] " Bernd Edlinger 2020-12-18 16:23 ` [PATCHv2] Fix partial symbols Bernd Edlinger 2021-01-03 15:36 ` [PATCH v3] " 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).