This is an archive of the discontinued LLVM Phabricator instance.

[ELF] - Do not handle ELF and program header as dummy sections
ClosedPublic

Authored by grimar on Apr 3 2016, 7:49 AM.

Details

Summary

During review of D18499 was requested to improve way how
we deal with dummies sections.
This patch does that. Now they are not part of OutputSections list,
but still keeping them as sections helps to simplify the code.

Diff Detail

Repository
rL LLVM

Event Timeline

grimar updated this revision to Diff 52502.Apr 3 2016, 7:49 AM
grimar retitled this revision from to [ELF] - Improved consept of dummies sections..
grimar updated this object.
grimar added reviewers: ruiu, rafael.
grimar added subscribers: llvm-commits, grimar.
ruiu edited edge metadata.Apr 3 2016, 12:00 PM

Generally looks good. A few nits.

ELF/Writer.cpp
103 ↗(On Diff #52502)

Why +1?

1346 ↗(On Diff #52502)

This function needs a comment.

1347–1349 ↗(On Diff #52502)

"Off" is always zero, so please do this.

Out<ELFT>::ElfHeader->setVA(Target->getVAStart());
Out<ELFT>::ElfHeader->setFileOffset(0);
ruiu added a comment.Apr 3 2016, 12:02 PM

Please change the title of this patch to something like "Do not handle ELF and program header as dummy sections", since what you are doing here is not improving the dummy sections but instead removing the concept of that.

grimar updated this revision to Diff 52510.Apr 3 2016, 1:42 PM
grimar retitled this revision from [ELF] - Improved consept of dummies sections. to [ELF] - Do not handle ELF and program header as dummy sections.
grimar edited edge metadata.
grimar marked 2 inline comments as done.
  • Addressed Rui's review comments.
ELF/Writer.cpp
103 ↗(On Diff #52502)

Because we have one "dummy-zero" section:

Section Headers:
  [Nr] Name              Type             Address           Offset
       Size              EntSize          Flags  Link  Info  Align
  [ 0]                   NULL             0000000000000000  00000000
       0000000000000000  0000000000000000           0     0     0
1346 ↗(On Diff #52502)

Done.

1347–1349 ↗(On Diff #52502)

Done.

grimar updated this revision to Diff 52511.Apr 3 2016, 1:50 PM
  • Fixed incompletely addressed place.
ruiu added inline comments.Apr 4 2016, 1:24 PM
ELF/Writer.cpp
103 ↗(On Diff #52511)

It's a bit weird to count the first null entry of the section table as a section. You can just remove this function, no?

grimar updated this revision to Diff 52657.Apr 4 2016, 11:28 PM
  • Addressed review comments.
ELF/Writer.cpp
103 ↗(On Diff #52511)

Removed, but there was 2 places where it was called, I put (OutputSections.size() + 1) there instead:

EHdr->e_shnum = OutputSections.size() + 1;
FileSize = SectionHeaderOff + (OutputSections.size() + 1) * sizeof(Elf_Shdr);
ruiu accepted this revision.Apr 5 2016, 7:51 AM
ruiu edited edge metadata.

LGTM

This revision is now accepted and ready to land.Apr 5 2016, 7:51 AM
emaste added a subscriber: emaste.Apr 5 2016, 12:24 PM
This revision was automatically updated to reflect the committed changes.