This is the mail archive of the libc-alpha@sourceware.org mailing list for the glibc project.
Index Nav: | [Date Index] [Subject Index] [Author Index] [Thread Index] | |
---|---|---|
Message Nav: | [Date Prev] [Date Next] | [Thread Prev] [Thread Next] |
Other format: | [Raw text] |
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
Index Nav: | [Date Index] [Subject Index] [Author Index] [Thread Index] | |
---|---|---|
Message Nav: | [Date Prev] [Date Next] | [Thread Prev] [Thread Next] |