Lazy bitfields

Lazy bitfields

[Vladimir Prus]

[-- Attachment #1: Type: text/plain, Size: 2495 bytes --]


Now, when creating values corresponding to bitfields, gdb immediately
fetches the values from the memory. That's not good if the memory 
is "read sensitive". 

The quick fix is just not to fetch the value, but in that case, if we have several
bitfields sharing one byte, calling value_fetch_lazy on each bitfield will repeatedly
read that byte -- again not good for read sensitive values.

We've discussed this with Daniel and Jim, and the only workable solution appears
to be adding a pointer from bitfield to the parent value. When calling value_fetch_lazy
on a bitfield, the parent's content is obtained and necessary bits are extracted.
If the parent is originally lazy, first access to its content fetches the bits from the memory.

So, if we have 32-bit value in memory and some 10 bitfields inside it, first access to
any bitfields will fetch the entire value from the memory, and accesses to other bitfields
will just access already-read data. Until the first bitfield is accessed, no memory reads
are performed at all.

One slight problem here is if we have 'parent' pointer, we should make sure that the
parent is not deleted while child still needs it. It turned out that a small bit of reference
counting for parent pointers is sufficient -- no global changes in gdb are required.

A patch that implements this idea is attached, no regressions on x86.

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.

Comments?

- Volodya

	* value.c (struct value): New fields fieldno,
	parent and reference_count.
	(allocate_value): Initialize the parent and reference_count
	fields.
	(value_parent, value_fieldno): New functions.
	(value_free): New function.
	(value_copy): Copy the parent field, increase
	reference count for parent.
	(value_primitive_field): When accessing bitfield,
	not unconditionally fetch them. Don't copy lval type
	is already set inside the function.
	* value.h (value_parent, value_fieldno): Define.
	(value_free): Define as function, not as macro.
	* valops.c (value_fetch_lazy): Handle fetching of
	bitfields.
	(value_assign): Don't handle bitfields in lval_memory
	and lval_register values.  Handle lval_bitfield only.
	* defs.h (enum lval_type): New enumerator lval_bitfield.



[-- Attachment #2: lazy_bitfields__gdb_mainline.diff --]
[-- Type: text/x-diff, Size: 10995 bytes --]

--- gdb/value.c	(/patches/gdb/value_primitive_field/gdb_mainline)	(revision 3210)
+++ gdb/value.c	(/patches/gdb/lazy_bitfields/gdb_mainline)	(revision 3210)
@@ -63,6 +63,10 @@ struct value
     struct internalvar *internalvar;
   } location;
 
+  /* For bitfields, this gives the index of 
+     the field inside parent.  */
+  int fieldno;
+
   /* Describes offset of a value within lval of a structure in bytes.
      If lval == lval_memory, this is an offset to the address.  If
      lval == lval_register, this is a further offset from
@@ -129,6 +133,18 @@ struct value
   int embedded_offset;
   int pointed_to_offset;
 
+  /* Pointer to parent variable.  This pointer is
+     set only when a value needs access to the value of the
+     parent, for example for bitfields.  */
+  struct value *parent;
+
+  /* The number of references for this value.  This counts
+     all references via the 'parent' pointer, and either
+     0 or 1 depending on whether there's reference to this
+     value from other gdb code.  References from that 'other code'
+     are managed using explicit calls to value_free and release_value.  */
+  int reference_count;
+
   /* Values are stored in a chain, so that they can be deleted easily
      over calls to the inferior.  Values assigned to internal
      variables or put into the value history are taken off this
@@ -233,6 +249,8 @@ allocate_value (struct type *type)
   val->embedded_offset = 0;
   val->pointed_to_offset = 0;
   val->modifiable = 1;
+  val->parent = NULL;
+  val->reference_count = 1;
   return val;
 }
 
@@ -432,6 +450,16 @@ deprecated_value_internalvar_hack (struc
   return &value->location.internalvar;
 }
 
+extern struct value *value_parent (struct value *value)
+{
+  return value->parent;
+}
+
+extern int value_fieldno (struct value *value)
+{
+  return value->fieldno;
+}
+
 struct frame_id *
 deprecated_value_frame_id_hack (struct value *value)
 {
@@ -540,6 +568,17 @@ value_release_to_mark (struct value *mar
   return val;
 }
 
+extern void value_free (struct value *value)
+{
+  /* If there's a parent, drop parents's reference count
+     and free it if necessary.  */
+  if (value->parent)
+    value_free (value->parent);
+
+  if (--value->reference_count == 0)
+    xfree (value);      
+}
+
 /* Return a copy of the value ARG.
    It contains the same contents, for same memory address,
    but it's a different block of storage.  */
@@ -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;
   if (!value_lazy (val))
     {
       memcpy (value_contents_all_raw (val), value_contents_all_raw (arg),
@@ -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))
     {
@@ -1340,7 +1392,9 @@ value_primitive_field (struct value *arg
       v->offset = (value_offset (arg1) + offset
 		   + value_embedded_offset (arg1));
     }
-  VALUE_LVAL (v) = VALUE_LVAL (arg1);
+  
+  if (v->lval == not_lval)
+    VALUE_LVAL (v) = VALUE_LVAL (arg1);
   if (VALUE_LVAL (arg1) == lval_internalvar)
     VALUE_LVAL (v) = lval_internalvar_component;
   v->location = arg1->location;
--- gdb/value.h	(/patches/gdb/value_primitive_field/gdb_mainline)	(revision 3210)
+++ gdb/value.h	(/patches/gdb/lazy_bitfields/gdb_mainline)	(revision 3210)
@@ -211,6 +211,10 @@ extern CORE_ADDR *deprecated_value_addre
 extern struct internalvar **deprecated_value_internalvar_hack (struct value *);
 #define VALUE_INTERNALVAR(val) (*deprecated_value_internalvar_hack (val))
 
+extern struct value *value_parent (struct value *v);
+
+extern int value_fieldno (struct value *v);
+
 /* Frame register value is relative to.  This will be described in the
    lval enum above as "lval_register".  */
 extern struct frame_id *deprecated_value_frame_id_hack (struct value *);
@@ -458,7 +462,7 @@ extern int unop_user_defined_p (enum exp
 
 extern int destructor_name_p (const char *name, const struct type *type);
 
-#define value_free(val) xfree (val)
+extern void value_free (struct value *value);
 
 extern void free_all_values (void);
 
--- gdb/valops.c	(/patches/gdb/value_primitive_field/gdb_mainline)	(revision 3210)
+++ gdb/valops.c	(/patches/gdb/lazy_bitfields/gdb_mainline)	(revision 3210)
@@ -541,13 +541,30 @@ value_at_lazy (struct type *type, CORE_A
 int
 value_fetch_lazy (struct value *val)
 {
-  CORE_ADDR addr = VALUE_ADDRESS (val) + value_offset (val);
-  int length = TYPE_LENGTH (value_enclosing_type (val));
-
   struct type *type = value_type (val);
-  if (length)
-    read_memory (addr, value_contents_all_raw (val), length);
+  if (VALUE_LVAL (val) == lval_bitfield)
+    {
+      LONGEST num;
+      int len;      
 
+      num = unpack_field_as_long (value_type (value_parent (val)),
+				  value_contents (value_parent (val)),
+				  value_fieldno (val));
+
+      type = check_typedef (type);
+      len = TYPE_LENGTH (type);
+	  
+      store_signed_integer (value_contents_raw (val), len, num);
+    }
+  else
+    {  
+      CORE_ADDR addr = VALUE_ADDRESS (val) + value_offset (val);
+      int length = TYPE_LENGTH (value_enclosing_type (val));
+       
+      if (length)
+	read_memory (addr, value_contents_all_raw (val), length);
+    }
+      
   set_value_lazy (val, 0);
   return 0;
 }
@@ -600,37 +617,15 @@ value_assign (struct value *toval, struc
 
     case lval_memory:
       {
+	int changed_len;
 	const gdb_byte *dest_buffer;
 	CORE_ADDR changed_addr;
-	int changed_len;
         gdb_byte buffer[sizeof (LONGEST)];
 
-	if (value_bitsize (toval))
-	  {
-	    /* We assume that the argument to read_memory is in units of
-	       host chars.  FIXME:  Is that correct?  */
-	    changed_len = (value_bitpos (toval)
-			   + value_bitsize (toval)
-			   + HOST_CHAR_BIT - 1)
-	      / HOST_CHAR_BIT;
-
-	    if (changed_len > (int) sizeof (LONGEST))
-	      error (_("Can't handle bitfields which don't fit in a %d bit word."),
-		     (int) sizeof (LONGEST) * HOST_CHAR_BIT);
-
-	    read_memory (VALUE_ADDRESS (toval) + value_offset (toval),
-			 buffer, changed_len);
-	    modify_field (buffer, value_as_long (fromval),
-			  value_bitpos (toval), value_bitsize (toval));
-	    changed_addr = VALUE_ADDRESS (toval) + value_offset (toval);
-	    dest_buffer = buffer;
-	  }
-	else
-	  {
-	    changed_addr = VALUE_ADDRESS (toval) + value_offset (toval);
-	    changed_len = TYPE_LENGTH (type);
-	    dest_buffer = value_contents (fromval);
-	  }
+	gdb_assert (!value_bitsize (toval));
+	changed_addr = VALUE_ADDRESS (toval) + value_offset (toval);
+	changed_len = TYPE_LENGTH (type);
+	dest_buffer = value_contents (fromval);
 
 	write_memory (changed_addr, dest_buffer, changed_len);
 	if (deprecated_memory_changed_hook)
@@ -659,38 +654,11 @@ value_assign (struct value *toval, struc
 	  }
 	else
 	  {
-	    if (value_bitsize (toval))
-	      {
-		int changed_len;
-		gdb_byte buffer[sizeof (LONGEST)];
-
-		changed_len = (value_bitpos (toval)
-			       + value_bitsize (toval)
-			       + HOST_CHAR_BIT - 1)
-		  / HOST_CHAR_BIT;
-
-		if (changed_len > (int) sizeof (LONGEST))
-		  error (_("Can't handle bitfields which don't fit in a %d bit word."),
-			 (int) sizeof (LONGEST) * HOST_CHAR_BIT);
-
-		get_frame_register_bytes (frame, value_reg,
-					  value_offset (toval),
-					  changed_len, buffer);
-
-		modify_field (buffer, value_as_long (fromval),
-			      value_bitpos (toval), value_bitsize (toval));
-
-		put_frame_register_bytes (frame, value_reg,
-					  value_offset (toval),
-					  changed_len, buffer);
-	      }
-	    else
-	      {
-		put_frame_register_bytes (frame, value_reg,
-					  value_offset (toval),
-					  TYPE_LENGTH (type),
-					  value_contents (fromval));
-	      }
+	    gdb_assert (!value_bitsize (toval));
+	    put_frame_register_bytes (frame, value_reg,
+				      value_offset (toval),
+				      TYPE_LENGTH (type),
+				      value_contents (fromval));
 	  }
 
 	if (deprecated_register_changed_hook)
@@ -698,6 +666,43 @@ value_assign (struct value *toval, struc
 	observer_notify_target_changed (&current_target);
 	break;
       }
+
+    case lval_bitfield:
+      {
+	int changed_len;
+	gdb_byte *content;
+	
+	/* We assume that the argument to read_memory is in units of
+	   host chars.  FIXME:  Is that correct?  */
+	changed_len = (value_bitpos (toval)
+		       + value_bitsize (toval)
+		       + HOST_CHAR_BIT - 1)
+	  / HOST_CHAR_BIT;
+	
+	if (changed_len > (int) sizeof (LONGEST))
+	  error (_("Can't handle bitfields which don't fit in a %d bit word."),
+		 (int) sizeof (LONGEST) * HOST_CHAR_BIT);
+
+	content = value_contents_writeable (value_parent (toval)) +
+	  value_offset (toval);
+	modify_field (content, value_as_long (fromval),
+		      value_bitpos (toval), value_bitsize (toval));
+
+	if (VALUE_LVAL (value_parent (toval)) == lval_register)
+	  {
+	    struct frame_info *frame = 
+	      frame_find_by_id (VALUE_FRAME_ID (toval));
+	    put_frame_register_bytes (frame, VALUE_REGNUM (toval),
+				      value_offset (toval),		
+				      changed_len, content);
+	  }
+	else
+	  write_memory (VALUE_ADDRESS (value_parent (toval))
+			+ value_offset (toval),
+			content, changed_len);
+
+	break;
+      }
       
     default:
       error (_("Left operand of assignment is not an lvalue."));
--- gdb/defs.h	(/patches/gdb/value_primitive_field/gdb_mainline)	(revision 3210)
+++ gdb/defs.h	(/patches/gdb/lazy_bitfields/gdb_mainline)	(revision 3210)
@@ -671,7 +671,9 @@ enum lval_type
     /* In a gdb internal variable.  */
     lval_internalvar,
     /* Part of a gdb internal variable (structure field).  */
-    lval_internalvar_component
+    lval_internalvar_component,
+    /* Bitfield of a structure.  */
+    lval_bitfield
   };
 
 /* Control types for commands */

Property changes on: 
___________________________________________________________________
Name: csl:base
 +/all/patches/gdb/value_primitive_field/gdb_mainline
Name: svk:merge
 +d48a11ec-ee1c-0410-b3f5-c20844f99675:/work/gdb/gdb_mainline:2531
 +e7755896-6108-0410-9592-8049d3e74e28:/mirrors/gdb/trunk:160955

Re: Lazy bitfields

[Daniel Jacobowitz]

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

Re: Lazy bitfields

[Daniel Jacobowitz]

On Thu, Feb 08, 2007 at 10:28:56AM -0500, Daniel Jacobowitz wrote:
> 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.

This is all very twisted.  I bet we could eliminate the non-zero
cases, but it would take some untangling - though the result would
be simpler than what we have now.  Here's a C++ test case:

struct A
{
  int x : 12;
  int y : 14;
};

struct A a;

struct pad
{
  int useless;
};

struct B : pad, A
{
  int no;
};

struct B b;

If you print out b.y, value_primitive_field will be called with an
offset of 4.  arg1 will be a value of type B, but arg_type will be
type A.  I think it should have passed a value of type A, instead
of trying to do two steps at once... but that's what it does now.

The difference between value_type (arg1) and arg_type is something else
I hadn't noticed before.  That means you're saving a fieldno into
arg_type but applying it to value_type (value_parent (val)), which
has a similar problem.

I think you can work around this in two ways.  We could fix all the
current jumping through hoops and eliminate the offset argument, or you
could avoid using the parent's type or fieldno.  You want value_bitsize
bits, from the parents contents plus a bit offset of:

   ((value_offset (val) - value_offset (value_parent (val))) * 8
    + value_bitpos (val))

The difference in the two offsets gives you back the 'offset' argument
to value_primitive_field plus the byte offset of the field, the
value_bitpos gives you the additional bit offset of the field.

You'd need something like unpack_field_as_long, but one which replaced
the type and fieldno arguments with field_type (for TYPE_UNSIGNED),
bitpos, and bitsize.

Did that make any sense at all?

-- 
Daniel Jacobowitz
CodeSourcery