Bug 6430 - --sort-common Not Implemented Per Documentation
Summary: --sort-common Not Implemented Per Documentation
Status: RESOLVED FIXED
Alias: None
Product: binutils
Classification: Unclassified
Component: ld (show other bugs)
Version: unspecified
: P2 normal
Target Milestone: ---
Assignee: unassigned
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2008-04-17 23:03 UTC by Evandro
Modified: 2008-05-31 16:34 UTC (History)
2 users (show)

See Also:
Host:
Target:
Build:
Last reconfirmed:


Attachments
Patch for correct --sort-common. (350 bytes, patch)
2008-04-17 23:04 UTC, Evandro
Details | Diff
Change the documentation according to the code. (446 bytes, patch)
2008-05-01 23:23 UTC, Evandro
Details | Diff
Patch providing the user with a choice. (1.91 KB, patch)
2008-05-21 21:35 UTC, Evandro
Details | Diff
Add test of --sort-common command line option (1.04 KB, patch)
2008-05-31 16:31 UTC, Nick Clifton
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Evandro 2008-04-17 23:03:49 UTC
It seems that although the documentation states that --sort-common will sort in
roughly ascending order by size, the implementation does so in descending order.
Comment 1 Evandro 2008-04-17 23:04:57 UTC
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.
Comment 2 H.J. Lu 2008-04-20 01:30:54 UTC
Can you add a testcase patch?
Comment 3 Evandro 2008-04-20 18:21:31 UTC
Sure, but I'll be hitting the road this week.  Later on then.
Comment 4 H.J. Lu 2008-04-21 21:20:14 UTC
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?
Comment 5 Evandro 2008-04-22 01:05:06 UTC
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...
Comment 6 Nick Clifton 2008-04-22 14:45:28 UTC
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


Comment 7 H.J. Lu 2008-04-22 16:06:44 UTC
To prevent gaps between symbols due to alignment constraints, shouldn't
we sort common symbols by their alignments?
Comment 8 Evandro 2008-04-22 16:56:55 UTC
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.
Comment 9 H.J. Lu 2008-04-22 17:17:54 UTC
(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.
Comment 10 Evandro 2008-05-01 23:23:17 UTC
Created attachment 2720 [details]
Change the documentation according to the code.
Comment 11 Nick Clifton 2008-05-21 11:00:44 UTC
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
Comment 12 Evandro 2008-05-21 14:01:22 UTC
I think that the choice should be available.  I'll craft it up in a moment.
Comment 13 Evandro 2008-05-21 21:35:09 UTC
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.
Comment 14 Nick Clifton 2008-05-31 16:31:59 UTC
Created attachment 2769 [details]
Add test of --sort-common command line option
Comment 15 Nick Clifton 2008-05-31 16:34:41 UTC
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.