[PATCH] elf: Replace PI_STATIC_AND_HIDDEN with opposite HIDDEN_VAR_NEEDS_DYNAMIC_RELOC

[Adhemerval Zanella]

On 25/04/2022 13:40, Adhemerval Zanella wrote:
> 
> 
> On 25/04/2022 04:58, Fangrui Song wrote:
>> PI_STATIC_AND_HIDDEN indicates whether accesses to internal linkage
>> variables and hidden visibility variables in a shared object (ld.so)
>> need dynamic relocations (usually R_*_RELATIVE). PI (position
>> independent) in the macro name is a misnomer: a code sequence using GOT
>> is typically position-independent as well, but using dynamic relocations
>> does not meet the requirement.
>>
>> Not defining PI_STATIC_AND_HIDDEN is legacy and we expect that all new
>> ports will define PI_STATIC_AND_HIDDEN. Current ports defining
>> PI_STATIC_AND_HIDDEN are more than the opposite. Change the configure
>> default.
>>
>> No functional change.
>>
>> Note: HIDDEN_VAR_NEEDS_DYNAMIC_RELOC will be removed when Alan Modra's
>> powerpc64 static-pie work
>> (https://sourceware.org/pipermail/libc-alpha/2022-April/137987.html)
>> lands.
> 
> I agree the macro naming is confusing and does not represent the intent
> is a straigthforward manner.  Patch looks ok, but I have added some
> comments below.
> 
>> ---
>>  config.h.in                            |   6 +-
>>  elf/rtld.c                             |   2 +-
>>  sysdeps/aarch64/configure              |   5 -
>>  sysdeps/aarch64/configure.ac           |   4 -
>>  sysdeps/alpha/configure                |   5 -
>>  sysdeps/alpha/configure.ac             |   4 -
>>  sysdeps/arc/configure                  |   2 -
>>  sysdeps/arc/configure.ac               |   1 -
>>  sysdeps/arm/configure                  |   3 -
>>  sysdeps/arm/configure.ac               |   5 -
>>  sysdeps/csky/configure                 |   3 -
>>  sysdeps/csky/configure.ac              |   2 -
>>  sysdeps/hppa/configure                 |   3 +
>>  sysdeps/hppa/configure.ac              |   2 +
>>  sysdeps/ia64/configure                 |   3 -
>>  sysdeps/ia64/configure.ac              |   4 -
>>  sysdeps/m68k/configure                 | 165 +++++++++++++++++++++++++
>>  sysdeps/m68k/configure.ac              |   6 +
>>  sysdeps/microblaze/configure           |   3 +
>>  sysdeps/microblaze/configure.ac        |   2 +
>>  sysdeps/mips/configure                 |   2 +
>>  sysdeps/mips/configure.ac              |   2 +-
>>  sysdeps/nios2/configure                |   3 +
>>  sysdeps/nios2/configure.ac             |   2 +
>>  sysdeps/or1k/configure                 |   3 -
>>  sysdeps/or1k/configure.ac              |   2 -
>>  sysdeps/powerpc/powerpc32/configure    |   3 +
>>  sysdeps/powerpc/powerpc32/configure.ac |   2 +
>>  sysdeps/powerpc/powerpc64/configure    |   3 +
>>  sysdeps/powerpc/powerpc64/configure.ac |   2 +
>>  sysdeps/powerpc/tst-tlsifunc.c         |   2 +-
>>  sysdeps/riscv/configure                |   3 -
>>  sysdeps/riscv/configure.ac             |   2 -
>>  sysdeps/s390/configure                 |   3 -
>>  sysdeps/s390/configure.ac              |   4 -
>>  sysdeps/sh/configure                   |   3 -
>>  sysdeps/sh/configure.ac                |   4 -
>>  sysdeps/sparc/configure                |   2 -
>>  sysdeps/sparc/configure.ac             |   2 -
>>  sysdeps/x86/configure                  |   3 -
>>  sysdeps/x86/configure.ac               |   4 -
>>  41 files changed, 204 insertions(+), 82 deletions(-)
>>  create mode 100644 sysdeps/m68k/configure
>>  create mode 100644 sysdeps/m68k/configure.ac
>>
>> diff --git a/config.h.in b/config.h.in
>> index a94f756859..916465fce8 100644
>> --- a/config.h.in
>> +++ b/config.h.in
>> @@ -84,9 +84,9 @@
>>  /* Define if the compiler\'s exception support is based on libunwind.  */
>>  #undef	HAVE_CC_WITH_LIBUNWIND
>>  
>> -/* Define if the access to static and hidden variables is position independent
>> -   and does not need relocations.  */
>> -#undef	PI_STATIC_AND_HIDDEN
>> +/* Define if the accesses to static and hidden variables in a shared object
>> +   need dynamic relocations.  */
>> +#undef	HIDDEN_VAR_NEEDS_DYNAMIC_RELOC
>>  
>>  /* Define this to disable the 'hidden_proto' et al macros in
>>     include/libc-symbols.h that avoid PLT slots in PIE.  */
> 
> OK, naming make more sense now.
> 
>> diff --git a/elf/rtld.c b/elf/rtld.c
>> index 19e328f89e..2174cc1da7 100644
>> --- a/elf/rtld.c
>> +++ b/elf/rtld.c
>> @@ -424,7 +424,7 @@ DL_SYSINFO_IMPLEMENTATION
>>     is fine, too.  The latter is important here.  We can avoid setting
>>     up a temporary link map for ld.so if we can mark _rtld_global as
>>     hidden.  */
>> -#ifdef PI_STATIC_AND_HIDDEN
>> +#ifndef HIDDEN_VAR_NEEDS_DYNAMIC_RELOC
>>  # define DONT_USE_BOOTSTRAP_MAP	1
>>  #endif
>>  
> 
> I think it would be better to now always assume the variable is defined:
> 
>   #if HIDDEN_VAR_NEEDS_DYNAMIC_RELOC
>   # define HIDDEN_VAR_NEEDS_DYNAMIC_RELOC 1
>   #endif

Thinking twice there is not how config.h work now so it is ok as is.  The
only issue with the patch is only the m68k configure that seems bogus.