This is an archive of the discontinued LLVM Phabricator instance.

[obj2yaml] - SHT_DYNAMIC and SHT_REL* sections: stop dumping sh_entsize field when it has the default value.
ClosedPublic

Authored by grimar on Mar 16 2020, 5:48 AM.

Details

Summary

Currently obj2yaml always emits the EntSize property when sh_entsize != 0.
It is not correct. For example, for SHT_DYNAMIC section, EntSize == 0
is abnormal, while sizeof(ELFT::Dyn) is the expected default.

To reduce the output produces we should not dump default values.

yaml2obj tests that shows sh_entsize values produced are:

  1. For SHT_REL* sections: yaml2obj\ELF\reloc-sec-entry-size.yaml
  2. For SHT_DYNAMIC: yaml2obj\ELF\dynamic-section.yaml (See D76226)

Diff Detail

Event Timeline

grimar created this revision.Mar 16 2020, 5:48 AM
grimar edited the summary of this revision. (Show Details)Mar 16 2020, 5:49 AM
MaskRay accepted this revision.Mar 16 2020, 11:38 AM

Quote the ELF spec.

Some sections hold a table of fixed-size entries, such as a symbol table. For such a section, this member gives the size in bytes of each entry. The member contains 0 if the section does not hold a table of fixed-size entries.

For SHT_DYNAMIC and SHT_REL*, I think omitting sh_entsize is fine because they can be followed by entries. The following entries make it clear that they are tables of fixed-size entries.

If they could't be followed by entries, I would be nervous as this change added hidden knowledge.

Please hold off and wait for @jhenderson

This revision is now accepted and ready to land.Mar 16 2020, 11:38 AM
jhenderson accepted this revision.Mar 17 2020, 2:54 AM

Looks good to me. I think it would be expected that an unspecified EntSize means "the default" for a section.

I assume there are test cases that show that we do dump it when it is not the default for these kinds of sections?

llvm/test/tools/obj2yaml/dynamic-section.test
4–7

If you reflow this comment, you should be able to fit it onto 3 lines, I think.

llvm/test/tools/obj2yaml/relr-section.yaml
4

Also, check we do not...

I assume there are test cases that show that we do dump it when it is not the default for these kinds of sections?

Nope :( There are two more patches requred for that. I am going to post them a bit later today and will hold this one until we land them.

This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptMar 19 2020, 7:32 AM