glibc __builtin_add_overflow problem with GCC < 7
Commit Message
Stefan Liebler's recent glibc commit "s390x: Require GCC 7.1 or later to
build glibc" is motivated by a bug with __builtin_add_overflow that he
encountered on s390x. However, it appears that the underlying GCC bug
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=98269
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=78559
is platform-independent (I am not sure of this, and am being conservative).
In Gnulib, Bruno Haible worked around the bug today by updating intprops.h
so that INT_ADD_WRAPV does not use __builtin_add_overflow in older GCCs.
When Gnulib intprops.h is copied to glibc this should fix glibc modules like
posix/regexec.c and time/mktime.c that are shared with Gnulib and use
intprops.h and INT_ADD_WRAPV etc.
However, several glibc modules (e.g., malloc/malloc.c) are not shared with
Gnulib and use __builtin_add_overflow directly, so they won't benefit from
the intprops.h fix.
A simple workaround would be to change INSTALL to recommend GCC 7.1 or
higher, as in the first attached patch (configure and sysdeps/s390/configure
files would also need to be regenerated). This patch also rejects icc, as
icc 2021.1 pretends to be GCC 10 but doesn't do proper integer overflow
checking.
Alternatively, if requiring GCC 7.1 is too drastic, glibc could change all
direct uses of __builtin_add_overflow etc. to use intprops.h and
INT_ADD_WRAPV etc., as Adhemerval suggested recently
<https://sourceware.org/pipermail/libc-alpha/2020-December/120855.html>. The
first step in this would be to update intprops.h from Gnulib (which should
be routine anyway), as in the second attached patch. We'd also need to
change malloc/malloc.c etc.
Comments
On 12/18/20 9:25 PM, Paul Eggert wrote:
> Stefan Liebler's recent glibc commit "s390x: Require GCC 7.1 or later to
> build glibc" is motivated by a bug with __builtin_add_overflow that he
> encountered on s390x. However, it appears that the underlying GCC bug
>
> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=98269
> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=78559
>
> is platform-independent (I am not sure of this, and am being conservative).
According to Andreas comment in the gcc-bugzilla 98269, the issue was
fixed with a common-code patch. On s390x this issue was hidden due to a
s390-specific patch (committed before the common-code fix).
Thus, I also think, that this bug could happen on all platforms, but I
don't know if this really happens anywhere.
The gcc-bugzilla 78559 (=> common-code patch) was reported on aarch64
for gcc > 6.2.1 && <= 7.0. (If you have an aarch64 system, you could try it)
I've just build and run tst-gcc-addoverflow.c (attached to the
gcc-bugzilla 98269) on x86_64 with
gcc version 6.5.0 20181026 (Ubuntu 6.5.0-2ubuntu1~18.04)
=> __builtin_add_overflow() SUCCEED
Florian Weimer added the usage of __builtin_add_overflow which fails
with gcc 6.5 on s390x. After reporting the issue, he said, that he did
not recognize the issue on other platforms. But I don't know which gcc
versions or platforms he has used for testing.
>
> In Gnulib, Bruno Haible worked around the bug today by updating
> intprops.h so that INT_ADD_WRAPV does not use __builtin_add_overflow in
> older GCCs. When Gnulib intprops.h is copied to glibc this should fix
> glibc modules like posix/regexec.c and time/mktime.c that are shared
> with Gnulib and use intprops.h and INT_ADD_WRAPV etc.
In the meantime, this already happened. See Adhemervals commit:
"Sync intprops.h with gnulib" (2021-01-04)
https://sourceware.org/git/?p=glibc.git;a=commit;h=11b2858bd153f6d68935bef74e48eaf6f2dda25e
>
> However, several glibc modules (e.g., malloc/malloc.c) are not shared
> with Gnulib and use __builtin_add_overflow directly, so they won't
> benefit from the intprops.h fix.
>
> A simple workaround would be to change INSTALL to recommend GCC 7.1 or
> higher, as in the first attached patch (configure and
> sysdeps/s390/configure files would also need to be regenerated). This
> patch also rejects icc, as icc 2021.1 pretends to be GCC 10 but doesn't
> do proper integer overflow checking.
On s390x perspective, I'm fine with requiring GCC 7.1.
For all other platforms, can somebody else (perhaps Joseph Myers) please
comment on requiring GCC 7.1 instead of GCC 6.2 and/or rejecting ICC?
>
> Alternatively, if requiring GCC 7.1 is too drastic, glibc could change
> all direct uses of __builtin_add_overflow etc. to use intprops.h and
> INT_ADD_WRAPV etc., as Adhemerval suggested recently
> <https://sourceware.org/pipermail/libc-alpha/2020-December/120855.html>.
> The first step in this would be to update intprops.h from Gnulib (which
> should be routine anyway), as in the second attached patch. We'd also
> need to change malloc/malloc.c etc.
Thanks,
Stefan
On Thu, 7 Jan 2021, Stefan Liebler via Libc-alpha wrote:
> On s390x perspective, I'm fine with requiring GCC 7.1.
> For all other platforms, can somebody else (perhaps Joseph Myers) please
> comment on requiring GCC 7.1 instead of GCC 6.2 and/or rejecting ICC?
Typically the minimum version of a tool for building glibc is increased
when the new version is old enough *and* the increase allows significant
cleanups in glibc. In this case there are quite a lot of __GNUC_PREREQ
conditionals that could be removed by requiring GCC 7 (but you can't
remove those in installed headers, of which there are also quite a lot,
and increasing the minimum does make it harder to test the older-GCC
conditionals in installed headers, since we don't have any way in the
glibc testsuite to test header functionality with older-GCC and non-GCC
compilers; <tgmath.h> is a specific case where GCC 6, 7 and >= 8 all use
significantly different header code).
* Stefan Liebler via Libc-alpha:
> On s390x perspective, I'm fine with requiring GCC 7.1.
> For all other platforms, can somebody else (perhaps Joseph Myers) please
> comment on requiring GCC 7.1 instead of GCC 6.2 and/or rejecting ICC?
I do not have any concerns about requiring GCC 7 everywhere. It's fine
for Fedora and Red Hat Enterprise Linux.
I don't think the difference matters for Ubuntu users, either. 16.04
LTS has GCC 5.3, so it's already too old. 18.04 TLS has GCC 7.3 or
later, so it aligns with the new requirement.
ICC does not matter for the glibc side because the code is internal to
glibc, and ICC cannot be used to build glibc anyway.
Thanks,
Florian
From c31fe81a71ba7e0dfb13ceeb602b453c73cd7384 Mon Sep 17 00:00:00 2001
From: Paul Eggert <eggert@cs.ucla.edu>
Date: Fri, 18 Dec 2020 12:10:16 -0800
Subject: [PATCH] Sync intprops.h from Gnulib
* include/intprops.h: Sync from Gnulib.
This fixes a bug with INT_ADD_WRAPV and GCC < 7.
---
include/intprops.h | 70 ++++++++++++++++++++++++++++++++++++++--------
1 file changed, 58 insertions(+), 12 deletions(-)
@@ -48,7 +48,7 @@
/* Minimum and maximum values for integer types and expressions. */
/* The width in bits of the integer type or expression T.
- Do not evaluate T.
+ Do not evaluate T. T must not be a bit-field expression.
Padding bits are not supported; this is checked at compile-time below. */
#define TYPE_WIDTH(t) (sizeof (t) * CHAR_BIT)
@@ -70,7 +70,7 @@
? _GL_SIGNED_INT_MAXIMUM (e) \
: _GL_INT_NEGATE_CONVERT (e, 1))
#define _GL_SIGNED_INT_MAXIMUM(e) \
- (((_GL_INT_CONVERT (e, 1) << (TYPE_WIDTH ((e) + 0) - 2)) - 1) * 2 + 1)
+ (((_GL_INT_CONVERT (e, 1) << (TYPE_WIDTH (+ (e)) - 2)) - 1) * 2 + 1)
/* Work around OpenVMS incompatibility with C99. */
#if !defined LLONG_MAX && defined __INT64_MAX
@@ -86,6 +86,7 @@
/* Does the __typeof__ keyword work? This could be done by
'configure', but for now it's easier to do it by hand. */
#if (2 <= __GNUC__ \
+ || (4 <= __clang_major__) \
|| (1210 <= __IBMC__ && defined __IBM__TYPEOF__) \
|| (0x5110 <= __SUNPRO_C && !__STDC__))
# define _GL_HAVE___TYPEOF__ 1
@@ -94,8 +95,9 @@
#endif
/* Return 1 if the integer type or expression T might be signed. Return 0
- if it is definitely unsigned. This macro does not evaluate its argument,
- and expands to an integer constant expression. */
+ if it is definitely unsigned. T must not be a bit-field expression.
+ This macro does not evaluate its argument, and expands to an
+ integer constant expression. */
#if _GL_HAVE___TYPEOF__
# define _GL_SIGNED_TYPE_OR_EXPR(t) TYPE_SIGNED (__typeof__ (t))
#else
@@ -108,6 +110,8 @@
#define INT_BITS_STRLEN_BOUND(b) (((b) * 146 + 484) / 485)
/* Bound on length of the string representing an integer type or expression T.
+ T must not be a bit-field expression.
+
Subtract 1 for the sign bit if T is signed, and then add 1 more for
a minus sign if needed.
@@ -119,7 +123,7 @@
+ _GL_SIGNED_TYPE_OR_EXPR (t))
/* Bound on buffer size needed to represent an integer type or expression T,
- including the terminating null. */
+ including the terminating null. T must not be a bit-field expression. */
#define INT_BUFSIZE_BOUND(t) (INT_STRLEN_BOUND (t) + 1)
@@ -222,7 +226,9 @@
/* True if __builtin_add_overflow (A, B, P) and __builtin_sub_overflow
(A, B, P) work when P is non-null. */
-#if 5 <= __GNUC__ && !defined __ICC
+/* __builtin_{add,sub}_overflow exists but is not reliable in GCC 5.x and 6.x,
+ see <https://gcc.gnu.org/bugzilla/show_bug.cgi?id=98269>. */
+#if 7 <= __GNUC__ && !defined __ICC
# define _GL_HAS_BUILTIN_ADD_OVERFLOW 1
#elif defined __has_builtin
# define _GL_HAS_BUILTIN_ADD_OVERFLOW __has_builtin (__builtin_add_overflow)
@@ -239,8 +245,18 @@
#endif
/* True if __builtin_add_overflow_p (A, B, C) works, and similarly for
- __builtin_mul_overflow_p and __builtin_mul_overflow_p. */
-#define _GL_HAS_BUILTIN_OVERFLOW_P (7 <= __GNUC__)
+ __builtin_sub_overflow_p and __builtin_mul_overflow_p. */
+#if defined __clang__ || defined __ICC
+/* Clang 11 lacks __builtin_mul_overflow_p, and even if it did it
+ would presumably run afoul of Clang bug 16404. ICC 2021.1's
+ __builtin_add_overflow_p etc. are not treated as integral constant
+ expressions even when all arguments are. */
+# define _GL_HAS_BUILTIN_OVERFLOW_P 0
+#elif defined __has_builtin
+# define _GL_HAS_BUILTIN_OVERFLOW_P __has_builtin (__builtin_mul_overflow_p)
+#else
+# define _GL_HAS_BUILTIN_OVERFLOW_P (7 <= __GNUC__)
+#endif
/* The _GL*_OVERFLOW macros have the same restrictions as the
*_RANGE_OVERFLOW macros, except that they do not assume that operands
@@ -373,8 +389,9 @@
_GL_INT_OP_WRAPV (a, b, r, -, _GL_INT_SUBTRACT_RANGE_OVERFLOW)
#endif
#if _GL_HAS_BUILTIN_MUL_OVERFLOW
-# if (9 < __GNUC__ + (3 <= __GNUC_MINOR__) \
- || (__GNUC__ == 8 && 4 <= __GNUC_MINOR__))
+# if ((9 < __GNUC__ + (3 <= __GNUC_MINOR__) \
+ || (__GNUC__ == 8 && 4 <= __GNUC_MINOR__)) \
+ && !defined __ICC)
# define INT_MULTIPLY_WRAPV(a, b, r) __builtin_mul_overflow (a, b, r)
# else
/* Work around GCC bug 91450. */
@@ -395,7 +412,7 @@
For now, assume all versions of GCC-like compilers generate bogus
warnings for _Generic. This matters only for compilers that
lack relevant builtins. */
-#if __GNUC__
+#if __GNUC__ || defined __clang__
# define _GL__GENERIC_BOGUS 1
#else
# define _GL__GENERIC_BOGUS 0
@@ -565,7 +582,7 @@
? (EXPR_SIGNED (_GL_INT_CONVERT (tmax, b)) \
? (a) < (tmax) / (b) \
: ((INT_NEGATE_OVERFLOW (b) \
- ? _GL_INT_CONVERT (b, tmax) >> (TYPE_WIDTH (b) - 1) \
+ ? _GL_INT_CONVERT (b, tmax) >> (TYPE_WIDTH (+ (b)) - 1) \
: (tmax) / -(b)) \
<= -1 - (a))) \
: INT_NEGATE_OVERFLOW (_GL_INT_CONVERT (b, tmin)) && (b) == -1 \
@@ -581,4 +598,33 @@
: (tmin) / (a) < (b)) \
: (tmax) / (b) < (a)))
+/* The following macros compute A + B, A - B, and A * B, respectively.
+ If no overflow occurs, they set *R to the result and return 1;
+ otherwise, they return 0 and may modify *R.
+
+ Example usage:
+
+ long int result;
+ if (INT_ADD_OK (a, b, &result))
+ printf ("result is %ld\n", result);
+ else
+ printf ("overflow\n");
+
+ A, B, and *R should be integers; they need not be the same type,
+ and they need not be all signed or all unsigned.
+
+ These macros work correctly on all known practical hosts, and do not rely
+ on undefined behavior due to signed arithmetic overflow.
+
+ These macros are not constant expressions.
+
+ These macros may evaluate their arguments zero or multiple times, so the
+ arguments should not have side effects.
+
+ These macros are tuned for B being a constant. */
+
+#define INT_ADD_OK(a, b, r) ! INT_ADD_WRAPV (a, b, r)
+#define INT_SUBTRACT_OK(a, b, r) ! INT_SUBTRACT_WRAPV (a, b, r)
+#define INT_MULTIPLY_OK(a, b, r) ! INT_MULTIPLY_WRAPV (a, b, r)
+
#endif /* _GL_INTPROPS_H */
--
2.29.2