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

[Michael Tokarev <mjt@tls.msk.ru>]

06.04.2014 14:18, 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'

Int128 is defined as { uint64_t lo, int64_t hi }, so the second half is
signed (because Int128 should be able to represent negative numbers too).

-9223372036854775808 is 0x8000000000000000 -- which is only sign bit = 1.

uint64_t - int64_t is already somewhat undefined - should the second
int be treated as unsigned too? (I'm sorry I don't really remember the
C specs in there).

But more to the point - the new behavour, while "defined", is just as
arbitrary as the old "undefined" behavour.  On overflow we can get either
truncated or negative result, neither of which is right.

> /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.

It is still arbitrary.

But whole thing looks more like an attempt to shut up a bogus compiler warning
really.  It is not correct either way because there's just no correct way,
because the end behavour is undefined in hardware, and we _are_ emulating
hardware here.

Yes, int128_add() has a cast already, and it is just as arbitrary as this one.

So.. maybe for consistency with _add(), maybe to shut up useless warning, maybe
to have something to base units-tests on (arbitrary but defined), this can be
applied... I think :)

So, applied to -trivial ;)

Thanks,

/mjt