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

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

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.


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

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

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.

> 
>> +/* 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