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

[ChenZheng via Phabricator via llvm-commits]

shchenz added a comment.

Thanks for adding objcopy support on AIX.

Some comments from me.

I have not finished all of the code reviewing yet.



================
Comment at: llvm/include/llvm/Object/XCOFFObjectFile.h:339
   int32_t getTimeStamp() const;
+  const XCOFFFileHeader32 *getFileHeader32() const;
 
----------------
I think it should be ok to call `fileHeader32` directly in the Reader?


================
Comment at: llvm/include/llvm/Object/XCOFFObjectFile.h:368
+  Expected<ArrayRef<uint8_t>>
+  getSectionContents(const XCOFFSectionHeader32 &Sec) const;
 
----------------
We already have a `getSectionContents` API in this class, can we reuse that function?


================
Comment at: llvm/tools/llvm-objcopy/XCOFF/Reader.cpp:19
+  ArrayRef<XCOFFSectionHeader32> Sections = XCOFFObj.sections32();
+  for (XCOFFSectionHeader32 Sec : Sections) {
+    Section ReadSec;
----------------
use `&`?


================
Comment at: llvm/tools/llvm-objcopy/XCOFF/XCOFFObjcopy.cpp:39
+        llvm::errc::invalid_argument,
+        "no flags are supported yet, only basic copying is allowed");
+  }
----------------
>From this comments, seems for now llvm-objcopy for XCOFF does not take any options? Even for `-h` or `-v`?
If so, I think the options list here should not be complete. For example, we miss `StripDebug` and there are also many others.

Maybe we need a new method to tell whether an option is specified instead of listing all of them.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D97656