Re: [PATCH] off_t: fix register pair calculation for 64-bit case

[Adhemerval Zanella <adhemerval dot zanella at linaro dot org>]

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
>