[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:245-246
+  // Set these section-related values if not set explicitly. We assume that the
+  // input YAML matches the format of the loadable object. But if input sections
+  // still have same type, the first section with that type prevails.
+  for (uint16_t I = 0, E = InitSections.size(); I < E; ++I) {
----------------



================
Comment at: llvm/lib/ObjectYAML/XCOFFEmitter.cpp:250-251
+    case XCOFF::STYP_TEXT:
+      if (!InitAuxFileHdr.TextSize)
+        InitAuxFileHdr.TextSize = InitSections[I].Size;
+      if (!InitAuxFileHdr.TextStartAddr)
----------------
I just remembered that `llvm::Optional` has a `getValueOr(T Value)` function which gives you the input parameter, if the optional is none, or the stored value otherwise. I think that would make the code throughout much simpler.


================
Comment at: llvm/lib/ObjectYAML/XCOFFEmitter.cpp:347
+void XCOFFWriter::writeAuxFileHeader() {
+  W.write<uint16_t>(InitAuxFileHdr.Magic ? (uint16_t)*InitAuxFileHdr.Magic : 1);
+  W.write<uint16_t>(InitAuxFileHdr.Version ? (uint16_t)*InitAuxFileHdr.Version
----------------
Do you actually need the casts? You've explicitly specified the type for `write` after all. If you do need a cast, use `static_cast`.


================
Comment at: llvm/lib/ObjectYAML/XCOFFEmitter.cpp:465
+    W.write<uint16_t>(InitAuxFileHdr.Flag ? (uint16_t)*InitAuxFileHdr.Flag
+                                          : 32768);
+    if (InitFileHdr.AuxHeaderSize > XCOFF::AuxFileHeaderSize64)
----------------
Give the `32768` magic number a name, i.e. put it in a variable or constant somewhere. Flags are also usually written as hex in the source code, which might help make things clearer.


================
Comment at: llvm/test/tools/yaml2obj/XCOFF/aux-hdr-32.yaml:8
+# CHECK-NEXT:   Version: 0x1
+# CHECK-NEXT:   Size of .text section: 0x6
+# CHECK-NEXT:   Size of .data section: 0x15
----------------
This and the 64-bit test need to show that the value isn't taken from the sections if present, i.e. they need to have at least one section of each type with values different to the explicitly specified values in the YAML.


================
Comment at: llvm/test/tools/yaml2obj/XCOFF/aux-hdr-omit.yaml:1
+## Test that yaml2obj automatically sets omited fields of the auxiliary file header if not set explicitly.
+# RUN: yaml2obj %s -o %t
----------------
I think you need a 32-bit and 64-bit version of this test. In other words, show what the default is for both formats. Does that make sense to you?

I think you also need to show what happens when there are no text/data/bss sections etc from which to derive the values if not specified.


================
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
----------------
I might be being dumb, but the sizes below look like 2 bytes to me.


================
Comment at: llvm/test/tools/yaml2obj/XCOFF/aux-hdr-omit.yaml:47
+  - Flags:       [ STYP_TEXT ]
+    SectionData: "34"
+  - Flags:       [ STYP_DATA ]
----------------
Assuming that a single section contributes to the field, you should use a different size here, to show that the right one is picked.


================
Comment at: llvm/test/tools/yaml2obj/XCOFF/aux-hdr-omit.yaml:48-49
+    SectionData: "34"
+  - Flags:       [ STYP_DATA ]
+    SectionData: "56"
+  - Flags:       [ STYP_BSS ]
----------------
Don't you need duplicates of this and some of the other sections, like .text, to show that the first one wins in all cases?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D111487