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

[Hans-Peter Nilsson <hp at bitrange dot com>]

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.

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

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

	* 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