Valgrind failed to build on Android for the arm64 target because llvm does not have __builtin_longjmp like gcc. The following are the error messages. external/valgrind/coregrind/m_signals.c:2013:7: warning: incompatible pointer types passing 'jmp_buf' (aka 'long [32]') to parameter of type 'void **' [-Wincompatible-pointer-types] VG_MINIMAL_LONGJMP(tst->sched_jmpbuf); ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ external/valgrind/include/pub_tool_libcsetjmp.h:126:53: note: expanded from macro 'VG_MINIMAL_LONGJMP' #define VG_MINIMAL_LONGJMP(_env) __builtin_longjmp((_env),1) ^~~~~~ external/valgrind/coregrind/m_signals.c:2013:7: error: __builtin_longjmp is not supported for the current target VG_MINIMAL_LONGJMP(tst->sched_jmpbuf); ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ external/valgrind/include/pub_tool_libcsetjmp.h:135:35: note: expanded from macro 'VG_MINIMAL_LONGJMP' #define VG_MINIMAL_LONGJMP(_env) __builtin_longjmp((_env),1) ^~~~~~~~~~~~~~~~~~~~~~~~~~~ Suggested fix, to add VG_MINIMAL_SETJMP and VG_MINIMAL_LONGJMP for VGP_arm64_linux: diff --git a/coregrind/m_libcsetjmp.c b/coregrind/m_libcsetjmp.c index 81a1a53..fe17819 100644 --- a/coregrind/m_libcsetjmp.c +++ b/coregrind/m_libcsetjmp.c @@ -36,11 +36,50 @@ /* See include/pub_tool_libcsetjmp.h for background and rationale. */ -/* The alternative implementations are for ppc{32,64}-linux and +/* The alternative implementations are for arm64-linux, ppc{32,64}-linux, and {amd64,x86}-{linux,darwin}. See #259977. That leaves only {arm,s390x}-linux using the gcc builtins now. */ +/* ------------ arm64-linux ------------ */ + +#if defined(VGP_arm64_linux) + +__asm__( +" .text" "\n" +" .align 2" "\n" +" .global VG_MINIMAL_SETJMP" "\n" +" .type VG_MINIMAL_SETJMP, @function" "\n" +"VG_MINIMAL_SETJMP:" "\n" +".L_BEGIN_SETJMP:" "\n" +" mov x1, x0" "\n" +" adrp x3, .L_BEGIN_SETJMP" "\n" +" mov x0, sp" "\n" +" mov x2, sp" "\n" +" add x4, x3, :lo12:.L_BEGIN_SETJMP" "\n" +" str x0, [x1,16]" "\n" +" mov x0, 0" "\n" +" str x2, [x1]" "\n" +" str x4, [x1,8]" "\n" +" ret" "\n" +"\n" +" .text" "\n" +" .align 2" "\n" +" .global VG_MINIMAL_LONGJMP" "\n" +" .type VG_MINIMAL_LONGJMP, @function" "\n" +"VG_MINIMAL_LONGJMP:" "\n" +" stp x29, x30, [sp, -16]!" "\n" +" mov x29, sp" "\n" +" ldr x1, [x0,8]" "\n" +" ldr x29, [x0]" "\n" +" ldr x0, [x0,16]" "\n" +" mov sp, x0" "\n" +" br x1" "\n" +); + +#endif /* VGP_arm64_linux */ + + /* ------------ ppc32-linux ------------ */ #if defined(VGP_ppc32_linux) diff --git a/include/pub_tool_libcsetjmp.h b/include/pub_tool_libcsetjmp.h index bb94a59..9045010 100644 --- a/include/pub_tool_libcsetjmp.h +++ b/include/pub_tool_libcsetjmp.h @@ -118,6 +118,15 @@ UWord VG_MINIMAL_SETJMP(VG_MINIMAL_JMP_BUF(_env)); __attribute__((noreturn)) void VG_MINIMAL_LONGJMP(VG_MINIMAL_JMP_BUF(_env)); +#elif defined(ANDROID) && defined(__clang__) && defined(__aarch64__) + +/* Android clang/llvm has no __builtin_{setjmp,longjmp} for aarch64. */ +#define VG_MINIMAL_JMP_BUF(_name) jmp_buf _name +__attribute__((returns_twice)) +UWord VG_MINIMAL_SETJMP(VG_MINIMAL_JMP_BUF(_env)); +__attribute__((noreturn)) +void VG_MINIMAL_LONGJMP(VG_MINIMAL_JMP_BUF(_env)); + #else /* The default implementation. */ Reproducible: Always Steps to Reproduce: With Android source, make external/valgrind with clang/llvm. Actual Results: Compilation error from source files that use setjmp/longjmp.
Another way to fix this problem is to link in ARM's aarch64 setjmp.S file and change include/pub_tool_libcsetjmp.h as follows. diff --git a/include/pub_tool_libcsetjmp.h b/include/pub_tool_libcsetjmp.h index bb94a59..60473a5 100644 --- a/include/pub_tool_libcsetjmp.h +++ b/include/pub_tool_libcsetjmp.h @@ -118,6 +118,14 @@ UWord VG_MINIMAL_SETJMP(VG_MINIMAL_JMP_BUF(_env)); __attribute__((noreturn)) void VG_MINIMAL_LONGJMP(VG_MINIMAL_JMP_BUF(_env)); +#elif defined(ANDROID) && defined(__aarch64__) + +/* Android clang/llvm has no __builtin_{setjmp,longjmp} for aarch64. */ +/* Use the same setjmp/longjmp functions for both gcc and clang. */ +#define VG_MINIMAL_JMP_BUF(_name) jmp_buf _name +#define VG_MINIMAL_SETJMP(_env) ((UWord)(setjmp((_env)))) +#define VG_MINIMAL_LONGJMP(_env) longjmp((_env),1) + #else /* The default implementation. */
(In reply to chh from comment #0) > Suggested fix, to add VG_MINIMAL_SETJMP and VG_MINIMAL_LONGJMP for > VGP_arm64_linux: > [..patch follows..] Thank you for looking into this. This looks like a good solution to me. But I did not understand the patch. All the other implementations of VG_MINIMAL_{SET,LONG}JMP save and restore all the integer registers. This patch definitely does not do that. Can you fix it? A good place to start is by copying the ppc32_linux case. Also .. when attaching patches, please attach them as a separate file, not as an in-line comment. Getting a usable patch out of an in-line comment is very difficult (try it!)
(In reply to Julian Seward from comment #2) Thanks for taking care of this issue and some other bugs I filed recently. You are right that my comment #0 suggestion was incomplete. I have a better fix now based on Android bionic setjmp.S. Please take a look of my pending Android change at https://android-review.googlesource.com/#/c/285947
Julian, could you take the patch in https://android-review.googlesource.com/#/c/285947 Thanks.
Please attach the patch here. We do not take patches from links pointing to other projects.
(In reply to chh from comment #4) > Julian, could you take the patch in > https://android-review.googlesource.com/#/c/285947 > Thanks. chh, please can you put a patch on this bugzilla. Firstly to make it easier for us and secondly so as to make it clearer that the patch is to be licensed how we need it to be (GPL 2+).
Created attachment 120388 [details] Implement VG_MINIMAL_SETJMP in inline assembly on Android/arm64 Here is another patch written from scratch / from first principles, separate from the bionic code that was discussed earlier here. It is available under the current licensing terms of the valgrind file it modifies. Comments should explain the details / why I think it's correct, but let me know if I missed something. I did take very literally the comment at the top of this file saying that it was enough to save only integer registers, so this patch does only that (the patch that had been considered earlier in this bug was also saving floating-point registers). I've also noticed that VG_MINIMAL_LONGJMP was hardcoding the value 1 for the second parameter, so this patch hardcodes that. So I really think this implementation is minimal. Together with the patch on bug 339861, this allows building Valgrind on Android NDK r19c, arm64 (clang toolchain). I've tested this manually on some testcases, this does seem to work as intended. The choice of inline assembly to implement this directly in the header was partly because it allowed to keep everything local (the definition of the jmpbuf struct and the two functions, setjmp and longjmp, that it's private to), partly because that saved me the hassle of touching the buildsystem and partly because I'm just more familiar with inline assembly. Let me know if you require an out-of-line implementation in a .S file but then please advise on the path of that file and give some pointers to integrate that in the build system (conditionally compiling that file on android/arm64). Technically, this code should be correct on arm64 regardless of OS(android) but there were just enough scary notes about some registers' special roles depending on the OS in section 5.1.1. of the Aaarch PCS document (link in comment in the patch) that I thought it safer to keep this restricted to android for now, which is all I've tested.
I'm working on a FreeBSD aarch64 port and have the same problem - clang is the FreeBSD system compiler.
I think that I'd prefer an out-of-line implementation, for consistency.
commit 18771b9329c590b97e68a7c7a264d3df4c3e0ad1 (HEAD -> master, origin/master, origin/HEAD) Author: Paul Floyd <pjfloyd@wanadoo.fr> Date: Thu Feb 22 16:56:22 2024 +0100 Bug 369723 - __builtin_longjmp not supported in clang/llvm on Android arm64 target Made the functions out-of-line, more like other platforms.