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

[Laszlo Ersek]

On 11/09/15 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 <address@hidden>
> Signed-off-by: Peter Maydell <address@hidden>
> ---
>  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);
>  }
>  
>  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);
>  }
>  
>  static uint64_t ticks_to_ns(uint64_t value)
> 

I'm late to the discussion, but I cannot imagine what would speak against:

    return (b < a);

The post-patch code still converts a uint64_t difference to int32_t.
According to the C standard(s), such a conversion (i.e., when the
integer value being converted doesn't fit in the target signed integer)
results in an implementation-defined value, or an implementation-defined
signal is raised.

On our platforms, the impl-def value is determined by "truncate to 32
bits, then reinterpret the bit pattern as two's complement signed
int32_t". Meaning, if:

    (b > a) && ((b - a) & (1u << 31))

(that is, "b" is so much larger than "a" that bit#31 is set in the (b-a)
difference), then hpet_time_after() will now incorrectly return 1.
(Because bit#31 will be interpreted as the sign bit, turned on.)

Again, what speaks against

    return (b < a);

?

(The pre-patch code dates back to commit 16b29ae1 (year 2008), which
offers precious little justification for the formula.)

Thanks
Laszlo