Re: [RFC PATCH] Consolidate __lseek(), __llseek implementation

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

On 24/08/2016 09:24, Yury Norov wrote:
> Hi Adhemerval,
> 
> Current __lseek() implementation under 
> sysdeps/unix/sysv/linux/generic/wordsize-32/lseek.c
> is broken if off_t is 64-bit type. In this patch it is redirected to 64-bit
> __llseek() version. It helps to avoid introducing platform implementation for
> __lseek() under aarch64/ilp32.
> 
> Notice that using SYSCALL_LL64() makes LTP hang for me (ltp-pan itself),
> so I don't use it.  I am also not sure this patch does not affect other
> ports, and that there's no alternative implementations in glibc that we'd
> cleenup. So I marked it as RFC. Please take a look. If you find it helpful,
> I'll handle SYSCALL_LL64() issue and resend it.
> 

I think the patch is incomplete in the sense that my idea is to consolidate
all the implementation at 'sysdeps/unix/sysv/linux' instead of generic one.
Also, based on kernel lseek/llseek syscall definition:

arch/mips/kernel/linux32.c:99:SYSCALL_DEFINE5(32_llseek, unsigned int, fd, unsigned int, offset_high,
arch/tile/kernel/compat.c:90:COMPAT_SYSCALL_DEFINE5(llseek, unsigned int, fd, unsigned int, offset_high,
arch/s390/kernel/compat_wrapper.c:97:COMPAT_SYSCALL_WRAP5(llseek, unsigned int, fd, unsigned long, high, unsigned long, low, loff_t __user *, result, unsigned int, whence);
fs/read_write.c:305:SYSCALL_DEFINE3(lseek, unsigned int, fd, off_t, offset, unsigned int, whence)
fs/read_write.c:324:COMPAT_SYSCALL_DEFINE3(lseek, unsigned int, fd, compat_off_t, offset, unsigned int, whence)
fs/read_write.c:331:SYSCALL_DEFINE5(llseek, unsigned int, fd, unsigned long, offset_high,

I follows the preadv/pwritev of explicit splitting the off_t argument
instead of relying on ABI.  So SYSCALL_LL{64} won't work on this, we
should use LO_HI_LONG instead.

My initial prototype on how to consolidate lseek/lseek64/llseek would
be something like the below. I haven't tested it yet and it will prop
require some work:

- lseek: ABIs with __OFF_T_MATCHES_OFF64_T != 1 will preferable use 
  __NR__llseek if kernel supports it, otherwise it will use __NR_lseek.

- lseek64: ABIs with __OFF_T_MATCHES_OFF64_T == 1 will use __NR_lseek.

- _llseek: files will be removed and alias to lseek64.

In this way ABI with __OFF_T_MATCHES_OFF64_T == 1 will have a empty
lseek.c and use __NR_lseek for lseek64.c (with correct alias for
_llseek).

One possible optimization that I did not added in snippet below, but
occurred to me now is for __OFF_T_MATCHES_OFF64_T != 1 where _NR__llseek
is defined we can just skip lseek.c compilation and alias the lseek64
to all required lseek symbols.  If it is the case where _NR__llseek
is defined on all supported arches for minimum current kernel I think
we can simplify the strategy below.

* sysdeps/unix/sysv/linux/lseek.c

#ifndef __OFF_T_MATCHES_OFF64_T 

off_t
__lseek (int fd, off_t offset, int whence)
{
#ifdef _NR__llseek
  loff_t res;
  int rc = INLINE_SYSCALL_CALL (_llseek, fd, LO_HI_LONG (offset), &res,
				whence);
  return rc ?: lseek_overflow (res);
#else
  return INLINE_SYSCALL_CALL (lseek, fd, offset, whence);
#endif
}
libc_hidden_def (__lseek)
weak_alias (__lseek, lseek)
strong_alias (__lseek, __libc_lseek)

#endif


* lseek64.c

off64_t
__lseek64 (int fd, off64_t offset, int whence)
{
#if __OFF_T_MATCHES_OFF64_T
  return INLINE_SYSCALL_CALL (lseek, fd, offset, whence);
#else
  loff_t res;
  int rc = INLINE_SYSCALL_CALL (_llseek, fd, LO_HI_LONG (offset), &res,
				whence);
  return rc ?: lseek_overflow (res);
#endif
}

#ifndef  __OFF_T_MATCHES_OFF64_T
weak_alias (__lseek64, lseek)
weak_alias (__lseek64, __lseek)
strong_alias (__lseek64, __libc_lseek)
libc_hidden_def (__lseek)
#endif

strong_alias (__lseek64, __libc_lseek64)
strong_alias (__lseek64, __lseek64)
weak_alias (__lseek64, lseek64)


/* llseek doesn't have a prototype.  Since the second parameter is a
   64bit type, this results in wrong behaviour if no prototype is
   provided.  */
strong_alias (__lseek64, _llseek)
link_warning (llseek, "\
the `llseek' function may be dangerous; use `lseek64' instead.")

llseek.c
/* Remove file.  */

What do you think?


> Yury.
> 
> Signed-off-by: Yury Norov <ynorov@caviumnetworks.com>
> ---
>  sysdeps/unix/sysv/linux/generic/wordsize-32/llseek.c | 7 +++++++
>  sysdeps/unix/sysv/linux/generic/wordsize-32/lseek.c  | 2 ++
>  2 files changed, 9 insertions(+)
> 
> diff --git a/sysdeps/unix/sysv/linux/generic/wordsize-32/llseek.c b/sysdeps/unix/sysv/linux/generic/wordsize-32/llseek.c
> index 458964c..3352ffd 100644
> --- a/sysdeps/unix/sysv/linux/generic/wordsize-32/llseek.c
> +++ b/sysdeps/unix/sysv/linux/generic/wordsize-32/llseek.c
> @@ -39,6 +39,13 @@ strong_alias (__llseek, __libc_lseek64)
>  strong_alias (__llseek, __lseek64)
>  weak_alias (__llseek, lseek64)
>  
> +#ifdef __OFF_T_MATCHES_OFF64_T
> +strong_alias (__llseek, __lseek)
> +libc_hidden_ver (__llseek,__lseek)
> +weak_alias (__llseek, lseek)
> +strong_alias (__llseek, __libc_lseek)
> +#endif
> +
>  /* llseek doesn't have a prototype.  Since the second parameter is a
>     64bit type, this results in wrong behaviour if no prototype is
>     provided.  */
> diff --git a/sysdeps/unix/sysv/linux/generic/wordsize-32/lseek.c b/sysdeps/unix/sysv/linux/generic/wordsize-32/lseek.c
> index dbf0b26..c953e79 100644
> --- a/sysdeps/unix/sysv/linux/generic/wordsize-32/lseek.c
> +++ b/sysdeps/unix/sysv/linux/generic/wordsize-32/lseek.c
> @@ -23,6 +23,7 @@
>  #include <sysdep.h>
>  #include <sys/syscall.h>
>  
> +#ifndef __OFF_T_MATCHES_OFF64_T
>  #include "overflow.h"
>  
>  off_t
> @@ -36,3 +37,4 @@ __lseek (int fd, off_t offset, int whence)
>  libc_hidden_def (__lseek)
>  weak_alias (__lseek, lseek)
>  strong_alias (__lseek, __libc_lseek)
> +#endif
>