This is an archive of the discontinued LLVM Phabricator instance.

[ELF] Define symbols "_end" and "end" if referenced.
ClosedPublic

Authored by ikudrin on Nov 19 2015, 10:45 AM.

Diff Detail

Repository
rL LLVM

Event Timeline

ikudrin updated this revision to Diff 40678.Nov 19 2015, 10:45 AM
ikudrin retitled this revision from to [ELF] Define symbols "_end" and "end" is referenced..
ikudrin updated this object.
ikudrin added reviewers: ruiu, davide.
ikudrin added a project: lld.
ikudrin added a subscriber: llvm-commits.
davide accepted this revision.Nov 19 2015, 10:48 AM
davide edited edge metadata.

This one looks fine, thanks. Testcases can be shrunk a little bit, e.g. you don't need to check the flags of BSS, that's covered somewhere else.

This revision is now accepted and ready to land.Nov 19 2015, 10:48 AM
ruiu added inline comments.Nov 19 2015, 10:57 AM
ELF/Writer.cpp
629–630 ↗(On Diff #40678)

I don't think it is unfortunate but naturally needed. Any logic to bin sections into segments affects the end position of .bss segment. It cannot be predicted here. (Otherwise we can construct segments right here instead of later.) I'd remove "Unfortunately."

633 ↗(On Diff #40678)

Move this to initSymbols in Symbols.cpp.

640 ↗(On Diff #40678)

It is a little bit alarming to use dyn_cast_or_null without using its return value, but I don't think we have isa_or_null. I'd probably write this way.

if (SymbolBody *B = Symtab.find("end"))
  if (isa<Undefined<ELFT>>(B))
    Symtab.addAbsoluteSym...
641 ↗(On Diff #40678)

So we define two symbols, end and _end, but we use only one SymbolBody. Cool.

emaste added a subscriber: emaste.Nov 19 2015, 11:16 AM

Testcases can be shrunk a little bit, e.g. you don't need to check the flags of BSS, that's covered somewhere else.

It's the only way I know to reach the lines like "Address:" and "Size:" and be sure that they are from the desired section.

davide added inline comments.Nov 19 2015, 11:25 AM
test/ELF/end-update.s
10 ↗(On Diff #40678)

This can be removed.

test/ELF/end.s
12 ↗(On Diff #40678)

This can be removed. Same for all the others (I think)

13 ↗(On Diff #40678)

This can be removed.

davide added a subscriber: davide.Nov 19 2015, 11:26 AM

hmm, for some reason phabricator doesn't put my comment at the correct
line. When I say this can be removed I mean "type"

e.g.

+// DEFAULT-NEXT: Type: SHT_NOBITS

ruiu edited edge metadata.Nov 19 2015, 11:31 AM

For reference, please add "Implements http://llvm.org/pr25528" to
the commit message.

ikudrin updated this revision to Diff 40685.Nov 19 2015, 11:45 AM
ikudrin retitled this revision from [ELF] Define symbols "_end" and "end" is referenced. to [ELF] Define symbols "_end" and "end" if referenced..
ikudrin updated this object.
ikudrin edited edge metadata.
  • Removed unnecessary flags from tests.
  • Moved initialization of the End variable to the right place.
  • Updated conditional statement.
  • Corrected comments.
ikudrin added inline comments.Nov 19 2015, 11:48 AM
ELF/Writer.cpp
641 ↗(On Diff #40685)

Not exactly. They only share the content.

ruiu accepted this revision.Nov 19 2015, 12:14 PM
ruiu edited edge metadata.

LGTM with nits.

ELF/Symbols.cpp
90 ↗(On Diff #40685)

Sort.

ELF/Writer.cpp
899 ↗(On Diff #40685)

them -> they

I want to mention that gold keeps at least _end even if not referenced. I'm not sure what's the rationale behind that but something you may want to keep in mind.

I want to mention that gold keeps at least _end even if not referenced. I'm not sure what's the rationale behind that but something you may want to keep in mind.

Let's wait until the real case emerges. As far as I know, we don't expose symbols unless we know the case where they are required. For example, we don't add a well-known symbol "_GLOBAL_OFFSET_TABLE_", just use the corresponding value during calculation of relocations instead.

ELF/Writer.cpp
899 ↗(On Diff #40685)

Oh, thanks!

This revision was automatically updated to reflect the committed changes.