This is an archive of the discontinued LLVM Phabricator instance.

[elf2] Combine adjacent compatible OutputSections in PT_LOADs.
ClosedPublic

Authored by Bigcheese on Sep 10 2015, 5:12 PM.

Details

Reviewers
rafael

Diff Detail

Event Timeline

Bigcheese updated this revision to Diff 34516.Sep 10 2015, 5:12 PM
Bigcheese retitled this revision from to [elf2] Combine adjacent compatible OutputSections in PT_LOADs..
Bigcheese updated this object.
Bigcheese added a reviewer: rafael.
Bigcheese added a subscriber: llvm-commits.
rafael edited edge metadata.Sep 11 2015, 6:17 AM
rafael added a subscriber: rafael.

This doesn't build:

/home/espindola/llvm/llvm/tools/lld/ELF/Writer.cpp:289:9: error:
member initializer 'ProgramHeader' does not name a non-static data
member or base class

: ProgramHeader(PT_DYNAMIC,
  ^~~~~~~~~~~~~~~~~~~~~~~~~

/home/espindola/llvm/llvm/tools/lld/ELF/Writer.cpp:291:5: error: use
of undeclared identifier 'Header'

Header.p_offset = Sec->getFileOff();
^

...

Bigcheese updated this revision to Diff 34608.Sep 11 2015, 5:22 PM
Bigcheese edited edge metadata.

Rebase and fix compile issues on non-msvc.

git-clang-format the patch

+ ProgramHeader(uintX_t p_type, uintX_t p_flags) : Closed(false) {

Used

bool Closed = false;

std::unique_ptr<llvm::FileOutputBuffer> Buffer;

+

llvm::SpecificBumpPtrAllocator<OutputSection<ELFT>> CAlloc;

Why?

+ void setFromSection(OutputSectionBase<Is64Bits> &Sec) {

Set what from section?

+ // Create a PHDR for the first output section which covers the file header.
+ PHDRs.push_back(new (PAlloc) ProgramHeader<ELFT::Is64Bits>(
+ PT_LOAD, convertSectionFlagsToPHDRFlags(OutputSections[0]->getFlags())));

This means that the file and program headers are now executable. While
that is the common default for elf linkers, it is undesirable in
security sensitive contexts. For now, just keep creating the first
PT_LOAD with just PF_R. We can change that (possible with an option)
on another patch.

+ if (Sec->getSize()) {

All sh_alloc segments come before all others, so it might be
possible/cleaner to write this with

if (outputSectionHasPHDR<ELFT>(Sec)) {

And just close the last program header out of the loop, no?

I noticed that this patch doesn't change compSec. This means that we
might alternate from, for example, PF_R to PF_R|PF_W back to PF_R and
create more segments than necessary. Changing this on another patch is
preferable to changing it on this one. I just wanted to point it out.

Cheers,
Rafael

Bigcheese updated this revision to Diff 34746.Sep 14 2015, 3:48 PM
std::unique_ptr<llvm::FileOutputBuffer> Buffer;

+

llvm::SpecificBumpPtrAllocator<OutputSection<ELFT>> CAlloc;

Why?

Separating out logically related members.

+ if (Sec->getSize()) {

All sh_alloc segments come before all others, so it might be
possible/cleaner to write this with

if (outputSectionHasPHDR<ELFT>(Sec)) {

And just close the last program header out of the loop, no?

There are zero sized allocated sections (.data, .bss) in some tests that have different flags, and would create zero sized PHDRs. This ignores them.

Closing the last program header outside of the loop makes that PT_LOAD include all of the non SHF_ALLOC sections.

rafael added inline comments.Sep 14 2015, 3:53 PM
test/elf2/basic.s
159

No point in checking the numeric value.

test/elf2/basic64be.s
152

Please make the witespace changes first if needed and rebase this patch on top.

Bigcheese updated this revision to Diff 34751.Sep 14 2015, 3:54 PM

Fix the default initializer.

Bigcheese updated this revision to Diff 34754.Sep 14 2015, 4:02 PM

Remove accidental test changes.

This now needs more tests.

The main thing this patch does is put different output sections into
the same program header. Add a new test that checks for just that:

The file has two sections that cannot possibly be in the section
output section (different names for example).

The sections have the same flags, so they end up in the same program header.

Cheers,
Rafael

Bigcheese updated this revision to Diff 34831.Sep 15 2015, 1:33 PM

Add tests and rebase.

ruiu added a subscriber: ruiu.Sep 15 2015, 1:40 PM
ruiu added inline comments.
ELF/Writer.cpp
404–411

Can this just be *PHDR = Header?

Bigcheese added inline comments.Sep 15 2015, 1:54 PM
ELF/Writer.cpp
404–411

No, they are different types.

test/elf2/basic64be.s
152–176

Ah, that wasn't supposed to happen.

rafael added inline comments.Sep 15 2015, 2:18 PM
ELF/Writer.cpp
838

This creates an output section even if the section is empty.

850–870

This does not. We should to treat the first section specially. What I would suggest is to have a "ProgramHeader *LastHeader" variable and initialize it to the file header program header.

In the loop you can check if the current section is compatible with the previous one. If not, create a new one and add to LastHeader.

Note in particular that if the first section that needs a program header is read only, the original program header gets extended.

test/elf2/program-header-layout.s
3 ↗(On Diff #34831)

It doesn't fit in 80 columns?

This does not. We should to treat the first section specially. What I would suggest is to have a "ProgramHeader *LastHeader" variable and initialize it to the file header program header.

We should *not* treat ...

Bigcheese updated this revision to Diff 34843.Sep 15 2015, 2:59 PM

Don't treat the first PHDR specially.

rafael accepted this revision.Sep 16 2015, 5:13 AM
rafael edited edge metadata.

LGTM on the condition that one extra test is added showing a read only section being included in the initial PT_LOAD.

This revision is now accepted and ready to land.Sep 16 2015, 5:13 AM

And please update cmpSec in a followup patch.

Eugene.Zelenko added a subscriber: Eugene.Zelenko.

Committed in rL247925.