mips/o32: fix internal_syscall5/6/7
Commit Message
On 2017-08-17 18:09, Adhemerval Zanella wrote:
>
>
> On 17/08/2017 17:34, Maciej W. Rozycki wrote:
> > On Thu, 17 Aug 2017, Adhemerval Zanella wrote:
> >
> >> My point is I think we should aim for compiler optimization safeness
> >> (to avoid code breakage over compiler defined default flags) and taking
> >> as base current approach to *avoid* VLA on GLIBC I do not think it is
> >> good approach to use it as a bridge to force GCC to generate the expected
> >> code.
> >
> > You certainly have a point here overall, although I don't think a VLA
> > whose size is always 0 really hurts. And we've used the approach with
> > `alloca' since forever with no adverse effects until we added a place
> > where the caller invokes the syscall wrapper in a loop. So I wouldn't
> > necessarily call it an issue. Mind that this is target-specific code, so
> > we can rely on a target-specific execution model rather than limiting
> > ourselves to what generic ISO C guarantees.
> >
> > Aurelien's figures indicating a clear size reduction certainly count as a
> > pro though.
>
> Joseph pointed out another advantage of avoid VLAs (building with
> -Werror=alloca -Werror=vla). My main problem here is we are betting that
> compiler won't mess with our assumptions and generate the desirable code
> without trying to adhere what it is suppose to provide. Target generic
> ISO C give us a better guarantee and any deviation indicates a possible
> compiler issue, not otherwise (such this case). My another point is we
> can optimize if required later if this is the case and imho this is hardly
> the case here (at least for latency).
>
> If I understood correctly Aurelien's suggestion of returning err in v1
> is not ABI strictly so it will end up calling __libc_do_syscall with a
> non-conformant ABI convention (similar to pipe implementation where requires
> assembly specific implementation for a lot of architectures to get this
> right). Again this is something I would really to avoid.
>
> >
> >> I still thinking trying to optimize for 5/6/7 syscall argument is over
> >> engineering in this *specific* case. As I put in my last message,
> >> 5/6/7 argument syscalls are used for
> >>
> >> pread, pwrite, lseek, llseek, ppoll, posix_fadvice, posix_fallocate,
> >> sync_file_range, fallocate, preadv, pwritev, preadv2, pwritev2, select,
> >> pselect, mmap, readahead, epoll_pwait, splice, recvfrom, sendto, recvmmsg,
> >> msgsnd, msgrcv, msgget, msgctl, semop, semget, semctl, semtimedop, shmat,
> >> shmdt, shmget, and shmctl.
> >>
> >> Which are the one generated from C implementation (some are still auto
> >> generated). The majority of them are blocking syscalls, so both context
> >> switch plus the required work for syscall completion itself will taking
> >> proportionally all the required time. So trying to squeeze some cycles
> >> don't really pay off comparing to code maintainability (just all this
> >> discussion of which C construct would be safe enough to generate the
> >> correct stack spill plus the current issue should indicate we should
> >> aim for correctness first).
> >
> > TBH, I find it questionable whether it's really the approach I proposed
> > that requires more engineering (and long-term maintenance) effort rather
> > than using a separate handwritten assembly-language call stub. Especially
> > if a non-standard calling convention is used.
>
> IMHO I find the VLA suggestion more fragile in long term.
>
> >
> > If everyone but me thinks there's a clear advantage in using such a
> > handcoded stub though, then as I previously noted please adjust the
> > affected MIPS16 stubs to avoid the extra indirection, i.e. you can call
> > `__libc_do_syscall' directly from MIPS16 code as you'd do from regular
> > MIPS or microMIPS code, as the lone reason for the existence of the MIPS16
> > stubs is the inexistence of a MIPS16 SYSCALL instruction.
>
> Ok, I will try to at least check it on qemu. If you have any points on how
> correctly build a mips16 glibc it could be helpful.
The patch below, based on Adhemerval's version should do it. The changes
I have done:
- return err through v1 instead of using a negative value
- fix build of mips16-syscallX.c
- route mips16 syscalls with 5 to 7th arguments through __libc_do_syscall
I see no regression on mipsel o32. I have only lightly tested mips o32
(the testsuite is still running). I haven't been able to fully compile
mips16 due to the following error compiling dl-tunables.c:
/tmp/ccI2NMgJ.s: Assembler messages:
/tmp/ccI2NMgJ.s:1376: Error: branch to a symbol in another ISA mode
It doesn't seem to be related to my changes.
Aurelien
Comments
On 17/08/2017 18:20, Aurelien Jarno wrote:
> On 2017-08-17 18:09, Adhemerval Zanella wrote:
>>
>>
>> On 17/08/2017 17:34, Maciej W. Rozycki wrote:
>>> On Thu, 17 Aug 2017, Adhemerval Zanella wrote:
>>>
>>>> My point is I think we should aim for compiler optimization safeness
>>>> (to avoid code breakage over compiler defined default flags) and taking
>>>> as base current approach to *avoid* VLA on GLIBC I do not think it is
>>>> good approach to use it as a bridge to force GCC to generate the expected
>>>> code.
>>>
>>> You certainly have a point here overall, although I don't think a VLA
>>> whose size is always 0 really hurts. And we've used the approach with
>>> `alloca' since forever with no adverse effects until we added a place
>>> where the caller invokes the syscall wrapper in a loop. So I wouldn't
>>> necessarily call it an issue. Mind that this is target-specific code, so
>>> we can rely on a target-specific execution model rather than limiting
>>> ourselves to what generic ISO C guarantees.
>>>
>>> Aurelien's figures indicating a clear size reduction certainly count as a
>>> pro though.
>>
>> Joseph pointed out another advantage of avoid VLAs (building with
>> -Werror=alloca -Werror=vla). My main problem here is we are betting that
>> compiler won't mess with our assumptions and generate the desirable code
>> without trying to adhere what it is suppose to provide. Target generic
>> ISO C give us a better guarantee and any deviation indicates a possible
>> compiler issue, not otherwise (such this case). My another point is we
>> can optimize if required later if this is the case and imho this is hardly
>> the case here (at least for latency).
>>
>> If I understood correctly Aurelien's suggestion of returning err in v1
>> is not ABI strictly so it will end up calling __libc_do_syscall with a
>> non-conformant ABI convention (similar to pipe implementation where requires
>> assembly specific implementation for a lot of architectures to get this
>> right). Again this is something I would really to avoid.
>>
>>>
>>>> I still thinking trying to optimize for 5/6/7 syscall argument is over
>>>> engineering in this *specific* case. As I put in my last message,
>>>> 5/6/7 argument syscalls are used for
>>>>
>>>> pread, pwrite, lseek, llseek, ppoll, posix_fadvice, posix_fallocate,
>>>> sync_file_range, fallocate, preadv, pwritev, preadv2, pwritev2, select,
>>>> pselect, mmap, readahead, epoll_pwait, splice, recvfrom, sendto, recvmmsg,
>>>> msgsnd, msgrcv, msgget, msgctl, semop, semget, semctl, semtimedop, shmat,
>>>> shmdt, shmget, and shmctl.
>>>>
>>>> Which are the one generated from C implementation (some are still auto
>>>> generated). The majority of them are blocking syscalls, so both context
>>>> switch plus the required work for syscall completion itself will taking
>>>> proportionally all the required time. So trying to squeeze some cycles
>>>> don't really pay off comparing to code maintainability (just all this
>>>> discussion of which C construct would be safe enough to generate the
>>>> correct stack spill plus the current issue should indicate we should
>>>> aim for correctness first).
>>>
>>> TBH, I find it questionable whether it's really the approach I proposed
>>> that requires more engineering (and long-term maintenance) effort rather
>>> than using a separate handwritten assembly-language call stub. Especially
>>> if a non-standard calling convention is used.
>>
>> IMHO I find the VLA suggestion more fragile in long term.
>>
>>>
>>> If everyone but me thinks there's a clear advantage in using such a
>>> handcoded stub though, then as I previously noted please adjust the
>>> affected MIPS16 stubs to avoid the extra indirection, i.e. you can call
>>> `__libc_do_syscall' directly from MIPS16 code as you'd do from regular
>>> MIPS or microMIPS code, as the lone reason for the existence of the MIPS16
>>> stubs is the inexistence of a MIPS16 SYSCALL instruction.
>>
>> Ok, I will try to at least check it on qemu. If you have any points on how
>> correctly build a mips16 glibc it could be helpful.
>
> The patch below, based on Adhemerval's version should do it. The changes
> I have done:
> - return err through v1 instead of using a negative value
> - fix build of mips16-syscallX.c
> - route mips16 syscalls with 5 to 7th arguments through __libc_do_syscall
Thanks for complementing it, I was about to send patched version with same
modification (I figured out 64 bits return ints can be used here reading
mips16 syscall code). Patch LGTM, I would just add some comments to give why
__libc_do_syscall is required (as for other ports). Comment below.
>
> I see no regression on mipsel o32. I have only lightly tested mips o32
> (the testsuite is still running). I haven't been able to fully compile
> mips16 due to the following error compiling dl-tunables.c:
>
> /tmp/ccI2NMgJ.s: Assembler messages:
> /tmp/ccI2NMgJ.s:1376: Error: branch to a symbol in another ISA mode
>
> It doesn't seem to be related to my changes.
I haven't tests with --enable-tunables, but I could build a mips16 with
default options. The only issue, although, is the new tst-gmon.c test:
tst-gmon.c: In function ‘f1’:
tst-gmon.c:25:1: sorry, unimplemented: mips16 function profiling
}
^
Which is unrelated to this patch.
>
> Aurelien
>
> diff --git a/sysdeps/unix/sysv/linux/mips/mips32/Makefile b/sysdeps/unix/sysv/linux/mips/mips32/Makefile
> index 33b461500c..cbdf032c3a 100644
> --- a/sysdeps/unix/sysv/linux/mips/mips32/Makefile
> +++ b/sysdeps/unix/sysv/linux/mips/mips32/Makefile
> @@ -1,8 +1,26 @@
> +ifeq ($(subdir),elf)
> +sysdep-dl-routines += libc-do-syscall
> +endif
> +
> ifeq ($(subdir),conform)
> # For bugs 17786 and 21278.
> conformtest-xfail-conds += mips-o32-linux
> endif
>
> +ifeq ($(subdir),io)
> +sysdep_routines += libc-do-syscall
> +endif
> +
> +ifeq ($(subdir),nptl)
> +libpthread-sysdep_routines += libc-do-syscall
> +libpthread-shared-only-routines += libc-do-syscall
> +endif
> +
> +ifeq ($(subdir),rt)
> +librt-sysdep_routines += libc-do-syscall
> +librt-shared-only-routines += libc-do-syscall
> +endif
> +
> ifeq ($(subdir),stdlib)
> tests += bug-getcontext-mips-gp
> endif
> diff --git a/sysdeps/unix/sysv/linux/mips/mips32/libc-do-syscall.S b/sysdeps/unix/sysv/linux/mips/mips32/libc-do-syscall.S
> new file mode 100644
> index 0000000000..c02f507008
> --- /dev/null
> +++ b/sysdeps/unix/sysv/linux/mips/mips32/libc-do-syscall.S
> @@ -0,0 +1,52 @@
> +/* Copyright (C) 2017 Free Software Foundation, Inc.
> + This file is part of the GNU C Library.
> +
> + The GNU C Library is free software; you can redistribute it and/or
> + modify it under the terms of the GNU Lesser General Public
> + License as published by the Free Software Foundation; either
> + version 2.1 of the License, or (at your option) any later version.
> +
> + The GNU C Library is distributed in the hope that it will be useful,
> + but WITHOUT ANY WARRANTY; without even the implied warranty of
> + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
> + Lesser General Public License for more details.
> +
> + You should have received a copy of the GNU Lesser General Public
> + License along with the GNU C Library. If not, see
> + <http://www.gnu.org/licenses/>. */
> +
> +#include <sys/asm.h>
> +#include <sysdep.h>
> +#include <asm/unistd.h>
> +#include <sgidefs.h>
> +
> +
> +/* long int __libc_do_syscall (long int, ...) */
I added some comments in my version:
/* Out-of-line syscall stub used for 5, 6, and 7 argument syscall which
requires arguments in stack. It follows the MIPS ABI similar to the C
prototype:
long int __libc_do_syscall (long int, ...)
With syscall number in a0, first argument in a1, second in a2, third
in a3 and the 4th-7th on stack. */
> +
> +#define FRAMESZ 32
> +
> + .text
> + .set nomips16
> + .hidden __libc_do_syscall
> +ENTRY(__libc_do_syscall)
> + move v0, a0
> + move a0, a1
> + move a1, a2
> + move a2, a3
> + lw a3, 16(sp)
> + lw t0, 20(sp)
> + lw t1, 24(sp)
> + lw t2, 28(sp)
> + .set noreorder
> + PTR_SUBU sp, FRAMESZ
> + cfi_adjust_cfa_offset (FRAMESZ)
> + sw t0, 16(sp)
> + sw t1, 20(sp)
> + sw t2, 24(sp)
> + syscall
> + PTR_ADDU sp, FRAMESZ
> + cfi_adjust_cfa_offset (-FRAMESZ)
> + .set reorder
> + move v1, a3
> +1: ret
> +END (__libc_do_syscall)
> diff --git a/sysdeps/unix/sysv/linux/mips/mips32/mips16/Makefile b/sysdeps/unix/sysv/linux/mips/mips32/mips16/Makefile
> index fa9fcb7e6f..6869bf4f7c 100644
> --- a/sysdeps/unix/sysv/linux/mips/mips32/mips16/Makefile
> +++ b/sysdeps/unix/sysv/linux/mips/mips32/mips16/Makefile
> @@ -1,13 +1,9 @@
> ifeq ($(subdir),misc)
> sysdep_routines += mips16-syscall0 mips16-syscall1 mips16-syscall2
> -sysdep_routines += mips16-syscall3 mips16-syscall4 mips16-syscall5
> -sysdep_routines += mips16-syscall6 mips16-syscall7
> +sysdep_routines += mips16-syscall3 mips16-syscall4
> CFLAGS-mips16-syscall0.c += -fexceptions
> CFLAGS-mips16-syscall1.c += -fexceptions
> CFLAGS-mips16-syscall2.c += -fexceptions
> CFLAGS-mips16-syscall3.c += -fexceptions
> CFLAGS-mips16-syscall4.c += -fexceptions
> -CFLAGS-mips16-syscall5.c += -fexceptions
> -CFLAGS-mips16-syscall6.c += -fexceptions
> -CFLAGS-mips16-syscall7.c += -fexceptions
> endif
> diff --git a/sysdeps/unix/sysv/linux/mips/mips32/mips16/Versions b/sysdeps/unix/sysv/linux/mips/mips32/mips16/Versions
> index 73bcfb566c..bb21747f44 100644
> --- a/sysdeps/unix/sysv/linux/mips/mips32/mips16/Versions
> +++ b/sysdeps/unix/sysv/linux/mips/mips32/mips16/Versions
> @@ -1,6 +1,6 @@
> libc {
> GLIBC_PRIVATE {
> __mips16_syscall0; __mips16_syscall1; __mips16_syscall2; __mips16_syscall3;
> - __mips16_syscall4; __mips16_syscall5; __mips16_syscall6; __mips16_syscall7;
> + __mips16_syscall4;
> }
> }
> diff --git a/sysdeps/unix/sysv/linux/mips/mips32/mips16/mips16-syscall.h b/sysdeps/unix/sysv/linux/mips/mips32/mips16/mips16-syscall.h
> index 880e9908e8..60f856d248 100644
> --- a/sysdeps/unix/sysv/linux/mips/mips32/mips16/mips16-syscall.h
> +++ b/sysdeps/unix/sysv/linux/mips/mips32/mips16/mips16-syscall.h
> @@ -21,17 +21,6 @@
>
> #define __nomips16 __attribute__ ((nomips16))
>
> -union __mips16_syscall_return
> - {
> - long long val;
> - struct
> - {
> - long v0;
> - long v1;
> - }
> - reg;
> - };
> -
> long long __nomips16 __mips16_syscall0 (long number);
> #define __mips16_syscall0(dummy, number) \
> __mips16_syscall0 ((long) (number))
> diff --git a/sysdeps/unix/sysv/linux/mips/mips32/mips16/mips16-syscall0.c b/sysdeps/unix/sysv/linux/mips/mips32/mips16/mips16-syscall0.c
> index 490245b34e..b9f78e875f 100644
> --- a/sysdeps/unix/sysv/linux/mips/mips32/mips16/mips16-syscall0.c
> +++ b/sysdeps/unix/sysv/linux/mips/mips32/mips16/mips16-syscall0.c
> @@ -24,7 +24,7 @@
> long long __nomips16
> __mips16_syscall0 (long number)
> {
> - union __mips16_syscall_return ret;
> + union __libc_do_syscall_return ret;
> ret.reg.v0 = INTERNAL_SYSCALL_MIPS16 (number, ret.reg.v1, 0);
> return ret.val;
> }
> diff --git a/sysdeps/unix/sysv/linux/mips/mips32/mips16/mips16-syscall1.c b/sysdeps/unix/sysv/linux/mips/mips32/mips16/mips16-syscall1.c
> index 3061e8accb..284ce712cc 100644
> --- a/sysdeps/unix/sysv/linux/mips/mips32/mips16/mips16-syscall1.c
> +++ b/sysdeps/unix/sysv/linux/mips/mips32/mips16/mips16-syscall1.c
> @@ -25,7 +25,7 @@ long long __nomips16
> __mips16_syscall1 (long a0,
> long number)
> {
> - union __mips16_syscall_return ret;
> + union __libc_do_syscall_return ret;
> ret.reg.v0 = INTERNAL_SYSCALL_MIPS16 (number, ret.reg.v1, 1,
> a0);
> return ret.val;
> diff --git a/sysdeps/unix/sysv/linux/mips/mips32/mips16/mips16-syscall2.c b/sysdeps/unix/sysv/linux/mips/mips32/mips16/mips16-syscall2.c
> index 440a4ed285..4e76329239 100644
> --- a/sysdeps/unix/sysv/linux/mips/mips32/mips16/mips16-syscall2.c
> +++ b/sysdeps/unix/sysv/linux/mips/mips32/mips16/mips16-syscall2.c
> @@ -25,7 +25,7 @@ long long __nomips16
> __mips16_syscall2 (long a0, long a1,
> long number)
> {
> - union __mips16_syscall_return ret;
> + union __libc_do_syscall_return ret;
> ret.reg.v0 = INTERNAL_SYSCALL_MIPS16 (number, ret.reg.v1, 2,
> a0, a1);
> return ret.val;
> diff --git a/sysdeps/unix/sysv/linux/mips/mips32/mips16/mips16-syscall3.c b/sysdeps/unix/sysv/linux/mips/mips32/mips16/mips16-syscall3.c
> index c3f83fc1f6..dbb31d2f20 100644
> --- a/sysdeps/unix/sysv/linux/mips/mips32/mips16/mips16-syscall3.c
> +++ b/sysdeps/unix/sysv/linux/mips/mips32/mips16/mips16-syscall3.c
> @@ -25,7 +25,7 @@ long long __nomips16
> __mips16_syscall3 (long a0, long a1, long a2,
> long number)
> {
> - union __mips16_syscall_return ret;
> + union __libc_do_syscall_return ret;
> ret.reg.v0 = INTERNAL_SYSCALL_MIPS16 (number, ret.reg.v1, 3,
> a0, a1, a2);
> return ret.val;
> diff --git a/sysdeps/unix/sysv/linux/mips/mips32/mips16/mips16-syscall4.c b/sysdeps/unix/sysv/linux/mips/mips32/mips16/mips16-syscall4.c
> index 496297d296..a5dade3b3f 100644
> --- a/sysdeps/unix/sysv/linux/mips/mips32/mips16/mips16-syscall4.c
> +++ b/sysdeps/unix/sysv/linux/mips/mips32/mips16/mips16-syscall4.c
> @@ -25,7 +25,7 @@ long long __nomips16
> __mips16_syscall4 (long a0, long a1, long a2, long a3,
> long number)
> {
> - union __mips16_syscall_return ret;
> + union __libc_do_syscall_return ret;
> ret.reg.v0 = INTERNAL_SYSCALL_MIPS16 (number, ret.reg.v1, 4,
> a0, a1, a2, a3);
> return ret.val;
> diff --git a/sysdeps/unix/sysv/linux/mips/mips32/mips16/mips16-syscall5.c b/sysdeps/unix/sysv/linux/mips/mips32/mips16/mips16-syscall5.c
> deleted file mode 100644
> index ad265d88e2..0000000000
> --- a/sysdeps/unix/sysv/linux/mips/mips32/mips16/mips16-syscall5.c
> +++ /dev/null
> @@ -1,33 +0,0 @@
> -/* MIPS16 syscall wrappers.
> - Copyright (C) 2013-2017 Free Software Foundation, Inc.
> - This file is part of the GNU C Library.
> -
> - The GNU C Library is free software; you can redistribute it and/or
> - modify it under the terms of the GNU Lesser General Public
> - License as published by the Free Software Foundation; either
> - version 2.1 of the License, or (at your option) any later version.
> -
> - The GNU C Library is distributed in the hope that it will be useful,
> - but WITHOUT ANY WARRANTY; without even the implied warranty of
> - MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
> - Lesser General Public License for more details.
> -
> - You should have received a copy of the GNU Lesser General Public
> - License along with the GNU C Library; if not, see
> - <http://www.gnu.org/licenses/>. */
> -
> -#include <sysdep.h>
> -#include <mips16-syscall.h>
> -
> -#undef __mips16_syscall5
> -
> -long long __nomips16
> -__mips16_syscall5 (long a0, long a1, long a2, long a3,
> - long a4,
> - long number)
> -{
> - union __mips16_syscall_return ret;
> - ret.reg.v0 = INTERNAL_SYSCALL_MIPS16 (number, ret.reg.v1, 5,
> - a0, a1, a2, a3, a4);
> - return ret.val;
> -}
> diff --git a/sysdeps/unix/sysv/linux/mips/mips32/mips16/mips16-syscall6.c b/sysdeps/unix/sysv/linux/mips/mips32/mips16/mips16-syscall6.c
> deleted file mode 100644
> index bfbd395ed3..0000000000
> --- a/sysdeps/unix/sysv/linux/mips/mips32/mips16/mips16-syscall6.c
> +++ /dev/null
> @@ -1,33 +0,0 @@
> -/* MIPS16 syscall wrappers.
> - Copyright (C) 2013-2017 Free Software Foundation, Inc.
> - This file is part of the GNU C Library.
> -
> - The GNU C Library is free software; you can redistribute it and/or
> - modify it under the terms of the GNU Lesser General Public
> - License as published by the Free Software Foundation; either
> - version 2.1 of the License, or (at your option) any later version.
> -
> - The GNU C Library is distributed in the hope that it will be useful,
> - but WITHOUT ANY WARRANTY; without even the implied warranty of
> - MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
> - Lesser General Public License for more details.
> -
> - You should have received a copy of the GNU Lesser General Public
> - License along with the GNU C Library; if not, see
> - <http://www.gnu.org/licenses/>. */
> -
> -#include <sysdep.h>
> -#include <mips16-syscall.h>
> -
> -#undef __mips16_syscall6
> -
> -long long __nomips16
> -__mips16_syscall6 (long a0, long a1, long a2, long a3,
> - long a4, long a5,
> - long number)
> -{
> - union __mips16_syscall_return ret;
> - ret.reg.v0 = INTERNAL_SYSCALL_MIPS16 (number, ret.reg.v1, 6,
> - a0, a1, a2, a3, a4, a5);
> - return ret.val;
> -}
> diff --git a/sysdeps/unix/sysv/linux/mips/mips32/mips16/mips16-syscall7.c b/sysdeps/unix/sysv/linux/mips/mips32/mips16/mips16-syscall7.c
> deleted file mode 100644
> index e1267616dc..0000000000
> --- a/sysdeps/unix/sysv/linux/mips/mips32/mips16/mips16-syscall7.c
> +++ /dev/null
> @@ -1,33 +0,0 @@
> -/* MIPS16 syscall wrappers.
> - Copyright (C) 2013-2017 Free Software Foundation, Inc.
> - This file is part of the GNU C Library.
> -
> - The GNU C Library is free software; you can redistribute it and/or
> - modify it under the terms of the GNU Lesser General Public
> - License as published by the Free Software Foundation; either
> - version 2.1 of the License, or (at your option) any later version.
> -
> - The GNU C Library is distributed in the hope that it will be useful,
> - but WITHOUT ANY WARRANTY; without even the implied warranty of
> - MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
> - Lesser General Public License for more details.
> -
> - You should have received a copy of the GNU Lesser General Public
> - License along with the GNU C Library; if not, see
> - <http://www.gnu.org/licenses/>. */
> -
> -#include <sysdep.h>
> -#include <mips16-syscall.h>
> -
> -#undef __mips16_syscall7
> -
> -long long __nomips16
> -__mips16_syscall7 (long a0, long a1, long a2, long a3,
> - long a4, long a5, long a6,
> - long number)
> -{
> - union __mips16_syscall_return ret;
> - ret.reg.v0 = INTERNAL_SYSCALL_MIPS16 (number, ret.reg.v1, 7,
> - a0, a1, a2, a3, a4, a5, a6);
> - return ret.val;
> -}
> diff --git a/sysdeps/unix/sysv/linux/mips/mips32/sysdep.h b/sysdeps/unix/sysv/linux/mips/mips32/sysdep.h
> index e9e3ee7e82..8e55538a5c 100644
> --- a/sysdeps/unix/sysv/linux/mips/mips32/sysdep.h
> +++ b/sysdeps/unix/sysv/linux/mips/mips32/sysdep.h
> @@ -49,9 +49,9 @@
> /* Define a macro which expands into the inline wrapper code for a system
> call. */
> #undef INLINE_SYSCALL
> -#define INLINE_SYSCALL(name, nr, args...) \
> +#define INLINE_SYSCALL(name, nr, ...) \
> ({ INTERNAL_SYSCALL_DECL (_sc_err); \
> - long result_var = INTERNAL_SYSCALL (name, _sc_err, nr, args); \
> + long result_var = INTERNAL_SYSCALL (name, _sc_err, nr, ## __VA_ARGS__); \
> if ( INTERNAL_SYSCALL_ERROR_P (result_var, _sc_err) ) \
> { \
> __set_errno (INTERNAL_SYSCALL_ERRNO (result_var, _sc_err)); \
> @@ -98,6 +98,19 @@
> #undef INTERNAL_SYSCALL
> #undef INTERNAL_SYSCALL_NCS
>
> +long long __attribute__ ((nomips16)) __libc_do_syscall (long int, ...) attribute_hidden;
Same as before, I think it worth adding a comment:
/* MIPS kernel ABI requires for internal_syscall5/6/7 that the 5th and
following arguments to be on the stack. To avoid trying force a stack
allocation (which compiler may optimize) the macros use a auxiliary
function to actually issue the syscall. */
> +
> +union __libc_do_syscall_return
> + {
> + long long val;
> + struct
> + {
> + long v0;
> + long v1;
> + }
> + reg;
> + };
> +
> #ifdef __mips16
> /* There's no MIPS16 syscall instruction, so we go through out-of-line
> standard MIPS wrappers. These do use inline snippets below though,
> @@ -107,13 +120,16 @@
>
> # include <mips16-syscall.h>
>
> -# define INTERNAL_SYSCALL(name, err, nr, args...) \
> - INTERNAL_SYSCALL_NCS (SYS_ify (name), err, nr, args)
> +# define INTERNAL_SYSCALL(name, err, nr, ...) \
> + INTERNAL_SYSCALL_NCS (SYS_ify (name), err, nr, ## __VA_ARGS__)
>
> -# define INTERNAL_SYSCALL_NCS(number, err, nr, args...) \
> +# define INTERNAL_SYSCALL_NCS(number, err, nr, ...) \
> ({ \
> - union __mips16_syscall_return _sc_ret; \
> - _sc_ret.val = __mips16_syscall##nr (args, number); \
> + union __libc_do_syscall_return _sc_ret; \
> + if (nr <= 4) \
> + _sc_ret.val = __mips16_syscall##nr (__VA_ARGS__, number); \
> + else \
> + _sc_ret.val = __libc_do_syscall (number, ## __VA_ARGS__); \
> err = _sc_ret.reg.v1; \
> _sc_ret.reg.v0; \
> })
> @@ -121,13 +137,13 @@
> # define INTERNAL_SYSCALL_MIPS16(number, err, nr, args...) \
> internal_syscall##nr ("lw\t%0, %2\n\t", \
> "R" (number), \
> - 0, err, args)
> + number, err, args)
>
> #else /* !__mips16 */
> # define INTERNAL_SYSCALL(name, err, nr, args...) \
> internal_syscall##nr ("li\t%0, %2\t\t\t# " #name "\n\t", \
> "IK" (SYS_ify (name)), \
> - 0, err, args)
> + SYS_ify(name), err, args)
>
> # define INTERNAL_SYSCALL_NCS(number, err, nr, args...) \
> internal_syscall##nr (MOVE32 "\t%0, %2\n\t", \
> @@ -136,6 +152,7 @@
>
> #endif /* !__mips16 */
>
> +
> #define internal_syscall0(v0_init, input, number, err, dummy...) \
> ({ \
> long _sys_result; \
> @@ -262,110 +279,34 @@
> _sys_result; \
> })
>
> -/* We need to use a frame pointer for the functions in which we
> - adjust $sp around the syscall, or debug information and unwind
> - information will be $sp relative and thus wrong during the syscall. As
> - of GCC 4.7, this is sufficient. */
> -#define FORCE_FRAME_POINTER \
> - void *volatile __fp_force __attribute__ ((unused)) = alloca (4)
> -
> #define internal_syscall5(v0_init, input, number, err, \
> arg1, arg2, arg3, arg4, arg5) \
> ({ \
> - long _sys_result; \
> - \
> - FORCE_FRAME_POINTER; \
> - { \
> - register long __s0 asm ("$16") __attribute__ ((unused)) \
> - = (number); \
> - register long __v0 asm ("$2"); \
> - register long __a0 asm ("$4") = (long) (arg1); \
> - register long __a1 asm ("$5") = (long) (arg2); \
> - register long __a2 asm ("$6") = (long) (arg3); \
> - register long __a3 asm ("$7") = (long) (arg4); \
> - __asm__ volatile ( \
> - ".set\tnoreorder\n\t" \
> - "subu\t$29, 32\n\t" \
> - "sw\t%6, 16($29)\n\t" \
> - v0_init \
> - "syscall\n\t" \
> - "addiu\t$29, 32\n\t" \
> - ".set\treorder" \
> - : "=r" (__v0), "+r" (__a3) \
> - : input, "r" (__a0), "r" (__a1), "r" (__a2), \
> - "r" ((long) (arg5)) \
> - : __SYSCALL_CLOBBERS); \
> - err = __a3; \
> - _sys_result = __v0; \
> - } \
> - _sys_result; \
> + union __libc_do_syscall_return _sys_result; \
> + _sys_result.val = __libc_do_syscall (number, arg1, arg2, arg3, \
> + arg4, arg5); \
> + err = _sys_result.reg.v1; \
> + _sys_result.reg.v0; \
> })
>
> #define internal_syscall6(v0_init, input, number, err, \
> arg1, arg2, arg3, arg4, arg5, arg6) \
> ({ \
> - long _sys_result; \
> - \
> - FORCE_FRAME_POINTER; \
> - { \
> - register long __s0 asm ("$16") __attribute__ ((unused)) \
> - = (number); \
> - register long __v0 asm ("$2"); \
> - register long __a0 asm ("$4") = (long) (arg1); \
> - register long __a1 asm ("$5") = (long) (arg2); \
> - register long __a2 asm ("$6") = (long) (arg3); \
> - register long __a3 asm ("$7") = (long) (arg4); \
> - __asm__ volatile ( \
> - ".set\tnoreorder\n\t" \
> - "subu\t$29, 32\n\t" \
> - "sw\t%6, 16($29)\n\t" \
> - "sw\t%7, 20($29)\n\t" \
> - v0_init \
> - "syscall\n\t" \
> - "addiu\t$29, 32\n\t" \
> - ".set\treorder" \
> - : "=r" (__v0), "+r" (__a3) \
> - : input, "r" (__a0), "r" (__a1), "r" (__a2), \
> - "r" ((long) (arg5)), "r" ((long) (arg6)) \
> - : __SYSCALL_CLOBBERS); \
> - err = __a3; \
> - _sys_result = __v0; \
> - } \
> - _sys_result; \
> + union __libc_do_syscall_return _sys_result; \
> + _sys_result.val = __libc_do_syscall (number, arg1, arg2, arg3, \
> + arg4, arg5, arg6); \
> + err = _sys_result.reg.v1; \
> + _sys_result.reg.v0; \
> })
>
> #define internal_syscall7(v0_init, input, number, err, \
> arg1, arg2, arg3, arg4, arg5, arg6, arg7) \
> ({ \
> - long _sys_result; \
> - \
> - FORCE_FRAME_POINTER; \
> - { \
> - register long __s0 asm ("$16") __attribute__ ((unused)) \
> - = (number); \
> - register long __v0 asm ("$2"); \
> - register long __a0 asm ("$4") = (long) (arg1); \
> - register long __a1 asm ("$5") = (long) (arg2); \
> - register long __a2 asm ("$6") = (long) (arg3); \
> - register long __a3 asm ("$7") = (long) (arg4); \
> - __asm__ volatile ( \
> - ".set\tnoreorder\n\t" \
> - "subu\t$29, 32\n\t" \
> - "sw\t%6, 16($29)\n\t" \
> - "sw\t%7, 20($29)\n\t" \
> - "sw\t%8, 24($29)\n\t" \
> - v0_init \
> - "syscall\n\t" \
> - "addiu\t$29, 32\n\t" \
> - ".set\treorder" \
> - : "=r" (__v0), "+r" (__a3) \
> - : input, "r" (__a0), "r" (__a1), "r" (__a2), \
> - "r" ((long) (arg5)), "r" ((long) (arg6)), "r" ((long) (arg7)) \
> - : __SYSCALL_CLOBBERS); \
> - err = __a3; \
> - _sys_result = __v0; \
> - } \
> - _sys_result; \
> + union __libc_do_syscall_return _sys_result; \
> + _sys_result.val = __libc_do_syscall (number, arg1, arg2, arg3, \
> + arg4, arg5, arg6, arg7); \
> + err = _sys_result.reg.v1; \
> + _sys_result.reg.v0; \
> })
>
> #define __SYSCALL_CLOBBERS "$1", "$3", "$8", "$9", "$10", "$11", "$12", "$13", \
>
>
On Thu, 17 Aug 2017, Aurelien Jarno wrote:
> I see no regression on mipsel o32. I have only lightly tested mips o32
> (the testsuite is still running). I haven't been able to fully compile
> mips16 due to the following error compiling dl-tunables.c:
>
> /tmp/ccI2NMgJ.s: Assembler messages:
> /tmp/ccI2NMgJ.s:1376: Error: branch to a symbol in another ISA mode
>
> It doesn't seem to be related to my changes.
If it's a regression, then it probably is. What compiler version? The
fix for missing trailing label annotation went in r242424, for GCC 7. If
it's in handcoded assembly OTOH, then the offending code has to be fixed.
Send me the generated assembly and I'll tell you what the case is. If
it's due to old buggy compiler and the branch target is really never
reached, then you can use `-Wa,-mignore-branch-isa' as a workaround.
> diff --git a/sysdeps/unix/sysv/linux/mips/mips32/libc-do-syscall.S b/sysdeps/unix/sysv/linux/mips/mips32/libc-do-syscall.S
> new file mode 100644
> index 0000000000..c02f507008
> --- /dev/null
> +++ b/sysdeps/unix/sysv/linux/mips/mips32/libc-do-syscall.S
> @@ -0,0 +1,52 @@
> +/* Copyright (C) 2017 Free Software Foundation, Inc.
> + This file is part of the GNU C Library.
> +
> + The GNU C Library is free software; you can redistribute it and/or
> + modify it under the terms of the GNU Lesser General Public
> + License as published by the Free Software Foundation; either
> + version 2.1 of the License, or (at your option) any later version.
> +
> + The GNU C Library is distributed in the hope that it will be useful,
> + but WITHOUT ANY WARRANTY; without even the implied warranty of
> + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
> + Lesser General Public License for more details.
> +
> + You should have received a copy of the GNU Lesser General Public
> + License along with the GNU C Library. If not, see
> + <http://www.gnu.org/licenses/>. */
> +
> +#include <sys/asm.h>
> +#include <sysdep.h>
> +#include <asm/unistd.h>
> +#include <sgidefs.h>
> +
> +
> +/* long int __libc_do_syscall (long int, ...) */
> +
> +#define FRAMESZ 32
> +
> + .text
> + .set nomips16
> + .hidden __libc_do_syscall
> +ENTRY(__libc_do_syscall)
> + move v0, a0
> + move a0, a1
> + move a1, a2
> + move a2, a3
Please rearrange this as we previously discussed, with your idea to have
the syscall number as the 4th argument. This will save cycles at no cost.
> + lw a3, 16(sp)
> + lw t0, 20(sp)
> + lw t1, 24(sp)
> + lw t2, 28(sp)
> + .set noreorder
Why `.set noreorder'?
> + PTR_SUBU sp, FRAMESZ
> + cfi_adjust_cfa_offset (FRAMESZ)
> + sw t0, 16(sp)
> + sw t1, 20(sp)
> + sw t2, 24(sp)
With the stub written solely in assembly only I wonder if we actually
need to mess with $sp in the first place. I think we can reuse the stack
argument save area and shuffle the incoming arguments in place. In C
language terms this would be equivalent to reassigning their values in the
callee, which is allowed by the language and IIUC does not require copying
the arguments out (so e.g. -O0 code would do just that), so the compiler
cannot assume the argument save area remains unclobbered after a function
return and use the returning values for anything.
Perhaps we could have separate `__libc_do_syscall5', `__libc_do_syscall6'
and `__libc_do_syscall7' stubs even, really minimal, with the only code
required being to load $v0 from the last argument, i.e.:
ENTRY(__libc_do_syscall5)
lw v0, 16(sp)
syscall
move v1, a3
jr ra
END(__libc_do_syscall5)
(and then $sp offsets of 20 and 24 for the other two)? I'd withdraw any
concerns about code complication I might have had so far then. :)
> + syscall
> + PTR_ADDU sp, FRAMESZ
> + cfi_adjust_cfa_offset (-FRAMESZ)
> + .set reorder
> + move v1, a3
> +1: ret
Are you sure it builds? There's no MIPS RET instruction; you meant `jr
ra' presumably. Also what is the `1' label for? It'll prevent MOVE from
being reordered by GAS into the JR's delay slot (in microMIPS assembly
it'll get relaxed to JRC in that case).
NB please use a tab rather than spaces to indent this instruction like
the rest.
> diff --git a/sysdeps/unix/sysv/linux/mips/mips32/sysdep.h b/sysdeps/unix/sysv/linux/mips/mips32/sysdep.h
> index e9e3ee7e82..8e55538a5c 100644
> --- a/sysdeps/unix/sysv/linux/mips/mips32/sysdep.h
> +++ b/sysdeps/unix/sysv/linux/mips/mips32/sysdep.h
> @@ -49,9 +49,9 @@
> /* Define a macro which expands into the inline wrapper code for a system
> call. */
> #undef INLINE_SYSCALL
> -#define INLINE_SYSCALL(name, nr, args...) \
> +#define INLINE_SYSCALL(name, nr, ...) \
> ({ INTERNAL_SYSCALL_DECL (_sc_err); \
> - long result_var = INTERNAL_SYSCALL (name, _sc_err, nr, args); \
> + long result_var = INTERNAL_SYSCALL (name, _sc_err, nr, ## __VA_ARGS__); \
What's this change for (and likewise throughout)?
> @@ -136,6 +152,7 @@
>
> #endif /* !__mips16 */
>
> +
Extraneous new line?
> @@ -262,110 +279,34 @@
> _sys_result; \
> })
>
> -/* We need to use a frame pointer for the functions in which we
> - adjust $sp around the syscall, or debug information and unwind
> - information will be $sp relative and thus wrong during the syscall. As
> - of GCC 4.7, this is sufficient. */
> -#define FORCE_FRAME_POINTER \
> - void *volatile __fp_force __attribute__ ((unused)) = alloca (4)
> -
> #define internal_syscall5(v0_init, input, number, err, \
> arg1, arg2, arg3, arg4, arg5) \
> ({ \
> - long _sys_result; \
> - \
> - FORCE_FRAME_POINTER; \
> - { \
> - register long __s0 asm ("$16") __attribute__ ((unused)) \
> - = (number); \
> - register long __v0 asm ("$2"); \
> - register long __a0 asm ("$4") = (long) (arg1); \
> - register long __a1 asm ("$5") = (long) (arg2); \
> - register long __a2 asm ("$6") = (long) (arg3); \
> - register long __a3 asm ("$7") = (long) (arg4); \
> - __asm__ volatile ( \
> - ".set\tnoreorder\n\t" \
> - "subu\t$29, 32\n\t" \
> - "sw\t%6, 16($29)\n\t" \
> - v0_init \
> - "syscall\n\t" \
> - "addiu\t$29, 32\n\t" \
> - ".set\treorder" \
> - : "=r" (__v0), "+r" (__a3) \
> - : input, "r" (__a0), "r" (__a1), "r" (__a2), \
> - "r" ((long) (arg5)) \
> - : __SYSCALL_CLOBBERS); \
> - err = __a3; \
> - _sys_result = __v0; \
> - } \
> - _sys_result; \
> + union __libc_do_syscall_return _sys_result; \
Indentation. Same with the repeated line throughout.
Otherwise it looks reasonable to me.
Maciej
On 2017-08-17 23:34, Maciej W. Rozycki wrote:
> On Thu, 17 Aug 2017, Aurelien Jarno wrote:
>
> > I see no regression on mipsel o32. I have only lightly tested mips o32
> > (the testsuite is still running). I haven't been able to fully compile
> > mips16 due to the following error compiling dl-tunables.c:
> >
> > /tmp/ccI2NMgJ.s: Assembler messages:
> > /tmp/ccI2NMgJ.s:1376: Error: branch to a symbol in another ISA mode
> >
> > It doesn't seem to be related to my changes.
>
> If it's a regression, then it probably is. What compiler version? The
> fix for missing trailing label annotation went in r242424, for GCC 7. If
> it's in handcoded assembly OTOH, then the offending code has to be fixed.
I am using GCC 6, so if the fix went in GCC 7, that's normal the issue
is present.
> > diff --git a/sysdeps/unix/sysv/linux/mips/mips32/libc-do-syscall.S b/sysdeps/unix/sysv/linux/mips/mips32/libc-do-syscall.S
> > new file mode 100644
> > index 0000000000..c02f507008
> > --- /dev/null
> > +++ b/sysdeps/unix/sysv/linux/mips/mips32/libc-do-syscall.S
> > @@ -0,0 +1,52 @@
> > +/* Copyright (C) 2017 Free Software Foundation, Inc.
> > + This file is part of the GNU C Library.
> > +
> > + The GNU C Library is free software; you can redistribute it and/or
> > + modify it under the terms of the GNU Lesser General Public
> > + License as published by the Free Software Foundation; either
> > + version 2.1 of the License, or (at your option) any later version.
> > +
> > + The GNU C Library is distributed in the hope that it will be useful,
> > + but WITHOUT ANY WARRANTY; without even the implied warranty of
> > + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
> > + Lesser General Public License for more details.
> > +
> > + You should have received a copy of the GNU Lesser General Public
> > + License along with the GNU C Library. If not, see
> > + <http://www.gnu.org/licenses/>. */
> > +
> > +#include <sys/asm.h>
> > +#include <sysdep.h>
> > +#include <asm/unistd.h>
> > +#include <sgidefs.h>
> > +
> > +
> > +/* long int __libc_do_syscall (long int, ...) */
> > +
> > +#define FRAMESZ 32
> > +
> > + .text
> > + .set nomips16
> > + .hidden __libc_do_syscall
> > +ENTRY(__libc_do_syscall)
> > + move v0, a0
> > + move a0, a1
> > + move a1, a2
> > + move a2, a3
>
> Please rearrange this as we previously discussed, with your idea to have
> the syscall number as the 4th argument. This will save cycles at no cost.
>
> > + lw a3, 16(sp)
> > + lw t0, 20(sp)
> > + lw t1, 24(sp)
> > + lw t2, 28(sp)
> > + .set noreorder
>
> Why `.set noreorder'?
It comes from Adhemerval's patch, and I guess it comes from the asm code
which uses noreorder/reorder around the syscall.
> > + PTR_SUBU sp, FRAMESZ
> > + cfi_adjust_cfa_offset (FRAMESZ)
> > + sw t0, 16(sp)
> > + sw t1, 20(sp)
> > + sw t2, 24(sp)
>
> With the stub written solely in assembly only I wonder if we actually
> need to mess with $sp in the first place. I think we can reuse the stack
> argument save area and shuffle the incoming arguments in place. In C
> language terms this would be equivalent to reassigning their values in the
> callee, which is allowed by the language and IIUC does not require copying
> the arguments out (so e.g. -O0 code would do just that), so the compiler
> cannot assume the argument save area remains unclobbered after a function
> return and use the returning values for anything.
>
> Perhaps we could have separate `__libc_do_syscall5', `__libc_do_syscall6'
> and `__libc_do_syscall7' stubs even, really minimal, with the only code
> required being to load $v0 from the last argument, i.e.:
>
> ENTRY(__libc_do_syscall5)
> lw v0, 16(sp)
> syscall
> move v1, a3
> jr ra
> END(__libc_do_syscall5)
>
> (and then $sp offsets of 20 and 24 for the other two)? I'd withdraw any
> concerns about code complication I might have had so far then. :)
That's an interesting idea. If we use a different stub depending on the
number of arguments, we can actually pass the syscall number last, which
is probably more readable. Could also be used for mips16 in all cases?
> > + syscall
> > + PTR_ADDU sp, FRAMESZ
> > + cfi_adjust_cfa_offset (-FRAMESZ)
> > + .set reorder
> > + move v1, a3
> > +1: ret
>
> Are you sure it builds? There's no MIPS RET instruction; you meant `jr
> ra' presumably. Also what is the `1' label for? It'll prevent MOVE from
> being reordered by GAS into the JR's delay slot (in microMIPS assembly
> it'll get relaxed to JRC in that case).
>
> NB please use a tab rather than spaces to indent this instruction like
> the rest.
I actually noticed the issue of the delay slot not being used, that's
why I tried alternative way to end the function. Using ret compiles and
is used in various glibc places (e.g. mips64/syscall.S). I'll fix that.
> > diff --git a/sysdeps/unix/sysv/linux/mips/mips32/sysdep.h b/sysdeps/unix/sysv/linux/mips/mips32/sysdep.h
> > index e9e3ee7e82..8e55538a5c 100644
> > --- a/sysdeps/unix/sysv/linux/mips/mips32/sysdep.h
> > +++ b/sysdeps/unix/sysv/linux/mips/mips32/sysdep.h
> > @@ -49,9 +49,9 @@
> > /* Define a macro which expands into the inline wrapper code for a system
> > call. */
> > #undef INLINE_SYSCALL
> > -#define INLINE_SYSCALL(name, nr, args...) \
> > +#define INLINE_SYSCALL(name, nr, ...) \
> > ({ INTERNAL_SYSCALL_DECL (_sc_err); \
> > - long result_var = INTERNAL_SYSCALL (name, _sc_err, nr, args); \
> > + long result_var = INTERNAL_SYSCALL (name, _sc_err, nr, ## __VA_ARGS__); \
>
> What's this change for (and likewise throughout)?
This change is need to be able to call __libc_do_syscall in the mips16
code. Without it, syscalls without arguments end up with an empty value
after the comma. Note that the generic sysdep.h has already been changed
to use C99 variadic macros.
Now if we use a different stub depending on the number of arguments, we
can probably get rid of that.
> > @@ -136,6 +152,7 @@
> >
> > #endif /* !__mips16 */
> >
> > +
>
> Extraneous new line?
Ack.
> > @@ -262,110 +279,34 @@
> > _sys_result; \
> > })
> >
> > -/* We need to use a frame pointer for the functions in which we
> > - adjust $sp around the syscall, or debug information and unwind
> > - information will be $sp relative and thus wrong during the syscall. As
> > - of GCC 4.7, this is sufficient. */
> > -#define FORCE_FRAME_POINTER \
> > - void *volatile __fp_force __attribute__ ((unused)) = alloca (4)
> > -
> > #define internal_syscall5(v0_init, input, number, err, \
> > arg1, arg2, arg3, arg4, arg5) \
> > ({ \
> > - long _sys_result; \
> > - \
> > - FORCE_FRAME_POINTER; \
> > - { \
> > - register long __s0 asm ("$16") __attribute__ ((unused)) \
> > - = (number); \
> > - register long __v0 asm ("$2"); \
> > - register long __a0 asm ("$4") = (long) (arg1); \
> > - register long __a1 asm ("$5") = (long) (arg2); \
> > - register long __a2 asm ("$6") = (long) (arg3); \
> > - register long __a3 asm ("$7") = (long) (arg4); \
> > - __asm__ volatile ( \
> > - ".set\tnoreorder\n\t" \
> > - "subu\t$29, 32\n\t" \
> > - "sw\t%6, 16($29)\n\t" \
> > - v0_init \
> > - "syscall\n\t" \
> > - "addiu\t$29, 32\n\t" \
> > - ".set\treorder" \
> > - : "=r" (__v0), "+r" (__a3) \
> > - : input, "r" (__a0), "r" (__a1), "r" (__a2), \
> > - "r" ((long) (arg5)) \
> > - : __SYSCALL_CLOBBERS); \
> > - err = __a3; \
> > - _sys_result = __v0; \
> > - } \
> > - _sys_result; \
> > + union __libc_do_syscall_return _sys_result; \
>
> Indentation. Same with the repeated line throughout.
Ack.
Note that in the meantime the testsuite finished on mips o32, and there
is no regression.
Aurelien
On Fri, 18 Aug 2017, Aurelien Jarno wrote:
> > If it's a regression, then it probably is. What compiler version? The
> > fix for missing trailing label annotation went in r242424, for GCC 7. If
> > it's in handcoded assembly OTOH, then the offending code has to be fixed.
>
> I am using GCC 6, so if the fix went in GCC 7, that's normal the issue
> is present.
OK then; you can use the workaround I suggested to verify MIPS16
compilation then.
> > > + lw a3, 16(sp)
> > > + lw t0, 20(sp)
> > > + lw t1, 24(sp)
> > > + lw t2, 28(sp)
> > > + .set noreorder
> >
> > Why `.set noreorder'?
>
> It comes from Adhemerval's patch, and I guess it comes from the asm code
> which uses noreorder/reorder around the syscall.
Indeed, though there doesn't seem to be a good reason to be there in the
first place. Overall our MIPS port uses `.set noreorder' in several
places where there is no justification for that, however I never got to
cleaning this up. It should only ever be used for manual delay-slot
filling in cases like where there is a data anti-dependency between the
two instructions involved. It is clearly not the case here.
> > Perhaps we could have separate `__libc_do_syscall5', `__libc_do_syscall6'
> > and `__libc_do_syscall7' stubs even, really minimal, with the only code
> > required being to load $v0 from the last argument, i.e.:
> >
> > ENTRY(__libc_do_syscall5)
> > lw v0, 16(sp)
> > syscall
> > move v1, a3
> > jr ra
> > END(__libc_do_syscall5)
> >
> > (and then $sp offsets of 20 and 24 for the other two)? I'd withdraw any
> > concerns about code complication I might have had so far then. :)
>
> That's an interesting idea. If we use a different stub depending on the
> number of arguments, we can actually pass the syscall number last, which
> is probably more readable. Could also be used for mips16 in all cases?
MIPS16 wrappers do that already, which is also why there is an individual
one for each syscall argument count.
> > > + syscall
> > > + PTR_ADDU sp, FRAMESZ
> > > + cfi_adjust_cfa_offset (-FRAMESZ)
> > > + .set reorder
> > > + move v1, a3
> > > +1: ret
> >
> > Are you sure it builds? There's no MIPS RET instruction; you meant `jr
> > ra' presumably. Also what is the `1' label for? It'll prevent MOVE from
> > being reordered by GAS into the JR's delay slot (in microMIPS assembly
> > it'll get relaxed to JRC in that case).
> >
> > NB please use a tab rather than spaces to indent this instruction like
> > the rest.
>
> I actually noticed the issue of the delay slot not being used, that's
> why I tried alternative way to end the function. Using ret compiles and
> is used in various glibc places (e.g. mips64/syscall.S). I'll fix that.
Ah, that comes from sysdeps/unix/mips/sysdep.h where `ret' is defined as:
#define ret j ra ; nop
i.e. with useless delay slot filling wasting an instruction, whether in
`.set noreorder' code or not. There are ret_NOERRNO and ret_ERRVAL
alternatives as well, all the same. This stuff doesn't make sense to me
and while a clean-up belongs to a separate change I don't think we want
to continue using these macros in new code.
> > > diff --git a/sysdeps/unix/sysv/linux/mips/mips32/sysdep.h b/sysdeps/unix/sysv/linux/mips/mips32/sysdep.h
> > > index e9e3ee7e82..8e55538a5c 100644
> > > --- a/sysdeps/unix/sysv/linux/mips/mips32/sysdep.h
> > > +++ b/sysdeps/unix/sysv/linux/mips/mips32/sysdep.h
> > > @@ -49,9 +49,9 @@
> > > /* Define a macro which expands into the inline wrapper code for a system
> > > call. */
> > > #undef INLINE_SYSCALL
> > > -#define INLINE_SYSCALL(name, nr, args...) \
> > > +#define INLINE_SYSCALL(name, nr, ...) \
> > > ({ INTERNAL_SYSCALL_DECL (_sc_err); \
> > > - long result_var = INTERNAL_SYSCALL (name, _sc_err, nr, args); \
> > > + long result_var = INTERNAL_SYSCALL (name, _sc_err, nr, ## __VA_ARGS__); \
> >
> > What's this change for (and likewise throughout)?
>
> This change is need to be able to call __libc_do_syscall in the mips16
> code. Without it, syscalls without arguments end up with an empty value
> after the comma. Note that the generic sysdep.h has already been changed
> to use C99 variadic macros.
OK, this would qualify for a separate preparatory change then.
> Now if we use a different stub depending on the number of arguments, we
> can probably get rid of that.
Ack.
Maciej
@@ -1,8 +1,26 @@
+ifeq ($(subdir),elf)
+sysdep-dl-routines += libc-do-syscall
+endif
+
ifeq ($(subdir),conform)
# For bugs 17786 and 21278.
conformtest-xfail-conds += mips-o32-linux
endif
+ifeq ($(subdir),io)
+sysdep_routines += libc-do-syscall
+endif
+
+ifeq ($(subdir),nptl)
+libpthread-sysdep_routines += libc-do-syscall
+libpthread-shared-only-routines += libc-do-syscall
+endif
+
+ifeq ($(subdir),rt)
+librt-sysdep_routines += libc-do-syscall
+librt-shared-only-routines += libc-do-syscall
+endif
+
ifeq ($(subdir),stdlib)
tests += bug-getcontext-mips-gp
endif
new file mode 100644
@@ -0,0 +1,52 @@
+/* Copyright (C) 2017 Free Software Foundation, Inc.
+ This file is part of the GNU C Library.
+
+ The GNU C Library is free software; you can redistribute it and/or
+ modify it under the terms of the GNU Lesser General Public
+ License as published by the Free Software Foundation; either
+ version 2.1 of the License, or (at your option) any later version.
+
+ The GNU C Library is distributed in the hope that it will be useful,
+ but WITHOUT ANY WARRANTY; without even the implied warranty of
+ MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
+ Lesser General Public License for more details.
+
+ You should have received a copy of the GNU Lesser General Public
+ License along with the GNU C Library. If not, see
+ <http://www.gnu.org/licenses/>. */
+
+#include <sys/asm.h>
+#include <sysdep.h>
+#include <asm/unistd.h>
+#include <sgidefs.h>
+
+
+/* long int __libc_do_syscall (long int, ...) */
+
+#define FRAMESZ 32
+
+ .text
+ .set nomips16
+ .hidden __libc_do_syscall
+ENTRY(__libc_do_syscall)
+ move v0, a0
+ move a0, a1
+ move a1, a2
+ move a2, a3
+ lw a3, 16(sp)
+ lw t0, 20(sp)
+ lw t1, 24(sp)
+ lw t2, 28(sp)
+ .set noreorder
+ PTR_SUBU sp, FRAMESZ
+ cfi_adjust_cfa_offset (FRAMESZ)
+ sw t0, 16(sp)
+ sw t1, 20(sp)
+ sw t2, 24(sp)
+ syscall
+ PTR_ADDU sp, FRAMESZ
+ cfi_adjust_cfa_offset (-FRAMESZ)
+ .set reorder
+ move v1, a3
+1: ret
+END (__libc_do_syscall)
@@ -1,13 +1,9 @@
ifeq ($(subdir),misc)
sysdep_routines += mips16-syscall0 mips16-syscall1 mips16-syscall2
-sysdep_routines += mips16-syscall3 mips16-syscall4 mips16-syscall5
-sysdep_routines += mips16-syscall6 mips16-syscall7
+sysdep_routines += mips16-syscall3 mips16-syscall4
CFLAGS-mips16-syscall0.c += -fexceptions
CFLAGS-mips16-syscall1.c += -fexceptions
CFLAGS-mips16-syscall2.c += -fexceptions
CFLAGS-mips16-syscall3.c += -fexceptions
CFLAGS-mips16-syscall4.c += -fexceptions
-CFLAGS-mips16-syscall5.c += -fexceptions
-CFLAGS-mips16-syscall6.c += -fexceptions
-CFLAGS-mips16-syscall7.c += -fexceptions
endif
@@ -1,6 +1,6 @@
libc {
GLIBC_PRIVATE {
__mips16_syscall0; __mips16_syscall1; __mips16_syscall2; __mips16_syscall3;
- __mips16_syscall4; __mips16_syscall5; __mips16_syscall6; __mips16_syscall7;
+ __mips16_syscall4;
}
}
@@ -21,17 +21,6 @@
#define __nomips16 __attribute__ ((nomips16))
-union __mips16_syscall_return
- {
- long long val;
- struct
- {
- long v0;
- long v1;
- }
- reg;
- };
-
long long __nomips16 __mips16_syscall0 (long number);
#define __mips16_syscall0(dummy, number) \
__mips16_syscall0 ((long) (number))
@@ -24,7 +24,7 @@
long long __nomips16
__mips16_syscall0 (long number)
{
- union __mips16_syscall_return ret;
+ union __libc_do_syscall_return ret;
ret.reg.v0 = INTERNAL_SYSCALL_MIPS16 (number, ret.reg.v1, 0);
return ret.val;
}
@@ -25,7 +25,7 @@ long long __nomips16
__mips16_syscall1 (long a0,
long number)
{
- union __mips16_syscall_return ret;
+ union __libc_do_syscall_return ret;
ret.reg.v0 = INTERNAL_SYSCALL_MIPS16 (number, ret.reg.v1, 1,
a0);
return ret.val;
@@ -25,7 +25,7 @@ long long __nomips16
__mips16_syscall2 (long a0, long a1,
long number)
{
- union __mips16_syscall_return ret;
+ union __libc_do_syscall_return ret;
ret.reg.v0 = INTERNAL_SYSCALL_MIPS16 (number, ret.reg.v1, 2,
a0, a1);
return ret.val;
@@ -25,7 +25,7 @@ long long __nomips16
__mips16_syscall3 (long a0, long a1, long a2,
long number)
{
- union __mips16_syscall_return ret;
+ union __libc_do_syscall_return ret;
ret.reg.v0 = INTERNAL_SYSCALL_MIPS16 (number, ret.reg.v1, 3,
a0, a1, a2);
return ret.val;
@@ -25,7 +25,7 @@ long long __nomips16
__mips16_syscall4 (long a0, long a1, long a2, long a3,
long number)
{
- union __mips16_syscall_return ret;
+ union __libc_do_syscall_return ret;
ret.reg.v0 = INTERNAL_SYSCALL_MIPS16 (number, ret.reg.v1, 4,
a0, a1, a2, a3);
return ret.val;
deleted file mode 100644
@@ -1,33 +0,0 @@
-/* MIPS16 syscall wrappers.
- Copyright (C) 2013-2017 Free Software Foundation, Inc.
- This file is part of the GNU C Library.
-
- The GNU C Library is free software; you can redistribute it and/or
- modify it under the terms of the GNU Lesser General Public
- License as published by the Free Software Foundation; either
- version 2.1 of the License, or (at your option) any later version.
-
- The GNU C Library is distributed in the hope that it will be useful,
- but WITHOUT ANY WARRANTY; without even the implied warranty of
- MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
- Lesser General Public License for more details.
-
- You should have received a copy of the GNU Lesser General Public
- License along with the GNU C Library; if not, see
- <http://www.gnu.org/licenses/>. */
-
-#include <sysdep.h>
-#include <mips16-syscall.h>
-
-#undef __mips16_syscall5
-
-long long __nomips16
-__mips16_syscall5 (long a0, long a1, long a2, long a3,
- long a4,
- long number)
-{
- union __mips16_syscall_return ret;
- ret.reg.v0 = INTERNAL_SYSCALL_MIPS16 (number, ret.reg.v1, 5,
- a0, a1, a2, a3, a4);
- return ret.val;
-}
deleted file mode 100644
@@ -1,33 +0,0 @@
-/* MIPS16 syscall wrappers.
- Copyright (C) 2013-2017 Free Software Foundation, Inc.
- This file is part of the GNU C Library.
-
- The GNU C Library is free software; you can redistribute it and/or
- modify it under the terms of the GNU Lesser General Public
- License as published by the Free Software Foundation; either
- version 2.1 of the License, or (at your option) any later version.
-
- The GNU C Library is distributed in the hope that it will be useful,
- but WITHOUT ANY WARRANTY; without even the implied warranty of
- MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
- Lesser General Public License for more details.
-
- You should have received a copy of the GNU Lesser General Public
- License along with the GNU C Library; if not, see
- <http://www.gnu.org/licenses/>. */
-
-#include <sysdep.h>
-#include <mips16-syscall.h>
-
-#undef __mips16_syscall6
-
-long long __nomips16
-__mips16_syscall6 (long a0, long a1, long a2, long a3,
- long a4, long a5,
- long number)
-{
- union __mips16_syscall_return ret;
- ret.reg.v0 = INTERNAL_SYSCALL_MIPS16 (number, ret.reg.v1, 6,
- a0, a1, a2, a3, a4, a5);
- return ret.val;
-}
deleted file mode 100644
@@ -1,33 +0,0 @@
-/* MIPS16 syscall wrappers.
- Copyright (C) 2013-2017 Free Software Foundation, Inc.
- This file is part of the GNU C Library.
-
- The GNU C Library is free software; you can redistribute it and/or
- modify it under the terms of the GNU Lesser General Public
- License as published by the Free Software Foundation; either
- version 2.1 of the License, or (at your option) any later version.
-
- The GNU C Library is distributed in the hope that it will be useful,
- but WITHOUT ANY WARRANTY; without even the implied warranty of
- MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
- Lesser General Public License for more details.
-
- You should have received a copy of the GNU Lesser General Public
- License along with the GNU C Library; if not, see
- <http://www.gnu.org/licenses/>. */
-
-#include <sysdep.h>
-#include <mips16-syscall.h>
-
-#undef __mips16_syscall7
-
-long long __nomips16
-__mips16_syscall7 (long a0, long a1, long a2, long a3,
- long a4, long a5, long a6,
- long number)
-{
- union __mips16_syscall_return ret;
- ret.reg.v0 = INTERNAL_SYSCALL_MIPS16 (number, ret.reg.v1, 7,
- a0, a1, a2, a3, a4, a5, a6);
- return ret.val;
-}
@@ -49,9 +49,9 @@
/* Define a macro which expands into the inline wrapper code for a system
call. */
#undef INLINE_SYSCALL
-#define INLINE_SYSCALL(name, nr, args...) \
+#define INLINE_SYSCALL(name, nr, ...) \
({ INTERNAL_SYSCALL_DECL (_sc_err); \
- long result_var = INTERNAL_SYSCALL (name, _sc_err, nr, args); \
+ long result_var = INTERNAL_SYSCALL (name, _sc_err, nr, ## __VA_ARGS__); \
if ( INTERNAL_SYSCALL_ERROR_P (result_var, _sc_err) ) \
{ \
__set_errno (INTERNAL_SYSCALL_ERRNO (result_var, _sc_err)); \
@@ -98,6 +98,19 @@
#undef INTERNAL_SYSCALL
#undef INTERNAL_SYSCALL_NCS
+long long __attribute__ ((nomips16)) __libc_do_syscall (long int, ...) attribute_hidden;
+
+union __libc_do_syscall_return
+ {
+ long long val;
+ struct
+ {
+ long v0;
+ long v1;
+ }
+ reg;
+ };
+
#ifdef __mips16
/* There's no MIPS16 syscall instruction, so we go through out-of-line
standard MIPS wrappers. These do use inline snippets below though,
@@ -107,13 +120,16 @@
# include <mips16-syscall.h>
-# define INTERNAL_SYSCALL(name, err, nr, args...) \
- INTERNAL_SYSCALL_NCS (SYS_ify (name), err, nr, args)
+# define INTERNAL_SYSCALL(name, err, nr, ...) \
+ INTERNAL_SYSCALL_NCS (SYS_ify (name), err, nr, ## __VA_ARGS__)
-# define INTERNAL_SYSCALL_NCS(number, err, nr, args...) \
+# define INTERNAL_SYSCALL_NCS(number, err, nr, ...) \
({ \
- union __mips16_syscall_return _sc_ret; \
- _sc_ret.val = __mips16_syscall##nr (args, number); \
+ union __libc_do_syscall_return _sc_ret; \
+ if (nr <= 4) \
+ _sc_ret.val = __mips16_syscall##nr (__VA_ARGS__, number); \
+ else \
+ _sc_ret.val = __libc_do_syscall (number, ## __VA_ARGS__); \
err = _sc_ret.reg.v1; \
_sc_ret.reg.v0; \
})
@@ -121,13 +137,13 @@
# define INTERNAL_SYSCALL_MIPS16(number, err, nr, args...) \
internal_syscall##nr ("lw\t%0, %2\n\t", \
"R" (number), \
- 0, err, args)
+ number, err, args)
#else /* !__mips16 */
# define INTERNAL_SYSCALL(name, err, nr, args...) \
internal_syscall##nr ("li\t%0, %2\t\t\t# " #name "\n\t", \
"IK" (SYS_ify (name)), \
- 0, err, args)
+ SYS_ify(name), err, args)
# define INTERNAL_SYSCALL_NCS(number, err, nr, args...) \
internal_syscall##nr (MOVE32 "\t%0, %2\n\t", \
@@ -136,6 +152,7 @@
#endif /* !__mips16 */
+
#define internal_syscall0(v0_init, input, number, err, dummy...) \
({ \
long _sys_result; \
@@ -262,110 +279,34 @@
_sys_result; \
})
-/* We need to use a frame pointer for the functions in which we
- adjust $sp around the syscall, or debug information and unwind
- information will be $sp relative and thus wrong during the syscall. As
- of GCC 4.7, this is sufficient. */
-#define FORCE_FRAME_POINTER \
- void *volatile __fp_force __attribute__ ((unused)) = alloca (4)
-
#define internal_syscall5(v0_init, input, number, err, \
arg1, arg2, arg3, arg4, arg5) \
({ \
- long _sys_result; \
- \
- FORCE_FRAME_POINTER; \
- { \
- register long __s0 asm ("$16") __attribute__ ((unused)) \
- = (number); \
- register long __v0 asm ("$2"); \
- register long __a0 asm ("$4") = (long) (arg1); \
- register long __a1 asm ("$5") = (long) (arg2); \
- register long __a2 asm ("$6") = (long) (arg3); \
- register long __a3 asm ("$7") = (long) (arg4); \
- __asm__ volatile ( \
- ".set\tnoreorder\n\t" \
- "subu\t$29, 32\n\t" \
- "sw\t%6, 16($29)\n\t" \
- v0_init \
- "syscall\n\t" \
- "addiu\t$29, 32\n\t" \
- ".set\treorder" \
- : "=r" (__v0), "+r" (__a3) \
- : input, "r" (__a0), "r" (__a1), "r" (__a2), \
- "r" ((long) (arg5)) \
- : __SYSCALL_CLOBBERS); \
- err = __a3; \
- _sys_result = __v0; \
- } \
- _sys_result; \
+ union __libc_do_syscall_return _sys_result; \
+ _sys_result.val = __libc_do_syscall (number, arg1, arg2, arg3, \
+ arg4, arg5); \
+ err = _sys_result.reg.v1; \
+ _sys_result.reg.v0; \
})
#define internal_syscall6(v0_init, input, number, err, \
arg1, arg2, arg3, arg4, arg5, arg6) \
({ \
- long _sys_result; \
- \
- FORCE_FRAME_POINTER; \
- { \
- register long __s0 asm ("$16") __attribute__ ((unused)) \
- = (number); \
- register long __v0 asm ("$2"); \
- register long __a0 asm ("$4") = (long) (arg1); \
- register long __a1 asm ("$5") = (long) (arg2); \
- register long __a2 asm ("$6") = (long) (arg3); \
- register long __a3 asm ("$7") = (long) (arg4); \
- __asm__ volatile ( \
- ".set\tnoreorder\n\t" \
- "subu\t$29, 32\n\t" \
- "sw\t%6, 16($29)\n\t" \
- "sw\t%7, 20($29)\n\t" \
- v0_init \
- "syscall\n\t" \
- "addiu\t$29, 32\n\t" \
- ".set\treorder" \
- : "=r" (__v0), "+r" (__a3) \
- : input, "r" (__a0), "r" (__a1), "r" (__a2), \
- "r" ((long) (arg5)), "r" ((long) (arg6)) \
- : __SYSCALL_CLOBBERS); \
- err = __a3; \
- _sys_result = __v0; \
- } \
- _sys_result; \
+ union __libc_do_syscall_return _sys_result; \
+ _sys_result.val = __libc_do_syscall (number, arg1, arg2, arg3, \
+ arg4, arg5, arg6); \
+ err = _sys_result.reg.v1; \
+ _sys_result.reg.v0; \
})
#define internal_syscall7(v0_init, input, number, err, \
arg1, arg2, arg3, arg4, arg5, arg6, arg7) \
({ \
- long _sys_result; \
- \
- FORCE_FRAME_POINTER; \
- { \
- register long __s0 asm ("$16") __attribute__ ((unused)) \
- = (number); \
- register long __v0 asm ("$2"); \
- register long __a0 asm ("$4") = (long) (arg1); \
- register long __a1 asm ("$5") = (long) (arg2); \
- register long __a2 asm ("$6") = (long) (arg3); \
- register long __a3 asm ("$7") = (long) (arg4); \
- __asm__ volatile ( \
- ".set\tnoreorder\n\t" \
- "subu\t$29, 32\n\t" \
- "sw\t%6, 16($29)\n\t" \
- "sw\t%7, 20($29)\n\t" \
- "sw\t%8, 24($29)\n\t" \
- v0_init \
- "syscall\n\t" \
- "addiu\t$29, 32\n\t" \
- ".set\treorder" \
- : "=r" (__v0), "+r" (__a3) \
- : input, "r" (__a0), "r" (__a1), "r" (__a2), \
- "r" ((long) (arg5)), "r" ((long) (arg6)), "r" ((long) (arg7)) \
- : __SYSCALL_CLOBBERS); \
- err = __a3; \
- _sys_result = __v0; \
- } \
- _sys_result; \
+ union __libc_do_syscall_return _sys_result; \
+ _sys_result.val = __libc_do_syscall (number, arg1, arg2, arg3, \
+ arg4, arg5, arg6, arg7); \
+ err = _sys_result.reg.v1; \
+ _sys_result.reg.v0; \
})
#define __SYSCALL_CLOBBERS "$1", "$3", "$8", "$9", "$10", "$11", "$12", "$13", \