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

[Tristan Gingold <gingold at adacore dot com>]

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.

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.

But there was no tests, so this is not a regression for mmo.

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

Regards,
Tristan.

> 
> 	* binutils-all/nm.exp: Xfail test for nm --size-sort for
> 	mmix-knuth-mmixware.
> 
> Index: nm.exp
> ===================================================================
> RCS file: /cvs/src/src/binutils/testsuite/binutils-all/nm.exp,v
> retrieving revision 1.7
> diff -p -u -C 7 -r1.7 nm.exp
> cvs diff: conflicting specifications of output style
> *** nm.exp	19 Aug 2013 15:08:52 -0000	1.7
> --- nm.exp	20 Aug 2013 01:16:57 -0000
> *************** if [is_remote host] {
> *** 175,188 ****
> --- 175,192 ----
> 
>  # This test does not work correctly on ECOFF targets, because ECOFF
>  # stores most symbols twice, which messes up the nm output.
>  setup_xfail "alpha*-*-osf*" "alpha*-*-netware*"
>  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"
> 
>  if [regexp $want $got] then {
>      pass "nm --size-sort"
>  } else {
> 
> brgds, H-P