Re: [PATCH 4/4] Consolidate pwrite/pwrite64 implementations

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

On 23-02-2016 15:51, Mike Frysinger wrote:
> On 23 Feb 2016 12:17, Adhemerval Zanella wrote:
>> 	* sysdeps/unix/sysv/linux/arm/pwrite.c: Remove file.
>> 	* sysdeps/unix/sysv/linux/arm/pwrite64.c: Likewise.
>> 	* sysdeps/unix/sysv/linux/generic/wordsize-32/pwrite.c: Likewise.
>> 	* sysdeps/unix/sysv/linux/generic/wordsize-32/pwrite64.c: Likewise.
>> 	* sysdeps/unix/sysv/linux/mips/pwrite.c: Likewise.
>> 	* sysdeps/unix/sysv/linux/mips/pwrite64.c: Likewise.
>> 	* sysdeps/unix/sysv/linux/powerpc/powerpc32/pwrite.c: Likewise.
>> 	* sysdeps/unix/sysv/linux/powerpc/powerpc32/pwrite64.c: Likewise.
>> 	* sysdeps/unix/sysv/linux/sh/pwrite.c: Likewise.
>> 	* sysdeps/unix/sysv/linux/sh/pwrite64.c: Likewise.
> 
> you aren't removing the mips/sh implementations anymore

In fact I fixed in CL, I just didn't update in commit msg. I will fix it.

> 
>> --- a/sysdeps/unix/sysv/linux/mips/pwrite.c
>> +++ b/sysdeps/unix/sysv/linux/mips/pwrite.c
>>
>> +/* The n32 have sizeof(off_t) != sizeof(off64_t) so we can't strong/weak
>> +   alias the pwrite to pwrite64.  We undefine the __ASSUME_WORDSIZE64_ILP32
>> +   so two implementation are built.  */
>> +#if _MIPS_SIM == _ABIN32
>> +# undef __ASSUME_WORDSIZE64_ILP32
>>  #endif
>>
>> +#include <sysdeps/unix/sysv/linux/pwrite.c>
> 
> this style seems fragile to me.  would it not be better to add a
> specific knob for files to set ?

I agree and I am inclined to add another define, set by the ABI sysdep.h,
to indicate either off_t and off64_t is the same size (so pread64 can
alias to pread). So we will have basically 3 scenarios:

1. __WORDSIZE == 32
   pread   with off_t
   pread64 with off64_t

2. __WORDSIZE == 64 and __ASSUME_OFF_DIFF_OFF64
   pread   with off_t
   pread64 with off64_t

3. __WORDSIZE == 64
   pread being and alias to pread64
   pread64

So for mip64-n32 it will define __ASSUME_OFF_DIFF_OFF64 and thus we can
remove the mips specific implementations.  The result patch is as above
and a similar strategy is done for pread:

--

diff --git a/sysdeps/unix/sysv/linux/arm/pread.c b/sysdeps/unix/sysv/linux/arm/pread.c
deleted file mode 100644
index 8c9b878..0000000
--- a/sysdeps/unix/sysv/linux/arm/pread.c
+++ /dev/null
diff --git a/sysdeps/unix/sysv/linux/arm/pread64.c b/sysdeps/unix/sysv/linux/arm/pread64.c
deleted file mode 100644
index 3364b6a..0000000
--- a/sysdeps/unix/sysv/linux/arm/pread64.c
+++ /dev/null
diff --git a/sysdeps/unix/sysv/linux/generic/wordsize-32/pread.c b/sysdeps/unix/sysv/linux/generic/wordsize-32/pread.c
deleted file mode 100644
index 0dff648..0000000
--- a/sysdeps/unix/sysv/linux/generic/wordsize-32/pread.c
+++ /dev/null
diff --git a/sysdeps/unix/sysv/linux/generic/wordsize-32/pread64.c b/sysdeps/unix/sysv/linux/generic/wordsize-32/pread64.c
deleted file mode 100644
index 8931900..0000000
--- a/sysdeps/unix/sysv/linux/generic/wordsize-32/pread64.c
+++ /dev/null
diff --git a/sysdeps/unix/sysv/linux/mips/kernel-features.h b/sysdeps/unix/sysv/linux/mips/kernel-features.h
index 17ab696..7cb32cd 100644
--- a/sysdeps/unix/sysv/linux/mips/kernel-features.h
+++ b/sysdeps/unix/sysv/linux/mips/kernel-features.h
@@ -58,4 +58,5 @@
    pass 64-bits values through syscalls.  */
 #if _MIPS_SIM == _ABIN32
 # define __ASSUME_WORDSIZE64_ILP32	1
+# define __ASSUME_OFF_DIFF_OFF64        1
 #endif
diff --git a/sysdeps/unix/sysv/linux/mips/pread.c b/sysdeps/unix/sysv/linux/mips/pread.c
deleted file mode 100644
index 02755cb..0000000
--- a/sysdeps/unix/sysv/linux/mips/pread.c
+++ /dev/null
diff --git a/sysdeps/unix/sysv/linux/mips/pread64.c b/sysdeps/unix/sysv/linux/mips/pread64.c
deleted file mode 100644
index ed0e91c..0000000
--- a/sysdeps/unix/sysv/linux/mips/pread64.c
+++ /dev/null
diff --git a/sysdeps/unix/sysv/linux/powerpc/powerpc32/pread.c b/sysdeps/unix/sysv/linux/powerpc/powerpc32/pread.c
deleted file mode 100644
index 2d67013..0000000
--- a/sysdeps/unix/sysv/linux/powerpc/powerpc32/pread.c
+++ /dev/null
diff --git a/sysdeps/unix/sysv/linux/powerpc/powerpc32/pread64.c b/sysdeps/unix/sysv/linux/powerpc/powerpc32/pread64.c
deleted file mode 100644
index 712ab72..0000000
--- a/sysdeps/unix/sysv/linux/powerpc/powerpc32/pread64.c
+++ /dev/null
diff --git a/sysdeps/unix/sysv/linux/powerpc/powerpc32/sysdep.h b/sysdeps/unix/sysv/linux/powerpc/powerpc32/sysdep.h
index 390ce24..c12fd7f 100644
--- a/sysdeps/unix/sysv/linux/powerpc/powerpc32/sysdep.h
+++ b/sysdeps/unix/sysv/linux/powerpc/powerpc32/sysdep.h
@@ -27,13 +27,6 @@
    Handle them here so they can be catched by both C and assembler stubs in
    glibc.  */
 
-#ifdef __NR_pread64
-# ifdef __NR_pread
-#  error "__NR_pread and __NR_pread64 both defined???"
-# endif
-# define __NR_pread __NR_pread64
-#endif
-
 #ifdef __NR_pwrite64
 # ifdef __NR_pwrite
 #  error "__NR_pwrite and __NR_pwrite64 both defined???"
diff --git a/sysdeps/unix/sysv/linux/powerpc/powerpc64/sysdep.h b/sysdeps/unix/sysv/linux/powerpc/powerpc64/sysdep.h
index 761f3ea..c39a0f2 100644
--- a/sysdeps/unix/sysv/linux/powerpc/powerpc64/sysdep.h
+++ b/sysdeps/unix/sysv/linux/powerpc/powerpc64/sysdep.h
@@ -34,13 +34,6 @@
    Handle them here so they can be catched by both C and assembler stubs in
    glibc.  */
 
-#ifdef __NR_pread64
-# ifdef __NR_pread
-#  error "__NR_pread and __NR_pread64 both defined???"
-# endif
-# define __NR_pread __NR_pread64
-#endif
-
 #ifdef __NR_pwrite64
 # ifdef __NR_pwrite
 #  error "__NR_pwrite and __NR_pwrite64 both defined???"
diff --git a/sysdeps/unix/sysv/linux/pread.c b/sysdeps/unix/sysv/linux/pread.c
index 4aa3c67..5663092 100644
--- a/sysdeps/unix/sysv/linux/pread.c
+++ b/sysdeps/unix/sysv/linux/pread.c
@@ -16,33 +16,22 @@
    License along with the GNU C Library; if not, see
    <http://www.gnu.org/licenses/>.  */
 
-#include <assert.h>
-#include <errno.h>
-#include <endian.h>
 #include <unistd.h>
-
 #include <sysdep-cancel.h>
-#include <sys/syscall.h>
 
-#ifdef __NR_pread64		/* Newer kernels renamed but it's the same.  */
-# ifdef __NR_pread
-#  error "__NR_pread and __NR_pread64 both defined???"
-# endif
-# define __NR_pread __NR_pread64
-#endif
+#if __WORDSIZE != 64 || defined (__ASSUME_OFF_DIFF_OFF64)
 
+# ifndef __NR_pread
+#  define __NR_pread __NR_pread64
+# endif
 
 ssize_t
 __libc_pread (int fd, void *buf, size_t count, off_t offset)
 {
-  ssize_t result;
-
-  assert (sizeof (offset) == 4);
-  result = SYSCALL_CANCEL (pread, fd, buf, count,
-			   __LONG_LONG_PAIR (offset >> 31, offset));
-
-  return result;
+  return SYSCALL_CANCEL (pread, fd, buf, count,
+			 __ALIGNMENT_ARG SYSCALL_LL (offset));
 }
 
 strong_alias (__libc_pread, __pread)
 weak_alias (__libc_pread, pread)
+#endif
diff --git a/sysdeps/unix/sysv/linux/pread64.c b/sysdeps/unix/sysv/linux/pread64.c
index 7b5019a..c599f76 100644
--- a/sysdeps/unix/sysv/linux/pread64.c
+++ b/sysdeps/unix/sysv/linux/pread64.c
@@ -16,28 +16,25 @@
    License along with the GNU C Library; if not, see
    <http://www.gnu.org/licenses/>.  */
 
-#include <errno.h>
-#include <endian.h>
 #include <unistd.h>
-
 #include <sysdep-cancel.h>
-#include <sys/syscall.h>
 
-#ifdef __NR_pread64		/* Newer kernels renamed but it's the same.  */
-# ifdef __NR_pread
-#  error "__NR_pread and __NR_pread64 both defined???"
-# endif
-# define __NR_pread __NR_pread64
+#ifndef __NR_pread64
+# define __NR_pread64 __NR_pread
 #endif
 
-
 ssize_t
 __libc_pread64 (int fd, void *buf, size_t count, off64_t offset)
 {
-  return SYSCALL_CANCEL (pread, fd, buf, count,
-			 __LONG_LONG_PAIR ((off_t) (offset >> 32),
-					   (off_t) (offset & 0xffffffff)));
+  return SYSCALL_CANCEL (pread64, fd, buf, count,
+			 __ALIGNMENT_ARG SYSCALL_LL64 (offset));
 }
 
 weak_alias (__libc_pread64, __pread64)
 weak_alias (__libc_pread64, pread64)
+
+#if __WORDSIZE == 64 && !defined (__ASSUME_OFF_DIFF_OFF64)
+strong_alias (__libc_pread64, __libc_pread)
+weak_alias (__libc_pread64, __pread)
+weak_alias (__libc_pread64, pread)
+#endif
diff --git a/sysdeps/unix/sysv/linux/sh/pread.c b/sysdeps/unix/sysv/linux/sh/pread.c
index 8afada5..d3f99f3 100644
--- a/sysdeps/unix/sysv/linux/sh/pread.c
+++ b/sysdeps/unix/sysv/linux/sh/pread.c
@@ -16,28 +16,8 @@
    License along with the GNU C Library; if not, see
    <http://www.gnu.org/licenses/>.  */
 
-#include <assert.h>
-#include <errno.h>
-#include <unistd.h>
-#include <endian.h>
-
-#include <sysdep-cancel.h>
-#include <sys/syscall.h>
-
-#ifdef __NR_pread64             /* Newer kernels renamed but it's the same.  */
-# ifdef __NR_pread
-#  error "__NR_pread and __NR_pread64 both defined???"
-# endif
-# define __NR_pread __NR_pread64
-#endif
-
-
-ssize_t
-__libc_pread (int fd, void *buf, size_t count, off_t offset)
-{
-  return SYSCALL_CANCEL (pread, fd, buf, count, 0,
-			 __LONG_LONG_PAIR (offset >> 31, offset));
-}
-
-strong_alias (__libc_pread, __pread)
-weak_alias (__libc_pread, pread)
+/* SH4 ABI does not really require argument alignment for 64-bits, but
+   the kernel interface for pread adds a dummy long argument before the
+   offset.  */
+#define __ALIGNMENT_ARG
+#include <sysdeps/unix/sysv/linux/pread.c>
diff --git a/sysdeps/unix/sysv/linux/sh/pread64.c b/sysdeps/unix/sysv/linux/sh/pread64.c
index cfc751d..b2e8a25 100644
--- a/sysdeps/unix/sysv/linux/sh/pread64.c
+++ b/sysdeps/unix/sysv/linux/sh/pread64.c
@@ -16,28 +16,8 @@
    License along with the GNU C Library; if not, see
    <http://www.gnu.org/licenses/>.  */
 
-#include <errno.h>
-#include <unistd.h>
-#include <endian.h>
-
-#include <sysdep-cancel.h>
-#include <sys/syscall.h>
-
-#ifdef __NR_pread64             /* Newer kernels renamed but it's the same.  */
-# ifdef __NR_pread
-#  error "__NR_pread and __NR_pread64 both defined???"
-# endif
-# define __NR_pread __NR_pread64
-#endif
-
-
-ssize_t
-__libc_pread64 (int fd, void *buf, size_t count, off64_t offset)
-{
-  return SYSCALL_CANCEL (pread, fd, buf, count, 0,
-			 __LONG_LONG_PAIR ((off_t) (offset >> 32),
-					   (off_t) (offset & 0xffffffff)));
-}
-
-weak_alias (__libc_pread64, __pread64)
-weak_alias (__libc_pread64, pread64)
+/* SH4 ABI does not really require argument alignment for 64-bits, but
+   the kernel interface for pread adds a dummy long argument before the
+   offset.  */
+#define __ALIGNMENT_ARG
+#include <sysdeps/unix/sysv/linux/pread64.c>
diff --git a/sysdeps/unix/sysv/linux/wordsize-64/pread64.c b/sysdeps/unix/sysv/linux/wordsize-64/pread64.c
deleted file mode 100644
index b7f298d..0000000
--- a/sysdeps/unix/sysv/linux/wordsize-64/pread64.c
+++ /dev/null
@@ -1 +0,0 @@
-/* Empty since the pread syscall is equivalent.  */
diff --git a/sysdeps/unix/sysv/linux/wordsize-64/syscalls.list b/sysdeps/unix/sysv/linux/wordsize-64/syscalls.list
index 19cc6d9..7d5f6a5 100644
--- a/sysdeps/unix/sysv/linux/wordsize-64/syscalls.list
+++ b/sysdeps/unix/sysv/linux/wordsize-64/syscalls.list
@@ -3,7 +3,6 @@
 # Whee! 64-bit systems naturally implement llseek.
 llseek		EXTRA	lseek		i:iii	__libc_lseek	__lseek lseek __libc_lseek64 __llseek llseek __lseek64 lseek64
 lseek		llseek	-
-pread		-	pread		Ci:ibni	__libc_pread	__libc_pread64 __pread pread __pread64 pread64
 pwrite		-	pwrite		Ci:ibni	__libc_pwrite	__libc_pwrite64 __pwrite pwrite __pwrite64 pwrite64
 fstatfs		-	fstatfs		i:ip	__fstatfs	fstatfs fstatfs64 __fstatfs64
 statfs		-	statfs		i:sp	__statfs	statfs statfs64