[PATCH] D64913: [yaml2obj] - Add a support for defining null sections in YAMLs.
George Rimar via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Mon Jul 22 07:38:45 PDT 2019
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
More information about the llvm-commits
mailing list