Re: [Qemu-devel] [PATCH 1/2] cutils: Proactively fix pow2floor(), switch to unsigned

[Dr. David Alan Gilbert]

* Markus Armbruster (address@hidden) wrote:
> The function's stated contract is simple enough: "round down to the
> nearest power of 2".  Suggests the domain is the representable numbers
> >= 1, because that's the smallest power of two.
> 
> The implementation doesn't check for domain errors, but returns
> garbage instead:
> 
> * For negative arguments, pow2floor() returns -2^63, which is not even
>   a power of two, let alone the nearest one.
> 
> * For a zero argument, pow2floor() shifts right by 64.  Undefined
>   behavior.  Common actual behavior is to shift by 0, yielding -2^63.
> 
> Fix by switching to unsigned types and amending the contract to map
> zero to zero.
> 
> Callers are fine with that:
> 
> * xbzrle_cache_resize()
> 
>   Passes an int64_t argument >= TARGET_PAGE_SIZE.  Thus, the argument
>   value is representable in uint64_t, and not zero.  The result is
>   converted back to int64_t.  Safe, because int64_t can represent the
>   value.
> 
>   No change.
> 
> * cache_init()
> 
>   Likewise, except > 0 instead of >= TARGET_PAGE_SIZE.  No change.
> 
> * cache_resize()
> 
>   Unused since commit fd8cec9.  Before, its caller passed an int64_t
>   argumet >= 1.
> 
> * spapr_node0_size() and spapr_populate_memory()
> 
>   Argument comes from numa_info[].node_mem.  This is uint64_t.  Before
>   this patch, we convert it to int64_t for pow2floor(), and convert
>   its result to hwaddr, i.e. back to uint64_t.  Not obviously safe.
>   The patch gets rid of the dubious conversions.
> 
>   The patch also gets rid of undefined behavior on zero, and maps zero
>   to zero instead.  Beats mapping it to 2^63, which is what we
>   commonly get from the undefined behavior before the patch.
> 
>   Thus, the patch is a strict improvement here.
> 
> Signed-off-by: Markus Armbruster <address@hidden>
> ---
>  include/qemu-common.h |  6 ++++--
>  util/cutils.c         | 11 +++++------
>  2 files changed, 9 insertions(+), 8 deletions(-)
> 
> diff --git a/include/qemu-common.h b/include/qemu-common.h
> index 644b46d..f3ede45 100644
> --- a/include/qemu-common.h
> +++ b/include/qemu-common.h
> @@ -415,8 +415,10 @@ static inline bool is_power_of_2(uint64_t value)
>      return !(value & (value - 1));
>  }
>  
> -/* round down to the nearest power of 2*/
> -int64_t pow2floor(int64_t value);
> +/**
> + * Return @value rounded down to the nearest power of two or zero.
> + */
> +uint64_t pow2floor(uint64_t value);
>  
>  #include "qemu/module.h"
>  
> diff --git a/util/cutils.c b/util/cutils.c
> index dbe7412..4b8c5be 100644
> --- a/util/cutils.c
> +++ b/util/cutils.c
> @@ -474,13 +474,12 @@ int qemu_parse_fd(const char *param)
>      return fd;
>  }
>  
> -/* round down to the nearest power of 2*/
> -int64_t pow2floor(int64_t value)
> +/**
> + * Return @value rounded down to the nearest power of two or zero.
> + */
> +uint64_t pow2floor(uint64_t value)
>  {
> -    if (!is_power_of_2(value)) {
> -        value = 0x8000000000000000ULL >> clz64(value);
> -    }
> -    return value;
> +    return !value ? 0 : 0x8000000000000000ull >> clz64(value);
>  }

Yes (it's odd that clz returns an int rather than unsigned)

Reviewed-by:  Dr. David Alan Gilbert <address@hidden>

Dave

>  
>  /*
> -- 
> 1.9.3
> 
> 
--
Dr. David Alan Gilbert / address@hidden / Manchester, UK