RFC: Lazy bitfields

[Daniel Jacobowitz <drow at false dot org>]

Two and a half years ago, Vladimir posted a patch to implement lazy
bitfields:

  http://sourceware.org/ml/gdb-patches/2007-01/msg00494.html

Our concern at the time was volatile bitfields mapped to hardware
registers.  Using MI, we ended up with a lazy value for each bitfield,
and this led to lots of reads from the target.  To solve this problem,
Vladimir added a "parent" pointer to each value.  We would create
all the children from the same parent, so they'd end up sharing
a contents buffer this way.

This patch is based on Vladimir's.  It's a bit smaller, because I
opted not to do any sharing for writes.  Suppose you have a
register...

  volatile struct {
    int a : 10;
    int b : 22;
  } REG;

With this patch, if you want to do a single write to REG, then assign
to the entire register.  Otherwise, assign to REG.a and GDB will do
what a compiler (generally) would: read REG, modify REG.a, write REG
back to memory.

I did add some cleverness to prefer container sizes.  So if we can,
we'll do an int sized memory write here.  That's the behavior
documented in the ARM EABI, and DJ Delorie is currently working on
implementation and documentation for this for all targets in GCC.
So it seems like a good choice in GDB too.

I'd make GDB obey the rules on reads, too, but that's much harder and
not as clear cut.  In a compiler, you want to access the register
twice to display REG.a and REG.b; but in GDB the intuitive thing is to
only access REG.  So, we read the entire containing value.  If you
have a structure for each REG, this works out perfectly.  If you have
a huge structure containing thousands of bitfields, accessing one will
read the whole structure.  Turns out that this is what GDB would
already do, which brings me to...

...the troublesome bit.  The patch didn't make as much difference
as I expected.  Without it, value_primitive_field contained a call to
value_contents on the containing structure for bitfields.
Non-bitfields could be created lazily, but not bitfields.  After the
patch, we do create lazy bitfields, but the only way I could coax GDB
into them lazy was to assign to them immediately.  Everything else
leads to a fetch pretty quickly.

My going theory is that one of the varobj patches between now and then
cut down on its use of lazy values.  Now we only create values if
we intend to read them, but I don't think that was always the case.

One thing it does help with is assignments to bitfields.  We no longer
read the old value (and thus the entire containing structure) from
memory if we don't have to, since it's created as lazy.  We still do a
read-modify-write cycle for the store, of course.

I can see plenty of ways that changing GDB could cause this to become
more important.  For example, if we clean up the warts in the value
printer to use "struct value" everywhere, it's likely to go through a
lot of lazy values.

Tested on x86_64-linux, no regressions.  Does anyone have comments?
Otherwise, I'll commit this in a few days.

-- 
Daniel Jacobowitz
CodeSourcery

2009-07-17  Daniel Jacobowitz  <dan@codesourcery.com>
	    Vladimir Prus <vladimir@codesourcery.com>

	* valops.c (value_fetch_lazy): Handle bitfields explicitly.
	(value_assign): Remove unnecessary FIXME.  Honor the container
	type of bitfields if possible.
	* value.c (struct value): Add parent field.
	(value_parent): New function.
	(value_free): Free the parent also.
	(value_copy): Copy the parent also.
	(value_primitive_field): Do not read the contents of a lazy
	value to create a child bitfield value.  Set bitpos and offset
	according to the container type if possible.
	(unpack_bits_as_long): Rename from unpack_field_as_long.  Take
	field_type, bitpos, and bitsize instead of type and fieldno.
	(unpack_field_as_long): Use unpack_bits_as_long.
	* value.h (value_parent, unpack_bits_as_long): New prototypes.

---
 gdb/valops.c |   31 +++++++++++++++++++++--
 gdb/value.c  |   77 +++++++++++++++++++++++++++++++++++++++++++++--------------
 gdb/value.h  |    8 ++++++
 3 files changed, 96 insertions(+), 20 deletions(-)

Index: src/gdb/valops.c
===================================================================
--- src.orig/gdb/valops.c	2009-07-17 14:20:08.000000000 -0400
+++ src/gdb/valops.c	2009-07-17 15:14:50.000000000 -0400
@@ -632,7 +632,25 @@ value_fetch_lazy (struct value *val)
 {
   gdb_assert (value_lazy (val));
   allocate_value_contents (val);
-  if (VALUE_LVAL (val) == lval_memory)
+  if (value_bitsize (val))
+    {
+      /* To read a lazy bitfield, read the entire enclosing value.  This
+	 prevents reading the same block of (possibly volatile) memory once
+         per bitfield.  It would be even better to read only the containing
+         word, but we have no way to record that just specific bits of a
+         value have been fetched.  */
+      struct type *type = check_typedef (value_type (val));
+      enum bfd_endian byte_order = gdbarch_byte_order (get_type_arch (type));
+      struct value *parent = value_parent (val);
+      LONGEST offset = value_offset (val);
+      LONGEST num = unpack_bits_as_long (value_type (val),
+					 value_contents (parent) + offset,
+					 value_bitpos (val),
+					 value_bitsize (val));
+      int length = TYPE_LENGTH (type);
+      store_signed_integer (value_contents_raw (val), length, byte_order, num);
+    }
+  else if (VALUE_LVAL (val) == lval_memory)
     {
       CORE_ADDR addr = value_address (val);
       int length = TYPE_LENGTH (check_typedef (value_enclosing_type (val)));
@@ -800,13 +818,20 @@ value_assign (struct value *toval, struc
 
 	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 we can read-modify-write exactly the size of the
+	       containing type (e.g. short or int) then do so.  This
+	       is safer for volatile bitfields mapped to hardware
+	       registers.  */
+	    if (changed_len < TYPE_LENGTH (type)
+		&& TYPE_LENGTH (type) <= (int) sizeof (LONGEST)
+		&& ((LONGEST) value_address (toval) % TYPE_LENGTH (type)) == 0)
+	      changed_len = TYPE_LENGTH (type);
+
 	    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);
Index: src/gdb/value.c
===================================================================
--- src.orig/gdb/value.c	2009-07-17 14:23:13.000000000 -0400
+++ src/gdb/value.c	2009-07-17 15:21:36.000000000 -0400
@@ -108,6 +108,11 @@ struct value
      gdbarch_bits_big_endian=1 targets, it is the position of the MSB. */
   int bitpos;
 
+  /* Only used for bitfields; the containing value.  This allows a
+     single read from the target when displaying multiple
+     bitfields.  */
+  struct value *parent;
+
   /* Frame register value is relative to.  This will be described in
      the lval enum above as "lval_register".  */
   struct frame_id frame_id;
@@ -396,6 +401,12 @@ set_value_bitsize (struct value *value, 
   value->bitsize = bit;
 }
 
+struct value *
+value_parent (struct value *value)
+{
+  return value->parent;
+}
+
 gdb_byte *
 value_contents_raw (struct value *value)
 {
@@ -615,6 +626,11 @@ value_free (struct value *val)
       if (val->reference_count > 0)
 	return;
 
+      /* If there's an associated parent value, drop our reference to
+	 it.  */
+      if (val->parent != NULL)
+	value_free (val->parent);
+
       if (VALUE_LVAL (val) == lval_computed)
 	{
 	  struct lval_funcs *funcs = val->location.computed.funcs;
@@ -737,6 +753,9 @@ value_copy (struct value *arg)
 	      TYPE_LENGTH (value_enclosing_type (arg)));
 
     }
+  val->parent = arg->parent;
+  if (val->parent)
+    value_incref (val->parent);
   if (VALUE_LVAL (val) == lval_computed)
     {
       struct lval_funcs *funcs = val->location.computed.funcs;
@@ -1859,15 +1878,28 @@ 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));
-      v->bitpos = TYPE_FIELD_BITPOS (arg_type, fieldno) % 8;
+      /* Create a new value for the bitfield, with bitpos and bitsize
+	 set.  If possible, arrange offset and bitpos so that we can
+	 do a single aligned read of the size of the containing type.
+	 Otherwise, adjust offset to the byte containing the first
+	 bit.  Assume that the address, offset, and embedded offset
+	 are sufficiently aligned.  */
+      int bitpos = TYPE_FIELD_BITPOS (arg_type, fieldno);
+      int container_bitsize = TYPE_LENGTH (type) * 8;
+
+      v = allocate_value_lazy (type);
       v->bitsize = TYPE_FIELD_BITSIZE (arg_type, fieldno);
-      v->offset = value_offset (arg1) + offset
-	+ TYPE_FIELD_BITPOS (arg_type, fieldno) / 8;
+      if ((bitpos % container_bitsize) + v->bitsize <= container_bitsize
+	  && TYPE_LENGTH (type) <= (int) sizeof (LONGEST))
+	v->bitpos = bitpos % container_bitsize;
+      else
+	v->bitpos = bitpos % 8;
+      v->offset = value_offset (arg1) + value_embedded_offset (arg1)
+	+ (bitpos - v->bitpos) / 8;
+      v->parent = arg1;
+      value_incref (v->parent);
+      if (!value_lazy (arg1))
+	value_fetch_lazy (v);
     }
   else if (fieldno < TYPE_N_BASECLASSES (arg_type))
     {
@@ -1992,8 +2024,9 @@ value_fn_field (struct value **arg1p, st
 }
 
 
-/* Unpack a field FIELDNO of the specified TYPE, from the anonymous object at
-   VALADDR.
+/* Unpack a bitfield of the specified FIELD_TYPE, from the anonymous
+   object at VALADDR.  The bitfield starts at BITPOS bits and contains
+   BITSIZE bits.
 
    Extracting bits depends on endianness of the machine.  Compute the
    number of least significant bits to discard.  For big endian machines,
@@ -2007,24 +2040,21 @@ value_fn_field (struct value **arg1p, st
    If the field is signed, we also do sign extension. */
 
 LONGEST
-unpack_field_as_long (struct type *type, const gdb_byte *valaddr, int fieldno)
+unpack_bits_as_long (struct type *field_type, const gdb_byte *valaddr,
+		     int bitpos, int bitsize)
 {
-  enum bfd_endian byte_order = gdbarch_byte_order (get_type_arch (type));
+  enum bfd_endian byte_order = gdbarch_byte_order (get_type_arch (field_type));
   ULONGEST val;
   ULONGEST valmask;
-  int bitpos = TYPE_FIELD_BITPOS (type, fieldno);
-  int bitsize = TYPE_FIELD_BITSIZE (type, fieldno);
   int lsbcount;
-  struct type *field_type;
 
   val = extract_unsigned_integer (valaddr + bitpos / 8,
 				  sizeof (val), byte_order);
-  field_type = TYPE_FIELD_TYPE (type, fieldno);
   CHECK_TYPEDEF (field_type);
 
   /* Extract bits.  See comment above. */
 
-  if (gdbarch_bits_big_endian (get_type_arch (type)))
+  if (gdbarch_bits_big_endian (get_type_arch (field_type)))
     lsbcount = (sizeof val * 8 - bitpos % 8 - bitsize);
   else
     lsbcount = (bitpos % 8);
@@ -2048,6 +2078,19 @@ unpack_field_as_long (struct type *type,
   return (val);
 }
 
+/* Unpack a field FIELDNO of the specified TYPE, from the anonymous object at
+   VALADDR.  See unpack_bits_as_long for more details.  */
+
+LONGEST
+unpack_field_as_long (struct type *type, const gdb_byte *valaddr, int fieldno)
+{
+  int bitpos = TYPE_FIELD_BITPOS (type, fieldno);
+  int bitsize = TYPE_FIELD_BITSIZE (type, fieldno);
+  struct type *field_type = TYPE_FIELD_TYPE (type, fieldno);
+
+  return unpack_bits_as_long (field_type, valaddr, bitpos, bitsize);
+}
+
 /* Modify the value of a bitfield.  ADDR points to a block of memory in
    target byte order; the bitfield starts in the byte pointed to.  FIELDVAL
    is the desired value of the field, in host byte order.  BITPOS and BITSIZE
Index: src/gdb/value.h
===================================================================
--- src.orig/gdb/value.h	2009-07-17 14:23:13.000000000 -0400
+++ src/gdb/value.h	2009-07-17 14:23:14.000000000 -0400
@@ -76,6 +76,12 @@ extern void set_value_bitsize (struct va
 extern int value_bitpos (struct value *);
 extern void set_value_bitpos (struct value *, int bit);
 
+/* Only used for bitfields; the containing value.  This allows a
+   single read from the target when displaying multiple
+   bitfields.  */
+
+struct value *value_parent (struct value *);
+
 /* 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 location.address
@@ -329,6 +335,8 @@ extern LONGEST unpack_long (struct type 
 extern DOUBLEST unpack_double (struct type *type, const gdb_byte *valaddr,
 			       int *invp);
 extern CORE_ADDR unpack_pointer (struct type *type, const gdb_byte *valaddr);
+LONGEST unpack_bits_as_long (struct type *field_type, const gdb_byte *valaddr,
+			     int bitpos, int bitsize);
 extern LONGEST unpack_field_as_long (struct type *type,
 				     const gdb_byte *valaddr,
 				     int fieldno);