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

[Pedro Alves <pedro@codesourcery.com>]

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