[PATCH] D111487: [XCOFF][yaml2obj] support for the auxiliary file header.

[Esme Yi via Phabricator via llvm-commits]

Esme added inline comments.


================
Comment at: llvm/test/tools/yaml2obj/XCOFF/aux-hdr-omit.yaml:50
+  - Flags:       [ STYP_TEXT ]
+    SectionData: "1234000000"
+  - Flags:       [ STYP_TEXT ]
----------------
jhenderson wrote:
> Probably fill with non-zero values all the way. Maybe also add a comment why you have two of each section.
I think setting section content with non-zero values is enough since the size and address will be derived automatically.


================
Comment at: llvm/test/tools/yaml2obj/XCOFF/aux-hdr-omit.yaml:74
+
+## Case2: same as case1, except it is 64-bit.
+# RUN: yaml2obj %s --docnum=1 -DMAGIC=0x1F7 -o %t2
----------------
DiggerLin wrote:
> jhenderson wrote:
> > Same below.
> > 
> > I wonder if you could reuse most of the FileCheck data here with the 32-bit case, rather than duplicating it? I've not looked too closely, but perhaps multiple check-prefixes would be appropriate, e.g. `--check-prefixes=COMMON1,CASE1-64`?
> the data members and the order the data members of auxiliary header between 32bit and 64bit are different , if using COMMON1,CASE1-64 , I think it maybe more difficult to read the test case.
Thanks for your comments and agree with Digger to keep the current check-style.


================
Comment at: llvm/test/tools/yaml2obj/XCOFF/aux-hdr-omit.yaml:8
+# CHECK-NEXT:   Version: 0x1
+# CHECK-NEXT:   Size of .text section: 0x4
+# CHECK-NEXT:   Size of .data section: 0x4
----------------
jhenderson wrote:
> Esme wrote:
> > jhenderson wrote:
> > > I might be being dumb, but the sizes below look like 2 bytes to me.
> > Because the DefaultSectionAlign is 4 in XCOFF.
> > If I didn't set the sizes explicitly, the calculated values are used.
> So in XCOFF are sections tail padded? In ELF, there are just unlabelled gaps between sections, if alignment padding is required.
Yes, it's required that the section address aligned to DefaultSectionAlign.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D111487