Date: Tue, 13 Aug 2024 22:07:44 -0500
From: Nathan Bossart <nathandbossart@gmail.com>
To: jian he <jian.universality@gmail.com>
Cc: Joseph Koshakow <koshy44@gmail.com>,
	Matthew Kim <matthewkmkim@gmail.com>,
	Alexander Lakhin <exclusion@gmail.com>,
	Tom Lane <tgl@sss.pgh.pa.us>,
	PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>,
	Andres Freund <andres@anarazel.de>
Subject: Re: Remove dependence on integer wrapping
Message-ID: <ZrwfgNyDXy99NUOS@nathan>
References: <ZqBzEucmgeib_6Fe@nathan>
 <CAGzNdfGMe8vK1BRvUmW29T7rkqUhM67t=wbgsCSDBu=T+rqZrw@mail.gmail.com>
 <ZrKY8PJwGg23eZ3R@nathan>
 <ZrON1CT7Lk9_EJOG@nathan>
 <CAAvxfHckS2181hMcr+N2HehUa-HuN-fyJV3WGfth=8tSZGQb-g@mail.gmail.com>
 <CAGzNdfF-nbL=-2vP=pCynn0-2kwTt3wW96dctdJYveeOWrvY1A@mail.gmail.com>
 <CACJufxHNwqoXoP91a-2D3NytrurTSK_+-hd+XeVCYAbk2F0Uhw@mail.gmail.com>
 <CAAvxfHfuxiMG4_YB5oBmJtK7mQx6FRKcUhNueXGwKT6_r=4W_Q@mail.gmail.com>
 <CACJufxFUKTQx8wUpxtRPT31D+a7fNxAL_A_-S4XaR-oZkKEgxA@mail.gmail.com>
 <ZrvUOkL19KHEH2sb@nathan>
MIME-Version: 1.0
Content-Type: multipart/mixed; boundary="wKSJ1tGmrykJcEVT"
Content-Disposition: inline
In-Reply-To: <ZrvUOkL19KHEH2sb@nathan>
Archived-At: <https://www.postgresql.org/message-id/ZrwfgNyDXy99NUOS%40nathan>
Precedence: bulk


--wKSJ1tGmrykJcEVT
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline

On Tue, Aug 13, 2024 at 04:46:34PM -0500, Nathan Bossart wrote:
> I've been preparing 0001 for commit.  I've attached what I have so far.
> 
> The main changes are the implementations of pg_abs_* and pg_neg_*.  For the
> former, I've used abs()/i64abs() for the short/int implementations.  For
> the latter, I've tried to use __builtin_sub_overflow() when possible, as
> that appears to produce slightly better code.  When
> __builtin_sub_overflow() is not available, the values are upcasted before
> negation, and we check that result before casting to the return type.  That
> approach more closely matches the surrounding functions.  (One exception is
> pg_neg_u64_overflow() when we have neither HAVE__BUILTIN_OP_OVERFLOW nor
> HAVE_INT128.  In that case, we have to hand-roll everything.)

And here's a new version of the patch in which I've attempted to fix the
silly mistakes.

-- 
nathan

--wKSJ1tGmrykJcEVT
Content-Type: text/plain; charset=us-ascii
Content-Disposition: attachment;
	filename="v20-0001-Remove-dependence-on-integer-wrapping.patch"

From 2b7b38f764f16b49594d0f8cc6192b7ba74ef906 Mon Sep 17 00:00:00 2001
From: Joseph Koshakow <koshy44@gmail.com>
Date: Sat, 8 Jun 2024 22:16:46 -0400
Subject: [PATCH v20 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                   | 92 ++++++++++++++++++++++
 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, 158 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 d0f0923710..3d69e77998 100644
--- a/src/backend/utils/adt/numeric.c
+++ b/src/backend/utils/adt/numeric.c
@@ -8117,7 +8117,7 @@ int64_to_numericvar(int64 val, NumericVar *var)
 	if (val < 0)
 	{
 		var->sign = NUMERIC_NEG;
-		uval = -val;
+		uval = pg_abs_s64(val);
 	}
 	else
 	{
@@ -11443,7 +11443,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..a3d7d6bf01 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 (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 (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 (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 (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 (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 (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..c76793f72d 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 (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 (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..7a4dec4635 100644
--- a/src/include/common/int.h
+++ b/src/include/common/int.h
@@ -32,6 +32,11 @@
  * - 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.
  *---------
  */
 
@@ -97,6 +102,12 @@ pg_mul_s16_overflow(int16 a, int16 b, int16 *result)
 #endif
 }
 
+static inline uint16
+pg_abs_s16(int16 a)
+{
+	return abs(a);
+}
+
 /*
  * INT32
  */
@@ -154,6 +165,12 @@ pg_mul_s32_overflow(int32 a, int32 b, int32 *result)
 #endif
 }
 
+static inline uint32
+pg_abs_s32(int32 a)
+{
+	return i64abs(a);
+}
+
 /*
  * INT64
  */
@@ -258,6 +275,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 +345,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 +418,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 +501,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..93d4cc323d 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 (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)


--wKSJ1tGmrykJcEVT--