Lazy bitfields

[Vladimir Prus <vladimir at codesourcery dot com>]

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.


--- 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