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

[James Henderson via Phabricator via llvm-commits]

jhenderson added a comment.

Aside from a few nits, and the acknowledged missing error path testing, this seems reasonable to me, but requires someone with XCOFF knowledge to review too.



================
Comment at: llvm/tools/llvm-objcopy/XCOFF/Reader.cpp:52
+  Symbols.reserve(XCOFFObj.getRawNumberOfSymbolTableEntries32());
+  for (const SymbolRef &S : XCOFFObj.symbols()) {
+    Symbol ReadSym;
----------------
`SymbolRef` is designed to be easily copyable. No need for `const &`.


================
Comment at: llvm/tools/llvm-objcopy/XCOFF/Writer.cpp:45
+  uint32_t NumOfSymEnt = 0;
+  for (Symbol &Sym : Obj.Symbols)
+    NumOfSymEnt += 1 + Sym.Sym.NumberOfAuxEntries;
----------------



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

----------------
jhenderson wrote:
> Nit: add new line at EOF.
Now there are too many new lines :D

Delete line 33. There should be a '\n' at the end of line 32, as the last byte in the file.


================
Comment at: llvm/tools/llvm-objcopy/llvm-objcopy.cpp:209
+  } else if (auto *XCOFFBinary = dyn_cast<object::XCOFFObjectFile>(&In)) {
+    Expected<const XCOFFConfig &> XCOFFConfig = Config.getXCOFFConfig();
+    if (!XCOFFConfig)
----------------
Are there likely to be any XCOFF-specific options added in the future? If not, I wonder if we could drop `XCOFFConfig` entirely. We'd still want the check for unsupported options, but perhaps that should be in a different function then (e.g. `Config.checkXCOFFOptions()`). What do you think?

(If there are XCOFF-specific options that are planned, having a separate config, even though empty for now, is fine).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D97656