This is an archive of the discontinued LLVM Phabricator instance.

Initialize Elf header to Zero
ClosedPublic

Authored by rdhindsa on Mar 28 2018, 10:23 AM.

Diff Detail

Event Timeline

rdhindsa created this revision.Mar 28 2018, 10:23 AM
mgrang added a subscriber: mgrang.Mar 28 2018, 10:28 AM
mgrang added inline comments.
lld/ELF/Writer.cpp
2111

Please make spacing uniform.

memset(Buf, 0, sizeof(Elf_Ehdr) + sizeof(Elf_Phdr));

Also could you please add a unit test?

ruiu added inline comments.Mar 28 2018, 10:53 AM
lld/ELF/Writer.cpp
2111

If -r option is given, we don't emit Phdr, so this is inaccurate. It's in practice not a problem because I can't think of an ELF file that is smaller than sizeof(Elf_Ehdr) + sizeof(Elf_Phdr), but being accurate helps readers understand the code.

So it is better to just set the first sizeof(Elf_Ehdr).

pcc added a subscriber: pcc.Mar 28 2018, 11:01 AM

I thought this was a memory-mapped file. Aren't its contents initialized to zero by the operating system?

rdhindsa updated this revision to Diff 140150.Mar 28 2018, 3:40 PM
rdhindsa marked 2 inline comments as done.
ruiu accepted this revision.Mar 28 2018, 4:00 PM

LGTM

lld/test/ELF/elf-header-initialized.s
1

I'd name this file just "elf-header.s".

4

Please remove '-r' as you are not using it.

7

Ditto

This revision is now accepted and ready to land.Mar 28 2018, 4:00 PM
pcc requested changes to this revision.Mar 28 2018, 4:02 PM
In D44986#1050562, @pcc wrote:

I thought this was a memory-mapped file. Aren't its contents initialized to zero by the operating system?

I would like an answer to this.

This revision now requires changes to proceed.Mar 28 2018, 4:02 PM
pcc added a comment.Mar 28 2018, 4:14 PM

I see, it is because we write trap instructions to executable segments before writing the header. Please add a comment explaining this.

rdhindsa updated this revision to Diff 140290.Mar 29 2018, 10:57 AM
rdhindsa retitled this revision from Initialize Elf header and Program Header to Zero to Initialize Elf header to Zero.
rdhindsa edited the summary of this revision. (Show Details)
pcc accepted this revision.Mar 29 2018, 12:09 PM

LGTM

This revision is now accepted and ready to land.Mar 29 2018, 12:09 PM
MaskRay added a subscriber: MaskRay.EditedMar 29 2018, 12:35 PM
template <class ELFT> void Writer<ELFT>::writeTrapInstr() {
  if (Script->HasSectionsCommand)
    return;

  // Fill the last page.
  uint8_t *Buf = Buffer->getBufferStart();
  for (PhdrEntry *P : Phdrs)
    if (P->p_type == PT_LOAD && (P->p_flags & PF_X)) ///////////////////
      fillTrap(Buf + alignDown(P->p_offset + P->p_filesz, Target->PageSize),
               Buf + alignTo(P->p_offset + P->p_filesz, Target->PageSize));

How about if (P->p_type == PT_LOAD && P->p_flags & PF_X && P->LastSec != Out::ProgramHeaders)

Other write* functions don't initialize the buffer to zero

For some cases, eg with -r option, Phdr is not emitted. Hence it is better to initialize ELF header to zero instead.

This revision was automatically updated to reflect the committed changes.