This is the mail archive of the
libc-alpha@sourceware.org
mailing list for the glibc project.
Re: [PATCH v2] Fix LO_HI_LONG definition
On Mon, Jul 11, 2016 at 2:28 PM, Chris Metcalf <cmetcalf@mellanox.com> wrote:
> 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.
At least, the old one is correct for x86.
> 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.
>
Yes, we have garbage in pos_h (%r8):
0000000000000000 <pwritev>:
0: 8b 05 00 00 00 00 mov 0x0(%rip),%eax # 6
<pwritev+0x6> 2: R_X86_64_PC32 __libc_multiple_threads-0x4
6: 49 89 ca mov %rcx,%r10
9: 85 c0 test %eax,%eax
b: 75 3b jne 48 <pwritev+0x48>
d: 49 89 c8 mov %rcx,%r8
10: 48 63 d2 movslq %edx,%rdx
13: 48 63 ff movslq %edi,%rdi
16: 49 c1 e8 20 shr $0x20,%r8
1a: b8 28 01 00 00 mov $0x128,%eax
1f: 0f 05 syscall
Luckily, kernel removes the garbage:
/* Provide a macro to pass the off{64}_t argument on p{readv,writev}{64}. */
#define LO_HI_LONG(val) \
(long) (val), \
(long) (((uint64_t) (val)) >> 32)
kernel has
static inline loff_t pos_from_hilo(unsigned long high, unsigned long low)
{
#define HALF_LONG_BITS (BITS_PER_LONG / 2)
return (((loff_t)high << HALF_LONG_BITS) << HALF_LONG_BITS) | low;
}
SYSCALL_DEFINE5(preadv, unsigned long, fd, const struct iovec __user *, vec,
unsigned long, vlen, unsigned long, pos_l, unsigned long, pos_h)
{
loff_t pos = pos_from_hilo(pos_h, pos_l);
But we don't pass garbage nor zero in pos_h. I am testing this patch:
diff --git a/sysdeps/unix/sysv/linux/x86_64/sysdep.h
b/sysdeps/unix/sysv/linux/x86_
64/sysdep.h
index d023d68..1a671e1 100644
--- a/sysdeps/unix/sysv/linux/x86_64/sysdep.h
+++ b/sysdeps/unix/sysv/linux/x86_64/sysdep.h
@@ -385,4 +385,8 @@
# endif
#endif
+/* How to pass the off{64}_t argument on p{readv,writev}{64}. */
+#undef LO_HI_LONG
+#define LO_HI_LONG(val) (val)
+
#endif /* linux/x86_64/sysdep.h */
--
H.J.