[RFC] "lazily" allocate the raw content of lazy values

[RFC] "lazily" allocate the raw content of lazy values

[Jerome Guitton]

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


The main purpose of this patch would be to allow a clean-up in
ada-lang.c, making it a little bit "closer" to the other language
supports. This implements lazy allocation of value content. That is
to say: the actual content part of the struct value would not be
allocated by default for lazy values; it would only be allocated at
"fetch" time.

Let me give some context. In Ada, passing a big array or record in
parameter of a function is a quite common programming idiom; this big
object is passed by referenced, its passing mode is typically "in
out". If GDB tries to allocate a struct value for such a beast, the
allocation may fail.

If the user would not expect to debugger to print the whole object, he
would at least think that printing a slice of his array (or some
components of his record) should be possible. For now, here is what
ada-lang does: it never dereferences such objects, and builds
references to slices (or references to fields).

This solution is not completely satisfactory; it is quite different
than what the other language supports do. And we end up manipulating
refs in a lot of places where they should not appear. For example,
consider the formatted-ref test in the GDB testsuite, and suppose that
you ask GDB to evaluate the Ada expression "s.x = 13" (equality test
between "s.x" and "13", "s" being a record passed by reference); to
compare a ref to "x" to the scalar 13, GDB would try to cast "13" to
the type "ref to integer". This would return an error: as "13" is not
in memory, making it a ref does not make much sense. In AdaCore's GDB,
we have changed ada_value_cast to use the target type when a reference
type is given in parameter. This fixes this problem, but makes the
semantics of ada_value_cast quite different than value_cast; not very
satisfactory...

Here, an elegant solution would be to manipulate lazy values instead
of references; but, the way it is currently implemented, whenever
GDB creates a value, lazy or not, it allocates sufficient space to
hold that value when it is eventually read. So this does not protect
us against the allocation failure I described.

This patch would fix this issue: the raw content spaces is not
allocated in the lazy value by default, it is only done if this raw
content is fetched from the target. Interestingly, a side effect of
this change is that it makes value_change_enclosing_type much simpler.

Comments? I have tested it on x86-linux, with a C++/Ada compiler,
no regression. 


2008-11-24  Jerome Guitton  <guitton@adacore.com>

	* value.h (allocate_value_lazy): New function declaration.
	(value_free): Remove macro, make it a function.
	* value.c (value_aligner): New union, extracted from struct value.
	(value): Move actual content outside of the struct; add a pointer
	to this actual content.
	(allocate_value_lazy, allocate_value_content): New function.
	(allocate_value): Reimplement using these two new functions.
	(value_contents_raw, value_contents_all_raw): If no memory
	has been allocated yet for the actual content, allocate it.
	(value_contents_all): Resync with struct value's changes.
	(value_free): New function.
	(value_copy, value_primitive_field): Use new function
	allocate_value_lazy to allocate lazy values.
	(value_change_enclosing_type): Resync with struct value's changes.
	As the value is not reallocated, remove the special handling for
	the value chain (now obsolete).
	* valops.c (value_at_lazy): Use new function allocate_value_lazy.
	(value_fetch_lazy): Allocate value content. Use allocate_value_lazy
	to allocate lazy values.
	(value_slice): Use allocate_value_lazy to allocate lazy values.


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

Index: value.c
===================================================================
--- value.c	(revision 138922)
+++ value.c	(working copy)
@@ -44,6 +44,20 @@
 
 void _initialize_values (void);
 
+/* Actual contents of the value.  For use of this value; setting it
+   uses the stuff defined in struct value.  Target byte-order.  We force it
+   to be aligned properly for any possible value.  Note that a value therefore
+   extends beyond what is declared here.  */
+
+union value_aligner
+{
+  gdb_byte contents[1];
+  DOUBLEST force_doublest_align;
+  LONGEST force_longest_align;
+  CORE_ADDR force_core_addr_align;
+  void *force_pointer_align;  
+};
+
 struct value
 {
   /* Type of value; either not an lval, or one of the various
@@ -163,21 +177,8 @@ struct value
   /* If value is a variable, is it initialized or not.  */
   int initialized;
 
-  /* Actual contents of the value.  For use of this value; setting it
-     uses the stuff above.  Not valid if lazy is nonzero.  Target
-     byte-order.  We force it to be aligned properly for any possible
-     value.  Note that a value therefore extends beyond what is
-     declared here.  */
-  union
-  {
-    gdb_byte contents[1];
-    DOUBLEST force_doublest_align;
-    LONGEST force_longest_align;
-    CORE_ADDR force_core_addr_align;
-    void *force_pointer_align;
-  } aligner;
-  /* Do not add any new members here -- contents above will trash
-     them.  */
+  /* Actual contents of the value. NULL or not valid if lazy is nonzero. */
+  union value_aligner *content;
 };
 
 /* Prototypes for local functions. */
@@ -213,15 +214,18 @@ static int value_history_count;	/* Abs n
 
 static struct value *all_values;
 
-/* Allocate a  value  that has the correct length for type TYPE.  */
+/* Allocate a lazy value for type TYPE.  Its actual content is
+   "lazily" allocated too: the content field of the return value is
+   NULL; it will be allocated when it is fetched from the target.  */
 
 struct value *
-allocate_value (struct type *type)
+allocate_value_lazy (struct type *type)
 {
   struct value *val;
   struct type *atype = check_typedef (type);
 
-  val = (struct value *) xzalloc (sizeof (struct value) + TYPE_LENGTH (atype));
+  val = (struct value *) xzalloc (sizeof (struct value));
+  val->content = NULL;
   val->next = all_values;
   all_values = val;
   val->type = type;
@@ -233,7 +237,7 @@ allocate_value (struct type *type)
   val->bitpos = 0;
   val->bitsize = 0;
   VALUE_REGNUM (val) = -1;
-  val->lazy = 0;
+  val->lazy = 1;
   val->optimized_out = 0;
   val->embedded_offset = 0;
   val->pointed_to_offset = 0;
@@ -242,6 +246,29 @@ allocate_value (struct type *type)
   return val;
 }
 
+/* Allocate the content of the value VAL.  */
+
+void
+allocate_value_content (struct value *val)
+{
+  if (!val->content)
+    {
+      val->content =
+        (union value_aligner *) xzalloc (TYPE_LENGTH (val->enclosing_type));
+    }
+}
+
+/* Allocate a  value  that has the correct length for type TYPE.  */
+
+struct value *
+allocate_value (struct type *type)
+{
+  struct value *val = allocate_value_lazy (type);
+  allocate_value_content (val);
+  val->lazy = 0;
+  return val;
+}
+
 /* Allocate a  value  that has the correct length
    for COUNT repetitions of type TYPE.  */
 
@@ -340,13 +367,17 @@ set_value_bitsize (struct value *value, 
 gdb_byte *
 value_contents_raw (struct value *value)
 {
-  return value->aligner.contents + value->embedded_offset;
+  if (!value->content)
+    allocate_value_content (value);
+  return value->content->contents + value->embedded_offset;
 }
 
 gdb_byte *
 value_contents_all_raw (struct value *value)
 {
-  return value->aligner.contents;
+  if (!value->content)
+    allocate_value_content (value);
+  return value->content->contents;
 }
 
 struct type *
@@ -360,7 +391,7 @@ value_contents_all (struct value *value)
 {
   if (value->lazy)
     value_fetch_lazy (value);
-  return value->aligner.contents;
+  return value->content->contents;
 }
 
 int
@@ -495,6 +526,14 @@ value_mark (void)
   return all_values;
 }
 
+void
+value_free (struct value *val)
+{
+  if (val && val->content)
+    xfree (val->content);
+  xfree (val);
+}
+
 /* Free all values allocated since MARK was obtained by value_mark
    (except for those released).  */
 void
@@ -579,7 +618,12 @@ struct value *
 value_copy (struct value *arg)
 {
   struct type *encl_type = value_enclosing_type (arg);
-  struct value *val = allocate_value (encl_type);
+  struct value *val;
+
+  if (value_lazy (arg))
+    val = allocate_value_lazy (encl_type);
+  else
+    val = allocate_value (encl_type);
   val->type = arg->type;
   VALUE_LVAL (val) = VALUE_LVAL (arg);
   val->location = arg->location;
@@ -1319,39 +1363,13 @@ value_static_field (struct type *type, i
 struct value *
 value_change_enclosing_type (struct value *val, struct type *new_encl_type)
 {
-  if (TYPE_LENGTH (new_encl_type) <= TYPE_LENGTH (value_enclosing_type (val))) 
-    {
-      val->enclosing_type = new_encl_type;
-      return val;
-    }
-  else
-    {
-      struct value *new_val;
-      struct value *prev;
-      
-      new_val = (struct value *) xrealloc (val, sizeof (struct value) + TYPE_LENGTH (new_encl_type));
-
-      new_val->enclosing_type = new_encl_type;
- 
-      /* We have to make sure this ends up in the same place in the value
-	 chain as the original copy, so it's clean-up behavior is the same. 
-	 If the value has been released, this is a waste of time, but there
-	 is no way to tell that in advance, so... */
-      
-      if (val != all_values) 
-	{
-	  for (prev = all_values; prev != NULL; prev = prev->next)
-	    {
-	      if (prev->next == val) 
-		{
-		  prev->next = new_val;
-		  break;
-		}
-	    }
-	}
-      
-      return new_val;
-    }
+  if (TYPE_LENGTH (new_encl_type) > TYPE_LENGTH (value_enclosing_type (val))) 
+    val->content =
+      (union value_aligner *) xrealloc (val->content,
+                                        TYPE_LENGTH (new_encl_type));
+
+  val->enclosing_type = new_encl_type;
+  return val;
 }
 
 /* Given a value ARG1 (offset by OFFSET bytes)
@@ -1388,18 +1406,22 @@ value_primitive_field (struct value *arg
       /* This field is actually a base subobject, so preserve the
          entire object's contents for later references to virtual
          bases, etc.  */
-      v = allocate_value (value_enclosing_type (arg1));
-      v->type = type;
 
       /* Lazy register values with offsets are not supported.  */
       if (VALUE_LVAL (arg1) == lval_register && value_lazy (arg1))
 	value_fetch_lazy (arg1);
 
       if (value_lazy (arg1))
-	set_value_lazy (v, 1);
+        {
+          v = allocate_value_lazy (value_enclosing_type (arg1));
+        }
       else
-	memcpy (value_contents_all_raw (v), value_contents_all_raw (arg1),
-		TYPE_LENGTH (value_enclosing_type (arg1)));
+        {
+          v = allocate_value (value_enclosing_type (arg1));
+          memcpy (value_contents_all_raw (v), value_contents_all_raw (arg1),
+                  TYPE_LENGTH (value_enclosing_type (arg1)));
+        }
+      v->type = type;
       v->offset = value_offset (arg1);
       v->embedded_offset = (offset + value_embedded_offset (arg1)
 			    + TYPE_FIELD_BITPOS (arg_type, fieldno) / 8);
@@ -1408,18 +1430,22 @@ value_primitive_field (struct value *arg
     {
       /* Plain old data member */
       offset += TYPE_FIELD_BITPOS (arg_type, fieldno) / 8;
-      v = allocate_value (type);
 
       /* Lazy register values with offsets are not supported.  */
       if (VALUE_LVAL (arg1) == lval_register && value_lazy (arg1))
 	value_fetch_lazy (arg1);
 
       if (value_lazy (arg1))
-	set_value_lazy (v, 1);
+        {
+          v = allocate_value_lazy (type);
+        }
       else
-	memcpy (value_contents_raw (v),
-		value_contents_raw (arg1) + offset,
-		TYPE_LENGTH (type));
+        {
+          v = allocate_value (type);
+          memcpy (value_contents_raw (v),
+                  value_contents_raw (arg1) + offset,
+                  TYPE_LENGTH (type));
+        }
       v->offset = (value_offset (arg1) + offset
 		   + value_embedded_offset (arg1));
     }
Index: value.h
===================================================================
--- value.h	(revision 138922)
+++ value.h	(working copy)
@@ -314,6 +314,8 @@ extern struct value *locate_var_value (s
 				       struct frame_info *frame);
 
 extern struct value *allocate_value (struct type *type);
+extern struct value *allocate_value_lazy (struct type *type);
+extern void allocate_value_content (struct value *value);
 
 extern struct value *allocate_repeat_value (struct type *type, int count);
 
@@ -500,7 +502,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 *val);
 
 extern void free_all_values (void);
 
Index: valops.c
===================================================================
--- valops.c	(revision 138922)
+++ valops.c	(working copy)
@@ -608,11 +608,10 @@ value_at_lazy (struct type *type, CORE_A
   if (TYPE_CODE (check_typedef (type)) == TYPE_CODE_VOID)
     error (_("Attempt to dereference a generic pointer."));
 
-  val = allocate_value (type);
+  val = allocate_value_lazy (type);
 
   VALUE_LVAL (val) = lval_memory;
   VALUE_ADDRESS (val) = addr;
-  set_value_lazy (val, 1);
 
   return val;
 }
@@ -634,6 +633,8 @@ value_at_lazy (struct type *type, CORE_A
 int
 value_fetch_lazy (struct value *val)
 {
+  gdb_assert (value_lazy (val));
+  allocate_value_content (val);
   if (VALUE_LVAL (val) == lval_memory)
     {
       CORE_ADDR addr = VALUE_ADDRESS (val) + value_offset (val);
@@ -1535,7 +1536,7 @@ search_struct_field (char *name, struct 
       if (BASETYPE_VIA_VIRTUAL (type, i))
 	{
 	  int boffset;
-	  struct value *v2 = allocate_value (basetype);
+	  struct value *v2;
 
 	  boffset = baseclass_offset (type, i,
 				      value_contents (arg1) + offset,
@@ -1553,6 +1554,7 @@ search_struct_field (char *name, struct 
 	    {
 	      CORE_ADDR base_addr;
 
+              v2  = allocate_value (basetype);
 	      base_addr = 
 		VALUE_ADDRESS (arg1) + value_offset (arg1) + boffset;
 	      if (target_read_memory (base_addr, 
@@ -1564,16 +1566,21 @@ search_struct_field (char *name, struct 
 	    }
 	  else
 	    {
+	      if (VALUE_LVAL (arg1) == lval_memory && value_lazy (arg1))
+                {
+                  v2  = allocate_value_lazy (basetype);
+                }
+	      else
+                {
+                  v2  = allocate_value (basetype);
+                  memcpy (value_contents_raw (v2),
+                          value_contents_raw (arg1) + boffset,
+                          TYPE_LENGTH (basetype));
+                }
 	      VALUE_LVAL (v2) = VALUE_LVAL (arg1);
 	      VALUE_ADDRESS (v2) = VALUE_ADDRESS (arg1);
 	      VALUE_FRAME_ID (v2) = VALUE_FRAME_ID (arg1);
 	      set_value_offset (v2, value_offset (arg1) + boffset);
-	      if (VALUE_LVAL (arg1) == lval_memory && value_lazy (arg1))
-		set_value_lazy (v2, 1);
-	      else
-		memcpy (value_contents_raw (v2),
-			value_contents_raw (arg1) + boffset,
-			TYPE_LENGTH (basetype));
 	    }
 
 	  if (found_baseclass)
@@ -2969,13 +2976,17 @@ value_slice (struct value *array, int lo
 				      slice_range_type);
       TYPE_CODE (slice_type) = TYPE_CODE (array_type);
 
-      slice = allocate_value (slice_type);
       if (VALUE_LVAL (array) == lval_memory && value_lazy (array))
-	set_value_lazy (slice, 1);
+        {
+          slice = allocate_value_lazy (slice_type);
+        }
       else
-	memcpy (value_contents_writeable (slice),
-		value_contents (array) + offset,
-		TYPE_LENGTH (slice_type));
+        {
+          slice = allocate_value (slice_type);
+          memcpy (value_contents_writeable (slice),
+                  value_contents (array) + offset,
+                  TYPE_LENGTH (slice_type));
+        }
 
       if (VALUE_LVAL (array) == lval_internalvar)
 	VALUE_LVAL (slice) = lval_internalvar_component;

Re: [RFC] "lazily" allocate the raw content of lazy values

[Tom Tromey]

>>>>> "Jerome" == Jerome Guitton <guitton@adacore.com> writes:

Jerome> Here, an elegant solution would be to manipulate lazy values instead
Jerome> of references
[...]

Very nice background explanation, thanks.

Jerome> +/* Actual contents of the value.  For use of this value; setting it
Jerome> +   uses the stuff defined in struct value.  Target byte-order.  We force it
Jerome> +   to be aligned properly for any possible value.  Note that a value therefore
Jerome> +   extends beyond what is declared here.  */
Jerome> +
Jerome> +union value_aligner
Jerome> +{
Jerome> +  gdb_byte contents[1];
Jerome> +  DOUBLEST force_doublest_align;
Jerome> +  LONGEST force_longest_align;
Jerome> +  CORE_ADDR force_core_addr_align;
Jerome> +  void *force_pointer_align;  
Jerome> +};

The purpose of this union is just to force alignment of the 'contents'
field, so that we can stuff any data type into it without worrying.
Once we're no longer using the struct hack to store the bits, there is
no need for this union.

Jerome> +  union value_aligner *content;

I think you might as well make this "gdb_byte *contents" now.

I think it would be nice to have a test case showing the Ada problem.
This would help prevent mistakes if we ever want to change the value
representation again.  Would this be hard to do?


After that I only have nits.

Jerome> +void
Jerome> +value_free (struct value *val)
Jerome> +{
Jerome> +  if (val && val->content)

No need to check val->content, xfree does that.

Jerome>  struct value *
Jerome>  value_change_enclosing_type (struct value *val, struct type *new_encl_type)

You were right, value_change_enclosing_type is much simpler now :-)

Jerome>        if (value_lazy (arg1))
Jerome> -	set_value_lazy (v, 1);
Jerome> +        {
Jerome> +          v = allocate_value_lazy (value_enclosing_type (arg1));
Jerome> +        }

In the GNU style, a single statement like this does not have braces
around it.  There are a few instances of this in the patch.

Tom

Re: [RFC] "lazily" allocate the raw content of lazy values

[Tom Tromey]

>>>>> "Tom" == Tom Tromey <tromey@redhat.com> writes:

Tom> I think it would be nice to have a test case showing the Ada problem.
Tom> This would help prevent mistakes if we ever want to change the value
Tom> representation again.  Would this be hard to do?

... and there it was, in your next message.  Sorry about that.

Tom

Re: [RFA] "lazily" allocate the raw content of lazy values

[Jerome Guitton]

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

Tom Tromey (tromey@redhat.com):

> Jerome> Here, an elegant solution would be to manipulate lazy values instead
> Jerome> of references
> [...]
> 
> Very nice background explanation, thanks.

You are welcome ;)

Thank you for your comments. I have taken them into account, with a few
additional changes:
* rename allocate_value_content to allocate_value_contents, for consistancy;
* remove the test "value->contents == NULL" in value_contents_raw and
value_contents_all_raw, as this is done in allocate_value_contents.

The new version of the patch is in attachment, re-tested against the
testsuite. Promoting this message to RFA now. OK to apply?

Thanks,
Jerome


2008-11-25  Jerome Guitton  <guitton@adacore.com>

	* value.h (allocate_value_lazy): New function declaration.
	(value_free): Remove macro, make it a function.
	* value.c (value): Move actual content outside of the memory space
        of the struct; add a pointer to this actual content.
	(allocate_value_lazy, allocate_value_contents): New function.
	(allocate_value): Reimplement using these two new functions.
	(value_contents_raw, value_contents_all_raw): If no memory
	has been allocated yet for the actual content, allocate it.
	(value_contents_all): Resync with struct value's changes.
	(value_free): New function.
	(value_copy, value_primitive_field): Use new function
	allocate_value_lazy to allocate lazy values.
	(value_change_enclosing_type): Resync with struct value's changes.
	As the value is not reallocated, remove the special handling for
	the value chain (now obsolete).
	* valops.c (value_at_lazy): Use new function allocate_value_lazy.
	(value_fetch_lazy): Allocate value content. Use allocate_value_lazy
	to allocate lazy values.
	(value_slice): Use allocate_value_lazy to allocate lazy values.


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

Index: value.c
===================================================================
--- value.c	(revision 139003)
+++ value.c	(working copy)
@@ -163,21 +163,9 @@ struct value
   /* If value is a variable, is it initialized or not.  */
   int initialized;
 
-  /* Actual contents of the value.  For use of this value; setting it
-     uses the stuff above.  Not valid if lazy is nonzero.  Target
-     byte-order.  We force it to be aligned properly for any possible
-     value.  Note that a value therefore extends beyond what is
-     declared here.  */
-  union
-  {
-    gdb_byte contents[1];
-    DOUBLEST force_doublest_align;
-    LONGEST force_longest_align;
-    CORE_ADDR force_core_addr_align;
-    void *force_pointer_align;
-  } aligner;
-  /* Do not add any new members here -- contents above will trash
-     them.  */
+  /* Actual contents of the value. Target byte-order. NULL or not valid if
+     lazy is nonzero. */
+  gdb_byte *contents;
 };
 
 /* Prototypes for local functions. */
@@ -213,15 +201,18 @@ static int value_history_count;	/* Abs n
 
 static struct value *all_values;
 
-/* Allocate a  value  that has the correct length for type TYPE.  */
+/* Allocate a lazy value for type TYPE.  Its actual content is
+   "lazily" allocated too: the content field of the return value is
+   NULL; it will be allocated when it is fetched from the target.  */
 
 struct value *
-allocate_value (struct type *type)
+allocate_value_lazy (struct type *type)
 {
   struct value *val;
   struct type *atype = check_typedef (type);
 
-  val = (struct value *) xzalloc (sizeof (struct value) + TYPE_LENGTH (atype));
+  val = (struct value *) xzalloc (sizeof (struct value));
+  val->contents = NULL;
   val->next = all_values;
   all_values = val;
   val->type = type;
@@ -233,7 +224,7 @@ allocate_value (struct type *type)
   val->bitpos = 0;
   val->bitsize = 0;
   VALUE_REGNUM (val) = -1;
-  val->lazy = 0;
+  val->lazy = 1;
   val->optimized_out = 0;
   val->embedded_offset = 0;
   val->pointed_to_offset = 0;
@@ -242,6 +233,28 @@ allocate_value (struct type *type)
   return val;
 }
 
+/* Allocate the contents of the value VAL if it has not been allocated yet.  */
+
+void
+allocate_value_contents (struct value *val)
+{
+  if (!val->contents)
+    {
+      val->contents = (gdb_byte *) xzalloc (TYPE_LENGTH (val->enclosing_type));
+    }
+}
+
+/* Allocate a  value  that has the correct length for type TYPE.  */
+
+struct value *
+allocate_value (struct type *type)
+{
+  struct value *val = allocate_value_lazy (type);
+  allocate_value_contents (val);
+  val->lazy = 0;
+  return val;
+}
+
 /* Allocate a  value  that has the correct length
    for COUNT repetitions of type TYPE.  */
 
@@ -340,13 +353,15 @@ set_value_bitsize (struct value *value, 
 gdb_byte *
 value_contents_raw (struct value *value)
 {
-  return value->aligner.contents + value->embedded_offset;
+  allocate_value_contents (value);
+  return value->contents + value->embedded_offset;
 }
 
 gdb_byte *
 value_contents_all_raw (struct value *value)
 {
-  return value->aligner.contents;
+  allocate_value_contents (value);
+  return value->contents;
 }
 
 struct type *
@@ -360,7 +375,7 @@ value_contents_all (struct value *value)
 {
   if (value->lazy)
     value_fetch_lazy (value);
-  return value->aligner.contents;
+  return value->contents;
 }
 
 int
@@ -495,6 +510,14 @@ value_mark (void)
   return all_values;
 }
 
+void
+value_free (struct value *val)
+{
+  if (val)
+    xfree (val->contents);
+  xfree (val);
+}
+
 /* Free all values allocated since MARK was obtained by value_mark
    (except for those released).  */
 void
@@ -579,7 +602,12 @@ struct value *
 value_copy (struct value *arg)
 {
   struct type *encl_type = value_enclosing_type (arg);
-  struct value *val = allocate_value (encl_type);
+  struct value *val;
+
+  if (value_lazy (arg))
+    val = allocate_value_lazy (encl_type);
+  else
+    val = allocate_value (encl_type);
   val->type = arg->type;
   VALUE_LVAL (val) = VALUE_LVAL (arg);
   val->location = arg->location;
@@ -1319,39 +1347,12 @@ value_static_field (struct type *type, i
 struct value *
 value_change_enclosing_type (struct value *val, struct type *new_encl_type)
 {
-  if (TYPE_LENGTH (new_encl_type) <= TYPE_LENGTH (value_enclosing_type (val))) 
-    {
-      val->enclosing_type = new_encl_type;
-      return val;
-    }
-  else
-    {
-      struct value *new_val;
-      struct value *prev;
-      
-      new_val = (struct value *) xrealloc (val, sizeof (struct value) + TYPE_LENGTH (new_encl_type));
-
-      new_val->enclosing_type = new_encl_type;
- 
-      /* We have to make sure this ends up in the same place in the value
-	 chain as the original copy, so it's clean-up behavior is the same. 
-	 If the value has been released, this is a waste of time, but there
-	 is no way to tell that in advance, so... */
-      
-      if (val != all_values) 
-	{
-	  for (prev = all_values; prev != NULL; prev = prev->next)
-	    {
-	      if (prev->next == val) 
-		{
-		  prev->next = new_val;
-		  break;
-		}
-	    }
-	}
-      
-      return new_val;
-    }
+  if (TYPE_LENGTH (new_encl_type) > TYPE_LENGTH (value_enclosing_type (val))) 
+    val->contents =
+      (gdb_byte *) xrealloc (val->contents, TYPE_LENGTH (new_encl_type));
+
+  val->enclosing_type = new_encl_type;
+  return val;
 }
 
 /* Given a value ARG1 (offset by OFFSET bytes)
@@ -1388,18 +1389,20 @@ value_primitive_field (struct value *arg
       /* This field is actually a base subobject, so preserve the
          entire object's contents for later references to virtual
          bases, etc.  */
-      v = allocate_value (value_enclosing_type (arg1));
-      v->type = type;
 
       /* Lazy register values with offsets are not supported.  */
       if (VALUE_LVAL (arg1) == lval_register && value_lazy (arg1))
 	value_fetch_lazy (arg1);
 
       if (value_lazy (arg1))
-	set_value_lazy (v, 1);
+        v = allocate_value_lazy (value_enclosing_type (arg1));
       else
-	memcpy (value_contents_all_raw (v), value_contents_all_raw (arg1),
-		TYPE_LENGTH (value_enclosing_type (arg1)));
+        {
+          v = allocate_value (value_enclosing_type (arg1));
+          memcpy (value_contents_all_raw (v), value_contents_all_raw (arg1),
+                  TYPE_LENGTH (value_enclosing_type (arg1)));
+        }
+      v->type = type;
       v->offset = value_offset (arg1);
       v->embedded_offset = (offset + value_embedded_offset (arg1)
 			    + TYPE_FIELD_BITPOS (arg_type, fieldno) / 8);
@@ -1408,18 +1411,20 @@ value_primitive_field (struct value *arg
     {
       /* Plain old data member */
       offset += TYPE_FIELD_BITPOS (arg_type, fieldno) / 8;
-      v = allocate_value (type);
 
       /* Lazy register values with offsets are not supported.  */
       if (VALUE_LVAL (arg1) == lval_register && value_lazy (arg1))
 	value_fetch_lazy (arg1);
 
       if (value_lazy (arg1))
-	set_value_lazy (v, 1);
+        v = allocate_value_lazy (type);
       else
-	memcpy (value_contents_raw (v),
-		value_contents_raw (arg1) + offset,
-		TYPE_LENGTH (type));
+        {
+          v = allocate_value (type);
+          memcpy (value_contents_raw (v),
+                  value_contents_raw (arg1) + offset,
+                  TYPE_LENGTH (type));
+        }
       v->offset = (value_offset (arg1) + offset
 		   + value_embedded_offset (arg1));
     }
Index: valops.c
===================================================================
--- valops.c	(revision 139003)
+++ valops.c	(working copy)
@@ -608,11 +608,10 @@ value_at_lazy (struct type *type, CORE_A
   if (TYPE_CODE (check_typedef (type)) == TYPE_CODE_VOID)
     error (_("Attempt to dereference a generic pointer."));
 
-  val = allocate_value (type);
+  val = allocate_value_lazy (type);
 
   VALUE_LVAL (val) = lval_memory;
   VALUE_ADDRESS (val) = addr;
-  set_value_lazy (val, 1);
 
   return val;
 }
@@ -634,6 +633,8 @@ value_at_lazy (struct type *type, CORE_A
 int
 value_fetch_lazy (struct value *val)
 {
+  gdb_assert (value_lazy (val));
+  allocate_value_contents (val);
   if (VALUE_LVAL (val) == lval_memory)
     {
       CORE_ADDR addr = VALUE_ADDRESS (val) + value_offset (val);
@@ -1535,7 +1536,7 @@ search_struct_field (char *name, struct 
       if (BASETYPE_VIA_VIRTUAL (type, i))
 	{
 	  int boffset;
-	  struct value *v2 = allocate_value (basetype);
+	  struct value *v2;
 
 	  boffset = baseclass_offset (type, i,
 				      value_contents (arg1) + offset,
@@ -1553,6 +1554,7 @@ search_struct_field (char *name, struct 
 	    {
 	      CORE_ADDR base_addr;
 
+              v2  = allocate_value (basetype);
 	      base_addr = 
 		VALUE_ADDRESS (arg1) + value_offset (arg1) + boffset;
 	      if (target_read_memory (base_addr, 
@@ -1564,16 +1566,19 @@ search_struct_field (char *name, struct 
 	    }
 	  else
 	    {
+	      if (VALUE_LVAL (arg1) == lval_memory && value_lazy (arg1))
+                v2  = allocate_value_lazy (basetype);
+	      else
+                {
+                  v2  = allocate_value (basetype);
+                  memcpy (value_contents_raw (v2),
+                          value_contents_raw (arg1) + boffset,
+                          TYPE_LENGTH (basetype));
+                }
 	      VALUE_LVAL (v2) = VALUE_LVAL (arg1);
 	      VALUE_ADDRESS (v2) = VALUE_ADDRESS (arg1);
 	      VALUE_FRAME_ID (v2) = VALUE_FRAME_ID (arg1);
 	      set_value_offset (v2, value_offset (arg1) + boffset);
-	      if (VALUE_LVAL (arg1) == lval_memory && value_lazy (arg1))
-		set_value_lazy (v2, 1);
-	      else
-		memcpy (value_contents_raw (v2),
-			value_contents_raw (arg1) + boffset,
-			TYPE_LENGTH (basetype));
 	    }
 
 	  if (found_baseclass)
@@ -2969,13 +2974,15 @@ value_slice (struct value *array, int lo
 				      slice_range_type);
       TYPE_CODE (slice_type) = TYPE_CODE (array_type);
 
-      slice = allocate_value (slice_type);
       if (VALUE_LVAL (array) == lval_memory && value_lazy (array))
-	set_value_lazy (slice, 1);
+        slice = allocate_value_lazy (slice_type);
       else
-	memcpy (value_contents_writeable (slice),
-		value_contents (array) + offset,
-		TYPE_LENGTH (slice_type));
+        {
+          slice = allocate_value (slice_type);
+          memcpy (value_contents_writeable (slice),
+                  value_contents (array) + offset,
+                  TYPE_LENGTH (slice_type));
+        }
 
       if (VALUE_LVAL (array) == lval_internalvar)
 	VALUE_LVAL (slice) = lval_internalvar_component;
Index: value.h
===================================================================
--- value.h	(revision 139003)
+++ value.h	(working copy)
@@ -314,6 +314,8 @@ extern struct value *locate_var_value (s
 				       struct frame_info *frame);
 
 extern struct value *allocate_value (struct type *type);
+extern struct value *allocate_value_lazy (struct type *type);
+extern void allocate_value_contents (struct value *value);
 
 extern struct value *allocate_repeat_value (struct type *type, int count);
 
@@ -500,7 +502,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 *val);
 
 extern void free_all_values (void);
 

Re: [RFA] "lazily" allocate the raw content of lazy values

[Tom Tromey]

>>>>> "Jerome" == Jerome Guitton <guitton@adacore.com> writes:

Jerome> The new version of the patch is in attachment, re-tested against the
Jerome> testsuite. Promoting this message to RFA now. OK to apply?

Just FYI -- I can't approve or reject it.
However, it seems sound to me.

A couple more formatting nits, sorry about that.

Jerome> +  /* Actual contents of the value. Target byte-order. NULL or not valid if
Jerome> +     lazy is nonzero. */
Jerome> +  gdb_byte *contents;

GNU style requires 2 spaces after a ".".

Jerome> +void
Jerome> +allocate_value_contents (struct value *val)
Jerome> +{
Jerome> +  if (!val->contents)
Jerome> +    {
Jerome> +      val->contents = (gdb_byte *) xzalloc (TYPE_LENGTH (val->enclosing_type));
Jerome> +    }

No need for the braces here.

Tom

Re: [RFA] "lazily" allocate the raw content of lazy values

[Jerome Guitton]

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

Tom Tromey (tromey@redhat.com):

> Just FYI -- I can't approve or reject it.
> However, it seems sound to me.
> 
> A couple more formatting nits, sorry about that.

Now fixed, thanks again! New patch in attachment.


2008-11-25  Jerome Guitton  <guitton@adacore.com>

	* value.h (allocate_value_lazy): New function declaration.
	(value_free): Remove macro, make it a function.
	* value.c (value): Move actual content outside of the memory space
        of the struct; add a pointer to this actual content.
	(allocate_value_lazy, allocate_value_contents): New function.
	(allocate_value): Reimplement using these two new functions.
	(value_contents_raw, value_contents_all_raw): If no memory
	has been allocated yet for the actual content, allocate it.
	(value_contents_all): Resync with struct value's changes.
	(value_free): New function.
	(value_copy, value_primitive_field): Use new function
	allocate_value_lazy to allocate lazy values.
	(value_change_enclosing_type): Resync with struct value's changes.
	As the value is not reallocated, remove the special handling for
	the value chain (now obsolete).
	* valops.c (value_at_lazy): Use new function allocate_value_lazy.
	(value_fetch_lazy): Allocate value content. Use allocate_value_lazy
	to allocate lazy values.
	(value_slice): Use allocate_value_lazy to allocate lazy values.


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

Index: value.c
===================================================================
--- value.c	(revision 139003)
+++ value.c	(working copy)
@@ -163,21 +163,9 @@ struct value
   /* If value is a variable, is it initialized or not.  */
   int initialized;
 
-  /* Actual contents of the value.  For use of this value; setting it
-     uses the stuff above.  Not valid if lazy is nonzero.  Target
-     byte-order.  We force it to be aligned properly for any possible
-     value.  Note that a value therefore extends beyond what is
-     declared here.  */
-  union
-  {
-    gdb_byte contents[1];
-    DOUBLEST force_doublest_align;
-    LONGEST force_longest_align;
-    CORE_ADDR force_core_addr_align;
-    void *force_pointer_align;
-  } aligner;
-  /* Do not add any new members here -- contents above will trash
-     them.  */
+  /* Actual contents of the value. Target byte-order. NULL or not valid if
+     lazy is nonzero.  */
+  gdb_byte *contents;
 };
 
 /* Prototypes for local functions. */
@@ -213,15 +201,18 @@ static int value_history_count;	/* Abs n
 
 static struct value *all_values;
 
-/* Allocate a  value  that has the correct length for type TYPE.  */
+/* Allocate a lazy value for type TYPE.  Its actual content is
+   "lazily" allocated too: the content field of the return value is
+   NULL; it will be allocated when it is fetched from the target.  */
 
 struct value *
-allocate_value (struct type *type)
+allocate_value_lazy (struct type *type)
 {
   struct value *val;
   struct type *atype = check_typedef (type);
 
-  val = (struct value *) xzalloc (sizeof (struct value) + TYPE_LENGTH (atype));
+  val = (struct value *) xzalloc (sizeof (struct value));
+  val->contents = NULL;
   val->next = all_values;
   all_values = val;
   val->type = type;
@@ -233,7 +224,7 @@ allocate_value (struct type *type)
   val->bitpos = 0;
   val->bitsize = 0;
   VALUE_REGNUM (val) = -1;
-  val->lazy = 0;
+  val->lazy = 1;
   val->optimized_out = 0;
   val->embedded_offset = 0;
   val->pointed_to_offset = 0;
@@ -242,6 +233,26 @@ allocate_value (struct type *type)
   return val;
 }
 
+/* Allocate the contents of the value VAL if it has not been allocated yet.  */
+
+void
+allocate_value_contents (struct value *val)
+{
+  if (!val->contents)
+    val->contents = (gdb_byte *) xzalloc (TYPE_LENGTH (val->enclosing_type));
+}
+
+/* Allocate a  value  that has the correct length for type TYPE.  */
+
+struct value *
+allocate_value (struct type *type)
+{
+  struct value *val = allocate_value_lazy (type);
+  allocate_value_contents (val);
+  val->lazy = 0;
+  return val;
+}
+
 /* Allocate a  value  that has the correct length
    for COUNT repetitions of type TYPE.  */
 
@@ -340,13 +351,15 @@ set_value_bitsize (struct value *value, 
 gdb_byte *
 value_contents_raw (struct value *value)
 {
-  return value->aligner.contents + value->embedded_offset;
+  allocate_value_contents (value);
+  return value->contents + value->embedded_offset;
 }
 
 gdb_byte *
 value_contents_all_raw (struct value *value)
 {
-  return value->aligner.contents;
+  allocate_value_contents (value);
+  return value->contents;
 }
 
 struct type *
@@ -360,7 +373,7 @@ value_contents_all (struct value *value)
 {
   if (value->lazy)
     value_fetch_lazy (value);
-  return value->aligner.contents;
+  return value->contents;
 }
 
 int
@@ -495,6 +508,14 @@ value_mark (void)
   return all_values;
 }
 
+void
+value_free (struct value *val)
+{
+  if (val)
+    xfree (val->contents);
+  xfree (val);
+}
+
 /* Free all values allocated since MARK was obtained by value_mark
    (except for those released).  */
 void
@@ -579,7 +600,12 @@ struct value *
 value_copy (struct value *arg)
 {
   struct type *encl_type = value_enclosing_type (arg);
-  struct value *val = allocate_value (encl_type);
+  struct value *val;
+
+  if (value_lazy (arg))
+    val = allocate_value_lazy (encl_type);
+  else
+    val = allocate_value (encl_type);
   val->type = arg->type;
   VALUE_LVAL (val) = VALUE_LVAL (arg);
   val->location = arg->location;
@@ -1319,39 +1345,12 @@ value_static_field (struct type *type, i
 struct value *
 value_change_enclosing_type (struct value *val, struct type *new_encl_type)
 {
-  if (TYPE_LENGTH (new_encl_type) <= TYPE_LENGTH (value_enclosing_type (val))) 
-    {
-      val->enclosing_type = new_encl_type;
-      return val;
-    }
-  else
-    {
-      struct value *new_val;
-      struct value *prev;
-      
-      new_val = (struct value *) xrealloc (val, sizeof (struct value) + TYPE_LENGTH (new_encl_type));
-
-      new_val->enclosing_type = new_encl_type;
- 
-      /* We have to make sure this ends up in the same place in the value
-	 chain as the original copy, so it's clean-up behavior is the same. 
-	 If the value has been released, this is a waste of time, but there
-	 is no way to tell that in advance, so... */
-      
-      if (val != all_values) 
-	{
-	  for (prev = all_values; prev != NULL; prev = prev->next)
-	    {
-	      if (prev->next == val) 
-		{
-		  prev->next = new_val;
-		  break;
-		}
-	    }
-	}
-      
-      return new_val;
-    }
+  if (TYPE_LENGTH (new_encl_type) > TYPE_LENGTH (value_enclosing_type (val))) 
+    val->contents =
+      (gdb_byte *) xrealloc (val->contents, TYPE_LENGTH (new_encl_type));
+
+  val->enclosing_type = new_encl_type;
+  return val;
 }
 
 /* Given a value ARG1 (offset by OFFSET bytes)
@@ -1388,18 +1387,20 @@ value_primitive_field (struct value *arg
       /* This field is actually a base subobject, so preserve the
          entire object's contents for later references to virtual
          bases, etc.  */
-      v = allocate_value (value_enclosing_type (arg1));
-      v->type = type;
 
       /* Lazy register values with offsets are not supported.  */
       if (VALUE_LVAL (arg1) == lval_register && value_lazy (arg1))
 	value_fetch_lazy (arg1);
 
       if (value_lazy (arg1))
-	set_value_lazy (v, 1);
+        v = allocate_value_lazy (value_enclosing_type (arg1));
       else
-	memcpy (value_contents_all_raw (v), value_contents_all_raw (arg1),
-		TYPE_LENGTH (value_enclosing_type (arg1)));
+        {
+          v = allocate_value (value_enclosing_type (arg1));
+          memcpy (value_contents_all_raw (v), value_contents_all_raw (arg1),
+                  TYPE_LENGTH (value_enclosing_type (arg1)));
+        }
+      v->type = type;
       v->offset = value_offset (arg1);
       v->embedded_offset = (offset + value_embedded_offset (arg1)
 			    + TYPE_FIELD_BITPOS (arg_type, fieldno) / 8);
@@ -1408,18 +1409,20 @@ value_primitive_field (struct value *arg
     {
       /* Plain old data member */
       offset += TYPE_FIELD_BITPOS (arg_type, fieldno) / 8;
-      v = allocate_value (type);
 
       /* Lazy register values with offsets are not supported.  */
       if (VALUE_LVAL (arg1) == lval_register && value_lazy (arg1))
 	value_fetch_lazy (arg1);
 
       if (value_lazy (arg1))
-	set_value_lazy (v, 1);
+        v = allocate_value_lazy (type);
       else
-	memcpy (value_contents_raw (v),
-		value_contents_raw (arg1) + offset,
-		TYPE_LENGTH (type));
+        {
+          v = allocate_value (type);
+          memcpy (value_contents_raw (v),
+                  value_contents_raw (arg1) + offset,
+                  TYPE_LENGTH (type));
+        }
       v->offset = (value_offset (arg1) + offset
 		   + value_embedded_offset (arg1));
     }
Index: valops.c
===================================================================
--- valops.c	(revision 139003)
+++ valops.c	(working copy)
@@ -608,11 +608,10 @@ value_at_lazy (struct type *type, CORE_A
   if (TYPE_CODE (check_typedef (type)) == TYPE_CODE_VOID)
     error (_("Attempt to dereference a generic pointer."));
 
-  val = allocate_value (type);
+  val = allocate_value_lazy (type);
 
   VALUE_LVAL (val) = lval_memory;
   VALUE_ADDRESS (val) = addr;
-  set_value_lazy (val, 1);
 
   return val;
 }
@@ -634,6 +633,8 @@ value_at_lazy (struct type *type, CORE_A
 int
 value_fetch_lazy (struct value *val)
 {
+  gdb_assert (value_lazy (val));
+  allocate_value_contents (val);
   if (VALUE_LVAL (val) == lval_memory)
     {
       CORE_ADDR addr = VALUE_ADDRESS (val) + value_offset (val);
@@ -1535,7 +1536,7 @@ search_struct_field (char *name, struct 
       if (BASETYPE_VIA_VIRTUAL (type, i))
 	{
 	  int boffset;
-	  struct value *v2 = allocate_value (basetype);
+	  struct value *v2;
 
 	  boffset = baseclass_offset (type, i,
 				      value_contents (arg1) + offset,
@@ -1553,6 +1554,7 @@ search_struct_field (char *name, struct 
 	    {
 	      CORE_ADDR base_addr;
 
+              v2  = allocate_value (basetype);
 	      base_addr = 
 		VALUE_ADDRESS (arg1) + value_offset (arg1) + boffset;
 	      if (target_read_memory (base_addr, 
@@ -1564,16 +1566,19 @@ search_struct_field (char *name, struct 
 	    }
 	  else
 	    {
+	      if (VALUE_LVAL (arg1) == lval_memory && value_lazy (arg1))
+                v2  = allocate_value_lazy (basetype);
+	      else
+                {
+                  v2  = allocate_value (basetype);
+                  memcpy (value_contents_raw (v2),
+                          value_contents_raw (arg1) + boffset,
+                          TYPE_LENGTH (basetype));
+                }
 	      VALUE_LVAL (v2) = VALUE_LVAL (arg1);
 	      VALUE_ADDRESS (v2) = VALUE_ADDRESS (arg1);
 	      VALUE_FRAME_ID (v2) = VALUE_FRAME_ID (arg1);
 	      set_value_offset (v2, value_offset (arg1) + boffset);
-	      if (VALUE_LVAL (arg1) == lval_memory && value_lazy (arg1))
-		set_value_lazy (v2, 1);
-	      else
-		memcpy (value_contents_raw (v2),
-			value_contents_raw (arg1) + boffset,
-			TYPE_LENGTH (basetype));
 	    }
 
 	  if (found_baseclass)
@@ -2969,13 +2974,15 @@ value_slice (struct value *array, int lo
 				      slice_range_type);
       TYPE_CODE (slice_type) = TYPE_CODE (array_type);
 
-      slice = allocate_value (slice_type);
       if (VALUE_LVAL (array) == lval_memory && value_lazy (array))
-	set_value_lazy (slice, 1);
+        slice = allocate_value_lazy (slice_type);
       else
-	memcpy (value_contents_writeable (slice),
-		value_contents (array) + offset,
-		TYPE_LENGTH (slice_type));
+        {
+          slice = allocate_value (slice_type);
+          memcpy (value_contents_writeable (slice),
+                  value_contents (array) + offset,
+                  TYPE_LENGTH (slice_type));
+        }
 
       if (VALUE_LVAL (array) == lval_internalvar)
 	VALUE_LVAL (slice) = lval_internalvar_component;
Index: value.h
===================================================================
--- value.h	(revision 139003)
+++ value.h	(working copy)
@@ -314,6 +314,8 @@ extern struct value *locate_var_value (s
 				       struct frame_info *frame);
 
 extern struct value *allocate_value (struct type *type);
+extern struct value *allocate_value_lazy (struct type *type);
+extern void allocate_value_contents (struct value *value);
 
 extern struct value *allocate_repeat_value (struct type *type, int count);
 
@@ -500,7 +502,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 *val);
 
 extern void free_all_values (void);
 

Re: [RFA] "lazily" allocate the raw content of lazy values

[Joel Brobecker]

I'll join Jerome in thanking Tom for taking time to review this patch,
and help improving in. Very very much appreciated. Thanks, Tom!

> 2008-11-25  Jerome Guitton  <guitton@adacore.com>
> 
> 	* value.h (allocate_value_lazy): New function declaration.
> 	(value_free): Remove macro, make it a function.
> 	* value.c (value): Move actual content outside of the memory space
>         of the struct; add a pointer to this actual content.
> 	(allocate_value_lazy, allocate_value_contents): New function.
> 	(allocate_value): Reimplement using these two new functions.
> 	(value_contents_raw, value_contents_all_raw): If no memory
> 	has been allocated yet for the actual content, allocate it.
> 	(value_contents_all): Resync with struct value's changes.
> 	(value_free): New function.
> 	(value_copy, value_primitive_field): Use new function
> 	allocate_value_lazy to allocate lazy values.
> 	(value_change_enclosing_type): Resync with struct value's changes.
> 	As the value is not reallocated, remove the special handling for
> 	the value chain (now obsolete).
> 	* valops.c (value_at_lazy): Use new function allocate_value_lazy.
> 	(value_fetch_lazy): Allocate value content. Use allocate_value_lazy
> 	to allocate lazy values.
> 	(value_slice): Use allocate_value_lazy to allocate lazy values.

Pre-approved after the following minor nits are fixed.

Also, we discussed on the phone the possibility of having contents == NULL
being equivalent to lazy (thus allowing us to get rid of lazy). But it
turns out that it makes the value structure a little less convenient
in certain cases (getting the value of each element in an array using
the same value). At least for now, Jerome elected to keep the two
concepts separate, but I think a comment explaining that would be
useful. This can be done as a followup patch so that people get a chance
to see the new comment and make comments.

> +  /* Actual contents of the value. Target byte-order. NULL or not valid if
                                                        ^^ 2 spaces here.

> +/* Allocate a  value  that has the correct length for type TYPE.  */
> +
> +struct value *
> +allocate_value (struct type *type)

In this case, I think it's important to say in the function description
that this function allocates the value contents. To me "has the correct
length for type TYPE" is a little unclear (I realize that this comes
from the original implementation).

>        /* Lazy register values with offsets are not supported.  */
>        if (VALUE_LVAL (arg1) == lval_register && value_lazy (arg1))
>  	value_fetch_lazy (arg1);
>  
>        if (value_lazy (arg1))
> -	set_value_lazy (v, 1);
> +        v = allocate_value_lazy (value_enclosing_type (arg1));

1 tab instead of 8 spaces. (I hate tabs)

> @@ -2969,13 +2974,15 @@ value_slice (struct value *array, int lo
>  				      slice_range_type);
>        TYPE_CODE (slice_type) = TYPE_CODE (array_type);
>  
> -      slice = allocate_value (slice_type);
>        if (VALUE_LVAL (array) == lval_memory && value_lazy (array))
> -	set_value_lazy (slice, 1);
> +        slice = allocate_value_lazy (slice_type);

Same here...

-- 
Joel

Re: [RFA] "lazily" allocate the raw content of lazy values

[Jerome Guitton]

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

Joel Brobecker (brobecker@adacore.com):

> > 2008-11-25  Jerome Guitton  <guitton@adacore.com>
> > 
> > 	* value.h (allocate_value_lazy): New function declaration.
> > 	(value_free): Remove macro, make it a function.
> > 	* value.c (value): Move actual content outside of the memory space
> >         of the struct; add a pointer to this actual content.
> > 	(allocate_value_lazy, allocate_value_contents): New function.
> > 	(allocate_value): Reimplement using these two new functions.
> > 	(value_contents_raw, value_contents_all_raw): If no memory
> > 	has been allocated yet for the actual content, allocate it.
> > 	(value_contents_all): Resync with struct value's changes.
> > 	(value_free): New function.
> > 	(value_copy, value_primitive_field): Use new function
> > 	allocate_value_lazy to allocate lazy values.
> > 	(value_change_enclosing_type): Resync with struct value's changes.
> > 	As the value is not reallocated, remove the special handling for
> > 	the value chain (now obsolete).
> > 	* valops.c (value_at_lazy): Use new function allocate_value_lazy.
> > 	(value_fetch_lazy): Allocate value content. Use allocate_value_lazy
> > 	to allocate lazy values.
> > 	(value_slice): Use allocate_value_lazy to allocate lazy values.
> 
> Pre-approved after the following minor nits are fixed.

Fixed in the patch in attachment. OK to apply?


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

Index: value.c
===================================================================
--- value.c	(revision 139003)
+++ value.c	(working copy)
@@ -163,21 +163,9 @@ struct value
   /* If value is a variable, is it initialized or not.  */
   int initialized;
 
-  /* Actual contents of the value.  For use of this value; setting it
-     uses the stuff above.  Not valid if lazy is nonzero.  Target
-     byte-order.  We force it to be aligned properly for any possible
-     value.  Note that a value therefore extends beyond what is
-     declared here.  */
-  union
-  {
-    gdb_byte contents[1];
-    DOUBLEST force_doublest_align;
-    LONGEST force_longest_align;
-    CORE_ADDR force_core_addr_align;
-    void *force_pointer_align;
-  } aligner;
-  /* Do not add any new members here -- contents above will trash
-     them.  */
+  /* Actual contents of the value.  Target byte-order.  NULL or not
+     valid if lazy is nonzero.  */
+  gdb_byte *contents;
 };
 
 /* Prototypes for local functions. */
@@ -213,15 +201,18 @@ static int value_history_count;	/* Abs n
 
 static struct value *all_values;
 
-/* Allocate a  value  that has the correct length for type TYPE.  */
+/* Allocate a lazy value for type TYPE.  Its actual content is
+   "lazily" allocated too: the content field of the return value is
+   NULL; it will be allocated when it is fetched from the target.  */
 
 struct value *
-allocate_value (struct type *type)
+allocate_value_lazy (struct type *type)
 {
   struct value *val;
   struct type *atype = check_typedef (type);
 
-  val = (struct value *) xzalloc (sizeof (struct value) + TYPE_LENGTH (atype));
+  val = (struct value *) xzalloc (sizeof (struct value));
+  val->contents = NULL;
   val->next = all_values;
   all_values = val;
   val->type = type;
@@ -233,7 +224,7 @@ allocate_value (struct type *type)
   val->bitpos = 0;
   val->bitsize = 0;
   VALUE_REGNUM (val) = -1;
-  val->lazy = 0;
+  val->lazy = 1;
   val->optimized_out = 0;
   val->embedded_offset = 0;
   val->pointed_to_offset = 0;
@@ -242,6 +233,26 @@ allocate_value (struct type *type)
   return val;
 }
 
+/* Allocate the contents of VAL if it has not been allocated yet.  */
+
+void
+allocate_value_contents (struct value *val)
+{
+  if (!val->contents)
+    val->contents = (gdb_byte *) xzalloc (TYPE_LENGTH (val->enclosing_type));
+}
+
+/* Allocate a  value  and its contents for type TYPE.  */
+
+struct value *
+allocate_value (struct type *type)
+{
+  struct value *val = allocate_value_lazy (type);
+  allocate_value_contents (val);
+  val->lazy = 0;
+  return val;
+}
+
 /* Allocate a  value  that has the correct length
    for COUNT repetitions of type TYPE.  */
 
@@ -340,13 +351,15 @@ set_value_bitsize (struct value *value, 
 gdb_byte *
 value_contents_raw (struct value *value)
 {
-  return value->aligner.contents + value->embedded_offset;
+  allocate_value_contents (value);
+  return value->contents + value->embedded_offset;
 }
 
 gdb_byte *
 value_contents_all_raw (struct value *value)
 {
-  return value->aligner.contents;
+  allocate_value_contents (value);
+  return value->contents;
 }
 
 struct type *
@@ -360,7 +373,7 @@ value_contents_all (struct value *value)
 {
   if (value->lazy)
     value_fetch_lazy (value);
-  return value->aligner.contents;
+  return value->contents;
 }
 
 int
@@ -495,6 +508,14 @@ value_mark (void)
   return all_values;
 }
 
+void
+value_free (struct value *val)
+{
+  if (val)
+    xfree (val->contents);
+  xfree (val);
+}
+
 /* Free all values allocated since MARK was obtained by value_mark
    (except for those released).  */
 void
@@ -579,7 +600,12 @@ struct value *
 value_copy (struct value *arg)
 {
   struct type *encl_type = value_enclosing_type (arg);
-  struct value *val = allocate_value (encl_type);
+  struct value *val;
+
+  if (value_lazy (arg))
+    val = allocate_value_lazy (encl_type);
+  else
+    val = allocate_value (encl_type);
   val->type = arg->type;
   VALUE_LVAL (val) = VALUE_LVAL (arg);
   val->location = arg->location;
@@ -1319,39 +1345,12 @@ value_static_field (struct type *type, i
 struct value *
 value_change_enclosing_type (struct value *val, struct type *new_encl_type)
 {
-  if (TYPE_LENGTH (new_encl_type) <= TYPE_LENGTH (value_enclosing_type (val))) 
-    {
-      val->enclosing_type = new_encl_type;
-      return val;
-    }
-  else
-    {
-      struct value *new_val;
-      struct value *prev;
-      
-      new_val = (struct value *) xrealloc (val, sizeof (struct value) + TYPE_LENGTH (new_encl_type));
-
-      new_val->enclosing_type = new_encl_type;
- 
-      /* We have to make sure this ends up in the same place in the value
-	 chain as the original copy, so it's clean-up behavior is the same. 
-	 If the value has been released, this is a waste of time, but there
-	 is no way to tell that in advance, so... */
-      
-      if (val != all_values) 
-	{
-	  for (prev = all_values; prev != NULL; prev = prev->next)
-	    {
-	      if (prev->next == val) 
-		{
-		  prev->next = new_val;
-		  break;
-		}
-	    }
-	}
-      
-      return new_val;
-    }
+  if (TYPE_LENGTH (new_encl_type) > TYPE_LENGTH (value_enclosing_type (val))) 
+    val->contents =
+      (gdb_byte *) xrealloc (val->contents, TYPE_LENGTH (new_encl_type));
+
+  val->enclosing_type = new_encl_type;
+  return val;
 }
 
 /* Given a value ARG1 (offset by OFFSET bytes)
@@ -1388,18 +1387,20 @@ value_primitive_field (struct value *arg
       /* This field is actually a base subobject, so preserve the
          entire object's contents for later references to virtual
          bases, etc.  */
-      v = allocate_value (value_enclosing_type (arg1));
-      v->type = type;
 
       /* Lazy register values with offsets are not supported.  */
       if (VALUE_LVAL (arg1) == lval_register && value_lazy (arg1))
 	value_fetch_lazy (arg1);
 
       if (value_lazy (arg1))
-	set_value_lazy (v, 1);
+	v = allocate_value_lazy (value_enclosing_type (arg1));
       else
-	memcpy (value_contents_all_raw (v), value_contents_all_raw (arg1),
-		TYPE_LENGTH (value_enclosing_type (arg1)));
+	{
+	  v = allocate_value (value_enclosing_type (arg1));
+	  memcpy (value_contents_all_raw (v), value_contents_all_raw (arg1),
+		  TYPE_LENGTH (value_enclosing_type (arg1)));
+	}
+      v->type = type;
       v->offset = value_offset (arg1);
       v->embedded_offset = (offset + value_embedded_offset (arg1)
 			    + TYPE_FIELD_BITPOS (arg_type, fieldno) / 8);
@@ -1408,18 +1409,20 @@ value_primitive_field (struct value *arg
     {
       /* Plain old data member */
       offset += TYPE_FIELD_BITPOS (arg_type, fieldno) / 8;
-      v = allocate_value (type);
 
       /* Lazy register values with offsets are not supported.  */
       if (VALUE_LVAL (arg1) == lval_register && value_lazy (arg1))
 	value_fetch_lazy (arg1);
 
       if (value_lazy (arg1))
-	set_value_lazy (v, 1);
+	v = allocate_value_lazy (type);
       else
-	memcpy (value_contents_raw (v),
-		value_contents_raw (arg1) + offset,
-		TYPE_LENGTH (type));
+	{
+	  v = allocate_value (type);
+	  memcpy (value_contents_raw (v),
+		  value_contents_raw (arg1) + offset,
+		  TYPE_LENGTH (type));
+	}
       v->offset = (value_offset (arg1) + offset
 		   + value_embedded_offset (arg1));
     }
Index: valops.c
===================================================================
--- valops.c	(revision 139003)
+++ valops.c	(working copy)
@@ -608,11 +608,10 @@ value_at_lazy (struct type *type, CORE_A
   if (TYPE_CODE (check_typedef (type)) == TYPE_CODE_VOID)
     error (_("Attempt to dereference a generic pointer."));
 
-  val = allocate_value (type);
+  val = allocate_value_lazy (type);
 
   VALUE_LVAL (val) = lval_memory;
   VALUE_ADDRESS (val) = addr;
-  set_value_lazy (val, 1);
 
   return val;
 }
@@ -634,6 +633,8 @@ value_at_lazy (struct type *type, CORE_A
 int
 value_fetch_lazy (struct value *val)
 {
+  gdb_assert (value_lazy (val));
+  allocate_value_contents (val);
   if (VALUE_LVAL (val) == lval_memory)
     {
       CORE_ADDR addr = VALUE_ADDRESS (val) + value_offset (val);
@@ -1535,7 +1536,7 @@ search_struct_field (char *name, struct 
       if (BASETYPE_VIA_VIRTUAL (type, i))
 	{
 	  int boffset;
-	  struct value *v2 = allocate_value (basetype);
+	  struct value *v2;
 
 	  boffset = baseclass_offset (type, i,
 				      value_contents (arg1) + offset,
@@ -1553,6 +1554,7 @@ search_struct_field (char *name, struct 
 	    {
 	      CORE_ADDR base_addr;
 
+	      v2  = allocate_value (basetype);
 	      base_addr = 
 		VALUE_ADDRESS (arg1) + value_offset (arg1) + boffset;
 	      if (target_read_memory (base_addr, 
@@ -1564,16 +1566,19 @@ search_struct_field (char *name, struct 
 	    }
 	  else
 	    {
+	      if (VALUE_LVAL (arg1) == lval_memory && value_lazy (arg1))
+		v2  = allocate_value_lazy (basetype);
+	      else
+		{
+		  v2  = allocate_value (basetype);
+		  memcpy (value_contents_raw (v2),
+			  value_contents_raw (arg1) + boffset,
+			  TYPE_LENGTH (basetype));
+		}
 	      VALUE_LVAL (v2) = VALUE_LVAL (arg1);
 	      VALUE_ADDRESS (v2) = VALUE_ADDRESS (arg1);
 	      VALUE_FRAME_ID (v2) = VALUE_FRAME_ID (arg1);
 	      set_value_offset (v2, value_offset (arg1) + boffset);
-	      if (VALUE_LVAL (arg1) == lval_memory && value_lazy (arg1))
-		set_value_lazy (v2, 1);
-	      else
-		memcpy (value_contents_raw (v2),
-			value_contents_raw (arg1) + boffset,
-			TYPE_LENGTH (basetype));
 	    }
 
 	  if (found_baseclass)
@@ -2969,13 +2974,15 @@ value_slice (struct value *array, int lo
 				      slice_range_type);
       TYPE_CODE (slice_type) = TYPE_CODE (array_type);
 
-      slice = allocate_value (slice_type);
       if (VALUE_LVAL (array) == lval_memory && value_lazy (array))
-	set_value_lazy (slice, 1);
+	slice = allocate_value_lazy (slice_type);
       else
-	memcpy (value_contents_writeable (slice),
-		value_contents (array) + offset,
-		TYPE_LENGTH (slice_type));
+	{
+	  slice = allocate_value (slice_type);
+	  memcpy (value_contents_writeable (slice),
+		  value_contents (array) + offset,
+		  TYPE_LENGTH (slice_type));
+	}
 
       if (VALUE_LVAL (array) == lval_internalvar)
 	VALUE_LVAL (slice) = lval_internalvar_component;
Index: value.h
===================================================================
--- value.h	(revision 139003)
+++ value.h	(working copy)
@@ -314,6 +314,8 @@ extern struct value *locate_var_value (s
 				       struct frame_info *frame);
 
 extern struct value *allocate_value (struct type *type);
+extern struct value *allocate_value_lazy (struct type *type);
+extern void allocate_value_contents (struct value *value);
 
 extern struct value *allocate_repeat_value (struct type *type, int count);
 
@@ -500,7 +502,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 *val);
 
 extern void free_all_values (void);
 

Re: [RFA] "lazily" allocate the raw content of lazy values

[Jerome Guitton]

Joel Brobecker (brobecker@adacore.com):

> > 2008-11-25  Jerome Guitton  <guitton@adacore.com>
> > 
> > 	* value.h (allocate_value_lazy): New function declaration.
> > 	(value_free): Remove macro, make it a function.
> > 	* value.c (value): Move actual content outside of the memory space
> >         of the struct; add a pointer to this actual content.
> > 	(allocate_value_lazy, allocate_value_contents): New function.
> > 	(allocate_value): Reimplement using these two new functions.
> > 	(value_contents_raw, value_contents_all_raw): If no memory
> > 	has been allocated yet for the actual content, allocate it.
> > 	(value_contents_all): Resync with struct value's changes.
> > 	(value_free): New function.
> > 	(value_copy, value_primitive_field): Use new function
> > 	allocate_value_lazy to allocate lazy values.
> > 	(value_change_enclosing_type): Resync with struct value's changes.
> > 	As the value is not reallocated, remove the special handling for
> > 	the value chain (now obsolete).
> > 	* valops.c (value_at_lazy): Use new function allocate_value_lazy.
> > 	(value_fetch_lazy): Allocate value content. Use allocate_value_lazy
> > 	to allocate lazy values.
> > 	(value_slice): Use allocate_value_lazy to allocate lazy values.
> 
> Pre-approved after the following minor nits are fixed.

Minor nits fixed, now commited.
Thank you both,
Jerome