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
[PATCH xserver 6/7 v2] sync: Convert from "CARD64" to int64_t.
The extension was using the name CARD64 to represent 64-bit values,
with a #define from CARD64 to XSyncValue, a struct with a pair of
32-bit values representing a signed 64-bit value. This interfered
with protocol headers using CARD64 to try to actually store a
uint64_t. Now that stdint.h exists, let's just use that here,
instead.
v2: Fix alarm delta changes.
Signed-off-by: Eric Anholt
---
Xext/sync.c | 302 +++-
Xext/syncsrv.h | 23 ++--
include/misc.h | 25
miext/sync/misync.c | 5 +-
miext/sync/misyncstr.h | 11 +-
present/present_fence.c | 2 +-
6 files changed, 186 insertions(+), 182 deletions(-)
diff --git a/Xext/sync.c b/Xext/sync.c
index a8db0ec22371..822c205a7a24 100644
--- a/Xext/sync.c
+++ b/Xext/sync.c
@@ -237,7 +237,7 @@ SyncAddTriggerToSyncObject(SyncTrigger * pTrigger)
*/
static Bool
-SyncCheckTriggerPositiveComparison(SyncTrigger * pTrigger, CARD64 oldval)
+SyncCheckTriggerPositiveComparison(SyncTrigger * pTrigger, int64_t oldval)
{
SyncCounter *pCounter;
@@ -248,12 +248,11 @@ SyncCheckTriggerPositiveComparison(SyncTrigger *
pTrigger, CARD64 oldval)
pCounter = (SyncCounter *) pTrigger->pSync;
-return (pCounter == NULL ||
-XSyncValueGreaterOrEqual(pCounter->value, pTrigger->test_value));
+return pCounter == NULL || pCounter->value >= pTrigger->test_value;
}
static Bool
-SyncCheckTriggerNegativeComparison(SyncTrigger * pTrigger, CARD64 oldval)
+SyncCheckTriggerNegativeComparison(SyncTrigger * pTrigger, int64_t oldval)
{
SyncCounter *pCounter;
@@ -264,12 +263,11 @@ SyncCheckTriggerNegativeComparison(SyncTrigger *
pTrigger, CARD64 oldval)
pCounter = (SyncCounter *) pTrigger->pSync;
-return (pCounter == NULL ||
-XSyncValueLessOrEqual(pCounter->value, pTrigger->test_value));
+return pCounter == NULL || pCounter->value <= pTrigger->test_value;
}
static Bool
-SyncCheckTriggerPositiveTransition(SyncTrigger * pTrigger, CARD64 oldval)
+SyncCheckTriggerPositiveTransition(SyncTrigger * pTrigger, int64_t oldval)
{
SyncCounter *pCounter;
@@ -281,12 +279,12 @@ SyncCheckTriggerPositiveTransition(SyncTrigger *
pTrigger, CARD64 oldval)
pCounter = (SyncCounter *) pTrigger->pSync;
return (pCounter == NULL ||
-(XSyncValueLessThan(oldval, pTrigger->test_value) &&
- XSyncValueGreaterOrEqual(pCounter->value, pTrigger->test_value)));
+(oldval < pTrigger->test_value &&
+ pCounter->value >= pTrigger->test_value));
}
static Bool
-SyncCheckTriggerNegativeTransition(SyncTrigger * pTrigger, CARD64 oldval)
+SyncCheckTriggerNegativeTransition(SyncTrigger * pTrigger, int64_t oldval)
{
SyncCounter *pCounter;
@@ -298,12 +296,12 @@ SyncCheckTriggerNegativeTransition(SyncTrigger *
pTrigger, CARD64 oldval)
pCounter = (SyncCounter *) pTrigger->pSync;
return (pCounter == NULL ||
-(XSyncValueGreaterThan(oldval, pTrigger->test_value) &&
- XSyncValueLessOrEqual(pCounter->value, pTrigger->test_value)));
+(oldval > pTrigger->test_value &&
+ pCounter->value <= pTrigger->test_value));
}
static Bool
-SyncCheckTriggerFence(SyncTrigger * pTrigger, CARD64 unused)
+SyncCheckTriggerFence(SyncTrigger * pTrigger, int64_t unused)
{
SyncFence *pFence = (SyncFence *) pTrigger->pSync;
@@ -389,16 +387,15 @@ SyncInitTrigger(ClientPtr client, SyncTrigger * pTrigger,
XID syncObject,
if (pTrigger->value_type == XSyncAbsolute)
pTrigger->test_value = pTrigger->wait_value;
else { /* relative */
-
Bool overflow;
if (pCounter == NULL)
return BadMatch;
-XSyncValueAdd(&pTrigger->test_value, pCounter->value,
- pTrigger->wait_value, &overflow);
+overflow = checked_int64_add(&pTrigger->test_value,
+ pCounter->value,
pTrigger->wait_value);
if (overflow) {
-client->errorValue = XSyncValueHigh32(pTrigger->wait_value);
+client->errorValue = pTrigger->wait_value >> 32;
return BadValue;
}
}
@@ -441,15 +438,15 @@ SyncSendAlarmNotifyEvents(SyncAlarm * pAlarm)
.type = SyncEventBase + XSyncAlarmNotify,
.kind = XSyncAlarmNotify,
.alarm = pAlarm->alarm_id,
-.alarm_value_hi = XSyncValueHigh32(pTrigger->test_value),
-.alarm_value_lo = XSyncValueLow32(pTrigger->test_value),
+.alarm_value_hi = pTrigger->test_value >> 32,
+.alarm_value_lo = pTrigger->test_value,
.time = currentTime.milliseconds,
.state = pAlarm->state
};
if (pTrigger->pSync && SYNC_COUNTER == pTrigger->pSync->type) {
-ane.counter_value_hi = XSyncValueHigh32(pCounter->value);
-ane.counter_value_lo = XSyncValueLow3
Re: [PATCH xserver 6/7 v2] sync: Convert from "CARD64" to int64_t.
Eric Anholt writes: > The extension was using the name CARD64 to represent 64-bit values, > with a #define from CARD64 to XSyncValue, a struct with a pair of > 32-bit values representing a signed 64-bit value. This interfered > with protocol headers using CARD64 to try to actually store a > uint64_t. Now that stdint.h exists, let's just use that here, > instead. > > v2: Fix alarm delta changes. 5.5/7 and 6 of 7 are 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 6/7 v2] sync: Convert from "CARD64" to int64_t.
The extension was using the name CARD64 to represent 64-bit values,
with a #define from CARD64 to XSyncValue, a struct with a pair of
32-bit values representing a signed 64-bit value. This interfered
with protocol headers using CARD64 to try to actually store a
uint64_t. Now that stdint.h exists, let's just use that here,
instead.
v2: Fix alarm delta changes.
Signed-off-by: Eric Anholt
---
Xext/sync.c | 302 +++-
Xext/syncsrv.h | 23 ++--
include/misc.h | 25
miext/sync/misync.c | 5 +-
miext/sync/misyncstr.h | 11 +-
present/present_fence.c | 2 +-
6 files changed, 186 insertions(+), 182 deletions(-)
diff --git a/Xext/sync.c b/Xext/sync.c
index a8db0ec22371..822c205a7a24 100644
--- a/Xext/sync.c
+++ b/Xext/sync.c
@@ -237,7 +237,7 @@ SyncAddTriggerToSyncObject(SyncTrigger * pTrigger)
*/
static Bool
-SyncCheckTriggerPositiveComparison(SyncTrigger * pTrigger, CARD64 oldval)
+SyncCheckTriggerPositiveComparison(SyncTrigger * pTrigger, int64_t oldval)
{
SyncCounter *pCounter;
@@ -248,12 +248,11 @@ SyncCheckTriggerPositiveComparison(SyncTrigger *
pTrigger, CARD64 oldval)
pCounter = (SyncCounter *) pTrigger->pSync;
-return (pCounter == NULL ||
-XSyncValueGreaterOrEqual(pCounter->value, pTrigger->test_value));
+return pCounter == NULL || pCounter->value >= pTrigger->test_value;
}
static Bool
-SyncCheckTriggerNegativeComparison(SyncTrigger * pTrigger, CARD64 oldval)
+SyncCheckTriggerNegativeComparison(SyncTrigger * pTrigger, int64_t oldval)
{
SyncCounter *pCounter;
@@ -264,12 +263,11 @@ SyncCheckTriggerNegativeComparison(SyncTrigger *
pTrigger, CARD64 oldval)
pCounter = (SyncCounter *) pTrigger->pSync;
-return (pCounter == NULL ||
-XSyncValueLessOrEqual(pCounter->value, pTrigger->test_value));
+return pCounter == NULL || pCounter->value <= pTrigger->test_value;
}
static Bool
-SyncCheckTriggerPositiveTransition(SyncTrigger * pTrigger, CARD64 oldval)
+SyncCheckTriggerPositiveTransition(SyncTrigger * pTrigger, int64_t oldval)
{
SyncCounter *pCounter;
@@ -281,12 +279,12 @@ SyncCheckTriggerPositiveTransition(SyncTrigger *
pTrigger, CARD64 oldval)
pCounter = (SyncCounter *) pTrigger->pSync;
return (pCounter == NULL ||
-(XSyncValueLessThan(oldval, pTrigger->test_value) &&
- XSyncValueGreaterOrEqual(pCounter->value, pTrigger->test_value)));
+(oldval < pTrigger->test_value &&
+ pCounter->value >= pTrigger->test_value));
}
static Bool
-SyncCheckTriggerNegativeTransition(SyncTrigger * pTrigger, CARD64 oldval)
+SyncCheckTriggerNegativeTransition(SyncTrigger * pTrigger, int64_t oldval)
{
SyncCounter *pCounter;
@@ -298,12 +296,12 @@ SyncCheckTriggerNegativeTransition(SyncTrigger *
pTrigger, CARD64 oldval)
pCounter = (SyncCounter *) pTrigger->pSync;
return (pCounter == NULL ||
-(XSyncValueGreaterThan(oldval, pTrigger->test_value) &&
- XSyncValueLessOrEqual(pCounter->value, pTrigger->test_value)));
+(oldval > pTrigger->test_value &&
+ pCounter->value <= pTrigger->test_value));
}
static Bool
-SyncCheckTriggerFence(SyncTrigger * pTrigger, CARD64 unused)
+SyncCheckTriggerFence(SyncTrigger * pTrigger, int64_t unused)
{
SyncFence *pFence = (SyncFence *) pTrigger->pSync;
@@ -389,16 +387,15 @@ SyncInitTrigger(ClientPtr client, SyncTrigger * pTrigger,
XID syncObject,
if (pTrigger->value_type == XSyncAbsolute)
pTrigger->test_value = pTrigger->wait_value;
else { /* relative */
-
Bool overflow;
if (pCounter == NULL)
return BadMatch;
-XSyncValueAdd(&pTrigger->test_value, pCounter->value,
- pTrigger->wait_value, &overflow);
+overflow = checked_int64_add(&pTrigger->test_value,
+ pCounter->value,
pTrigger->wait_value);
if (overflow) {
-client->errorValue = XSyncValueHigh32(pTrigger->wait_value);
+client->errorValue = pTrigger->wait_value >> 32;
return BadValue;
}
}
@@ -441,15 +438,15 @@ SyncSendAlarmNotifyEvents(SyncAlarm * pAlarm)
.type = SyncEventBase + XSyncAlarmNotify,
.kind = XSyncAlarmNotify,
.alarm = pAlarm->alarm_id,
-.alarm_value_hi = XSyncValueHigh32(pTrigger->test_value),
-.alarm_value_lo = XSyncValueLow32(pTrigger->test_value),
+.alarm_value_hi = pTrigger->test_value >> 32,
+.alarm_value_lo = pTrigger->test_value,
.time = currentTime.milliseconds,
.state = pAlarm->state
};
if (pTrigger->pSync && SYNC_COUNTER == pTrigger->pSync->type) {
-ane.counter_value_hi = XSyncValueHigh32(pCounter->value);
-ane.counter_value_lo = XSyncValueLow3
