This is an archive of the discontinued LLVM Phabricator instance.

[yaml2obj] Write the section header table after section contents
ClosedPublic

Authored by MaskRay on Sep 5 2019, 5:03 AM.

Details

Summary

Linkers (ld.bfd/gold/lld) place the section header table at the very
end. The section header table is optional in executable/shared objects,
so this allows tools to strip it. In addition, if we add or remove a
section, the size of the section header table will change. Placing the
section header table in the end keeps section offsets unchanged.

yaml2obj currently places the section header table immediately after the
program header. Follow what linkers do to make offset updating easier.

Diff Detail

Repository
rL LLVM

Event Timeline

MaskRay created this revision.Sep 5 2019, 5:03 AM
Herald added a project: Restricted Project. · View Herald TranscriptSep 5 2019, 5:03 AM

There are 20+ other tests (llvm-readobj,llvm-objcopy,etc) that are affected. I am still updating them.

grimar added a comment.Sep 5 2019, 5:19 AM

Generally the idea is OK I think.

lib/ObjectYAML/ELFEmitter.cpp
233 ↗(On Diff #218895)

Seems you can just call initELFHeader later instead?
I think placing it right before OS.write((const char *)&Header, sizeof(Header)); in ELFState<ELFT>::writeELF should work.

It needs rewriting the assignment:

const size_t SectionContentBeginOffset = Header.e_ehsize +
                                         Header.e_phentsize * Header.e_phnum +
                                         Header.e_shentsize * Header.e_shnum;

to something like

const size_t SectionContentBeginOffset = sizeof(Elf_Ehdr) +
                                         sizeof(Elf_Phdr) * Doc.ProgramHeaders.size() + ... ;

but seems there are no more problems?

1030 ↗(On Diff #218895)

typedef typename ELFT::uint uintX_t; ?

1063 ↗(On Diff #218895)

You don't need (void) I think. We already have one more place where the result isn't used.

MaskRay updated this revision to Diff 218899.Sep 5 2019, 5:38 AM
MaskRay marked 4 inline comments as done.

Update remaining tests
Address review comments

MaskRay added inline comments.Sep 5 2019, 5:38 AM
lib/ObjectYAML/ELFEmitter.cpp
233 ↗(On Diff #218895)

Reordered.

Elf_Ehdr Header;
uint64_t SHOff;
(void)CBA.getOSAndAlignedOffset(SHOff, sizeof(typename ELFT::uint));
State.initELFHeader(Header, SHOff);
OS.write((const char *)&Header, sizeof(Header));

is still needed.

1030 ↗(On Diff #218895)

Moved one use of typename ELFT::uint into initELFHeader, so this function just uses typename ELFT::uint once. The typedef will be reundant, then.

grimar added inline comments.Sep 5 2019, 5:52 AM
lib/ObjectYAML/ELFEmitter.cpp
1060 ↗(On Diff #218899)

Side note: we are not consistent in naming... we have ShOffset vs SHOffset and what is worse IO.mapOptional("SHOffset", FileHdr.SHOffset); vs IO.mapOptional("ShOffset", Section.ShOffset)

1063 ↗(On Diff #218899)

What do you think about moving this code to a new method which can replace the
initELFHeader? Something like:

void ELFState<ELFT>::writeElfHeader() {
  uint64_t SHOff;
  CBA.getOSAndAlignedOffset(SHOff, sizeof(typename ELFT::uint));

  Elf_Ehdr Header;

  /* Place the code from initELFHeader() method here */

  OS.write((const char *)&Header, sizeof(Header));
}

That way you do not need to declare and pass Elf_Ehdr and SHOff from here.

MaskRay updated this revision to Diff 218902.Sep 5 2019, 6:07 AM
MaskRay marked 3 inline comments as done.

Replace initELFHeader with writeELFHeader

MaskRay added inline comments.Sep 5 2019, 6:11 AM
lib/ObjectYAML/ELFEmitter.cpp
1060 ↗(On Diff #218899)

Changed to ShOffset

MaskRay updated this revision to Diff 218903.Sep 5 2019, 6:11 AM

SHOff -> ShOffset

grimar accepted this revision.Sep 5 2019, 6:16 AM

LGTM

test/tools/llvm-objcopy/ELF/overlapping-sections-in-segments.test
13 ↗(On Diff #218902)

Seems we should be able to rewrite this and "overlapping-sections.test" tests to use just YAML, since we have override the sh_offset now. I'll try to do this.

This revision is now accepted and ready to land.Sep 5 2019, 6:16 AM
grimar added inline comments.Sep 5 2019, 6:16 AM
test/tools/llvm-objcopy/ELF/overlapping-sections-in-segments.test
13 ↗(On Diff #218902)

since we have -> since we can

This revision was automatically updated to reflect the committed changes.

Thanks for doing this. I have vague concerns that there might be some tests out there that assume the section header table's position, and somehow rely on it, but are now passing for unrelated reasons to their original intent, but I don't have a good way to check for them short of auditing every yaml2obj user, which seems silly, so I guess we just have to take that risk.

It broke ASan BB:
http://lab.llvm.org:8011/builders/sanitizer-x86_64-linux-bootstrap/builds/13820/steps/check-llvm%20asan/logs/stdio

It is not a problem of this patch and hopefully is fixed now by rL371318. I am waiting for BB build result.

Thanks for doing this. I have vague concerns that there might be some tests out there that assume the section header table's position, and somehow rely on it, but are now passing for unrelated reasons to their original intent, but I don't have a good way to check for them short of auditing every yaml2obj user, which seems silly, so I guess we just have to take that risk.

I have considered this use case. An unconventional placement of the section header table may detect problems that reply on its exact position. Making it follow the convention will no longer detect such problems. Just like we have SHOff, SHNum, ShOffset, etc, we can probably add an option to place the section header table immediately after the program header, if we consider this check useful. A more general option is to add a gap between the program header table and the first section.

Thanks!

I have considered this use case. An unconventional placement of the section header table may detect problems that reply on its exact position. Making it follow the convention will no longer detect such problems. Just like we have SHOff, SHNum, ShOffset, etc, we can probably add an option to place the section header table immediately after the program header, if we consider this check useful. A more general option is to add a gap between the program header table and the first section.

I might even go one step further and allow an option to force the exact offset of the section header table (and the program header table for that matter), as opposed to just the value written in the ELF header. That would allow for maximum flexibility. Having a default of after the sections though makes a lot of sense to me.