[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

Re: gnulib-ci project

[Bruno Haible]

Simon Josefsson wrote:
> Was anyone working on setting up CI/CD for gnulib on GitLab?
> I recall there was a private project for it, but I don't have access.
> Any reason for this?  I have become rather aquinted with GitLab CI/CD
> lately so it would be easy for me to setup something from scratch.

There's no point in duplicating the effort already done, and creating
confusion by having two different gnulib CIs on the same platform.

https://gitlab.com/gnulib/gnulib-ci/-/pipelines

Tim,

I have asked you twice [1][2] whether it's OK to make the project public.
I didn't receive an answer. When you introduced me to the project (on
the phone), you didn't mention a reason for keeping it private, as far
as I can remember. So, to fulfil the demands of Jim and Simon, I have
now made the project public. I hope that you will agree a posteriori.

Bruno

[1] https://lists.gnu.org/archive/html/bug-gnulib/2020-09/msg00107.html
[2] https://lists.gnu.org/archive/html/bug-gnulib/2020-11/msg00042.html

Re: gnulib-ci project

[Simon Josefsson via Gnulib discussion list]

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

Bruno Haible <bruno@clisp.org> writes:

> Simon Josefsson wrote:
>> Was anyone working on setting up CI/CD for gnulib on GitLab?
>> I recall there was a private project for it, but I don't have access.
>> Any reason for this?  I have become rather aquinted with GitLab CI/CD
>> lately so it would be easy for me to setup something from scratch.
>
> There's no point in duplicating the effort already done, and creating
> confusion by having two different gnulib CIs on the same platform.
>
> https://gitlab.com/gnulib/gnulib-ci/-/pipelines

Great -- seems straightforward, although it would be nice to have it
passing a build before adding more platforms.  It seems there are only
two tests that unexpectedly pass, the test-asyncsafe-linked_list-weak.sh
and test-asyncsafe-linked_list-strong.sh.  Any ideas on these?  I'm not
sure if they have already been discussed.

https://gitlab.com/gnulib/gnulib-ci/-/jobs/1246762022/artifacts/file/gnulib/testdir/gltests/test-asyncsafe-linked_list-strong.sh.log

> Tim,
>
> I have asked you twice [1][2] whether it's OK to make the project public.

I know Tim is busy on a new job so let's make him proud of what he
started by improving it :-)

/Simon

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 227 bytes --]

Re: gnulib-ci project

[Bruno Haible]

Simon Josefsson wrote:
> It seems there are only
> two tests that unexpectedly pass, the test-asyncsafe-linked_list-weak.sh
> and test-asyncsafe-linked_list-strong.sh.  Any ideas on these?

I was meaning to clean this up soon, but haven't gotten around to it. So,
let me move them into a tests module that does not get included by default.


2021-05-14  Bruno Haible  <bruno@clisp.org>

	linked-list-unportable-test: New module.
	* modules/linked-list-unportable-tests: New file, based on
	modules/linked-list-tests.
	* modules/linked-list-tests: Remove the unportable tests from here.
	Depend on linked-list-unportable-tests.

diff --git a/modules/linked-list-tests b/modules/linked-list-tests
index c45f02b..53472ed 100644
--- a/modules/linked-list-tests
+++ b/modules/linked-list-tests
@@ -1,32 +1,13 @@
 Files:
 tests/test-linked_list.c
-tests/test-asyncsafe-linked_list-weak.sh
-tests/test-asyncsafe-linked_list-weak.c
-tests/test-asyncsafe-linked_list-strong.sh
-tests/test-asyncsafe-linked_list-strong.c
 tests/macros.h
 
 Depends-on:
 array-list
-thread
-yield
-nanosleep
-sigaction
-sigprocmask
+linked-list-unportable-tests
 
 configure.ac:
 
 Makefile.am:
-TESTS += \
-  test-linked_list \
-  test-asyncsafe-linked_list-weak.sh \
-  test-asyncsafe-linked_list-strong.sh
-XFAIL_TESTS += \
-  test-asyncsafe-linked_list-weak.sh \
-  test-asyncsafe-linked_list-strong.sh
-check_PROGRAMS += \
-  test-linked_list \
-  test-asyncsafe-linked_list-weak \
-  test-asyncsafe-linked_list-strong
-test_asyncsafe_linked_list_weak_LDADD = $(LDADD) @LIBMULTITHREAD@ @YIELD_LIB@
-test_asyncsafe_linked_list_strong_LDADD = $(LDADD) @LIBMULTITHREAD@ @YIELD_LIB@
+TESTS += test-linked_list
+check_PROGRAMS += test-linked_list
diff --git a/modules/linked-list-unportable-tests b/modules/linked-list-unportable-tests
new file mode 100644
index 0000000..0c91c72
--- /dev/null
+++ b/modules/linked-list-unportable-tests
@@ -0,0 +1,32 @@
+Files:
+tests/test-asyncsafe-linked_list-weak.sh
+tests/test-asyncsafe-linked_list-weak.c
+tests/test-asyncsafe-linked_list-strong.sh
+tests/test-asyncsafe-linked_list-strong.c
+tests/macros.h
+
+Status:
+unportable-test
+
+Depends-on:
+linked-list
+thread
+yield
+nanosleep
+sigaction
+sigprocmask
+
+configure.ac:
+
+Makefile.am:
+TESTS += \
+  test-asyncsafe-linked_list-weak.sh \
+  test-asyncsafe-linked_list-strong.sh
+XFAIL_TESTS += \
+  test-asyncsafe-linked_list-weak.sh \
+  test-asyncsafe-linked_list-strong.sh
+check_PROGRAMS += \
+  test-asyncsafe-linked_list-weak \
+  test-asyncsafe-linked_list-strong
+test_asyncsafe_linked_list_weak_LDADD = $(LDADD) @LIBMULTITHREAD@ @YIELD_LIB@
+test_asyncsafe_linked_list_strong_LDADD = $(LDADD) @LIBMULTITHREAD@ @YIELD_LIB@