This is an archive of the discontinued LLVM Phabricator instance.

[ELF] - Get rid of SectionOrder array.
ClosedPublic

Authored by grimar on Apr 15 2016, 11:55 AM.

Details

Summary

SectionOrder vector was a part of LinkerScript class.

After commit of D18499 it can be removed because Locations vector
contains the same information and SectiorOrder is just a subset.

Diff Detail

Event Timeline

grimar updated this revision to Diff 53927.Apr 15 2016, 11:55 AM
grimar retitled this revision from to [ELF] - Get rid of SectionOrder array..
grimar updated this object.
grimar added reviewers: ruiu, rafael.
grimar added subscribers: llvm-commits, grimar.
grimar added inline comments.
ELF/LinkerScript.cpp
172

So compareSections() is used to sort OutputSections.
Later linkerscript uses this order in fixupLocations() to add orphans.

Therefore Locations vector is updated at the very end,
before actual address assignment.

Another variant would be to call fixupLocations() earlier, before compareSections().
Then orpan order would be script driven, but will add some code complexity as will need export of
fixupLocations() method.

I like both approach. First (this patch) for simple code, second for giving
all cards to script to control orphans order.

This patch also fixes the section indexes. We should keep OutputSections order the same as Locations order because of code that assigns them:

unsigned I = 1;
for (OutputSectionBase<ELFT> *Sec : OutputSections) {
  Sec->SectionIndex = I++;
  Sec->setSHName(Out<ELFT>::ShStrTab->addString(Sec->getName()));
}
rafael edited edge metadata.Apr 20 2016, 11:53 AM

I think this is fine, but I will let Rui have the final say on liker script stuff.

ruiu accepted this revision.Apr 20 2016, 1:35 PM
ruiu edited edge metadata.

LGTM with a nit.

ELF/LinkerScript.cpp
93

The return value will never overflow in reality, so let's use just int instead of uint32_t. Then you can report the absent key as just -1 instead of UINT32_MAX.

This revision is now accepted and ready to land.Apr 20 2016, 1:35 PM
This revision was automatically updated to reflect the committed changes.
grimar added inline comments.Apr 21 2016, 3:28 AM
ELF/LinkerScript.cpp
93

Doing that will require changes that might overcomplicate the current
code of compareSections():

uint32_t I = getSectionOrder(A);
uint32_t J = getSectionOrder(B);
if (I == (uint32_t)-1 && J == (uint32_t)-1)
  return 0;
return I < J ? -1 : 1;

For example if I has some order, but J does not then:
Since J is uint32_t (and == UINT32_MAX) then I < J will work as expected, placing A before B.

If we switch to int then I guess something like next should be written
(and it does not seem to worth that):

int I = getSectionOrder(A);
int J = getSectionOrder(B);
if (I == -1 && J == -1)
  return 0;
if (I == -1)
  return 1;
if (J == -1)
  return -1;
return I < J ? -1 : 1;

So I committed without that change for now.

ruiu added inline comments.Apr 21 2016, 12:06 PM
ELF/LinkerScript.cpp
172

Makes sense, but in that case (uint32_t)-1 is not an error code, but it actually mean the sorting order of the given section name (e.g. the lowest priority). In that case (uint32_t)-1 looks odd. Please use UINT32_MAX instead because it's what you really mean. Also it needs a comment about when UINT32_MAX returns.

Also, there's no reason to use uint32_t. I'd use int and INT_MAX.