diff mbox series

[3/3] mips: remove useless register spill

Message ID 20201114064200.1214-4-huangpei@loongson.cn
State Superseded
Headers show
Series [1/3] mips: add hp-timing support for MIPS R2 | expand

Commit Message

Huang Pei Nov. 14, 2020, 6:42 a.m. UTC
Signed-off-by: Huang Pei <huangpei@loongson.cn>
---
 sysdeps/unix/sysv/linux/mips/mips64/syscall.S | 14 ++------------
 1 file changed, 2 insertions(+), 12 deletions(-)

Comments

Maciej W. Rozycki Nov. 15, 2020, 1:22 a.m. UTC | #1
On Sat, 14 Nov 2020, Huang Pei wrote:

> Signed-off-by: Huang Pei <huangpei@loongson.cn>

 Please be a bit more verbose in the change description.  This used to be 
an implementation of the old syscall restart convention, where $v0 had to 
be (re)loaded by the instruction immediately preceding SYSCALL, obviously
from a source that does not get clobbered in syscall processing (i.e. 
either memory or a static register).  I imagine a good change description 
would say at least actually why this convention is no longer relevant.

 Refer to the comment in sysdeps/unix/sysv/linux/mips/mips32/sysdep.h or
sysdeps/unix/sysv/linux/mips/mips64/sysdep.h for further details.

 Also I am not sure if it makes sense to remove this single instance only, 
possibly for the least used syscall in user code, and leave all the other 
places unchanged.

  Maciej
Huang Pei Dec. 5, 2020, 5:47 a.m. UTC | #2
On Fri, Dec 04, 2020 at 11:24:24AM +0000, Maciej W. Rozycki wrote:
> Hi Huang Pei,
> 
>  Apologies for my late reply, I've had a hectic time recently and some GCC 
> stuff had to take priority due to the end of Stage 1 there.
> 
> > I must admit that I did not realize that Patch 3  would cause compatibiliy
> > issue, that is Linux/MIPS kernel before 2.6.36 depended on
> > "move a0, s0 ' just preceding "syscall" to make SYSCALL Restart
> > work, while  Linux/MIPS after 2.6.36 do not
> 
>  Or `move v0, s0' (in this case) to be correct.
Yes
> 
>  Yes, this has been quite obscure and used to be hardly documented up to 
> the point that breakage happened while the old scheme was still supported.  
> And given that syscall restarts are not at all that frequent any symptom 
> would be limited to an occasional program crash or other odd behaviour as 
> the wrong syscall was dispatched upon a restart.
> 
>  This is why I added the explanation along with fixes for BZ #15054 made 
> with commit b82ba2f011fc ("MIPS: Respect the legacy syscall restart 
> convention.").  Before that we only had Linux sources as a reference.
> 
>  NB the old scheme was removed from Linux with commit 8f5a00eb422e ("MIPS: 
> Sanitize restart logics").
> 
> > since glibc 2.24,the minimum kernel version is 3.2, is it ok to add Patch
> > 3 for glibc after version 2.24? If so, I would send V3, which merge Patch
> > 2 and Patch 3 and rewrite the change description with minimium kernel
> > version requirement.
> 
>  As I noted elsewhere I object such a change if made on its own, as it 
> would only cause a mess.

This one was still a mess, since the ".set noreorder" is within
b82ba2f011fc, but missing here(I hate '.set noreorder', which bite me
too much)

I came to this one accidentally when I addressed a unaligned access issue, 
but the fix is not the key point(see Patch V1 2/3), and Patch V1 3/3
without touching ".set noreorder"(I focus on) is.

PS:

I think it would be mutch better without both ".set reorder" and ".set 
noreorder", and with 8f5a00eb422e, we can remove more ".set noreorder"


> 
>  You can review my commit referred above, and also commit 43301bd3c281 
> ("Add support for building as MIPS16 code.") to see what has to be done to 
> remove bits for the old feature in a consistent manner.
> 
>   Maciej

Now it is OK to remove the mess as a whole, I will send V3 to get rid of it
all
diff mbox series

Patch

diff --git a/sysdeps/unix/sysv/linux/mips/mips64/syscall.S b/sysdeps/unix/sysv/linux/mips/mips64/syscall.S
index aab1f389aa..089524a40b 100644
--- a/sysdeps/unix/sysv/linux/mips/mips64/syscall.S
+++ b/sysdeps/unix/sysv/linux/mips/mips64/syscall.S
@@ -27,14 +27,9 @@ 
 
 	.text
 NESTED (syscall, SZREG, ra)
-	.mask 0x00010000, -2 * SZREG
+	.mask 0x00000000, 0
 	.fmask 0x00000000, 0
-	PTR_ADDIU sp, -2 * SZREG
-	cfi_adjust_cfa_offset (2 * SZREG)
-	REG_S s0, (sp)
-	cfi_rel_offset (s0, 0)
-
-	move s0, a0
+	move v0, a0
 	move a0, a1		/* shift arg1 - arg7.  */
 	move a1, a2
 	move a2, a3
@@ -43,13 +38,8 @@  NESTED (syscall, SZREG, ra)
 	move a5, a6
 	move a6, a7
 
-	move v0, s0		/* Syscall number -> v0 */
 	syscall			/* Do the system call.  */
 
-	REG_L s0, (sp)
-	cfi_restore (s0)
-	PTR_ADDIU sp, 2 * SZREG
-	cfi_adjust_cfa_offset (-2 * 2 * SZREG)
 	bne a3, zero, L(error)
 
 	ret