Re: [PATCH] mips/o32: fix internal_syscall5/6/7

[Aurelien Jarno <aurelien at aurel32 dot net>]

On 2017-08-21 11:48, Maciej W. Rozycki wrote:
> On Sat, 19 Aug 2017, Aurelien Jarno wrote:
> > >  The MIPS16 wrappers were split into individual files so that only ones 
> > > that are actually used by `ld.so' are pulled.  I think it would be good if 
> > > we preserved that.  I'll see if I can experiment with keeping the original 
> > > MIPS16 0-3 wrappers.
> > 
> > For what I have seen, ld.so already uses syscalls with 1 to 4 arguments.
> > It doesn't use any syscall without argument though. So it's only 4
> > instructions overhead.
> 
>  Why make things worse where they don't have to be and there's no benefit
> elsewhere that would balance the regression?
> 
>  Here's what I had in mind.  The key was updating the macros appropriately 
> in sysdeps/unix/sysv/linux/mips/mips32/mips16/mips16-syscall.h.  Beyond 
> that I made a number of clean-ups:
> 
> 1. I renamed `__libc_do_syscall?' to `__mips_syscall?', for consistency 
>    with `__mips16_syscall?', and consequently `__libc_do_syscall_return' 
>    to `__mips_syscall_return'.
> 
> 2. I exported `__mips_syscall?' wrappers from `libc.so' rather than making
>    them hidden.  This is also consistent with `__mips16_syscall?' wrappers
>    and reduces code duplication of doubtful benefit -- it could be that 
>    some calls, if internal, could be subject to the JALR->BAL
>    optimisation, however only those that are in range and only in regular 
>    MIPS code, for a minimal execution time saving on some processors only.
>    Exporting these entries makes the maintenance effort much easier 
>    though, as we don't have to track and record their use in the
>    individual subdirectories in Makefile.
> 
> 3. I renamed `_sys_result' to `_sc_ret' where it is declared as `union 
>    __mips_syscall_return', again for clarity and consistency with MIPS16
>    INTERNAL_SYSCALL_NCS.
> 
> 4. I wrapped `number' in parentheses in `internal_syscall5', 
>    `internal_syscall6' and `internal_syscall7', and cast it to `long'.
> 
> 5. I have adjusted some comments.
> 
> Please let me know if you have any questions or concerns about this 
> update.
> 
>  This passes o32 regular MIPS regression testing, however my GCC 8 binary 
> seems to miscompile support_test_main.c in the MIPS16 mode, causing all 
> the relevant test cases to crash with SIGILL (in the `support_set_test_dir 
> (test_dir)' invocation it's `test_dir' that is jumped to rather than 
> `support_set_test_dir'; I'll file a bug separately).  I'll find a way to 
> run MIPS16 testing eventually, however meanwhile I will appreciate if you 
> do it for me.

Thanks, this looks fine for me. I don't have more comments than the ones
already done by Adhemerval. I have tested it on MIPS O32 BE and LE, I
don't see any regression.

I can do a test build for MIPS16 and run basic testing under QEMU, but I
don't have the hardware to do more.

Aurelien

-- 
Aurelien Jarno                          GPG: 4096R/1DDD8C9B
aurelien@aurel32.net                 http://www.aurel32.net