off_t: fix register pair calculation for 64-bit case
diff mbox

Message ID 1466770980-18933-1-git-send-email-ynorov@caviumnetworks.com
State New
Headers show

Commit Message

Yury Norov June 24, 2016, 12:23 p.m. UTC
There are 3 syscall wrappers under sysdeps/unix/sysv/linux that 
calculate register pair for off_t like this:
	__LONG_LONG_PAIR (offset >> 31, offset)

While it works for 32-bit off_t, new 32-bit APIs that use 64-bit
off_t will be broken with it. This patch redirects affected syscalls
to their 64-bit versions. It also saves few instructions and symbols
in glibc, as 32-bit syscall wrappers are not generated anymore.

Tested on AARCH64/ILP32.

Signed-off-by: Yury Norov <ynorov@caviumnetworks.com>
---
 sysdeps/unix/sysv/linux/fallocate.c         | 4 ++++
 sysdeps/unix/sysv/linux/fallocate64.c       | 4 ++++
 sysdeps/unix/sysv/linux/posix_fadvise.c     | 4 ++++
 sysdeps/unix/sysv/linux/posix_fadvise64.c   | 4 ++++
 sysdeps/unix/sysv/linux/posix_fallocate.c   | 4 ++++
 sysdeps/unix/sysv/linux/posix_fallocate64.c | 4 ++++
 6 files changed, 24 insertions(+)

Comments

H.J. Lu June 24, 2016, 12:30 p.m. UTC | #1
On Fri, Jun 24, 2016 at 5:23 AM, Yury Norov <ynorov@caviumnetworks.com> wrote:
> There are 3 syscall wrappers under sysdeps/unix/sysv/linux that
> calculate register pair for off_t like this:
>         __LONG_LONG_PAIR (offset >> 31, offset)
>
> While it works for 32-bit off_t, new 32-bit APIs that use 64-bit
> off_t will be broken with it. This patch redirects affected syscalls
> to their 64-bit versions. It also saves few instructions and symbols
> in glibc, as 32-bit syscall wrappers are not generated anymore.

If you have 64-bit register, should you use wordsize-64, like

sysdeps/unix/sysv/linux/wordsize-64



H.J.
Yury Norov June 24, 2016, 12:41 p.m. UTC | #2
On Fri, Jun 24, 2016 at 05:30:32AM -0700, H.J. Lu wrote:
> On Fri, Jun 24, 2016 at 5:23 AM, Yury Norov <ynorov@caviumnetworks.com> wrote:
> > There are 3 syscall wrappers under sysdeps/unix/sysv/linux that
> > calculate register pair for off_t like this:
> >         __LONG_LONG_PAIR (offset >> 31, offset)
> >
> > While it works for 32-bit off_t, new 32-bit APIs that use 64-bit
> > off_t will be broken with it. This patch redirects affected syscalls
> > to their 64-bit versions. It also saves few instructions and symbols
> > in glibc, as 32-bit syscall wrappers are not generated anymore.
> 
> If you have 64-bit register, should you use wordsize-64, like
> 
> sysdeps/unix/sysv/linux/wordsize-64
> 
> H.J.

Sometimes it's not possible. AARCh64/ILP32 requires to pass 64-bit
parameters as pair. (this is one of two options for ILP32 that is
under discussion)
H.J. Lu June 24, 2016, 12:57 p.m. UTC | #3
On Fri, Jun 24, 2016 at 5:41 AM, Yury Norov <ynorov@caviumnetworks.com> wrote:
> On Fri, Jun 24, 2016 at 05:30:32AM -0700, H.J. Lu wrote:
>> On Fri, Jun 24, 2016 at 5:23 AM, Yury Norov <ynorov@caviumnetworks.com> wrote:
>> > There are 3 syscall wrappers under sysdeps/unix/sysv/linux that
>> > calculate register pair for off_t like this:
>> >         __LONG_LONG_PAIR (offset >> 31, offset)
>> >
>> > While it works for 32-bit off_t, new 32-bit APIs that use 64-bit
>> > off_t will be broken with it. This patch redirects affected syscalls
>> > to their 64-bit versions. It also saves few instructions and symbols
>> > in glibc, as 32-bit syscall wrappers are not generated anymore.
>>
>> If you have 64-bit register, should you use wordsize-64, like
>>
>> sysdeps/unix/sysv/linux/wordsize-64
>>
>> H.J.
>
> Sometimes it's not possible. AARCh64/ILP32 requires to pass 64-bit
> parameters as pair. (this is one of two options for ILP32 that is
> under discussion)

You should still use wordsize-64 and make special exceptions if needed.
Arnd Bergmann June 24, 2016, 1:15 p.m. UTC | #4
On Friday, June 24, 2016 5:57:26 AM CEST H.J. Lu wrote:
> On Fri, Jun 24, 2016 at 5:41 AM, Yury Norov <ynorov@caviumnetworks.com> wrote:
> > On Fri, Jun 24, 2016 at 05:30:32AM -0700, H.J. Lu wrote:
> >> On Fri, Jun 24, 2016 at 5:23 AM, Yury Norov <ynorov@caviumnetworks.com> wrote:
> >> > There are 3 syscall wrappers under sysdeps/unix/sysv/linux that
> >> > calculate register pair for off_t like this:
> >> >         __LONG_LONG_PAIR (offset >> 31, offset)
> >> >
> >> > While it works for 32-bit off_t, new 32-bit APIs that use 64-bit
> >> > off_t will be broken with it. This patch redirects affected syscalls
> >> > to their 64-bit versions. It also saves few instructions and symbols
> >> > in glibc, as 32-bit syscall wrappers are not generated anymore.
> >>
> >> If you have 64-bit register, should you use wordsize-64, like
> >>
> >> sysdeps/unix/sysv/linux/wordsize-64
> >>
> >> H.J.
> >
> > Sometimes it's not possible. AARCh64/ILP32 requires to pass 64-bit
> > parameters as pair. (this is one of two options for ILP32 that is
> > under discussion)
> 
> You should still use wordsize-64 and make special exceptions if needed.

Can the syscall ABI be a single exception then? I think at this
point the syscall interface for aarch64 ILP32 is exactly the same
as for 32-bit RISC-V.

I guess it makes sense to use sysdeps/wordsize-64/ and 
sysdeps/ieee754/dbl-64/wordsize-64/, but the
sysdeps/unix/sysv/linux/wordsize-64/ directory seems to only
contain files for syscalls that differ between 32-bit and
64-bit architectures, so each one of them would otherwise
need a separate override that redirects to the normal 32-bit
syscall.

	Arnd
Yury Norov June 27, 2016, 6:26 a.m. UTC | #5
On Fri, Jun 24, 2016 at 03:15:50PM +0200, Arnd Bergmann wrote:
> On Friday, June 24, 2016 5:57:26 AM CEST H.J. Lu wrote:
> > On Fri, Jun 24, 2016 at 5:41 AM, Yury Norov <ynorov@caviumnetworks.com> wrote:
> > > On Fri, Jun 24, 2016 at 05:30:32AM -0700, H.J. Lu wrote:
> > >> On Fri, Jun 24, 2016 at 5:23 AM, Yury Norov <ynorov@caviumnetworks.com> wrote:
> > >> > There are 3 syscall wrappers under sysdeps/unix/sysv/linux that
> > >> > calculate register pair for off_t like this:
> > >> >         __LONG_LONG_PAIR (offset >> 31, offset)
> > >> >
> > >> > While it works for 32-bit off_t, new 32-bit APIs that use 64-bit
> > >> > off_t will be broken with it. This patch redirects affected syscalls
> > >> > to their 64-bit versions. It also saves few instructions and symbols
> > >> > in glibc, as 32-bit syscall wrappers are not generated anymore.
> > >>
> > >> If you have 64-bit register, should you use wordsize-64, like
> > >>
> > >> sysdeps/unix/sysv/linux/wordsize-64
> > >>
> > >> H.J.
> > >
> > > Sometimes it's not possible. AARCh64/ILP32 requires to pass 64-bit
> > > parameters as pair. (this is one of two options for ILP32 that is
> > > under discussion)
> > 
> > You should still use wordsize-64 and make special exceptions if needed.
> 
> Can the syscall ABI be a single exception then? I think at this
> point the syscall interface for aarch64 ILP32 is exactly the same
> as for 32-bit RISC-V.
> 
> I guess it makes sense to use sysdeps/wordsize-64/ and 
> sysdeps/ieee754/dbl-64/wordsize-64/, but the
> sysdeps/unix/sysv/linux/wordsize-64/ directory seems to only
> contain files for syscalls that differ between 32-bit and
> 64-bit architectures, so each one of them would otherwise
> need a separate override that redirects to the normal 32-bit
> syscall.
> 
> 	Arnd
> 

Hi Arnd, H.J. Lu, others,

I'm not so experienced in the glibc, and it seems I lost your point.

The whole idea of ILP32 patchset is to be a counterpart for kernel code
that clears top halves of registers unconditionally at now. It means we
cannot pass any 64-bit value in a single register, and that's what
the code under sysdeps/unix/sysv/linux/wordsize-64 does. So I don't
understand how we can use it.

Regarding this patch. As far as I understand, each ABI can define size
of it's types with no relation to register size. And modern
32-bit ABIs should have off_t, ino_t etc 64-bit. So we have off_t
64-bit but pass it in a pair. That's what the code under
sysdeps/unix/sysv/linux/ does, except that it does not do it right.

I didn't find the limitation on sysdeps/unix/sysv/linux/ to have off_t
exactly 32-bit, and it means it should work correct for both cases.
And therefore my patch fixes real bug.

In other hand, if sysdeps/unix/sysv/linux/ should work with 32-bit
off_t only, I suggest to describe it explicitly with code like this:

        #ifdef __OFF_T_MATCHES_OFF64_T
        # error  off_t is 32-bit only
        #endif

where needed. But then I'll still have to use sysdeps/unix/sysv/linux/
for ILP32, and will redirect off_t-related syscalls in platform code.

Yury
Arnd Bergmann June 27, 2016, 9:08 a.m. UTC | #6
On Monday, June 27, 2016 9:26:46 AM CEST Yury Norov wrote:
> On Fri, Jun 24, 2016 at 03:15:50PM +0200, Arnd Bergmann wrote:
> > On Friday, June 24, 2016 5:57:26 AM CEST H.J. Lu wrote:
> > > On Fri, Jun 24, 2016 at 5:41 AM, Yury Norov <ynorov@caviumnetworks.com> wrote:
> > > > On Fri, Jun 24, 2016 at 05:30:32AM -0700, H.J. Lu wrote:
> > > >> On Fri, Jun 24, 2016 at 5:23 AM, Yury Norov <ynorov@caviumnetworks.com> wrote:
> > > >> > There are 3 syscall wrappers under sysdeps/unix/sysv/linux that
> > > >> > calculate register pair for off_t like this:
> > > >> >         __LONG_LONG_PAIR (offset >> 31, offset)
> > > >> >
> > > >> > While it works for 32-bit off_t, new 32-bit APIs that use 64-bit
> > > >> > off_t will be broken with it. This patch redirects affected syscalls
> > > >> > to their 64-bit versions. It also saves few instructions and symbols
> > > >> > in glibc, as 32-bit syscall wrappers are not generated anymore.
> > > >>
> > > >> If you have 64-bit register, should you use wordsize-64, like
> > > >>
> > > >> sysdeps/unix/sysv/linux/wordsize-64
> > > >>
> > > >> H.J.
> > > >
> > > > Sometimes it's not possible. AARCh64/ILP32 requires to pass 64-bit
> > > > parameters as pair. (this is one of two options for ILP32 that is
> > > > under discussion)
> > > 
> > > You should still use wordsize-64 and make special exceptions if needed.
> > 
> > Can the syscall ABI be a single exception then? I think at this
> > point the syscall interface for aarch64 ILP32 is exactly the same
> > as for 32-bit RISC-V.
> > 
> > I guess it makes sense to use sysdeps/wordsize-64/ and 
> > sysdeps/ieee754/dbl-64/wordsize-64/, but the
> > sysdeps/unix/sysv/linux/wordsize-64/ directory seems to only
> > contain files for syscalls that differ between 32-bit and
> > 64-bit architectures, so each one of them would otherwise
> > need a separate override that redirects to the normal 32-bit
> > syscall.
> > 
> Hi Arnd, H.J. Lu, others,
> 
> I'm not so experienced in the glibc, and it seems I lost your point.

I'm also not very experienced in glibc, it's possible that I'm the
one who's confused

> The whole idea of ILP32 patchset is to be a counterpart for kernel code
> that clears top halves of registers unconditionally at now. It means we
> cannot pass any 64-bit value in a single register, and that's what
> the code under sysdeps/unix/sysv/linux/wordsize-64 does. So I don't
> understand how we can use it.

The code in sysdeps/unix/sysv/linux/wordsize-64 seems to be made for
the 64-bit syscall API, which is not appropriate here as the kernel
port uses the 32-bit syscall API, now basically unchanged.

This is the same as tile64/ilp32 does, but different from x86-64/ilp32
(x32).

> Regarding this patch. As far as I understand, each ABI can define size
> of it's types with no relation to register size. And modern
> 32-bit ABIs should have off_t, ino_t etc 64-bit. So we have off_t
> 64-bit but pass it in a pair. That's what the code under
> sysdeps/unix/sysv/linux/ does, except that it does not do it right.
>
> I didn't find the limitation on sysdeps/unix/sysv/linux/ to have off_t
> exactly 32-bit, and it means it should work correct for both cases.
> And therefore my patch fixes real bug.
>
> In other hand, if sysdeps/unix/sysv/linux/ should work with 32-bit
> off_t only, I suggest to describe it explicitly with code like this:
> 
>         #ifdef __OFF_T_MATCHES_OFF64_T
>         # error  off_t is 32-bit only
>         #endif
> 
> where needed. But then I'll still have to use sysdeps/unix/sysv/linux/
> for ILP32, and will redirect off_t-related syscalls in platform code.

I agree your patch looks fine, and it fixes the problem for 32-bit
RISC-V as well.

Redirecting off_t based syscalls to architecture specific code sounds
wrong: We should do the same thing for aarch64/ilp32 and 32-bit
risc-v, whatever we end up doing. The alternative that I think
Mike Frysinger was hinting at would be to leave off_t as 32-bit even
on aarch64/ilp32, but then just not use it. If you build your compiler
to always pass _FILE_OFFSET_BITS=64, you can leave this part of
glibc completely untouched and will get the symbols for handling
32-bit off_t, but all applications built against the libc will
use 64-bit off_t anyway. To me, that sounds like a bigger hack
for the whole system, but it makes the glibc platform specific code
a bit simpler because we avoid the special case.

	Arnd
Adhemerval Zanella June 27, 2016, 12:11 p.m. UTC | #7
On 27/06/2016 06:08, Arnd Bergmann wrote:
> On Monday, June 27, 2016 9:26:46 AM CEST Yury Norov wrote:
>> On Fri, Jun 24, 2016 at 03:15:50PM +0200, Arnd Bergmann wrote:
>>> On Friday, June 24, 2016 5:57:26 AM CEST H.J. Lu wrote:
>>>> On Fri, Jun 24, 2016 at 5:41 AM, Yury Norov <ynorov@caviumnetworks.com> wrote:
>>>>> On Fri, Jun 24, 2016 at 05:30:32AM -0700, H.J. Lu wrote:
>>>>>> On Fri, Jun 24, 2016 at 5:23 AM, Yury Norov <ynorov@caviumnetworks.com> wrote:
>>>>>>> There are 3 syscall wrappers under sysdeps/unix/sysv/linux that
>>>>>>> calculate register pair for off_t like this:
>>>>>>>         __LONG_LONG_PAIR (offset >> 31, offset)
>>>>>>>
>>>>>>> While it works for 32-bit off_t, new 32-bit APIs that use 64-bit
>>>>>>> off_t will be broken with it. This patch redirects affected syscalls
>>>>>>> to their 64-bit versions. It also saves few instructions and symbols
>>>>>>> in glibc, as 32-bit syscall wrappers are not generated anymore.
>>>>>>
>>>>>> If you have 64-bit register, should you use wordsize-64, like
>>>>>>
>>>>>> sysdeps/unix/sysv/linux/wordsize-64
>>>>>>
>>>>>> H.J.
>>>>>
>>>>> Sometimes it's not possible. AARCh64/ILP32 requires to pass 64-bit
>>>>> parameters as pair. (this is one of two options for ILP32 that is
>>>>> under discussion)
>>>>
>>>> You should still use wordsize-64 and make special exceptions if needed.
>>>
>>> Can the syscall ABI be a single exception then? I think at this
>>> point the syscall interface for aarch64 ILP32 is exactly the same
>>> as for 32-bit RISC-V.
>>>
>>> I guess it makes sense to use sysdeps/wordsize-64/ and 
>>> sysdeps/ieee754/dbl-64/wordsize-64/, but the
>>> sysdeps/unix/sysv/linux/wordsize-64/ directory seems to only
>>> contain files for syscalls that differ between 32-bit and
>>> 64-bit architectures, so each one of them would otherwise
>>> need a separate override that redirects to the normal 32-bit
>>> syscall.
>>>
>> Hi Arnd, H.J. Lu, others,
>>
>> I'm not so experienced in the glibc, and it seems I lost your point.
> 
> I'm also not very experienced in glibc, it's possible that I'm the
> one who's confused
> 
>> The whole idea of ILP32 patchset is to be a counterpart for kernel code
>> that clears top halves of registers unconditionally at now. It means we
>> cannot pass any 64-bit value in a single register, and that's what
>> the code under sysdeps/unix/sysv/linux/wordsize-64 does. So I don't
>> understand how we can use it.
> 
> The code in sysdeps/unix/sysv/linux/wordsize-64 seems to be made for
> the 64-bit syscall API, which is not appropriate here as the kernel
> port uses the 32-bit syscall API, now basically unchanged.
> 
> This is the same as tile64/ilp32 does, but different from x86-64/ilp32
> (x32).

I intend to send a patch upstream to consolidate all the fallocate
implementation to help this very issue.  The idea is to use the same
pread consolidate idea:

  1. Each architecture/ABI defines if its a ILP32 (__ASSUME_WORDSIZE64_ILP32)
     and if off64_t differs in size of off_t (__ASSUME_OFF_DIFF_OFF64).
     Currently, x32 defines __ASSUME_WORDSIZE64_ILP32 and only MIPS64-n32
     defines both __ASSUME_WORDSIZE64_ILP32 and __ASSUME_OFF_DIFF_OFF64.

  2. For the default function implementation (without the 64 suffix)
     the symbol will be built if is 32-bits (__WORDSIZE==64) or
     if off_t differs in size from off64_t (__ASSUME_OFF_DIFF_OFF64).

     It means that for architecture that only pass 64-bit off_t this
     symbol won't be build.

  3. The 64 variant of the function implementation (with the 64 suffix)
     will be always build and a weak alias for the non-suffix variant
     will be created if __WORDSIZE == 64 and if size of off64_t differs
     from off_t.

     It means that for architecture that only pass 64-bits off_t 
     function will be an alias to function64.

I think with this patch there is no need to more arch-specific implementation.

> 
>> Regarding this patch. As far as I understand, each ABI can define size
>> of it's types with no relation to register size. And modern
>> 32-bit ABIs should have off_t, ino_t etc 64-bit. So we have off_t
>> 64-bit but pass it in a pair. That's what the code under
>> sysdeps/unix/sysv/linux/ does, except that it does not do it right.
>>
>> I didn't find the limitation on sysdeps/unix/sysv/linux/ to have off_t
>> exactly 32-bit, and it means it should work correct for both cases.
>> And therefore my patch fixes real bug.
>>
>> In other hand, if sysdeps/unix/sysv/linux/ should work with 32-bit
>> off_t only, I suggest to describe it explicitly with code like this:
>>
>>         #ifdef __OFF_T_MATCHES_OFF64_T
>>         # error  off_t is 32-bit only
>>         #endif
>>
>> where needed. But then I'll still have to use sysdeps/unix/sysv/linux/
>> for ILP32, and will redirect off_t-related syscalls in platform code.
> 
> I agree your patch looks fine, and it fixes the problem for 32-bit
> RISC-V as well.

I think this patch is incomplete: it should use the SYSCALL_LL{64} macros
for passing off_t{64} arguments which is being used in p{read,write}. 
I will send a consolidation patch today.

> 
> Redirecting off_t based syscalls to architecture specific code sounds
> wrong: We should do the same thing for aarch64/ilp32 and 32-bit
> risc-v, whatever we end up doing. The alternative that I think
> Mike Frysinger was hinting at would be to leave off_t as 32-bit even
> on aarch64/ilp32, but then just not use it. If you build your compiler
> to always pass _FILE_OFFSET_BITS=64, you can leave this part of
> glibc completely untouched and will get the symbols for handling
> 32-bit off_t, but all applications built against the libc will
> use 64-bit off_t anyway. To me, that sounds like a bigger hack
> for the whole system, but it makes the glibc platform specific code
> a bit simpler because we avoid the special case.
> 
> 	Arnd
>
Arnd Bergmann June 27, 2016, 9:01 p.m. UTC | #8
On Monday, June 27, 2016 9:11:51 AM CEST Adhemerval Zanella wrote:
> > 
> >> The whole idea of ILP32 patchset is to be a counterpart for kernel code
> >> that clears top halves of registers unconditionally at now. It means we
> >> cannot pass any 64-bit value in a single register, and that's what
> >> the code under sysdeps/unix/sysv/linux/wordsize-64 does. So I don't
> >> understand how we can use it.
> > 
> > The code in sysdeps/unix/sysv/linux/wordsize-64 seems to be made for
> > the 64-bit syscall API, which is not appropriate here as the kernel
> > port uses the 32-bit syscall API, now basically unchanged.
> > 
> > This is the same as tile64/ilp32 does, but different from x86-64/ilp32
> > (x32).
> 
> I intend to send a patch upstream to consolidate all the fallocate
> implementation to help this very issue.  The idea is to use the same
> pread consolidate idea:
> 
>   1. Each architecture/ABI defines if its a ILP32 (__ASSUME_WORDSIZE64_ILP32)
>      and if off64_t differs in size of off_t (__ASSUME_OFF_DIFF_OFF64).
>      Currently, x32 defines __ASSUME_WORDSIZE64_ILP32 and only MIPS64-n32
>      defines both __ASSUME_WORDSIZE64_ILP32 and __ASSUME_OFF_DIFF_OFF64.
>
>   2. For the default function implementation (without the 64 suffix)
>      the symbol will be built if is 32-bits (__WORDSIZE==64) or
>      if off_t differs in size from off64_t (__ASSUME_OFF_DIFF_OFF64).
> 
>      It means that for architecture that only pass 64-bit off_t this
>      symbol won't be build.
> 
>   3. The 64 variant of the function implementation (with the 64 suffix)
>      will be always build and a weak alias for the non-suffix variant
>      will be created if __WORDSIZE == 64 and if size of off64_t differs
>      from off_t.
> 
>      It means that for architecture that only pass 64-bits off_t 
>      function will be an alias to function64.
> 
> I think with this patch there is no need to more arch-specific implementation.

Doesn't that assume that the kernel interface uses 64-bit registers
to pass off_t?  Yury's patch was specifically for the case where
you use two 32-bit registers (or two lower halves of 64-bit registers
in case of aarch64) but still want 64-bit off_t by default,
i.e. (!defined(__ASSUME_WORDSIZE64_ILP32) && !defined(__ASSUME_OFF_DIFF_OFF64)).

	Arnd
Adhemerval Zanella June 27, 2016, 11 p.m. UTC | #9
On 27/06/2016 18:01, Arnd Bergmann wrote:
> On Monday, June 27, 2016 9:11:51 AM CEST Adhemerval Zanella wrote:
>>>
>>>> The whole idea of ILP32 patchset is to be a counterpart for kernel code
>>>> that clears top halves of registers unconditionally at now. It means we
>>>> cannot pass any 64-bit value in a single register, and that's what
>>>> the code under sysdeps/unix/sysv/linux/wordsize-64 does. So I don't
>>>> understand how we can use it.
>>>
>>> The code in sysdeps/unix/sysv/linux/wordsize-64 seems to be made for
>>> the 64-bit syscall API, which is not appropriate here as the kernel
>>> port uses the 32-bit syscall API, now basically unchanged.
>>>
>>> This is the same as tile64/ilp32 does, but different from x86-64/ilp32
>>> (x32).
>>
>> I intend to send a patch upstream to consolidate all the fallocate
>> implementation to help this very issue.  The idea is to use the same
>> pread consolidate idea:
>>
>>   1. Each architecture/ABI defines if its a ILP32 (__ASSUME_WORDSIZE64_ILP32)
>>      and if off64_t differs in size of off_t (__ASSUME_OFF_DIFF_OFF64).
>>      Currently, x32 defines __ASSUME_WORDSIZE64_ILP32 and only MIPS64-n32
>>      defines both __ASSUME_WORDSIZE64_ILP32 and __ASSUME_OFF_DIFF_OFF64.
>>
>>   2. For the default function implementation (without the 64 suffix)
>>      the symbol will be built if is 32-bits (__WORDSIZE==64) or
>>      if off_t differs in size from off64_t (__ASSUME_OFF_DIFF_OFF64).
>>
>>      It means that for architecture that only pass 64-bit off_t this
>>      symbol won't be build.
>>
>>   3. The 64 variant of the function implementation (with the 64 suffix)
>>      will be always build and a weak alias for the non-suffix variant
>>      will be created if __WORDSIZE == 64 and if size of off64_t differs
>>      from off_t.
>>
>>      It means that for architecture that only pass 64-bits off_t 
>>      function will be an alias to function64.
>>
>> I think with this patch there is no need to more arch-specific implementation.
> 
> Doesn't that assume that the kernel interface uses 64-bit registers
> to pass off_t?  Yury's patch was specifically for the case where
> you use two 32-bit registers (or two lower halves of 64-bit registers
> in case of aarch64) but still want 64-bit off_t by default,
> i.e. (!defined(__ASSUME_WORDSIZE64_ILP32) && !defined(__ASSUME_OFF_DIFF_OFF64)).
> 
> 	Arnd
> 

So if I understood correctly AArch64/ILP32 will another way to handle 
off_t/off64_t, which indeed the original patch make sense.
Yury Norov June 28, 2016, 3:44 a.m. UTC | #10
On Mon, Jun 27, 2016 at 08:00:41PM -0300, Adhemerval Zanella wrote:
> 
> 
> On 27/06/2016 18:01, Arnd Bergmann wrote:
> > On Monday, June 27, 2016 9:11:51 AM CEST Adhemerval Zanella wrote:
> >>>
> >>>> The whole idea of ILP32 patchset is to be a counterpart for kernel code
> >>>> that clears top halves of registers unconditionally at now. It means we
> >>>> cannot pass any 64-bit value in a single register, and that's what
> >>>> the code under sysdeps/unix/sysv/linux/wordsize-64 does. So I don't
> >>>> understand how we can use it.
> >>>
> >>> The code in sysdeps/unix/sysv/linux/wordsize-64 seems to be made for
> >>> the 64-bit syscall API, which is not appropriate here as the kernel
> >>> port uses the 32-bit syscall API, now basically unchanged.
> >>>
> >>> This is the same as tile64/ilp32 does, but different from x86-64/ilp32
> >>> (x32).
> >>
> >> I intend to send a patch upstream to consolidate all the fallocate
> >> implementation to help this very issue.  The idea is to use the same
> >> pread consolidate idea:
> >>
> >>   1. Each architecture/ABI defines if its a ILP32 (__ASSUME_WORDSIZE64_ILP32)
> >>      and if off64_t differs in size of off_t (__ASSUME_OFF_DIFF_OFF64).
> >>      Currently, x32 defines __ASSUME_WORDSIZE64_ILP32 and only MIPS64-n32
> >>      defines both __ASSUME_WORDSIZE64_ILP32 and __ASSUME_OFF_DIFF_OFF64.
> >>
> >>   2. For the default function implementation (without the 64 suffix)
> >>      the symbol will be built if is 32-bits (__WORDSIZE==64) or
> >>      if off_t differs in size from off64_t (__ASSUME_OFF_DIFF_OFF64).
> >>
> >>      It means that for architecture that only pass 64-bit off_t this
> >>      symbol won't be build.
> >>
> >>   3. The 64 variant of the function implementation (with the 64 suffix)
> >>      will be always build and a weak alias for the non-suffix variant
> >>      will be created if __WORDSIZE == 64 and if size of off64_t differs
> >>      from off_t.
> >>
> >>      It means that for architecture that only pass 64-bits off_t 
> >>      function will be an alias to function64.
> >>
> >> I think with this patch there is no need to more arch-specific implementation.
> > 
> > Doesn't that assume that the kernel interface uses 64-bit registers
> > to pass off_t?  Yury's patch was specifically for the case where
> > you use two 32-bit registers (or two lower halves of 64-bit registers
> > in case of aarch64) but still want 64-bit off_t by default,
> > i.e. (!defined(__ASSUME_WORDSIZE64_ILP32) && !defined(__ASSUME_OFF_DIFF_OFF64)).
> > 
> > 	Arnd
> > 
> 
> So if I understood correctly AArch64/ILP32 will another way to handle 
> off_t/off64_t, which indeed the original patch make sense.

Yes. You understood correctly. This is how RISC-V works, and this is
default behavior for all new 32-bit ABIs, both native and ILP32.

Is my understanding correct that for AARCH64/ILP32, in glibc terms
we have solid understanding that __ASSUME_OFF_DIFF_OFF64 is disabled;
and__ASSUME_WORDSIZE64_ILP32 depends on kernel wrappers, and under
discussion.

Could you (someone) explain me the difference between
__ASSUME_OFF_DIFF_OFF64 and __OFF_T_MATCHES_OFF64_T?
At first glance they are mutual exclusive, and so we
can drop one of them.

Yury
Adhemerval Zanella June 28, 2016, 12:21 p.m. UTC | #11
On 28/06/2016 00:44, Yury Norov wrote:
> On Mon, Jun 27, 2016 at 08:00:41PM -0300, Adhemerval Zanella wrote:
>>
>>
>> On 27/06/2016 18:01, Arnd Bergmann wrote:
>>> On Monday, June 27, 2016 9:11:51 AM CEST Adhemerval Zanella wrote:
>>>>>
>>>>>> The whole idea of ILP32 patchset is to be a counterpart for kernel code
>>>>>> that clears top halves of registers unconditionally at now. It means we
>>>>>> cannot pass any 64-bit value in a single register, and that's what
>>>>>> the code under sysdeps/unix/sysv/linux/wordsize-64 does. So I don't
>>>>>> understand how we can use it.
>>>>>
>>>>> The code in sysdeps/unix/sysv/linux/wordsize-64 seems to be made for
>>>>> the 64-bit syscall API, which is not appropriate here as the kernel
>>>>> port uses the 32-bit syscall API, now basically unchanged.
>>>>>
>>>>> This is the same as tile64/ilp32 does, but different from x86-64/ilp32
>>>>> (x32).
>>>>
>>>> I intend to send a patch upstream to consolidate all the fallocate
>>>> implementation to help this very issue.  The idea is to use the same
>>>> pread consolidate idea:
>>>>
>>>>   1. Each architecture/ABI defines if its a ILP32 (__ASSUME_WORDSIZE64_ILP32)
>>>>      and if off64_t differs in size of off_t (__ASSUME_OFF_DIFF_OFF64).
>>>>      Currently, x32 defines __ASSUME_WORDSIZE64_ILP32 and only MIPS64-n32
>>>>      defines both __ASSUME_WORDSIZE64_ILP32 and __ASSUME_OFF_DIFF_OFF64.
>>>>
>>>>   2. For the default function implementation (without the 64 suffix)
>>>>      the symbol will be built if is 32-bits (__WORDSIZE==64) or
>>>>      if off_t differs in size from off64_t (__ASSUME_OFF_DIFF_OFF64).
>>>>
>>>>      It means that for architecture that only pass 64-bit off_t this
>>>>      symbol won't be build.
>>>>
>>>>   3. The 64 variant of the function implementation (with the 64 suffix)
>>>>      will be always build and a weak alias for the non-suffix variant
>>>>      will be created if __WORDSIZE == 64 and if size of off64_t differs
>>>>      from off_t.
>>>>
>>>>      It means that for architecture that only pass 64-bits off_t 
>>>>      function will be an alias to function64.
>>>>
>>>> I think with this patch there is no need to more arch-specific implementation.
>>>
>>> Doesn't that assume that the kernel interface uses 64-bit registers
>>> to pass off_t?  Yury's patch was specifically for the case where
>>> you use two 32-bit registers (or two lower halves of 64-bit registers
>>> in case of aarch64) but still want 64-bit off_t by default,
>>> i.e. (!defined(__ASSUME_WORDSIZE64_ILP32) && !defined(__ASSUME_OFF_DIFF_OFF64)).
>>>
>>> 	Arnd
>>>
>>
>> So if I understood correctly AArch64/ILP32 will another way to handle 
>> off_t/off64_t, which indeed the original patch make sense.
> 
> Yes. You understood correctly. This is how RISC-V works, and this is
> default behavior for all new 32-bit ABIs, both native and ILP32.
> 
> Is my understanding correct that for AARCH64/ILP32, in glibc terms
> we have solid understanding that __ASSUME_OFF_DIFF_OFF64 is disabled;
> and__ASSUME_WORDSIZE64_ILP32 depends on kernel wrappers, and under
> discussion.
> 
> Could you (someone) explain me the difference between
> __ASSUME_OFF_DIFF_OFF64 and __OFF_T_MATCHES_OFF64_T?
> At first glance they are mutual exclusive, and so we
> can drop one of them.

I see they are essentially the same and I added __ASSUME_OFF_DIFF_OFF64
on my pread consolidation by two main reasons:

  1. It is defined only internally on GLIBC
  2. By default on 32 bits it assumes off_t differs from off64_t and
     for 64 bits it is the contrary.  

On current GLIBC supported architectures it requires only MIPS64-n32 to
define it.  However since current kernel new port approach is make
size of off_t the same as off64_t I see that all new ports will require
to define it.

I do not have a strong opinion which is the better approach, the only
nit is I think it should not be on an installed header.

Patch
diff mbox

diff --git a/sysdeps/unix/sysv/linux/fallocate.c b/sysdeps/unix/sysv/linux/fallocate.c
index 6a58a5f..4ec55a5 100644
--- a/sysdeps/unix/sysv/linux/fallocate.c
+++ b/sysdeps/unix/sysv/linux/fallocate.c
@@ -20,6 +20,8 @@ 
 #include <sysdep-cancel.h>
 
 
+#ifndef	__OFF_T_MATCHES_OFF64_T
+
 /* Reserve storage for the data of the file associated with FD.  */
 int
 fallocate (int fd, int mode, __off_t offset, __off_t len)
@@ -33,3 +35,5 @@  fallocate (int fd, int mode, __off_t offset, __off_t len)
   return -1;
 #endif
 }
+
+#endif /* __OFF_T_MATCHES_OFF64_T */
diff --git a/sysdeps/unix/sysv/linux/fallocate64.c b/sysdeps/unix/sysv/linux/fallocate64.c
index 8e76d6f..f4f73d5 100644
--- a/sysdeps/unix/sysv/linux/fallocate64.c
+++ b/sysdeps/unix/sysv/linux/fallocate64.c
@@ -35,3 +35,7 @@  fallocate64 (int fd, int mode, __off64_t offset, __off64_t len)
   return -1;
 #endif
 }
+
+#ifdef __OFF_T_MATCHES_OFF64_T
+weak_alias(fallocate64, fallocate)
+#endif
diff --git a/sysdeps/unix/sysv/linux/posix_fadvise.c b/sysdeps/unix/sysv/linux/posix_fadvise.c
index 093d707..8356bc7 100644
--- a/sysdeps/unix/sysv/linux/posix_fadvise.c
+++ b/sysdeps/unix/sysv/linux/posix_fadvise.c
@@ -19,6 +19,8 @@ 
 #include <fcntl.h>
 #include <sysdep.h>
 
+#ifndef	__OFF_T_MATCHES_OFF64_T
+
 /* Advice the system about the expected behaviour of the application with
    respect to the file associated with FD.  */
 
@@ -46,3 +48,5 @@  posix_fadvise (int fd, off_t offset, off_t len, int advise)
   return ENOSYS;
 #endif
 }
+
+#endif /* __OFF_T_MATCHES_OFF64_T */
diff --git a/sysdeps/unix/sysv/linux/posix_fadvise64.c b/sysdeps/unix/sysv/linux/posix_fadvise64.c
index 6d10558..c76d52f 100644
--- a/sysdeps/unix/sysv/linux/posix_fadvise64.c
+++ b/sysdeps/unix/sysv/linux/posix_fadvise64.c
@@ -56,3 +56,7 @@  compat_symbol (libc, __posix_fadvise64_l32, posix_fadvise64, GLIBC_2_2);
 #else
 strong_alias (__posix_fadvise64_l64, posix_fadvise64);
 #endif
+
+#ifdef	__OFF_T_MATCHES_OFF64_T
+weak_alias(__posix_fadvise64_l64, __posix_fadvise)
+#endif /* __OFF_T_MATCHES_OFF64_T */
diff --git a/sysdeps/unix/sysv/linux/posix_fallocate.c b/sysdeps/unix/sysv/linux/posix_fallocate.c
index fc9ac37..f9ca34b 100644
--- a/sysdeps/unix/sysv/linux/posix_fallocate.c
+++ b/sysdeps/unix/sysv/linux/posix_fallocate.c
@@ -18,6 +18,8 @@ 
 #include <fcntl.h>
 #include <sysdep.h>
 
+#ifndef	__OFF_T_MATCHES_OFF64_T
+
 #define posix_fallocate static internal_fallocate
 #include <sysdeps/posix/posix_fallocate.c>
 #undef posix_fallocate
@@ -37,3 +39,5 @@  posix_fallocate (int fd, __off_t offset, __off_t len)
     return INTERNAL_SYSCALL_ERRNO (res, err);
   return internal_fallocate (fd, offset, len);
 }
+
+#endif /* __OFF_T_MATCHES_OFF64_T */
diff --git a/sysdeps/unix/sysv/linux/posix_fallocate64.c b/sysdeps/unix/sysv/linux/posix_fallocate64.c
index 4a0a722..3a65d35 100644
--- a/sysdeps/unix/sysv/linux/posix_fallocate64.c
+++ b/sysdeps/unix/sysv/linux/posix_fallocate64.c
@@ -40,3 +40,7 @@  __posix_fallocate64_l64 (int fd, __off64_t offset, __off64_t len)
     return INTERNAL_SYSCALL_ERRNO (res, err);
   return internal_fallocate64 (fd, offset, len);
 }
+
+#ifdef	__OFF_T_MATCHES_OFF64_T
+weak_alias(__posix_fallocate64_l64, posix_fallocate)
+#endif