riscv-musl
riscv-musl copied to clipboard
riscvarchive
Upstreaming plans
I've just crossed off my last TODO by adding a VDSO implementation for #4. I intend to leave it open for feedback for a week or two from any interested parties, then I'm going to prepare a patch for musl upstream. My plan is to only include riscv64 in this patch.
I've ported a substantial subset of Alpine Linux to riscv64 based on this work, and I'm fairly confident in the quality of the patch.
If anyone has any objections or feedback before then, please let me know!
I've got feedback: this is awesome news and I really appreciate the work you're doing on this. Thanks Drew!
My patches from when I looked at this are available here: https://github.com/michaelforney/musl/compare/6a4f4a9...riscv
Based on what you've already integrated into the staging branch, I think the only things left are related to riscv32 and soft-float. Since you're only proposing riscv64 at this time, I think you can ignore the riscv32 ones for now (though perhaps consider them for integration into riscv-musl).
Regarding soft-float, I think you'll need:
-
https://github.com/michaelforney/musl/commit/3d06ad0561856b2ed6e03b022333348140bd0340 The current configure check looks for
__riscv_soft_float, but this is not defined by gcc anywhere. Instead, I think the relevant defines are__riscv_float_abi_softto detect soft-float ABI, and__riscv_flento detect support for the hardware extensions (https://github.com/gcc-mirror/gcc/blob/master/gcc/config/riscv/riscv-c.c). This patch needs some review; I only made my best guesses about whether the ifdef was trying to check for soft-float ABI or lack of floating point instructions. Looking at this again, maybe thesetjmp/longjmpifdefs should be for__riscv_fleninstead to save the floating registers even if using the soft-float ABI? Not sure. -
https://github.com/michaelforney/musl/commit/988d64a480bdae3a3ac7158e018198af8495304f https://github.com/michaelforney/musl/commit/99ab803bdf1abc8b9468f74e5e867549b2bc2002 (squash into previous) These are necessary since otherwise it doesn't fall back to the C versions when the F or D extensions are not available. Try building musl with
gcc -march=rv64imac -mabi=lp64to see the errors.
Alternatively, maybe you can sidestep these issues for now and simply fail at configure if __riscv_float_abi_soft is defined. Something like
diff --git a/configure b/configure
index 4d3d8b44..0adec994 100755
--- a/configure
+++ b/configure
@@ -644,7 +644,7 @@ fi
if test "$ARCH" = "riscv" || test "$ARCH" = "riscv64" ; then
trycppif "RISCVEB || _RISCVEB || __RISCVEB || __RISCVEB__" "$t" && SUBARCH=${SUBARCH}eb
-trycppif __riscv_soft_float "$t" && SUBARCH=${SUBARCH}-sf
+trycppif __riscv_float_abi_soft "$t" && fail "$0: error: soft-float not supported on riscv"
fi
if test "$ARCH" = "sh" ; then
Thanks for clarifying!
Alternatively, maybe you can sidestep these issues for now and simply fail at configure if
__riscv_float_abi_softis defined.
Up to you if you want to have this included in the first upstream patch. We can explain the unknowns in the patch comments and do a code review on the list if you'd like, or we can skip it and integrate soft float separately.
Also, I'm not sure if the merged fix for the fcntl.h issues is acceptable by upstream. See Rich Felker's recent comment on including bits/* headers from other headers:
https://www.openwall.com/lists/musl/2019/02/12/7
LONG_BIT might not even be defined if certain feature-test macros aren't defined. I think it would be simpler to just fix the constants in arch/riscv64/bits/fcntl.h as I did in https://github.com/michaelforney/musl/commit/dd59ec7eba1f32b865568ea9196be6ff5adb8717, and then start a thread on the mailing list about how to make the generic header work for both 32-bit and 64-bit targets.
Up to you if you want to have this included in the first upstream patch. We can explain the unknowns in the patch comments and do a code review on the list if you'd like, or we can skip it and integrate soft float separately.
Either way is fine with me. Whatever you think is easier.
Looking at glibc's setjmp (https://sourceware.org/git/?p=glibc.git;a=blob;f=sysdeps/riscv/setjmp.S;h=38a93b78e88ec13535934394e1302a7ddbc12e8d;hb=HEAD#l46), I see they also use __riscv_float_abi_soft, so that gives me some confidence that this is the correct thing to check.
So maybe just integrate those three soft-float patches, and then if any issues come up on the list, you can resubmit with the change to fail at configure.
Also, I'm not sure if the merged fix for the fcntl.h issues is acceptable by upstream. See Rich Felker's recent comment on including bits/* headers from other headers:
Will investigate.
So maybe just integrate those three soft-float patches, and then if any issues come up on the list, you can resubmit with the change to fail at configure.
Sounds good :smile:
I think it's best to hold off on rv32 until we have glibc upstream and the kernel ABI sorted out.
I think ima / imac support would be very useful. Could we please merge michaelforney's patches in so that we can test them ?
It would be good to have a "staging-experimental" branch with the @michaelforney patches.
Blocker: fixing #1 properly
Also, update: I should have time to put a bow on this next week.
Regarding __riscv_float_abi_soft vs __riscv_flen for setjmp, it's possibly more complicated, and depends on whether there's an ABI obligation to treat the call-saved floating point registers as call-saved even on the soft-float ABI. For example the ARM EABI does have such a requirement, and this means that no compile-time macros are sufficient to know whether saving is required. Because it's possible that libc was built as pure soft-float but the application is using floating point registers (hard float with soft-float ABI), setjmp must query the kernel-provided AT_HWCAP to know what to do.
If riscv ABI has anything like that, you may need to do the same. Otherwise (if float regs are only call-saved on hardfloat ABI), then checking __riscv_float_abi_soft is exactly right.
Here's what the psABI says:
Floating-point values in callee-saved registers are only preserved across calls if they are no larger than the width of a floating-point register in the targeted ABI. Therefore, these registers can always be considered temporaries if targeting the base integer calling convention.
I think this means that they do not need to be saved on the soft-float ABI.
Yes. The "width 0" interpretation needed to get there is a little bit confusing and unclear whether it's intentional, but with the text following "therefore", I think it's very clear. Thanks for finding that.
The "FLEN=0" concept is used in a handful of places, so I think that's why it was mentioned so quickly there. GCC generates code with the expected behavior
$ cat test.c
double f(long x)
{
return x;
}
double g(long x)
{
register double y __asm__("fs0");
__asm__ ("fcvt.d.l %0, %1" : "=f"(y) : "r"(x));
double z = f(y);
return y + z;
}
$ riscv64-unknown-elf-gcc test.c -O3 -S -o- -fno-inline -march=rv64imafdc -mabi=lp64
.file "test.c"
.option nopic
.text
.align 1
.globl f
.type f, @function
f:
fcvt.d.l fa5,a0
fmv.x.d a0,fa5
ret
.size f, .-f
.align 1
.globl g
.type g, @function
g:
#APP
# 9 "test.c" 1
fcvt.d.l fs0, a0
# 0 "" 2
#NO_APP
fcvt.l.d a0,fs0,rtz
addi sp,sp,-32
sd ra,24(sp)
fsd fs0,8(sp)
call f
fld fs0,8(sp)
fmv.d.x fa5,a0
ld ra,24(sp)
fadd.d fa5,fs0,fa5
addi sp,sp,32
fmv.x.d a0,fa5
jr ra
.size g, .-g
.ident "GCC: (GNU) 8.1.0"
Sorry for the delay, I've been busier than I thought. I've made the following changes to staging branch:
- Rebased against the latest changes in musl upstream
- Drop the old fcntl.h fixes in favor of @michaelforney's b92c52c24292d4de4f4aa902c5ced848ac447d5a
- Drop my old v2 syscall patches in favor of non-riscv-specific patches (which have been sent upstream separately) plus @michaelforney's 8e7cd87cfc03889b2e1d3e1a3f93770e653ee8df
- Incorporate @michaelforney's soft float changes
I've created a new develop branch based on this, to give people a space to work on riscv32. My plan is:
- [x] Split patches which affect both riscv32 and riscv64 in twain in the develop branch
- [x] Pull only the riscv64 patches into the staging branch
- [x] Test riscv64 on Alpine Linux and fix any issues which come up
- [x] Test #7 & pull to staging/develop
I think we have two outstanding issues that block upstreaming:
- setjmp issues, addressed by #7
- barriers misordered as discussed in #6
My plan is to shepard fixes for these issues into the staging and develop branches, then spend some time testing the staging branch and awaiting feedback from those interested. Then we can rebase this into one patch to present upstream, hopefully in time for musl 1.1.23.
How is this one going? The Haiku folks would love to switch to musl from their hacked together glibc. Since i've been working on the riscv64 port, this is an important repo :-)
Ah, so waiting on the Barrier fixes @sorear mentioned?
Proposed changes for the above are in #7 ; I'll make a separate PR to fix the barriers
I'm still unclear on what the "barrier issue" is, but as noted in my reply to the patch on-list, I think atomic_arch.h is currently broken (not providing working atomics). There are a few other small issues/questions I have there, but mostly this looks good, and I now have a toolchain for testing.
Thanks for the feedback, Rich! I'll have time to update the patch per your feedback next week.
Ping.
I would really like to have some interaction in terms of replies to review rather than just a week of radio silence then a new patch that might or might not address the important concerns. I think we're really close here, but I've had that impression several times before then things just went silent. Let's get this upstream this time.
Sorry Rich, I thought I had mentioned that I was about to travel and would be picking this back up once I returned. My plan is to address your feedback in a new patch on Monday.
No problem. Somehow I misinterpreted "next week" (at the time of the above comment) as the week that just passed.
