Re: [PATCH 3/4] Consolidate pread/pread64 implementations

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

On 23-02-2016 14:57, Joseph Myers wrote:
> On Tue, 23 Feb 2016, Adhemerval Zanella wrote:
> 
>> +/* The n32 have sizeof(off_t) != sizeof(off64_t) so we can't strong/weak
>> +   alias the pread to pread64.  We undefine the __ASSUME_WORDSIZE64_ILP32
>> +   so two implementation are built.  */
>> +#if _MIPS_SIM == _ABIN32
>> +# undef __ASSUME_WORDSIZE64_ILP32
>>  #endif
> 
> I don't see how this will work, since you still need 64-bit arguments 
> passed in a single register, and __ASSUME_WORDSIZE64_ILP32 controls 
> SYSCALL_LL* in your patch.

It will since mips p{read,write}.c implementation first include sysdep-cancel.h
and by that it will get the correct SYSCALL_LL{64} definition for the ABI.
The undefine and later include on generic p{read,write} will just block the
build symbol and create the alias for p{read,write}64.


> 
> Also, you're including sysdeps/unix/sysv/linux/generic/sysdep.h for 
> architectures that don't use the generic ABI.  If that's safe, that 
> suggests it should just be merged into sysdeps/unix/sysv/linux/sysdep.h 
> (in a separate patch with a careful argument for why the merge is safe) 
> rather than keeping the headers separate.

I decided to add it on generic sysdep.h because it is where port will get
the definition of __ALIGNMENT_ARG.  However I can split the second path
in two, one to include generic sysdep.h and another to define SYSCALL_LL{64}.


> 
> The n32 ABI is that 32-bit arguments must be sign-extended to 64-bit when 
> passed in registers (even if the arguments are of an unsigned type).  So I 
> think it should be safe for pread to be aliased to pread64 for n32 (it 
> does have to be that way round, the function being defined with the 64-bit 
> argument type) - though aliasing functions with incompatible C types may 
> be tricky.

The patch is doing what current implementation for pread{64}.c does for mips.
This alias could be a following cleanup patch (which is not the intention
of this patch).