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

[Yury Norov <ynorov at caviumnetworks dot com>]

Hi Adhemerval,

On Thu, Aug 25, 2016 at 08:17:33PM -0300, Adhemerval Zanella wrote:
> This patch consolidates all Linux lseek/lseek64/llseek implementation
> in on on sysdeps/unix/sysv/linux/lseek{64}.c.  It also removes the llseek
> file and instead consolidate the LFS lseek implementation on lseek64.c
> as for other LFS symbols implementations.
> 
> The general idea is:
> 
>   - lseek: ABIs without __OFF_T_MATCHES_OFF64_T  will preferable use
>   __NR__llseek if kernel supports it, otherwise they will use __NR_lseek.
>   ABIs with __OFF_T_MATCHES_OFF64_T won't produce any symbol.
> 
>   - lseek64: ABIs with __OFF_T_MATCHES_OFF64_T will use __NR_lseek since
>   it will use 64-bit arguments without low/high splitting as for __NR__lseek.
>   ABIs without __OFF_T_MATCHES_OFF64_T will use __NR_llseek instead.
> 
>   - llseek: files will be removed and symbols will be aliased ot lseek64.
> 
> ABI without __OFF_T_MATCHES_OFF64_T and without __NR_llseek (basically MIPS64n32
> so far) are covered by building lseek with off_t as expected and lseek64
> using __NR_lseek (as expected for off64_t being passed using 64-bit registers).
> 
> For this consolidation I mantained the x32 assembly specific implementation
> because to correctly fix this it would required both the x32 fix for
> {INLINE,INTERNAL}_SYSCALL [1] and a wrapper to correctly subscribe it to
> return 64 bits instead of default 32 bits (as for times).  It could a future
> cleanup.
> 
> It is based on my previous {INTERNAL,INLINE}_SYSCALL_CALL macro [2],
> although it is mainly for simplification.
> 
> Tested on x86_64, i686, aarch64, armhf, and powerpc64le.
> 
> 	* nptl/Makefile (libpthread-routines): Remove ptw-llseek and add
> 	ptw-lseek64.
> 	* sysdeps/unix/sysv/linux/Makefile (sysdeps_routines): Remove llseek.
> 	* sysdeps/unix/sysv/linux/alpha/Makefile  (sysdeps_routines):
> 	Likewise.
> 	* sysdeps/unix/sysv/linux/generic/wordsize-32/llseek.c: Remove file.
> 	* sysdeps/unix/sysv/linux/generic/wordsize-32/lseek.c: Remove file.
> 	* sysdeps/unix/sysv/linux/mips/mips64/llseek.c: Likewise.
> 	* sysdeps/unix/sysv/linux/llseek.c: Remove file.
> 	* sysdeps/unix/sysv/linux/lseek.c: New file.
> 	* sysdeps/unix/sysv/linux/lseek64.c: Add default Linux implementation.
> 	* sysdeps/unix/sysv/linux/mips/mips64/syscalls.list: Remove lseek and
> 	__libc_lseek64 from auto-generation.
> 	* sysdeps/unix/sysv/linux/wordsize-64/syscalls.list: Likewise.
> 	* sysdeps/unix/sysv/linux/x86_64/x32/lseek64.S: New file.
> 
> [1] https://sourceware.org/ml/libc-alpha/2016-08/msg00443.html
> [2] https://sourceware.org/ml/libc-alpha/2016-08/msg00646.html
> ---
>  nptl/Makefile                                      |  2 +-
>  sysdeps/unix/sysv/linux/Makefile                   |  2 +-
>  sysdeps/unix/sysv/linux/alpha/Makefile             |  2 +-
>  .../unix/sysv/linux/generic/wordsize-32/llseek.c   | 46 ------------------
>  .../unix/sysv/linux/generic/wordsize-32/lseek.c    | 38 ---------------
>  sysdeps/unix/sysv/linux/llseek.c                   | 46 ------------------
>  sysdeps/unix/sysv/linux/lseek.c                    | 54 ++++++++++++++++++++++
>  sysdeps/unix/sysv/linux/lseek64.c                  | 52 ++++++++++++++++++++-
>  sysdeps/unix/sysv/linux/mips/mips64/llseek.c       |  1 -
>  sysdeps/unix/sysv/linux/mips/mips64/syscalls.list  |  2 -
>  sysdeps/unix/sysv/linux/wordsize-64/syscalls.list  |  3 --
>  sysdeps/unix/sysv/linux/x86_64/x32/lseek64.S       |  1 +
>  13 files changed, 125 insertions(+), 140 deletions(-)
>  delete mode 100644 sysdeps/unix/sysv/linux/generic/wordsize-32/llseek.c
>  delete mode 100644 sysdeps/unix/sysv/linux/generic/wordsize-32/lseek.c
>  delete mode 100644 sysdeps/unix/sysv/linux/llseek.c
>  create mode 100644 sysdeps/unix/sysv/linux/lseek.c
>  delete mode 100644 sysdeps/unix/sysv/linux/mips/mips64/llseek.c
>  create mode 100644 sysdeps/unix/sysv/linux/x86_64/x32/lseek64.S
> 
> diff --git a/nptl/Makefile b/nptl/Makefile
> index 2ddcd2b..31abb0e 100644
> --- a/nptl/Makefile
> +++ b/nptl/Makefile
> @@ -110,7 +110,7 @@ libpthread-routines = nptl-init vars events version pt-interp \
>  		      pt-fork pt-vfork \
>  		      ptw-write ptw-read ptw-close ptw-fcntl ptw-accept \
>  		      ptw-connect ptw-recv ptw-recvfrom ptw-send \
> -		      ptw-sendto ptw-fsync ptw-lseek ptw-llseek \
> +		      ptw-sendto ptw-fsync ptw-lseek ptw-lseek64 \
>  		      ptw-msync ptw-nanosleep ptw-open ptw-open64 ptw-pause \
>  		      ptw-pread ptw-pread64 ptw-pwrite ptw-pwrite64 \
>  		      ptw-tcdrain ptw-wait ptw-waitpid ptw-msgrcv ptw-msgsnd \
> diff --git a/sysdeps/unix/sysv/linux/Makefile b/sysdeps/unix/sysv/linux/Makefile
> index 4ece07c..0d5db81 100644
> --- a/sysdeps/unix/sysv/linux/Makefile
> +++ b/sysdeps/unix/sysv/linux/Makefile
> @@ -14,7 +14,7 @@ endif
>  ifeq ($(subdir),misc)
>  include $(firstword $(wildcard $(sysdirs:=/sysctl.mk)))
>  
> -sysdep_routines += clone llseek umount umount2 readahead \
> +sysdep_routines += clone umount umount2 readahead \
>  		   setfsuid setfsgid epoll_pwait signalfd \
>  		   eventfd eventfd_read eventfd_write prlimit \
>  		   personality
> 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?

>  # Support old timeval32 entry points
>  sysdep_routines += osf_select osf_gettimeofday osf_settimeofday \
> diff --git a/sysdeps/unix/sysv/linux/generic/wordsize-32/llseek.c b/sysdeps/unix/sysv/linux/generic/wordsize-32/llseek.c
> deleted file mode 100644
> index 458964c..0000000
> --- a/sysdeps/unix/sysv/linux/generic/wordsize-32/llseek.c
> +++ /dev/null
> @@ -1,46 +0,0 @@
> -/* Copyright (C) 2011-2016 Free Software Foundation, Inc.
> -   This file is part of the GNU C Library.
> -   Contributed by Chris Metcalf <cmetcalf@tilera.com>, 2011.
> -
> -   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 <errno.h>
> -#include <sys/types.h>
> -
> -#include <sysdep.h>
> -#include <sys/syscall.h>
> -
> -/* Seek to OFFSET on FD, starting from WHENCE.  */
> -extern loff_t __llseek (int fd, loff_t offset, int whence);
> -
> -loff_t
> -__llseek (int fd, loff_t offset, int whence)
> -{
> -  loff_t retval;
> -
> -  return (loff_t) (INLINE_SYSCALL (_llseek, 5, fd, (off_t) (offset >> 32),
> -				   (off_t) (offset & 0xffffffff),
> -				   &retval, whence) ?: retval);
> -}
> -weak_alias (__llseek, llseek)
> -strong_alias (__llseek, __libc_lseek64)
> -strong_alias (__llseek, __lseek64)
> -weak_alias (__llseek, 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.  */
> -link_warning (llseek, "\
> -the `llseek' function may be dangerous; use `lseek64' instead.")
> diff --git a/sysdeps/unix/sysv/linux/generic/wordsize-32/lseek.c b/sysdeps/unix/sysv/linux/generic/wordsize-32/lseek.c
> deleted file mode 100644
> index dbf0b26..0000000
> --- a/sysdeps/unix/sysv/linux/generic/wordsize-32/lseek.c
> +++ /dev/null
> @@ -1,38 +0,0 @@
> -/* Copyright (C) 2011-2016 Free Software Foundation, Inc.
> -   This file is part of the GNU C Library.
> -   Contributed by Chris Metcalf <cmetcalf@tilera.com>, 2011.
> -
> -   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 <errno.h>
> -#include <unistd.h>
> -#include <sys/types.h>
> -
> -#include <sysdep.h>
> -#include <sys/syscall.h>
> -
> -#include "overflow.h"
> -
> -off_t
> -__lseek (int fd, off_t offset, int whence)
> -{
> -  loff_t res;
> -  int rc = INLINE_SYSCALL (_llseek, 5, fd, (off_t) (offset >> 31),
> -                           (off_t) offset, &res, whence);
> -  return rc ?: lseek_overflow (res);
> -}
> -libc_hidden_def (__lseek)
> -weak_alias (__lseek, lseek)
> -strong_alias (__lseek, __libc_lseek)
> diff --git a/sysdeps/unix/sysv/linux/llseek.c b/sysdeps/unix/sysv/linux/llseek.c
> deleted file mode 100644
> index b6f3ea5..0000000
> --- a/sysdeps/unix/sysv/linux/llseek.c
> +++ /dev/null
> @@ -1,46 +0,0 @@
> -/* Long-long seek operation.
> -   Copyright (C) 1996-2016 Free Software Foundation, Inc.
> -   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 <errno.h>
> -#include <sys/types.h>
> -
> -#include <sysdep.h>
> -#include <sys/syscall.h>
> -
> -/* Seek to OFFSET on FD, starting from WHENCE.  */
> -extern loff_t __llseek (int fd, loff_t offset, int whence);
> -
> -loff_t
> -__llseek (int fd, loff_t offset, int whence)
> -{
> -  loff_t retval;
> -
> -  return (loff_t) (INLINE_SYSCALL (_llseek, 5, fd, (off_t) (offset >> 32),
> -				   (off_t) (offset & 0xffffffff),
> -				   &retval, whence) ?: retval);
> -}
> -weak_alias (__llseek, llseek)
> -strong_alias (__llseek, __libc_lseek64)
> -strong_alias (__llseek, __lseek64)
> -weak_alias (__llseek, 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.  */
> -link_warning (llseek, "\
> -the `llseek' function may be dangerous; use `lseek64' instead.")
> 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"

> +   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()?

> +  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?

> +   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.

> +  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);

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?

> +/* 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 function is what we need.  */
> -- 
> 2.7.4