This is the mail archive of the
libc-alpha@sourceware.org
mailing list for the glibc project.
Re: [PATCH] powerpc: Move cache line size to rtld_global_ro
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