[PATCH] verify: avoid __builtin_assume

[PATCH] verify: avoid __builtin_assume

[Paul Eggert]

Our latest attempt to use Clang’s __builtin_assume caused a crash
in GNU Emacs that we spent quite some time tracking down as being
caused by the switch to __builtin_assume.  It’s not known whether
the crash is due is a Clang bug or a portability bug in GNU Emacs.
For now, play it safe and avoid __builtin_assume.
* lib/verify.h (_GL_HAS_BUILTIN_ASSUME): Remove.
(assume): Simplify by not trying to use Clang’s __builtin_assume.
---
 ChangeLog    | 11 +++++++++++
 lib/verify.h | 45 ++++++++-------------------------------------
 2 files changed, 19 insertions(+), 37 deletions(-)

diff --git a/ChangeLog b/ChangeLog
index 454bd762b..053487c72 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,14 @@
+2020-09-05  Paul Eggert  <eggert@cs.ucla.edu>
+
+	verify: avoid __builtin_assume
+	Our latest attempt to use Clang’s __builtin_assume caused a crash
+	in GNU Emacs that we spent quite some time tracking down as being
+	caused by the switch to __builtin_assume.  It’s not known whether
+	the crash is due is a Clang bug or a portability bug in GNU Emacs.
+	For now, play it safe and avoid __builtin_assume.
+	* lib/verify.h (_GL_HAS_BUILTIN_ASSUME): Remove.
+	(assume): Simplify by not trying to use Clang’s __builtin_assume.
+
 2020-09-05  Bruno Haible  <bruno@clisp.org>
 
 	Fix several "warning: no previous prototype for function".
diff --git a/lib/verify.h b/lib/verify.h
index ca2a15407..fa1ed717d 100644
--- a/lib/verify.h
+++ b/lib/verify.h
@@ -246,13 +246,6 @@ template <int w>
 
 /* @assert.h omit start@  */
 
-#if defined __has_builtin
-/* <https://clang.llvm.org/docs/LanguageExtensions.html#builtin-functions> */
-# define _GL_HAS_BUILTIN_ASSUME __has_builtin (__builtin_assume)
-#else
-# define _GL_HAS_BUILTIN_ASSUME 0
-#endif
-
 #if 3 < __GNUC__ + (3 < __GNUC_MINOR__ + (4 <= __GNUC_PATCHLEVEL__))
 # define _GL_HAS_BUILTIN_TRAP 1
 #elif defined __has_builtin
@@ -312,36 +305,14 @@ template <int w>
 
    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.  */
-
-/* Use __builtin_assume in preference to __builtin_unreachable, because
-   in clang versions 8.0.x and older, the definition based on
-   __builtin_assume has an effect on optimizations, whereas the definition
-   based on __builtin_unreachable does not.  (GCC so far has only
-   __builtin_unreachable.)  */
-#if _GL_HAS_BUILTIN_ASSUME
-/* Use __builtin_constant_p to help clang's data-flow analysis for the case
-   assume (0).
-   Use a temporary variable, to avoid a clang warning
-   "the argument to '__builtin_assume' has side effects that will be discarded"
-   if R contains invocations of functions not marked as 'const'.
-   The type of the temporary variable can't be __typeof__ (R), because that
-   does not work on bit field expressions.  Use '_Bool' or 'bool' as type
-   instead.  */
-# if defined __cplusplus
-#  define assume(R) \
-     (__builtin_constant_p (R) && !(R) \
-      ? (void) __builtin_unreachable () \
-      : (void) ({ bool _gl_verify_temp = (R); \
-                  __builtin_assume (_gl_verify_temp); }))
-# else
-#  define assume(R) \
-     (__builtin_constant_p (R) && !(R) \
-      ? (void) __builtin_unreachable () \
-      : (void) ({ _Bool _gl_verify_temp = (R); \
-                  __builtin_assume (_gl_verify_temp); }))
-# endif
-#elif _GL_HAS_BUILTIN_UNREACHABLE
+   features such as function calls not inlined by the compiler.
+
+   Avoid Clang's __builtin_assume, as it breaks GNU Emacs master
+   as of 2020-08-23T21:09:49Z!eggert@cs.ucla.edu; see
+   <https://bugs.gnu.org/43152#71>.  It's not known whether this breakage
+   is a Clang bug or an Emacs bug; play it safe for now.  */
+
+#if _GL_HAS_BUILTIN_UNREACHABLE
 # define assume(R) ((R) ? (void) 0 : __builtin_unreachable ())
 #elif 1200 <= _MSC_VER
 # define assume(R) __assume (R)
-- 
2.25.4

Re: [PATCH] verify: avoid __builtin_assume

[Bruno Haible]

Paul Eggert wrote:
> It’s not known whether
> the crash is due is a Clang bug or a portability bug in GNU Emacs.

Since around the time I added the __builtin_assume use in verify.h,
the gnulib integration test that Tim Rühsen maintains started to
occasionally fail - only in the clang/Debian run, not in the gcc/Debian
run. See <https://gitlab.com/gnulib/gnulib-ci/-/jobs>.

I say "around the time", since this integration test runs only once
a week.

I tried to reproduce locally, but couldn't - in my local build,
it succeeded.

The actual failure is a
  FAIL: test-regex
during "make check".

Like Emacs, the regex code is quite complicated, so it is possible
that both the Emacs crash and the regex failure are caused by the
same clang bug.

Let's see how the gnulib integration test will react to the revert...

Bruno

Re: [PATCH] verify: avoid __builtin_assume

[Bruno Haible]

On 2020-09-06 I wrote:
> The actual failure is a
>   FAIL: test-regex
> during "make check".
> 
> Like Emacs, the regex code is quite complicated, so it is possible
> that both the Emacs crash and the regex failure are caused by the
> same clang bug.
> 
> Let's see how the gnulib integration test will react to the revert...

The gnulib integration test [1] is green in the last two builds. It looks
like avoiding __builtin_assume fixed the test-regex failures with clang.

Thanks, Paul. It saved me hours of investigation.

Bruno

[1] https://gitlab.com/gnulib/gnulib-ci/-/pipelines

Re: [PATCH] verify: avoid __builtin_assume

[Jim Meyering]

On Sun, Sep 20, 2020 at 7:59 AM Bruno Haible <bruno@clisp.org> wrote:
> On 2020-09-06 I wrote:
> > The actual failure is a
> >   FAIL: test-regex
> > during "make check".
> >
> > Like Emacs, the regex code is quite complicated, so it is possible
> > that both the Emacs crash and the regex failure are caused by the
> > same clang bug.
> >
> > Let's see how the gnulib integration test will react to the revert...
>
> The gnulib integration test [1] is green in the last two builds. It looks
> like avoiding __builtin_assume fixed the test-regex failures with clang.
>
> Thanks, Paul. It saved me hours of investigation.
>
> Bruno
>
> [1] https://gitlab.com/gnulib/gnulib-ci/-/pipelines

Interested to see those tests, I visited your link, but for me it gets a 404.
Is there another way to get to the desired page?

Re: gnulib-ci project

[Bruno Haible]

Hi Tim,

> > [1] https://gitlab.com/gnulib/gnulib-ci/-/pipelines
> 
> Interested to see those tests, I visited your link, but for me it gets a 404.
> Is there another way to get to the desired page?

Indeed, this Gitlab project is private. Would you agree to making it
public, i.e. world-visible?

Bruno