This is an archive of the discontinued LLVM Phabricator instance.

[ObjectYAML] Handle SHF_COMPRESSED
ClosedPublic

Authored by smeenai on Oct 2 2017, 8:52 PM.

Details

Summary

This was previously being silently dropped by obj2yaml and caused
parsing errors with yaml2obj.

Event Timeline

smeenai created this revision.Oct 2 2017, 8:52 PM
davide accepted this revision.Oct 2 2017, 9:07 PM
This revision is now accepted and ready to land.Oct 2 2017, 9:07 PM
davide added a comment.Oct 2 2017, 9:09 PM

FWIW, the fact we're dropping flags we don't know about it a little annoying.
We should error instead.

To clarify, it's dropped silently by obj2yaml, but yaml2obj complains if it doesn't recognize a flag value. I'll update the commit message before committing.

I was contemplating adding a test for obj2yaml as well to ensure its output didn't drop SHF_COMPRESSED, but I figured it would be slightly redundant, since obj2yaml and yaml2obj use the same flags handling logic.

davide added a comment.Oct 2 2017, 9:42 PM

To clarify, it's dropped silently by obj2yaml, but yaml2obj complains if it doesn't recognize a flag value. I'll update the commit message before committing.

Thanks for clarifying.

I was contemplating adding a test for obj2yaml as well to ensure its output didn't drop SHF_COMPRESSED, but I figured it would be slightly redundant, since obj2yaml and yaml2obj use the same flags handling logic.

Probably it doesn't matter.

davide added inline comments.Oct 2 2017, 9:45 PM
test/tools/yaml2obj/shf-compressed.yaml
1–2 ↗(On Diff #117467)

if you really want to make this test slightly fancier, you can test that the outputs of yaml2obj and obj2yaml are the same up to isomorphism (for some very relaxed definition of isomorphism, as you're not guaranteed that converting and then converting back will give you the original object for a series of reason).

smeenai updated this revision to Diff 117469.Oct 2 2017, 10:30 PM
smeenai edited the summary of this revision. (Show Details)
smeenai removed a reviewer: davide.
smeenai removed a subscriber: davide.

Clarify description and test obj2yaml

This revision now requires review to proceed.Oct 2 2017, 10:30 PM

Argh, totally didn't mean to remove the accept.

test/tools/yaml2obj/shf-compressed.yaml
1–2 ↗(On Diff #117467)

I didn't go that fancy, but I did add verification that the obj2yaml output includes the flag.

davide accepted this revision.Oct 2 2017, 11:29 PM

lgtm, thanks Shoaib.

This revision is now accepted and ready to land.Oct 2 2017, 11:29 PM
This revision was automatically updated to reflect the committed changes.