Re: [patch] [VLA 1/2] Read in the DW_FORM_block* type attributes

[Joel Brobecker <brobecker at adacore dot com>]

Hi Jan,

Today, I might have enough time only for this patch. I'll review
the second patch asap.

> While this part could be applied separately it has a regression
> without 2/2 on: gdb.ada/null_array.exp: print my_table

Let's not apply the patches separately, then.

> -static int dwarf2_get_attr_constant_value (struct attribute *, int);
> +enum dwarf2_get_attr_value
> +  {
> +    dwarf2_attr_unknown,
> +    dwarf2_attr_const,
> +    dwarf2_attr_block
> +  };
> +static enum dwarf2_get_attr_value dwarf2_get_attr_value (struct attribute *attr,
> +							 int *val_return);

The function determines the type of attribute given, and only
if it is of one kind does it set VAL_RETURN. I really think
it's awkward, and I much prefer the current approach where we
have "attr_form_is_..." functions. I think that keeping that
approach will make your patch fit better in the current code.

So:

> -static int attr_form_is_constant (struct attribute *);
> -

Let's not remove that...

> +static struct dwarf2_locexpr_baton *dwarf_block_to_locexpr_baton
> +  (struct dwarf_block *dwarf_block, struct dwarf2_cu *cu);
> +

This part (together with the change in dwarf2_symbol_mark_computed)
seems to be pure code reorganization, which is independent of the
goal for this patch, no? So the hunk above, together with the following
two hunks:

> +/* Convert type dwarf_block type into struct dwarf2_locexpr_baton.  
> +   Note that we're just copying the block's data pointer here, not the actual
> +   data.  We're still pointing into the info_buffer for CU's objfile; right now
> +   we never release that buffer, but when we do clean up properly this may need
> +   to change.  */
> +
> +static struct dwarf2_locexpr_baton *
> +dwarf_block_to_locexpr_baton (struct dwarf_block *dwarf_block,
> +                             struct dwarf2_cu *cu)
> +  struct dwarf2_locexpr_baton *baton;
> +
> +  baton = obstack_alloc (&cu->objfile->objfile_obstack, sizeof (*baton));
> +  baton->per_cu = cu->per_cu;
> +  baton->size = dwarf_block->size;
> +  baton->data = dwarf_block->data;
> +
> +  gdb_assert (baton->per_cu != NULL);
> +  gdb_assert (baton->size == 0 || baton->data != NULL);
> +
> +  return baton;
>  }
> 
>  static void
> @@ -10118,6 +10156,12 @@ dwarf2_symbol_mark_computed (struct attr
>        SYMBOL_OPS (sym) = &dwarf2_loclist_funcs;
>        SYMBOL_LOCATION_BATON (sym) = baton;
>      }
> +  else if (attr_form_is_block (attr))
> +    {
> +      SYMBOL_OPS (sym) = &dwarf2_locexpr_funcs;
> +      SYMBOL_LOCATION_BATON (sym)
> +        = dwarf_block_to_locexpr_baton (DW_BLOCK (attr), cu);
> +    }
>    else
>      {
>        struct dwarf2_locexpr_baton *baton;

... are pre-approved as a separate change. Just one comment, however,
in dwarf_block_to_locexpr_baton: How about moving the assert to the
beginning of the function, in order to avoid the memory allocation
if one of the assertion fails?

>  /* Try to locate the sections we need for DWARF 2 debugging
>     information and return true if we have enough to do something.  */
>  
> @@ -3623,10 +3631,17 @@ dwarf2_add_field (struct field_info *fip
>                dwarf2_complex_location_expr_complaint ();
>                byte_offset = 0;
>              }
> -          else if (attr_form_is_constant (attr))
> -            byte_offset = dwarf2_get_attr_constant_value (attr, 0);
> -          else
> -            byte_offset = decode_locdesc (DW_BLOCK (attr), cu);
> +	  else switch (dwarf2_get_attr_value (attr, &byte_offset))
> +	    {
> +	    case dwarf2_attr_const:
> +	      break;
> +	    case dwarf2_attr_block:
> +	      byte_offset = decode_locdesc (DW_BLOCK (attr), cu);
> +	      break;
> +	    case dwarf2_attr_unknown:
> +	      /* complaint has been called by dwarf2_get_attr_value.  */
> +	      break;
> +	    }

This is one area, for instance, where I think that using
an "else if attr_form_is_block (...)" instead of replacing
part of the if/else-if/else by your switch will keep the code
simple.

> +  /* LOW_BOUND and HIGH_BOUND are set their final values below.  */
> +  range_type = create_range_type (NULL, base_type, 0, - 1);
                                                         ^^^^
                                                     No space in -1

> +  switch (dwarf2_get_attr_value (attr, &low))
> +    {
> +    case dwarf2_attr_unknown:
[...]
I think I see here the reason you introduced the function and the enum.
But I still feel that the interface is a bit awkward. Using a series of
if/else-if/else series should still keep the code readable.

> +    case dwarf2_attr_const:
> +      TYPE_LOW_BOUND (range_type) = low;
> +      if (low >= 0)
> +	TYPE_UNSIGNED (range_type) = 1;

This part, I don't understand. Can you explain why you are doing that?
While trying to find a case where this could cause some problems,
I remembered Ada's empty ranges, that are usually expressed with
a high-bound that's less than the low-bound. Typically, programers
will use "1 .. 0", but also sometimes "0 .. -1". With the code above,
I wonder if "0 .. -1" might be accidently transformed into a very large
range type.

(humpf, I see we do the exact same inside create_range_type :-()

>  #define TYPE_ARRAY_LOWER_BOUND_VALUE(arraytype) \
>     (TYPE_LOW_BOUND(TYPE_INDEX_TYPE((arraytype))))
> +#define TYPE_ARRAY_UPPER_BOUND_VALUE(arraytype) \
> +   (TYPE_HIGH_BOUND(TYPE_INDEX_TYPE((arraytype))))

I'm a little bit worried about these macros, now, because they
definitely assume that the bound is a constant. Should we add
an assertion in TYPE_LOW_BOUND and TYPE_HIGH_BOUND that
TYPE_FIELD_LOC_KIND is FIELD_LOC_KIND_BITPOS?

I would also like to have a comment that explains that these macros
will check that the bound kind is a constant (either because the
bound was not defined, or because it was defined as a constant).
Would you mind adding it?

Thanks,
-- 
Joel