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

[Jordan Rupprecht via Phabricator via llvm-commits]

rupprecht added inline comments.


================
Comment at: llvm/tools/llvm-objcopy/MachO/MachOObjcopy.cpp:68-76
+static Error addSection(StringRef SecName, StringRef Filename, Object &Obj) {
+  ErrorOr<std::unique_ptr<MemoryBuffer>> BufOrErr =
+      MemoryBuffer::getFile(Filename);
+  if (!BufOrErr)
+    return createFileError(Filename, errorCodeToError(BufOrErr.getError()));
+  std::unique_ptr<MemoryBuffer> Buf = std::move(*BufOrErr);
+  ArrayRef<uint8_t> Content(
----------------
This boilerplate should be handled in CopyConfig instead of copied in every platform-specific implementation, can you add a FIXME to consolidate it?


================
Comment at: llvm/tools/llvm-objcopy/MachO/MachOObjcopy.cpp:80
+  StringRef TargetSegName = Pair.first;
+  Section Sec(TargetSegName, SecName);
+  Sec.setOwnedContentData(Content);
----------------
`SecName` is actually the pair of <segment>,<section>, so shouldn't `SecName` be replaced with `Pair.second`? I think the test case you have shows this.


================
Comment at: llvm/tools/llvm-objcopy/MachO/MachOObjcopy.cpp:96
+  if (!NewSegment)
+    return createFileError(Filename, errorCodeToError(BufOrErr.getError()));
+  NewSegment->Sections.push_back(Sec);
----------------
If the segment can't be created, it seems like it's not at all related to the file content, so either the filename here should be the file we're trying to add a segment to, or it should be a regular StringError.


================
Comment at: llvm/tools/llvm-objcopy/MachO/Object.cpp:44-46
+    return StringRef(MLC.segment_command_data.segname,
+                     strnlen(MLC.segment_command_data.segname,
+                             sizeof(MLC.segment_command_data.segname)));
----------------
Is there a utility you could extract to convert from possibly-null-delimited segment/section names to StringRefs? This pattern is used above with the strncpy and also in MachOReader


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D66283