Re: [Qemu-devel] [Qemu-trivial] [PATCH 2/3] int128.h: Avoid undefined behaviours involving signed arithmetic

[Avi Kivity <avi@cloudius-systems.com>]

On 04/06/2014 01:18 PM, Peter Maydell wrote:
> On 6 April 2014 08:09, Michael Tokarev <mjt@tls.msk.ru> wrote:
>> 28.03.2014 19:12, Peter Maydell wrote:
>>> Add casts when we're performing arithmetic on the .hi parts of an
>>> Int128, to avoid undefined behaviour.
>> []
>>>   static inline Int128 int128_sub(Int128 a, Int128 b)
>>>   {
>>> -    return (Int128){ a.lo - b.lo, a.hi - b.hi - (a.lo < b.lo) };
>>> +    return (Int128){ a.lo - b.lo, (uint64_t)a.hi - b.hi - (a.lo < b.lo) };
>> What was wrong with this one?  I don't think casting to unsigned here is
>> a good idea.
> This patch is fixing these three clang sanitizer warnings:
> /home/petmay01/linaro/qemu-from-laptop/qemu/include/qemu/int128.h:81:40:
> runtime error: signed integer overflow: 0 - -9223372036854775808
> cannot be represented in type 'long'
> /home/petmay01/linaro/qemu-from-laptop/qemu/include/qemu/int128.h:81:47:
> runtime error: signed integer overflow: -9223372036854775808 - 1
> cannot be represented in type 'long'
> /home/petmay01/linaro/qemu-from-laptop/qemu/include/qemu/int128.h:56:47:
> runtime error: left shift of negative value -9223372036854775807
>
> of which the first two are in this function.
>
> Note that int128_add() already has a cast.
>
> The alternative would be to say that Int128 should have
> undefined behaviour on underflow/overflow and the test
> code is wrong, but that doesn't seem very useful to me.
>
>

Isn't the test broken here?  It is trying to add (or shift) -2^127 and 
something else, and the result truly overflows.

A better behaviour would be to abort when this happens.  Int128 was 
designed to avoid silent overflows, not to silently cause breakage.

Not that I think it is necessary, there is no way for the guest to 
trigger an overflow.