Re: [glibc] preadv/pwritev question

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

On 31/05/2016 19:00, Mike Frysinger wrote:
> On 31 May 2016 17:00, Chris Metcalf wrote:
>> On 5/31/2016 4:04 PM, Yury Norov wrote:
>>> In path a63c7fa18a (Add sysdeps/unix/sysv/linux/generic/.) you add
>>> this:
>>> +++ b/sysdeps/unix/sysv/linux/generic/wordsize-32/preadv.c
>>>
>>> [...]
>>>
>>> +static ssize_t
>>> +do_preadv (int fd, const struct iovec *vector, int count, off_t
>>> offset)
>>> +{
>>> +  assert (sizeof (offset) == 4);
>>> +  return INLINE_SYSCALL (preadv, __ALIGNMENT_COUNT (5, 6), fd,
>>> +                         vector, count, __ALIGNMENT_ARG
>>> +                         __LONG_LONG_PAIR (offset >> 31, offset));
>>> +}
>>> +
>>>
>>> And this is the code that is picked up if I choose wordsize-32 for my
>>> AARCH64/ILP32. So I have questions.
>>>
>>> 1. What is the assert for? We agreed that all new ABIs will be 64-bit
>>> off_t only.
>>>
>>> I fixed it internally like this:
>>> +#ifndef __OFF_T_MATCHES_OFF64_T
>>>          assert (sizeof (offset) == 4);
>>> +#endif
>>>
>>> There is a bunch of similar assertions in glibc.
>>>
>>> 2. This one looks weird:
>>> __LONG_LONG_PAIR (offset >> 31, offset))
>>> Why 31-bit offset? And why you don't mask 2nd argument?
>>> Later in your patch I see this:
>>> +static ssize_t
>>> +do_preadv64 (int fd, const struct iovec *vector, int count, off64_t
>>> offset)
>>>
>>> +{
>>> +  return INLINE_SYSCALL (preadv, __ALIGNMENT_COUNT (5, 6), fd,
>>> +                         vector, count, __ALIGNMENT_ARG
>>> +                         __LONG_LONG_PAIR ((off_t) (offset >> 32),
>>> +                                           (off_t) (offset & 0xffffffff)));
>>> +}
>>>
>>> And it looks correct to me. If 1st version is correct as well, I think
>>> it should be commented.
>>
>> I did this work before x32 came out, so I tried to model it more closely on
>> the existing x86 compat API.  I agree that a 64-bit off_t model seems reasonable;
>> however, the code does exactly what it does to match x86, namely preadv() takes
>> a 32-bit offset, and preadv64() take a 64-bit offset.  The assert() in preadv to force
>> sizeof to be 4 is exactly why in that routine we use (offset >> 31, offset).  Since
>> we know offset fits in 32 bits, all we need to do is properly sign-extend it into
>> 64 bits in the high register of the pair, which is what (offset >> 31) does - you end
>> up with only 0 or -1, thus sign-extending the 32-bit signed off_t. Then in
>> preadv64() we actually need to break apart the 64-bit offset into a high 32 bits
>> and a low 32 bits, which is what (offset >> 32, offset & 0xffffffff) does.
>>
>> For a 64-bit off_t you will want to not compile preadv.c at all, and instead make
>> __libc_preadv() and friends be aliases of __libc_preadv64().
> 
> sounds like Adhemerval's pread/pwrite unify work should be extended to
> the preadv/pwritev funcs.  it deals with the ilp32 case and uses the new
> SYSCALL_LL macro to deal with the ugly shifting/masking.
> 
> check out these commits:
> https://sourceware.org/git/?p=glibc.git;a=commit;h=071af4769fcdfe2cd349157b01f27c9571478ace
> https://sourceware.org/git/?p=glibc.git;a=commit;h=77a4fbd53611720cd6ae845de560df5dd332b28e
> https://sourceware.org/git/?p=glibc.git;a=commit;h=eeddfa91cbb1a619af135c7a9ac14251ec094b7a
> -mike

In fact I already did this for preadv/pwrite that I plan to send for
review soon.

Attachment: signature.asc
Description: OpenPGP digital signature