[PATCH] Fix output of LD_SHOW_AUXV=1.

Tue Mar 12 15:06:00 GMT 2019

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.
> 

-------------- next part --------------
A non-text attachment was scrubbed...
Name: 20190312_ldshowauxv.patch
Type: text/x-patch
Size: 3336 bytes
Desc: not available
URL: <http://sourceware.org/pipermail/libc-alpha/attachments/20190312/8d39eea6/attachment.bin>