Page MenuHomePhabricator

[yaml2obj] Make e_phoff and e_phentsize 0 if there are no program headers
ClosedPublic

Authored by abrachet on Sep 1 2019, 3:15 PM.

Details

Summary

It says here that if there are no program headers than e_phoff should be 0, but currently it is always set after the header. GNU's readelf (but not llvm-readelf) complains about this: readelf: Warning: possibly corrupt ELF header - it has a non-zero program header offset, but no program headers.

Diff Detail

Repository
rL LLVM

Event Timeline

abrachet created this revision.Sep 1 2019, 3:15 PM
Herald added a project: Restricted Project. · View Herald TranscriptSep 1 2019, 3:15 PM
MaskRay added inline comments.Sep 1 2019, 7:24 PM
llvm/test/tools/yaml2obj/no-segments-phoff.test
1 ↗(On Diff #218277)

The test may be renamed to elf-header-ephoff.yaml

abrachet updated this revision to Diff 218285.Sep 1 2019, 10:13 PM

Changed test name

abrachet marked an inline comment as done.Sep 1 2019, 10:13 PM
MaskRay accepted this revision.Sep 1 2019, 11:52 PM
This revision is now accepted and ready to land.Sep 1 2019, 11:52 PM
grimar accepted this revision.Sep 2 2019, 1:53 AM

LGTM

llvm/lib/ObjectYAML/ELFEmitter.cpp
231 ↗(On Diff #218285)

Perhaps we might want to set this to 0 too when there are no headers.

jhenderson added inline comments.Sep 2 2019, 2:13 AM
llvm/lib/ObjectYAML/ELFEmitter.cpp
231 ↗(On Diff #218285)

+1 to this (it's not required by the gABI, but it seems to be common practice). We might as well do it at the same time.

llvm/test/tools/yaml2obj/elf-header-ephoff.yaml
1 ↗(On Diff #218285)

Nit: missing trailing full stop.

If you go with changing the size field too, I'd test that and e_phnum in the same test, and rename this to "elf-header-no-phdrs.yaml" or even "elf-no-phdrs.yaml".

abrachet updated this revision to Diff 218401.Sep 2 2019, 9:51 PM

Made e_phentsize 0 when there are no program headers too.

abrachet marked 3 inline comments as done.Sep 2 2019, 9:52 PM
MaskRay accepted this revision.Sep 2 2019, 9:56 PM
MaskRay added inline comments.
llvm/test/tools/yaml2obj/elf-no-phdrs.yaml
1 ↗(On Diff #218401)

e_phoff and e_phnum are set to 0

1 ↗(On Diff #218401)

Sorry, I meant e_phoff and e_phentsize

abrachet updated this revision to Diff 218404.Sep 2 2019, 10:09 PM
abrachet retitled this revision from [yaml2obj] Make e_phoff 0 if there are no program headers to [yaml2obj] Make e_phoff and e_phentsize 0 if there are no program headers.

Changed test comment.

abrachet marked 2 inline comments as done.Sep 2 2019, 10:10 PM
jhenderson accepted this revision.Sep 3 2019, 1:17 AM

LGTM, with one suggestion.

llvm/test/tools/yaml2obj/elf-no-phdrs.yaml
8 ↗(On Diff #218404)

Minor thing: it's probably worth adding a {{$}} to the end of this line (and optionally the previous line for symmetry). Without it, ProgramHeaderEntrySize: 0 could match ProgramHeaderEntrySize: 0x1234 which would obviously be bad.

abrachet updated this revision to Diff 219022.Sep 5 2019, 7:12 PM

Addressed review comments.

Addressed review comments.

Thanks. I was still seeing warnings from object files yesterday and I know your patch can fix that :)

abrachet marked an inline comment as done.Sep 5 2019, 7:26 PM
This revision was automatically updated to reflect the committed changes.