Re: Lazy bitfields

[Daniel Jacobowitz <drow@false.org>]

On Wed, Jan 24, 2007 at 11:23:43AM +0300, Vladimir Prus wrote:
> One nit is that when writing a bitfield, we first fetch the entire
> parent value, modify bits and write entire parent value. In some
> *limited* set of cases it is possible to read only some of the bytes,
> or not read anything at all, but introducing bit-mask telling which
> bits are read and which are not is more trouble that its worth.

The other nit is that when we read a bitfield, we read the whole
struct, which might be much larger.

I don't think either of these nits are problematic.  Your approach
seems fine.


> +extern struct value *value_parent (struct value *value)
> +{
> +  return value->parent;
> +}
> +
> +extern int value_fieldno (struct value *value)
> +{
> +  return value->fieldno;
> +}

> +extern void value_free (struct value *value)

I haven't gotten to complain about your formatting in a while :-)
Function at start of line please.

> +  /* If there's a parent, drop parents's reference count

"parent's"

> @@ -562,6 +601,11 @@ value_copy (struct value *arg)
>    val->embedded_offset = value_embedded_offset (arg);
>    val->pointed_to_offset = arg->pointed_to_offset;
>    val->modifiable = arg->modifiable;
> +  val->fieldno = arg->fieldno;
> +  val->parent = arg->parent;
> +  if (val->parent)
> +    ++val->parent->reference_count;
> +  val->reference_count = 1;

Don't think you need the last line - allocate_value should have set
it to one.

> @@ -1300,15 +1344,23 @@ value_primitive_field (struct value *arg
>  
>    if (TYPE_FIELD_BITSIZE (arg_type, fieldno))
>      {
> -      v = value_from_longest (type,
> -			      unpack_field_as_long (arg_type,
> -						    value_contents (arg1)
> -						    + offset,
> -						    fieldno));
> +      LONGEST num;
> +      int len;
> +
> +      
> +      v = allocate_value (type);
>        v->bitpos = TYPE_FIELD_BITPOS (arg_type, fieldno) % 8;
>        v->bitsize = TYPE_FIELD_BITSIZE (arg_type, fieldno);
>        v->offset = value_offset (arg1) + offset
>  	+ TYPE_FIELD_BITPOS (arg_type, fieldno) / 8;
> +      v->parent = arg1;
> +      v->lval = lval_bitfield;
> +      v->fieldno = fieldno;
> +      
> +      if (!value_lazy (arg1))
> +	value_fetch_lazy (v);
> +      else
> +	set_value_lazy (v, 1);
>      }
>    else if (fieldno < TYPE_N_BASECLASSES (arg_type))
>      {

Two problems here.  First of all, the whole point of adding reference
counts is to increment the parent's reference count here, right? 
Better do that then :-)

Second, I'm worried about the "offset" argument to
value_primitive_field.  This was primarily for CHILL, due to a
strange choice when generating or reading debug info, and it's
possible that it's never non-zero any more, which would be nice.
But if it ever is non-zero, this will break it.  It gets lumped
in with value_offset so when you call unpack_field_as_long in
value_fetch_lazy you can't pick it out again to apply it to the
parent.

That's my only real concern with this patch; everything else
looks OK.  I'm going to try to figure out if that argument is
ever non-zero or if we can eliminate it now.

-- 
Daniel Jacobowitz
CodeSourcery