Re: Committed: fix "[Patch] Fix nm --size-sort on non-elf targets"

[Tristan Gingold <gingold at adacore dot com>]

On Aug 21, 2013, at 1:41 AM, Hans-Peter Nilsson <hp@bitrange.com> wrote:

> On Tue, 20 Aug 2013, Tristan Gingold wrote:
>> On Aug 20, 2013, at 3:32 AM, Hans-Peter Nilsson <hp@bitrange.com> wrote:
>> 
>>> On Mon, 19 Aug 2013, Tristan Gingold wrote:
>>>> On Aug 10, 2013, at 11:34 AM, Alan Modra <amodra@gmail.com> wrote:
>>>>> On Fri, Jul 19, 2013 at 09:50:00AM +0200, Tristan Gingold wrote:
>>>>>> binutils/
>>>>>> 2013-07-19  Tristan Gingold  <gingold@adacore.com>
>>>>>> 
>>>>>> 	* nm.c (print_size_symbols): Directly get symbol size.
>>>>>> 
>>>>>> binutils/testsuite/
>>>>>> 2013-07-19  Tristan Gingold  <gingold@adacore.com>
>>>>>> 
>>>>>> 	* binutils-all/nm.exp: Add a test for nm --size-sort
>>>>>> 	* binutils-all/nm-elf-1.s: New file.
>>>>>> 	* binutils-all/nm-1.s: New file.
>>>>> 
>>>>> This is OK.
>>>> 
>>>> Thanks, now committed.
>>> 
>>> Didn't work on mmix-knuth-mmixware.  I assume by "Fix nm
>>> --size-sort on non-elf targets" you actually mean "Fix nm
>>> --size-sort on non-elf targets where symbols have a size
>>> attribute", else please shout.  I committed the following.
>> 
>> So I shout.
> 
> Rightly so, thanks.  I should have noticed that the file was
> only assembled, not linked (separate nm tests for linked files
> seem missing) and so my MMO file comments were not applicable
> per se.
> 
>> I don't know the MMO file format, but the code in sort_symbols_by_size
>> is clear: there is a special circuitry for the ELF format, symbols are
>> first sorted by address on other formats.
> 
> And the ELF source file is *required* for the ELF format as it
> doesn't fall back to checking addresses if the ELF size is
> zero, as it will be if the generic nm-1.s file is used for ELF.
> Fixed as below.

This is certainly the good fix.

>> But there was no tests, so this is not a regression for mmo.
> 
> There was no talk about regressions.  It seems there's an
> implication that you actually built and checked for
> mmix-knuth-mmixware but ignored the new failure?  Tsk tsk: you
> wrote the test.  Anyway, moving on.

I haven't checked for mmix, but as this is a new tests, failures
are expected (and such failures aren't regressions).  Anyway...

>>> Conceptually it's wrong to xfail the test for targets where it's
>>> (by design) not supposed to work; it should instead be skipped.
>>> Though, adding skip-machinery would just clutter up the code for
>>> no actual benefit, at least when those targets that should be
>>> skipped is an insignificant number of one, IMHO.  To the point,
>>> nm --size-sort should actually be made to work for those ECOFF
>>> targets, right?
>> 
>> Well, according to the comment, nm fails on ECOFF because symbols
>> appear twice.  Different story.
> 
> I read the comment and purposely included it in the diff.  I
> guess by different story you mean that the test needs to be
> tweaked and that the xfail is there as a stopgap; that nm
> --size-sort *should* work.
> 
> ...but, looking closer, the comment seems out of date, referring
> to obsolete targets (mips-ecoff and alpha-dec-osf not supported
> by gas and ld, for example) and that the comment is a copy-paste
> of doubtful applicability from the nm -P tests just above.
> 
> After finding a supported ecoff target (not obvious), I built
> and tested alpha-unknown-linux-gnuecoff (not xfailed).  The
> --size-sort test fails there, but I don't see any trace of nm
> (or objdump) displaying two symbols.  Actually, the failure
> seems to be that the symbol values are fouled up with
> --size-sort compared to no nm options or -n.  Unknown if that's
> related to your changes.  Anyway, having gone that far I went
> the rest of that leg and entered PR15869.  Maybe it really is
> because there are two symbols internally somewhere.
> 
> Anyway I committed the following.

Thank you for the head up.

Tristan.

> 
> 	* binutils-all/nm.exp: Revert last change.  Instead use nm-elf-1.s
> 	for mmix-knuth-mmixware.
> 
> Index: binutils-all/nm.exp
> ===================================================================
> RCS file: /cvs/src/src/binutils/testsuite/binutils-all/nm.exp,v
> retrieving revision 1.8
> diff -p -u -r1.8 nm.exp
> --- binutils-all/nm.exp	20 Aug 2013 01:29:44 -0000	1.8
> +++ binutils-all/nm.exp	20 Aug 2013 22:47:15 -0000
> @@ -157,7 +157,10 @@ if [regexp $want $got] then {
> 
> # Test nm --size-sort
> 
> -if [is_elf_format] {
> +# The target exceptions here are intended for targets that have ELF as
> +# an intermediate format or otherwise require the ELF-variant syntax
> +# for proper size annotation.
> +if {[is_elf_format] || [istarget "mmix-knuth-mmixware"]} {
>     set nm_1_src "nm-elf-1.s"
> } else {
>     set nm_1_src "nm-1.s"
> @@ -179,10 +182,6 @@ setup_xfail "alpha*-*-osf*" "alpha*-*-ne
> setup_xfail "mips*-*-ultrix*" "mips*-*-ecoff*" "mips*-*-irix4*"
> setup_xfail "mips*-*-riscos*" "mips*-*-sysv3*" "mips*-sony-bsd*"
> 
> -# It also doesn't work with the MMO object format, because a MMO
> -# symbol has no size information.
> -setup_xfail "mmix-knuth-mmixware"
> -
> set got [binutils_run $NM "$NMFLAGS --size-sort $tempfile"]
> 
> set want "0*4 T text_symbol3.*0*8 T text_symbol2.*0*c T text_symbol1"
> 
> brgds, H-P