Upstreaming plans

[ddevault]

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!

[asb]

I've got feedback: this is awesome news and I really appreciate the work you're doing on this. Thanks Drew!

[ddevault]

@michaelforney got anything you want included?

[michaelforney]

My patches from when I looked at this are available here:
michaelforney/musl@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:

• michaelforney/musl@3d06ad0
  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_soft to detect soft-float ABI, and __riscv_flen to 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 the setjmp/longjmp ifdefs should be for __riscv_flen instead to save the floating registers even if using the soft-float ABI? Not sure.

• michaelforney/musl@988d64a
  michaelforney/musl@99ab803 (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=lp64 to 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

[ddevault]

Thanks for clarifying!

Alternatively, maybe you can sidestep these issues for now and simply fail at configure if __riscv_float_abi_soft is 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.

[michaelforney]

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 michaelforney/musl@dd59ec7, 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.

[michaelforney]

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.

[ddevault]

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 😄

[palmer-dabbelt]

I think it's best to hold off on rv32 until we have glibc upstream and the kernel ABI sorted out.

[ddevault]

I don't intend to include rv32.

[mickflemm]

I think ima / imac support would be very useful. Could we please merge michaelforney's patches in so that we can test them ?

[swdevlimacon]

It would be good to have a "staging-experimental" branch with the @michaelforney patches.

[ddevault]

That seems more complicated than it ought to be.

[ddevault]

Blocker: fixing #1 properly

Also, update: I should have time to put a bow on this next week.

[richfelker]

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.

[michaelforney]

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.

[richfelker]

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.

[palmer-dabbelt]

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"

[ddevault]

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 b92c52c
• Drop my old v2 syscall patches in favor of non-riscv-specific patches (which have been sent upstream separately) plus @michaelforney's 8e7cd87
• 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:

• Split patches which affect both riscv32 and riscv64 in twain in the develop branch
• Pull only the riscv64 patches into the staging branch
• Test riscv64 on Alpine Linux and fix any issues which come up
• Test Proposed changes for #6 #7 & pull to staging/develop

I think we have two outstanding issues that block upstreaming:

• setjmp issues, addressed by Proposed changes for #6 #7
• barriers misordered as discussed in sizeof(jmp_buf) possibly wrong #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.

[kallisti5]

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 :-)

[ddevault]

Those two oustanding issues remain outstanding.

[kallisti5]

Oh? I thought #7 was done per the comments in it?

[ddevault]

Whoops, you're right. So it's just #6 afaik

[kallisti5]

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

[ddevault]

Patch sent upstream

[richfelker]

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.

[ddevault]

Thanks for the feedback, Rich! I'll have time to update the patch per your feedback next week.

[richfelker]

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.

[ddevault]

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.

[richfelker]

No problem. Somehow I misinterpreted "next week" (at the time of the above comment) as the week that just passed.

[imheresamir]

Any update on upstreaming status?

[ddevault]

It's upstream!

[richfelker]

This issue could probably be closed since it seems to be scoped to 64-bit, but riscv32 upstreaming still remains open and I think would be appropriate to do now. Basically all the the prerequisites are done due to time64 work. Adding SYS_waitid backend where SYS_wait4 is not supported is probably the only missing ingredient. Of course the kernel folks haven't made it clear whether we can expect syscall ABI to be stable, but it looks like the only way it's going to get declared stable is to have a libc using it, and glibc is moving slowly on this.

[liuyingying19]

Hi All,

Is there any plan for the upstreaming of riscv32?

[ddevault]

Someone needs to take the lead on it. Why not you?

[richfelker]

I'd like it to happen in this release cycle if at all possible. Kernel seems stabilized now. I don't think there's much left to do. Some refactoring needed to match upstream musl now and corresponding fixes made in rv64 upstream. Probably a few places we need to #ifdef out use of syscalls rv32 doesn't have in fallback code paths.

[sorear]

I'm willing to spend time on this but I'm not sure how to test it, partly on musl's end but mostly due to unclarity about what interfaces the kernel intends to make stable.

[richfelker]

Talk on the glibc side is that 5.4 will be their minimum kernel version so I think everything that was in 5.4 is "stable".

[liuyingying19]

I'm willing to spend some time. if kernel 5.4 is stable, so what we need to do is upstream musl now and corresponding fixes made in rv64 upstream? or there will still be need to #ifdef out some rv32 syscalls.

[richfelker]

I'm not sure but I'm happy to review a proposal.

[palmer-dabbelt]

For glibc we're saying 5.4 because it's a long-term stable release, but IIRC the last actual ABI change we had landed in 5.0 -- specifically d4c08b9776b3 ("riscv: Use latest system call ABI"). I doubt anyone is going to look closely, though. I'm not sure when the rv32 musl port was last updated, but it's possible that nothing actually ended up changing.

[sorear]

If my memory serves you can't implement POSIX semantics on the riscv32 linux 5.3 because waitpid(0) was removed from the kernel before waitid(P_PGID, 0) support was added.

[sorear]

I have a musl patch that's close to working, I will send it out for review at some point soon.

[richfelker]

Great. One thing I plan to do when reviewing it is diff all files against corresponding riscv64 files and make sure the differences look like what I'd expect, so that might be a useful check for you to do too.

[sorear]

That may not be terribly informative since I did this by copying all the riscv64 files and changing everything that was observably bitsize dependent.

[richfelker]

OK, thanks for telling me. That actually serves as a good point of confidence in what you've done, since I was worried there might be rv32 bitrot or things we fixed in discussion of rv64 that weren't carried over to rv32. It might make sense to actually do the opposite with your new patch - diffing it against the old rv32 proposal to see what changed by virtue of redoing it off rv64 as the base.

[sorear]

https://www.openwall.com/lists/musl/2020/09/03/2

all interested parties please follow there

[AndrewD]

https://www.openwall.com/lists/musl/2020/09/03/2

all interested parties please follow there

Are you still using musl with rv32?