This is the mail archive of the
libc-alpha@sourceware.org
mailing list for the glibc project.
Re: [RFC PATCH] Consolidate __lseek(), __llseek implementation
- From: Adhemerval Zanella <adhemerval dot zanella at linaro dot org>
- To: Yury Norov <ynorov at caviumnetworks dot com>
- Cc: libc-alpha at sourceware dot org
- Date: Wed, 24 Aug 2016 15:23:12 -0300
- Subject: Re: [RFC PATCH] Consolidate __lseek(), __llseek implementation
- Authentication-results: sourceware.org; auth=none
- References: <1472041476-14664-1-git-send-email-ynorov@caviumnetworks.com>
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
>