Re: [patch] static_kind -> bit0, bit1 [Re: [gdb] Fortran dynamic arrays]

Joel Brobecker
2008-10-06T20:00:42Z

> 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