Summary: | __builtin_longjmp not supported in clang/llvm on Android arm64 target | ||
---|---|---|---|
Product: | [Developer tools] valgrind | Reporter: | chh |
Component: | general | Assignee: | Paul Floyd <pjfloyd> |
Status: | RESOLVED FIXED | ||
Severity: | normal | CC: | benoitjacob, dimitry, enh, ivosh, pjfloyd |
Priority: | NOR | ||
Version: | 3.11.0 | ||
Target Milestone: | --- | ||
Platform: | Android | ||
OS: | Other | ||
Latest Commit: | Version Fixed In: | ||
Attachments: | Implement VG_MINIMAL_SETJMP in inline assembly on Android/arm64 |
Description
chh
2016-10-03 16:56:09 UTC
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. |