[PATCH] Fix partial symbols

[PATCH] Fix partial symbols

[Bernd Edlinger]

[-- 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

Re: [PATCH] Fix partial symbols

[Andrew Burgess]

* 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
> 

Re: [PATCH] Fix partial symbols

[Bernd Edlinger]

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
>>
> 

[PATCH] Skip .cold functions in search_minsyms_for_name

[Bernd Edlinger]

[-- 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

[PING] [PATCH] Skip .cold functions in search_minsyms_for_name

[Bernd Edlinger]

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.
> 

[PATCHv2] Fix partial symbols

[Bernd Edlinger]

[-- 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

[PATCH v3] Fix partial symbols

[Bernd Edlinger]

[-- 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