Re: __builtin_add_overflow on clang

[Paul Eggert <eggert@cs.ucla.edu>]

[-- Attachment #1: Type: text/plain, Size: 2445 bytes --]

Mattias Engdegård wrote:
> On macOS (and *BSD, probably), compiler-rt is linked by default so the problem never arises there. Much as I dislike them in general, perhaps a configure test would be appropriate?

I've been avoiding configure-time tests for intprops.h, since we haven't needed 
them yet and intprops.h is occasionally used outside Gnulib. And strictly 
speaking we don't need the tests here, as the fallback code works correctly for 
Clang (albeit most likely not as efficiently).

The reason I patched Gnulib in 2017 to work around the Clang bug:

https://git.savannah.gnu.org/cgit/gnulib.git/commit/?id=04441fd8156978cfc51d6c203fd43c23b64f95df

was in response to a private bug report for GNU 'sed' builds from Nelson H.F. 
Beebe, who reported finding the Clang bug on these platforms:

>     alpine3           dragonflybsd47    mageia6           salix142
>     alpine343         fedorarh          mint18            slackware142
>     alpine35          fedorarh          mint181           slackware142x
>     beta              gentoo            openbsd60         solus12
>     clearos7          grousecreek       opensusetw        ubuntu1610
>     dragonflybsd46    kali2             pclinuxos         ubunturr
>     dragonflybsd461   mageia6           pclinuxosb        void64
>     dragonflybsd46b

These failures occurred with ordinary integer types, as sed doesn't use 128-bit 
types.

Beebe also wrote:

> I fear that others might hit the problem, and because Mac OS X
> and recent FreeBSD have switched to clang as the default C compiler,
> the same problem might start showing up on those platforms.

Given the prevalence of this Clang bug, and the iffiness of whether the bug 
might occur on macOS or FreeBSD, I was cautious and stuck with the fallback code 
for Clang. However, now that you mention it this problem seems limited to 
multiplication and it's easy to change intprops.h to use Clang's 
__builtin_add_overflow and __builtin_sub_overflow even if its 
__builtin_mul_overflow is buggy. So I installed the attached patch into Gnulib 
to do that.

If we definitely know that this bug cannot occur on FreeBSD and/or macOS, I 
suppose we could add further tests on the appropriate preprocessor macros. But 
that would require further investigation as to what packages are available in 
these platforms and whether the relevant clang runtimes can be installed separately.

[-- Attachment #2: 0001-intprops.h-verify.h-port-better-to-clang.txt --]
[-- Type: text/plain, Size: 10981 bytes --]

From b8ae85960f9d20b41e6e15de104b38da2ac40e0f Mon Sep 17 00:00:00 2001
From: Paul Eggert <eggert@cs.ucla.edu>
Date: Sun, 25 Aug 2019 19:29:15 -0700
Subject: [PATCH] intprops.h, verify.h: port better to clang
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

Improve code generated by INT_ADD_WRAPV and INT_SUBTRACT_WRAPV
with Clang.  Problem reported privately by Mattias Engdegård.
Also, insulate intprops.h and verify.h better against each other’s
definitions of __has_builtin on non-Clang hosts.
* lib/intprops.h (__has_builtin): Define a temporary substitute
if __has_builtin is not already defined.
(_GL_HAS___builtin_add_overflow, _GL_TEMPDEF___has_builtin):
New temporary internal macros.
(_GL_HAS_BUILTIN_ADD_OVERFLOW, _GL_HAS_BUILTIN_MUL_OVERFLOW):
Now two separate macros, replacing the old
_GL_HAS_BUILTIN_OVERFLOW, since we no longer assume that
__builtin_mul_overflow is like the rest.  All uses changed.
(INT_ADD_WRAPV, INT_SUBTRACT_WRAPV, INT_MULTIPLY_WRAPV):
Adjust to above changes.
(_GL_INT_OP_WRAPV): Remove ‘builtin’ arg, since it’s no
longer relevant.  All uses changed.
* lib/verify.h (__has_builtin): Treat like intprops.h,
so that the two .h files do not collide with each other.
(_GL_HAS___builtin_unreachable, _GL_HAS___builtin_trap)
(_GL_TEMPDEF___has_builtin): New temporary internal macros.
---
 ChangeLog      | 24 +++++++++++++
 lib/intprops.h | 93 ++++++++++++++++++++++++++++++++------------------
 lib/verify.h   | 28 ++++++++++-----
 3 files changed, 103 insertions(+), 42 deletions(-)

diff --git a/ChangeLog b/ChangeLog
index f7fbcf413..e6f5a096a 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,27 @@
+2019-08-25  Paul Eggert  <eggert@cs.ucla.edu>
+
+	intprops.h, verify.h: port better to clang
+	Improve code generated by INT_ADD_WRAPV and INT_SUBTRACT_WRAPV
+	with Clang.  Problem reported privately by Mattias Engdegård.
+	Also, insulate intprops.h and verify.h better against each other’s
+	definitions of __has_builtin on non-Clang hosts.
+	* lib/intprops.h (__has_builtin): Define a temporary substitute
+	if __has_builtin is not already defined.
+	(_GL_HAS___builtin_add_overflow, _GL_TEMPDEF___has_builtin):
+	New temporary internal macros.
+	(_GL_HAS_BUILTIN_ADD_OVERFLOW, _GL_HAS_BUILTIN_MUL_OVERFLOW):
+	Now two separate macros, replacing the old
+	_GL_HAS_BUILTIN_OVERFLOW, since we no longer assume that
+	__builtin_mul_overflow is like the rest.  All uses changed.
+	(INT_ADD_WRAPV, INT_SUBTRACT_WRAPV, INT_MULTIPLY_WRAPV):
+	Adjust to above changes.
+	(_GL_INT_OP_WRAPV): Remove ‘builtin’ arg, since it’s no
+	longer relevant.  All uses changed.
+	* lib/verify.h (__has_builtin): Treat like intprops.h,
+	so that the two .h files do not collide with each other.
+	(_GL_HAS___builtin_unreachable, _GL_HAS___builtin_trap)
+	(_GL_TEMPDEF___has_builtin): New temporary internal macros.
+
 2019-08-24  Paul Eggert  <eggert@cs.ucla.edu>
 
 	intprops: say why not Clang __builtin_add_overflow
diff --git a/lib/intprops.h b/lib/intprops.h
index fbbc3cffe..ffd737028 100644
--- a/lib/intprops.h
+++ b/lib/intprops.h
@@ -22,6 +22,18 @@
 
 #include <limits.h>
 
+/* If the compiler lacks __has_builtin, define it well enough for this
+   source file only.  */
+#ifndef __has_builtin
+# define __has_builtin(x) _GL_HAS_##x
+# if 5 <= __GNUC__ && !defined __ICC
+#  define _GL_HAS___builtin_add_overflow 1
+# else
+#  define _GL_HAS___builtin_add_overflow 0
+# endif
+# define _GL_TEMPDEF___has_builtin
+#endif
+
 /* Return a value with the common real type of E and V and the value of V.
    Do not evaluate E.  */
 #define _GL_INT_CONVERT(e, v) ((1 ? 0 : (e)) + (v))
@@ -220,16 +232,24 @@
    ? (a) < (min) >> (b)                                 \
    : (max) >> (b) < (a))
 
-/* True if __builtin_add_overflow (A, B, P) works when P is non-null.
-   See <https://bugs.llvm.org/show_bug.cgi?id=16404> for why this is
-   false for Clang.  */
-#if 5 <= __GNUC__ && !defined __ICC
-# define _GL_HAS_BUILTIN_OVERFLOW 1
+/* True if __builtin_add_overflow (A, B, P) and __builtin_sub_overflow
+   (A, B, P) work when P is non-null.  */
+#if __has_builtin (__builtin_add_overflow)
+# define _GL_HAS_BUILTIN_ADD_OVERFLOW 1
+#else
+# define _GL_HAS_BUILTIN_ADD_OVERFLOW 0
+#endif
+
+/* True if __builtin_mul_overflow (A, B, P) works when P is non-null.  */
+#ifdef __clang__
+/* Work around Clang bug <https://bugs.llvm.org/show_bug.cgi?id=16404>.  */
+# define _GL_HAS_BUILTIN_MUL_OVERFLOW 0
 #else
-# define _GL_HAS_BUILTIN_OVERFLOW 0
+# define _GL_HAS_BUILTIN_MUL_OVERFLOW _GL_HAS_BUILTIN_ADD_OVERFLOW
 #endif
 
-/* True if __builtin_add_overflow_p (A, B, C) works.  */
+/* 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__)
 
 /* The _GL*_OVERFLOW macros have the same restrictions as the
@@ -353,29 +373,33 @@
 
 /* Store the low-order bits of A + B, A - B, A * B, respectively, into *R.
    Return 1 if the result overflows.  See above for restrictions.  */
-#define INT_ADD_WRAPV(a, b, r) \
-  _GL_INT_OP_WRAPV (a, b, r, +, __builtin_add_overflow, \
-                    _GL_INT_ADD_RANGE_OVERFLOW)
-#define INT_SUBTRACT_WRAPV(a, b, r) \
-  _GL_INT_OP_WRAPV (a, b, r, -, __builtin_sub_overflow, \
-                    _GL_INT_SUBTRACT_RANGE_OVERFLOW)
-#define INT_MULTIPLY_WRAPV(a, b, r) \
-   _GL_INT_OP_WRAPV (a, b, r, *, _GL_BUILTIN_MUL_OVERFLOW, \
-                     _GL_INT_MULTIPLY_RANGE_OVERFLOW)
-
-/* Like __builtin_mul_overflow, but work around GCC bug 91450.  */
-#define _GL_BUILTIN_MUL_OVERFLOW(a, b, r) \
-  ((!_GL_SIGNED_TYPE_OR_EXPR (*(r)) && EXPR_SIGNED (a) && EXPR_SIGNED (b) \
-    && _GL_INT_MULTIPLY_RANGE_OVERFLOW (a, b, 0, (__typeof__ (*(r))) -1)) \
-   ? ((void) __builtin_mul_overflow (a, b, r), 1) \
-   : __builtin_mul_overflow (a, b, r))
+#if _GL_HAS_BUILTIN_ADD_OVERFLOW
+# define INT_ADD_WRAPV(a, b, r) __builtin_add_overflow (a, b, r)
+# define INT_SUBTRACT_WRAPV(a, b, r) __builtin_sub_overflow (a, b, r)
+#else
+# define INT_ADD_WRAPV(a, b, r) \
+   _GL_INT_OP_WRAPV (a, b, r, +, _GL_INT_ADD_RANGE_OVERFLOW)
+# define INT_SUBTRACT_WRAPV(a, b, r) \
+   _GL_INT_OP_WRAPV (a, b, r, -, _GL_INT_SUBTRACT_RANGE_OVERFLOW)
+#endif
+#if _GL_HAS_BUILTIN_MUL_OVERFLOW
+/* Work around GCC bug 91450.  */
+# define INT_MULTIPLY_WRAPV(a, b, r) \
+   ((!_GL_SIGNED_TYPE_OR_EXPR (*(r)) && EXPR_SIGNED (a) && EXPR_SIGNED (b) \
+     && _GL_INT_MULTIPLY_RANGE_OVERFLOW (a, b, 0, (__typeof__ (*(r))) -1)) \
+    ? ((void) __builtin_mul_overflow (a, b, r), 1) \
+    : __builtin_mul_overflow (a, b, r))
+#else
+# define INT_MULTIPLY_WRAPV(a, b, r) \
+   _GL_INT_OP_WRAPV (a, b, r, *, _GL_INT_MULTIPLY_RANGE_OVERFLOW)
+#endif
 
 /* Nonzero if this compiler has GCC bug 68193 or Clang bug 25390.  See:
    https://gcc.gnu.org/bugzilla/show_bug.cgi?id=68193
    https://llvm.org/bugs/show_bug.cgi?id=25390
    For now, assume all versions of GCC-like compilers generate bogus
-   warnings for _Generic.  This matters only for older compilers that
-   lack __builtin_add_overflow.  */
+   warnings for _Generic.  This matters only for compilers that
+   lack relevant builtins.  */
 #if __GNUC__
 # define _GL__GENERIC_BOGUS 1
 #else
@@ -383,13 +407,10 @@
 #endif
 
 /* Store the low-order bits of A <op> B into *R, where OP specifies
-   the operation.  BUILTIN is the builtin operation, and OVERFLOW the
-   overflow predicate.  Return 1 if the result overflows.  See above
-   for restrictions.  */
-#if _GL_HAS_BUILTIN_OVERFLOW
-# define _GL_INT_OP_WRAPV(a, b, r, op, builtin, overflow) builtin (a, b, r)
-#elif 201112 <= __STDC_VERSION__ && !_GL__GENERIC_BOGUS
-# define _GL_INT_OP_WRAPV(a, b, r, op, builtin, overflow) \
+   the operation and OVERFLOW the overflow predicate.  Return 1 if the
+   result overflows.  See above for restrictions.  */
+#if 201112 <= __STDC_VERSION__ && !_GL__GENERIC_BOGUS
+# define _GL_INT_OP_WRAPV(a, b, r, op, overflow) \
    (_Generic \
     (*(r), \
      signed char: \
@@ -444,7 +465,7 @@
         : (*(r) = _GL_INT_OP_WRAPV_VIA_UNSIGNED (a,b,op,unsigned,st), 0)))
 # endif
 
-# define _GL_INT_OP_WRAPV(a, b, r, op, builtin, overflow) \
+# define _GL_INT_OP_WRAPV(a, b, r, op, overflow) \
    (sizeof *(r) == sizeof (signed char) \
     ? _GL_INT_OP_WRAPV_SMALLISH (a, b, r, op, overflow, \
                                  signed char, SCHAR_MIN, SCHAR_MAX, \
@@ -565,4 +586,10 @@
          : (tmin) / (a) < (b)) \
       : (tmax) / (b) < (a)))
 
+#ifdef _GL_TEMPDEF___has_builtin
+# undef __has_builtin
+# undef _GL_HAS___builtin_add_overflow
+# undef _GL_TEMPDEF___has_builtin
+#endif
+
 #endif /* _GL_INTPROPS_H */
diff --git a/lib/verify.h b/lib/verify.h
index afdc1ad81..06e975ebf 100644
--- a/lib/verify.h
+++ b/lib/verify.h
@@ -56,6 +56,16 @@
 # undef _Static_assert
 #endif
 
+/* If the compiler lacks __has_builtin, define it well enough for this
+   source file only.  */
+#ifndef __has_builtin
+# define __has_builtin(x) _GL_HAS_##x
+# define _GL_HAS___builtin_unreachable (4 < __GNUC__ + (5 <= __GNUC_MINOR__))
+# define _GL_HAS___builtin_trap \
+    (3 < __GNUC__ + (3 < __GNUC_MINOR__ + (4 <= __GNUC_PATCHLEVEL__)))
+# define _GL_TEMPDEF___has_builtin
+#endif
+
 /* Each of these macros verifies that its argument R is nonzero.  To
    be portable, R should be an integer constant expression.  Unlike
    assert (R), there is no run-time overhead.
@@ -260,24 +270,17 @@ template <int w>
 # define verify(R) _GL_VERIFY (R, "verify (" #R ")", -)
 #endif
 
-#ifndef __has_builtin
-# define __has_builtin(x) 0
-#endif
-
 /* Assume that R always holds.  Behavior is undefined if R is false,
    fails to evaluate, or has side effects.  Although assuming R can
    help a compiler generate better code or diagnostics, performance
    can suffer if R uses hard-to-optimize features such as function
    calls not inlined by the compiler.  */
 
-#if (__has_builtin (__builtin_unreachable) \
-     || 4 < __GNUC__ + (5 <= __GNUC_MINOR__))
+#if __has_builtin (__builtin_unreachable)
 # define assume(R) ((R) ? (void) 0 : __builtin_unreachable ())
 #elif 1200 <= _MSC_VER
 # define assume(R) __assume (R)
-#elif ((defined GCC_LINT || defined lint) \
-       && (__has_builtin (__builtin_trap) \
-           || 3 < __GNUC__ + (3 < __GNUC_MINOR__ + (4 <= __GNUC_PATCHLEVEL__))))
+#elif (defined GCC_LINT || defined lint) && __has_builtin (__builtin_trap)
   /* Doing it this way helps various packages when configured with
      --enable-gcc-warnings, which compiles with -Dlint.  It's nicer
      when 'assume' silences warnings even with older GCCs.  */
@@ -287,6 +290,13 @@ template <int w>
 # define assume(R) ((R) ? (void) 0 : /*NOTREACHED*/ (void) 0)
 #endif
 
+#ifdef _GL_TEMPDEF___has_builtin
+# undef __has_builtin
+# undef _GL_HAS___builtin_unreachable
+# undef _GL_HAS___builtin_trap
+# undef _GL_TEMPDEF___has_builtin
+#endif
+
 /* @assert.h omit end@  */
 
 #endif
-- 
2.17.1