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

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

Hi

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.

> 
> [...]
> 
>> 	* 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.

> 
> 
>> diff --git a/sysdeps/unix/sysv/linux/sysdep-vdso.h b/sysdeps/unix/sysv/linux/sysdep-vdso.h
>> new file mode 100644
>> index 0000000..70458d1
>> --- /dev/null
>> +++ b/sysdeps/unix/sysv/linux/sysdep-vdso.h
>> @@ -0,0 +1,98 @@
>> +/* vDSO common definition for Linux.
>> +   Copyright (C) 2015 Free Software Foundation, Inc.
>> +   This file is part of the GNU C Library.
>> +
>> +   The GNU C Library is free software; you can redistribute it and/or
>> +   modify it under the terms of the GNU Lesser General Public
>> +   License as published by the Free Software Foundation; either
>> +   version 2.1 of the License, or (at your option) any later version.
>> +G
> 
>    ^ Typo?
> 

Yes, I will fix, thanks.

> 
>> +   The GNU C Library is distributed in the hope that it will be useful,
>> +   but WITHOUT ANY WARRANTY; without even the implied warranty of
>> +   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
>> +   Lesser General Public License for more details.
>> +
>> +   You should have received a copy of the GNU Lesser General Public
>> +   License along with the GNU C Library; if not, see
>> +   <http://www.gnu.org/licenses/>.  */
>