[PATCH 0/4] Use varargs for common syscall() implementation
Hello, This is a followup to the ("f764bcffe" or1k: syscall: Pass arguments on the stack) patch which fixes the issue where the definition of syscall() in unistd.h and the common implementation in uclibc-ng don't match. This series allows the implementation to match the definition and then goes on to remove generic implementations used by or1k, nds32 and arc. -Stafford Stafford Horne (4): syscall: Make common implementation match unistd.h or1k: Use new common syscall() implementation nds32: Use new common syscall() implementation arc: Use new common syscall() implementation libc/sysdeps/linux/arc/Makefile.arch | 2 +- libc/sysdeps/linux/arc/syscall.c | 17 ----------------- libc/sysdeps/linux/common/syscall.c | 18 +++++++++++++++++- libc/sysdeps/linux/nds32/Makefile.arch | 4 ++-- libc/sysdeps/linux/nds32/syscall.c | 28 ---------------------------- libc/sysdeps/linux/or1k/Makefile.arch | 2 +- libc/sysdeps/linux/or1k/syscall.c | 32 -------------------------------- 7 files changed, 21 insertions(+), 82 deletions(-) delete mode 100644 libc/sysdeps/linux/arc/syscall.c delete mode 100644 libc/sysdeps/linux/nds32/syscall.c delete mode 100644 libc/sysdeps/linux/or1k/syscall.c -- 2.13.6
The definition of syscall() in unistd.h is with varargs. Traditionally the common implementation in uclibc has been with regular arguments. This patch updates that by using varargs. This has caused issues on architectures like or1k which have different calling conventions for varargs and regular arg parameters. The implementation here is based on an implementation from Joel Stanley <joel@jms.id.au>. There is a difference that I do not initialize the stack args with 0 as they are immediately overwritten by va_args. Signed-off-by: Stafford Horne <shorne@gmail.com> --- libc/sysdeps/linux/common/syscall.c | 18 +++++++++++++++++- 1 file changed, 17 insertions(+), 1 deletion(-) diff --git a/libc/sysdeps/linux/common/syscall.c b/libc/sysdeps/linux/common/syscall.c index 61f798e2c..d173d2c54 100644 --- a/libc/sysdeps/linux/common/syscall.c +++ b/libc/sysdeps/linux/common/syscall.c @@ -4,9 +4,25 @@ * Licensed under the LGPL v2.1, see the file COPYING.LIB in this tarball. */ +#include <stdarg.h> #include <sys/syscall.h> +#include <unistd.h> -long syscall(long sysnum, long arg1, long arg2, long arg3, long arg4, long arg5, long arg6) +long syscall(long sysnum, ...) { + + unsigned long arg1, arg2, arg3, arg4, arg5, arg6; + va_list arg; + + va_start (arg, sysnum); + arg1 = va_arg (arg, unsigned long); + arg2 = va_arg (arg, unsigned long); + arg3 = va_arg (arg, unsigned long); + arg4 = va_arg (arg, unsigned long); + arg5 = va_arg (arg, unsigned long); + arg6 = va_arg (arg, unsigned long); + va_end (arg); + + __asm__ volatile ( "" ::: "memory" ); return INLINE_SYSCALL_NCS(sysnum, 6, arg1, arg2, arg3, arg4, arg5, arg6); } -- 2.13.6
Now that the common syscall implementation supports vararg calling conventions we can safely use it on OpenRISC. This saves a bit of code in the openrisc implementation. Signed-off-by: Stafford Horne <shorne@gmail.com> --- libc/sysdeps/linux/or1k/Makefile.arch | 2 +- libc/sysdeps/linux/or1k/syscall.c | 32 -------------------------------- 2 files changed, 1 insertion(+), 33 deletions(-) delete mode 100644 libc/sysdeps/linux/or1k/syscall.c diff --git a/libc/sysdeps/linux/or1k/Makefile.arch b/libc/sysdeps/linux/or1k/Makefile.arch index 191eebafd..f6758fa63 100644 --- a/libc/sysdeps/linux/or1k/Makefile.arch +++ b/libc/sysdeps/linux/or1k/Makefile.arch @@ -5,5 +5,5 @@ # Licensed under the LGPL v2.1, see the file COPYING.LIB in this tarball. # -CSRC-y := __syscall_error.c __init_brk.c brk.c sbrk.c clone.c syscall.c +CSRC-y := __syscall_error.c __init_brk.c brk.c sbrk.c clone.c SSRC-y := __longjmp.S setjmp.S or1k_clone.S diff --git a/libc/sysdeps/linux/or1k/syscall.c b/libc/sysdeps/linux/or1k/syscall.c deleted file mode 100644 index 2f4356737..000000000 --- a/libc/sysdeps/linux/or1k/syscall.c +++ /dev/null @@ -1,32 +0,0 @@ -/* - * Copyright (C) 2017 Joel Stanley <joel@jms.id.au> - * Licensed under the LGPL v2.1, see the file COPYING.LIB in this tarball. - */ - -#include <stdarg.h> -#include <sys/syscall.h> -#include <unistd.h> - -long int syscall (long num, ...) -{ - unsigned long arg1 = 0; - unsigned long arg2 = 0; - unsigned long arg3 = 0; - unsigned long arg4 = 0; - unsigned long arg5 = 0; - unsigned long arg6 = 0; - va_list arg; - - va_start (arg, num); - arg1 = va_arg (arg, unsigned long); - arg2 = va_arg (arg, unsigned long); - arg3 = va_arg (arg, unsigned long); - arg4 = va_arg (arg, unsigned long); - arg5 = va_arg (arg, unsigned long); - arg6 = va_arg (arg, unsigned long); - va_end (arg); - - __asm__ volatile ( "" ::: "memory" ); - - return INLINE_SYSCALL_NCS(num, 6, arg1, arg2, arg3, arg4, arg5, arg6); -} -- 2.13.6
Traditionally nds32 has had a generic syscall implementation supporting varargs. During an audit it was found that this implementation seems to duplicate the new common implementation and is no longer needed. Signed-off-by: Stafford Horne <shorne@gmail.com> --- libc/sysdeps/linux/nds32/Makefile.arch | 4 ++-- libc/sysdeps/linux/nds32/syscall.c | 28 ---------------------------- 2 files changed, 2 insertions(+), 30 deletions(-) delete mode 100644 libc/sysdeps/linux/nds32/syscall.c diff --git a/libc/sysdeps/linux/nds32/Makefile.arch b/libc/sysdeps/linux/nds32/Makefile.arch index d5cdddbfa..caf163844 100644 --- a/libc/sysdeps/linux/nds32/Makefile.arch +++ b/libc/sysdeps/linux/nds32/Makefile.arch @@ -1,7 +1,7 @@ # Copyright (C) 2016 Andes Technology, Inc. # Licensed under the LGPL v2.1, see the file COPYING.LIB in this tarball. -CSRC-y := brk.c prctl.c mremap.c syscall.c -SSRC-y := setjmp.S __longjmp.S bsd-setjmp.S bsd-_setjmp.S clone.S vfork.S sysdep.S syscall.S +CSRC-y := brk.c prctl.c mremap.c +SSRC-y := setjmp.S __longjmp.S bsd-setjmp.S bsd-_setjmp.S clone.S vfork.S sysdep.S CSRC-$(UCLIBC_HAS_CONTEXT_FUNCS) += makecontext.c swapcontext.c SSRC-$(UCLIBC_HAS_CONTEXT_FUNCS) += getcontext.S setcontext.S diff --git a/libc/sysdeps/linux/nds32/syscall.c b/libc/sysdeps/linux/nds32/syscall.c deleted file mode 100644 index 2c949ef3a..000000000 --- a/libc/sysdeps/linux/nds32/syscall.c +++ /dev/null @@ -1,28 +0,0 @@ -/* - * Copyright (C) 2017 Andes Technology, Inc. - * Licensed under the LGPL v2.1, see the file COPYING.LIB in this tarball. - */ - -#include <errno.h> -#include <stdarg.h> -#include <sys/syscall.h> -#include <sysdep.h> -#include <unistd.h> -long int syscall (long int __sysno, ...) -{ - - int result; - unsigned long arg1,arg2,arg3,arg4,arg5,arg6; - va_list arg; - va_start (arg, __sysno); - arg1 = va_arg (arg, unsigned long); - arg2 = va_arg (arg, unsigned long); - arg3 = va_arg (arg, unsigned long); - arg4 = va_arg (arg, unsigned long); - arg5 = va_arg (arg, unsigned long); - arg6 = va_arg (arg, unsigned long); - va_end (arg); - __asm__ volatile ( "" ::: "memory" ); - result = INLINE_SYSCALL(syscall,7,__sysno,arg1,arg2,arg3,arg4,arg5,arg6); - return result; -} -- 2.13.6
Hi Stafford, Stafford Horne wrote,
Traditionally nds32 has had a generic syscall implementation supporting varargs.
During an audit it was found that this implementation seems to duplicate the new common implementation and is no longer needed.
nds32 syscall is special. I get following compile error: libc/sysdeps/linux/common/syscall.c: In function 'syscall': libc/sysdeps/linux/common/syscall.c:27:2: warning: implicit declaration of function 'internal_syscall_ncs6' [-Wimplicit-function-declaration] return INLINE_SYSCALL_NCS(sysnum, 6, arg1, arg2, arg3, arg4, arg5, arg6); ^ In file included from ./include/sys/syscall.h:33:0, from libc/sysdeps/linux/common/syscall.c:8: ./include/bits/syscalls-common.h:49:39: error: '__err' undeclared (first use in this function) __res = INTERNAL_SYSCALL_NCS(num, __err, nr, args); \ Not sure if we should touch nds32. best regards Waldemar
Hello, On Tue, Dec 26, 2017 at 7:44 PM, Waldemar Brodkorb <wbx@uclibc-ng.org> wrote:
Hi Stafford, Stafford Horne wrote,
Traditionally nds32 has had a generic syscall implementation supporting varargs.
During an audit it was found that this implementation seems to duplicate the new common implementation and is no longer needed.
nds32 syscall is special. I get following compile error: libc/sysdeps/linux/common/syscall.c: In function 'syscall': libc/sysdeps/linux/common/syscall.c:27:2: warning: implicit declaration of function 'internal_syscall_ncs6' [-Wimplicit-function-declaration] return INLINE_SYSCALL_NCS(sysnum, 6, arg1, arg2, arg3, arg4, arg5, arg6); ^ In file included from ./include/sys/syscall.h:33:0, from libc/sysdeps/linux/common/syscall.c:8: ./include/bits/syscalls-common.h:49:39: error: '__err' undeclared (first use in this function) __res = INTERNAL_SYSCALL_NCS(num, __err, nr, args); \
Not sure if we should touch nds32.
Looking at : libc/sysdeps/linux/nds32/bits/syscalls.h It may be easy to add, but maybe we can leave this for the maintainers to reply. -Stafford
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 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); -} -- 2.13.6
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
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
Hi Stafford, Stafford Horne wrote,
Hello,
This is a followup to the ("f764bcffe" or1k: syscall: Pass arguments on the stack) patch which fixes the issue where the definition of syscall() in unistd.h and the common implementation in uclibc-ng don't match.
This series allows the implementation to match the definition and then goes on to remove generic implementations used by or1k, nds32 and arc.
Finally applied and pushed the or1k and nds32 patches. Arc patches are dropped as suggested by Vineet. The nds32 patchset was fixed up a little before pushing. No regressions found by running the uClibc-ng testsuite. Thanks a lot, Waldemar
participants (3)
-
Stafford Horne -
Vineet Gupta -
Waldemar Brodkorb