Re: Support constants for DW_AT_data_member_location

[Jim Blandy <jimb at codesourcery dot com>]

Sorry --- I committed, and then saw your review.  I'll revise this.

Vladimir Prus <vladimir at codesourcery.com> writes:
> On Saturday 01 December 2007 04:48:09 Jim Blandy wrote:
>
>> > So, DW_FORM_data4 used for DW_AT_data_member_location is never interepreted
>> > as constant. Maybe we should have "is_surely_constant" function?
>> 
>> Oh --- great catch, thanks. Â(We don't support location lists for
>> DW_AT_data_member_location yet, but we certainly should warn, not
>> randomly misinterpret the value.)
>> 
>> How's this, then?
>> 
>> gdb/ChangeLog:
>> 2007-11-30 ÂJim Blandy Â<jimb@codesourcery.com>
>> 
>> ÂÂÂÂÂÂÂÂ* dwarf2read.c (attr_form_is_constant): New function.
>> ÂÂÂÂÂÂÂÂ(dwarf2_add_field): Use it and attr_form_is_section_offset to
>> ÂÂÂÂÂÂÂÂrecognize DW_AT_data_member_location attributes. ÂUse
>> ÂÂÂÂÂÂÂÂdwarf2_get_attr_constant_value when the attribute is a constant.
>> 
>> ÂÂÂÂÂÂÂÂ* dwarf2read.c (attr_form_is_section_offset): New function.
>> ÂÂÂÂÂÂÂÂ(dwarf_add_member_fn, read_common_block, read_partial_die)
>> ÂÂÂÂÂÂÂÂ(dwarf2_symbol_mark_computed): Use it, instead of writing it out.
>> 
>> diff -r c4f654de59cf gdb/dwarf2read.c
>> --- a/gdb/dwarf2read.cÂÂThu Nov 29 11:28:59 2007 -0800
>> +++ b/gdb/dwarf2read.cÂÂFri Nov 30 17:32:46 2007 -0800
>> @@ -1039,6 +1039,10 @@ static void dwarf_decode_macros (struct 
>> Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â char *, bfd *, struct dwarf2_cu *);
>> Â
>> Âstatic int attr_form_is_block (struct attribute *);
>> +
>> +static int attr_form_is_section_offset (struct attribute *);
>> +
>> +static int attr_form_is_constant (struct attribute *);
>> Â
>> Âstatic void dwarf2_symbol_mark_computed (struct attribute *attr,
>> ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ struct symbol *sym,
>> @@ -3380,8 +3384,16 @@ dwarf2_add_field (struct field_info *fip
>> Â Â Â Âattr = dwarf2_attr (die, DW_AT_data_member_location, cu);
>> Â Â Â Âif (attr)
>> ÂÂÂÂÂÂÂÂ{
>> -ÂÂÂÂÂÂÂ ÂFIELD_BITPOS (*fp) =
>> -ÂÂÂÂÂÂÂ Â Âdecode_locdesc (DW_BLOCK (attr), cu) * bits_per_byte;
>> + Â Â Â Â Âif (attr_form_is_section_offset (attr))
>> + Â Â Â Â Â Â{
>> + Â Â Â Â Â Â Âdwarf2_complex_location_expr_complaint ();
>> + Â Â Â Â Â Â ÂFIELD_BITPOS (*fp) = 0;
>> + Â Â Â Â Â Â}
>> + Â Â Â Â Âelse if (attr_form_is_constant (attr))
>> + Â Â Â Â Â ÂFIELD_BITPOS (*fp) = dwarf2_get_attr_constant_value (attr, 0);
>
> You need "* 8" above, since the value of data_member_location is offset
> in bytes.

Fixed --- thanks.

>> + Â Â Â Â Âelse
>> + Â Â Â Â Â ÂFIELD_BITPOS (*fp) =
>> + Â Â Â Â Â Â Âdecode_locdesc (DW_BLOCK (attr), cu) * bits_per_byte;
>
> Or, if we really want GDB to support processors with 9-bit bytes, you
> need "* bits_per_byte".

That's what I used.  I'm not sure what read_memory would return on
such processors, but... :)

>> +/* Return non-zero if ATTR's value is a section offset (classes
>> + Â lineptr, loclistptr, macptr or rangelistptr). 
>
> I'd also note here that in all cases, an attribute value
> may fall to only one class listed above.

I've added a note citing 7.5.4, which says this (after the list of
classes).

>> In this case, 
>> + Â you may use DW_UNSND (attr) to retrieve the offset. Â*/
>> +static int
>> +attr_form_is_section_offset (struct attribute *attr)
>> +{
>> + Âreturn (attr->form == DW_FORM_data4
>> + Â Â Â Â Â|| attr->form == DW_FORM_data8);
>
> I'd probably add a comment here saying that in DWARF standand,
> those are only two forms that can encode those classes. Not being
> DWARF expert, it took me a bit of looking to verify that.  If you
> explicitly mention this, then future readers can just trust the
> comment ;-)

Hmm.  If I look through the class definitions in 7.5.4, each paragraph
ends with "It is either form DW_FORM_data4 or form DW_FORM_data8."  I
feel odd putting in a comment saying "Those classes use only these
forms", because if that weren't true, then the comment saying what the
function does wouldn't be accurate; it'd be missing cases.

> With the "* bits_per_byte" change, this patch fixes the problem I saw.
> Assuming there are no regressions, can you check it in?

Thanks for checking this.  I did run the tests before I committed, but
I didn't notice any failures.  I must be using a compiler that never
generates anything but expressions.