[PATCHv2] Fix partial symbols

[Bernd Edlinger <bernd.edlinger@hotmail.de>]

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