Re: [PATCH v2 1/3] Consolidate vDSO macros and usage

[Nathan Lynch <Nathan_Lynch at codesourcery dot com>]

On 04/22/2015 01:18 PM, Adhemerval Zanella wrote:
> On 22-04-2015 13:17, Nathan Lynch wrote:
>> On 04/20/2015 11:47 AM, Adhemerval Zanella wrote:
>>> This patch consolidate the Linux vDSO define and usage across all ports
>>> that uses it.  The common vDSO definitions and calling through
>>> {INLINE/INTERNAL}_VSYSCALL macros are moved to a common header
>>> sysdep-vdso.h and vDSO name declaration and prototype is defined
>>> using a common macro.
>>>
>>> Also PTR_{MANGLE,DEMANGLE} is added to ports that does not use them
>>> for vDSO calls (aarch64, powerpc, s390, and tile).
>>
>> Obviously we can expect this patch to result in changes to generated
>> code for ports which did not use pointer mangling before.  But what
>> about the others?  It looks to me like the generated code for s390, i386
>> and x86_64 should not change, is that right?
> 
> The idea is to use only a single INLINE_VSYSCALL over all the arches
> and to accommodate for ports that do not define it it would require
> more complexity and another define to enable PTR_MANGLE/DEMANGLE.
> My understanding is PTR_MANGLE should be use on all internal GLIBC
> pointer-as-function call, so I see the best approach to add
> PTR_MANGLE/DEMANGLE on vDSO to all archs.
> 
> I can split the patch in two parts, the refactor and the PTR_MANGLE
> consolidation.  But, as before, I think it will just overcomplicate
> the cleanup.

I didn't suggest splitting up the patch (although I wouldn't object to
it), nor did I object to adding pointer mangling where it's needed.  I
just think it could ease review to note which ports should be expected
to experience no change in generated code.


>>
>> [...]
>>
>>> 	* sysdeps/unix/sysv/linux/powerpc/powerpc32/sysdep.h
>>> 	(INLINE_VSYSCALL): Remove macro.
>>> 	(INTERNAL_VSYSCALL): Remove macro.
>>> 	(INTERNAL_VSYSCALL_NCS): Define using INTERNAL_VSYSCALL_NCS_TYPE.
>>
>> Does continuing to use the "NCS" term in vDSO-related code make sense?
>> As best I can tell, "NCS" refers to support for non-constant syscall
>> numbers, as in INTERNAL_SYSCALL_NCS.  I have found its current usage in
>> powerpc et al. confusing -- INTERNAL_VSYSCALL_NCS does not have the same
>> relationship to INTERNAL_VSYSCALL as INTERNAL_SYSCALL_NCS has to
>> INTERNAL_SYSCALL.
>>
> 
> Good question, I do not have a preference here.  Maybe to change
> INTERNAL_VSYSCALL_NCS to INTERNAL_VSYSCALL_CALL would be better.

Something like that, yes.