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

[Esme Yi via Phabricator via llvm-commits]

Esme added inline comments.


================
Comment at: llvm/lib/ObjectYAML/XCOFFEmitter.cpp:250-251
+    case XCOFF::STYP_TEXT:
+      if (!InitAuxFileHdr.TextSize)
+        InitAuxFileHdr.TextSize = InitSections[I].Size;
+      if (!InitAuxFileHdr.TextStartAddr)
----------------
jhenderson wrote:
> 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.
Thanks!
I used the function `getValueOr(T Value)` in `XCOFFWriter::writeAuxFileHeader()`, which make the code much simpler.
While I didn't use it here because I think it would be better to use the `if` condition to bail out early rather than call `getValueOr()` every time. As well as it will not make the code simpler here.


================
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
----------------
jhenderson wrote:
> 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`.
The casts were necessary in the comparison expressions.
However I don't need them anymore because the `getValueOr(T Value)` is adopted now.


================
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
----------------
jhenderson wrote:
> 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.
Yes! The suggestions make sense to me. Thanks!


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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D111487