[PATCH] D66283: [llvm-objcopy][MachO] Implement --add-section

[Jordan Rupprecht via Phabricator via llvm-commits]

rupprecht added a comment.

Don't think I have any comments besides these two, so LGTM once they're addressed & you get a MachO reviewer to be happy w/ this.



================
Comment at: llvm/tools/llvm-objcopy/MachO/MachOReader.cpp:33
+  StringRef Sectname(Sec.sectname, strnlen(Sec.sectname, sizeof(Sec.sectname)));
+  StringRef Segname(Sec.segname, strnlen(Sec.segname, sizeof(Sec.sectname)));
+  Section S(Segname, Sectname);
----------------
`sizeof(Sec.sectname)` should be `sizeof(Sec.segname)` here?
(They're the same size, fortunately)


================
Comment at: llvm/tools/llvm-objcopy/MachO/Object.h:76-81
+  StringRef getContent() const {
+    if (OwnedContentData.size() > 0)
+      return StringRef(reinterpret_cast<const char *>(OwnedContentData.data()),
+                       OwnedContentData.size());
+    return Content;
+  }
----------------
A common use of GNU objcopy is to run with `--add-section .note.GNU-stack=/dev/null`, which I imagine corresponds to a 0-sized section. I think you might want to wrap `OwnedContentData` in `Optional<>`. (Assuming a zero-sized section is valid in Mach-O). Additionally, can you add a test for --add-section w/ /dev/null?

The ELF code doesn't use `Optional` because it takes a different approach by having subclasses for different types of sections, so `OwnedDataSection` is different from `Section` -- I don't know if that makes sense here


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D66283