[uclibc-ng-devel,4/4] arc: Use new common syscall() implementation

Message ID 20171214062909.8602-5-shorne@gmail.com
State New
Headers show
Series
  • Use varargs for common syscall() implementation
Related show

Commit Message

Stafford Horne Dec. 14, 2017, 6:29 a.m.
Traditionally arc has had a generic syscall implementation of syscall()
matchig the common implementation.

During an audit it was found that this implementation seems to duplicate
common implementation and is no longer needed.

Signed-off-by: Stafford Horne <shorne@gmail.com>
---
 libc/sysdeps/linux/arc/Makefile.arch |  2 +-
 libc/sysdeps/linux/arc/syscall.c     | 17 -----------------
 2 files changed, 1 insertion(+), 18 deletions(-)
 delete mode 100644 libc/sysdeps/linux/arc/syscall.c

Comments

Vineet Gupta Jan. 5, 2018, 11:33 p.m. | #1
On 12/13/2017 10:29 PM, Stafford Horne wrote:
> Traditionally arc has had a generic syscall implementation of syscall()
> matchig the common implementation.
> 
> During an audit it was found that this implementation seems to duplicate
> common implementation and is no longer needed.
> 
> Signed-off-by: Stafford Horne <shorne@gmail.com>
> ---
>   libc/sysdeps/linux/arc/Makefile.arch |  2 +-
>   libc/sysdeps/linux/arc/syscall.c     | 17 -----------------
>   2 files changed, 1 insertion(+), 18 deletions(-)

[snip]
  -
> -extern long syscall(long int sysnum, long a, long b, long c, long d, long e, long f);
> -
> -long syscall(long int sysnum, long a, long b, long c, long d, long e, long f)
> -{
> -	return INLINE_SYSCALL_NCS(sysnum, 6, a, b, c, d, e, f);
> -}

varargs generate pathetic code on ARC atleast, it is partly due to the ABI (the 
args need to be stored on stack)

With current implementation we get

000100ac <syscall>:
    100ac:	mov_s	r8,r0
    100ae:	mov_s	r0,r1
    100b0:	mov_s	r1,r2
    100b2:	mov_s	r2,r3
    100b4:	mov_s	r3,r4
    100b6:	mov_s	r4,r5
    100b8:	mov_s	r5,r6
    100ba:	trap_s	0
    100bc:	cmp	r0,-1024
    100c0:	jls	[blink]
    100c4:	st.aw	blink,[sp,-4]
    100c8:	bl	84	;1011c <__syscall_error>
    100cc:	ld.ab	blink,[sp,4]
    100d0:	j_s	[blink]
    100d2:	nop_s

With varagrs implementation we get tons of useless stores. So I'd prefer we keep 
the current implementation for ARC - although granted syscall() may not be the 
fast path syscall.

00010398 <syscall>:
    10398:	sub_s	sp,sp,0x1c
    1039a:	mov_s	r8,r0
    1039c:	st_s	r1,[sp,0]
    1039e:	st_s	r2,[sp,0x4]
    103a0:	st_s	r3,[sp,0x8]
    103a2:	st	r4,[sp,12]
    103a6:	st	r5,[sp,16]
    103aa:	st	r6,[sp,20]
    103ae:	st	r7,[sp,24]
    103b2:	mov_s	r0,r1
    103b4:	mov_s	r1,r2
    103b6:	mov_s	r2,r3
    103b8:	mov_s	r3,r4
    103ba:	mov_s	r4,r5
    103bc:	mov_s	r5,r6
    103be:	trap_s	0
    103c0:	brhs.nt	0xfffffc00,r0,20	;103d4 <syscall+0x3c>
    103c8:	st.aw	blink,[sp,-4]
    103cc:	bl	84	;10420 <__syscall_error>
    103d0:	ld.ab	blink,[sp,4]
    103d4:	j_s.d	[blink]
    103d6:	add_s	sp,sp,0x1c
Stafford Horne Jan. 6, 2018, 2:57 a.m. | #2
On Fri, Jan 05, 2018 at 03:33:55PM -0800, Vineet Gupta wrote:
> On 12/13/2017 10:29 PM, Stafford Horne wrote:
> > Traditionally arc has had a generic syscall implementation of syscall()
> > matchig the common implementation.
> > 
> > During an audit it was found that this implementation seems to duplicate
> > common implementation and is no longer needed.
> > 
> > Signed-off-by: Stafford Horne <shorne@gmail.com>
> > ---
> >   libc/sysdeps/linux/arc/Makefile.arch |  2 +-
> >   libc/sysdeps/linux/arc/syscall.c     | 17 -----------------
> >   2 files changed, 1 insertion(+), 18 deletions(-)
> 
> [snip]
>  -
> > -extern long syscall(long int sysnum, long a, long b, long c, long d, long e, long f);
> > -
> > -long syscall(long int sysnum, long a, long b, long c, long d, long e, long f)
> > -{
> > -	return INLINE_SYSCALL_NCS(sysnum, 6, a, b, c, d, e, f);
> > -}
> 
> varargs generate pathetic code on ARC atleast, it is partly due to the ABI
> (the args need to be stored on stack)
> 
> With current implementation we get
> 
> 000100ac <syscall>:
>    100ac:	mov_s	r8,r0
>    100ae:	mov_s	r0,r1
>    100b0:	mov_s	r1,r2
>    100b2:	mov_s	r2,r3
>    100b4:	mov_s	r3,r4
>    100b6:	mov_s	r4,r5
>    100b8:	mov_s	r5,r6
>    100ba:	trap_s	0
>    100bc:	cmp	r0,-1024
>    100c0:	jls	[blink]
>    100c4:	st.aw	blink,[sp,-4]
>    100c8:	bl	84	;1011c <__syscall_error>
>    100cc:	ld.ab	blink,[sp,4]
>    100d0:	j_s	[blink]
>    100d2:	nop_s
> 
> With varagrs implementation we get tons of useless stores. So I'd prefer we
> keep the current implementation for ARC - although granted syscall() may not
> be the fast path syscall.

Hello Vineet,

Thanks for testing, I did realize varargs might generate slightly less optimal
code.  But I figured a few thing things

 - syscall() is probably not going to be used in any critical code sections.
 - the compiler could be optimized to make the varargs handling more optimal
 - we could remove code (but actually ends up with a bigger binary, so not
   great)

But for now, lets just drop this patch.  Thanks for the feedback.

-Stafford

> 00010398 <syscall>:
>    10398:	sub_s	sp,sp,0x1c
>    1039a:	mov_s	r8,r0
>    1039c:	st_s	r1,[sp,0]
>    1039e:	st_s	r2,[sp,0x4]
>    103a0:	st_s	r3,[sp,0x8]
>    103a2:	st	r4,[sp,12]
>    103a6:	st	r5,[sp,16]
>    103aa:	st	r6,[sp,20]
>    103ae:	st	r7,[sp,24]
>    103b2:	mov_s	r0,r1
>    103b4:	mov_s	r1,r2
>    103b6:	mov_s	r2,r3
>    103b8:	mov_s	r3,r4
>    103ba:	mov_s	r4,r5
>    103bc:	mov_s	r5,r6
>    103be:	trap_s	0
>    103c0:	brhs.nt	0xfffffc00,r0,20	;103d4 <syscall+0x3c>
>    103c8:	st.aw	blink,[sp,-4]
>    103cc:	bl	84	;10420 <__syscall_error>
>    103d0:	ld.ab	blink,[sp,4]
>    103d4:	j_s.d	[blink]
>    103d6:	add_s	sp,sp,0x1c

Patch

diff --git a/libc/sysdeps/linux/arc/Makefile.arch b/libc/sysdeps/linux/arc/Makefile.arch
index a4aa72c0a..7591a9892 100644
--- a/libc/sysdeps/linux/arc/Makefile.arch
+++ b/libc/sysdeps/linux/arc/Makefile.arch
@@ -5,7 +5,7 @@ 
 # Licensed under the LGPL v2.1 or later, see the file COPYING.LIB in this tarball.
 #
 
-CSRC-y := syscall.c sigaction.c __syscall_error.c cacheflush.c
+CSRC-y := sigaction.c __syscall_error.c cacheflush.c
 
 SSRC-y := __longjmp.S setjmp.S bsd-setjmp.S bsd-_setjmp.S vfork.S clone.S \
 	sigrestorer.S
diff --git a/libc/sysdeps/linux/arc/syscall.c b/libc/sysdeps/linux/arc/syscall.c
deleted file mode 100644
index 5648b0e1f..000000000
--- a/libc/sysdeps/linux/arc/syscall.c
+++ /dev/null
@@ -1,17 +0,0 @@ 
-/*
- * Copyright (C) 2013 Synopsys, Inc. (www.synopsys.com)
- *
- * Licensed under the LGPL v2.1 or later, see the file COPYING.LIB in this tarball.
- */
-
-#include <features.h>
-#include <errno.h>
-#include <sys/types.h>
-#include <sys/syscall.h>
-
-extern long syscall(long int sysnum, long a, long b, long c, long d, long e, long f);
-
-long syscall(long int sysnum, long a, long b, long c, long d, long e, long f)
-{
-	return INLINE_SYSCALL_NCS(sysnum, 6, a, b, c, d, e, f);
-}