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

[Peter Maydell]

On 6 April 2014 15:13, Michael Tokarev <address@hidden> wrote:
> 06.04.2014 14:18, Peter Maydell wrote:
>> On 6 April 2014 08:09, Michael Tokarev <address@hidden> 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.

Yes, and 0 - this in signed arithmetic is undefined behaviour, which
is why clang is complaining.

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

This is well defined -- see C99 6.3.1.8 "Usual arithmetic conversions":
the int64_t value is converted to uint64_t and the operation is performed
using unsigned arithmetic (and the result of converting any int64_t to
a uint64_t is a well defined value).

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

Currently on overflow we are giving undefined behavour -- that
means the compiler is allowed to return 42, make QEMU segfault,
delete all the user's files, or anything else it feels like. It does not
simply mean "result is unknown" or "result is what you might expect
on a 2s complement arithmetic CPU".

The obvious semantics here are to treat Int128 as a 2s complement
value. This is particularly useful since there is no UInt128, since it
means you can actually treat it as an unsigned 128 bit integer if
you want to (though you wouldn't be able to use the comparison ops,
obviously).

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

This seems to be where we differ. This is not a bogus warning:
we're doing undefined behaviour, so either:
 (a) the test code is wrong
 (b) our implementation is wrong

"Leave it alone" is about the only thing that's definitely wrong.

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

No, it also is avoiding undefined behaviour.

thanks
-- PMM