[PATCH xserver] squash! sync: Convert from "CARD64" to int64_t. (v2)

[Eric Anholt]

Pekka Paalanen <ppaalanen at gmail.com> writes:

> [ Unknown signature status ]
> On Fri,  1 Sep 2017 11:55:15 -0700
> Eric Anholt <eric at anholt.net> wrote:
>
>> ---
>> 
>> Pekka - that link didn't help, because we still need a correct
>> "result" value.  I don't believe that the compiler could break uint ->
>> int conversions with the high bit, but here's the patch I think we
>> would need for that.  I still think v1 is the better version.
>
> Hi,
>
> sorry, but I'm confused. What is the correct "result" value in case of
> an overflow?

The 2s complement addition/subtraction result.

>>  include/misc.h | 21 +++++++++++++++------
>>  1 file changed, 15 insertions(+), 6 deletions(-)
>> 
>> diff --git a/include/misc.h b/include/misc.h
>> index 0feeaebc7c1a..fc1a55dac343 100644
>> --- a/include/misc.h
>> +++ b/include/misc.h
>> @@ -327,13 +327,21 @@ bswap_32(uint32_t x)
>>  static inline Bool
>>  checked_int64_add(int64_t *out, int64_t a, int64_t b)
>>  {
>> -    int64_t result = a + b;
>> +    /* Note that overflow behavior with signed ints in C is undefined,
>> +     * and the compiler might optimize our check away if we do so.  In
>> +     * the discussion about it, people raised the concern that even
>> +     * casting from uint to int would be undefined, so we stick with
>> +     * all of our math in uint and memcpy the result, out of extreme
>> +     * paranoia.
>> +     */
>> +    uint64_t result = (uint64_t)a + (uint64_t)b;
>>      /* signed addition overflows if operands have the same sign, and
>>       * the sign of the result doesn't match the sign of the inputs.
>>       */
>> -    Bool overflow = (a < 0) == (b < 0) && (a < 0) != (result < 0);
>> +    Bool result_negative = (result & (1ull << 63)) != 0;
>> +    Bool overflow = (a < 0) == (b < 0) && (a < 0) != result_negative;
>>  
>> -    *out = result;
>> +    memcpy(out, &result, sizeof(result));
>
> You might hate the memcpy() and so do I, but better ideas seem scarce.
>
> One might be a union { int64_t; uint64_t; } for the "casting".
>
> Another would be to write the code any way you please, but add a test
> that ensures the possibly-not-guaranteed behaviour you rely on is
> actually there and correct.
>
> This is more of a learning experience for me as well, than already
> knowing what's a good way.

I already wrote the unit test, it's in patch 5 that we're replying to.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 832 bytes
Desc: not available
URL: <https://lists.x.org/archives/xorg-devel/attachments/20170905/9dbe20e0/attachment.sig>