[PATCH] D90690: [llvm-objcopy][MachO] Fix adding multiple sections

[Alexander Shaposhnikov via Phabricator via llvm-commits]

alexshap added inline comments.


================
Comment at: llvm/test/tools/llvm-objcopy/MachO/add-multiple-sections.test:1
+## This test verifies that llvm-objcopy can add multiple sections to a Mach-O binary.
+
----------------
smeenai wrote:
> I'd also add a test for adding two new sections into two different segments.
> 
> There's a couple of more tests for `--add-section` that would be good to have, but fall outside the scope of this diff (they're better suited for `add-section-error.test`). I'm also note sure what the desired behavior for them is (maybe @alexshap would know):
> * Issuing multiple `--add-section` commands with the same section name.
> * Attempting to add a section with the same name and segment as an existing section.
> I'd also add a test for adding two new sections into two different segments.
  +1

> Issuing multiple --add-section commands with the same section name.
I'm not sure either, e.g. I've done a quick experiment on OSX -  created a binary with multiple sections having the same name (and belonging to the same segment) and it appears to work fine (in particular, the dynamic loader doesn't complain). There are other potential issues here which would be good to detect (e.g. adding sections into an existing binary can actually break it (e.g. segments might start to overlap), but I think this is out of scope for the current diff, the proposed fix (in MachOObject.cpp) is actually very useful.



================
Comment at: llvm/test/tools/llvm-objcopy/MachO/add-multiple-sections.test:7
+
+## Case 1: Add a new sections twice into an existing segment.
+# RUN: llvm-objcopy --add-section __TEXT,__foo=%t.foo.data %t %t.foo.out
----------------
nit: Add a new section twice ...


================
Comment at: llvm/test/tools/llvm-objcopy/MachO/add-multiple-sections.test:13
+
+## Case 2: Add a two new sections into an existing segment.
+# RUN: llvm-objcopy --add-section __TEXT,__foo=%t.foo.data --add-section __TEXT,__bar=%t.bar.data %t %t.foo_bar.out
----------------
nit: Add two new sections ...


================
Comment at: llvm/test/tools/llvm-objcopy/MachO/add-multiple-sections.test:55
+
+# CHECK:  Format: Mach-O 64-bit x86-64
+# CHECK:  Arch: x86_64
----------------
1. nit: the lines 55-57 are not particularly useful / relevant for this test, I'd start with Sections [ ...

2. CHECK: 
    CHECK-NEXT: ...


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D90690