mips fp32/fpxx/fp64 issues, r6 sjlj broken

Submitted by Rich Felker on Sept. 27, 2019, 12:38 a.m.

Details

Message ID 20190927003821.GE9017@brightrain.aerifal.cx
State New
Series "mips fp32/fpxx/fp64 issues, r6 sjlj broken"
Headers show

Commit Message

Rich Felker Sept. 27, 2019, 12:38 a.m.
On Thu, Sep 26, 2019 at 07:23:50PM -0400, Rich Felker wrote:
> On Thu, Sep 26, 2019 at 06:45:21PM -0400, Rich Felker wrote:
> > Also, mipsr6 (the new mips-family ISA that's not compatible with
> > previous mips) always uses the 64-bit register mode. We presently do
> > not have setjmp/longjmp code that works with this case at all
> > (existing code will wrongly save low 32-bits of 2 registers instead of
> > single whole double register); somehow nobody has noticed that this is
> > broken. Making this conditional on __mips_isa_rev >= 6 should not be
> > hard.
> 
> Attached patch should work, but maybe isn't the best thing to do. I
> think using sdc1/ldc1 and just even indices like on r6 would also be
> valid for pre-r6 mips using fp32 or fpxx abi; with FR=0, it would
> save/restore the pair of 32-bit registers, and with FR=1, fp32 code
> could not be running anyway, and fpxx code should work fine. However,
> mips I lacks the ldc1/stc1 instructions, so at the very least we'd
> need to leave the old form in place for mips I. Or maybe use the s.d
> and l.d mnemonics that automatically assemble to the right choice
> based on the isa level...

Two new versions of the patch. I think I prefer the last one.

l.d and s.d expand to pairs of lwc1 and swc1 on mips1, and otherwise
expand to ldc1 and sdc1. ldc1 and sdc1 in turn behave just like pairs
of lwc1 and swc1 when FR=0, but additionally match the fpxx ABI when
FR=1.

With this the r6 and clang issues should be fixed.

Rich

diff --git a/src/setjmp/mips/longjmp.S b/src/setjmp/mips/longjmp.S
index fdb6c95d..ecf40855 100644
--- a/src/setjmp/mips/longjmp.S
+++ b/src/setjmp/mips/longjmp.S
@@ -12,18 +12,12 @@ longjmp:
 	addu    $2, $2, 1
 1:
 #ifndef __mips_soft_float
-	lwc1    $20, 56($4)
-	lwc1    $21, 60($4)
-	lwc1    $22, 64($4)
-	lwc1    $23, 68($4)
-	lwc1    $24, 72($4)
-	lwc1    $25, 76($4)
-	lwc1    $26, 80($4)
-	lwc1    $27, 84($4)
-	lwc1    $28, 88($4)
-	lwc1    $29, 92($4)
-	lwc1    $30, 96($4)
-	lwc1    $31, 100($4)
+	l.d     $f20, 56($4)
+	l.d     $f22, 64($4)
+	l.d     $f24, 72($4)
+	l.d     $f26, 80($4)
+	l.d     $f28, 88($4)
+	l.d     $f30, 96($4)
 #endif
 	lw      $ra,  0($4)
 	lw      $sp,  4($4)
diff --git a/src/setjmp/mips/setjmp.S b/src/setjmp/mips/setjmp.S
index 501d5264..7ae8832d 100644
--- a/src/setjmp/mips/setjmp.S
+++ b/src/setjmp/mips/setjmp.S
@@ -22,18 +22,12 @@ setjmp:
 	sw      $30, 40($4)
 	sw      $28, 44($4)
 #ifndef __mips_soft_float
-	swc1    $20, 56($4)
-	swc1    $21, 60($4)
-	swc1    $22, 64($4)
-	swc1    $23, 68($4)
-	swc1    $24, 72($4)
-	swc1    $25, 76($4)
-	swc1    $26, 80($4)
-	swc1    $27, 84($4)
-	swc1    $28, 88($4)
-	swc1    $29, 92($4)
-	swc1    $30, 96($4)
-	swc1    $31, 100($4)
+	s.d     $f20, 56($4)
+	s.d     $f22, 64($4)
+	s.d     $f24, 72($4)
+	s.d     $f26, 80($4)
+	s.d     $f28, 88($4)
+	s.d     $f30, 96($4)
 #endif
 	jr      $ra
 	li      $2, 0

Patch hide | download patch | download mbox

diff --git a/src/setjmp/mips/longjmp.S b/src/setjmp/mips/longjmp.S
index fdb6c95d..4d39c88e 100644
--- a/src/setjmp/mips/longjmp.S
+++ b/src/setjmp/mips/longjmp.S
@@ -12,6 +12,14 @@  longjmp:
 	addu    $2, $2, 1
 1:
 #ifndef __mips_soft_float
+#if __mips >= 2
+	ldc1	$20, 56($4)
+	ldc1	$22, 64($4)
+	ldc1	$24, 72($4)
+	ldc1	$26, 80($4)
+	ldc1	$28, 88($4)
+	ldc1	$30, 96($4)
+#else
 	lwc1    $20, 56($4)
 	lwc1    $21, 60($4)
 	lwc1    $22, 64($4)
@@ -24,6 +32,7 @@  longjmp:
 	lwc1    $29, 92($4)
 	lwc1    $30, 96($4)
 	lwc1    $31, 100($4)
+#endif
 #endif
 	lw      $ra,  0($4)
 	lw      $sp,  4($4)
diff --git a/src/setjmp/mips/setjmp.S b/src/setjmp/mips/setjmp.S
index 501d5264..5d385f91 100644
--- a/src/setjmp/mips/setjmp.S
+++ b/src/setjmp/mips/setjmp.S
@@ -22,6 +22,14 @@  setjmp:
 	sw      $30, 40($4)
 	sw      $28, 44($4)
 #ifndef __mips_soft_float
+#if __mips >= 2
+	sdc1	$20, 56($4)
+	sdc1	$22, 64($4)
+	sdc1	$24, 72($4)
+	sdc1	$26, 80($4)
+	sdc1	$28, 88($4)
+	sdc1	$30, 96($4)
+#else
 	swc1    $20, 56($4)
 	swc1    $21, 60($4)
 	swc1    $22, 64($4)
@@ -34,6 +42,7 @@  setjmp:
 	swc1    $29, 92($4)
 	swc1    $30, 96($4)
 	swc1    $31, 100($4)
+#endif
 #endif
 	jr      $ra
 	li      $2, 0

Comments

Szabolcs Nagy Sept. 27, 2019, 11:10 a.m.
* Rich Felker <dalias@libc.org> [2019-09-26 20:38:21 -0400]:
> On Thu, Sep 26, 2019 at 07:23:50PM -0400, Rich Felker wrote:
> > On Thu, Sep 26, 2019 at 06:45:21PM -0400, Rich Felker wrote:
> > > Also, mipsr6 (the new mips-family ISA that's not compatible with
> > > previous mips) always uses the 64-bit register mode. We presently do
> > > not have setjmp/longjmp code that works with this case at all
> > > (existing code will wrongly save low 32-bits of 2 registers instead of
> > > single whole double register); somehow nobody has noticed that this is
> > > broken. Making this conditional on __mips_isa_rev >= 6 should not be
> > > hard.
> > 
> > Attached patch should work, but maybe isn't the best thing to do. I
> > think using sdc1/ldc1 and just even indices like on r6 would also be
> > valid for pre-r6 mips using fp32 or fpxx abi; with FR=0, it would
> > save/restore the pair of 32-bit registers, and with FR=1, fp32 code
> > could not be running anyway, and fpxx code should work fine. However,
> > mips I lacks the ldc1/stc1 instructions, so at the very least we'd
> > need to leave the old form in place for mips I. Or maybe use the s.d
> > and l.d mnemonics that automatically assemble to the right choice
> > based on the isa level...
> 
> Two new versions of the patch. I think I prefer the last one.
> 
> l.d and s.d expand to pairs of lwc1 and swc1 on mips1, and otherwise
> expand to ldc1 and sdc1. ldc1 and sdc1 in turn behave just like pairs
> of lwc1 and swc1 when FR=0, but additionally match the fpxx ABI when
> FR=1.

so a mips1 libc.so won't work on a system with FR=1?
but a mips2 libc.so works with both FR=1 and FR=0?

if mipsisa32r6 uses FR=1 and normal 32bit mips uses FR=0
then this sounds like an issue.

> 
> With this the r6 and clang issues should be fixed.
> 
> Rich

> diff --git a/src/setjmp/mips/longjmp.S b/src/setjmp/mips/longjmp.S
> index fdb6c95d..4d39c88e 100644
> --- a/src/setjmp/mips/longjmp.S
> +++ b/src/setjmp/mips/longjmp.S
> @@ -12,6 +12,14 @@ longjmp:
>  	addu    $2, $2, 1
>  1:
>  #ifndef __mips_soft_float
> +#if __mips >= 2
> +	ldc1	$20, 56($4)
> +	ldc1	$22, 64($4)
> +	ldc1	$24, 72($4)
> +	ldc1	$26, 80($4)
> +	ldc1	$28, 88($4)
> +	ldc1	$30, 96($4)
> +#else
>  	lwc1    $20, 56($4)
>  	lwc1    $21, 60($4)
>  	lwc1    $22, 64($4)
> @@ -24,6 +32,7 @@ longjmp:
>  	lwc1    $29, 92($4)
>  	lwc1    $30, 96($4)
>  	lwc1    $31, 100($4)
> +#endif
>  #endif
>  	lw      $ra,  0($4)
>  	lw      $sp,  4($4)
> diff --git a/src/setjmp/mips/setjmp.S b/src/setjmp/mips/setjmp.S
> index 501d5264..5d385f91 100644
> --- a/src/setjmp/mips/setjmp.S
> +++ b/src/setjmp/mips/setjmp.S
> @@ -22,6 +22,14 @@ setjmp:
>  	sw      $30, 40($4)
>  	sw      $28, 44($4)
>  #ifndef __mips_soft_float
> +#if __mips >= 2
> +	sdc1	$20, 56($4)
> +	sdc1	$22, 64($4)
> +	sdc1	$24, 72($4)
> +	sdc1	$26, 80($4)
> +	sdc1	$28, 88($4)
> +	sdc1	$30, 96($4)
> +#else
>  	swc1    $20, 56($4)
>  	swc1    $21, 60($4)
>  	swc1    $22, 64($4)
> @@ -34,6 +42,7 @@ setjmp:
>  	swc1    $29, 92($4)
>  	swc1    $30, 96($4)
>  	swc1    $31, 100($4)
> +#endif
>  #endif
>  	jr      $ra
>  	li      $2, 0

> diff --git a/src/setjmp/mips/longjmp.S b/src/setjmp/mips/longjmp.S
> index fdb6c95d..ecf40855 100644
> --- a/src/setjmp/mips/longjmp.S
> +++ b/src/setjmp/mips/longjmp.S
> @@ -12,18 +12,12 @@ longjmp:
>  	addu    $2, $2, 1
>  1:
>  #ifndef __mips_soft_float
> -	lwc1    $20, 56($4)
> -	lwc1    $21, 60($4)
> -	lwc1    $22, 64($4)
> -	lwc1    $23, 68($4)
> -	lwc1    $24, 72($4)
> -	lwc1    $25, 76($4)
> -	lwc1    $26, 80($4)
> -	lwc1    $27, 84($4)
> -	lwc1    $28, 88($4)
> -	lwc1    $29, 92($4)
> -	lwc1    $30, 96($4)
> -	lwc1    $31, 100($4)
> +	l.d     $f20, 56($4)
> +	l.d     $f22, 64($4)
> +	l.d     $f24, 72($4)
> +	l.d     $f26, 80($4)
> +	l.d     $f28, 88($4)
> +	l.d     $f30, 96($4)
>  #endif
>  	lw      $ra,  0($4)
>  	lw      $sp,  4($4)
> diff --git a/src/setjmp/mips/setjmp.S b/src/setjmp/mips/setjmp.S
> index 501d5264..7ae8832d 100644
> --- a/src/setjmp/mips/setjmp.S
> +++ b/src/setjmp/mips/setjmp.S
> @@ -22,18 +22,12 @@ setjmp:
>  	sw      $30, 40($4)
>  	sw      $28, 44($4)
>  #ifndef __mips_soft_float
> -	swc1    $20, 56($4)
> -	swc1    $21, 60($4)
> -	swc1    $22, 64($4)
> -	swc1    $23, 68($4)
> -	swc1    $24, 72($4)
> -	swc1    $25, 76($4)
> -	swc1    $26, 80($4)
> -	swc1    $27, 84($4)
> -	swc1    $28, 88($4)
> -	swc1    $29, 92($4)
> -	swc1    $30, 96($4)
> -	swc1    $31, 100($4)
> +	s.d     $f20, 56($4)
> +	s.d     $f22, 64($4)
> +	s.d     $f24, 72($4)
> +	s.d     $f26, 80($4)
> +	s.d     $f28, 88($4)
> +	s.d     $f30, 96($4)
>  #endif
>  	jr      $ra
>  	li      $2, 0
Rich Felker Sept. 27, 2019, 11:52 a.m.
On Fri, Sep 27, 2019 at 01:10:28PM +0200, Szabolcs Nagy wrote:
> * Rich Felker <dalias@libc.org> [2019-09-26 20:38:21 -0400]:
> > On Thu, Sep 26, 2019 at 07:23:50PM -0400, Rich Felker wrote:
> > > On Thu, Sep 26, 2019 at 06:45:21PM -0400, Rich Felker wrote:
> > > > Also, mipsr6 (the new mips-family ISA that's not compatible with
> > > > previous mips) always uses the 64-bit register mode. We presently do
> > > > not have setjmp/longjmp code that works with this case at all
> > > > (existing code will wrongly save low 32-bits of 2 registers instead of
> > > > single whole double register); somehow nobody has noticed that this is
> > > > broken. Making this conditional on __mips_isa_rev >= 6 should not be
> > > > hard.
> > > 
> > > Attached patch should work, but maybe isn't the best thing to do. I
> > > think using sdc1/ldc1 and just even indices like on r6 would also be
> > > valid for pre-r6 mips using fp32 or fpxx abi; with FR=0, it would
> > > save/restore the pair of 32-bit registers, and with FR=1, fp32 code
> > > could not be running anyway, and fpxx code should work fine. However,
> > > mips I lacks the ldc1/stc1 instructions, so at the very least we'd
> > > need to leave the old form in place for mips I. Or maybe use the s.d
> > > and l.d mnemonics that automatically assemble to the right choice
> > > based on the isa level...
> > 
> > Two new versions of the patch. I think I prefer the last one.
> > 
> > l.d and s.d expand to pairs of lwc1 and swc1 on mips1, and otherwise
> > expand to ldc1 and sdc1. ldc1 and sdc1 in turn behave just like pairs
> > of lwc1 and swc1 when FR=0, but additionally match the fpxx ABI when
> > FR=1.
> 
> so a mips1 libc.so won't work on a system with FR=1?
> but a mips2 libc.so works with both FR=1 and FR=0?

The ISA spec mandates that all mips r5 and earlier (this includes
mips1, mips2, mips32 up to r5) support FR=0, and the ABI for the
"mips" arch in musl is FR=0. So ability to work with FR=1 is not a
requirement. If built as fpxx (the default on all but old toolchains
or -march=mips1), the code could theoretically be linked with
fp64-model code and run in FR=1 mode, but musl does not support this;
doing it dynamically would require the dynamic linker manage the FR
mode, which is outside the scope of our ABIs model.

> if mipsisa32r6 uses FR=1 and normal 32bit mips uses FR=0
> then this sounds like an issue.

mipsisa32r6 is a different incompatible ISA, so I don't see how it
poses an issue. Is there a particular concern you have that I'm
missing?

Rich
Szabolcs Nagy Sept. 27, 2019, 11:55 a.m.
* Rich Felker <dalias@libc.org> [2019-09-27 07:52:54 -0400]:

> On Fri, Sep 27, 2019 at 01:10:28PM +0200, Szabolcs Nagy wrote:
> > * Rich Felker <dalias@libc.org> [2019-09-26 20:38:21 -0400]:
> > > On Thu, Sep 26, 2019 at 07:23:50PM -0400, Rich Felker wrote:
> > > > On Thu, Sep 26, 2019 at 06:45:21PM -0400, Rich Felker wrote:
> > > > > Also, mipsr6 (the new mips-family ISA that's not compatible with
> > > > > previous mips) always uses the 64-bit register mode. We presently do
> > > > > not have setjmp/longjmp code that works with this case at all
> > > > > (existing code will wrongly save low 32-bits of 2 registers instead of
> > > > > single whole double register); somehow nobody has noticed that this is
> > > > > broken. Making this conditional on __mips_isa_rev >= 6 should not be
> > > > > hard.
> > > > 
> > > > Attached patch should work, but maybe isn't the best thing to do. I
> > > > think using sdc1/ldc1 and just even indices like on r6 would also be
> > > > valid for pre-r6 mips using fp32 or fpxx abi; with FR=0, it would
> > > > save/restore the pair of 32-bit registers, and with FR=1, fp32 code
> > > > could not be running anyway, and fpxx code should work fine. However,
> > > > mips I lacks the ldc1/stc1 instructions, so at the very least we'd
> > > > need to leave the old form in place for mips I. Or maybe use the s.d
> > > > and l.d mnemonics that automatically assemble to the right choice
> > > > based on the isa level...
> > > 
> > > Two new versions of the patch. I think I prefer the last one.
> > > 
> > > l.d and s.d expand to pairs of lwc1 and swc1 on mips1, and otherwise
> > > expand to ldc1 and sdc1. ldc1 and sdc1 in turn behave just like pairs
> > > of lwc1 and swc1 when FR=0, but additionally match the fpxx ABI when
> > > FR=1.
> > 
> > so a mips1 libc.so won't work on a system with FR=1?
> > but a mips2 libc.so works with both FR=1 and FR=0?
> 
> The ISA spec mandates that all mips r5 and earlier (this includes
> mips1, mips2, mips32 up to r5) support FR=0, and the ABI for the
> "mips" arch in musl is FR=0. So ability to work with FR=1 is not a
> requirement. If built as fpxx (the default on all but old toolchains
> or -march=mips1), the code could theoretically be linked with
> fp64-model code and run in FR=1 mode, but musl does not support this;
> doing it dynamically would require the dynamic linker manage the FR
> mode, which is outside the scope of our ABIs model.
> 
> > if mipsisa32r6 uses FR=1 and normal 32bit mips uses FR=0
> > then this sounds like an issue.
> 
> mipsisa32r6 is a different incompatible ISA, so I don't see how it
> poses an issue. Is there a particular concern you have that I'm
> missing?

ah right r6 uses different dynamic linker name so everything is fine then.

either patch looks ok to me.
Rich Felker Sept. 27, 2019, 2:44 p.m.
On Fri, Sep 27, 2019 at 01:55:06PM +0200, Szabolcs Nagy wrote:
> * Rich Felker <dalias@libc.org> [2019-09-27 07:52:54 -0400]:
> 
> > On Fri, Sep 27, 2019 at 01:10:28PM +0200, Szabolcs Nagy wrote:
> > > * Rich Felker <dalias@libc.org> [2019-09-26 20:38:21 -0400]:
> > > > On Thu, Sep 26, 2019 at 07:23:50PM -0400, Rich Felker wrote:
> > > > > On Thu, Sep 26, 2019 at 06:45:21PM -0400, Rich Felker wrote:
> > > > > > Also, mipsr6 (the new mips-family ISA that's not compatible with
> > > > > > previous mips) always uses the 64-bit register mode. We presently do
> > > > > > not have setjmp/longjmp code that works with this case at all
> > > > > > (existing code will wrongly save low 32-bits of 2 registers instead of
> > > > > > single whole double register); somehow nobody has noticed that this is
> > > > > > broken. Making this conditional on __mips_isa_rev >= 6 should not be
> > > > > > hard.
> > > > > 
> > > > > Attached patch should work, but maybe isn't the best thing to do. I
> > > > > think using sdc1/ldc1 and just even indices like on r6 would also be
> > > > > valid for pre-r6 mips using fp32 or fpxx abi; with FR=0, it would
> > > > > save/restore the pair of 32-bit registers, and with FR=1, fp32 code
> > > > > could not be running anyway, and fpxx code should work fine. However,
> > > > > mips I lacks the ldc1/stc1 instructions, so at the very least we'd
> > > > > need to leave the old form in place for mips I. Or maybe use the s.d
> > > > > and l.d mnemonics that automatically assemble to the right choice
> > > > > based on the isa level...
> > > > 
> > > > Two new versions of the patch. I think I prefer the last one.
> > > > 
> > > > l.d and s.d expand to pairs of lwc1 and swc1 on mips1, and otherwise
> > > > expand to ldc1 and sdc1. ldc1 and sdc1 in turn behave just like pairs
> > > > of lwc1 and swc1 when FR=0, but additionally match the fpxx ABI when
> > > > FR=1.
> > > 
> > > so a mips1 libc.so won't work on a system with FR=1?
> > > but a mips2 libc.so works with both FR=1 and FR=0?
> > 
> > The ISA spec mandates that all mips r5 and earlier (this includes
> > mips1, mips2, mips32 up to r5) support FR=0, and the ABI for the
> > "mips" arch in musl is FR=0. So ability to work with FR=1 is not a
> > requirement. If built as fpxx (the default on all but old toolchains
> > or -march=mips1), the code could theoretically be linked with
> > fp64-model code and run in FR=1 mode, but musl does not support this;
> > doing it dynamically would require the dynamic linker manage the FR
> > mode, which is outside the scope of our ABIs model.
> > 
> > > if mipsisa32r6 uses FR=1 and normal 32bit mips uses FR=0
> > > then this sounds like an issue.
> > 
> > mipsisa32r6 is a different incompatible ISA, so I don't see how it
> > poses an issue. Is there a particular concern you have that I'm
> > missing?
> 
> ah right r6 uses different dynamic linker name so everything is fine then.
> 
> either patch looks ok to me.

Thanks. Applying the latesst one since it should also solve the clang
issue.

Rich