powerpc: Use FLAG_ELF_LIBC6 for 32-bit known libraries

  In systems with more versions of the known libraries, i.e. on IBM
Advance Toolchain, ldconfig will order them incorrectly on ld.cache.

The issue only occurs with 32-bit libraries that don't depend on libc or
libm. That's because process_elf32_file check if the elf depends on one
of the libraries at known_libs to select the elf flag. For example, as
libc.so.6 don't depend on itself or on libm it will be flagged as
FLAG_ELF instead of FLAG_ELF_LIBC6 as expected.

This commit fixes this by checking if a appropriate flag was set by
process_elf32_file. If not it will search on known_libs and use the flag
in there.
---
 sysdeps/unix/sysv/linux/powerpc/readelflib.c | 21 +++++++++++++++++---
 1 file changed, 18 insertions(+), 3 deletions(-)
  

Ping.

Quoting Lucas A. M. Magalhaes via Libc-alpha (2021-11-04 15:10:39)
> In systems with more versions of the known libraries, i.e. on IBM
> Advance Toolchain, ldconfig will order them incorrectly on ld.cache.
> 
> The issue only occurs with 32-bit libraries that don't depend on libc or
> libm. That's because process_elf32_file check if the elf depends on one
> of the libraries at known_libs to select the elf flag. For example, as
> libc.so.6 don't depend on itself or on libm it will be flagged as
> FLAG_ELF instead of FLAG_ELF_LIBC6 as expected.
> 
> This commit fixes this by checking if a appropriate flag was set by
> process_elf32_file. If not it will search on known_libs and use the flag
> in there.
> ---
>  sysdeps/unix/sysv/linux/powerpc/readelflib.c | 21 +++++++++++++++++---
>  1 file changed, 18 insertions(+), 3 deletions(-)
> 
> diff --git a/sysdeps/unix/sysv/linux/powerpc/readelflib.c b/sysdeps/unix/sysv/linux/powerpc/readelflib.c
> index 51f8a9496a..94da21c407 100644
> --- a/sysdeps/unix/sysv/linux/powerpc/readelflib.c
> +++ b/sysdeps/unix/sysv/linux/powerpc/readelflib.c
> @@ -33,11 +33,26 @@ process_elf_file (const char *file_name, const char *lib, int *flag,
>                   char **soname, void *file_contents, size_t file_length)
>  {
>    ElfW(Ehdr) *elf_header = (ElfW(Ehdr) *) file_contents;
> -  int ret;
> +  int ret, j;
>  
>    if (elf_header->e_ident [EI_CLASS] == ELFCLASS32)
> -    return process_elf32_file (file_name, lib, flag, osversion, isa_level,
> -                              soname, file_contents, file_length);
> +    {
> +      ret = process_elf32_file (file_name, lib, flag, osversion, isa_level,
> +                                soname, file_contents, file_length);
> +      /* Use the apropriate flag for known_libs instead of FLAG_ELF.  */
> +      if (*flag == FLAG_ELF)
> +        {
> +          for (j = 0;
> +               j < sizeof (known_libs) / sizeof (known_libs [0]);
> +               ++j)
> +            if (strcmp (lib, known_libs [j].soname) == 0)
> +              {
> +                *flag = known_libs [j].flag;
> +                break;
> +              }
> +        }
> +      return ret;
> +    }
>    else
>      {
>        ret = process_elf64_file (file_name, lib, flag, osversion, isa_level,
> -- 
> 2.31.1
>
  

* Lucas A. M. Magalhaes:

> In systems with more versions of the known libraries, i.e. on IBM
> Advance Toolchain, ldconfig will order them incorrectly on ld.cache.
>
> The issue only occurs with 32-bit libraries that don't depend on libc or
> libm. That's because process_elf32_file check if the elf depends on one
> of the libraries at known_libs to select the elf flag. For example, as
> libc.so.6 don't depend on itself or on libm it will be flagged as
> FLAG_ELF instead of FLAG_ELF_LIBC6 as expected.
>
> This commit fixes this by checking if a appropriate flag was set by
> process_elf32_file. If not it will search on known_libs and use the flag
> in there.

I would like to defer review of this patch to the POWER maintainers, sorry.
(I just spotted a typo in the first version.)

Thanks,
Florian
  

On 04/11/2021 15:10, Lucas A. M. Magalhaes via Libc-alpha wrote:
> In systems with more versions of the known libraries, i.e. on IBM
> Advance Toolchain, ldconfig will order them incorrectly on ld.cache.
> 
> The issue only occurs with 32-bit libraries that don't depend on libc or
> libm. That's because process_elf32_file check if the elf depends on one
> of the libraries at known_libs to select the elf flag. For example, as
> libc.so.6 don't depend on itself or on libm it will be flagged as
> FLAG_ELF instead of FLAG_ELF_LIBC6 as expected.

Wouldn't be simpler to check if the DT_SONAME matches any on 'known_libs' after
dynamic section parsing and set the appropriated flag on generic 'process_file'? 

(also powerpc SYSDEP_KNOWN_LIBRARY_NAMES seems redundant).

> 
> This commit fixes this by checking if a appropriate flag was set by
> process_elf32_file. If not it will search on known_libs and use the flag
> in there.
> ---
>  sysdeps/unix/sysv/linux/powerpc/readelflib.c | 21 +++++++++++++++++---
>  1 file changed, 18 insertions(+), 3 deletions(-)
> 
> diff --git a/sysdeps/unix/sysv/linux/powerpc/readelflib.c b/sysdeps/unix/sysv/linux/powerpc/readelflib.c
> index 51f8a9496a..94da21c407 100644
> --- a/sysdeps/unix/sysv/linux/powerpc/readelflib.c
> +++ b/sysdeps/unix/sysv/linux/powerpc/readelflib.c
> @@ -33,11 +33,26 @@ process_elf_file (const char *file_name, const char *lib, int *flag,
>  		  char **soname, void *file_contents, size_t file_length)
>  {
>    ElfW(Ehdr) *elf_header = (ElfW(Ehdr) *) file_contents;
> -  int ret;
> +  int ret, j;
>  
>    if (elf_header->e_ident [EI_CLASS] == ELFCLASS32)
> -    return process_elf32_file (file_name, lib, flag, osversion, isa_level,
> -			       soname, file_contents, file_length);
> +    {
> +      ret = process_elf32_file (file_name, lib, flag, osversion, isa_level,
> +                                soname, file_contents, file_length);
> +      /* Use the apropriate flag for known_libs instead of FLAG_ELF.  */
> +      if (*flag == FLAG_ELF)
> +        {
> +          for (j = 0;
> +               j < sizeof (known_libs) / sizeof (known_libs [0]);
> +               ++j)
> +            if (strcmp (lib, known_libs [j].soname) == 0)
> +              {
> +                *flag = known_libs [j].flag;
> +                break;
> +              }
> +        }
> +      return ret;
> +    }
>    else
>      {
>        ret = process_elf64_file (file_name, lib, flag, osversion, isa_level,
>
  

I tested this on top of master without regression in the following
configurations:

powerpc-linux-gnu
powerpc64-linux-gnu  (with -m64 and with m32)
powerpc64le-linux-gnu


I know that Adhemerval raised some points about the approach chosen (I
don't have the knowledge to comment about that right now), but apart
from that this implementation LGTM.


o/
Raoni
  

On Fri, Dec 03, 2021 at 11:51:29AM -0300, AL glibc-alpha wrote:
> 
> 
> On 04/11/2021 15:10, Lucas A. M. Magalhaes via Libc-alpha wrote:
> > In systems with more versions of the known libraries, i.e. on IBM
> > Advance Toolchain, ldconfig will order them incorrectly on ld.cache.
> > 
> > The issue only occurs with 32-bit libraries that don't depend on libc or
> > libm. That's because process_elf32_file check if the elf depends on one
> > of the libraries at known_libs to select the elf flag. For example, as
> > libc.so.6 don't depend on itself or on libm it will be flagged as
> > FLAG_ELF instead of FLAG_ELF_LIBC6 as expected.
> 
> Wouldn't be simpler to check if the DT_SONAME matches any on 'known_libs' after
> dynamic section parsing and set the appropriated flag on generic 'process_file'? 

But then it would touch every architecture, I think since this is only a
problem affecting powerpc (BE 32bits) I don't think it is worth touch
the generic code if will not help any other architecture.


> (also powerpc SYSDEP_KNOWN_LIBRARY_NAMES seems redundant).

Seems to be the same as every other arch except i386. Lookin at Lucas's
RFC for removal of libc.5 and libc.4 compatibility, maybe it being
diferent in i386 is just that it was never really used anymore, and so
never updated.


o/
Raoni
  

On 02/03/2022 16:30, Raoni Fassina Firmino wrote:
> On Fri, Dec 03, 2021 at 11:51:29AM -0300, AL glibc-alpha wrote:
>>
>>
>> On 04/11/2021 15:10, Lucas A. M. Magalhaes via Libc-alpha wrote:
>>> In systems with more versions of the known libraries, i.e. on IBM
>>> Advance Toolchain, ldconfig will order them incorrectly on ld.cache.
>>>
>>> The issue only occurs with 32-bit libraries that don't depend on libc or
>>> libm. That's because process_elf32_file check if the elf depends on one
>>> of the libraries at known_libs to select the elf flag. For example, as
>>> libc.so.6 don't depend on itself or on libm it will be flagged as
>>> FLAG_ELF instead of FLAG_ELF_LIBC6 as expected.
>>
>> Wouldn't be simpler to check if the DT_SONAME matches any on 'known_libs' after
>> dynamic section parsing and set the appropriated flag on generic 'process_file'? 
> 
> But then it would touch every architecture, I think since this is only a
> problem affecting powerpc (BE 32bits) I don't think it is worth touch
> the generic code if will not help any other architecture.

Which is not a bad idea, removing arch-specific code and make the ldconfig
have a similar semantic interdependently of the ABI is a good move forward. 

> 
> 
>> (also powerpc SYSDEP_KNOWN_LIBRARY_NAMES seems redundant).
> 
> Seems to be the same as every other arch except i386. Lookin at Lucas's
> RFC for removal of libc.5 and libc.4 compatibility, maybe it being
> diferent in i386 is just that it was never really used anymore, and so
> never updated.

I think it would be better to send this change along the the RPC to
remove libc4/libc5 compatibility.  I am already proposing dropping some 
support for old lib5/aout files [1], which Florian thinks it is ok,
so I take that dropping all support for libc4/libc5 is a way forward.

In fact, with libc4/libc5 removal I think this patch is not really 
required.

[1] https://patchwork.sourceware.org/project/glibc/patch/20220304133801.1868553-4-adhemerval.zanella@linaro.org/