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

[George Rimar via Phabricator via llvm-commits]

grimar added inline comments.


================
Comment at: llvm/include/llvm/ObjectYAML/ELFYAML.h:196
+// size and program headers produced.
+struct SyntheticFill : Chunk {
+  yaml::BinaryRef Pattern;
----------------
jhenderson wrote:
> Might cause some naming ambiguities, so feel free to leave as is, but this can probably be named simply "Fill".
Renamed to `Fill`, went fine.


================
Comment at: llvm/include/llvm/ObjectYAML/ELFYAML.h:639-640
+template <> struct MappingTraits<std::unique_ptr<ELFYAML::Chunk>> {
+  static void mapping(IO &IO, std::unique_ptr<ELFYAML::Chunk> &Section);
+  static StringRef validate(IO &io, std::unique_ptr<ELFYAML::Chunk> &Section);
 };
----------------
jhenderson wrote:
> Should the parameters here still be called `Section`?
Fixed.


================
Comment at: llvm/lib/ObjectYAML/ELFEmitter.cpp:94
+struct PhdrEntry {
+  uint64_t ShOffset;
+  uint64_t ShSize;
----------------
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?


================
Comment at: llvm/test/tools/yaml2obj/custom-fill.yaml:137
+
+## Check that the "Pattern" field is mandatory.
+# RUN: not yaml2obj --docnum=4 2>&1 %s | FileCheck %s --check-prefix=NOPATTERN
----------------
jhenderson wrote:
> I think it's probably harmless to allow fills to have no Pattern field. This is not significantly different from the case where the Pattern is an empty string.
Ok.


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

https://reviews.llvm.org/D69709