On 3/8/19 3:51 PM, Carlos O'Donell wrote: > On 3/8/19 7:52 AM, Stefan Liebler wrote: >> Hi, >> >> starting with commit 1616d034b61622836d3a36af53dcfca7624c844e >> the output was corrupted on some platforms as _dl_procinfo >> was called for every auxv entry and on some architectures like s390 >> all entries were represented as "AT_HWCAP". >> >> This patch fixes the condition which determines if _dl_procinfo >> is called and adjusts _dl_procinfo implementations which assumed >> that they are only called for AT_HWCAP or AT_HWCAP2. >> >> A further question is if we should always call _dl_procinfo without a >> condition and let it decide if it is able to print an entry or if it >> should be printed with help of the auxvars list? > > This is a design question: > > (a) Call machine-specific routine *after* filtering out generic options. > > vs. > > (b) Call machine-specific routine for *all* options and let the the > machine-specific routine call a generic routine after it handles > all the options it wants. > > We should *always* call _dl_procinfo, and let the architectures decide > if they will handle the entry in a special way. Exactly this is currently done as the condition is always true and e.g. for x86_64 or powerpc it works correct. The generic code (in elf/dl-sysdep.c) is printing an entry if the arch is correctly returning a non-zero-value if it is not able to print it. This is either done via auxvars information or by just printing "AT_??? ..." for unknown entries. Please have a look at the attached patch. It does not require a helper function or refactoring. In fact it simplifies the patch. Compared to my previous patch only elf/dl-sysdep.c is changed. Thanks, Stefan > > We need to provide a "base" function helper to allow the arches to print > the information in a generic manner if their own code decides it doesn't > want to handle it. > > This would involve a refactoring of the existing code though, so I'm not > going to require it as part of fixing this for s390x. > > However, if we implement (b), then the generic code doesn't need to know > what will be handled by the machine-specific routines, so I see that as > a good cleanup. > > $0.02. > >> Okay to commit? >> Once it is committed I would also backport it to glibc 2.29 release branch. > > OK for master. > > Reviewed-by: Carlos O'Donell > >> Bye, >> Stefan >> >> ChangeLog: >> >>     * elf/dl-sysdep.c (_dl_show_auxv): Fix condition if _dl_procinfo >>     is called. >>     * sysdeps/unix/sysv/linux/s390/dl-procinfo.h (_dl_procinfo): >>     Ignore types other than AT_HWCAP. >>     * sysdeps/sparc/dl-procinfo.h (_dl_procinfo): Likewise. >>     * sysdeps/unix/sysv/linux/i386/dl-procinfo.h (_dl_procinfo): >>     Likewise. >> >> 20190308_ldshowauxv.patch >> >> commit 5bbde0719d7c5fa58b0140d6fb1c6fa31a372772 >> Author: Stefan Liebler >> Date: Fri Mar 8 11:18:53 2019 +0100 >> >> Fix output of LD_SHOW_AUXV=1. >> >> Starting with commit 1616d034b61622836d3a36af53dcfca7624c844e >> the output was corrupted on some platforms as _dl_procinfo >> was called for every auxv entry and on some architectures like s390 >> all entries were represented as "AT_HWCAP". >> >> This patch fixes the condition which determines if _dl_procinfo >> is called and adjusts _dl_procinfo implementations which assumed >> that they are only called for AT_HWCAP or AT_HWCAP2. > > OK. > >> ChangeLog: >> >> * elf/dl-sysdep.c (_dl_show_auxv): Fix condition if _dl_procinfo >> is called. >> * sysdeps/unix/sysv/linux/s390/dl-procinfo.h (_dl_procinfo): >> Ignore types other than AT_HWCAP. >> * sysdeps/sparc/dl-procinfo.h (_dl_procinfo): Likewise. >> * sysdeps/unix/sysv/linux/i386/dl-procinfo.h (_dl_procinfo): Likewise. >> >> diff --git a/elf/dl-sysdep.c b/elf/dl-sysdep.c >> index 5f6c679a3f..1588555651 100644 >> --- a/elf/dl-sysdep.c >> +++ b/elf/dl-sysdep.c >> @@ -329,8 +329,10 @@ _dl_show_auxv (void) >> assert (AT_IGNORE == 1); >> >> if (av->a_type == AT_HWCAP || av->a_type == AT_HWCAP2 >> - || AT_L1I_CACHEGEOMETRY || AT_L1D_CACHEGEOMETRY >> - || AT_L2_CACHEGEOMETRY || AT_L3_CACHEGEOMETRY) >> + || av->a_type == AT_L1I_CACHEGEOMETRY >> + || av->a_type == AT_L1D_CACHEGEOMETRY >> + || av->a_type == AT_L2_CACHEGEOMETRY >> + || av->a_type == AT_L3_CACHEGEOMETRY) > > OK. Fixes it so that we call into _dl_procinfo properly for the special cases. > >> { >> /* These are handled in a special way per platform. */ >> if (_dl_procinfo (av->a_type, av->a_un.a_val) == 0) >> diff --git a/sysdeps/sparc/dl-procinfo.h b/sysdeps/sparc/dl-procinfo.h >> index 282b8c5117..cc2687e99b 100644 >> --- a/sysdeps/sparc/dl-procinfo.h >> +++ b/sysdeps/sparc/dl-procinfo.h >> @@ -32,7 +32,7 @@ _dl_procinfo (unsigned int type, unsigned long int word) >> int i; >> >> /* Fallback to unknown output mechanism. */ >> - if (type == AT_HWCAP2) >> + if (type != AT_HWCAP) > > OK. > >> return -1; >> >> _dl_printf ("AT_HWCAP: "); >> diff --git a/sysdeps/unix/sysv/linux/i386/dl-procinfo.h b/sysdeps/unix/sysv/linux/i386/dl-procinfo.h >> index 22b43431bc..3aef14c6c1 100644 >> --- a/sysdeps/unix/sysv/linux/i386/dl-procinfo.h >> +++ b/sysdeps/unix/sysv/linux/i386/dl-procinfo.h >> @@ -31,7 +31,7 @@ _dl_procinfo (unsigned int type, unsigned long int word) >> int i; >> >> /* Fallback to unknown output mechanism. */ >> - if (type == AT_HWCAP2) >> + if (type != AT_HWCAP) > > OK. > >> return -1; >> >> _dl_printf ("AT_HWCAP: "); >> diff --git a/sysdeps/unix/sysv/linux/s390/dl-procinfo.h b/sysdeps/unix/sysv/linux/s390/dl-procinfo.h >> index 19329a335b..16739fd6cd 100644 >> --- a/sysdeps/unix/sysv/linux/s390/dl-procinfo.h >> +++ b/sysdeps/unix/sysv/linux/s390/dl-procinfo.h >> @@ -33,7 +33,7 @@ _dl_procinfo (unsigned int type, unsigned long int word) >> int i; >> >> /* Fallback to unknown output mechanism. */ >> - if (type == AT_HWCAP2) >> + if (type != AT_HWCAP) > > OK. > >> return -1; >> >> _dl_printf ("AT_HWCAP: "); > > OK, verified sysdeps/unix/sysv/linux/arm/dl-procinfo.h uses a switch > statement that handles this correctly. > > OK, verified sysdeps/powerpc/dl-procinfo.h uses a switch statement that > handles this correctly. >