From: Stefan Liebler <stli@linux.ibm.com>
To: "Carlos O'Donell" <carlos@redhat.com>,
GNU C Library <libc-alpha@sourceware.org>
Subject: Re: [PATCH] Fix output of LD_SHOW_AUXV=1.
Date: Tue, 12 Mar 2019 16:06:41 +0100 [thread overview]
Message-ID: <d1bd871f-c327-6ec1-0795-55129ac4901f@linux.ibm.com> (raw)
In-Reply-To: <b5420a20-30f8-effc-799b-0661d2c0a3f9@redhat.com>
[-- Attachment #1: Type: text/plain, Size: 6579 bytes --]
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 <carlos@redhat.com>
>
>> 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 <stli@linux.ibm.com>
>> 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.
>
[-- Attachment #2: 20190312_ldshowauxv.patch --]
[-- Type: text/x-patch, Size: 3336 bytes --]
commit 988376a08d6d36ea321203b9c039db4b266609ae
Author: Stefan Liebler <stli@linux.ibm.com>
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: ");
next prev parent reply other threads:[~2019-03-12 15:06 UTC|newest]
Thread overview: 5+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-03-08 12:52 [PATCH] Fix output of LD_SHOW_AUXV=1 Stefan Liebler
2019-03-08 14:51 ` Carlos O'Donell
2019-03-12 15:06 ` Stefan Liebler [this message]
2019-03-12 20:11 ` Carlos O'Donell
2019-03-13 9:54 ` [2.29 COMMITTED] " Stefan Liebler
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
List information: https://www.gnu.org/software/libc/involved.html
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=d1bd871f-c327-6ec1-0795-55129ac4901f@linux.ibm.com \
--to=stli@linux.ibm.com \
--cc=carlos@redhat.com \
--cc=libc-alpha@sourceware.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).