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

[George Rimar via Phabricator via llvm-commits]

grimar planned changes to this revision.
grimar marked an inline comment as done.
grimar added inline comments.


================
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:
> > 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.



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

https://reviews.llvm.org/D83482