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
On 27/12/2019 12:42, Tulio Magno Quites Machado Filho wrote:
> 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?
Or something similar to avoid the duplicated defition here and at
sysdeps/powerpc/powerpc64/memset.S.
>
>>> 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).
But with a generic dl-auxv.h that defines DL_PLATFORM_AUXV, this
files is not really required.
>
>>> 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.
Indeed, but I think we can refactor alpha or x86 later.