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

[Joel Brobecker <brobecker@adacore.com>]

> 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