Re: [Qemu-devel] [PATCH RFC 4/4] range: Replace internal representation of Range

[Markus Armbruster]

"Michael S. Tsirkin" <address@hidden> writes:

> On Wed, Jun 15, 2016 at 10:41:50PM +0200, Markus Armbruster wrote:
>> Range represents a range as follows.  Member @start is the inclusive
>> lower bound, member @end is the exclusive upper bound.  Zero @end is
>> special: if @start is also zero, the range is empty, else @end is to
>> be interpreted as 2^64.  No other empty ranges may occur.
>> 
>> The range [0,2^64-1] cannot be represented.  If you try to create it
>> with range_set_bounds1(), you get the empty range instead.  If you try
>> to create it with range_set_bounds() or range_extend(), assertions
>> fail.  Before range_set_bounds() existed, the open-coded creation
>> usually got you the empty range instead.  Open deathtrap.
>> 
>> Moreover, the code dealing with the janus-faced @end is too clever by
>> half.
>> 
>> Dumb this down to a more pedestrian representation: members @lob and
>> @upb are inclusive lower and upper bounds.  The empty range is encoded
>> as @lob = 1, @upb = 0.
>> 
>> Signed-off-by: Markus Armbruster <address@hidden>
>
> And now we can create the range [0,2^64-1] without issues. Nice!
> Add a test for that then?

Sure!

>> ---
>>  include/qemu/range.h | 55 
>> +++++++++++++++++++++++++---------------------------
>>  util/range.c         | 13 +++----------
>>  2 files changed, 29 insertions(+), 39 deletions(-)
>> 
>> diff --git a/include/qemu/range.h b/include/qemu/range.h
>> index c8c46a9..06ff361 100644
>> --- a/include/qemu/range.h
>> +++ b/include/qemu/range.h
>> @@ -26,37 +26,37 @@
>>  /*
>>   * Operations on 64 bit address ranges.
>>   * Notes:
>> - *   - ranges must not wrap around 0, but can include the last byte ~0x0LL.
>> - *   - this can not represent a full 0 to ~0x0LL range.
>> + * - Ranges must not wrap around 0, but can include UINT64_MAX.
>>   */
>>  
>> -/* A structure representing a range of addresses. */
>>  struct Range {
>> -    uint64_t begin; /* First byte of the range, or 0 if empty. */
>> -    uint64_t end;   /* 1 + the last byte. 0 if range empty or ends at 
>> ~0x0LL. */
>> +    /*
>> +     * A non-empty range has @lob <= @upb.
>> +     * An empty range has @lob == @upb + 1.
>> +     */
>> +    uint64_t lob;        /* inclusive lower bound */
>> +    uint64_t upb;        /* inclusive upper bound */
>>  };
>>  
>>  static inline void range_invariant(Range *range)
>>  {
>> -    assert((!range->begin && !range->end) /* empty */
>> -           || range->begin <= range->end - 1); /* non-empty */
>> +    assert(range->lob <= range->upb || range->lob == range->upb + 1);
>>  }
>>  
>>  /* Compound literal encoding the empty range */
>> -#define range_empty ((Range){ .begin = 0, .end = 0 })
>> +#define range_empty ((Range){ .lob = 1, .upb = 0 })
>>  
>>  /* Is @range empty? */
>>  static inline bool range_is_empty(Range *range)
>>  {
>>      range_invariant(range);
>> -    return !range->begin && !range->end;
>> +    return range->lob > range->upb;
>>  }
>>  
>>  /* Does @range contain @val? */
>>  static inline bool range_contains(Range *range, uint64_t val)
>>  {
>> -    return !range_is_empty(range)
>> -        && val >= range->begin && val <= range->end - 1;
>> +    return val >= range->lob && val <= range->upb;
>>  }
>>  
>>  /* Initialize @range to the empty range */
>> @@ -71,14 +71,11 @@ static inline void range_make_empty(Range *range)
>>   * Both bounds are inclusive.
>>   * The interval must not be empty, i.e. @lob must be less than or
>>   * equal @upb.
>> - * The interval must not be [0,UINT64_MAX], because Range can't
>> - * represent that.
>>   */
>>  static inline void range_set_bounds(Range *range, uint64_t lob, uint64_t 
>> upb)
>>  {
>> -    assert(lob <= upb);
>> -    range->begin = lob;
>> -    range->end = upb + 1;       /* may wrap to zero, that's okay */
>> +    range->lob = lob;
>> +    range->upb = upb;
>>      assert(!range_is_empty(range));
>>  }
>>  
>> @@ -91,8 +88,12 @@ static inline void range_set_bounds(Range *range, 
>> uint64_t lob, uint64_t upb)
>>  static inline void range_set_bounds1(Range *range,
>>                                       uint64_t lob, uint64_t upb_plus1)
>>  {
>> -    range->begin = lob;
>> -    range->end = upb_plus1;
>> +    if (!lob && !upb_plus1) {
>> +        *range = range_empty;
>> +    } else {
>> +        range->lob = lob;
>> +        range->upb = upb_plus1 - 1;
>> +    }
>>      range_invariant(range);
>>  }
>>  
>> @@ -100,20 +101,18 @@ static inline void range_set_bounds1(Range *range,
>>  static inline uint64_t range_lob(Range *range)
>>  {
>>      assert(!range_is_empty(range));
>> -    return range->begin;
>> +    return range->lob;
>>  }
>>  
>>  /* Return @range's upper bound.  @range must not be empty. */
>>  static inline uint64_t range_upb(Range *range)
>>  {
>>      assert(!range_is_empty(range));
>> -    return range->end - 1;
>> +    return range->upb;
>>  }
>>  
>>  /*
>>   * Extend @range to the smallest interval that includes @extend_by, too.
>> - * This must not extend @range to cover the interval [0,UINT64_MAX],
>> - * because Range can't represent that.
>>   */
>>  static inline void range_extend(Range *range, Range *extend_by)
>>  {
>> @@ -124,15 +123,13 @@ static inline void range_extend(Range *range, Range 
>> *extend_by)
>>          *range = *extend_by;
>>          return;
>>      }
>> -    if (range->begin > extend_by->begin) {
>> -        range->begin = extend_by->begin;
>> +    if (range->lob > extend_by->lob) {
>> +        range->lob = extend_by->lob;
>>      }
>> -    /* Compare last byte in case region ends at ~0x0LL */
>> -    if (range->end - 1 < extend_by->end - 1) {
>> -        range->end = extend_by->end;
>> +    if (range->upb < extend_by->upb) {
>> +        range->upb = extend_by->upb;
>>      }
>> -    /* Must not extend to { .begin = 0, .end = 0 }: */
>> -    assert(!range_is_empty(range));
>> +    range_invariant(range);
>>  }
>>  
>>  /* Get last byte of a range from offset + length.
>> diff --git a/util/range.c b/util/range.c
>> index ca149a0..8359066 100644
>> --- a/util/range.c
>> +++ b/util/range.c
>> @@ -21,21 +21,14 @@
>>  #include "qemu/osdep.h"
>>  #include "qemu/range.h"
>>  
>> -/*
>> - * Operations on 64 bit address ranges.
>> - * Notes:
>> - *   - ranges must not wrap around 0, but can include the last byte ~0x0LL.
>> - *   - this can not represent a full 0 to ~0x0LL range.
>> - */
>> -
>>  /* Return -1 if @a < @b, 1 if greater, and 0 if they touch or overlap. */
>>  static inline int range_compare(Range *a, Range *b)
>>  {
>> -    /* Zero a->end is 2**64, and therefore not less than any b->begin */
>> -    if (a->end && a->end < b->begin) {
>> +    /* Careful, avoid wraparound */
>> +    if (b->lob && b->lob - 1 > a->upb) {
>>          return -1;
>>      }
>> -    if (b->end && a->begin > b->end) {
>> +    if (a->lob && a->lob - 1 > b->upb) {
>>          return 1;
>>      }
>>      return 0;
>
> It looks like previously, an empty range was considered
> overlapping any other range: a->begin and a->end are 0.
>
> After this change, IIUC it is smaller than any other range,
> which seems a bit arbitrary (why not greater?),
> except for another empty range, for which it is
> considered overlapping.
>
> I think it's unlikely to break anything but might be
> worth changing to match previous semantics.

It can't break anything because the only caller asserts "no empty
ranges".  I'll explain this in the commit message.

I can also make function comment cover empty ranges explicitly.  Would
"unspecified result" be okay with you?