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