[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]

jhenderson added inline comments.


================
Comment at: llvm/include/llvm/ObjectYAML/ELFYAML.h:156
+
+  Section(SectionKind K) : Kind(K) {}
+  virtual ~Section();
----------------
grimar wrote:
> jhenderson wrote:
> > Maybe we should rename this class to something more generic, like `Block` or `Piece` or similar? A fill isn't really a section, and calling it that might get a bit confusing. `RegularSection` could then just be called `Section`.
> I've renamed to `Chunk`. Does it sound OK?
> 
> Also I had to rename the enum.
`Chunk` sounds good.


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


================
Comment at: llvm/include/llvm/ObjectYAML/ELFYAML.h:201
+  // We have to remember the offset of the fill, because it does not have
+  // a corresponding section header, like a section. We might need this
+  // information when writing the output.
----------------
like -> unlike (sorry I got that one wrong!)


================
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);
 };
----------------
Should the parameters here still be called `Section`?


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


================
Comment at: llvm/lib/ObjectYAML/ELFEmitter.cpp:1219
+
+  // Fill the content with a pattern bytes specified.
+  uint64_t Written = 0;
----------------
with the specified pattern.


================
Comment at: llvm/test/tools/yaml2obj/custom-fill.yaml:129
+    Sections:
+      - Section: .bar
+      - Section: fill2
----------------
I meant to have two fills in this segment, as well as .bar:

```
Sections:
  - Section: fill
  - Section: .bar
  - Section: fill2
```


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


================
Comment at: llvm/test/tools/yaml2obj/custom-fill.yaml:266
+## Check the error message produced when a program header references
+## an unknown section or fill and have at least one Fill defined.
+
----------------
I think this comment needs updating now that there aren't different error messages for where there is a/is no defined fill.


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

https://reviews.llvm.org/D69709