Re: Support constants for DW_AT_data_member_location

[Jim Blandy <jimb@codesourcery.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.