This is the mail archive of the libc-alpha@sourceware.org mailing list for the glibc project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: [PATCH v2] Fix LO_HI_LONG definition


On 7/11/2016 5:09 PM, H.J. Lu wrote:
On Wed, Jul 6, 2016 at 12:52 PM, Adhemerval Zanella
<adhemerval.zanella@linaro.org> wrote:
The p{read,write}v{64} consolidation patch [1] added a wrong guard
for LO_HI_LONG definition.  It currently uses both
'__WORDSIZE == 64' and 'defined __ASSUME_WORDSIZE64_ILP32' to set
the value to be passed in one argument, otherwise it will be split
in two.  Since kernel commit 601cc11d054 the non-compat
preadv/pwritev in same order, so the LO_HI_LONG is the same regardless
off/off64_t size or wordsize.

The tests were passing on 64-bit because files were small enough so
the high part of offset is 0 regardless.  This patch also changes the
tst-preadvwritev64 test to check reads and writes on a sparse file
larger than 4G.

Checked on x86_64, i686, x32, aarch64, armhf, and s390.

         * sysdeps/unix/sysv/linux/sysdep.h
         [__WORDSIZE == 64 || __ASSUME_WORDSIZE64_ILP32] (LO_HI_LONG): Remove
         guards.
         * misc/tst-preadvwritev-common.c: New file.
         * misc/tst-preadvwritev.c: Use tst-preadvwritev-common.c.
         * misc/tst-preadvwritev64.c: Use tst-preadwritev-common.c and add
         a check for files larger than 2GB.

diff --git a/sysdeps/unix/sysv/linux/sysdep.h b/sysdeps/unix/sysv/linux/sysdep.h
index 8c9e62e..a469f57 100644
--- a/sysdeps/unix/sysv/linux/sysdep.h
+++ b/sysdeps/unix/sysv/linux/sysdep.h
@@ -49,10 +49,6 @@
  #endif

  /* Provide a macro to pass the off{64}_t argument on p{readv,writev}{64}.  */
-#if __WORDSIZE == 64 || defined __ASSUME_WORDSIZE64_ILP32
-# define LO_HI_LONG(val) (val)
-#else
-# define LO_HI_LONG(val) \
-  (long) (val), \
-  (long) (((uint64_t) (val)) >> 32)
-#endif
+#define LO_HI_LONG(val) \
+ (long) (val), \
+ (long) (((uint64_t) (val)) >> 32)
--
This change is incorrect for x32.  __ASSUME_WORDSIZE64_ILP32 seems
to mean different things for mips and x86-64. X86-64 always passes 64-bit
value in a 64-bit register.

Also, I'm not convinced the old code was right either.

In the 64-bit case (or HJ's 32-bit with 64-bit syscall args), shouldn't we
have "#define LO_HI_LONG(val) (val), 0"?  The trailing zero will ensure that
the kernel's pos_h argument gets a zero value so we don't end up
or'ing in garbage with the pos_from_hilo() call.

--
Chris Metcalf, Mellanox Technologies
http://www.mellanox.com


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]