This is the mail archive of the
gdb-patches@sourceware.org
mailing list for the GDB project.
Re: [patch] static_kind -> bit0, bit1 [Re: [gdb] Fortran dynamic arrays]
- From: Joel Brobecker <brobecker at adacore dot com>
- To: Jan Kratochvil <jan dot kratochvil at redhat dot com>
- Cc: gdb-patches at sourceware dot org, Tobias Burnus <burnus at net-b dot de>, Ulrich Weigand <uweigand at de dot ibm dot com>, Jim Blandy <jimb at red-bean dot com>
- Date: Mon, 6 Oct 2008 16:00:01 -0400
- Subject: Re: [patch] static_kind -> bit0, bit1 [Re: [gdb] Fortran dynamic arrays]
- 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>
> 2008-10-04 Jan Kratochvil <jan.kratochvil@redhat.com>
>
> 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