Re: [PATCH] gdb: work around negative DW_AT_data_member_location GCC 11 bug

[Keith Seitz <keiths@redhat.com>]

On 1/26/22 09:45, Simon Marchi via Gdb-patches wrote:
> Ping.
> 
> On 2021-11-29 10:37, Simon Marchi wrote:
>> From: Simon Marchi <simon.marchi@polymtl.ca>
>>
>> Since there are some binaries with this in the wild, I think it would be
>> useful for GDB to work around this.

Re: "in the wild": I checked on several Fedora releases whether this was
a problem, and sure enough, there binaries in Fedora 35 and Rawhide that
do exhibit this bug.

So, thank you for catching this and submitting a fix!

>> I did the obvious simple thing, if the DW_AT_data_member_location's
>> value is -1, replace it with 0.  I didn't add a producer check,
>> because I don't think that a DW_AT_data_member_location value of -1
>> is ever legitimate, but we could certainly add one if needed.

That seems like a reasonable workaround to me.

Aside: One thing I've always wanted was some sort of quasi-automatic
deprecation of these types of bug workarounds... Something even
as simple as a standardized marker in the source that some
nightly/weekly/monthly/quarterly script could search for and
send out a notification to remove (or do it itself). That's a
conversation for another year, though.

>> The difficult part would be if GCC 11 ever emits a legitimate
>> DW_AT_data_member_location value of -1 in other situations, then we
>> would need to identify when the -1 is legitimate and when it is
>> not.
We'll cross that bridge when/if we get to it? ;-)

I just have two tiny nits...

>> diff --git a/gdb/dwarf2/read.c b/gdb/dwarf2/read.c
>> index 737d8a4c81b..0c66a6daf97 100644
>> --- a/gdb/dwarf2/read.c
>> +++ b/gdb/dwarf2/read.c
>> @@ -14489,6 +14489,16 @@ handle_member_location (struct die_info *die, struct dwarf2_cu *cu,
>>         if (attr->form_is_constant ())
>>   	{
>>   	  LONGEST offset = attr->constant_value (0);
>> +
>> +	  /* Work around this GCC 11 bug, where it would erroneously use -1
>> +	     data member locations, instead of 0:
>> +
>> +	       Negative DW_AT_data_member_location
>> +	       https://gcc.gnu.org/bugzilla/show_bug.cgi?id=101378
>> +	     */
>> +	  if (offset == -1)
>> +	    offset = 0;
>> +

Kevin and I discussed this briefly, and he convinced me that a complaint here might
be useful. What do you think?

member-location.exp
>> new file mode 100644
>> index 00000000000..664c4e47acc
>> --- /dev/null
>> +++ b/gdb/testsuite/gdb.dwarf2/negative-data-member-location.exp
>> @@ -0,0 +1,76 @@
>> +# 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/>.
>> +
>> +# Test our workaround for a GCC 11 bug, where it sometimes puts a -1 value for
>> +# DW_AT_data_member_location:
>> +#
>> +#   Negative DW_AT_data_member_location
>> +#   https://gcc.gnu.org/bugzilla/show_bug.cgi?id=101378
>> +
>> +load_lib dwarf.exp
>> +
>> +# This test can only be run on targets which support DWARF-2 and use gas.
>> +if ![dwarf2_support] {
>> +    return 0
>> +}
>> +

Please pardon my Tclish fetish, but since this is a new file (and likely to
be used as a model for future contributions), may I ask that we be more
pedantic about the formatting? I.e., all conditional expressions enclosed in
curly brackets, no use of "then" in "if" statements? Those are so
last-century. [:-)]

I think this is go to go regardless. I recommend you approve your patch.

Keith

>> +standard_testfile .c -dw.S
>> +
>> +set asm_file [standard_output_file ${srcfile2}]
>> +
>> +Dwarf::assemble ${asm_file} {
>> +    cu {} {
>> +	DW_TAG_compile_unit {
>> +	    {DW_AT_language @DW_LANG_C99}
>> +	    {name ${::srcfile}}
>> +        } {
>> +	    declare_labels int_label struct_label
>> +
>> +	    int_label: DW_TAG_base_type {
>> +		{DW_AT_byte_size 4 DW_FORM_udata}
>> +		{DW_AT_encoding @DW_ATE_signed}
>> +		{DW_AT_name "int"}
>> +	    }
>> +
>> +	    struct_label: DW_TAG_structure_type {
>> +		{DW_AT_name "the_struct"}
>> +		{DW_AT_byte_size 4 DW_FORM_udata}
>> +	    } {
>> +		DW_TAG_member {
>> +		    {DW_AT_name "field"}
>> +		    {DW_AT_type :$int_label}
>> +		    {DW_AT_data_member_location -1 DW_FORM_sdata}
>> +		}
>> +	    }
>> +
>> +	    DW_TAG_variable {
>> +		{DW_AT_name "s"}
>> +		{DW_AT_type :$struct_label}
>> +		{DW_AT_location {DW_OP_addr [gdb_target_symbol "s"]} SPECIAL_expr}
>> +	    }
>> +	}
>> +    }
>> +}
>> +
>> +if [prepare_for_testing "failed to prepare" ${testfile} [list $srcfile $asm_file] {nodebug}] {
>> +    return
>> +}
>> +
>> +if ![runto_main] then {
>> +    return
>> +}
>> +
>> +gdb_test "print /x s" " = {field = 0x11222211}"
>> +gdb_test "print /x s.field" " = 0x11222211"
>