[PATCH] D64913: [yaml2obj] - Add a support for defining null sections in YAMLs.

[George Rimar via Phabricator via llvm-commits]

grimar marked 3 inline comments as done.
grimar added inline comments.


================
Comment at: test/tools/yaml2obj/elf-custom-null-section.yaml:41
+    EntSize:      0x0
+    Link:         ""
+
----------------
jhenderson wrote:
> Specify Link explicitly with an integer here (i.e. Link: 0).
> 
> What about Info, and Address and perhaps sh_offset?
> What about Info, and Address and perhaps sh_offset?

I focused on a use cases which are correct from ELF spec view in this patch.

Adding support for another fields can be a part of follow-up refactoring.
(I think we might be able to handle SHT_NULL in the same loop as other sections in `initSectionHeaders`,
that would allow to support setting all other fields at once).


================
Comment at: tools/yaml2obj/yaml2elf.cpp:301
   for (StringRef Name : State.implicitSectionNames())
-    if (State.SN2I.get(Name) > Doc.Sections.size())
+    if (State.SN2I.get(Name) > (Doc.Sections.size() - (DocNullSec ? 1 : 0)))
       ImplicitSections.push_back(Name);
----------------
MaskRay wrote:
> jhenderson wrote:
> > I wonder if it would make sense to treat the null section the same as any other implicit section? What do you think?
> How to do that? SHN_UNDEF (of SHT_NULL) is the first section, different from other implicit sections...
Yes, it is very different. I also do not see a clean way to handle it as a regular implicit section.
It would require lot of changes everywhere, at least because we place implicit sections to the end,
what doesn't work for SHT_NULL.

But seems the solution is to land D64999 first. It should allow to significantly simplify this patch.


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

https://reviews.llvm.org/D64913