From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 20421 invoked by alias); 13 Dec 2007 19:23:23 -0000 Received: (qmail 20405 invoked by uid 22791); 13 Dec 2007 19:23:21 -0000 X-Spam-Check-By: sourceware.org Received: from mail.codesourcery.com (HELO mail.codesourcery.com) (65.74.133.4) by sourceware.org (qpsmtpd/0.31) with ESMTP; Thu, 13 Dec 2007 19:23:14 +0000 Received: (qmail 11132 invoked from network); 13 Dec 2007 19:23:12 -0000 Received: from unknown (HELO localhost) (jimb@127.0.0.2) by mail.codesourcery.com with ESMTPA; 13 Dec 2007 19:23:12 -0000 To: Vladimir Prus Cc: gdb-patches@sources.redhat.com Subject: Re: Support constants for DW_AT_data_member_location References: <200711291254.14423.vladimir@codesourcery.com> <200711292317.53967.vladimir@codesourcery.com> <200712132005.17970.vladimir@codesourcery.com> From: Jim Blandy Date: Thu, 13 Dec 2007 19:29:00 -0000 In-Reply-To: <200712132005.17970.vladimir@codesourcery.com> (Vladimir Prus's message of "Thu, 13 Dec 2007 21:05:17 +0400") Message-ID: User-Agent: Gnus/5.11 (Gnus v5.11) Emacs/22.0.50 (gnu/linux) MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable X-IsSubscribed: yes Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org X-SW-Source: 2007-12/txt/msg00177.txt.bz2 Sorry --- I committed, and then saw your review. I'll revise this. Vladimir Prus 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 interep= reted >> > as constant. Maybe we should have "is_surely_constant" function? >>=20 >> Oh --- great catch, thanks. =C2=A0(We don't support location lists for >> DW_AT_data_member_location yet, but we certainly should warn, not >> randomly misinterpret the value.) >>=20 >> How's this, then? >>=20 >> gdb/ChangeLog: >> 2007-11-30 =C2=A0Jim Blandy =C2=A0 >>=20 >> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0* dwarf2read.c (attr_for= m_is_constant): New function. >> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0(dwarf2_add_field): Use = it and attr_form_is_section_offset to >> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0recognize DW_AT_data_mem= ber_location attributes. =C2=A0Use >> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0dwarf2_get_attr_constant= _value when the attribute is a constant. >>=20 >> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0* dwarf2read.c (attr_for= m_is_section_offset): New function. >> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0(dwarf_add_member_fn, re= ad_common_block, read_partial_die) >> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0(dwarf2_symbol_mark_comp= uted): Use it, instead of writing it out. >>=20 >> diff -r c4f654de59cf gdb/dwarf2read.c >> --- a/gdb/dwarf2read.c=C2=A0=C2=A0Thu Nov 29 11:28:59 2007 -0800 >> +++ b/gdb/dwarf2read.c=C2=A0=C2=A0Fri Nov 30 17:32:46 2007 -0800 >> @@ -1039,6 +1039,10 @@ static void dwarf_decode_macros (struct=20 >> =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 char *, bfd *, struct dwar= f2_cu *); >> =C2=A0 >> =C2=A0static int attr_form_is_block (struct attribute *); >> + >> +static int attr_form_is_section_offset (struct attribute *); >> + >> +static int attr_form_is_constant (struct attribute *); >> =C2=A0 >> =C2=A0static void dwarf2_symbol_mark_computed (struct attribute *attr, >> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2= =A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0=C2=A0=C2=A0 struct symbol *sym, >> @@ -3380,8 +3384,16 @@ dwarf2_add_field (struct field_info *fip >> =C2=A0 =C2=A0 =C2=A0 =C2=A0attr =3D dwarf2_attr (die, DW_AT_data_member_= location, cu); >> =C2=A0 =C2=A0 =C2=A0 =C2=A0if (attr) >> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0{ >> -=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 =C2=A0FIELD_BITPOS (*fp) =3D >> -=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 =C2=A0 =C2=A0decode_locdesc = (DW_BLOCK (attr), cu) * bits_per_byte; >> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0if (attr_form_is_section_offset (att= r)) >> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0{ >> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0dwarf2_complex_locatio= n_expr_complaint (); >> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0FIELD_BITPOS (*fp) =3D= 0; >> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0} >> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0else if (attr_form_is_constant (attr= )) >> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0FIELD_BITPOS (*fp) =3D dwarf2= _get_attr_constant_value (attr, 0); > > You need "* 8" above, since the value of data_member_location is offset > in bytes. Fixed --- thanks. >> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0else >> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0FIELD_BITPOS (*fp) =3D >> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0decode_locdesc (DW_BLO= CK (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 >> + =C2=A0 lineptr, loclistptr, macptr or rangelistptr).=20 > > 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,=20 >> + =C2=A0 you may use DW_UNSND (attr) to retrieve the offset. =C2=A0*/ >> +static int >> +attr_form_is_section_offset (struct attribute *attr) >> +{ >> + =C2=A0return (attr->form =3D=3D DW_FORM_data4 >> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0|| attr->form =3D=3D 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.