Re: [PATCH] powerpc: Move cache line size to rtld_global_ro

[Tulio Magno Quites Machado Filho <tuliom@ascii.art.br>]

Adhemerval Zanella <adhemerval.zanella@linaro.org> writes:

> On 23/12/2019 15:45, Tulio Magno Quites Machado Filho wrote:
>> GCC 10.0 enabled -fno-common by default and this started to point that
>> __cache_line_size had been implemented in 2 different places: loader and
>> libc.
>> 
>> In order to avoid this duplication, the libc variable has been removed
>> and the loader variable is moved to rtld_global_ro.
>> 
>> File sysdeps/unix/sysv/linux/powerpc/dl-auxv.h has been added in order
>> to reuse code for both static and dynamic linking scenarios.
>> 
>> Signed-off-by: Tulio Magno Quites Machado Filho <tuliom@linux.ibm.com>
>
> We do not use signed-off.  

Ack.

> Although not strickly necessary since the memset.c implementation already
> check for non set _dl_cache_line_size, you may also add the cache-line
> initialization for the static dlopen on dl-static.c (as powerpc
> already does for dl_pagesize).

Good point.  That would allow a dlopened DSO from a statically linked
executable access dl_cache_line_size too.

> I assume you have also checked on powerpc-linux-gnu and powerpc64-linux-gnu,
> correct?

Correct.

>> diff --git a/sysdeps/powerpc/powerpc32/a2/memcpy.S b/sysdeps/powerpc/powerpc32/a2/memcpy.S
>> index 9bc91a8df1..ecfea5c832 100644
>> --- a/sysdeps/powerpc/powerpc32/a2/memcpy.S
>> +++ b/sysdeps/powerpc/powerpc32/a2/memcpy.S
>> @@ -18,6 +18,7 @@
>>     <https://www.gnu.org/licenses/>.  */
>>  
>>  #include <sysdep.h>
>> +#include <rtld-global-offsets.h>
>>  
>>  #define PREFETCH_AHEAD 4        /* no cache lines SRC prefetching ahead  */
>>  #define ZERO_AHEAD 2            /* no cache lines DST zeroing ahead  */
>> @@ -106,25 +107,23 @@ EALIGN (memcpy, 5, 0)
>>  L(dst_aligned):
>>  
>>  
>> -#ifdef SHARED
>> +#ifdef PIC
>>  	mflr    r0
>> -/* Establishes GOT addressability so we can load __cache_line_size
>> -   from static. This value was set from the aux vector during startup.  */
>> +/* Establishes GOT addressability so we can load the cache line size
>> +   from rtld_global_ro. This value was set from the aux vector during
>> +   startup.  */
>
> My understanding is code guidelines states two spaces after the end of a 
> sentence in comments.

Indeed.

>> diff --git a/sysdeps/powerpc/powerpc64/a2/memcpy.S b/sysdeps/powerpc/powerpc64/a2/memcpy.S
>> index 6d5c5afddb..e515255126 100644
>> --- a/sysdeps/powerpc/powerpc64/a2/memcpy.S
>> +++ b/sysdeps/powerpc/powerpc64/a2/memcpy.S
>> @@ -18,6 +18,7 @@
>>     <https://www.gnu.org/licenses/>.  */
>>  
>>  #include <sysdep.h>
>> +#include <rtld-global-offsets.h>
>>  
>>  #ifndef MEMCPY
>>  # define MEMCPY memcpy
>> @@ -27,8 +28,19 @@
>>  #define ZERO_AHEAD 2            /* no cache lines DST zeroing ahead  */
>>  
>>  	.section        ".toc","aw"
>> -.LC0:
>> -	.tc __cache_line_size[TC],__cache_line_size
>> +.LC__dl_cache_line_size:
>> +#ifdef SHARED
>> +# if IS_IN (rtld)
>> +	/* Inside ld.so we use the local alias to avoid runtime GOT
>> +	   relocations.  */
>> +	.tc _rtld_local_ro[TC],_rtld_local_ro
>> +# else
>> +	.tc _rtld_global_ro[TC],_rtld_global_ro
>> +# endif
>> +#else
>> +	.tc _dl_cache_line_size[TC],_dl_cache_line_size
>> +#endif
>> +
>>  	.section        ".text"
>>  	.align 2
>>  
>
> Ok. Maybe add a similar macro for powerpc64 as you did for powerpc32 with
> __GLRO ?

Are you suggesting to create a __GLRO macro that would define this label?

>> diff --git a/sysdeps/unix/sysv/linux/powerpc/dl-support.c b/sysdeps/unix/sysv/linux/powerpc/dl-support.c
>> new file mode 100644
>> index 0000000000..2a792d7092
>> --- /dev/null
>> +++ b/sysdeps/unix/sysv/linux/powerpc/dl-support.c
>> @@ -0,0 +1,23 @@
>> +/* Support for dynamic linking code in static libc.  Linux/PPC version.
>> +   Copyright (C) 2019 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.
>> +
>> +   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
>> +   <https://www.gnu.org/licenses/>.  */
>> +
>> +#include "dl-auxv.h"
>> +#include <ldsodefs.h>
>> +int GLRO(dl_cache_line_size);
>> +
>> +#include <elf/dl-support.c>
>
> Why is it required? It sould be already covered by inclusion of
> sysdeps/powerpc/dl-procinfo.c and it does not required the dl-auxv.h
> definitions here.

This is the code that initializes it statically.  It avoids duplicating code.
dl-procinfo.c does not copy AT_DCACHEBSIZE to GLRO(dl_cache_line_size).

>> diff --git a/sysdeps/unix/sysv/linux/powerpc/dl-sysdep.c b/sysdeps/unix/sysv/linux/powerpc/dl-sysdep.c
>> index fa19cc66c0..6d51d6061e 100644
>> --- a/sysdeps/unix/sysv/linux/powerpc/dl-sysdep.c
>> +++ b/sysdeps/unix/sysv/linux/powerpc/dl-sysdep.c
>> @@ -18,16 +18,6 @@
>>  
>>  #include <config.h>
>>  #include <ldsodefs.h>
>> -
>> -int __cache_line_size attribute_hidden;
>> -
>> -/* Scan the Aux Vector for the "Data Cache Block Size" entry.  If found
>> -   verify that the static extern __cache_line_size is defined by checking
>> -   for not NULL.  If it is defined then assign the cache block size
>> -   value to __cache_line_size.  */
>> -#define DL_PLATFORM_AUXV						      \
>> -      case AT_DCACHEBSIZE:						      \
>> -	__cache_line_size = av->a_un.a_val;				      \
>> -	break;
>> +#include <dl-auxv.h>
>>  
>>  #include <sysdeps/unix/sysv/linux/dl-sysdep.c>
>
> This is essentially an empty file that just include dl-auxv.h. I think
> it would be better to just create a generic empty dl-auxv.h, include it
> on default dl-sysdep.c, and remove this file.

I'm fine with this proposal, but I don't think it's going to help neither alpha
or x86 which have slightly different implementations.

-- 
Tulio Magno