[llvm-objcopy][MachO] Fix adding multiple sections

[eqv]

Summary

Added a test for the broken behavior and fixed size field in newly added sections.

Test Plan:

in
cd ~/local/toolchain_dev/build/Release/Linux-x86_64/toolchain # run:
ninja check-llvm-tools-llvm-objcopy # run all tests for objcopy
ninja check-llvm # run all LLVM tests ... it's good practice

[smeenai]

Mechanical note: we try to keep the first line of the commit message concise (preferably under 72 characters; some people try to keep it under 50). We also usually add tags in square brackets to indicate the areas the patch touches. A conventional subject line for this commit would be something like:

[llvm-objcopy][MachO] Fix adding multiple sections

llvm/test/tools/llvm-objcopy/MachO/add-multiple-sections.test

1

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.

[MaskRay]

llvm/test/tools/llvm-objcopy/MachO/add-multiple-sections.test

3

Drive-by: I think the test should be folded into add-section-64.test

[eqv]

Any ideas why seemingly unrelated tests are failing? Is this something I should worry about?

[smeenai]

In D90690#2372192, @eqv wrote:

Any ideas why seemingly unrelated tests are failing? Is this something I should worry about?

I wouldn't worry about it. The symtab-sh-info.s failure is a known issue, and D88348 should fix that. Dunno about the other one, but it's pretty clearly unrelated.

[alexander-shaposhnikov]

llvm/test/tools/llvm-objcopy/MachO/add-multiple-sections.test

1

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.

7

nit: Add a new section twice ...

13

nit: Add two new sections ...

55

1. nit: the lines 55-57 are not particularly useful / relevant for this test, I'd start with Sections [ ...

2. CHECK: CHECK-NEXT: ...

[alexander-shaposhnikov]

llvm/test/tools/llvm-objcopy/MachO/add-multiple-sections.test

1

(i meant adding sections into an existing segment ...)

[alexander-shaposhnikov]

llvm/test/tools/llvm-objcopy/MachO/add-multiple-sections.test

3

I was thinking about it too and but now it seems to me that it would be better to keep this test separate.
That test (as well as add-section-32.test) verifies a number of things related to adding sections and I would prefer not to overload it further with extra logic. It makes things a bit easier to maintain.

[eqv]

fixed nitpicks

[eqv]

added test for two different segments

[alexander-shaposhnikov]

besides a small nit this looks reasonable to me, I'd wait for @smeenai to take a look as well

llvm/test/tools/llvm-objcopy/MachO/add-multiple-sections.test

12

nit: i'd probably rename CHECK-A -> CASE1, CHECK-B -> CASE3

[smeenai]

LGTM

[eqv]

fixed CHECK-A => CHECK-1

llvm/test/tools/llvm-objcopy/MachO/add-multiple-sections.test

+## This test verifies that llvm-objcopy can add multiple sections to a Mach-O binary.
+
+# RUN: yaml2obj %s -o %t
+# RUN: echo -n FOOabcdefg > %t.foo.data
+# RUN: echo -n BARabcdefg > %t.bar.data
+
+## 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
+# RUN: llvm-objcopy --add-section __TEXT,__bar=%t.bar.data %t.foo.out %t.foo.bar.out
+# RUN: llvm-readobj --sections --section-data %t.foo.bar.out \
+# RUN:   | FileCheck %s
+
+## 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
+# RUN: llvm-readobj --sections --section-data %t.foo_bar.out \
+# RUN:   | FileCheck %s
+
+--- !mach-o
+FileHeader:
+  magic:           0xFEEDFACF
+  cputype:         0x01000007
+  cpusubtype:      0x00000003
+  filetype:        0x00000001
+  ncmds:           1
+  sizeofcmds:      152
+  flags:           0x00002000
+  reserved:        0x00000000
+LoadCommands:
+  - cmd:             LC_SEGMENT_64
+    cmdsize:         152
+    segname:         __TEXT
+    vmaddr:          0
+    vmsize:          4
+    fileoff:         184
+    filesize:        4
+    maxprot:         7
+    initprot:        7
+    nsects:          1
+    flags:           0
+    Sections:
+      - sectname:        __text
+        segname:         __TEXT
+        addr:            0x0000000000000000
+        content:         'AABBCCDD'
+        size:            4
+        offset:          184
+        align:           0
+        reloff:          0x00000000
+        nreloc:          0
+        flags:           0x80000400
+        reserved1:       0x00000000
+        reserved2:       0x00000000
+        reserved3:       0x00000000
+
+# CHECK:  Format: Mach-O 64-bit x86-64
+# CHECK:  Arch: x86_64
+# CHECK:  AddressSize: 64bit
+# CHECK:  Sections [
+# CHECK:    Section {
+# CHECK:      Index: 0
+# CHECK:      Name: __text (5F 5F 74 65 78 74 00 00 00 00 00 00 00 00 00 00)
+# CHECK:      Segment: __TEXT (5F 5F 54 45 58 54 00 00 00 00 00 00 00 00 00 00)
+# CHECK:      Address: 0x0
+# CHECK:      Size: 0x4
+# CHECK:      Offset: 344
+# CHECK:      Alignment: 0
+# CHECK:      RelocationOffset: 0x0
+# CHECK:      RelocationCount: 0
+# CHECK:      Type: Regular (0x0)
+# CHECK:      Attributes [ (0x800004)
+# CHECK:        PureInstructions (0x800000)
+# CHECK:        SomeInstructions (0x4)
+# CHECK:      ]
+# CHECK:      Reserved1: 0x0
+# CHECK:      Reserved2: 0x0
+# CHECK:      Reserved3: 0x0
+# CHECK:      SectionData (
+# CHECK:        0000: AABBCCDD                             |....|
+# CHECK:      )
+# CHECK:    }
+# CHECK:    Section {
+# CHECK:      Index: 1
+# CHECK:      Name: __foo (5F 5F 66 6F 6F 00 00 00 00 00 00 00 00 00 00 00)
+# CHECK:      Segment: __TEXT (5F 5F 54 45 58 54 00 00 00 00 00 00 00 00 00 00)
+# CHECK:      Address: 0x4
+# CHECK:      Size: 0xA
+# CHECK:      Offset: 348
+# CHECK:      Alignment: 0
+# CHECK:      RelocationOffset: 0x0
+# CHECK:      RelocationCount: 0
+# CHECK:      Type: Regular (0x0)
+# CHECK:      Attributes [ (0x0)
+# CHECK:      ]
+# CHECK:      Reserved1: 0x0
+# CHECK:      Reserved2: 0x0
+# CHECK:      Reserved3: 0x0
+# CHECK:      SectionData (
+# CHECK:        0000: 464F4F61 62636465 6667               |FOOabcdefg|
+# CHECK:      )
+# CHECK:    }
+# CHECK:    Section {
+# CHECK:      Index: 2
+# CHECK:      Name: __bar (5F 5F 62 61 72 00 00 00 00 00 00 00 00 00 00 00)
+# CHECK:      Segment: __TEXT (5F 5F 54 45 58 54 00 00 00 00 00 00 00 00 00 00)
+# CHECK:      Address: 0xE
+# CHECK:      Size: 0xA
+# CHECK:      Offset: 358
+# CHECK:      Alignment: 0
+# CHECK:      RelocationOffset: 0x0
+# CHECK:      RelocationCount: 0
+# CHECK:      Type: Regular (0x0)
+# CHECK:      Attributes [ (0x0)
+# CHECK:      ]
+# CHECK:      Reserved1: 0x0
+# CHECK:      Reserved2: 0x0
+# CHECK:      Reserved3: 0x0
+# CHECK:      SectionData (
+# CHECK:        0000: 42415261 62636465 6667               |BARabcdefg|
+# CHECK:      )
+# CHECK:    }
+# CHECK:  ]

llvm/tools/llvm-objcopy/MachO/MachOObjcopy.cpp

static Error addSection(StringRef SecName, StringRef Filename, Object &Obj) {
   if (!BufOrErr)
     return createFileError(Filename, errorCodeToError(BufOrErr.getError()));
   std::unique_ptr<MemoryBuffer> Buf = std::move(*BufOrErr);
 
   std::pair<StringRef, StringRef> Pair = SecName.split(',');
   StringRef TargetSegName = Pair.first;
   Section Sec(TargetSegName, Pair.second);
   Sec.Content = Obj.NewSectionsContents.save(Buf->getBuffer());
+  Sec.Size = Sec.Content.size();
 
   // Add the a section into an existing segment.
   for (LoadCommand &LC : Obj.LoadCommands) {
     Optional<StringRef> SegName = LC.getSegmentName();
     if (SegName && SegName == TargetSegName) {
       uint64_t Addr = *LC.getSegmentVMAddr();
       for (const std::unique_ptr<Section> &S : LC.Sections)
         Addr = std::max(Addr, S->Addr + S->Size);