This is the mail archive of the
gdb-patches@sourceware.org
mailing list for the GDB project.
Re: [unavailable values part 1, 01/17] base support for unavailable value contents
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))
{