This is an archive of the discontinued LLVM Phabricator instance.

[ELF] - Reorder local symbols.
ClosedPublic

Authored by grimar on Apr 5 2018, 8:10 AM.

Details

Summary

This fixes PR36716 (https://bugs.llvm.org/show_bug.cgi?id=36716),

Patch sorts local symbols to match the
following order: file1, local1, hidden1, file2, local2, hidden2 ...

Diff Detail

Event Timeline

grimar created this revision.Apr 5 2018, 8:10 AM
grimar added a reviewer: pcc.Apr 5 2018, 8:11 AM
grimar updated this revision to Diff 141160.Apr 5 2018, 8:15 AM
  • Finished unfinished comment in the test case.
grimar updated this revision to Diff 141622.Apr 9 2018, 5:12 AM
grimar edited the summary of this revision. (Show Details)
  • Rebased.
  • Simplified.
ruiu added inline comments.Apr 9 2018, 11:10 AM
ELF/SyntheticSections.cpp
1571

How does this guarantee that a local symbol group for some file always starts with a STT_FILE symbol?

espindola added inline comments.Apr 9 2018, 2:17 PM
ELF/SyntheticSections.cpp
1571

It is not obvious and needs a comment, but it should work:

We first add local symbols to the symbol table in copyLocalSymbols. Each STT_FILE in the original .o will precede other local symbols.

The hidden symbols are added in finalizeSections, so they are also after STT_FILE.

Finally this uses a stable_sort, so the above properties remain.

One thing that is not clear is where do we put STT_SECTION created by addSectionSymbols.

Also, could we merge the partition and the sort?

ruiu added a comment.Apr 9 2018, 2:21 PM

Also, could we merge the partition and the sort?

I thought the same thing, but I guess separating partition and sort is faster than merging them into one sort function call. Partition is O(n) and sort is O(n log n). So, IIUC, partitioning into local and global symbols first and then sorting only the local part should be faster than sorting the entire vector.

Also, could we merge the partition and the sort?

I thought the same thing, but I guess separating partition and sort is faster than merging them into one sort function call. Partition is O(n) and sort is O(n log n). So, IIUC, partitioning into local and global symbols first and then sorting only the local part should be faster than sorting the entire vector.

Sounds good. Lets keep both for now.

Also, could we merge the partition and the sort?

I thought the same thing, but I guess separating partition and sort is faster than merging them into one sort function call. Partition is O(n) and sort is O(n log n). So, IIUC, partitioning into local and global symbols first and then sorting only the local part should be faster than sorting the entire vector.

Sounds good. Lets keep both for now.

And it also makes final code simpler IMO. We need to set Info to the number of locals and it is simple to do with partitioning.

grimar updated this revision to Diff 141823.Apr 10 2018, 4:21 AM
  • Addressed review comments.
ELF/SyntheticSections.cpp
1571

One thing that is not clear is where do we put STT_SECTION created by addSectionSymbols.

We create them before hidden symbols, hence they are placed before them. I updated the test case.

Should we place them right after the STT_FILE? It seems a bit nicer,
though they are created only for --emit-relocs and -r, so I am not sure we should care.

espindola accepted this revision.Apr 10 2018, 2:29 PM

LGTM

I think this is fine. It is a bit funny that we count the section symbols as being part of the first file that had that section, but that is unlikely to be a problem.

This revision is now accepted and ready to land.Apr 10 2018, 2:29 PM

LGTM

I think this is fine. It is a bit funny that we count the section symbols as being part of the first file that had that section, but that is unlikely to be a problem.

We could just stop passing the file when we create them in addSectionSymbols probably.

This revision was automatically updated to reflect the committed changes.