[PATCH] D69709: [yaml2obj] - Add a way to describe the custom data that is not part of an output section.

James Henderson via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Fri Nov 8 05:28:59 PST 2019


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

LGTM.



================
Comment at: llvm/lib/ObjectYAML/ELFEmitter.cpp:94
+struct PhdrEntry {
+  uint64_t ShOffset;
+  uint64_t ShSize;
----------------
grimar wrote:
> jhenderson wrote:
> > jhenderson wrote:
> > > grimar wrote:
> > > > MaskRay wrote:
> > > > > MaskRay wrote:
> > > > > > `SegmentFragment` may be a better name. `Phdr` (program header) looks to me properties that describe the program header, but here this structure describes fragments that make up the segment content.
> > > > > The name is inspired by `MCFragment`, a concept in MC. It has several subclasses such as MCFillFragment, MCPaddingFragment, etc. 
> > > > > 
> > > > > We probably should drop the `Segment` prefix from the name `SegmentFragment`. This structure can describe fragments of non-segment part as well, e.g. gaps between 2 non-SHF_ALLOC sections.
> > > > Renamed to `Segment`. Also, renamed the method: `getEntriesForPhdr` -> `getPhdrSegments`.
> > > Maybe the fields need renaming, as they aren't necessarily related to the sh_* section header fields? Probably just Offset, Size etc. What does Type mean for a non-section fragment?
> > Ping?
> Sorry, I've missed this comment somehow.
> I've renamed the fields.
> 
> > What does Type mean for a non-section fragment?
> 
> We have the following code when handling program headers:
> ```
>     uint64_t FileOffset = PHeader.p_offset, MemOffset = PHeader.p_offset;
>     for (const Fragment &F : Fragments) {
>       uint64_t End = F.ShOffset + F.ShSize;
>       MemOffset = std::max(MemOffset, End);
> 
>       if (F.ShType != llvm::ELF::SHT_NOBITS)
>         FileOffset = std::max(FileOffset, End);
>     }
> ```
> 
> i.e. we handle `SHT_NOBITS` differently, so I had to intoduce a field to differentiate them.
> The `ShType` was just consistent with other `Sh*` fields, thats why I added it.
> We can have `bool IsNobits` or alike instead though. What do you prefer?
Type is fine, I think. Thanks for the explanation.


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

https://reviews.llvm.org/D69709





More information about the llvm-commits mailing list