Page MenuHomePhabricator

[ELF] - Get rid of postThunkContents().
ClosedPublic

Authored by grimar on Jul 19 2018, 8:05 AM.

Details

Summary

It turns out that postThunkContents() is only used for
sorting symbols in .symtab.

Though we can instead move the logic to finalizeContents,
postpone calling it and then get rid of postThunkContents() completely.

Diff Detail

Repository
rL LLVM

Event Timeline

grimar created this revision.Jul 19 2018, 8:05 AM

What about doing this change?

Looks reasonable to me. I've made a suggestion that might simplify a bit further, I've not tried or tested it though.

Apologies for the late response; I've had this stuck in a draft state for a while, must have forgotten to press submit.

ELF/SyntheticSections.cpp
1822 ↗(On Diff #156274)

If the only line of code that SymbolTableSection (SHT_SYMTAB) and SymbolTableBaseSection (SHT_DYNSYM) share is getParent()->Link ... Would it be possible to move sortSymTabSymbols into SymbolTableSection::finalizeContents() add the getParent()->Link and let the virtual call mechanism replace the test?

Thanks for the feedback, Peter!

ELF/SyntheticSections.cpp
1822 ↗(On Diff #156274)

I am not sure I follow here.

SHT_SYMTAB and SHT_DYNSYM are both SymbolTableSection<ELFT>:

InX::SymTab = make<SymbolTableSection<ELFT>>(*InX::StrTab);
...
InX::DynSymTab = make<SymbolTableSection<ELFT>>(*InX::DynStrTab);

So I think the virtual call mechanism will not be helpful here?
(Since we do not have different classes for them atm. And need them both to be SymbolTableSection for writeTo I think).

peter.smith accepted this revision.Jul 31 2018, 6:01 AM

LGTM. Probably worth waiting a bit to see if Rui has any opinions.

ELF/SyntheticSections.cpp
1822 ↗(On Diff #156274)

My apologies, I think I must have assumed that the DynSym and the SymTab where of different types. Please ignore the suggestion.

This revision is now accepted and ready to land.Jul 31 2018, 6:01 AM
grimar added a comment.Aug 7 2018, 5:35 AM

Ping. Rui?

grimar added a comment.Aug 9 2018, 6:35 AM

Since there are no objections, I am going to commit this tomorrow.

This revision was automatically updated to reflect the committed changes.