This is an archive of the discontinued LLVM Phabricator instance.

Split Undefined and UndefinedElf
ClosedPublic

Authored by rafael on Dec 21 2015, 1:20 PM.

Details

Reviewers
ruiu
davide
grimar
Summary

I am working on adding LTO support to the new ELF lld.

In order to do that, it will be necessary to represent defined and undefined symbols that are not from ELF files. One way to do it is to change the symbol hierarchy to look like

Defined : SymbolBody
Undefined : SymbolBody

DefinedElf<ELFT> : Defined
UndefinedElf<ELFT> : Undefined

Another option would be to use bogus Elf_Sym, but I think that is getting a bit too hackish.

The attached patch does the Undefined/UndefinedElf. Split. The next one will do the Defined/DefinedElf split.

Diff Detail

Event Timeline

rafael updated this revision to Diff 43396.Dec 21 2015, 1:20 PM
rafael retitled this revision from to Split Undefined and UndefinedElf.
rafael updated this object.
rafael added reviewers: rui314, grimar.
rafael added a subscriber: llvm-commits.
davide added a subscriber: davide.Dec 21 2015, 1:32 PM

The conversion seems pretty mechanical and it doesn't seem to complicate the code -- I think I like the idea better than using bogus Elf_Sym. Couple of nits. Thanks!

ELF/OutputSections.cpp
812

Unsorted.

ELF/Symbols.h
67

Ditto.

rafael updated this revision to Diff 43398.Dec 21 2015, 1:45 PM

Sort entries.

emaste added a subscriber: emaste.Dec 21 2015, 1:58 PM
ruiu accepted this revision.Dec 21 2015, 5:07 PM
ruiu added a reviewer: ruiu.
ruiu added a subscriber: ruiu.

LGTM with a few nits. Nice change.

ELF/OutputSections.cpp
1335

Why const auto instead of auto? (It's not wrong but looks like excessive.)

ELF/SymbolTable.h
21

Sort.

ELF/Symbols.cpp
82

Pass CanKeepUndefined instead of false.

124

You can write

template class lld::elf2::UndefinedElf<ELF...>;
ELF/Symbols.h
133–136

Thank you for removing this FIXME.

This revision is now accepted and ready to land.Dec 21 2015, 5:07 PM
ruiu removed a reviewer: rui314.Dec 21 2015, 5:07 PM
grimar accepted this revision.Dec 22 2015, 1:04 AM
grimar edited edge metadata.

LGTM with a few nits.

ELF/OutputSections.cpp
1333

I am not sure if this place is clang formatted (looks like not).

ELF/Symbols.cpp
82

This false is about IsTls I think.
Thats the problem of these constructors - them are very similiar, but isTls/CanKeepUndefined differs what confuses. May be it is worth to keep both flags here ? One more excess argument is not good either though.

davide accepted this revision.Dec 22 2015, 2:32 AM
davide added a reviewer: davide.

Forgot to mark this as accepted yesterday FWIW.

rafael closed this revision.Dec 22 2015, 3:04 PM