[m-rev.] for review: Don't use __builtin_setjmp, __builtin_longjmp on AArch64.

[Peter Wang]

On Wed, 25 May 2022 18:52:22 +1000 Julien Fischer <jfischer at opturion.com> wrote:
> 
> Hi Peter,
> 
> On Wed, 25 May 2022, Peter Wang wrote:
> 
> > There is an issue on Linux/aarch64/glibc where a commit performed on a
> > thread using the gcc __builtin_longjmp function causes an assertion
> > failure in pthread_create() if the Mercury runtime is dynamically
> > linked:
> >
> >    pthread_create.c:430: start_thread: Assertion `freesize < pd->stackblock_size' failed.
> >
> > or a crash if the Mercury runtime is statically linked:
> >
> >    *** Mercury runtime: caught segmentation violation ***
> >    cause: address not mapped to object
> >
> > Tested using gcc version 6.3.0 20170516 (Debian 6.3.0-18)
> 
> Are you in a position to test it with other versions?

Yes. The problem also occurs with gcc 8.3.0, but not gcc 10.2.
I have updated the patch accordingly.

> > diff --git a/runtime/mercury.h b/runtime/mercury.h
> > index 699dc9712..748e38906 100644
> > --- a/runtime/mercury.h
> > +++ b/runtime/mercury.h
> 
> ...
> 
> > @@ -75,12 +75,20 @@
> > // The jmp_buf type used by MR_builtin_setjmp()
> > // to save the stack context when implementing commits.
> > 
> > -#ifdef MR_GNUC
> > -  // For GCC, we use `__builtin_setjmp' and `__builtin_longjmp'.
> > -  // These are documented (in gcc/builtins.c in the GCC source code)
> > -  // as taking for their parameter a pointer to an array of five words.
> > +#if (MR_GNUC > 2 || (MR_GNUC == 2 && __GNUC_MINOR__ >= 8))
> 
> Perhaps just test if MR_GNUC is > 2 here, since we don't support GCC
> versions that old anyway.

Leaving it for now.

> > +  // There is some problem with using __builtin_setjmp and __builtin_longjmp on
> > +  // Linux/glibc/aarch64. When a thread commits using __builtin_longjmp, either
> > +  // we get an assertion failure in pthread_create or a segmentation fault.
> > +  #if !defined(__aarch64__) && !defined(MR_DARWIN_SETJMP_WORKAROUND)
> 
> Aside: we can probably get rid of MR_DARWIN_SETJMP_WORKAROUND as it only
> applied to the i*86 Darwin target and that's no longer a thing.  (Also,
> the bug in GCC that was being worked around has long since been fixed.)

Feel free to change it afterwards.

> > diff --git a/tests/hard_coded/thread_commit.m b/tests/hard_coded/thread_commit.m
> > new file mode 100644
> > index 000000000..86e46bbc9
> > --- /dev/null
> > +++ b/tests/hard_coded/thread_commit.m
> > @@ -0,0 +1,99 @@
> > +%---------------------------------------------------------------------------%
> > +% vim: ts=4 sw=4 et ft=mercury
> > +%---------------------------------------------------------------------------%
> > +
> > +% This program checks if performing a commit on a thread causes any issues.
> 
> There should be a comment here describing what the two expected outputs
> are for.

Done.

> 
> That looks fine otherwise; I assume I should apply this one to the
> release branch as well?

Yes, thanks.

The updated patch follows (for review).

Peter

----

Don't use __builtin_setjmp, __builtin_longjmp on AArch64 with older GCCs.

There is an issue on Linux/aarch64 with older versions of gcc where a
commit performed on a thread using the gcc __builtin_longjmp function
causes an assertion failure in pthread_create() if the Mercury runtime
is dynamically linked:

    pthread_create.c:430: start_thread: Assertion `freesize < pd->stackblock_size' failed.

or a crash if the Mercury runtime is statically linked:

    *** Mercury runtime: caught segmentation violation ***
    cause: address not mapped to object

The gcc versions tested are:

    BAD - gcc version 6.3.0 20170516 (Debian 6.3.0-18) from Debian 9
    BAD - gcc version 8.3.0 (Debian 8.3.0-2) from Debian 10
    OK  - gcc version 10.2.1 20210110 (Debian 10.2.1-6) from Debian 11

on Debian with glibc.

runtime/mercury.h:
    Define MR_USE_GCC_BUILTIN_SETJMP_LONGJMP if we decide to use gcc's
    __builtin_setjmp and __builtin_longjmp functions instead of the
    standard functions.

    Use standard setjmp/longjmp on aarch64 if gcc version is < 10.

    Update the comment for MR_builtin_jmp_buf to refer to the GCC manual
    instead of the GCC source code. Use an array of 'intptr_t's instead
    of 'void *'s, to match the manual.

tests/hard_coded/Mmakefile
tests/hard_coded/thread_commit.m:
tests/hard_coded/thread_commit.exp:
tests/hard_coded/thread_commit.exp2:
    Add test case.

diff --git a/runtime/mercury.h b/runtime/mercury.h
index 699dc9712..235df9777 100644
--- a/runtime/mercury.h
+++ b/runtime/mercury.h
@@ -1,7 +1,7 @@
 // vim: ts=4 sw=4 expandtab ft=c
 
 // Copyright (C) 1999-2006, 2011 The University of Melbourne.
-// Copyright (C) 2014, 2015-2016, 2018 The Mercury team.
+// Copyright (C) 2014, 2015-2016, 2018, 2022 The Mercury team.
 // This file is distributed under the terms specified in COPYING.LIB.
 
 // mercury.h - This file defines the macros, types, etc. that
@@ -75,12 +75,25 @@
 // The jmp_buf type used by MR_builtin_setjmp()
 // to save the stack context when implementing commits.
 
-#ifdef MR_GNUC
-  // For GCC, we use `__builtin_setjmp' and `__builtin_longjmp'.
-  // These are documented (in gcc/builtins.c in the GCC source code)
-  // as taking for their parameter a pointer to an array of five words.
+#if (MR_GNUC > 2 || (MR_GNUC == 2 && __GNUC_MINOR__ >= 8))
+  // There is an issue with using __builtin_setjmp and __builtin_longjmp on
+  // Linux/aarch64. When a thread commits using __builtin_longjmp, we get an
+  // assertion failure or a segmentation fault. The problem appears to have
+  // been fixed in some version between gcc 8.3.0 and gcc 10.2.
+  #if defined(__aarch64__) && (MR_GNUC < 10)
+    // Don't define MR_USE_GCC_BUILTIN_SETJMP_LONGJMP.
+  #elif defined(MR_DARWIN_SETJMP_WORKAROUND)
+    // Don't define MR_USE_GCC_BUILTIN_SETJMP_LONGJMP.
+  #else
+    #define MR_USE_GCC_BUILTIN_SETJMP_LONGJMP
+  #endif
+#endif
 
-  typedef void *MR_builtin_jmp_buf[5];
+#ifdef MR_USE_GCC_BUILTIN_SETJMP_LONGJMP
+  // The jump buffer for GCC `__builtin_setjmp' and `__builtin_longjmp'
+  // functions is documented to be an array of five intptr_t's
+  // (GCC manual section 6.5 Nonlocal gotos).
+  typedef intptr_t MR_builtin_jmp_buf[5];
 #else
   // Otherwise we use the standard jmp_buf type.
   typedef jmp_buf MR_builtin_jmp_buf;
@@ -141,14 +154,11 @@ extern  MR_Word mercury__private_builtin__dummy_var;
 //  2. The call to __builtin_longjmp() must not be in the same
 //      function as the call to __builtin_setjmp().
 
-#if (MR_GNUC > 2 || (MR_GNUC == 2 && __GNUC_MINOR__ >= 8))
-  #ifndef MR_DARWIN_SETJMP_WORKAROUND
-    #define MR_builtin_setjmp(buf)  __builtin_setjmp((buf))
-    #define MR_builtin_longjmp(buf, val)    __builtin_longjmp((buf), (val))
-  #endif
-#endif
-#ifndef MR_builtin_setjmp
-  #define MR_builtin_setjmp(buf)    setjmp((buf))
+#ifdef MR_USE_GCC_BUILTIN_SETJMP_LONGJMP
+  #define MR_builtin_setjmp(buf)        __builtin_setjmp((buf))
+  #define MR_builtin_longjmp(buf, val)  __builtin_longjmp((buf), (val))
+#else
+  #define MR_builtin_setjmp(buf)        setjmp((buf))
   #define MR_builtin_longjmp(buf, val)  longjmp((buf), (val))
 #endif
 
[test case changes omitted]