diff mbox

Fix LO_HI_LONG definition

Message ID 1467652080-19812-1-git-send-email-adhemerval.zanella@linaro.org
State New
Headers show

Commit Message

Adhemerval Zanella Netto July 4, 2016, 5:08 p.m. UTC
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.

However it fails on MIPS64n32 where syscalls n32 uses the compat
implementation in the kernel meaning the off_t arguments are passed
in two separate registers.

GLIBC already defines a macro for such cases (__OFF_T_MATCHES_OFF64_T),
so this patch uses it instead.

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

[1] 4751bbe2ad4d1bfa05774e29376d553ecfe563b0
---
 ChangeLog                        | 4 ++++
 sysdeps/unix/sysv/linux/sysdep.h | 2 +-
 2 files changed, 5 insertions(+), 1 deletion(-)

Comments

Florian Weimer July 5, 2016, 1:18 p.m. UTC | #1
On 07/04/2016 07:08 PM, Adhemerval Zanella 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.
>
> However it fails on MIPS64n32 where syscalls n32 uses the compat
> implementation in the kernel meaning the off_t arguments are passed
> in two separate registers.

Was this bug caught by the glibc test suite on the affected architecture?

Thanks,
Florian
Adhemerval Zanella Netto July 5, 2016, 1:59 p.m. UTC | #2
On 05/07/2016 10:18, Florian Weimer wrote:
> On 07/04/2016 07:08 PM, Adhemerval Zanella 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.
>>
>> However it fails on MIPS64n32 where syscalls n32 uses the compat
>> implementation in the kernel meaning the off_t arguments are passed
>> in two separate registers.
> 
> Was this bug caught by the glibc test suite on the affected architecture?

Yes, Joseph reported it [1].

[1] https://sourceware.org/ml/libc-alpha/2016-07/msg00051.html
Florian Weimer July 6, 2016, 11:37 a.m. UTC | #3
On 07/05/2016 03:59 PM, Adhemerval Zanella wrote:
>
>
> On 05/07/2016 10:18, Florian Weimer wrote:
>> On 07/04/2016 07:08 PM, Adhemerval Zanella 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.
>>>
>>> However it fails on MIPS64n32 where syscalls n32 uses the compat
>>> implementation in the kernel meaning the off_t arguments are passed
>>> in two separate registers.
>>
>> Was this bug caught by the glibc test suite on the affected architecture?
>
> Yes, Joseph reported it [1].
>
> [1] https://sourceware.org/ml/libc-alpha/2016-07/msg00051.html

Thanks.  I looked at existing uses of __OFF_T_MATCHES_OFF64_T, and it 
seems that it is used as if indicating whether off_t and off64_t are the 
same type.

I'm surprised it's the right conditional here because just because the 
types are the same in user space, the system call convention supports 
64-bit arguments.

Thanks,
Florian
Adhemerval Zanella Netto July 6, 2016, 2:53 p.m. UTC | #4
On 06/07/2016 08:37, Florian Weimer wrote:
> On 07/05/2016 03:59 PM, Adhemerval Zanella wrote:
>>
>>
>> On 05/07/2016 10:18, Florian Weimer wrote:
>>> On 07/04/2016 07:08 PM, Adhemerval Zanella 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.
>>>>
>>>> However it fails on MIPS64n32 where syscalls n32 uses the compat
>>>> implementation in the kernel meaning the off_t arguments are passed
>>>> in two separate registers.
>>>
>>> Was this bug caught by the glibc test suite on the affected architecture?
>>
>> Yes, Joseph reported it [1].
>>
>> [1] https://sourceware.org/ml/libc-alpha/2016-07/msg00051.html
> 
> Thanks.  I looked at existing uses of __OFF_T_MATCHES_OFF64_T, and it seems that it is used as if indicating whether off_t and off64_t are the same type.
> 
> I'm surprised it's the right conditional here because just because the types are the same in user space, the system call convention supports 64-bit arguments.

And with you remark I noted this fix is incorrect. 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.  I will send an updated fix.
For testcase, I am not sure it is acceptable to create a 4GB file to
check on high part of offset (so I am accepting recommendations).
Florian Weimer July 6, 2016, 2:55 p.m. UTC | #5
On 07/06/2016 04:53 PM, Adhemerval Zanella wrote:
> And with you remark I noted this fix is incorrect. 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.

Hah, nice.

> The tests were passing on 64-bit because files were small enough so
> the high part of offset is 0 regardless.  I will send an updated fix.
> For testcase, I am not sure it is acceptable to create a 4GB file to
> check on high part of offset (so I am accepting recommendations).

You can create a sparse file.  I don't think you have to turn it into an 
xcheck if you do that.

Florian
diff mbox

Patch

diff --git a/ChangeLog b/ChangeLog
index 7213a8d..9983d6b 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,5 +1,9 @@ 
 2016-07-04  Adhemerval Zanella  <adhemerval.zanella@linaro.org>
 
+	* sysdeps/unix/sysv/linux/sysdep.h
+	[__WORDSIZE == 64 || __ASSUME_WORDSIZE64_ILP32] (LO_HI_LONG): Use
+	__OFF_T_MATCHES_OFF64_T to define macro.
+
 	* sysdeps/unix/sysv/linux/mips/kernel-features.h
 	(__ASSUME_OFF_DIFF_OFF64): Remove define.
 	* sysdeps/unix/sysv/linux/pread.c
diff --git a/sysdeps/unix/sysv/linux/sysdep.h b/sysdeps/unix/sysv/linux/sysdep.h
index 8c9e62e..5c31249 100644
--- a/sysdeps/unix/sysv/linux/sysdep.h
+++ b/sysdeps/unix/sysv/linux/sysdep.h
@@ -49,7 +49,7 @@ 
 #endif
 
 /* Provide a macro to pass the off{64}_t argument on p{readv,writev}{64}.  */
-#if __WORDSIZE == 64 || defined __ASSUME_WORDSIZE64_ILP32
+#ifdef __OFF_T_MATCHES_OFF64_T
 # define LO_HI_LONG(val) (val)
 #else
 # define LO_HI_LONG(val) \