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

[Fangrui Song via Phabricator via llvm-commits]

MaskRay added inline comments.


================
Comment at: llvm/include/llvm/ObjectYAML/ELFYAML.h:150
+    Addrsig,
+    SyntheticFiller
   };
----------------
jhenderson wrote:
> Maybe SyntheticFill or even just Fill instead of SyntheticFiller? This would better match the linker script directive. Everything in the output is technically Synthetic too...
"Fill" looks good.


================
Comment at: llvm/lib/ObjectYAML/ELFYAML.cpp:1144
+    IO.mapRequired("Type", StrType);
+    if (StrType == "CustomFiller") {
+      Section.reset(new ELFYAML::SyntheticFiller());
----------------
jhenderson wrote:
> I'd just call it "Fill".
"Fill" looks good.


================
Comment at: llvm/lib/ObjectYAML/ELFYAML.cpp:1485
   IO.mapOptional("ProgramHeaders", Object.ProgramHeaders);
-  IO.mapOptional("Sections", Object.Sections);
+  IO.mapOptional("Sections", Object.Descriptions);
   IO.mapOptional("Symbols", Object.Symbols);
----------------
jhenderson wrote:
> grimar wrote:
> > MaskRay wrote:
> > > I feel that using `Sections` for `Fragment`s may be confusing because `Fragment`s are not described by the section header table. Does a new field add a lot of complexity? If not, we can add a parallel YAML mapping key `Fragments`, and use `Fragments` instead of `Sections` when there is a need for a `Fragment` (i.e. when there is non-section data that needs describing).
> > > 
> > > Opinions? @rupprecht @jhenderson @labath
> > To clarify I understood the idea: do you mean that YAML writers should be able to use both "Sections" and "Fragments", where the latter will be just an alias for "Sections"? I.e to make both below to be valid?
> > 
> > ```
> > Fragments:
> > - Section1
> > - Fragmen1
> > - Section2
> > ```
> > 
> > ```
> > Sections:
> > - Section1
> > - Fragmen1
> > - Section2
> > ```
> > 
> > With that since they are both optional we will need to add a check that they are not specified at the same time. Also, I guess most of users will just randomly use either "Sections" or "Fragments" because of common copy-paste approach. I'd prefer just to rename "Sections" to something if there are any concerns about the naming.
> > 
> > When I wrote this patch I thought about this question though and came up to thinking about this part of YAML just like about a linker script's "SECTIONS" command. It might contain not only sections, but location counter moving, commands like "ASSERT" e.t.c, but it is still called "SECTIONS" and I probably do not find it confusing.
> I'd keep it as "Sections". This is similar to linker scripts as @grimar says, but also more pragmatic, as I think the fill case is going to be rare, but when it is used, I think they should be mixed in order with the sections, as otherwise the order may be non-obvious.
I like the SECTIONS command analogy :) And I see why you rename the `Sections` field to `Descriptions` (output section descriptions) now. I am fine with the simple approach, just asked about the thoughts of alternatives in case people might find it confusing. Since we don't find it confusing, keeping it as-is looks good.


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

https://reviews.llvm.org/D69709