This is an archive of the discontinued LLVM Phabricator instance.

[yaml2obj] - Allow producing ELFDATANONE ELFs
ClosedPublic

Authored by grimar on Mar 7 2019, 2:09 AM.

Details

Summary

I need this to remove a binary from LLD test suite.
The patch also simplifies the code a bit.

Diff Detail

Event Timeline

grimar created this revision.Mar 7 2019, 2:09 AM
grimar planned changes to this revision.Mar 7 2019, 2:34 AM

That is not enough it seems. Planning change.

grimar requested review of this revision.EditedMar 7 2019, 2:56 AM

This is fine as is. Just LLD side requires a slightly more complex test than I expected initially.
It is: https://reviews.llvm.org/D59085

What about support for other invalid data encodings e.g. 3, 4 etc?

lib/ObjectYAML/ELFYAML.cpp
235–236 ↗(On Diff #189667)

I'd completely rewrite the first part of the sentence, up to the comma as "ELFDATANONE is an invalid data encoding, ..."
"because want" -> "because we want"
Remove the quotes around "invalid"

test/tools/yaml2obj/elf-header-data.yaml
1 ↗(On Diff #189667)

Please name this test after ELFDATANONE, e.g. elfdatanone.yaml or elf-header-elfdatanone.aml etc?

6 ↗(On Diff #189667)

What about llvm-objdump? I'm guessing they all rely on the same Object library, so all get the error from the same place?

Perhaps a better test is to show using something like od that the byte at EI_DATA is 0?

grimar added a comment.Mar 7 2019, 3:23 AM

What about support for other invalid data encodings e.g. 3, 4 etc?

I do not have a use case for that, so I am not sure it is worth to think about that atm.

grimar updated this revision to Diff 189687.Mar 7 2019, 3:47 AM
grimar marked 3 inline comments as done.
  • Addressed review comments.
test/tools/yaml2obj/elf-header-data.yaml
6 ↗(On Diff #189667)

What about llvm-objdump? I'm guessing they all rely on the same Object library, so all get the error from the same place?

Yes.

Perhaps a better test is to show using something like od that the byte at EI_DATA is 0?

Done, thanks for the hint about od!

jhenderson added inline comments.Mar 7 2019, 3:51 AM
test/tools/yaml2obj/elf-header-elfdatanone.yaml
1 ↗(On Diff #189687)

YAML -> "YAML file" or "YAML input"

jhenderson accepted this revision.Mar 7 2019, 3:52 AM

LGTM, with one nit.

This revision is now accepted and ready to land.Mar 7 2019, 3:52 AM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptMar 7 2019, 4:10 AM
Herald added a subscriber: hiraditya. · View Herald Transcript