From: Tulio Magno Quites Machado Filho <tuliom@ascii.art.br>
To: Adhemerval Zanella <adhemerval.zanella@linaro.org>,
libc-alpha@sourceware.org
Subject: Re: [PATCH] powerpc: Move cache line size to rtld_global_ro
Date: Fri, 27 Dec 2019 12:42:07 -0300 [thread overview]
Message-ID: <875zi16ai8.fsf@linux.ibm.com> (raw)
In-Reply-To: <b719ad08-c818-7963-1b10-341f04af7c5e@linaro.org>
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
next prev parent reply other threads:[~2019-12-27 15:42 UTC|newest]
Thread overview: 37+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-11-21 1:19 powerpc build failure with GCC mainline -fno-common change Joseph Myers
2019-11-21 9:23 ` Andreas Schwab
2019-12-23 18:45 ` [PATCH] powerpc: Move cache line size to rtld_global_ro Tulio Magno Quites Machado Filho
2019-12-26 17:04 ` Adhemerval Zanella
2019-12-27 15:42 ` Tulio Magno Quites Machado Filho [this message]
2019-12-27 16:47 ` Adhemerval Zanella
2020-01-09 10:29 ` Florian Weimer
2020-01-09 16:43 ` Jeff Law
2020-01-09 17:20 ` Tulio Magno Quites Machado Filho
2020-01-09 18:03 ` Adhemerval Zanella
2020-01-09 18:49 ` Tulio Magno Quites Machado Filho
2020-01-09 18:54 ` Adhemerval Zanella
2020-01-10 22:27 ` [PATCH 1/2] powerpc: Initialize rtld_global_ro for static dlopen [BZ #20802] Tulio Magno Quites Machado Filho
2020-01-10 22:27 ` [PATCH 2/2] powerpc: Move cache line size to rtld_global_ro Tulio Magno Quites Machado Filho
2020-01-16 16:37 ` Carlos O'Donell
2020-01-17 3:56 ` Siddhesh Poyarekar
2020-01-17 12:39 ` Tulio Magno Quites Machado Filho
2020-01-17 17:14 ` Florian Weimer
2020-01-17 17:15 ` Joseph Myers
2020-01-17 22:28 ` Tulio Magno Quites Machado Filho
2020-01-17 23:45 ` [PATCH] powerpc32: Fix syntax error in __GLRO macro Andreas Schwab
2020-01-17 23:56 ` Tulio Magno Quites Machado Filho
2020-07-23 14:47 ` [PATCH 2/2] powerpc: Move cache line size to rtld_global_ro Joseph Myers
2020-07-23 16:46 ` Andreas Schwab
2020-07-31 12:42 ` Florian Weimer via Libc-alpha
2020-07-31 18:35 ` Joseph Myers
2020-08-03 8:15 ` Florian Weimer via Libc-alpha
2020-08-03 16:07 ` Carlos O'Donell via Libc-alpha
2020-01-16 16:21 ` [PATCH 1/2] powerpc: Initialize rtld_global_ro for static dlopen [BZ #20802] Carlos O'Donell
2020-01-16 16:25 ` Siddhesh Poyarekar
2020-01-09 18:56 ` [PATCH] powerpc: Move cache line size to rtld_global_ro Tulio Magno Quites Machado Filho
2020-01-10 9:38 ` Florian Weimer
2020-01-10 16:09 ` Shawn Landden
2020-01-10 18:19 ` Adhemerval Zanella
2019-11-22 16:58 ` powerpc build failure with GCC mainline -fno-common change Tulio Magno Quites Machado Filho
2019-11-23 13:23 ` Florian Weimer
2019-11-26 13:26 ` Tulio Magno Quites Machado Filho
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
List information: https://www.gnu.org/software/libc/involved.html
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=875zi16ai8.fsf@linux.ibm.com \
--to=tuliom@ascii.art.br \
--cc=adhemerval.zanella@linaro.org \
--cc=libc-alpha@sourceware.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).