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

[George Rimar via Phabricator via llvm-commits]

grimar added inline comments.


================
Comment at: test/tools/yaml2obj/elf-custom-null-section.yaml:41
+    EntSize:      0x0
+    Link:         ""
+
----------------
grimar wrote:
> 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).
> What about Info, and Address and perhaps sh_offset?

Oops, I misread slightly. I added `Info` and `Address` zeroed fields here.

This patch still doesn't support setting them to something else though.
I didn't add `ShOffset` or `ShSize`, this is out of the area of this patch and
anyways not supported either.


================
Comment at: test/tools/yaml2obj/elf-custom-null-section.yaml:46
+# RUN: yaml2obj --docnum=3 %s -o %t3
+# RUN: llvm-readelf --sections %t3 | FileCheck %s --check-prefix=CASE2
+
----------------
MaskRay wrote:
> `CASE2` -> a descriptive name. (I'd call it `EXPLICIT-ZERO` but there may be a better name).
I used `ZERO-VS-NONZERO` because the test case above already explicitly defines the zero section.


================
Comment at: test/tools/yaml2obj/elf-custom-null-section.yaml:142
+Sections:
+  - Type: SHT_NULL
----------------
jhenderson wrote:
> How about a test case for a SHT_NULL section that is not the first listed section? I don't think there's anything in the ELF spec saying that you can't have one elsewhere in the section header table.
I think that wasn't never intentionally supported, I added a test to check we do not crash.
Not sure if we want to check something else here.


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

https://reviews.llvm.org/D64913