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 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