Re: [PATCH] Consolidate lseek/lseek64/llseek implementations

[Yury Norov <ynorov at caviumnetworks dot com>]

On Mon, Aug 29, 2016 at 04:24:35PM -0300, Adhemerval Zanella wrote:
> tatus: O
> Content-Length: 8675
> Lines: 240
> 
> 
> 
> On 29/08/2016 05:40, Yury Norov wrote:
> 
> >> diff --git a/sysdeps/unix/sysv/linux/alpha/Makefile b/sysdeps/unix/sysv/linux/alpha/Makefile
> >> index 3b523b7..60e294e 100644
> >> --- a/sysdeps/unix/sysv/linux/alpha/Makefile
> >> +++ b/sysdeps/unix/sysv/linux/alpha/Makefile
> >> @@ -10,7 +10,7 @@ ifeq ($(subdir),misc)
> >>  sysdep_headers += alpha/ptrace.h alpha/regdef.h sys/io.h
> >>  
> >>  sysdep_routines += ieee_get_fp_control ieee_set_fp_control \
> >> -		   ioperm llseek
> >> +		   ioperm
> > 
> > You can join lines now, right?
> 
> I think so, although I do not have any preference here.  I will
> wrap the lines.
> 
> >> diff --git a/sysdeps/unix/sysv/linux/lseek.c b/sysdeps/unix/sysv/linux/lseek.c
> >> new file mode 100644
> >> index 0000000..15bee74
> >> --- /dev/null
> >> +++ b/sysdeps/unix/sysv/linux/lseek.c
> >> @@ -0,0 +1,54 @@
> >> +/* Copyright (C) 2016 Free Software Foundation, Inc.
> > 
> > Missed "short one-line description"
> 
> Ack, I will add it.
> 
> > 
> >> +   This file is part of the GNU C Library.
> >> +
> >> +   The GNU C Library is free software; you can redistribute it and/or
> >> +   modify it under the terms of the GNU Lesser General Public
> >> +   License as published by the Free Software Foundation; either
> >> +   version 2.1 of the License, or (at your option) any later version.
> >> +
> >> +   The GNU C Library is distributed in the hope that it will be useful,
> >> +   but WITHOUT ANY WARRANTY; without even the implied warranty of
> >> +   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> >> +   Lesser General Public License for more details.
> >> +
> >> +   You should have received a copy of the GNU Lesser General Public
> >> +   License along with the GNU C Library.  If not, see
> >> +   <http://www.gnu.org/licenses/>.  */
> >> +
> >> +#include <unistd.h>
> >> +#include <sys/types.h>
> >> +#include <sysdep.h>
> >> +#include <errno.h>
> >> +
> >> +#ifndef __OFF_T_MATCHES_OFF64_T
> >> +
> >> +/* Test for overflows of structures where we ask the kernel to fill them
> >> +   in with standard 64-bit syscalls but return them through APIs that
> >> +   only expose the low 32 bits of some fields.  */
> >> +
> >> +static inline off_t lseek_overflow (loff_t res)
> >> +{
> >> +  off_t retval = (off_t) res;
> >> +  if (retval == res)
> >> +    return retval;
> >> +
> >> +  __set_errno (EOVERFLOW);
> >> +  return (off_t) -1;
> >> +}
> >> +
> >> +off_t
> >> +__lseek (int fd, off_t offset, int whence)
> >> +{
> >> +# ifdef __NR__llseek
> >> +  loff_t res;
> >> +  int rc = INLINE_SYSCALL_CALL (_llseek, fd, (off_t)(offset >> 31),
> >> +				(off_t) offset, &res, whence);
> > 
> > Why you not use SYSCALL_LL()?
> 
> Because Linux define __NR__llseek interface as:
> 
> fs/read_write.c:
>  330 #ifdef __ARCH_WANT_SYS_LLSEEK
>  331 SYSCALL_DEFINE5(llseek, unsigned int, fd, unsigned long, offset_high,
>  332                 unsigned long, offset_low, loff_t __user *, result,
>  333                 unsigned int, whence)
> 
> And SYSCALL_LL{64} uses __LONG_LONG_PAIR which will set the (high,low)
> call to (low,high) in little-endian mode (as expected in pread/pwrite
> calls).
> 
> I think we could add another macro, but I do not think it is worth
> adding for this specific case.

oops... That's why it wasn't working in my patch. Sorry for spamming.

> 
> > 
> >> +  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 /* __OFF_T_MATCHES_OFF64_T  */
> >> diff --git a/sysdeps/unix/sysv/linux/lseek64.c b/sysdeps/unix/sysv/linux/lseek64.c
> >> index d81e98f..704d5b3 100644
> >> --- a/sysdeps/unix/sysv/linux/lseek64.c
> >> +++ b/sysdeps/unix/sysv/linux/lseek64.c
> >> @@ -1 +1,51 @@
> >> -/* We don't need a definition since the llseek function is what we need.  */
> >> +/* Copyright (C) 2016 Free Software Foundation, Inc.
> > 
> > Single-line comment?
> > 
> 
> Ack, I will add it.
> 
> >> +   This file is part of the GNU C Library.
> >> +
> >> +   The GNU C Library is free software; you can redistribute it and/or
> >> +   modify it under the terms of the GNU Lesser General Public
> >> +   License as published by the Free Software Foundation; either
> >> +   version 2.1 of the License, or (at your option) any later version.
> >> +
> >> +   The GNU C Library is distributed in the hope that it will be useful,
> >> +   but WITHOUT ANY WARRANTY; without even the implied warranty of
> >> +   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> >> +   Lesser General Public License for more details.
> >> +
> >> +   You should have received a copy of the GNU Lesser General Public
> >> +   License along with the GNU C Library.  If not, see
> >> +   <http://www.gnu.org/licenses/>.  */
> >> +
> >> +#include <unistd.h>
> >> +#include <sys/types.h>
> >> +#include <sysdep.h>
> >> +#include <errno.h>
> >> +
> >> +off64_t
> >> +__lseek64 (int fd, off64_t offset, int whence)
> >> +{
> >> +#if defined(__OFF_T_MATCHES_OFF64_T) || !defined(__NR__llseek)
> > 
> > This will not work for aarch64/ilp32, as __OFF_T_MATCHES_OFF64_T
> > is defined there, but offset is passed in a pair.
> 
> This code make the assumption that:
> 
> 1. __OFF_T_MATCHES_OFF64_T arch will use __NR_lseek that (as for
>    all 64-bit ABI) will pass 64-bit arguments
> 
> 2. if __OFF_T_MATCHES_OFF64_T is not defined then it will use
>    __NR__llseek that splits the off64_t in (high, low)
> 
> So if I understand correctly aarch64/ilp32 uses __NR_lseek with
> __NR__lseek signature? Why wire up __NR_lseek different?

aarch64/ilp32 uses __NR__llseek as all compat arches.  __OFF_T_MATCHES_OFF64_T
means (at least I think so) that off_t is 64-bit, and it doesn't make
assumptions how glibc passes it to kernel. ILP32 passes it as pair because
kernel clears top halves of registers.

I'm not sure how it will affect other ports, but this code will work if you
remove the check against __OFF_T_MATCHES_OFF64_T and just have:

#if !defined(__NR__llseek)


> >> +  return INLINE_SYSCALL_CALL (lseek, fd, offset, whence);
> >> +#else
> >> +  loff_t res;
> >> +  int rc = INLINE_SYSCALL_CALL (_llseek, fd, (off_t) (offset >> 31),
> >> +				(off_t) offset, &res, whence);
> > 

Except this. (off_t) (offset >> 31) doesn't work if off_t is 64-bit.

So, I see 3 options:
 - apply proposed fixes.
 - if not possible, introduce new option (you have to add it for all ports).
 - leave things as is and handle lseek() in arch code for aarch64/ilp32
   (as now).

> > Why are you not using your SYSCALL_LL() macro?
> > 
> >> +  return rc ?: res;
> >> +#endif
> >> +}
> >> +
> >> +#ifdef  __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)
> >> +weak_alias (__lseek64, lseek64)
> > 
> > Can someone give me more detailed explanation about using strong_alias
> > vs weak_alias? What's the rule to choose between them?
> 
> The idea here is to avoid namespace pollution in static linking.  Basically
> strong_alias do not allow symbol override, so for __libc_lseek64 since it
> on a glibc specific namespace (due '__' prefix) we can use a strong alias
> and assume user programs won't be able/require to use the name.  This is
> not true for lseek64, since it is a platform specific name program might
> use.
 
Thanks.

> > 
> >> +/* llseek doesn't have a prototype.  Since the second parameter is a
> >> +   64bit type, this results in wrong behaviour if no prototype is
> >> +   provided.  */
> >> +weak_alias (__lseek64, llseek)
> >> +link_warning (llseek, "\
> >> +the `llseek' function may be dangerous; use `lseek64' instead.")
> >> diff --git a/sysdeps/unix/sysv/linux/mips/mips64/llseek.c b/sysdeps/unix/sysv/linux/mips/mips64/llseek.c
> >> deleted file mode 100644
> >> index 24013a8..0000000
> >> --- a/sysdeps/unix/sysv/linux/mips/mips64/llseek.c
> >> +++ /dev/null
> >> @@ -1 +0,0 @@
> >> -/* lseek() is 64-bit capable already.  */
> >> diff --git a/sysdeps/unix/sysv/linux/mips/mips64/syscalls.list b/sysdeps/unix/sysv/linux/mips/mips64/syscalls.list
> >> index 66cc687..d2d851e 100644
> >> --- a/sysdeps/unix/sysv/linux/mips/mips64/syscalls.list
> >> +++ b/sysdeps/unix/sysv/linux/mips/mips64/syscalls.list
> >> @@ -1,7 +1,5 @@
> >>  # File name	Caller	Syscall name	Args	Strong name	Weak names
> >>  
> >> -lseek		-	lseek		i:iii	__libc_lseek	__lseek lseek __llseek llseek __libc_lseek64 __lseek64 lseek64
> >> -
> >>  ftruncate	-	ftruncate	i:ii	__ftruncate	ftruncate ftruncate64 __ftruncate64
> >>  truncate	-	truncate	i:si	truncate	truncate64
> >>  
> >> diff --git a/sysdeps/unix/sysv/linux/wordsize-64/syscalls.list b/sysdeps/unix/sysv/linux/wordsize-64/syscalls.list
> >> index 2eb9419..3f3569f 100644
> >> --- a/sysdeps/unix/sysv/linux/wordsize-64/syscalls.list
> >> +++ b/sysdeps/unix/sysv/linux/wordsize-64/syscalls.list
> >> @@ -1,8 +1,5 @@
> >>  # File name	Caller	Syscall name	# args	Strong name	Weak names
> >>  
> >> -# Whee! 64-bit systems naturally implement llseek.
> >> -llseek		EXTRA	lseek		i:iii	__libc_lseek	__lseek lseek __libc_lseek64 __llseek llseek __lseek64 lseek64
> >> -lseek		llseek	-
> >>  fstatfs		-	fstatfs		i:ip	__fstatfs	fstatfs fstatfs64 __fstatfs64
> >>  statfs		-	statfs		i:sp	__statfs	statfs statfs64
> >>  mmap		-	mmap		b:aniiii __mmap		mmap __mmap64 mmap64
> >> diff --git a/sysdeps/unix/sysv/linux/x86_64/x32/lseek64.S b/sysdeps/unix/sysv/linux/x86_64/x32/lseek64.S
> >> new file mode 100644
> >> index 0000000..d81e98f
> >> --- /dev/null
> >> +++ b/sysdeps/unix/sysv/linux/x86_64/x32/lseek64.S
> >> @@ -0,0 +1 @@
> >> +/* We don't need a definition since the llseek func