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 <arm...@redhat.com>
And now we can create the range [0,2^64-1] without issues. Nice!
Add a test for that then?
> ---
> 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.
> --
> 2.5.5
