Use SYSCALL_LL[64] to pass 64-bit value [BZ #20349]
Commit Message
On Mon, Jul 11, 2016 at 2:04 PM, H.J. Lu <hjl.tools@gmail.com> wrote:
> On Mon, Jul 11, 2016 at 1:36 PM, Chris Metcalf <cmetcalf@mellanox.com> wrote:
>> On 7/11/2016 3:26 PM, H.J. Lu wrote:
>>>
>>> SYSCALL_LL/SYSCALL_LL64 should be used to pass 64-bit value to system
>>> calls.
>>
>>
>> What problem are you trying to solve?
>>
>> SYSCALL_LL and LO_HI_LONG are different on big-endian systems. In this
>> case, LO_HI_LONG is correct, since the kernel API is "unsigned long pos_l,
>> unsigned long pos_h". SYSCALL_LL won't do the right thing.
>>
>> __ALIGNMENT_ARG introduces an extra dummy arguments to be inserted before
>> a 64-bit value split into two 32-bit registers, if required by the platform.
>> Since preadv/pwritev explicitly use split arguments to construct a 64-bit
>> loff_t, __ALIGNMENT_ARG will just add a random inappropriate dummy arg
>> to an API that doesn't need one.
>>
>> I reviewed the casting for LO_HI_LONG and it looks OK; I initially was
>> wondering whether losing the "val >> 31, val" semantics from SYSCALL_LL()
>> might have been problematic, but it seems like LO_HI_LONG should generate
>> the same results.
>>
>
> /* 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)
>
> is wrong for __ASSUME_WORDSIZE64_ILP32 platform.
>
I am testing this.
Comments
On Mon, Jul 11, 2016 at 3:06 PM, H.J. Lu <hjl.tools@gmail.com> wrote:
> On Mon, Jul 11, 2016 at 2:04 PM, H.J. Lu <hjl.tools@gmail.com> wrote:
>> On Mon, Jul 11, 2016 at 1:36 PM, Chris Metcalf <cmetcalf@mellanox.com> wrote:
>>> On 7/11/2016 3:26 PM, H.J. Lu wrote:
>>>>
>>>> SYSCALL_LL/SYSCALL_LL64 should be used to pass 64-bit value to system
>>>> calls.
>>>
>>>
>>> What problem are you trying to solve?
>>>
>>> SYSCALL_LL and LO_HI_LONG are different on big-endian systems. In this
>>> case, LO_HI_LONG is correct, since the kernel API is "unsigned long pos_l,
>>> unsigned long pos_h". SYSCALL_LL won't do the right thing.
>>>
>>> __ALIGNMENT_ARG introduces an extra dummy arguments to be inserted before
>>> a 64-bit value split into two 32-bit registers, if required by the platform.
>>> Since preadv/pwritev explicitly use split arguments to construct a 64-bit
>>> loff_t, __ALIGNMENT_ARG will just add a random inappropriate dummy arg
>>> to an API that doesn't need one.
>>>
>>> I reviewed the casting for LO_HI_LONG and it looks OK; I initially was
>>> wondering whether losing the "val >> 31, val" semantics from SYSCALL_LL()
>>> might have been problematic, but it seems like LO_HI_LONG should generate
>>> the same results.
>>>
>>
>> /* 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)
>>
>> is wrong for __ASSUME_WORDSIZE64_ILP32 platform.
>>
>
> I am testing this.
I am checking it in.
On 7/11/2016 6:06 PM, H.J. Lu wrote:
> --- 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)
> +
But any 64-bit platform (or other ILP32 platform with 64-bit kernel API)
that uses the consolidated preadv/pwritev will still have the bug you have
fixed for x86.
I suspect Adhermerval's commit 468700675f7f ("Fix LO_HI_LONG definition")
may not have been the patch he intended, since his commit comment says:
GLIBC already defines a macro for such cases (__OFF_T_MATCHES_OFF64_T),
so this patch uses it instead.
but the actual code does not use __OFF_T_MATCHES_OFF64_T. I suspect
that we actually want what his commit comment implied.
On Wed, Jul 13, 2016 at 9:15 AM, Chris Metcalf <cmetcalf@mellanox.com> wrote:
> On 7/11/2016 6:06 PM, H.J. Lu wrote:
>>
>> --- 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)
>> +
>
>
> But any 64-bit platform (or other ILP32 platform with 64-bit kernel API)
> that uses the consolidated preadv/pwritev will still have the bug you have
> fixed for x86.
>
> I suspect Adhermerval's commit 468700675f7f ("Fix LO_HI_LONG definition")
> may not have been the patch he intended, since his commit comment says:
>
> GLIBC already defines a macro for such cases (__OFF_T_MATCHES_OFF64_T),
> so this patch uses it instead.
>
> but the actual code does not use __OFF_T_MATCHES_OFF64_T. I suspect
> that we actually want what his commit comment implied.
>
__OFF_T_MATCHES_OFF64_T.has nothing to do how 64-bit off_t is passed
to p{readv,writev}{64}. Kernel has
sys_preadv, compat_sys_preadv and compat_sys_preadv64.
64-bit platforms always use sys_preadv and should use:
#define LO_HI_LONG(val) (val)
32-bit platforms which use compat_sys_preadv64, like x32, should also use
#define LO_HI_LONG(val) (val)
From 4124e3305bdf0060d3766486673ad0dd29a3a727 Mon Sep 17 00:00:00 2001
From: "H.J. Lu" <hjl.tools@gmail.com>
Date: Mon, 11 Jul 2016 14:56:33 -0700
Subject: [PATCH] X86-64: Define LO_HI_LONG to skip pos_h [BZ #20349]
Define LO_HI_LONG to skip pos_h since it is ignored by kernel:
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;
}
where size of loff_t == size of long.
[BZ #20349]
* sysdeps/unix/sysv/linux/x86_64/sysdep.h (LO_HI_LONG): New.
---
sysdeps/unix/sysv/linux/x86_64/sysdep.h | 4 ++++
1 file changed, 4 insertions(+)
@@ -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 */
--
2.7.4