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

[Julien Fischer]

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?

> I have not reproduced the issue on Linux/aarch64/musl.
>
> 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.
>
>    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.
> ---
> runtime/mercury.h                   | 33 ++++++----
> tests/hard_coded/Mmakefile          |  1 +
> tests/hard_coded/thread_commit.exp  |  7 ++
> tests/hard_coded/thread_commit.exp2 |  1 +
> tests/hard_coded/thread_commit.m    | 99 +++++++++++++++++++++++++++++
> 5 files changed, 127 insertions(+), 14 deletions(-)
> create mode 100644 tests/hard_coded/thread_commit.exp
> create mode 100644 tests/hard_coded/thread_commit.exp2
> create mode 100644 tests/hard_coded/thread_commit.m
>
> 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.

> +  // 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.)

> +    #define MR_USE_GCC_BUILTIN_SETJMP_LONGJMP
> +  #endif
> +#endif
>

...

> diff --git a/tests/hard_coded/thread_commit.exp2 b/tests/hard_coded/thread_commit.exp2
> new file mode 100644
> index 000000000..ff161d491
> --- /dev/null
> +++ b/tests/hard_coded/thread_commit.exp2
> @@ -0,0 +1 @@
> +spawn_native not supported.
> 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.

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

Julien.