Re: Fix MIPS o32 posix_fadvise [committed]

[Adhemerval Zanella <adhemerval dot zanella at linaro dot org>]

On 12/01/2017 12:08, Joseph Myers wrote:
> On Thu, 12 Jan 2017, Adhemerval Zanella wrote:
> 
>> Thanks for fixing it Joseph and I although I agree this fix should be the
>> most straightforward one for 2.25, I would like to get back on it on 2.26.
>> If arm behavior for posix_advise is not an outliner (as I wrongly supposed)
>> I think we can incorporate it on generic code an get rid of both arch
>> specific implementations.
> 
> Note that at the syscall level ARM and MIPS are different; it's just using 
> __posix_fadvise64_l64 that works for both.
> 
> ARM defines only __NR_arm_fadvise64_64.  That syscall has permuted 
> arguments (__ASSUME_FADVISE64_64_6ARG defined in kernel-features.h for 
> ARM).  kernel-features.h then defines __NR_fadvise64_64 to 
> __NR_arm_fadvise64_64 so that generic code can work for ARM.  I suspect 
> the generic C version of posix_fadvise should work for ARM, given those 
> definitions and care about avoiding the alignment argument.  (Why does 
> posix_fadvise64.c do
> 
> #ifdef __ASSUME_FADVISE64_64_6ARG
>   int ret = INTERNAL_SYSCALL_CALL (fadvise64_64, err, fd, advise,
>                                    SYSCALL_LL64 (offset), SYSCALL_LL64 (len));
> #else
> 
> with the alignment argument never used in that case, but posix_fadvise.c 
> do
> 
> #  ifdef __ASSUME_FADVISE64_64_6ARG
>   int ret = INTERNAL_SYSCALL_CALL (fadvise64_64, err, fd, advise,
>                                    __ALIGNMENT_ARG SYSCALL_LL (offset),
>                                    SYSCALL_LL (len));
> #  else
> 
> which does use an alignment argument with the same syscall and argument 
> order?)

Indeed this __ALIGNMENT_ARG on posix_fadvise.c code for __NR_fadvise64_64
with __ASSUME_FADVISE64_64_6ARG seems wrong.  We are not hitting it because
no architecture actually uses this syscall issue option.

On posix_fadvise.c powerpc32 will use the first option (__NR_fadvise64).
Tile 32-bits will use the third, _NR_fadvise64_64 with
__ASSUME_FADVISE64_64_NO_ALIGN, so a 6 argument call with advise as last
argument.

So with fix below, ARM can use the generic code since it will use the
2 option (__NR_fadvise64 with __ASSUME_FADVISE64_64_6ARG):

diff --git a/sysdeps/unix/sysv/linux/posix_fadvise.c b/sysdeps/unix/sysv/linux/posix_fadvise.c
index 15d72d4..f3b5d22 100644
--- a/sysdeps/unix/sysv/linux/posix_fadvise.c
+++ b/sysdeps/unix/sysv/linux/posix_fadvise.c
@@ -46,8 +46,7 @@ posix_fadvise (int fd, off_t offset, off_t len, int advise)
 # else
 #  ifdef __ASSUME_FADVISE64_64_6ARG
   int ret = INTERNAL_SYSCALL_CALL (fadvise64_64, err, fd, advise,
-                                  __ALIGNMENT_ARG SYSCALL_LL (offset),
-                                  SYSCALL_LL (len));
+                                  SYSCALL_LL (offset), SYSCALL_LL (len));
 #  else

 #   ifdef __ASSUME_FADVISE64_64_NO_ALIGN


> 
> MIPS defines only __NR_fadvise64, but it has the fadvise64_64 semantics 
> (and does not permute arguments, so uses a 7-argument syscall for o32).  
> To use generic C code for o32, you'd need to e.g. have a macro that says 
> to prefer fadvise64_64 to fadvise64 (and then define __NR_fadvise64_64 to 
> __NR_fadvise64 if not already defined, as done in posix_fadvise64.c).
> 

The only missing ABI that defines __ASSUME_ALIGNED_REGISTER_PAIRS is mips32
and as you noted we can't use default code as is.  I have a local patch that
add this mips behaviour on posix_fadvise and I will send upstream.