From patchwork Tue Mar 12 15:06:41 2019 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Patchwork-Submitter: Stefan Liebler X-Patchwork-Id: 31822 Received: (qmail 86397 invoked by alias); 12 Mar 2019 15:06:57 -0000 Mailing-List: contact libc-alpha-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Unsubscribe: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: libc-alpha-owner@sourceware.org Delivered-To: mailing list libc-alpha@sourceware.org Received: (qmail 86389 invoked by uid 89); 12 Mar 2019 15:06:56 -0000 Authentication-Results: sourceware.org; auth=none X-Spam-SWARE-Status: =?ISO-8859-1?Q?No, score=-26.5 required=5.0 tests=BAYES_00, GIT_PATCH_0, GIT_PATCH_1, GIT_PATCH_2, GIT_PATCH_3, KAM_LOTSOFHASH, KHOP_DYNAMIC, RCVD_IN_DNSWL_LOW, SPF_PASS autolearn=ham version=3.3.1 spammy=if, or, or=c2, is=c2?= X-HELO: mx0a-001b2d01.pphosted.com Subject: Re: [PATCH] Fix output of LD_SHOW_AUXV=1. To: "Carlos O'Donell" , GNU C Library References: From: Stefan Liebler Date: Tue, 12 Mar 2019 16:06:41 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101 Thunderbird/60.5.0 MIME-Version: 1.0 In-Reply-To: x-cbid: 19031215-0012-0000-0000-00000301B2A4 X-IBM-AV-DETECTION: SAVI=unused REMOTE=unused XFE=unused x-cbparentid: 19031215-0013-0000-0000-00002138D486 Message-Id: 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. > Reviewed-by: Carlos O'Donell commit 988376a08d6d36ea321203b9c039db4b266609ae 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 is removing the condition and let _dl_procinfo decide if an entry is printed in a platform specific or generic way. This patch also adjusts all _dl_procinfo implementations which assumed that they are only called for AT_HWCAP or AT_HWCAP2. They are now just returning a non-zero-value for entries which are not handled platform specifc. ChangeLog: * elf/dl-sysdep.c (_dl_show_auxv): Remove condition and always call _dl_procinfo. * 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..5d19b100b2 100644 --- a/elf/dl-sysdep.c +++ b/elf/dl-sysdep.c @@ -328,14 +328,9 @@ _dl_show_auxv (void) assert (AT_NULL == 0); 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) - { - /* These are handled in a special way per platform. */ - if (_dl_procinfo (av->a_type, av->a_un.a_val) == 0) - continue; - } + /* Some entries are handled in a special way per platform. */ + if (_dl_procinfo (av->a_type, av->a_un.a_val) == 0) + continue; if (idx < sizeof (auxvars) / sizeof (auxvars[0]) && auxvars[idx].form != unknown) 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) 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) 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) return -1; _dl_printf ("AT_HWCAP: ");