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

[James Henderson via Phabricator via llvm-commits]

jhenderson added a comment.

I don't know anything about the MachO file format really, so I'm not sure I'm well qualified to review the file format side of this. I've made a couple of drive-by nitpick comments though, and may come back later with more substantial comments.

One general review-related comment, is this the minimal required to get a trivial MachO file copied? If it isn't, it might be nice to ignore the unnecessary components for this first commit. Of course, this assumes that we are thinking ahead to dependency issues, so that we don't end up with the issues we had with ELF support.

One more general comment I have, that doesn't really affect this change, but will impact later things, is that I suspect adding MachO support will require lots of new switches. If these switches aren't applicable to ELF, I'd like us to consider how we can sanitize the help text, so that ELF-only options and MachO-only options are not visible to users of the other version. I'm not sure how to achieve that necessarily though. In LLD and llvm-readobj, the executable name has an effect on option support. We already do something similar with llvm-strip versus llvm-objcopy too. One possibility might be to do the same here (e.g. llvm-elfcopy for ELF switches, llvm-machocopy for MachO switches, and llvm-objcopy for all). Alternatively, if that's not really viable, or possibly in addition to this,  I'd support having some easy way of disabling (or at least hiding) all options related to a specific file format, so that downstream ports don't have to worry about file formats that they don't care about.



================
Comment at: test/tools/llvm-objcopy/MachO/basic-copy.test:1
+# RUN: yaml2obj %s > %t
+# RUN: llvm-objcopy %t %t2
----------------
May be worth naming this file basic-little-endian-copy.test.

I take it MachO doesn't have 32/64 bit variations?


================
Comment at: tools/llvm-objcopy/MachO/Object.h:93
+
+/**
+ * The location of the rebase info inside the binary is described by
----------------
My reading of the LLVM coding standards suggests that we should only be using C++ style comments here, rather than C-style.


================
Comment at: tools/llvm-objcopy/llvm-objcopy.cpp:27
+#include "llvm/Object/MachO.h"
+
 #include "llvm/Option/Arg.h"
----------------
Spurious blank line?


Repository:
  rL LLVM

https://reviews.llvm.org/D54674