Created attachment 239166 [details] `git format-patch` patch file On RISC-V, calling `longjmp(x, 0);` makes `setjmp(x)` return 0, which normally causes an infinite loop, and is against the ISO C standard for setjmp/longjmp. Instead, using a value of 0 should make `setjmp` return 1: > The `longjmp` function cannot cause the `setjmp` macro to return the > value 0; if `val` is 0, the `setjmp` macro returns the value 1. > > _Taken from §7.13.2.1.4 of the C99 spec_ See https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=268521 for my similar patch for arm64 libc. That patch also has a test-case that I used to test this RISC-V implementation. --- I think I've actually managed to beat the compiler in this patch. Both GCC and Clang do something like the following at `-Os`: ```Assembly mv a0,a1 bne a1,zero,.L2 li a0,1 .L2: ret ``` It took a while of scouring the RISC-V ISA spec (it's my first time using RISC-V assembly), but I found a method that doesn't use branching and has less one less instruction!
This is a subset of https://reviews.freebsd.org/D29363 which I should get back to. Note I instead opted for a conditional li, as both those instructions compress and in theory should be macro-op fusable, or have a short forward-branch optimisation applied, on cores that implement that. I believe the SiFive 7 series does the latter.
(In reply to Jessica Clarke from comment #1) The current version of that revision seems actionable, can it be committed (minus the MIPS bits) now? Fixing the kernel implementation does not need to block the fixes for libc.
(In reply to Jessica Clarke from comment #1) I'm not an expert on RISC-V, so I'll trust your expertise on it! And apologies for not seeing that Phabricator Differential, I did do a search on Bugzilla for any PRs and could only find https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=255320, which didn't have much information. --- (In reply to Mitchell Horne from comment #2) I can't comment on the MIPS bit, since I don't know anything about MIPS, but I'd be happy to give the test-case/ARM64/RISC-V code an approving review too if that helps (let me know and I'll set up a Phabricator account).
(In reply to Mitchell Horne from comment #2) It fell off my radar and I forgot the userspace side of things was ready to go. I agree the kernel side can come separately (and is also less important since it doesn't affect third-party code). I'll commit it at last later this week when not taking time off (and the MIPS code can come back when MFC'ing...).
A commit in branch main references this bug: URL: https://cgit.FreeBSD.org/src/commit/?id=9fb118bebced1452a46756a13be0161021b10905 commit 9fb118bebced1452a46756a13be0161021b10905 Author: Jessica Clarke <jrtc27@FreeBSD.org> AuthorDate: 2023-01-09 18:34:43 +0000 Commit: Jessica Clarke <jrtc27@FreeBSD.org> CommitDate: 2023-01-09 18:34:43 +0000 libc: Fix longjmp/_longjmp(buf, 0) for AArch64 and RISC-V These architectures fail to handle this special case, and will cause the corresponding setjmp/_setjmp to return 0 rather than 1. Fix this and add regression tests (also committed upstream). PR: 268684 Reviewed by: arichardson, jhb MFC after: 1 week Differential Revision: https://reviews.freebsd.org/D29363 contrib/netbsd-tests/lib/libc/setjmp/t_setjmp.c | 50 ++++++++++++++++++++++--- lib/libc/aarch64/gen/_setjmp.S | 3 +- lib/libc/aarch64/gen/setjmp.S | 3 +- lib/libc/riscv/gen/_setjmp.S | 3 ++ lib/libc/riscv/gen/setjmp.S | 3 ++ 5 files changed, 55 insertions(+), 7 deletions(-)
A commit in branch stable/13 references this bug: URL: https://cgit.FreeBSD.org/src/commit/?id=cc58b7123be2862237746a37c1d658d0d5ca8b13 commit cc58b7123be2862237746a37c1d658d0d5ca8b13 Author: Jessica Clarke <jrtc27@FreeBSD.org> AuthorDate: 2023-01-09 18:34:43 +0000 Commit: Jessica Clarke <jrtc27@FreeBSD.org> CommitDate: 2023-01-31 01:46:18 +0000 libc: Fix longjmp/_longjmp(buf, 0) for AArch64 and RISC-V These architectures fail to handle this special case, and will cause the corresponding setjmp/_setjmp to return 0 rather than 1. Fix this and add regression tests (also committed upstream). PR: 268684 Reviewed by: arichardson, jhb MFC after: 1 week Differential Revision: https://reviews.freebsd.org/D29363 (cherry picked from commit 9fb118bebced1452a46756a13be0161021b10905) contrib/netbsd-tests/lib/libc/setjmp/t_setjmp.c | 50 ++++++++++++++++++++++--- lib/libc/aarch64/gen/_setjmp.S | 3 +- lib/libc/aarch64/gen/setjmp.S | 3 +- lib/libc/riscv/gen/_setjmp.S | 3 ++ lib/libc/riscv/gen/setjmp.S | 3 ++ 5 files changed, 55 insertions(+), 7 deletions(-)