Re: [Qemu-devel] [PATCH for-2.5] hw/timer/hpet.c: Avoid signed integer overflow which results in bugs on OSX

[Paolo Bonzini <pbonzini@redhat.com>]

On 09/11/2015 15:56, Peter Maydell wrote:
> Signed integer overflow in C is undefined behaviour, and the compiler
> is at liberty to assume it can never happen and optimize accordingly.
> In particular, the subtractions in hpet_time_after() and hpet_time_after64()
> were causing OSX clang to optimize the code such that it was prone to
> hangs and complaints about the main loop stalling (presumably because
> we were spending all our time trying to service very high frequency
> HPET timer callbacks). The clang sanitizer confirms the UB:
> 
> hw/timer/hpet.c:119:26: runtime error: signed integer overflow: -2146967296 - 2147003978 cannot be represented in type 'int'
> 
> Fix this by doing the subtraction as an unsigned operation and then
> converting to signed for the comparison.
> 
> Reported-by: Aaron Elkins <threcius@yahoo.com>
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>  hw/timer/hpet.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/timer/hpet.c b/hw/timer/hpet.c
> index 3037bef..7f0391c 100644
> --- a/hw/timer/hpet.c
> +++ b/hw/timer/hpet.c
> @@ -116,12 +116,12 @@ static uint32_t timer_enabled(HPETTimer *t)
>  
>  static uint32_t hpet_time_after(uint64_t a, uint64_t b)
>  {
> -    return ((int32_t)(b) - (int32_t)(a) < 0);
> +    return ((int32_t)(b - a) < 0);

Does checkpatch complain about the outer parentheses?

>  }
>  
>  static uint32_t hpet_time_after64(uint64_t a, uint64_t b)
>  {
> -    return ((int64_t)(b) - (int64_t)(a) < 0);
> +    return ((int64_t)(b - a) < 0);
>  }

Cc: qemu-stable@nongnu.org

Looks good, thanks!  Can you apply it yourself?

Paolo

>  static uint64_t ticks_to_ns(uint64_t value)
>