This is an archive of the discontinued LLVM Phabricator instance.

[ELF] Don't output headers into a segment if there's no space for them
ClosedPublic

Authored by phosek on Aug 2 2017, 5:59 PM.

Details

Summary

Currently, LLD checks whether there's enough space for headers by
checking if headers fit below the address of the first allocated
section. However, that's always thue if the binary doesn't start
at zero which means that LLD always emits a segment for headers,
even if no other sections belong to that segment.

This is a problem in cases when linker script is being used with a
non-zero start address when we don't want to make the headers visible
by not leaving enough space for them. This pattern is common in
embedded programming but doesn't work in LLD.

This patch changes the behavior of LLD in case when linker script
is being to match the behavior of BFD ld and gold, which is to only
place headers into a segment when they're covered by some output
section.

Diff Detail

Repository
rL LLVM

Event Timeline

phosek created this revision.Aug 2 2017, 5:59 PM
phosek added a subscriber: mcgrathr.Aug 2 2017, 5:59 PM

The new test case exhibits the correct behavior.

grimar added a subscriber: grimar.Aug 3 2017, 1:58 AM
grimar added inline comments.Aug 18 2017, 1:21 AM
ELF/LinkerScript.cpp
725

Why do you need this change ?

736

And this one ?

I removed both and had no no any test failtures:

OutputSection *ActualFirst = nullptr;
for (OutputSection *Sec : OutputSections) {
  if (Sec->FirstInPtLoad == Out::ElfHeader) {
    ActualFirst = Sec;
    break;
  }
}
if (ActualFirst) {
  for (OutputSection *Sec : OutputSections)
    if (Sec->FirstInPtLoad == Out::ElfHeader)
      Sec->FirstInPtLoad = ActualFirst;
  FirstPTLoad->First = ActualFirst;
} else {
  Phdrs.erase(It);
}
test/ELF/linkerscript/segment-headers.s
3

This can be single line probably.
echo "SECTIONS { . = 0x2000; .text : AT(0x100000) { *(.text) } }" > %t.script

26

Do you need to check anything except first load ?

I'd like to see something like this for embedded systems as the ELF file is often not directly run on the target (PT_LOAD regions are extracted into binary or hex and burned into flash), having the ELF headers mapped in the first PT_LOAD is often detrimental. I suggest you add Rafael as a reviewer as well as he has touched this area fairly recently.

ELF/LinkerScript.cpp
704

I found the combined if/else quite hard to follow, especially trying to work out what the differences between the two were as the logic is quite similar. Would it make sense to rewrite the combined if/else as:

uint64_t Base = (Opt.HasSections) ? alignDown(Min, Config->MaxPageSize) : 0;
  if (HeaderSize <= Min - Base || Script->hasPhdrsCommands()) {
    Min = (Opt.HasSections) ? Base : alignDown(Min - HeaderSize,
                                               Config->MaxPageSize);
    Out::ElfHeader->Addr = Min;
    Out::ProgramHeaders->Addr = Min + Out::ElfHeader->Size;
    return;
  }

A comment to explain what we are trying to do would help here as it isn't immediately obvious from the code why we are doing this.

phosek updated this revision to Diff 111774.Aug 18 2017, 5:17 PM
phosek marked 5 inline comments as done.
phosek added a reviewer: rafael.
phosek added inline comments.
ELF/LinkerScript.cpp
736

You're right, that was a leftover from a previous version of the patch but that's no longer needed.

ruiu edited edge metadata.Aug 18 2017, 5:28 PM

I'm sorry but I realized that I don't actually understand what LinkerScript::allocateHeaders exactly does, so I don't understand the new code as well. I believe you know better than me, so can you add a function comment to describe what that function is supposed to do?

phosek updated this revision to Diff 112250.Aug 22 2017, 4:08 PM
phosek updated this revision to Diff 112255.Aug 22 2017, 4:16 PM
In D36256#846163, @ruiu wrote:

I'm sorry but I realized that I don't actually understand what LinkerScript::allocateHeaders exactly does, so I don't understand the new code as well. I believe you know better than me, so can you add a function comment to describe what that function is supposed to do?

Done, let me know if the comment makes sense to you.

ruiu accepted this revision.Aug 22 2017, 4:30 PM

Thank you for adding the comment! I now understand what this function is doing. LGTM

This revision is now accepted and ready to land.Aug 22 2017, 4:30 PM
This revision was automatically updated to reflect the committed changes.