This is an archive of the discontinued LLVM Phabricator instance.

[lib/ObjectYAML] - Make `ELFYAML::Relocation::Offset` optional.
ClosedPublic

Authored by grimar on Mar 4 2020, 6:05 AM.

Details

Summary

Currently yaml2obj require Offset field in a relocation description.
There are many cases when Offset is insignificant in a context of a test case.

Making Offset optional allows to simplify our test cases.
This is what this patch does.

Also, with this patch obj2yaml does not dump a zero offset of a relocation.

Diff Detail

Event Timeline

grimar created this revision.Mar 4 2020, 6:05 AM
Herald added a project: Restricted Project. · View Herald Transcript
MaskRay accepted this revision.Mar 4 2020, 5:54 PM

I think this is fine, but please wait for @jhenderson's opinions.

This revision is now accepted and ready to land.Mar 4 2020, 5:54 PM

I don't have any strong opinion on this. I personally would be slightly confused not seing an Offset printed by obj2yaml for one of the relocations, but I'm okay with it if you prefer that.

llvm/test/Object/X86/yaml-elf-x86-rel-broken.yaml
4 ↗(On Diff #248159)

Maybe worth doing this first?

grimar marked an inline comment as done.Mar 5 2020, 6:10 AM

I don't have any strong opinion on this. I personally would be slightly confused not seing an Offset printed by obj2yaml for one of the relocations, but I'm okay with it if you prefer that.

I think that one of the reason why we consistently see overloaded YAMLs on reviews is that because obj2yaml seems prints more data than it can.

llvm/test/Object/X86/yaml-elf-x86-rel-broken.yaml
4 ↗(On Diff #248159)

D75679 removes this file.

This revision was automatically updated to reflect the committed changes.