[PATCH] D95505: [yaml2obj] Initial support for 32-bit XCOFF in yaml2obj.

[James Henderson via Phabricator via llvm-commits]

jhenderson added inline comments.


================
Comment at: llvm/lib/ObjectYAML/XCOFFEmitter.cpp:13
+//===----------------------------------------------------------------------===//
+#include "llvm/ADT/DenseMap.h"
+#include "llvm/BinaryFormat/XCOFF.h"
----------------
Nit: LLVM usually has a new line between the licence header and the `#include` list (based on a quick look at 3 or 4 files).


================
Comment at: llvm/lib/ObjectYAML/XCOFFEmitter.cpp:67
+bool XCOFFWriter::nameShouldBeInStringTable(StringRef SymbolName) {
+  return (SymbolName.size() > XCOFF::NameSize);
+}
----------------
You can drop the parentheses. Do you need to allow space for a null terminator, or does `NameSize` take that into account?


================
Comment at: llvm/lib/ObjectYAML/XCOFFEmitter.cpp:126-128
+  if (!initRelocations(CurrentOffset))
+    return false;
+  return true;
----------------
How about simply `return initRelocations(CurrentOffset);`?


================
Comment at: llvm/lib/ObjectYAML/XCOFFEmitter.cpp:293
+  if (Is64Bit) {
+    ErrHandler("only XCOFF32 is supported now");
+    return false;
----------------
Nit: the suggested inline edit sounds a bit better to me.


================
Comment at: llvm/test/tools/yaml2obj/XCOFF/basic-doc.yaml:1
+# Check that yaml2obj automatically assigns ommited fields with values.
+# RUN: yaml2obj %s -o %t
----------------
Nit: newer tests I'm involved with at least tend to use '##' for comments, to make them stand out from lit and FileCheck lines.


================
Comment at: llvm/test/tools/yaml2obj/XCOFF/basic-doc.yaml:11
+  - Name:          .text
+    Flags:         0x20
+    SectionData:   "9061FFF8808200008064000038630001906400008061FFF8386300019061FFF88061FFF88082000480840000888400007C6322149061FFF88061FFF84E8000200000000000092040800001010000000000000040000466756E310000600000007C0802A693E1FFFC900100089421FFB07C3F0B7838600000907F004880620008907F0044808300003884000190830000806300004BFFFF6D60000000807F0044806300004BFFFF5D60000000382100508001000883E1FFFC7C0803A64E8000200000000000092261800100010000006000046D61696E1F006162636400000000"
----------------
Is there an enum you could use to represent this set of flags? It would be preferable to be able write either of the following (flag values are placeholders):
```
Flags: Exec
```
or probably
```
Flags: [Exec, Alloc]
```
although `Flags: 0x20` (or possibly `Flags: [0x20]`) should probably still be permitted.


================
Comment at: llvm/test/tools/yaml2obj/XCOFF/basic-doc.yaml:12
+    Flags:         0x20
+    SectionData:   "9061FFF8808200008064000038630001906400008061FFF8386300019061FFF88061FFF88082000480840000888400007C6322149061FFF88061FFF84E8000200000000000092040800001010000000000000040000466756E310000600000007C0802A693E1FFFC900100089421FFB07C3F0B7838600000907F004880620008907F0044808300003884000190830000806300004BFFFF6D60000000807F0044806300004BFFFF5D60000000382100508001000883E1FFFC7C0803A64E8000200000000000092261800100010000006000046D61696E1F006162636400000000"
+  - Name:          .data
----------------
It seems to me you could get away with much less data in this section (probably a half-dozen bytes)? I don't think this needs to be a real object for this test case? Same below.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D95505