Bug 6430

Created attachment 2707 [details]
Patch for correct --sort-common.

Either the documentation or the implementation is incorrect.  In the latter
case, this patch fixes the implementation.

Can you add a testcase patch?

Sure, but I'll be hitting the road this week.  Later on then.

Linker manual says

`--sort-common'
     This option tells `ld' to sort the common symbols by size when it
     places them in the appropriate output sections.  First come all
     the one byte symbols, then all the two byte, then all the four
     byte, and then everything else.  This is to prevent gaps between
     symbols due to alignment constraints.

To prevent gaps between common symbols with different alignments,
shouldn't we sort common symbols by their sizes in descending order,
assuming small sizes need small alignments?

You are correct in that this way a gap would be avoided when the size changes. 
But given that all like-sized symbols are laid out back to back, this only
happens when the size changes.

So, either the manual must be changed or the code.  I, for one, would rather
have the code changed...

Subject: Re:  --sort-common Not Implemented Per Documentation

Hi Guys,

> You are correct in that this way a gap would be avoided when the size changes. 
> But given that all like-sized symbols are laid out back to back, this only
> happens when the size changes.
> 
> So, either the manual must be changed or the code.  I, for one, would rather
> have the code changed...

But this might break existing applications which rely upon the current 
behaviour.  Better I feel would be to fix the documentation and then 
provide a extended version of the --sort-common switch which takes an 
argument which specifies the direction of the sorting.  eg:

   --sort-common=biggest-first  [this would be the default]
   --sort-common=smallest-first

Cheers
   Nick

To prevent gaps between symbols due to alignment constraints, shouldn't
we sort common symbols by their alignments?

As a matter of fact, the current implementation actually sorts by alignment.

I like Nick's suggestion, only I prefer terser wording, such as "ascending" or
"descending".  But that's me.

(In reply to comment #8)
> As a matter of fact, the current implementation actually sorts by alignment.
> 
> I like Nick's suggestion, only I prefer terser wording, such as "ascending" or
> "descending".  But that's me.

In that case, we need to fix the typo in the linker manual first: `--sort-common'
should be sorted by alignment, not by size.

Created attachment 2720 [details]
Change the documentation according to the code.

Hi Evandro,

  Right - I have checked in your documentation patch.  (Sorry about the delay in
getting to this).

  Do you have a patch to implement the new user-choice-of-sorting algorithm, or
is there really no need for this now that the documentation is correct ?

Cheers
  Nick

I think that the choice should be available.  I'll craft it up in a moment.

Created attachment 2750 [details]
Patch providing the user with a choice.

Please, review this patch that allows the user to choose between ascending or
descending order, assuming descending order as currently if nothing is
specified.

Created attachment 2769 [details]
Add test of --sort-common command line option

Hi Evandro,

  Thanks for the patch.  I have applied it together with the changelog entry
below.  Plus I added a line about it to the ld/NEWS file, and I created a linker
testsuite test for it so that we can be sure that the option continues to work.

  Interestingly the new test fails for the cr16-elf and crx-elf ports, but I
could not determine why this was happening.

Cheers
  Nick

ld/ChangeLog
2008-05-31  Evandro Menezes  <evandro@yahoo.com>

	PR ld/6430
	* ld.h (enum sort_order): New.
	* ldlang.c (lang_check: Fix comment.
	(lang_common): Sort commons in ascending or descending order.
	(lang_one_common): Likewise.
	* lexsup.c (ld_options): Have --sort-common take an option
	argument.
	(parse_args): Handle argument to --sort-common.
	* ld.texinfo (--sort-common): Document new optional argument.
	* NEWS: Mention new feature.


ld/testsuite/ChangeLog
2008-05-31  Nick Clifton  <nickc@redhat.com>

	PR ld/6430
	* ld-elfcomm/elfcomm.exp (test_sort_common): Test the
	ascending/descending argument to the --sort-common command line
	option.
	* ld-elfcomm/sort-common.s: New file.