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

[James Henderson via Phabricator via llvm-commits]

jhenderson added inline comments.


================
Comment at: llvm/test/tools/llvm-objcopy/MachO/add-section.test:4
+
+# RUN: yaml2obj --docnum=1 %s > %t
+# RUN: yaml2obj --docnum=2 %s > %t.32bit
----------------
jhenderson wrote:
> By the way, there seems to be a degree of consensus that `-o` should be preferred to a shell redirection with yaml2obj. No need to modify old tests, but if you're adding new ones/updating them, it's probably good to make this change too.
It probably makes sense to call `%t` `%t.64bit`.


================
Comment at: llvm/test/tools/llvm-objcopy/MachO/add-section.test:4-5
+
+# RUN: yaml2obj --docnum=1 %s > %t
+# RUN: yaml2obj --docnum=2 %s > %t.32bit
+# RUN: echo -n abcdefg > %t.data
----------------
By the way, there seems to be a degree of consensus that `-o` should be preferred to a shell redirection with yaml2obj. No need to modify old tests, but if you're adding new ones/updating them, it's probably good to make this change too.


================
Comment at: llvm/test/tools/llvm-objcopy/MachO/add-section.test:19
+## Case 1: Add a new section into an existing segment.
+# RUN: llvm-objcopy --add-section __TEXT,__text=%t.data %t %t.out1
+# RUN: llvm-objcopy --add-section __TEXT,__text=%t.data %t.32bit %t.out1.32bit
----------------
`%t.out1` -> `%t.out1.64bit` or something along those lines


================
Comment at: llvm/tools/llvm-objcopy/MachO/Object.cpp:45-49
+    MachO::segment_command &Seg = LC.MachOLoadCommand.segment_command_data;
+    assert(SegName.size() <= sizeof(Seg.segname) && "too long segment name");
+    memset(&Seg, 0, sizeof(MachO::segment_command));
+    Seg.cmd = MachO::LC_SEGMENT;
+    strncpy(Seg.segname, SegName.data(), SegName.size());
----------------
This and the above block are very similar. You might want to consider pulling them into some kind of function to reduce the duplication.


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

https://reviews.llvm.org/D66283