[PATCH] D54674: [llvm-objcopy] First bits for MachO

[Jake Ehrlich via Phabricator via llvm-commits]

jakehehrlich added a comment.

I had a few bike-shed level comments. I'll dig into the code more deeply soon but my near complete lack of knowledge of MachO makes giving any actually useful feedback hard.

The high level structure looks good to me. Looks like we're avoiding lots of mistakes I made in the ELF code base which is good.



================
Comment at: tools/llvm-objcopy/MachO/MachOObjcopy.cpp:21
+  MachOReader Reader(In);
+  std::unique_ptr<Object> O = Reader.create();
+  assert(O && "Unable to deserialize MachO object");
----------------
Where is Object forward declared to make this valid?


================
Comment at: tools/llvm-objcopy/MachO/Object.h:73
+
+struct NListEntry {
+  uint32_t n_strx;
----------------
This appears to be a maximally sized version of something that already exists in BianryFormat for MachO. Can we use expanded names instead of mimicked names?


================
Comment at: tools/llvm-objcopy/MachO/Object.h:90
+struct StringTable {
+  std::vector<std::string> Strings;
+};
----------------
Early on I took this approach for ELF as well but it wound up being a mistake for ELF. 1) Something already existed that would construct string tables for me and 2) It didn't handle sharing correctly. Do these issues not occur in MachO?


================
Comment at: tools/llvm-objcopy/MachO/Object.h:200
+  /// The index of LC_SYMTAB load command if present.
+  Optional<size_t> SymTabCommandIndex;
+  /// The index of LC_DYLD_INFO or LC_DYLD_INFO_ONLY load command if present.
----------------
For indexes would it be better to use uint64_t instead of size_t?


================
Comment at: tools/llvm-objcopy/llvm-objcopy.cpp:130
     return elf::executeObjcopyOnBinary(Config, *ELFBinary, Out);
+  else if (auto *MachOBinary = dyn_cast<object::MachOObjectFile>(&In))
+    return macho::executeObjcopyOnBinary(Config, *MachOBinary, Out);
----------------
Do we want to be silent on mixed ELF/MachO archives? It's an absurd case that I don't suspect will ever happen it was just a thought I had. I'm fine leaving this as is.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D54674