diff mbox

[C/C++] Implement -Wshift-negative-value (PR c/65179)

Message ID 554D309E.4050605@cs.ucla.edu
State New
Headers show

Commit Message

Paul Eggert May 8, 2015, 9:54 p.m. UTC 
On 05/08/2015 09:59 AM, Joseph Myers wrote:
> Paul, although glibc's copy of parts of tzcode is a bit out of date, it
> looks like the currenthttps://github.com/eggert/tz.git  still has the
> problematic code in private.h, relying on left-shifting -1 which has
> undefined behavior in C99/C11 (implementation-defined in C90, as per
> DR#081).
Thanks for reporting that.  I installed the attached patch into the 
experimental tz version on github <https://github.com/eggert/tz>, with 
the intent that this fix propagate into the next tz release and thus 
into glibc etc.

Comments

Steve Ellcey May 11, 2015, 10:18 p.m. UTC | #1 
On Fri, 2015-05-08 at 14:54 -0700, Paul Eggert wrote:
> On 05/08/2015 09:59 AM, Joseph Myers wrote:
> > Paul, although glibc's copy of parts of tzcode is a bit out of date, it
> > looks like the currenthttps://github.com/eggert/tz.git  still has the
> > problematic code in private.h, relying on left-shifting -1 which has
> > undefined behavior in C99/C11 (implementation-defined in C90, as per
> > DR#081).
> Thanks for reporting that.  I installed the attached patch into the 
> experimental tz version on github <https://github.com/eggert/tz>, with 
> the intent that this fix propagate into the next tz release and thus 
> into glibc etc.

FYI: I put this into my glibc sources (the private.h, zdump.c, and zic.c
parts, glibc doesn't have localtime.c in the timezone directory and the
one in the time directory doesn't look like it matches what was
patched).  And my GCC/glibc toolchain now builds.  I didn't run the
glibc testsuite but I ran the GCC testsuite using this patch and
everything looked fine.

Steve Ellcey
sellcey@imgtec.com
diff mbox

Patch

From b8f4f998104e74fc2c4a3759317b5153e95db16e Mon Sep 17 00:00:00 2001
From: Paul Eggert <eggert@cs.ucla.edu>
Date: Fri, 8 May 2015 14:47:40 -0700
Subject: [PATCH] Avoid left-shift-into-sign-bit undefined behavior

Problem reported by Joseph Myers in:
https://gcc.gnu.org/ml/gcc-patches/2015-05/msg00704.html
* localtime.c (detzcode, detzcode64): Don't rely on
undefined behavior with left shift into sign bit.
Port better to non-2's-complement machines.
* private.h (TWOS_COMPLEMENT, MAXVAL, MINVAL): New macros.
* private.h (time_t_min, time_t_max):
* zic.c (min_time, max_time): Use them to avoid undefined behavior.
* zdump.c (atime_shift): New constant.
(absolute_min_time, absolute_max_time):
Use it to avoid undefined behavior.
---
 localtime.c | 32 +++++++++++++++++++++++++++-----
 private.h   | 23 ++++++++++++++---------
 zdump.c     |  6 ++++--
 zic.c       |  4 ++--
 4 files changed, 47 insertions(+), 18 deletions(-)

diff --git a/localtime.c b/localtime.c
index a2d9aa8..423e13e 100644
--- a/localtime.c
+++ b/localtime.c
@@ -216,22 +216,44 @@  detzcode(const char *const codep)
 {
 	register int_fast32_t	result;
 	register int		i;
+	int_fast32_t one = 1;
+	int_fast32_t halfmaxval = one << (32 - 2);
+	int_fast32_t maxval = halfmaxval - 1 + halfmaxval;
+	int_fast32_t minval = -1 - maxval;
 
-	result = (codep[0] & 0x80) ? -1 : 0;
-	for (i = 0; i < 4; ++i)
+	result = codep[0] & 0x7f;
+	for (i = 1; i < 4; ++i)
 		result = (result << 8) | (codep[i] & 0xff);
+
+	if (codep[0] & 0x80) {
+	  /* Do two's-complement negation even on non-two's-complement machines.
+	     If the result would be minval - 1, return minval.  */
+	  result -= !TWOS_COMPLEMENT(int_fast32_t) && result != 0;
+	  result += minval;
+	}
 	return result;
 }
 
 static int_fast64_t
 detzcode64(const char *const codep)
 {
-	register int_fast64_t result;
+	register uint_fast64_t result;
 	register int	i;
+	int_fast64_t one = 1;
+	int_fast64_t halfmaxval = one << (64 - 2);
+	int_fast64_t maxval = halfmaxval - 1 + halfmaxval;
+	int_fast64_t minval = -TWOS_COMPLEMENT(int_fast64_t) - maxval;
 
-	result = (codep[0] & 0x80) ? -1 : 0;
-	for (i = 0; i < 8; ++i)
+	result = codep[0] & 0x7f;
+	for (i = 1; i < 8; ++i)
 		result = (result << 8) | (codep[i] & 0xff);
+
+	if (codep[0] & 0x80) {
+	  /* Do two's-complement negation even on non-two's-complement machines.
+	     If the result would be minval - 1, return minval.  */
+	  result -= !TWOS_COMPLEMENT(int_fast64_t) && result != 0;
+	  result += minval;
+	}
 	return result;
 }
 
diff --git a/private.h b/private.h
index 61cf922..f277e7a 100644
--- a/private.h
+++ b/private.h
@@ -472,15 +472,20 @@  time_t time2posix_z(timezone_t, time_t) ATTRIBUTE_PURE;
 #define TYPE_SIGNED(type) (((type) -1) < 0)
 #endif /* !defined TYPE_SIGNED */
 
-/* The minimum and maximum finite time values.  */
-static time_t const time_t_min =
-  (TYPE_SIGNED(time_t)
-   ? (time_t) -1 << (CHAR_BIT * sizeof (time_t) - 1)
-   : 0);
-static time_t const time_t_max =
-  (TYPE_SIGNED(time_t)
-   ? - (~ 0 < 0) - ((time_t) -1 << (CHAR_BIT * sizeof (time_t) - 1))
-   : -1);
+#define TWOS_COMPLEMENT(t) ((t) ~ (t) 0 < 0)
+
+/* Max and min values of the integer type T, of which only the bottom
+   B bits are used, and where the highest-order used bit is considered
+   to be a sign bit if T is signed.  */
+#define MAXVAL(t, b)						\
+  ((t) (((t) 1 << ((b) - 1 - TYPE_SIGNED(t)))			\
+	- 1 + ((t) 1 << ((b) - 1 - TYPE_SIGNED(t)))))
+#define MINVAL(t, b)						\
+  ((t) (TYPE_SIGNED(t) ? - TWOS_COMPLEMENT(t) - MAXVAL(t, b) : 0))
+
+/* The minimum and maximum finite time values.  This assumes no padding.  */
+static time_t const time_t_min = MINVAL(time_t, TYPE_BIT(time_t));
+static time_t const time_t_max = MAXVAL(time_t, TYPE_BIT(time_t));
 
 #ifndef INT_STRLEN_MAXIMUM
 /*
diff --git a/zdump.c b/zdump.c
index 13bbb0e..adb806c 100644
--- a/zdump.c
+++ b/zdump.c
@@ -246,13 +246,15 @@  extern int	optind;
 extern char *	tzname[2];
 
 /* The minimum and maximum finite time values.  */
+enum { atime_shift = CHAR_BIT * sizeof (time_t) - 2 };
 static time_t const absolute_min_time =
   ((time_t) -1 < 0
-   ? (time_t) -1 << (CHAR_BIT * sizeof (time_t) - 1)
+   ? (- ((time_t) ~ (time_t) 0 < 0)
+      - (((time_t) 1 << atime_shift) - 1 + ((time_t) 1 << atime_shift)))
    : 0);
 static time_t const absolute_max_time =
   ((time_t) -1 < 0
-   ? - (~ 0 < 0) - ((time_t) -1 << (CHAR_BIT * sizeof (time_t) - 1))
+   ? (((time_t) 1 << atime_shift) - 1 + ((time_t) 1 << atime_shift))
    : -1);
 static int	longest;
 static char *	progname;
diff --git a/zic.c b/zic.c
index 636649b..52b45ad 100644
--- a/zic.c
+++ b/zic.c
@@ -813,8 +813,8 @@  warning(_("hard link failed, symbolic link used"));
 
 #define TIME_T_BITS_IN_FILE	64
 
-static const zic_t min_time = (zic_t) -1 << (TIME_T_BITS_IN_FILE - 1);
-static const zic_t max_time = -1 - ((zic_t) -1 << (TIME_T_BITS_IN_FILE - 1));
+static zic_t const min_time = MINVAL (zic_t, TIME_T_BITS_IN_FILE);
+static zic_t const max_time = MAXVAL (zic_t, TIME_T_BITS_IN_FILE);
 
 /* Estimated time of the Big Bang, in seconds since the POSIX epoch.
    rounded downward to the negation of a power of two that is
-- 
2.1.0