From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 15844 invoked by alias); 6 Oct 2008 20:00:42 -0000 Received: (qmail 15834 invoked by uid 22791); 6 Oct 2008 20:00:40 -0000 X-Spam-Check-By: sourceware.org Received: from rock.gnat.com (HELO rock.gnat.com) (205.232.38.15) by sourceware.org (qpsmtpd/0.31) with ESMTP; Mon, 06 Oct 2008 20:00:05 +0000 Received: from localhost (localhost.localdomain [127.0.0.1]) by filtered-rock.gnat.com (Postfix) with ESMTP id 1FF8E2A967E; Mon, 6 Oct 2008 16:00:03 -0400 (EDT) Received: from rock.gnat.com ([127.0.0.1]) by localhost (rock.gnat.com [127.0.0.1]) (amavisd-new, port 10024) with LMTP id I-RzTFhfpL4j; Mon, 6 Oct 2008 16:00:03 -0400 (EDT) Received: from joel.gnat.com (localhost.localdomain [127.0.0.1]) by rock.gnat.com (Postfix) with ESMTP id A19ED2A963E; Mon, 6 Oct 2008 16:00:02 -0400 (EDT) Received: by joel.gnat.com (Postfix, from userid 1000) id E3F7EE7ACD; Mon, 6 Oct 2008 13:00:01 -0700 (PDT) Date: Mon, 06 Oct 2008 20:00:00 -0000 From: Joel Brobecker To: Jan Kratochvil Cc: gdb-patches@sourceware.org, Tobias Burnus , Ulrich Weigand , Jim Blandy Subject: Re: [patch] static_kind -> bit0, bit1 [Re: [gdb] Fortran dynamic arrays] Message-ID: <20081006200001.GC3588@adacore.com> References: <20080818111120.GE16894@adacore.com> <200808181553.m7IFrG3w005270@d12av02.megacenter.de.ibm.com> <48A59B3C.9050801@net-b.de> <20080818111120.GE16894@adacore.com> <20080907115637.GA12939@host0.dyn.jankratochvil.net> <20080919221221.GA23372@adacore.com> <20081004202457.GA20726@host0.dyn.jankratochvil.net> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20081004202457.GA20726@host0.dyn.jankratochvil.net> User-Agent: Mutt/1.4.2.2i 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: 2008-10/txt/msg00154.txt.bz2 > 2008-10-04 Jan Kratochvil > > Convert static_kind into loc_kind enum. [...] Thanks for the patch. I have a few tiny comments left, but the principle looks good to me. > --- gdb/gdbtypes.h 2 Oct 2008 22:06:07 -0000 1.92 > +++ gdb/gdbtypes.h 4 Oct 2008 19:44:27 -0000 > @@ -316,6 +316,16 @@ enum type_instance_flag_value > #define TYPE_ADDRESS_CLASS_ALL(t) (TYPE_INSTANCE_FLAGS(t) \ > & TYPE_INSTANCE_FLAG_ADDRESS_CLASS_ALL) > > +/* Determine which field of the union main_type.fields[x].loc is used. */ > + > +enum field_loc_kind > + { > + FIELD_LOC_KIND_BITPOS, /* bitpos */ Is it worth forcing the value of FIELD_LOC_KIND_BITPOS to zero? FIELD_LOC_KIND_BITPOS = 0, (this is where my lack of knowledge of C shows up) > + /* For variable length arrays. Passed to dwarf_locexpr_baton_eval. */ > + struct dwarf2_locexpr_baton *dwarf_block; I would prefer a more generic explaination, because this could be used in any situation where the field location is not known at compile time. /* The field location can be computed by evaluating the following DWARF block. This can be used in Fortran variable-length arrays, for instance. */ > +#define FIELD_STATIC(thisfld) \ > + (FIELD_LOC_KIND (thisfld) == FIELD_LOC_KIND_PHYSNAME \ > + || FIELD_LOC_KIND (thisfld) == FIELD_LOC_KIND_PHYSADDR) With the new scheme, this is no longer an accessor macro, but more like a logical deduction based on field.loc_kind. So I suggest we implement it as a function instead rather than following the accessor-macro style. In theory, using the loc discriminant seems a little iffy to me, compared to having a dedicated flag for that. But it'll work in practice, at least for now. Back to the function, I propose: int field_is_static (struct field *f) { /* "static" fields are the fields whose location is not relative to the address of the enclosing struct. It would be nice to have a dedicated flag that would be set for static fields when the type is being created. But in practice, checking the field loc_kind should give us an accurate answer (at least as long as we assume that DWARF block locations are not going to be used for static fields). FIXME? */ return (FIELD_LOC_KIND (*f) == FIELD_LOC_KIND_PHYSNAME || FIELD_LOC_KIND (*f) == FIELD_LOC_KIND_PHYSADDR); } > +#define TYPE_FIELD_STATIC(thistype, n) FIELD_STATIC (TYPE_FIELD (thistype, n)) If we implement field_is_static, then I propose we delete this macro entirely. > --- gdb/dwarf2read.c 30 Sep 2008 16:57:37 -0000 1.285 > +++ gdb/dwarf2read.c 4 Oct 2008 19:44:18 -0000 > @@ -3561,8 +3561,6 @@ dwarf2_add_field (struct field_info *fip One interesting consequence of your change is that it should help when we want to deal with fields whose position is described with a location list. > /* Get type of field. */ > fp->type = die_type (die, cu); > > - FIELD_STATIC_KIND (*fp) = 0; [...] > - > /* Get bit size of field (zero if none). */ > attr = dwarf2_attr (die, DW_AT_bit_size, cu); > if (attr) > @@ -3590,10 +3588,10 @@ dwarf2_add_field (struct field_info *fip > else > byte_offset = decode_locdesc (DW_BLOCK (attr), cu); > > - FIELD_BITPOS (*fp) = byte_offset * bits_per_byte; > + SET_FIELD_BITPOS (*fp, byte_offset * bits_per_byte); > } > else > - FIELD_BITPOS (*fp) = 0; > + SET_FIELD_BITPOS (*fp, 0); Can we replace the "FIELD_STATIC_KIND = 0" line by "SET_FIELD_BITPOS (*fp, 0);" and then remove the "else" part below? I am convinced that your change is correct, and the kind will always be set because it is set in both branches of the "if/else", but this way feels safer against future changes. -- Joel