This is an archive of the discontinued LLVM Phabricator instance.

[yaml2obj] - Add a syntax to override e_phoff, e_phentsize and e_phnum fields.
ClosedPublic

Authored by grimar on Jul 9 2020, 6:18 AM.

Details

Summary

This adds EPhOff, EPhEntSize and EPhNum keys.
Will be useful for creating broken objects for testing llvm-readelf.

Diff Detail

Event Timeline

grimar created this revision.Jul 9 2020, 6:18 AM
Herald added a project: Restricted Project. · View Herald Transcript
grimar updated this revision to Diff 277048.Jul 10 2020, 8:08 AM
  • Fix the test, broken by mistake.
jhenderson added inline comments.Jul 13 2020, 12:23 AM
llvm/lib/ObjectYAML/ELFYAML.cpp
873–875

The section header table related fields use "SH" not "Sh". It probably makes sense for us to standardise on one form or the other. In other words, I think we should probably use "PH*" here.

llvm/test/tools/yaml2obj/ELF/header-sh-fields.yaml
4–16

This case isn't really checking the default values any more. It's checking that the default variable values as specified in the YAML are followed, but the purpose of the check seems to me to be to show what happens when the fields aren't specified.

4–16

Didn't realise this is part of a stack - the comment applies to the previous commit.

grimar marked an inline comment as done.Jul 13 2020, 3:24 AM
grimar added inline comments.
llvm/lib/ObjectYAML/ELFYAML.cpp
873–875

Fields in Sections class also use Sh*:

// This can be used to override the offset stored in the sh_name field.
// It does not affect the name stored in the string table.
Optional<llvm::yaml::Hex64> ShName;

// This can be used to override the sh_offset field. It does not place the
// section data at the offset specified.
Optional<llvm::yaml::Hex64> ShOffset;

// This can be used to override the sh_size field. It does not affect the
// content written.
Optional<llvm::yaml::Hex64> ShSize;

// This can be used to override the sh_flags field.
Optional<llvm::yaml::Hex64> ShFlags;

So we have 2 options it seems:

  1. Use "SH*" + "PH*" everywhere (i.e make the renaming you suggest + rename fields in Sections).
  2. Rename "PH*" -> "Ph*" in a follow-up here (I assumed this action when wrote this patch).

What should we do?

Relatedly, how about we actually name these fields more precisely like their spec names, e.g. EShoff, EPhoff, EShentsize (or something equivalent). I don't mind which approach we go with naming, but I'd recommend we be consistent.

grimar updated this revision to Diff 277414.Jul 13 2020, 6:54 AM
grimar marked 3 inline comments as done.
grimar edited the summary of this revision. (Show Details)
  • Address review comments.
llvm/lib/ObjectYAML/ELFYAML.cpp
873–875

Relatedly, how about we actually name these fields more precisely like their spec names, e.g. EShoff, EPhoff, EShentsize (or something equivalent).

But we do not add E prefix to other fields. E.g: we call e_type as Type.
I renamed them to PH* (e.g. PhEntSize -> PHEntSize), because phentsize is Program Header Entry Size, so it makes sense I think.

grimar planned changes to this revision.Jul 13 2020, 7:08 AM
grimar marked an inline comment as done.
grimar added inline comments.
llvm/lib/ObjectYAML/ELFYAML.cpp
873–875

But we do not add E prefix to other fields. E.g: we call e_type as Type.

Thinking about it again, probably we should use the E prefix for these fields, because them are special.
Currently we name sh_name -> ShName, sh_offset -> ShOffset etc for section fields.
So, probably, the consistent way would indeed be to name e_phentsize as EPHEntSize/`EPhEntSize. I.e. to use the E prefix.
I'll add it.

grimar updated this revision to Diff 277422.Jul 13 2020, 7:19 AM
  • Use E prefix for names.
grimar edited the summary of this revision. (Show Details)Jul 13 2020, 7:28 AM
jhenderson accepted this revision.Jul 14 2020, 12:33 AM

LGTM, thanks.

llvm/lib/ObjectYAML/ELFYAML.cpp
873–875

Right, exactly. The convention we seem to have adopted is <Prefix><Thing> for the direct writing to fields that would typically be auto-generated/inferred from other fields etc. which should have no impact on anything except the final value of the field in the output, and just <Thing> for the other fields.

This revision is now accepted and ready to land.Jul 14 2020, 12:33 AM
This revision was automatically updated to reflect the committed changes.