[unavailable values part 1, 01/17] base support for unavailable value contents

[unavailable values part 1, 01/17] base support for unavailable value contents

[Pedro Alves]

This adds the base support for marking chunks of a given
value's contents as unavailable.  It then makes it so that 
it is an error to get at the contents of such values with
value_contents() or value_contents_all(), similarly to how
it is also an error to do the same to optimized out values.
This in turn automaticaly makes it an error to use unavailable
values in expression evaluation.
Trying to print formatted or unformatted scalars (even if they're
part of a compound) is also caught already by this patch, where
we now print <unavailable> in place of "0" or some other garbage.
This was made simple and centralized since the introduction
of val_print_scalar_formatted in
<http://sourceware.org/ml/gdb-patches/2011-01/msg00505.html>
(yay for pieces falling in together!).

Python pretty-printing does not yet know what to do
to unavailable values, so this disables it.  Does not mean
someone can't add it later.

Note that nothing is actually marking chunks of a value's
contents as unavailable yet in this patch, so it's
not testable yet.

We give preference to printing <optimized out> rather
than <unavailable>, since if a value had been optimized out
at compile time, it can never be collected at run-time.

-- 
Pedro Alves

2011-02-07  Pedro Alves  <pedro@codesourcery.com>

	Base support for <unavailable> value contents.

	gdb/
	* value.h (value_bytes_available): Declare.
	(mark_value_bytes_unavailable): Declare.
	* value.c (struct range): New struct.
	(range_s): New typedef.
	(ranges_overlap): New function.
	(ranges_contain_p): New function.
	(ranges_copy): New function.
	(struct value) <unavailable>: New field.
	(value_bytes_available): New function.
	(mark_value_bytes_unavailable): New function.
	(require_not_optimized_out): Constify parameter.
	(require_available): New function.
	(value_contents_all, value_contents): Require all bytes be
	available.
	(value_free): Free `unavailable'.
	(value_copy): Copy `unavailable'.
	* valprint.h (val_print_unavailable): Declare.
	* valprint.c (valprint_check_validity): Rename `offset' parameter
	to `embedded_offset'.  If printing a scalar, check whether the
	value chunk is available.
	(val_print_unavailable): New.
	(val_print_scalar_formatted): Check whether the value is
	available.
	* python/py-prettyprint.c (apply_val_pretty_printer): Refuse
	pretty-printing unavailable values.

---
 gdb/python/py-prettyprint.c |    5 +
 gdb/valprint.c              |   22 +++++-
 gdb/valprint.h              |    2 
 gdb/value.c                 |  156 +++++++++++++++++++++++++++++++++++++++++++-
 gdb/value.h                 |   14 +++
 5 files changed, 194 insertions(+), 5 deletions(-)

Index: src/gdb/value.h
===================================================================
--- src.orig/gdb/value.h	2011-02-07 11:31:36.446706000 +0000
+++ src/gdb/value.h	2011-02-07 11:31:37.986706000 +0000
@@ -360,6 +360,20 @@ extern int value_bits_valid (const struc
 extern int value_bits_synthetic_pointer (const struct value *value,
 					 int offset, int length);
 
+/* Given a value, determine whether the contents bytes starting at
+   OFFSET and extending for LENGTH bytes are available.  This returns
+   zero if all bytes in the given range are available, zero if any
+   byte is unavailable.  */
+
+extern int value_bytes_available (const struct value *value,
+				  int offset, int length);
+
+/* Mark VALUE's content bytes starting at OFFSET and extending for
+   LENGTH bytes as unavailable.  */
+
+extern void mark_value_bytes_unavailable (struct value *value,
+					  int offset, int length);
+
 \f
 
 #include "symtab.h"
Index: src/gdb/value.c
===================================================================
--- src.orig/gdb/value.c	2011-02-07 11:31:36.446706000 +0000
+++ src/gdb/value.c	2011-02-07 11:31:37.996706001 +0000
@@ -63,6 +63,67 @@ struct internal_function
   void *cookie;
 };
 
+/* Defines an [OFFSET, OFFSET + LENGTH) range.  */
+
+struct range
+{
+  /* Lowest offset in the range.  */
+  int offset;
+
+  /* Length of the range.  */
+  int length;
+};
+
+typedef struct range range_s;
+
+DEF_VEC_O(range_s);
+
+/* Returns true if the ranges defined by [offset1, offset1+len1) and
+   [offset2, offset2+len2) overlap.  */
+
+static int
+ranges_overlap (int offset1, int len1,
+		int offset2, int len2)
+{
+  ULONGEST h, l;
+
+  l = max (offset1, offset2);
+  h = min (offset1 + len1, offset2 + len2);
+  return (l < h);
+}
+
+/* Returns true if RANGES contains any range that overlaps [OFFSET,
+   OFFSET+LENGTH).  */
+
+static int
+ranges_contain_p (VEC(range_s) *ranges, int offset, int length)
+{
+  int i;
+  range_s *r;
+
+  for (i = 0; VEC_iterate (range_s, ranges, i, r); i++)
+    if (ranges_overlap (r->offset, r->length,
+			offset, length))
+      return 1;
+
+  return 0;
+}
+
+/* Returns a deep copy of RANGES.  */
+
+static VEC(range_s) *
+ranges_copy (VEC(range_s) *ranges)
+{
+  int i;
+  range_s *r;
+  VEC(range_s) *copy = NULL;
+
+  for (i = 0; VEC_iterate (range_s, ranges, i, r); i++)
+    VEC_safe_push (range_s, copy, r);
+
+  return copy;
+}
+
 static struct cmd_list_element *functionlist;
 
 struct value
@@ -206,6 +267,11 @@ struct value
      valid if lazy is nonzero.  */
   gdb_byte *contents;
 
+  /* Unavailable ranges in CONTENTS.  We mark unavailable ranges,
+     rather than available, since the common and default case is for a
+     value to be available.  This is filled in at value read time.  */
+  VEC(range_s) *unavailable;
+
   /* The number of references to this value.  When a value is created,
      the value chain holds a reference, so REFERENCE_COUNT is 1.  If
      release_value is called, this value is removed from the chain but
@@ -214,6 +280,83 @@ struct value
   int reference_count;
 };
 
+int
+value_bytes_available (const struct value *value, int offset, int length)
+{
+  gdb_assert (!value->lazy);
+
+  return !ranges_contain_p (value->unavailable, offset, length);
+}
+
+void
+mark_value_bytes_unavailable (struct value *value, int offset, int length)
+{
+  struct range *r;
+  int i;
+
+  /* Insert the range sorted.  If there's overlap or the new range
+     would be contiguous with an existing range, merge.  */
+  for (i = 0; VEC_iterate (range_s, value->unavailable, i, r); i++)
+    {
+      if (ranges_overlap (r->offset, r->length, offset, length))
+	{
+	  ULONGEST l = min (r->offset, offset);
+	  ULONGEST h = max (r->offset + r->length, offset + length);
+
+	  r->offset = l;
+	  r->length = h - l;
+	  break;
+	}
+      else if (offset == r->offset + r->length)
+	{
+	  r->length += length;
+	  break;
+	}
+      else if (offset < r->offset)
+	{
+	  struct range *nr;
+
+	  nr = VEC_safe_insert (range_s, value->unavailable, i, NULL);
+	  nr->offset = offset;
+	  nr->length = length;
+	  break;
+	}
+    }
+
+  /* Check whether the ranges following the one we've just added can
+     be folded in.  */
+  if (i < VEC_length (range_s, value->unavailable))
+    {
+      /* Get the range we just touched.  */
+      struct range *t = VEC_index (range_s, value->unavailable, i);
+      int removed = 0;
+
+      for (; VEC_iterate (range_s, value->unavailable, i, r); i++)
+	if (r->offset <= t->offset + t->length)
+	  {
+	    ULONGEST l, h;
+
+	    l = min (t->offset, r->offset);
+	    h = max (t->offset + t->length, r->offset + r->length);
+
+	    t->offset = l;
+	    t->length = h - l;
+
+	    removed++;
+	  }
+
+      if (removed != 0)
+	VEC_truncate (range_s, value->unavailable,
+		      VEC_length (range_s, value->unavailable) - removed);
+    }
+  else
+    {
+      r = VEC_safe_push (range_s, value->unavailable, NULL);
+      r->offset = offset;
+      r->length = length;
+    }
+}
+
 /* Prototypes for local functions.  */
 
 static void show_values (char *, int);
@@ -420,12 +563,19 @@ value_enclosing_type (struct value *valu
 }
 
 static void
-require_not_optimized_out (struct value *value)
+require_not_optimized_out (const struct value *value)
 {
   if (value->optimized_out)
     error (_("value has been optimized out"));
 }
 
+static void
+require_available (const struct value *value)
+{
+  if (!VEC_empty (range_s, value->unavailable))
+    error (_("value is not available"));
+}
+
 const gdb_byte *
 value_contents_for_printing (struct value *value)
 {
@@ -446,6 +596,7 @@ value_contents_all (struct value *value)
 {
   const gdb_byte *result = value_contents_for_printing (value);
   require_not_optimized_out (value);
+  require_available (value);
   return result;
 }
 
@@ -478,6 +629,7 @@ value_contents (struct value *value)
 {
   const gdb_byte *result = value_contents_writeable (value);
   require_not_optimized_out (value);
+  require_available (value);
   return result;
 }
 
@@ -703,6 +855,7 @@ value_free (struct value *val)
 	}
 
       xfree (val->contents);
+      VEC_free (range_s, val->unavailable);
     }
   xfree (val);
 }
@@ -833,6 +986,7 @@ value_copy (struct value *arg)
 	      TYPE_LENGTH (value_enclosing_type (arg)));
 
     }
+  val->unavailable = ranges_copy (arg->unavailable);
   val->parent = arg->parent;
   if (val->parent)
     value_incref (val->parent);
Index: src/gdb/valprint.h
===================================================================
--- src.orig/gdb/valprint.h	2011-02-07 11:31:36.446706000 +0000
+++ src/gdb/valprint.h	2011-02-07 11:31:37.996706001 +0000
@@ -154,4 +154,6 @@ int read_string (CORE_ADDR addr, int len
 
 extern void val_print_optimized_out (struct ui_file *stream);
 
+extern void val_print_unavailable (struct ui_file *stream);
+
 #endif
Index: src/gdb/valprint.c
===================================================================
--- src.orig/gdb/valprint.c	2011-02-07 11:31:36.446706000 +0000
+++ src/gdb/valprint.c	2011-02-07 11:31:37.996706001 +0000
@@ -260,7 +260,7 @@ scalar_type_p (struct type *type)
 static int
 valprint_check_validity (struct ui_file *stream,
 			 struct type *type,
-			 int offset,
+			 int embedded_offset,
 			 const struct value *val)
 {
   CHECK_TYPEDEF (type);
@@ -269,19 +269,25 @@ valprint_check_validity (struct ui_file
       && TYPE_CODE (type) != TYPE_CODE_STRUCT
       && TYPE_CODE (type) != TYPE_CODE_ARRAY)
     {
-      if (! value_bits_valid (val, TARGET_CHAR_BIT * offset,
-			      TARGET_CHAR_BIT * TYPE_LENGTH (type)))
+      if (!value_bits_valid (val, TARGET_CHAR_BIT * embedded_offset,
+			     TARGET_CHAR_BIT * TYPE_LENGTH (type)))
 	{
 	  val_print_optimized_out (stream);
 	  return 0;
 	}
 
-      if (value_bits_synthetic_pointer (val, TARGET_CHAR_BIT * offset,
+      if (value_bits_synthetic_pointer (val, TARGET_CHAR_BIT * embedded_offset,
 					TARGET_CHAR_BIT * TYPE_LENGTH (type)))
 	{
 	  fputs_filtered (_("<synthetic pointer>"), stream);
 	  return 0;
 	}
+
+      if (!value_bytes_available (val, embedded_offset, TYPE_LENGTH (type)))
+	{
+	  val_print_unavailable (stream);
+	  return 0;
+	}
     }
 
   return 1;
@@ -293,6 +299,12 @@ val_print_optimized_out (struct ui_file
   fprintf_filtered (stream, _("<optimized out>"));
 }
 
+void
+val_print_unavailable (struct ui_file *stream)
+{
+  fprintf_filtered (stream, _("<unavailable>"));
+}
+
 /* Print using the given LANGUAGE the data of type TYPE located at
    VALADDR + EMBEDDED_OFFSET (within GDB), which came from the
    inferior at address ADDRESS + EMBEDDED_OFFSET, onto stdio stream
@@ -560,6 +572,8 @@ val_print_scalar_formatted (struct type
   if (!value_bits_valid (val, TARGET_CHAR_BIT * embedded_offset,
 			      TARGET_CHAR_BIT * TYPE_LENGTH (type)))
     val_print_optimized_out (stream);
+  else if (!value_bytes_available (val, embedded_offset, TYPE_LENGTH (type)))
+    val_print_unavailable (stream);
   else
     print_scalar_formatted (valaddr + embedded_offset, type,
 			    options, size, stream);
Index: src/gdb/python/py-prettyprint.c
===================================================================
--- src.orig/gdb/python/py-prettyprint.c	2011-02-07 11:31:36.446706000 +0000
+++ src/gdb/python/py-prettyprint.c	2011-02-07 11:31:37.996706001 +0000
@@ -690,6 +690,11 @@ apply_val_pretty_printer (struct type *t
   struct cleanup *cleanups;
   int result = 0;
   enum string_repr_result print_result;
+
+  /* No pretty-printer support for unavailable values.  */
+  if (!value_bytes_available (val, embedded_offset, TYPE_LENGTH (type)))
+    return 0;
+
   cleanups = ensure_python_env (gdbarch, language);
 
   /* Instantiate the printer.  */

Re: [unavailable values part 1, 01/17] base support for unavailable value contents

[Tom Tromey]

>>>>> "Pedro" == Pedro Alves <pedro@codesourcery.com> writes:

Pedro> Python pretty-printing does not yet know what to do
Pedro> to unavailable values, so this disables it.  Does not mean
Pedro> someone can't add it later.

Could you file a bug report for this?

I think it may be fine to just leave out this change, but I am not
positive.

Pedro> +/* Returns true if RANGES contains any range that overlaps [OFFSET,
Pedro> +   OFFSET+LENGTH).  */
Pedro> +
Pedro> +static int
Pedro> +ranges_contain_p (VEC(range_s) *ranges, int offset, int length)
Pedro> +{
Pedro> +  int i;
Pedro> +  range_s *r;
Pedro> +
Pedro> +  for (i = 0; VEC_iterate (range_s, ranges, i, r); i++)
Pedro> +    if (ranges_overlap (r->offset, r->length,
Pedro> +			offset, length))
Pedro> +      return 1;
Pedro> +
Pedro> +  return 0;
Pedro> +}
Pedro> +

It seems to me that since the ranges are sorted by starting address, and
coalesced overlap, then you could use a binary search here, aka
VEC_lower_bound.  It may not be worth doing though.

Pedro> +static VEC(range_s) *
Pedro> +ranges_copy (VEC(range_s) *ranges)
Pedro> +{
Pedro> +  int i;
Pedro> +  range_s *r;
Pedro> +  VEC(range_s) *copy = NULL;
Pedro> +
Pedro> +  for (i = 0; VEC_iterate (range_s, ranges, i, r); i++)
Pedro> +    VEC_safe_push (range_s, copy, r);

It is slightly more memory-efficient to grow the copy vector to the
right size and then quick_push the elements.

Pedro> +  /* Insert the range sorted.  If there's overlap or the new range
Pedro> +     would be contiguous with an existing range, merge.  */

This could also use a binary search.  Again, maybe not worth the effort.

Tom

Re: [unavailable values part 1, 01/17] base support for unavailable value contents

[Joel Brobecker]

> +/* Given a value, determine whether the contents bytes starting at
> +   OFFSET and extending for LENGTH bytes are available.  This returns
> +   zero if all bytes in the given range are available, zero if any
> +   byte is unavailable.  */

s/zero/nonzero/ in the first "zero".

-- 
Joel

Re: [unavailable values part 1, 01/17] base support for unavailable value contents

[Pedro Alves]

On Thursday 10 February 2011 15:47:29, Tom Tromey wrote:
> >>>>> "Pedro" == Pedro Alves <pedro@codesourcery.com> writes:
> 
> Pedro> Python pretty-printing does not yet know what to do
> Pedro> to unavailable values, so this disables it.  Does not mean
> Pedro> someone can't add it later.
> 
> Could you file a bug report for this?

Sure, will do once this is in.

> I think it may be fine to just leave out this change, but I am not
> positive.

Not sure either.  That'll mean the printer will be
working with / showing bogus value contents (usually 0, but
not garanteed) and have no way to know it and let the user know
(as today).  Supposedly, someone else on the frontend side
thought this was a good idea, since I had the written requirement
that pretty printed varobjs shall only be supported if the entire
object correspond to is collected.  If we disable pretty-printing,
we'll at least print using the raw format, showing the <unavailable>s.
That said, I don't insist on this.  It's just "temporary" until someone
implements the python pretty-printer support.   Just let me
know what you prefer.

> Pedro> +/* Returns true if RANGES contains any range that overlaps [OFFSET,
> Pedro> +   OFFSET+LENGTH).  */
> Pedro> +
> Pedro> +static int
> Pedro> +ranges_contain_p (VEC(range_s) *ranges, int offset, int length)
> Pedro> +{
> Pedro> +  int i;
> Pedro> +  range_s *r;
> Pedro> +
> Pedro> +  for (i = 0; VEC_iterate (range_s, ranges, i, r); i++)
> Pedro> +    if (ranges_overlap (r->offset, r->length,
> Pedro> +			offset, length))
> Pedro> +      return 1;
> Pedro> +
> Pedro> +  return 0;
> Pedro> +}
> Pedro> +
> 
> It seems to me that since the ranges are sorted by starting address, and
> coalesced overlap, then you could use a binary search here, aka
> VEC_lower_bound.  It may not be worth doing though.

Yeah, I don't expect to have more than a handful of ranges to work with.
But in any case, I've done that change.  Below's my current patch.
WDYT?

> 
> Pedro> +static VEC(range_s) *
> Pedro> +ranges_copy (VEC(range_s) *ranges)
> Pedro> +{
> Pedro> +  int i;
> Pedro> +  range_s *r;
> Pedro> +  VEC(range_s) *copy = NULL;
> Pedro> +
> Pedro> +  for (i = 0; VEC_iterate (range_s, ranges, i, r); i++)
> Pedro> +    VEC_safe_push (range_s, copy, r);
> 
> It is slightly more memory-efficient to grow the copy vector to the
> right size and then quick_push the elements.

Indeed, and there's an even better way that I missed
completely: "VEC_copy".  :-)  Thanks for pointing this out.

> 
> Pedro> +  /* Insert the range sorted.  If there's overlap or the new range
> Pedro> +     would be contiguous with an existing range, merge.  */
> 
> This could also use a binary search.  Again, maybe not worth the effort.

Also done.  I've added some "pictures" in the comments, but I'm
not sure if that's overboard.

The new tests still pass cleanly with the series fully applied on
top of this.

-- 
Pedro Alves

Index: src/gdb/value.h
===================================================================
--- src.orig/gdb/value.h	2011-02-10 18:21:33.747075995 +0000
+++ src/gdb/value.h	2011-02-10 18:42:12.037075997 +0000
@@ -360,6 +360,20 @@ extern int value_bits_valid (const struc
 extern int value_bits_synthetic_pointer (const struct value *value,
 					 int offset, int length);
 
+/* Given a value, determine whether the contents bytes starting at
+   OFFSET and extending for LENGTH bytes are available.  This returns
+   nonzero if all bytes in the given range are available, zero if any
+   byte is unavailable.  */
+
+extern int value_bytes_available (const struct value *value,
+				  int offset, int length);
+
+/* Mark VALUE's content bytes starting at OFFSET and extending for
+   LENGTH bytes as unavailable.  */
+
+extern void mark_value_bytes_unavailable (struct value *value,
+					  int offset, int length);
+
 \f
 
 #include "symtab.h"
Index: src/gdb/value.c
===================================================================
--- src.orig/gdb/value.c	2011-02-10 18:21:33.747075995 +0000
+++ src/gdb/value.c	2011-02-10 18:42:13.497075998 +0000
@@ -63,6 +63,110 @@ struct internal_function
   void *cookie;
 };
 
+/* Defines an [OFFSET, OFFSET + LENGTH) range.  */
+
+struct range
+{
+  /* Lowest offset in the range.  */
+  int offset;
+
+  /* Length of the range.  */
+  int length;
+};
+
+typedef struct range range_s;
+
+DEF_VEC_O(range_s);
+
+/* Returns true if the ranges defined by [offset1, offset1+len1) and
+   [offset2, offset2+len2) overlap.  */
+
+static int
+ranges_overlap (int offset1, int len1,
+		int offset2, int len2)
+{
+  ULONGEST h, l;
+
+  l = max (offset1, offset2);
+  h = min (offset1 + len1, offset2 + len2);
+  return (l < h);
+}
+
+/* Returns true if the first argument is strictly less than the
+   second, useful for VEC_lower_bound.  We keep ranges sorted by
+   offset and coalesce overlapping and contiguous ranges, so this just
+   compares the starting offset.  */
+
+static int
+range_lessthan (const range_s *r1, const range_s *r2)
+{
+  return r1->offset < r2->offset;
+}
+
+/* Returns true if RANGES contains any range that overlaps [OFFSET,
+   OFFSET+LENGTH).  */
+
+static int
+ranges_contain_p (VEC(range_s) *ranges, int offset, int length)
+{
+  range_s what;
+  int i;
+
+  what.offset = offset;
+  what.length = length;
+
+  /* We keep ranges sorted by offset and coalesce overlapping and
+     contiguous ranges, so to check if a range list contains a given
+     range, we can do a binary search for the position the given range
+     would be inserted if we only considered the starting OFFSET of
+     ranges.  We call that position I.  Since we also have LENGTH to
+     care for (this is a range afterall), we need to check if the
+     _previous_ range overlaps the I range.  E.g.,
+
+         R
+         |---|
+       |---|    |---|  |------| ... |--|
+       0        1      2            N
+
+       I=1
+
+     In the case above, the binary search would return `I=1', meaning,
+     this OFFSET should be inserted at position 1, and the current
+     position 1 should be pushed further (and before 2).  But, `0'
+     overlaps with R.
+
+     Then we need to check if the I range overlaps the I range itself.
+     E.g.,
+
+              R
+              |---|
+       |---|    |---|  |-------| ... |--|
+       0        1      2             N
+
+       I=1
+  */
+
+  i = VEC_lower_bound (range_s, ranges, &what, range_lessthan);
+
+  if (i > 0)
+    {
+      struct range *bef = VEC_index (range_s, ranges, i - 1);
+
+      if (ranges_overlap (bef->offset, bef->length, offset, length))
+	return 1;
+    }
+
+  if (i < VEC_length (range_s, ranges))
+    {
+      struct range *r = VEC_index (range_s, ranges, i);
+
+      if (ranges_overlap (r->offset, r->length, offset, length))
+	return 1;
+    }
+
+  return 0;
+}
+
 static struct cmd_list_element *functionlist;
 
 struct value
@@ -206,6 +310,11 @@ struct value
      valid if lazy is nonzero.  */
   gdb_byte *contents;
 
+  /* Unavailable ranges in CONTENTS.  We mark unavailable ranges,
+     rather than available, since the common and default case is for a
+     value to be available.  This is filled in at value read time.  */
+  VEC(range_s) *unavailable;
+
   /* The number of references to this value.  When a value is created,
      the value chain holds a reference, so REFERENCE_COUNT is 1.  If
      release_value is called, this value is removed from the chain but
@@ -214,6 +323,179 @@ struct value
   int reference_count;
 };
 
+int
+value_bytes_available (const struct value *value, int offset, int length)
+{
+  gdb_assert (!value->lazy);
+
+  return !ranges_contain_p (value->unavailable, offset, length);
+}
+
+void
+mark_value_bytes_unavailable (struct value *value, int offset, int length)
+{
+  range_s newr;
+  int i;
+
+  /* Insert the range sorted.  If there's overlap or the new range
+     would be contiguous with an existing range, merge.  */
+
+  newr.offset = offset;
+  newr.length = length;
+
+  /* Do a binary search for the position the given range would be
+     inserted if we only considered the starting OFFSET of ranges.
+     Call that position I.  Since we also have LENGTH to care for
+     (this is a range afterall), we need to check if the _previous_
+     range overlaps the I range.  E.g., calling R the new range:
+
+       #1 - overlaps with previous
+
+	   R
+	   |-...-|
+	 |---|     |---|  |------| ... |--|
+	 0         1      2            N
+
+	 I=1
+
+     In the case #1 above, the binary search would return `I=1',
+     meaning, this OFFSET should be inserted at position 1, and the
+     current position 1 should be pushed further (and become 2).  But,
+     note that `0' overlaps with R, so we want to merge them.
+
+     A similar consideration needs to be taken if the new range would
+     be contiguous with the previous range:
+
+       #2 - contiguous with previous
+
+	    R
+	    |-...-|
+	 |--|       |---|  |------| ... |--|
+	 0          1      2            N
+
+	 I=1
+
+     If there's no overlap with the previous range, as in:
+
+       #3 - not overlapping and not contiguous
+
+	       R
+	       |-...-|
+	  |--|         |---|  |------| ... |--|
+	  0            1      2            N
+
+	 I=0
+
+     or if I is 0:
+
+       #4 - R is the range with lowest offset
+
+	  R
+	 |-...-|
+	         |--|       |---|  |------| ... |--|
+	         0          1      2            N
+
+	 I=0
+
+     ... we just push the new range to the head.
+
+     All the 4 cases above need to consider that the new range may
+     also overlap several of the ranges that follow, or that R may be
+     contiguous with the following range, and merge.  E.g.,
+
+       #5 - overlapping following ranges
+
+	  R
+	 |------------------------|
+	         |--|       |---|  |------| ... |--|
+	         0          1      2            N
+
+	 I=0
+
+       or:
+
+	    R
+	    |-------|
+	 |--|       |---|  |------| ... |--|
+	 0          1      2            N
+
+	 I=1
+
+  */
+
+  i = VEC_lower_bound (range_s, value->unavailable, &newr, range_lessthan);
+  if (i > 0)
+    {
+      struct range *bef = VEC_index (range_s, value->unavailable, i - i);
+
+      if (ranges_overlap (bef->offset, bef->length, offset, length))
+	{
+	  /* #1 */
+	  ULONGEST l = min (bef->offset, offset);
+	  ULONGEST h = max (bef->offset + bef->length, offset + length);
+
+	  bef->offset = l;
+	  bef->length = h - l;
+	  i--;
+	}
+      else if (offset == bef->offset + bef->length)
+	{
+	  /* #2 */
+	  bef->length += length;
+	  i--;
+	}
+      else
+	{
+	  /* #3 */
+	  VEC_safe_insert (range_s, value->unavailable, i, &newr);
+	}
+    }
+  else
+    {
+      /* #4 */
+      VEC_safe_insert (range_s, value->unavailable, i, &newr);
+    }
+
+  /* Check whether the ranges following the one we've just added or
+     touched can be folded in (#5 above).  */
+  if (i + 1 < VEC_length (range_s, value->unavailable))
+    {
+      struct range *t;
+      struct range *r;
+      int removed = 0;
+      int next = i + 1;
+
+      /* Get the range we just touched.  */
+      t = VEC_index (range_s, value->unavailable, i);
+      removed = 0;
+
+      i = next;
+      for (; VEC_iterate (range_s, value->unavailable, i, r); i++)
+	if (r->offset <= t->offset + t->length)
+	  {
+	    ULONGEST l, h;
+
+	    l = min (t->offset, r->offset);
+	    h = max (t->offset + t->length, r->offset + r->length);
+
+	    t->offset = l;
+	    t->length = h - l;
+
+	    removed++;
+	  }
+	else
+	  {
+	    /* If we couldn't merge this one, we won't be able to
+	       merge following ones either, since the ranges are
+	       always sorted by OFFSET.  */
+	    break;
+	  }
+
+      if (removed != 0)
+	VEC_block_remove (range_s, value->unavailable, next, removed);
+    }
+}
+
 /* Prototypes for local functions.  */
 
 static void show_values (char *, int);
@@ -420,12 +702,19 @@ value_enclosing_type (struct value *valu
 }
 
 static void
-require_not_optimized_out (struct value *value)
+require_not_optimized_out (const struct value *value)
 {
   if (value->optimized_out)
     error (_("value has been optimized out"));
 }
 
+static void
+require_available (const struct value *value)
+{
+  if (!VEC_empty (range_s, value->unavailable))
+    error (_("value is not available"));
+}
+
 const gdb_byte *
 value_contents_for_printing (struct value *value)
 {
@@ -446,6 +735,7 @@ value_contents_all (struct value *value)
 {
   const gdb_byte *result = value_contents_for_printing (value);
   require_not_optimized_out (value);
+  require_available (value);
   return result;
 }
 
@@ -478,6 +768,7 @@ value_contents (struct value *value)
 {
   const gdb_byte *result = value_contents_writeable (value);
   require_not_optimized_out (value);
+  require_available (value);
   return result;
 }
 
@@ -703,6 +994,7 @@ value_free (struct value *val)
 	}
 
       xfree (val->contents);
+      VEC_free (range_s, val->unavailable);
     }
   xfree (val);
 }
@@ -833,6 +1125,7 @@ value_copy (struct value *arg)
 	      TYPE_LENGTH (value_enclosing_type (arg)));
 
     }
+  val->unavailable = VEC_copy (range_s, arg->unavailable);
   val->parent = arg->parent;
   if (val->parent)
     value_incref (val->parent);
Index: src/gdb/valprint.h
===================================================================
--- src.orig/gdb/valprint.h	2011-02-10 18:21:33.747075995 +0000
+++ src/gdb/valprint.h	2011-02-10 18:42:12.037075997 +0000
@@ -154,4 +154,6 @@ int read_string (CORE_ADDR addr, int len
 
 extern void val_print_optimized_out (struct ui_file *stream);
 
+extern void val_print_unavailable (struct ui_file *stream);
+
 #endif
Index: src/gdb/valprint.c
===================================================================
--- src.orig/gdb/valprint.c	2011-02-10 18:21:33.747075995 +0000
+++ src/gdb/valprint.c	2011-02-10 18:42:12.037075997 +0000
@@ -260,7 +260,7 @@ scalar_type_p (struct type *type)
 static int
 valprint_check_validity (struct ui_file *stream,
 			 struct type *type,
-			 int offset,
+			 int embedded_offset,
 			 const struct value *val)
 {
   CHECK_TYPEDEF (type);
@@ -269,19 +269,25 @@ valprint_check_validity (struct ui_file
       && TYPE_CODE (type) != TYPE_CODE_STRUCT
       && TYPE_CODE (type) != TYPE_CODE_ARRAY)
     {
-      if (! value_bits_valid (val, TARGET_CHAR_BIT * offset,
-			      TARGET_CHAR_BIT * TYPE_LENGTH (type)))
+      if (!value_bits_valid (val, TARGET_CHAR_BIT * embedded_offset,
+			     TARGET_CHAR_BIT * TYPE_LENGTH (type)))
 	{
 	  val_print_optimized_out (stream);
 	  return 0;
 	}
 
-      if (value_bits_synthetic_pointer (val, TARGET_CHAR_BIT * offset,
+      if (value_bits_synthetic_pointer (val, TARGET_CHAR_BIT * embedded_offset,
 					TARGET_CHAR_BIT * TYPE_LENGTH (type)))
 	{
 	  fputs_filtered (_("<synthetic pointer>"), stream);
 	  return 0;
 	}
+
+      if (!value_bytes_available (val, embedded_offset, TYPE_LENGTH (type)))
+	{
+	  val_print_unavailable (stream);
+	  return 0;
+	}
     }
 
   return 1;
@@ -293,6 +299,12 @@ val_print_optimized_out (struct ui_file
   fprintf_filtered (stream, _("<optimized out>"));
 }
 
+void
+val_print_unavailable (struct ui_file *stream)
+{
+  fprintf_filtered (stream, _("<unavailable>"));
+}
+
 /* Print using the given LANGUAGE the data of type TYPE located at
    VALADDR + EMBEDDED_OFFSET (within GDB), which came from the
    inferior at address ADDRESS + EMBEDDED_OFFSET, onto stdio stream
@@ -560,6 +572,8 @@ val_print_scalar_formatted (struct type
   if (!value_bits_valid (val, TARGET_CHAR_BIT * embedded_offset,
 			      TARGET_CHAR_BIT * TYPE_LENGTH (type)))
     val_print_optimized_out (stream);
+  else if (!value_bytes_available (val, embedded_offset, TYPE_LENGTH (type)))
+    val_print_unavailable (stream);
   else
     print_scalar_formatted (valaddr + embedded_offset, type,
 			    options, size, stream);
Index: src/gdb/python/py-prettyprint.c
===================================================================
--- src.orig/gdb/python/py-prettyprint.c	2011-02-10 18:21:33.747075995 +0000
+++ src/gdb/python/py-prettyprint.c	2011-02-10 18:42:12.037075997 +0000
@@ -690,6 +690,11 @@ apply_val_pretty_printer (struct type *t
   struct cleanup *cleanups;
   int result = 0;
   enum string_repr_result print_result;
+
+  /* No pretty-printer support for unavailable values.  */
+  if (!value_bytes_available (val, embedded_offset, TYPE_LENGTH (type)))
+    return 0;
+
   cleanups = ensure_python_env (gdbarch, language);
 
   /* Instantiate the printer.  */

Re: [unavailable values part 1, 01/17] base support for unavailable value contents

[Pedro Alves]

On Thursday 10 February 2011 04:17:31, Joel Brobecker wrote:
> > +/* Given a value, determine whether the contents bytes starting at
> > +   OFFSET and extending for LENGTH bytes are available.  This returns
> > +   zero if all bytes in the given range are available, zero if any
> > +   byte is unavailable.  */
> 
> s/zero/nonzero/ in the first "zero".

Whoops.  Thanks!

-- 
Pedro Alves

Re: [unavailable values part 1, 01/17] base support for unavailable value contents

[Pedro Alves]

On Thursday 10 February 2011 18:46:38, Pedro Alves wrote:
> +       #3 - not overlapping and not contiguous
> +
> +              R
> +              |-...-|
> +         |--|         |---|  |------| ... |--|
> +         0            1      2            N
> +
> +        I=0
> +
> +     or if I is 0:
> +
> +       #4 - R is the range with lowest offset
> +
> +         R
> +        |-...-|
> +                |--|       |---|  |------| ... |--|
> +                0          1      2            N
> +
> +        I=0
> +
> +     ... we just push the new range to the head.

Sigh, these comments are wrong/misleading...  Fixed below.
Sorry about that.

-- 
Pedro Alves

Index: src/gdb/value.h
===================================================================
--- src.orig/gdb/value.h	2011-02-10 18:21:33.747075995 +0000
+++ src/gdb/value.h	2011-02-10 19:28:12.827076001 +0000
@@ -360,6 +360,20 @@ extern int value_bits_valid (const struc
 extern int value_bits_synthetic_pointer (const struct value *value,
 					 int offset, int length);
 
+/* Given a value, determine whether the contents bytes starting at
+   OFFSET and extending for LENGTH bytes are available.  This returns
+   nonzero if all bytes in the given range are available, zero if any
+   byte is unavailable.  */
+
+extern int value_bytes_available (const struct value *value,
+				  int offset, int length);
+
+/* Mark VALUE's content bytes starting at OFFSET and extending for
+   LENGTH bytes as unavailable.  */
+
+extern void mark_value_bytes_unavailable (struct value *value,
+					  int offset, int length);
+
 \f
 
 #include "symtab.h"
Index: src/gdb/value.c
===================================================================
--- src.orig/gdb/value.c	2011-02-10 18:21:33.747075995 +0000
+++ src/gdb/value.c	2011-02-10 19:28:34.607075995 +0000
@@ -63,6 +63,110 @@ struct internal_function
   void *cookie;
 };
 
+/* Defines an [OFFSET, OFFSET + LENGTH) range.  */
+
+struct range
+{
+  /* Lowest offset in the range.  */
+  int offset;
+
+  /* Length of the range.  */
+  int length;
+};
+
+typedef struct range range_s;
+
+DEF_VEC_O(range_s);
+
+/* Returns true if the ranges defined by [offset1, offset1+len1) and
+   [offset2, offset2+len2) overlap.  */
+
+static int
+ranges_overlap (int offset1, int len1,
+		int offset2, int len2)
+{
+  ULONGEST h, l;
+
+  l = max (offset1, offset2);
+  h = min (offset1 + len1, offset2 + len2);
+  return (l < h);
+}
+
+/* Returns true if the first argument is strictly less than the
+   second, useful for VEC_lower_bound.  We keep ranges sorted by
+   offset and coalesce overlapping and contiguous ranges, so this just
+   compares the starting offset.  */
+
+static int
+range_lessthan (const range_s *r1, const range_s *r2)
+{
+  return r1->offset < r2->offset;
+}
+
+/* Returns true if RANGES contains any range that overlaps [OFFSET,
+   OFFSET+LENGTH).  */
+
+static int
+ranges_contain_p (VEC(range_s) *ranges, int offset, int length)
+{
+  range_s what;
+  int i;
+
+  what.offset = offset;
+  what.length = length;
+
+  /* We keep ranges sorted by offset and coalesce overlapping and
+     contiguous ranges, so to check if a range list contains a given
+     range, we can do a binary search for the position the given range
+     would be inserted if we only considered the starting OFFSET of
+     ranges.  We call that position I.  Since we also have LENGTH to
+     care for (this is a range afterall), we need to check if the
+     _previous_ range overlaps the I range.  E.g.,
+
+         R
+         |---|
+       |---|    |---|  |------| ... |--|
+       0        1      2            N
+
+       I=1
+
+     In the case above, the binary search would return `I=1', meaning,
+     this OFFSET should be inserted at position 1, and the current
+     position 1 should be pushed further (and before 2).  But, `0'
+     overlaps with R.
+
+     Then we need to check if the I range overlaps the I range itself.
+     E.g.,
+
+              R
+              |---|
+       |---|    |---|  |-------| ... |--|
+       0        1      2             N
+
+       I=1
+  */
+
+  i = VEC_lower_bound (range_s, ranges, &what, range_lessthan);
+
+  if (i > 0)
+    {
+      struct range *bef = VEC_index (range_s, ranges, i - 1);
+
+      if (ranges_overlap (bef->offset, bef->length, offset, length))
+	return 1;
+    }
+
+  if (i < VEC_length (range_s, ranges))
+    {
+      struct range *r = VEC_index (range_s, ranges, i);
+
+      if (ranges_overlap (r->offset, r->length, offset, length))
+	return 1;
+    }
+
+  return 0;
+}
+
 static struct cmd_list_element *functionlist;
 
 struct value
@@ -206,6 +310,11 @@ struct value
      valid if lazy is nonzero.  */
   gdb_byte *contents;
 
+  /* Unavailable ranges in CONTENTS.  We mark unavailable ranges,
+     rather than available, since the common and default case is for a
+     value to be available.  This is filled in at value read time.  */
+  VEC(range_s) *unavailable;
+
   /* The number of references to this value.  When a value is created,
      the value chain holds a reference, so REFERENCE_COUNT is 1.  If
      release_value is called, this value is removed from the chain but
@@ -214,6 +323,179 @@ struct value
   int reference_count;
 };
 
+int
+value_bytes_available (const struct value *value, int offset, int length)
+{
+  gdb_assert (!value->lazy);
+
+  return !ranges_contain_p (value->unavailable, offset, length);
+}
+
+void
+mark_value_bytes_unavailable (struct value *value, int offset, int length)
+{
+  range_s newr;
+  int i;
+
+  /* Insert the range sorted.  If there's overlap or the new range
+     would be contiguous with an existing range, merge.  */
+
+  newr.offset = offset;
+  newr.length = length;
+
+  /* Do a binary search for the position the given range would be
+     inserted if we only considered the starting OFFSET of ranges.
+     Call that position I.  Since we also have LENGTH to care for
+     (this is a range afterall), we need to check if the _previous_
+     range overlaps the I range.  E.g., calling R the new range:
+
+       #1 - overlaps with previous
+
+	   R
+	   |-...-|
+	 |---|     |---|  |------| ... |--|
+	 0         1      2            N
+
+	 I=1
+
+     In the case #1 above, the binary search would return `I=1',
+     meaning, this OFFSET should be inserted at position 1, and the
+     current position 1 should be pushed further (and become 2).  But,
+     note that `0' overlaps with R, so we want to merge them.
+
+     A similar consideration needs to be taken if the new range would
+     be contiguous with the previous range:
+
+       #2 - contiguous with previous
+
+	    R
+	    |-...-|
+	 |--|       |---|  |------| ... |--|
+	 0          1      2            N
+
+	 I=1
+
+     If there's no overlap with the previous range, as in:
+
+       #3 - not overlapping and not contiguous
+
+	       R
+	       |-...-|
+	  |--|         |---|  |------| ... |--|
+	  0            1      2            N
+
+	 I=1
+
+     or if I is 0:
+
+       #4 - R is the range with lowest offset
+
+	  R
+	 |-...-|
+	         |--|       |---|  |------| ... |--|
+	         0          1      2            N
+
+	 I=0
+
+     ... we just push the new range to I.
+
+     All the 4 cases above need to consider that the new range may
+     also overlap several of the ranges that follow, or that R may be
+     contiguous with the following range, and merge.  E.g.,
+
+       #5 - overlapping following ranges
+
+	  R
+	 |------------------------|
+	         |--|       |---|  |------| ... |--|
+	         0          1      2            N
+
+	 I=0
+
+       or:
+
+	    R
+	    |-------|
+	 |--|       |---|  |------| ... |--|
+	 0          1      2            N
+
+	 I=1
+
+  */
+
+  i = VEC_lower_bound (range_s, value->unavailable, &newr, range_lessthan);
+  if (i > 0)
+    {
+      struct range *bef = VEC_index (range_s, value->unavailable, i - i);
+
+      if (ranges_overlap (bef->offset, bef->length, offset, length))
+	{
+	  /* #1 */
+	  ULONGEST l = min (bef->offset, offset);
+	  ULONGEST h = max (bef->offset + bef->length, offset + length);
+
+	  bef->offset = l;
+	  bef->length = h - l;
+	  i--;
+	}
+      else if (offset == bef->offset + bef->length)
+	{
+	  /* #2 */
+	  bef->length += length;
+	  i--;
+	}
+      else
+	{
+	  /* #3 */
+	  VEC_safe_insert (range_s, value->unavailable, i, &newr);
+	}
+    }
+  else
+    {
+      /* #4 */
+      VEC_safe_insert (range_s, value->unavailable, i, &newr);
+    }
+
+  /* Check whether the ranges following the one we've just added or
+     touched can be folded in (#5 above).  */
+  if (i + 1 < VEC_length (range_s, value->unavailable))
+    {
+      struct range *t;
+      struct range *r;
+      int removed = 0;
+      int next = i + 1;
+
+      /* Get the range we just touched.  */
+      t = VEC_index (range_s, value->unavailable, i);
+      removed = 0;
+
+      i = next;
+      for (; VEC_iterate (range_s, value->unavailable, i, r); i++)
+	if (r->offset <= t->offset + t->length)
+	  {
+	    ULONGEST l, h;
+
+	    l = min (t->offset, r->offset);
+	    h = max (t->offset + t->length, r->offset + r->length);
+
+	    t->offset = l;
+	    t->length = h - l;
+
+	    removed++;
+	  }
+	else
+	  {
+	    /* If we couldn't merge this one, we won't be able to
+	       merge following ones either, since the ranges are
+	       always sorted by OFFSET.  */
+	    break;
+	  }
+
+      if (removed != 0)
+	VEC_block_remove (range_s, value->unavailable, next, removed);
+    }
+}
+
 /* Prototypes for local functions.  */
 
 static void show_values (char *, int);
@@ -420,12 +702,19 @@ value_enclosing_type (struct value *valu
 }
 
 static void
-require_not_optimized_out (struct value *value)
+require_not_optimized_out (const struct value *value)
 {
   if (value->optimized_out)
     error (_("value has been optimized out"));
 }
 
+static void
+require_available (const struct value *value)
+{
+  if (!VEC_empty (range_s, value->unavailable))
+    error (_("value is not available"));
+}
+
 const gdb_byte *
 value_contents_for_printing (struct value *value)
 {
@@ -446,6 +735,7 @@ value_contents_all (struct value *value)
 {
   const gdb_byte *result = value_contents_for_printing (value);
   require_not_optimized_out (value);
+  require_available (value);
   return result;
 }
 
@@ -478,6 +768,7 @@ value_contents (struct value *value)
 {
   const gdb_byte *result = value_contents_writeable (value);
   require_not_optimized_out (value);
+  require_available (value);
   return result;
 }
 
@@ -703,6 +994,7 @@ value_free (struct value *val)
 	}
 
       xfree (val->contents);
+      VEC_free (range_s, val->unavailable);
     }
   xfree (val);
 }
@@ -833,6 +1125,7 @@ value_copy (struct value *arg)
 	      TYPE_LENGTH (value_enclosing_type (arg)));
 
     }
+  val->unavailable = VEC_copy (range_s, arg->unavailable);
   val->parent = arg->parent;
   if (val->parent)
     value_incref (val->parent);
Index: src/gdb/valprint.h
===================================================================
--- src.orig/gdb/valprint.h	2011-02-10 18:21:33.747075995 +0000
+++ src/gdb/valprint.h	2011-02-10 18:42:12.037075997 +0000
@@ -154,4 +154,6 @@ int read_string (CORE_ADDR addr, int len
 
 extern void val_print_optimized_out (struct ui_file *stream);
 
+extern void val_print_unavailable (struct ui_file *stream);
+
 #endif
Index: src/gdb/valprint.c
===================================================================
--- src.orig/gdb/valprint.c	2011-02-10 18:21:33.747075995 +0000
+++ src/gdb/valprint.c	2011-02-10 19:28:12.757075997 +0000
@@ -260,7 +260,7 @@ scalar_type_p (struct type *type)
 static int
 valprint_check_validity (struct ui_file *stream,
 			 struct type *type,
-			 int offset,
+			 int embedded_offset,
 			 const struct value *val)
 {
   CHECK_TYPEDEF (type);
@@ -269,19 +269,25 @@ valprint_check_validity (struct ui_file
       && TYPE_CODE (type) != TYPE_CODE_STRUCT
       && TYPE_CODE (type) != TYPE_CODE_ARRAY)
     {
-      if (! value_bits_valid (val, TARGET_CHAR_BIT * offset,
-			      TARGET_CHAR_BIT * TYPE_LENGTH (type)))
+      if (!value_bits_valid (val, TARGET_CHAR_BIT * embedded_offset,
+			     TARGET_CHAR_BIT * TYPE_LENGTH (type)))
 	{
 	  val_print_optimized_out (stream);
 	  return 0;
 	}
 
-      if (value_bits_synthetic_pointer (val, TARGET_CHAR_BIT * offset,
+      if (value_bits_synthetic_pointer (val, TARGET_CHAR_BIT * embedded_offset,
 					TARGET_CHAR_BIT * TYPE_LENGTH (type)))
 	{
 	  fputs_filtered (_("<synthetic pointer>"), stream);
 	  return 0;
 	}
+
+      if (!value_bytes_available (val, embedded_offset, TYPE_LENGTH (type)))
+	{
+	  val_print_unavailable (stream);
+	  return 0;
+	}
     }
 
   return 1;
@@ -293,6 +299,12 @@ val_print_optimized_out (struct ui_file
   fprintf_filtered (stream, _("<optimized out>"));
 }
 
+void
+val_print_unavailable (struct ui_file *stream)
+{
+  fprintf_filtered (stream, _("<unavailable>"));
+}
+
 /* Print using the given LANGUAGE the data of type TYPE located at
    VALADDR + EMBEDDED_OFFSET (within GDB), which came from the
    inferior at address ADDRESS + EMBEDDED_OFFSET, onto stdio stream
@@ -560,6 +572,8 @@ val_print_scalar_formatted (struct type
   if (!value_bits_valid (val, TARGET_CHAR_BIT * embedded_offset,
 			      TARGET_CHAR_BIT * TYPE_LENGTH (type)))
     val_print_optimized_out (stream);
+  else if (!value_bytes_available (val, embedded_offset, TYPE_LENGTH (type)))
+    val_print_unavailable (stream);
   else
     print_scalar_formatted (valaddr + embedded_offset, type,
 			    options, size, stream);
Index: src/gdb/python/py-prettyprint.c
===================================================================
--- src.orig/gdb/python/py-prettyprint.c	2011-02-10 18:21:33.747075995 +0000
+++ src/gdb/python/py-prettyprint.c	2011-02-10 18:42:12.037075997 +0000
@@ -690,6 +690,11 @@ apply_val_pretty_printer (struct type *t
   struct cleanup *cleanups;
   int result = 0;
   enum string_repr_result print_result;
+
+  /* No pretty-printer support for unavailable values.  */
+  if (!value_bytes_available (val, embedded_offset, TYPE_LENGTH (type)))
+    return 0;
+
   cleanups = ensure_python_env (gdbarch, language);
 
   /* Instantiate the printer.  */

Re: [unavailable values part 1, 01/17] base support for unavailable value contents

[Tom Tromey]

>>>>> "Pedro" == Pedro Alves <pedro@codesourcery.com> writes:

Pedro> Yeah, I don't expect to have more than a handful of ranges to work with.
Pedro> But in any case, I've done that change.  Below's my current patch.
Pedro> WDYT?

Looks great.

Tom> This could also use a binary search.  Again, maybe not worth the effort.

Pedro> Also done.  I've added some "pictures" in the comments, but I'm
Pedro> not sure if that's overboard.

M-x artist-mode FTW :-)
Seriously, though, looks good to me.

Tom

Re: [unavailable values part 1, 01/17] base support for unavailable value contents

[Pedro Alves]

On Monday 14 February 2011 21:10:55, Tom Tromey wrote:
> >>>>> "Pedro" == Pedro Alves <pedro@codesourcery.com> writes:
> 
> Pedro> Yeah, I don't expect to have more than a handful of ranges to work with.
> Pedro> But in any case, I've done that change.  Below's my current patch.
> Pedro> WDYT?
> 
> Looks great.
> 
> Tom> This could also use a binary search.  Again, maybe not worth the effort.
> 
> Pedro> Also done.  I've added some "pictures" in the comments, but I'm
> Pedro> not sure if that's overboard.
> 
> M-x artist-mode FTW :-)
> Seriously, though, looks good to me.

:-)

Thanks for the review.  I've checked it in.

-- 
Pedro Alves

Re: [unavailable values part 1, 01/17] base support for unavailable value contents

[Jan Kratochvil]

On Thu, 10 Feb 2011 20:30:26 +0100, Pedro Alves wrote:
> +/* Defines an [OFFSET, OFFSET + LENGTH) range.  */
> +
> +struct range
> +{
> +  /* Lowest offset in the range.  */
> +  int offset;
> +
> +  /* Length of the range.  */
> +  int length;
> +};

I would find [LOW, HIGH) fields more readable as the code now still calculates
offset + length back and forth all over the code.  FYI, not trying to require
it, though.


> +/* Returns true if RANGES contains any range that overlaps [OFFSET,
> +   OFFSET+LENGTH).  */
> +
> +static int
> +ranges_contain_p (VEC(range_s) *ranges, int offset, int length)

Couldn't even this function stick with the `overlap' term?

`contain' associates to me:
  Returns true if each byte of [OFFSET, OFFSET+LENGTH) is overlapping with any
  range in the RANGES list.

(My English association may not be right, though.)


> @@ -206,6 +310,11 @@ struct value
> +  /* Unavailable ranges in CONTENTS.  We mark unavailable ranges,
> +     rather than available, since the common and default case is for a
> +     value to be available.  This is filled in at value read time.  */
> +  VEC(range_s) *unavailable;

Was there considered the opposite way to have a list of available ranges?

Besides cleaning up the inversion code implemented by this patchset in
read_value_memory it would also enable storing discontiguous memory with
a value.

Currently if you store inferior C++ object into a $convenience_variable you
cannot do much with it as it can no longer read the virtual method table
- besides it may no longer exist in the inferior the current code will not
even try to read it from the inferior.

I faced it also with archer-jankratochvil-vla - if you have a very large
inferior array printing only some slices/subsets of it - you still have to
store for $convenience_variable the whole range between first byte and last
byte accessed, despite most of the ranges in between get never accessed.  You
will mostly print some slices/subsets because the whole array is too large.

So I was considering to turn value->contents into some discontiguous ranges
and this patch could also benefit from it.


>  };
>  
> +int
> +value_bytes_available (const struct value *value, int offset, int length)

ctags will never find a comment defined in a .h file.  I would prefer at least
a stub comment:

/* Function comment present at the declaration.  */

Many legacy functions just do not have any comment so one just does not search
more for a comment when there isn't any shown on the ctags-jump.



> +  i = VEC_lower_bound (range_s, value->unavailable, &newr, range_lessthan);
> +  if (i > 0)
> +    {
> +      struct range *bef = VEC_index (range_s, value->unavailable, i - i);

While this patch revisiou fixed two bugs
it has introduced a new bug - "i - i" -> "i - 1".


Thanks,
Jan

Re: [unavailable values part 1, 01/17] base support for unavailable value contents

[Pedro Alves]

Hi Jan.  Thanks for thorough review of the whole series.
As you'll see, I checked it in just before you sent
your comments.  :-/

On Monday 14 February 2011 11:59:19, Jan Kratochvil wrote:
> On Thu, 10 Feb 2011 20:30:26 +0100, Pedro Alves wrote:
> > +/* Defines an [OFFSET, OFFSET + LENGTH) range.  */
> > +
> > +struct range
> > +{
> > +  /* Lowest offset in the range.  */
> > +  int offset;
> > +
> > +  /* Length of the range.  */
> > +  int length;
> > +};
> 
> I would find [LOW, HIGH) fields more readable as the code now still calculates
> offset + length back and forth all over the code.  FYI, not trying to require
> it, though.

Maybe.  I just picked an option and sticked to it.  I don't
know if [LOW, HIGH] wouldn't make `high - low' appear all over.

> 
> > +/* Returns true if RANGES contains any range that overlaps [OFFSET,
> > +   OFFSET+LENGTH).  */
> > +
> > +static int
> > +ranges_contain_p (VEC(range_s) *ranges, int offset, int length)
> 
> Couldn't even this function stick with the `overlap' term?

I guess it could.  There's already a ranges_overlap function
though.  I'm open to concrete suggestions, though IMO this
isn't worth the bother.

> 
> `contain' associates to me:
>   Returns true if each byte of [OFFSET, OFFSET+LENGTH) is overlapping with any
>   range in the RANGES list.
> 
> (My English association may not be right, though.)
> 
> 
> > @@ -206,6 +310,11 @@ struct value
> > +  /* Unavailable ranges in CONTENTS.  We mark unavailable ranges,
> > +     rather than available, since the common and default case is for a
> > +     value to be available.  This is filled in at value read time.  */
> > +  VEC(range_s) *unavailable;
> 
> Was there considered the opposite way to have a list of available ranges?

As is written on the comment above, the default and most common case,
which is whenever you are not inspecting a value that came from
a traceframe, you'll have the value wholly available.  That means
we can just leave the value->unavailable VEC empty/NULL in most cases.
If we switch the logic, we need to always allocate the VEC with a single
range covering the whole contents, and then punch holes as we
find them.  As that's more wasteful in terms of memory, I opted
for the current logic.  Or maybe we could special case the empty
case as meaning all-available?  It might work.  Not sure it'd end
up looking simpler, given the punch-holes need.

> it would also enable storing discontiguous memory with
> a value.

Can't see why you can't keep the "unavailable" ranges logic
even if we store discontiguous memory in values.  I haven't
looked at your code yet though.

> > +  i = VEC_lower_bound (range_s, value->unavailable, &newr, range_lessthan);
> > +  if (i > 0)
> > +    {
> > +      struct range *bef = VEC_index (range_s, value->unavailable, i - i);
> 
> While this patch revisiou fixed two bugs
> it has introduced a new bug - "i - i" -> "i - 1".

Bummer.  I did a bunch of unit testing with a hack
that called mark_value_bytes_unavailable in different combinations
and then checked the VEC for expected results, and this still made
it through....

Fixed now, as below...

-- 
Pedro Alves

2011-02-14  Pedro Alves  <pedro@codesourcery.com>
	    Jan Kratochvil  <jan.kratochvil@redhat.com>

	gdb/
	* value.c (mark_value_bytes_unavailable): Fix indexing the `bef'
	range.

---
 gdb/value.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Index: src/gdb/value.c
===================================================================
--- src.orig/gdb/value.c	2011-02-14 11:51:57.000000000 +0000
+++ src/gdb/value.c	2011-02-14 12:43:22.691772006 +0000
@@ -439,7 +439,7 @@ mark_value_bytes_unavailable (struct val
   i = VEC_lower_bound (range_s, value->unavailable, &newr, range_lessthan);
   if (i > 0)
     {
-      struct range *bef = VEC_index (range_s, value->unavailable, i - i);
+      struct range *bef = VEC_index (range_s, value->unavailable, i - 1);
 
       if (ranges_overlap (bef->offset, bef->length, offset, length))
 	{

Re: [unavailable values part 1, 01/17] base support for unavailable value contents

[Jan Kratochvil]

On Mon, 14 Feb 2011 13:59:11 +0100, Pedro Alves wrote:
> On Monday 14 February 2011 11:59:19, Jan Kratochvil wrote:
> > On Thu, 10 Feb 2011 20:30:26 +0100, Pedro Alves wrote:
> > > +static int
> > > +ranges_contain_p (VEC(range_s) *ranges, int offset, int length)
> > 
> > Couldn't even this function stick with the `overlap' term?
> 
> I guess it could.  There's already a ranges_overlap function
> though.  I'm open to concrete suggestions, though IMO this
> isn't worth the bother.

mem_ranges_list_overlaps?

Contrary to:
mem_ranges_overlap (CORE_ADDR start1, int len1, CORE_ADDR start2, int len2)


> If we switch the logic, we need to always allocate the VEC with a single
> range covering the whole contents, and then punch holes as we
> find them.  As that's more wasteful in terms of memory, I opted
> for the current logic.  Or maybe we could special case the empty
> case as meaning all-available?  It might work.

I agree but I do not consider this difference so important.


> > it would also enable storing discontiguous memory with
> > a value.
> 
> Can't see why you can't keep the "unavailable" ranges logic
> even if we store discontiguous memory in values.

It is questionable if it is the same.  I thought that any memory which is not
stored is <unavailable>.  Another look may be the memory is of three kinds:
 * stored/available
 * marked as <unavailable>
 * not stored, internal GDB error if an access is attempted

In the three-kinds model you are right these two features are unrelated.


Anyway your patchset is checked in and I do not plan to place much new
features on top of archer-jankratochvil-vla before it gets merged in some
form.


> I haven't looked at your code yet though.

archer-jankratochvil-vla currently does not contain any discontiguous memory
ranges code.  I mentioned it as it could benefit from such a feature.


Thanks,
Jan