From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.2 (2018-09-13) on dcvr.yhbt.net X-Spam-Level: X-Spam-ASN: AS31976 209.132.180.0/23 X-Spam-Status: No, score=-3.3 required=3.0 tests=AWL,BAD_ENC_HEADER,BAYES_00, BODY_8BITS,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_EF, HEADER_FROM_DIFFERENT_DOMAINS,MAILING_LIST_MULTI,RCVD_IN_DNSWL_MED, SPF_HELO_PASS,SPF_PASS shortcircuit=no autolearn=ham autolearn_force=no version=3.4.2 Received: from sourceware.org (server1.sourceware.org [209.132.180.131]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by dcvr.yhbt.net (Postfix) with ESMTPS id B6DFB20248 for ; Tue, 12 Mar 2019 15:06:59 +0000 (UTC) DomainKey-Signature: a=rsa-sha1; c=nofws; d=sourceware.org; h=list-id :list-unsubscribe:list-subscribe:list-archive:list-post :list-help:sender:subject:to:references:from:date:mime-version :in-reply-to:content-type:message-id; q=dns; s=default; b=NSQ2+Z gTSMHAcUKyI+KkwU4TlQ8tBG5VSrLX2rMmVndbAwsXnNNfpoWFDZgncwlYF0UnIt IyQXVyyhvGou9tuWA+qKL3xuvydTdrMgMH2YwS6saY2eem6JYZ4av2V7oibT+Dxg PKRJoACwQ+kCh1qmrVHe4Xh5z4s1kIrqld8II= DKIM-Signature: v=1; a=rsa-sha1; c=relaxed; d=sourceware.org; h=list-id :list-unsubscribe:list-subscribe:list-archive:list-post :list-help:sender:subject:to:references:from:date:mime-version :in-reply-to:content-type:message-id; s=default; bh=RTLxNXvy4q1N uhwO3/yOZ1ihwps=; b=vI/QGoRNuji4q1UzyULGXKlzq0VOxZMWcFXC5UHAE/vI dE2c1SoydnThROIn5W3z99UafTzFCMeQWetxNcdLB2kZftOtpTYd2FMr4pApU+0c GZsv5gLO6m8YIluv1sqpI28V/V7SVRe3Nz+fOS0vTvOr18ZQ3rRgQxf6FDVSrxo= 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 Received: (qmail 86389 invoked by uid 89); 12 Mar 2019 15:06:56 -0000 Authentication-Results: sourceware.org; auth=none 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: Content-Type: multipart/mixed; boundary="------------FB463E280B30FD4487B466EF" 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: This is a multi-part message in MIME format. --------------FB463E280B30FD4487B466EF Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 8bit 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. > --------------FB463E280B30FD4487B466EF Content-Type: text/x-patch; name="20190312_ldshowauxv.patch" Content-Transfer-Encoding: 7bit Content-Disposition: attachment; filename="20190312_ldshowauxv.patch" 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: "); --------------FB463E280B30FD4487B466EF--