[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:150
+    Addrsig,
+    SyntheticFiller
   };
----------------
MaskRay wrote:
> 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.
Ok, renamed to "Fill".


================
Comment at: llvm/include/llvm/ObjectYAML/ELFYAML.h:156
+
+  Section(SectionKind K) : Kind(K) {}
+  virtual ~Section();
----------------
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.


================
Comment at: llvm/lib/ObjectYAML/ELFEmitter.cpp:673
+ELFState<ELFT>::getPhdrFragments(const ELFYAML::ProgramHeader &Phdr,
+                                 std::vector<typename ELFT::Shdr> &SHeaders) {
+  DenseMap<StringRef, ELFYAML::SyntheticFiller *> NameToFiller;
----------------
jhenderson wrote:
> It doesn't look like `SHeaders` is modified. Can this be an `ArrayRef`?
Yes. Done.


================
Comment at: llvm/lib/ObjectYAML/ELFEmitter.cpp:1242
+  StringSet<> Seen;
+  for (const std::unique_ptr<ELFYAML::Section> &Sec : Doc.Descriptions) {
+    auto S = dyn_cast<ELFYAML::SyntheticFiller>(Sec.get());
----------------
jhenderson wrote:
> I think it would be simpler to fold these two loops together, and then add one simple if to check whether to add to the DotShStrtab. You could use the `Seen` set to check for duplicates of both then, rather than needing to do extra look ups in `SN2I`. I'd also then tweak the error messages so that they say "section/fill" instead of just "section" or "CustomFiller".
Ok, done. (I had loops splitted initially because I suppose that fills will be used not often, hence a message saying "section/fill" might raise a question about "what is fill?" when it is not important at all. But I do not expect seeing many errors of that kind anyways).


================
Comment at: llvm/lib/ObjectYAML/YAML.cpp:47
+
+  assert((Data.size() % 2 == 0) && (N == UINT64_MAX || N <= Data.size() / 2));
+  for (uint64_t I = 0, E = std::min(N, Data.size() / 2); I != E; ++I) {
----------------
jhenderson wrote:
> I'm not sure I follow the point of this assert? It probably needs an accompanying message.
After the change to "the maximum size to write" we do not need it probably.


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

https://reviews.llvm.org/D69709