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

[James Henderson via Phabricator via llvm-commits]

jhenderson added inline comments.


================
Comment at: llvm/lib/ObjectYAML/XCOFFEmitter.cpp:351-352
+  if (Is64Bit) {
+    // Reserved for debugger.
+    W.OS.write_zeros(4);
+    W.write<uint64_t>(InitAuxFileHdr.TextStartAddr.getValueOr(yaml::Hex64(0)));
----------------
I might put the comment on the same line, because it's a little confusing whether it's referring to just this one field or not.


================
Comment at: llvm/lib/ObjectYAML/XCOFFEmitter.cpp:375
+  W.write<uint8_t>(InitAuxFileHdr.CpuFlag.getValueOr(yaml::Hex16(0)));
+  // Reserved for CUP type.
+  W.write<uint8_t>(0);
----------------
DiggerLin wrote:
> should be  yaml::Hex8(0) for flag.
Just checking: should this comment be `CPU` rather than `CUP`?


================
Comment at: llvm/test/tools/yaml2obj/XCOFF/aux-hdr-full-contents.yaml:91-94
+  EntryPointAddr:        0x2824
+  TextStartAddr:         0x1128
+  DataStartAddr:         0x2740
+  TOCAnchorAddr:         0x2844
----------------
These numbers aren't obviously arbitrary, I'd suggest making them more obviously so, e.g. `0x1111`, `0x2222` etc.


================
Comment at: llvm/test/tools/yaml2obj/XCOFF/aux-hdr-omit.yaml:1
+## Test that yaml2obj automatically sets fields of the auxiliary file header
+## if not set explicitly.
----------------
Perhaps rename the test to `aux-hdr-defaults.yaml`.


================
Comment at: llvm/test/tools/yaml2obj/XCOFF/aux-hdr-omit.yaml:50
+  - Flags:       [ STYP_TEXT ]
+    SectionData: "1234000000"
+  - Flags:       [ STYP_TEXT ]
----------------
Probably fill with non-zero values all the way. Maybe also add a comment why you have two of each section.


================
Comment at: llvm/test/tools/yaml2obj/XCOFF/aux-hdr-omit.yaml:69
+    SectionData: "1234"
+  - Flags:       [ STYP_LOADER ]
+    SectionData: "1234000000"
----------------
DiggerLin wrote:
> in the xcoff document , some section are not multi section allow. https://www.ibm.com/docs/en/aix/7.2?topic=formats-xcoff-object-file-format#XCOFF__a2zjvh2b9shar 
> for  example,  loader section.  but it is a test , it do not matter. 
Remember, this is yaml2obj, so we should be as flexible as possible.


================
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
----------------
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`?


================
Comment at: llvm/test/tools/yaml2obj/XCOFF/aux-hdr-omit.yaml:113
+## Case3: if all fields in the aux header are omitted and text/data/bss/tdata/tbss/loader
+##        sections are not set, we set the fields using defaule values.
+# RUN: yaml2obj %s --docnum=2 -o %t3
----------------



================
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
----------------
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D111487