This is the mail archive of the
libc-alpha@sourceware.org
mailing list for the glibc project.
Re: [PATCH] mips/o32: fix internal_syscall5/6/7
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
--
Aurelien Jarno GPG: 4096R/1DDD8C9B
aurelien@aurel32.net http://www.aurel32.net