Re: [PATCH] Fix p{readv,writev}{64} consolidation implementation

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

On 15/06/2016 02:37, Mike Frysinger wrote:
> On 14 Jun 2016 18:54, Adhemerval Zanella wrote:
>> This patch fixes the p{readv,writev}{64} consolidation implementation
>> from commits 4e77815 and af5fdf5.  Different from pread/pwrite
>> implementation, preadv/pwritev implementation does not require
>> __ALIGNMENT_ARG because kernel syscall prototypes define
>> the high and low part of the off_t, if it is the case, directly
>> (different from pread/pwrite where the architecture ABI for passing
>> 64-bit values must be in consideration for passsing the arguments).
> 
> i had looked at that specifically but thought it ok because the old code
> was using the alignment arg.  was the old code broken too ?
> 
> this is what the preadv code looked like:
> -ssize_t
> -__libc_preadv (int fd, const struct iovec *vector, int count, off_t offset)
> -{
> -  assert (sizeof (offset) == 4);
> -  return SYSCALL_CANCEL (preadv, fd,
> -                         vector, count, __ALIGNMENT_ARG
> -                         __LONG_LONG_PAIR (offset >> 31, offset));
> -}
> 
> although i guess this isn't too surprising as this code was in the
> generic sysdeps dir which currently doesn't have as many users as
> we wish it did :).

I though it too, but before the consolidation patch only nios2 and tile 32-bits
used the generic preadv.c implementation.  And only tile 32-bits defines 
__ASSUME_ALIGNED_REGISTER_PAIRS (so __ALIGNMENT_ARG will pad the argument with 0).
However since the syscall is defined as in linux source:

fs/read_write.c:

 991 SYSCALL_DEFINE5(preadv, unsigned long, fd, const struct iovec __user *, vec,
 992                 unsigned long, vlen, unsigned long, pos_l, unsigned long, pos_h)
 993 {
 994         loff_t pos = pos_from_hilo(pos_h, pos_l);
 995 
 996         return do_preadv(fd, vec, vlen, pos, 0);
 997 }


The idea is not really to align the argument to zero pass, but rather to split
the possible 64-bits argument in high and low as required (as the default 
implementation was doing [2]). On tile, it is working because the preadv.c
offset is 32-bits and thus the high word is indeed zero, but it is passing
one superfluous argument.  Also my understanding is this generic implementation
does work correctly in every architecture because __LONG_LONG_PAIR relies
on endianess. 


[1] sysdeps/unix/sysv/linux/generic/wordsize-32/preadv.c
[2] sysdeps/unix/sysv/linux/preadv.c

> 
>> +#define FAIL() \
>> +  do { printf ("error: failure on line %d\n", __LINE__); return 1; } while (0)
>> ...
>> +  ret = pwritev (temp_fd, iov, 2, 0);
>> +  if (ret == -1)
>> +    FAIL ();
> 
> as a personal stance, never been a big fan of messages that just have a
> line number.  when you get reports/logs, you end having to chase down the
> exact same source tree as the reporter.
> 

Right, I will change to a more descriptive error message.

> why can't we just assert() everywhere ?  we rely on that in tests already
> and we wouldn't have to do any checking otherwise.
>   ret = pwritev (temp_fd, iov, 2, 0);
>   assert (ret == sizeof buf1 + sizeof buf2);
> -mike
> 

As Florian has stated assert writes on stderr :(