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

[Eric Anholt]

Pekka Paalanen  writes:

> [ Unknown signature status ]
> On Fri,  1 Sep 2017 11:55:15 -0700
> Eric Anholt  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, , 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.


signature.asc
Description: PGP signature
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: https://lists.x.org/mailman/listinfo/xorg-devel

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

[Pekka Paalanen]

On Fri,  1 Sep 2017 11:55:15 -0700
Eric Anholt  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, , 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, , sizeof(result));
>  
>  return overflow;
>  }



pgpgi5zabZdKv.pgp
Description: OpenPGP digital signature
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: https://lists.x.org/mailman/listinfo/xorg-devel

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

[Eric Anholt]

---

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.

 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, , sizeof(result));
 
 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, , sizeof(result));
 
 return overflow;
 }
-- 
2.14.1

___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: https://lists.x.org/mailman/listinfo/xorg-devel