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

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

On 08-01-2016 18:43, Mike Frysinger wrote:
> On 07 Jan 2016 19:03, Adhemerval Zanella wrote:
>> On 07-01-2016 18:54, Mike Frysinger wrote:
>>> On 07 Jan 2016 18:30, Adhemerval Zanella wrote:
>>>> On 07-01-2016 17:52, Mike Frysinger wrote:
>>>>> On 07 Jan 2016 15:18, Adhemerval Zanella wrote:
>>>>>> From: Adhemerval Zanella <adhemerval.zanella@linaro.com>
>>>>>>
>>>>>> This patch consolidates all the pread/pread64 implementation for Linux
>>>>>> in only one (sysdeps/unix/sysv/linux/pread.c).  It also removes the
>>>>>> syscall from the auto-generation using assembly macros.
>>>>>>
>>>>>> For pread{64} offset argument placement the new SYSCALL_LL{64} macro
>>>>>> is used.  For pread ports that do not define __NR_pread will use
>>>>>> __NR_pread64 and for pread64 ports that dot define __NR_pread64 will
>>>>>> use __NR_pread for the syscall.
>>>>>
>>>>> just to make sure i'm reading this correctly, in the past 64-bit ports
>>>>> would have a pread symbol and pread64 would be an alias to it.  but with
>>>>> this patch, 64-bit ports now have two symbols w/identical code ?
>>>>
>>>> Indeed it is something I was thinking and I was not sure how to
>>>> fix it while not breaking the existing ABI.  Maybe only building
>>>> the p{read,write}.c if not WORDSIZE64 is define and creating an
>>>> alias on p{read,write}64.c. What do you think?
>>>
>>> why do you need to delete the pread64.c & syscall.lists entries ?
>>> if you kept those, wouldn't things remain the same ?
>>>
>>> otherwise you'd have to keep the pread64.c and add a new pread.c
>>> that just includes the 32-bit pread.c but also adds the aliases.
>>
>> The idea is exactly to remove the auto-generation from syscalls.list
>> to simplify the cancellation syscall generation (to in the future
>> just get rid of the assembly hackery in sysdep-cancel.h).
> 
> i'm not sold on the idea that syscalls.list is a bad thing and that
> having a lot of little stub files like below is an improvement:
> int close(int fd)
> {
>   return INLINE_SYSCALL (close, 1, fd);
> }
> 
> this is actually what we do in uClibc and i find it often has scaling
> issues when needing to fix issues across files.
> 
> it also requires that the port have the ability to do inline assembly
> such that it can stuff specific registers ... not all ports do.  but
> maybe glibc does not care about those and would rather require them to
> implement it.
> -mike
> 

I did not say that, what I said is the 'cancellable' syscalls is not a
good idea to go in syscalls.list.  You have duplicate logic (the assembly
macros) and one need to reimplement function calls in assembly (which 
should be handled by C) while taking care of PIC and non-PIC.

GLIBC already requires support of inline syscalls through INLINE macros,
so I do see a much better strategy to just implement cancellable
entrypoint on top of that.