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

[James Henderson via Phabricator via llvm-commits]

jhenderson accepted this revision.
jhenderson added a comment.
This revision is now accepted and ready to land.

LGTM, thanks.



================
Comment at: llvm/lib/ObjectYAML/ELFYAML.cpp:873-875
+  IO.mapOptional("PhOff", FileHdr.PhOff);
+  IO.mapOptional("PhEntSize", FileHdr.PhEntSize);
+  IO.mapOptional("PhNum", FileHdr.PhNum);
----------------
grimar wrote:
> grimar wrote:
> > grimar wrote:
> > > jhenderson wrote:
> > > > 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.
> > > 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?
> > >>! In D83482#2147128, @jhenderson wrote:
> > > 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 **H**eader Entry Size, so it makes sense I think.
> > 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.
> 
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.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D83482/new/

https://reviews.llvm.org/D83482