This is an archive of the discontinued LLVM Phabricator instance.

[ELF] Generalize ELF symbol type handling
ClosedPublic

Authored by davide on Mar 3 2016, 3:27 PM.

Details

Summary

SymbolBody constructor and friends take isFunc and isTLS boolean arguments. ELF symbols have already a type so than be easily passed down. This makes code a little bit more readable, I think.
The next patch in this series will remove isTLS.

Diff Detail

Event Timeline

davide updated this revision to Diff 49783.Mar 3 2016, 3:27 PM
davide retitled this revision from to [ELF] Generalize ELF symbol type handling.
davide updated this object.
davide added reviewers: rafael, grimar, ruiu.
davide added subscribers: emaste, llvm-commits.
davide added a comment.Mar 3 2016, 3:31 PM

Note (to future self and others): this minor yak was motivated by a bug I found.
Some symbols are not emitted in the symbol table with the appropriate type, e.g. STT_OBJECT.
The current code logic would require passing another bool isObject around. Up to two additional argument, this stretching exercise was a little bit goofy but still acceptable, but with more types to support, is just too much, IMHO.

davide removed a subscriber: emaste.
ruiu edited edge metadata.Mar 3 2016, 3:42 PM

This looks good to me, but do you want to do isTLS in the same patch? This patch itself does not reduce the number of parameters, so I think this patch more sense if you do both at once.

davide added a comment.Mar 3 2016, 3:45 PM
In D17871#367706, @ruiu wrote:

This looks good to me, but do you want to do isTLS in the same patch? This patch itself does not reduce the number of parameters, so I think this patch more sense if you do both at once.

Sure, why not.

davide updated this revision to Diff 49786.Mar 3 2016, 5:01 PM
davide edited edge metadata.

Addressed Rui's comments. Removed isTls sooner rather than later.

ruiu accepted this revision.Mar 3 2016, 5:38 PM
ruiu edited edge metadata.

LGTM

This revision is now accepted and ready to land.Mar 3 2016, 5:38 PM
emaste accepted this revision.Mar 3 2016, 5:53 PM
emaste edited edge metadata.
This revision was automatically updated to reflect the committed changes.
grimar edited edge metadata.Mar 4 2016, 1:59 AM

Hi Davide,

Change itself looks good for me, but I have few thoughts about possible implementation and prepared a little patch here: http://reviews.llvm.org/D17879.