[PATCH] D97656: [llvm-objcopy] Initial XCOFF32 support.

[James Henderson via Phabricator via llvm-commits]

jhenderson added a comment.

You need testing for your error paths, I think.



================
Comment at: llvm/test/tools/llvm-objcopy/XCOFF/basic-copy.test:7
+FileHeader:
+  MagicNumber:     0x1DF
+Sections:
----------------



================
Comment at: llvm/test/tools/llvm-objcopy/XCOFF/basic-copy.test:9-18
+  - Name:          .text
+    Flags:         0x20
+    SectionData:   "9061FFF8808200008064000038630001906400008061FFF8386300019061FFF88061FFF88082000480840000888400007C6322149061FFF88061FFF84E8000200000000000092040800001010000000000000040000466756E310000600000007C0802A693E1FFFC900100089421FFB07C3F0B7838600000907F004880620008907F0044808300003884000190830000806300004BFFFF6D60000000807F0044806300004BFFFF5D60000000382100508001000883E1FFFC7C0803A64E8000200000000000092261800100010000006000046D61696E1F006162636400000000"
+  - Name:          .data
+    Flags:         0x40
+    SectionData:   "00000000000000FC0000000000000060000000FC00000000000000D800000108000000F800000000"
+    Relocations:
----------------
Maybe add a comment to the final section consisting solely of "Flags" to clarify that it is a nameless and empty section? It might make more sense to put the empty section earlier in the sections too, to show the impact (or lack thereof) it has on the later section offsets.


================
Comment at: llvm/test/tools/llvm-objcopy/XCOFF/basic-copy.test:11
+    Flags:         0x20
+    SectionData:   "9061FFF8808200008064000038630001906400008061FFF8386300019061FFF88061FFF88082000480840000888400007C6322149061FFF88061FFF84E8000200000000000092040800001010000000000000040000466756E310000600000007C0802A693E1FFFC900100089421FFB07C3F0B7838600000907F004880620008907F0044808300003884000190830000806300004BFFFF6D60000000807F0044806300004BFFFF5D60000000382100508001000883E1FFFC7C0803A64E8000200000000000092261800100010000006000046D61696E1F006162636400000000"
+  - Name:          .data
----------------
Do we really need this much data? Seems to me like a few arbitrary bytes (e.g. "12345678") would be sufficient.


================
Comment at: llvm/tools/llvm-objcopy/XCOFF/Reader.cpp:41
+        return Relocations.takeError();
+      for (XCOFFRelocation32 Reloc : Relocations.get())
+        ReadSec.Relocations.push_back(Reloc);
----------------
Should this be a `const &`?


================
Comment at: llvm/tools/llvm-objcopy/XCOFF/Writer.cpp:19-21
+  size_t FileSize = 0;
+  // Finalize file headers.
+  FileSize += XCOFF::FileHeaderSize32;
----------------
Why not as inline?


================
Comment at: llvm/tools/llvm-objcopy/XCOFF/Writer.cpp:20
+  size_t FileSize = 0;
+  // Finalize file headers.
+  FileSize += XCOFF::FileHeaderSize32;
----------------
"header" rather than "headers" maybe? Is there more than one file header after all?


================
Comment at: llvm/tools/llvm-objcopy/XCOFF/Writer.cpp:25
+
+  // Finalize sections.
+  FileSize += XCOFF::SectionHeaderSize32 * Obj.Sections.size();
----------------
Rather than these comments, maybe it would be clearer if you split this function into multiple functions, e.g. `finalizeHeader`, `finalizeSections` etc.


================
Comment at: llvm/tools/llvm-objcopy/XCOFF/Writer.cpp:64
+
+  // Write the file header.
+  uint8_t *Ptr = reinterpret_cast<uint8_t *>(Buf->getBufferStart());
----------------
Similar to above, I'd move these blocks into separate functions. That way, if they need to increase in complexity here, it's easy to do so.


================
Comment at: llvm/tools/llvm-objcopy/XCOFF/Writer.cpp:86
+          Sec.SectionHeader.FileOffsetToRelocationInfo;
+    for (XCOFFRelocation32 &Relo : Sec.Relocations) {
+      memcpy(Ptr, &Relo, sizeof(XCOFFRelocation32));
----------------
`Rel` or `Reloc` are generally more common abbreviations for "Relocation" in other parts of the code. I'd recommend renaming here to match.


================
Comment at: llvm/tools/llvm-objcopy/XCOFF/XCOFFObjcopy.h:32
+#endif // LLVM_TOOLS_LLVM_OBJCOPY_XCOFFOBJCOPY_H
\ No newline at end of file

----------------
Nit: add new line at EOF.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D97656