On Wed, Aug 14, 2024 at 10:02:28AM +0300, Heikki Linnakangas wrote:
> On 14/08/2024 06:07, Nathan Bossart wrote:
>> And here's a new version of the patch in which I've attempted to fix the
>> silly mistakes.
>
> LGTM, just a few small comments:
Thanks for reviewing.
>> * - If a * b overflows, return true, otherwise store the result of a * b
>> * into *result. The content of *result is implementation defined in case of
>> * overflow.
>> + * - If -a overflows, return true, otherwise store the result of -a into
>> + * *result. The content of *result is implementation defined in case of
>> + * overflow.
>> + * - Return the absolute value of a as an unsigned integer of the same
>> + * width.
>> *---------
>> */
>
> The last "Return the absolute value of a ..." sentence feels a bit weird. In
> all the preceding sentences, 'a' is part of an "If a" sentence that defines
> what 'a' is. In the last one, it's kind of just hanging there.
How about:
If a is negative, return -a, otherwise return a. Overflow cannot occur
because the return value is an unsigned integer with the same width as
the argument.
>> +static inline uint16
>> +pg_abs_s16(int16 a)
>> +{
>> + return abs(a);
>> +}
>> +
>
> This is correct, but it took me a while to understand why. Maybe some
> comments would be in order.
>
> The function it calls is "int abs(int)". So this first widens the int16 to
> int32, and then narrows the result from int32 to uint16.
>
> The man page for abs() says "Trying to take the absolute value of the most
> negative integer is not defined." That's OK in this case, because that
> refers to the most negative int32 value, and the argument here is int16. But
> that's why the pg_abs_s64(int64) function needs the special check for the
> most negative value.
Yeah, I've added some casts/comments to make this clear. I got too excited
about trimming it down and ended up obfuscating these important details.
> There's also some code in libpq's pqCheckOutBufferSpace() function that
> could use these functions.
Duly noted.
--
nathan
>From 08b32a02629a7628353643960b9956d01a795f18 Mon Sep 17 00:00:00 2001
From: Joseph Koshakow <[email protected]>
Date: Sat, 8 Jun 2024 22:16:46 -0400
Subject: [PATCH v21 1/1] Remove dependence on integer wrapping
This commit updates various parts of the code to no longer rely on
integer wrapping for correctness. Not all compilers support -fwrapv, so
it's best not to rely on it.
---
src/backend/utils/adt/cash.c | 12 ++-
src/backend/utils/adt/numeric.c | 4 +-
src/backend/utils/adt/numutils.c | 34 ++++---
src/backend/utils/adt/timestamp.c | 28 +-----
src/include/common/int.h | 103 +++++++++++++++++++++
src/interfaces/ecpg/pgtypeslib/timestamp.c | 11 +--
src/test/regress/expected/timestamp.out | 13 +++
src/test/regress/expected/timestamptz.out | 13 +++
src/test/regress/sql/timestamp.sql | 4 +
src/test/regress/sql/timestamptz.sql | 4 +
10 files changed, 169 insertions(+), 57 deletions(-)
diff --git a/src/backend/utils/adt/cash.c b/src/backend/utils/adt/cash.c
index ec3c08acfc..c1a743b2a6 100644
--- a/src/backend/utils/adt/cash.c
+++ b/src/backend/utils/adt/cash.c
@@ -387,6 +387,7 @@ Datum
cash_out(PG_FUNCTION_ARGS)
{
Cash value = PG_GETARG_CASH(0);
+ uint64 uvalue;
char *result;
char buf[128];
char *bufptr;
@@ -429,8 +430,6 @@ cash_out(PG_FUNCTION_ARGS)
if (value < 0)
{
- /* make the amount positive for digit-reconstruction loop */
- value = -value;
/* set up formatting data */
signsymbol = (*lconvert->negative_sign != '\0') ?
lconvert->negative_sign : "-";
sign_posn = lconvert->n_sign_posn;
@@ -445,6 +444,9 @@ cash_out(PG_FUNCTION_ARGS)
sep_by_space = lconvert->p_sep_by_space;
}
+ /* make the amount positive for digit-reconstruction loop */
+ uvalue = pg_abs_s64(value);
+
/* we build the digits+decimal-point+sep string right-to-left in buf[]
*/
bufptr = buf + sizeof(buf) - 1;
*bufptr = '\0';
@@ -470,10 +472,10 @@ cash_out(PG_FUNCTION_ARGS)
memcpy(bufptr, ssymbol, strlen(ssymbol));
}
- *(--bufptr) = ((uint64) value % 10) + '0';
- value = ((uint64) value) / 10;
+ *(--bufptr) = (uvalue % 10) + '0';
+ uvalue = uvalue / 10;
digit_pos--;
- } while (value || digit_pos >= 0);
+ } while (uvalue || digit_pos >= 0);
/*----------
* Now, attach currency symbol and sign symbol in the correct order.
diff --git a/src/backend/utils/adt/numeric.c b/src/backend/utils/adt/numeric.c
index 763a7f4be0..5ee001e382 100644
--- a/src/backend/utils/adt/numeric.c
+++ b/src/backend/utils/adt/numeric.c
@@ -8111,7 +8111,7 @@ int64_to_numericvar(int64 val, NumericVar *var)
if (val < 0)
{
var->sign = NUMERIC_NEG;
- uval = -val;
+ uval = pg_abs_s64(val);
}
else
{
@@ -11437,7 +11437,7 @@ power_var_int(const NumericVar *base, int exp, int
exp_dscale,
* Now we can proceed with the multiplications.
*/
neg = (exp < 0);
- mask = abs(exp);
+ mask = pg_abs_s32(exp);
init_var(&base_prod);
set_var_from_var(base, &base_prod);
diff --git a/src/backend/utils/adt/numutils.c b/src/backend/utils/adt/numutils.c
index adc1e8a4cb..63c2beb6a2 100644
--- a/src/backend/utils/adt/numutils.c
+++ b/src/backend/utils/adt/numutils.c
@@ -18,6 +18,7 @@
#include <limits.h>
#include <ctype.h>
+#include "common/int.h"
#include "port/pg_bitutils.h"
#include "utils/builtins.h"
@@ -131,6 +132,7 @@ pg_strtoint16_safe(const char *s, Node *escontext)
uint16 tmp = 0;
bool neg = false;
unsigned char digit;
+ int16 result;
/*
* The majority of cases are likely to be base-10 digits without any
@@ -190,10 +192,9 @@ pg_strtoint16_safe(const char *s, Node *escontext)
if (neg)
{
- /* check the negative equivalent will fit without overflowing */
- if (unlikely(tmp > (uint16) (-(PG_INT16_MIN + 1)) + 1))
+ if (unlikely(pg_neg_u16_overflow(tmp, &result)))
goto out_of_range;
- return -((int16) tmp);
+ return result;
}
if (unlikely(tmp > PG_INT16_MAX))
@@ -333,10 +334,9 @@ slow:
if (neg)
{
- /* check the negative equivalent will fit without overflowing */
- if (tmp > (uint16) (-(PG_INT16_MIN + 1)) + 1)
+ if (unlikely(pg_neg_u16_overflow(tmp, &result)))
goto out_of_range;
- return -((int16) tmp);
+ return result;
}
if (tmp > PG_INT16_MAX)
@@ -393,6 +393,7 @@ pg_strtoint32_safe(const char *s, Node *escontext)
uint32 tmp = 0;
bool neg = false;
unsigned char digit;
+ int32 result;
/*
* The majority of cases are likely to be base-10 digits without any
@@ -452,10 +453,9 @@ pg_strtoint32_safe(const char *s, Node *escontext)
if (neg)
{
- /* check the negative equivalent will fit without overflowing */
- if (unlikely(tmp > (uint32) (-(PG_INT32_MIN + 1)) + 1))
+ if (unlikely(pg_neg_u32_overflow(tmp, &result)))
goto out_of_range;
- return -((int32) tmp);
+ return result;
}
if (unlikely(tmp > PG_INT32_MAX))
@@ -595,10 +595,9 @@ slow:
if (neg)
{
- /* check the negative equivalent will fit without overflowing */
- if (tmp > (uint32) (-(PG_INT32_MIN + 1)) + 1)
+ if (unlikely(pg_neg_u32_overflow(tmp, &result)))
goto out_of_range;
- return -((int32) tmp);
+ return result;
}
if (tmp > PG_INT32_MAX)
@@ -655,6 +654,7 @@ pg_strtoint64_safe(const char *s, Node *escontext)
uint64 tmp = 0;
bool neg = false;
unsigned char digit;
+ int64 result;
/*
* The majority of cases are likely to be base-10 digits without any
@@ -714,10 +714,9 @@ pg_strtoint64_safe(const char *s, Node *escontext)
if (neg)
{
- /* check the negative equivalent will fit without overflowing */
- if (unlikely(tmp > (uint64) (-(PG_INT64_MIN + 1)) + 1))
+ if (unlikely(pg_neg_u64_overflow(tmp, &result)))
goto out_of_range;
- return -((int64) tmp);
+ return result;
}
if (unlikely(tmp > PG_INT64_MAX))
@@ -857,10 +856,9 @@ slow:
if (neg)
{
- /* check the negative equivalent will fit without overflowing */
- if (tmp > (uint64) (-(PG_INT64_MIN + 1)) + 1)
+ if (unlikely(pg_neg_u64_overflow(tmp, &result)))
goto out_of_range;
- return -((int64) tmp);
+ return result;
}
if (tmp > PG_INT64_MAX)
diff --git a/src/backend/utils/adt/timestamp.c
b/src/backend/utils/adt/timestamp.c
index 69fe7860ed..43800addf4 100644
--- a/src/backend/utils/adt/timestamp.c
+++ b/src/backend/utils/adt/timestamp.c
@@ -618,19 +618,8 @@ make_timestamp_internal(int year, int month, int day,
time = (((hour * MINS_PER_HOUR + min) * SECS_PER_MINUTE)
* USECS_PER_SEC) + (int64) rint(sec * USECS_PER_SEC);
- result = date * USECS_PER_DAY + time;
- /* check for major overflow */
- if ((result - time) / USECS_PER_DAY != date)
- ereport(ERROR,
- (errcode(ERRCODE_DATETIME_VALUE_OUT_OF_RANGE),
- errmsg("timestamp out of range: %d-%02d-%02d
%d:%02d:%02g",
- year, month, day,
- hour, min, sec)));
-
- /* check for just-barely overflow (okay except time-of-day wraps) */
- /* caution: we want to allow 1999-12-31 24:00:00 */
- if ((result < 0 && date > 0) ||
- (result > 0 && date < -1))
+ if (unlikely(pg_mul_s64_overflow(date, USECS_PER_DAY, &result) ||
+ pg_add_s64_overflow(result, time, &result)))
ereport(ERROR,
(errcode(ERRCODE_DATETIME_VALUE_OUT_OF_RANGE),
errmsg("timestamp out of range: %d-%02d-%02d
%d:%02d:%02g",
@@ -2010,17 +1999,8 @@ tm2timestamp(struct pg_tm *tm, fsec_t fsec, int *tzp,
Timestamp *result)
date = date2j(tm->tm_year, tm->tm_mon, tm->tm_mday) -
POSTGRES_EPOCH_JDATE;
time = time2t(tm->tm_hour, tm->tm_min, tm->tm_sec, fsec);
- *result = date * USECS_PER_DAY + time;
- /* check for major overflow */
- if ((*result - time) / USECS_PER_DAY != date)
- {
- *result = 0; /* keep compiler quiet */
- return -1;
- }
- /* check for just-barely overflow (okay except time-of-day wraps) */
- /* caution: we want to allow 1999-12-31 24:00:00 */
- if ((*result < 0 && date > 0) ||
- (*result > 0 && date < -1))
+ if (unlikely(pg_mul_s64_overflow(date, USECS_PER_DAY, result) ||
+ pg_add_s64_overflow(*result, time, result)))
{
*result = 0; /* keep compiler quiet */
return -1;
diff --git a/src/include/common/int.h b/src/include/common/int.h
index 7fc046e78a..ead74d64d8 100644
--- a/src/include/common/int.h
+++ b/src/include/common/int.h
@@ -32,6 +32,12 @@
* - If a * b overflows, return true, otherwise store the result of a * b
* into *result. The content of *result is implementation defined in case of
* overflow.
+ * - If -a overflows, return true, otherwise store the result of -a into
+ * *result. The content of *result is implementation defined in case of
+ * overflow.
+ * - If a is negative, return -a, otherwise return a. Overflow cannot occur
+ * because the return value is an unsigned integer with the same type as the
+ * argument.
*---------
*/
@@ -97,6 +103,17 @@ pg_mul_s16_overflow(int16 a, int16 b, int16 *result)
#endif
}
+static inline uint16
+pg_abs_s16(int16 a)
+{
+ /*
+ * This first widens the argument from int16 to int32 for use with
abs().
+ * The result is then narrowed from int32 to uint16. This prevents any
+ * possibility of overflow.
+ */
+ return (uint16) abs((int32) a);
+}
+
/*
* INT32
*/
@@ -154,6 +171,17 @@ pg_mul_s32_overflow(int32 a, int32 b, int32 *result)
#endif
}
+static inline uint32
+pg_abs_s32(int32 a)
+{
+ /*
+ * This first widens the argument from int32 to int64 for use with
+ * i64abs(). The result is then narrowed from int64 to uint32. This
+ * prevents any possibility of overflow.
+ */
+ return (uint32) i64abs((int64) a);
+}
+
/*
* INT64
*/
@@ -258,6 +286,16 @@ pg_mul_s64_overflow(int64 a, int64 b, int64 *result)
#endif
}
+static inline uint64
+pg_abs_s64(int64 a)
+{
+ if (unlikely(a == PG_INT64_MIN))
+ return (uint64) PG_INT64_MAX + 1;
+ if (a < 0)
+ return -a;
+ return a;
+}
+
/*------------------------------------------------------------------------
* Overflow routines for unsigned integers
*------------------------------------------------------------------------
@@ -318,6 +356,24 @@ pg_mul_u16_overflow(uint16 a, uint16 b, uint16 *result)
#endif
}
+static inline bool
+pg_neg_u16_overflow(uint16 a, int16 *result)
+{
+#if defined(HAVE__BUILTIN_OP_OVERFLOW)
+ return __builtin_sub_overflow(0, a, result);
+#else
+ int32 res = -((int32) a);
+
+ if (unlikely(res < PG_INT16_MIN))
+ {
+ *result = 0x5EED; /* to avoid spurious warnings */
+ return true;
+ }
+ *result = res;
+ return false;
+#endif
+}
+
/*
* INT32
*/
@@ -373,6 +429,24 @@ pg_mul_u32_overflow(uint32 a, uint32 b, uint32 *result)
#endif
}
+static inline bool
+pg_neg_u32_overflow(uint32 a, int32 *result)
+{
+#if defined(HAVE__BUILTIN_OP_OVERFLOW)
+ return __builtin_sub_overflow(0, a, result);
+#else
+ int64 res = -((int64) a);
+
+ if (unlikely(res < PG_INT32_MIN))
+ {
+ *result = 0x5EED; /* to avoid spurious warnings */
+ return true;
+ }
+ *result = res;
+ return false;
+#endif
+}
+
/*
* UINT64
*/
@@ -438,6 +512,35 @@ pg_mul_u64_overflow(uint64 a, uint64 b, uint64 *result)
#endif
}
+static inline bool
+pg_neg_u64_overflow(uint64 a, int64 *result)
+{
+#if defined(HAVE__BUILTIN_OP_OVERFLOW)
+ return __builtin_sub_overflow(0, a, result);
+#elif defined(HAVE_INT128)
+ uint128 res = -((int128) a);
+
+ if (unlikely(res < PG_INT64_MIN))
+ {
+ *result = 0x5EED; /* to avoid spurious warnings */
+ return true;
+ }
+ *result = res;
+ return false;
+#else
+ if (unlikely(a > (uint64) PG_INT64_MAX + 1))
+ {
+ *result = 0x5EED; /* to avoid spurious warnings */
+ return true;
+ }
+ if (unlikely(a == (uint64) PG_INT64_MAX + 1))
+ *result = PG_INT64_MIN;
+ else
+ *result = -((int64) a);
+ return false;
+#endif
+}
+
/*------------------------------------------------------------------------
*
* Comparison routines for integer types.
diff --git a/src/interfaces/ecpg/pgtypeslib/timestamp.c
b/src/interfaces/ecpg/pgtypeslib/timestamp.c
index f1b143fbd2..402fae6da6 100644
--- a/src/interfaces/ecpg/pgtypeslib/timestamp.c
+++ b/src/interfaces/ecpg/pgtypeslib/timestamp.c
@@ -11,6 +11,7 @@
#error -ffast-math is known to break this code
#endif
+#include "common/int.h"
#include "dt.h"
#include "pgtypes_date.h"
#include "pgtypes_timestamp.h"
@@ -48,14 +49,8 @@ tm2timestamp(struct tm *tm, fsec_t fsec, int *tzp, timestamp
* result)
dDate = date2j(tm->tm_year, tm->tm_mon, tm->tm_mday) - date2j(2000, 1,
1);
time = time2t(tm->tm_hour, tm->tm_min, tm->tm_sec, fsec);
- *result = (dDate * USECS_PER_DAY) + time;
- /* check for major overflow */
- if ((*result - time) / USECS_PER_DAY != dDate)
- return -1;
- /* check for just-barely overflow (okay except time-of-day wraps) */
- /* caution: we want to allow 1999-12-31 24:00:00 */
- if ((*result < 0 && dDate > 0) ||
- (*result > 0 && dDate < -1))
+ if (unlikely(pg_mul_s64_overflow(dDate, USECS_PER_DAY, result) ||
+ pg_add_s64_overflow(*result, time, result)))
return -1;
if (tzp != NULL)
*result = dt2local(*result, -(*tzp));
diff --git a/src/test/regress/expected/timestamp.out
b/src/test/regress/expected/timestamp.out
index cf337da517..e287260051 100644
--- a/src/test/regress/expected/timestamp.out
+++ b/src/test/regress/expected/timestamp.out
@@ -2201,3 +2201,16 @@ select age(timestamp '-infinity', timestamp 'infinity');
select age(timestamp '-infinity', timestamp '-infinity');
ERROR: interval out of range
+-- test timestamp near POSTGRES_EPOCH_JDATE
+select timestamp '1999-12-31 24:00:00';
+ timestamp
+--------------------------
+ Sat Jan 01 00:00:00 2000
+(1 row)
+
+select make_timestamp(1999, 12, 31, 24, 0, 0);
+ make_timestamp
+--------------------------
+ Sat Jan 01 00:00:00 2000
+(1 row)
+
diff --git a/src/test/regress/expected/timestamptz.out
b/src/test/regress/expected/timestamptz.out
index bfb3825ff6..d01d174983 100644
--- a/src/test/regress/expected/timestamptz.out
+++ b/src/test/regress/expected/timestamptz.out
@@ -3286,3 +3286,16 @@ SELECT age(timestamptz '-infinity', timestamptz
'infinity');
SELECT age(timestamptz '-infinity', timestamptz '-infinity');
ERROR: interval out of range
+-- test timestamp near POSTGRES_EPOCH_JDATE
+select timestamptz '1999-12-31 24:00:00';
+ timestamptz
+------------------------------
+ Sat Jan 01 00:00:00 2000 PST
+(1 row)
+
+select make_timestamptz(1999, 12, 31, 24, 0, 0);
+ make_timestamptz
+------------------------------
+ Sat Jan 01 00:00:00 2000 PST
+(1 row)
+
diff --git a/src/test/regress/sql/timestamp.sql
b/src/test/regress/sql/timestamp.sql
index 820ef7752a..748469576d 100644
--- a/src/test/regress/sql/timestamp.sql
+++ b/src/test/regress/sql/timestamp.sql
@@ -424,3 +424,7 @@ select age(timestamp 'infinity', timestamp 'infinity');
select age(timestamp 'infinity', timestamp '-infinity');
select age(timestamp '-infinity', timestamp 'infinity');
select age(timestamp '-infinity', timestamp '-infinity');
+
+-- test timestamp near POSTGRES_EPOCH_JDATE
+select timestamp '1999-12-31 24:00:00';
+select make_timestamp(1999, 12, 31, 24, 0, 0);
diff --git a/src/test/regress/sql/timestamptz.sql
b/src/test/regress/sql/timestamptz.sql
index ccfd90d646..c71d5489b4 100644
--- a/src/test/regress/sql/timestamptz.sql
+++ b/src/test/regress/sql/timestamptz.sql
@@ -668,3 +668,7 @@ SELECT age(timestamptz 'infinity', timestamptz 'infinity');
SELECT age(timestamptz 'infinity', timestamptz '-infinity');
SELECT age(timestamptz '-infinity', timestamptz 'infinity');
SELECT age(timestamptz '-infinity', timestamptz '-infinity');
+
+-- test timestamp near POSTGRES_EPOCH_JDATE
+select timestamptz '1999-12-31 24:00:00';
+select make_timestamptz(1999, 12, 31, 24, 0, 0);
--
2.39.3 (Apple Git-146)
