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

[Pekka Paalanen]

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?

I was expecting the result to not be used in case of an overflow.

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


Thanks,
pq

>  
>      return overflow;
>  }
> @@ -341,10 +349,11 @@ checked_int64_add(int64_t *out, int64_t a, int64_t b)
>  static inline Bool
>  checked_int64_subtract(int64_t *out, int64_t a, int64_t b)
>  {
> -    int64_t result = a - b;
> -    Bool overflow = (a < 0) != (b < 0) && (a < 0) != (result < 0);
> +    uint64_t result = (uint64_t)a - (uint64_t)b;
> +    Bool result_negative = (result & (1ull << 63)) != 0;
> +    Bool overflow = (a < 0) != (b < 0) && (a < 0) != result_negative;
>  
> -    *out = result;
> +    memcpy(out, &result, sizeof(result));
>  
>      return overflow;
>  }

-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 833 bytes
Desc: OpenPGP digital signature
URL: <https://lists.x.org/archives/xorg-devel/attachments/20170904/ece8ef0a/attachment.sig>