Re: [PATCH xserver] squash! sync: Convert from "CARD64" to int64_t.
Michel Dänzer writes: > Is assigning an unsigned value with the MSB set to a signed variable > well-defined in C? I have no idea. And I just spent a few hours wading through the N1570 draft of the C standard on a related issue. In particular, this is worrying: 6.3.1.3 3 Otherwise, the new type is signed and the value cannot be represented in it; either the result is implementation-defined or an implementation-defined signal is raised. I don't think the absence of a cast matters. Surely some better language lawyer can tell us the answer... -- -keith 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.
On 25/08/17 03:57 AM, Eric Anholt wrote:
> ---
>
> We pass the overflow unit tests both before and after this change, but
> this should be safer.
>
> include/misc.h | 8 ++--
> 1 file changed, 6 insertions(+), 2 deletions(-)
>
> diff --git a/include/misc.h b/include/misc.h
> index 0feeaebc7c1a..9d0e422e36b4 100644
> --- a/include/misc.h
> +++ b/include/misc.h
> @@ -327,7 +327,11 @@ 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;
> +/* Do the potentially overflowing math as uint64_t, as signed
> + * integers in C are undefined on overflow (and the compiler may
> + * optimize out our overflow check below, otherwise)
> + */
> +int64_t result = (uint64_t)a + (uint64_t)b;
Is assigning an unsigned value with the MSB set to a signed variable
well-defined in C?
--
Earthling Michel Dänzer | http://www.amd.com
Libre software enthusiast | Mesa and X developer
___
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.
Eric Anholt writes: > --- > > We pass the overflow unit tests both before and after this change, but > this should be safer. I've seen GCC do precisely the 'optimization' you are concerned about, but only when one of the operands is constant. One should use -fwrapv whenever using gcc so that you get correct behavior, instead of compiler-writers-potluck. I hope the gcc authors agree with your reading of the spec in this case; it's getting very hard to trust them to ever do the right thing though. Reviewed-by: Keith Packard -- -keith 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
[PATCH xserver] squash! sync: Convert from "CARD64" to int64_t.
---
We pass the overflow unit tests both before and after this change, but
this should be safer.
include/misc.h | 8 ++--
1 file changed, 6 insertions(+), 2 deletions(-)
diff --git a/include/misc.h b/include/misc.h
index 0feeaebc7c1a..9d0e422e36b4 100644
--- a/include/misc.h
+++ b/include/misc.h
@@ -327,7 +327,11 @@ 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;
+/* Do the potentially overflowing math as uint64_t, as signed
+ * integers in C are undefined on overflow (and the compiler may
+ * optimize out our overflow check below, otherwise)
+ */
+int64_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.
*/
@@ -341,7 +345,7 @@ 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;
+int64_t result = (uint64_t)a - (uint64_t)b;
Bool overflow = (a < 0) != (b < 0) && (a < 0) != (result < 0);
*out = result;
--
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
Re: [PATCH xserver 6/7 v2] sync: Convert from "CARD64" to int64_t.
2017-08-23 22:06 GMT+05:00 Eric Anholt :
> diff --git a/include/misc.h b/include/misc.h
> index 38af70ff9e89..0feeaebc7c1a 100644
> --- a/include/misc.h
> +++ b/include/misc.h
> @@ -324,6 +324,31 @@ bswap_32(uint32_t x)
> ((x & 0x00FF) << 24));
> }
>
> +static inline Bool
> +checked_int64_add(int64_t *out, int64_t a, int64_t b)
> +{
> +int64_t result = a + 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);
> +
> +*out = result;
> +
> +return overflow;
> +}
> +
> +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);
> +
> +*out = result;
> +
> +return overflow;
> +}
> +
NAK.
C compilers are allowed to assume that signed arithmetical operations
never overflow. I.e. to optimize your overflow check, because it never
triggers if there is no overflow.
https://www.airs.com/blog/archives/120
Please either make sure that all code that includes this header is
compiled with -fno-strict-overflow, or rewrite the check in a way that
does not check the result but only the operands and things like
INT64_MAX.
--
Alexander E. Patrakov
___
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
