[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

2

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

4

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

2

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.

8

nit: Add a new section twice ...

14

nit: Add two new sections ...

56

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

2

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

[alexander-shaposhnikov]

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

4

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 section twice into an existing segment.
+# RUN: llvm-objcopy --add-section __TEXT,__foo=%t.foo.data %t %t.foo.1
+# RUN: llvm-objcopy --add-section __TEXT,__bar=%t.bar.data %t.foo.1 %t.foo.bar.1
+# RUN: llvm-readobj --sections --section-data %t.foo.bar.1 \
+# RUN:   | FileCheck %s -check-prefix=CASE1
+
+## Case 2: Add 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.2
+# RUN: llvm-readobj --sections --section-data %t.foo.bar.2 \
+# RUN:   | FileCheck %s -check-prefix=CASE1
+
+## Case 3: Add two new sections into two different segments.
+# RUN: llvm-objcopy --add-section __FOO,__foo=%t.foo.data --add-section __BAR,__bar=%t.bar.data %t %t.foo.bar.3
+# RUN: llvm-readobj --sections --section-data %t.foo.bar.3 \
+# RUN:   | FileCheck %s -check-prefix=CASE3
+
+--- !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
+
+# CASE1:        Sections [
+# CASE1-NEXT:    Section {
+# CASE1-NEXT:      Index: 0
+# CASE1-NEXT:      Name: __text (5F 5F 74 65 78 74 00 00 00 00 00 00 00 00 00 00)
+# CASE1-NEXT:      Segment: __TEXT (5F 5F 54 45 58 54 00 00 00 00 00 00 00 00 00 00)
+# CASE1-NEXT:      Address: 0x0
+# CASE1-NEXT:      Size: 0x4
+# CASE1-NEXT:      Offset: 344
+# CASE1-NEXT:      Alignment: 0
+# CASE1-NEXT:      RelocationOffset: 0x0
+# CASE1-NEXT:      RelocationCount: 0
+# CASE1-NEXT:      Type: Regular (0x0)
+# CASE1-NEXT:      Attributes [ (0x800004)
+# CASE1-NEXT:        PureInstructions (0x800000)
+# CASE1-NEXT:        SomeInstructions (0x4)
+# CASE1-NEXT:      ]
+# CASE1-NEXT:      Reserved1: 0x0
+# CASE1-NEXT:      Reserved2: 0x0
+# CASE1-NEXT:      Reserved3: 0x0
+# CASE1-NEXT:      SectionData (
+# CASE1-NEXT:        0000: AABBCCDD                             |....|
+# CASE1-NEXT:      )
+# CASE1-NEXT:    }
+# CASE1-NEXT:    Section {
+# CASE1-NEXT:      Index: 1
+# CASE1-NEXT:      Name: __foo (5F 5F 66 6F 6F 00 00 00 00 00 00 00 00 00 00 00)
+# CASE1-NEXT:      Segment: __TEXT (5F 5F 54 45 58 54 00 00 00 00 00 00 00 00 00 00)
+# CASE1-NEXT:      Address: 0x4
+# CASE1-NEXT:      Size: 0xA
+# CASE1-NEXT:      Offset: 348
+# CASE1-NEXT:      Alignment: 0
+# CASE1-NEXT:      RelocationOffset: 0x0
+# CASE1-NEXT:      RelocationCount: 0
+# CASE1-NEXT:      Type: Regular (0x0)
+# CASE1-NEXT:      Attributes [ (0x0)
+# CASE1-NEXT:      ]
+# CASE1-NEXT:      Reserved1: 0x0
+# CASE1-NEXT:      Reserved2: 0x0
+# CASE1-NEXT:      Reserved3: 0x0
+# CASE1-NEXT:      SectionData (
+# CASE1-NEXT:        0000: 464F4F61 62636465 6667               |FOOabcdefg|
+# CASE1-NEXT:      )
+# CASE1-NEXT:    }
+# CASE1-NEXT:    Section {
+# CASE1-NEXT:      Index: 2
+# CASE1-NEXT:      Name: __bar (5F 5F 62 61 72 00 00 00 00 00 00 00 00 00 00 00)
+# CASE1-NEXT:      Segment: __TEXT (5F 5F 54 45 58 54 00 00 00 00 00 00 00 00 00 00)
+# CASE1-NEXT:      Address: 0xE
+# CASE1-NEXT:      Size: 0xA
+# CASE1-NEXT:      Offset: 358
+# CASE1-NEXT:      Alignment: 0
+# CASE1-NEXT:      RelocationOffset: 0x0
+# CASE1-NEXT:      RelocationCount: 0
+# CASE1-NEXT:      Type: Regular (0x0)
+# CASE1-NEXT:      Attributes [ (0x0)
+# CASE1-NEXT:      ]
+# CASE1-NEXT:      Reserved1: 0x0
+# CASE1-NEXT:      Reserved2: 0x0
+# CASE1-NEXT:      Reserved3: 0x0
+# CASE1-NEXT:      SectionData (
+# CASE1-NEXT:        0000: 42415261 62636465 6667               |BARabcdefg|
+# CASE1-NEXT:      )
+# CASE1-NEXT:    }
+# CASE1-NEXT:  ]
+
+# CASE3:        Sections [
+# CASE3-NEXT:    Section {
+# CASE3-NEXT:      Index: 0
+# CASE3-NEXT:      Name: __text (5F 5F 74 65 78 74 00 00 00 00 00 00 00 00 00 00)
+# CASE3-NEXT:      Segment: __TEXT (5F 5F 54 45 58 54 00 00 00 00 00 00 00 00 00 00)
+# CASE3-NEXT:      Address: 0x0
+# CASE3-NEXT:      Size: 0x4
+# CASE3-NEXT:      Offset: 488
+# CASE3-NEXT:      Alignment: 0
+# CASE3-NEXT:      RelocationOffset: 0x0
+# CASE3-NEXT:      RelocationCount: 0
+# CASE3-NEXT:      Type: Regular (0x0)
+# CASE3-NEXT:      Attributes [ (0x800004)
+# CASE3-NEXT:        PureInstructions (0x800000)
+# CASE3-NEXT:        SomeInstructions (0x4)
+# CASE3-NEXT:      ]
+# CASE3-NEXT:      Reserved1: 0x0
+# CASE3-NEXT:      Reserved2: 0x0
+# CASE3-NEXT:      Reserved3: 0x0
+# CASE3-NEXT:      SectionData (
+# CASE3-NEXT:        0000: AABBCCDD                             |....|
+# CASE3-NEXT:      )
+# CASE3-NEXT:    }
+# CASE3-NEXT:    Section {
+# CASE3-NEXT:      Index: 1
+# CASE3-NEXT:      Name: __foo (5F 5F 66 6F 6F 00 00 00 00 00 00 00 00 00 00 00)
+# CASE3-NEXT:      Segment: __FOO (5F 5F 46 4F 4F 00 00 00 00 00 00 00 00 00 00 00)
+# CASE3-NEXT:      Address: 0xB8
+# CASE3-NEXT:      Size: 0xA
+# CASE3-NEXT:      Offset: 492
+# CASE3-NEXT:      Alignment: 0
+# CASE3-NEXT:      RelocationOffset: 0x0
+# CASE3-NEXT:      RelocationCount: 0
+# CASE3-NEXT:      Type: Regular (0x0)
+# CASE3-NEXT:      Attributes [ (0x0)
+# CASE3-NEXT:      ]
+# CASE3-NEXT:      Reserved1: 0x0
+# CASE3-NEXT:      Reserved2: 0x0
+# CASE3-NEXT:      Reserved3: 0x0
+# CASE3-NEXT:      SectionData (
+# CASE3-NEXT:        0000: 464F4F61 62636465 6667               |FOOabcdefg|
+# CASE3-NEXT:      )
+# CASE3-NEXT:    }
+# CASE3-NEXT:    Section {
+# CASE3-NEXT:      Index: 2
+# CASE3-NEXT:      Name: __bar (5F 5F 62 61 72 00 00 00 00 00 00 00 00 00 00 00)
+# CASE3-NEXT:      Segment: __BAR (5F 5F 42 41 52 00 00 00 00 00 00 00 00 00 00 00)
+# CASE3-NEXT:      Address: 0x40B8
+# CASE3-NEXT:      Size: 0xA
+# CASE3-NEXT:      Offset: 502
+# CASE3-NEXT:      Alignment: 0
+# CASE3-NEXT:      RelocationOffset: 0x0
+# CASE3-NEXT:      RelocationCount: 0
+# CASE3-NEXT:      Type: Regular (0x0)
+# CASE3-NEXT:      Attributes [ (0x0)
+# CASE3-NEXT:      ]
+# CASE3-NEXT:      Reserved1: 0x0
+# CASE3-NEXT:      Reserved2: 0x0
+# CASE3-NEXT:      Reserved3: 0x0
+# CASE3-NEXT:      SectionData (
+# CASE3-NEXT:        0000: 42415261 62636465 6667               |BARabcdefg|
+# CASE3-NEXT:      )
+# CASE3-NEXT:    }
+# CASE3-NEXT:  ]
+

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);
       LC.Sections.push_back(std::make_unique<Section>(Sec));
       LC.Sections.back()->Addr = Addr;
       return Error::success();
     }
   }
 
   // There's no segment named TargetSegName. Create a new load command and
   // Insert a new section into it.
-  LoadCommand &NewSegment = Obj.addSegment(TargetSegName);
+  LoadCommand &NewSegment =
+      Obj.addSegment(TargetSegName, alignTo(Sec.Size, 16384));
   NewSegment.Sections.push_back(std::make_unique<Section>(Sec));
   NewSegment.Sections.back()->Addr = *NewSegment.getSegmentVMAddr();
   return Error::success();
 }
 
 // isValidMachOCannonicalName returns success if Name is a MachO cannonical name
 // ("<segment>,<section>") and lengths of both segment and section names are
 // valid.

llvm/tools/llvm-objcopy/MachO/Object.h

struct Object {
 
   Error removeLoadCommands(function_ref<bool(const LoadCommand &)> ToRemove);
 
   void updateLoadCommandIndexes();
 
   /// Creates a new segment load command in the object and returns a reference
   /// to the newly created load command. The caller should verify that SegName
   /// is not too long (SegName.size() should be less than or equal to 16).
-  LoadCommand &addSegment(StringRef SegName);
+  LoadCommand &addSegment(StringRef SegName, uint64_t SegVMSize);
 
   bool is64Bit() const {
     return Header.Magic == MachO::MH_MAGIC_64 ||
            Header.Magic == MachO::MH_CIGAM_64;
   }
 
   uint64_t nextAvailableSegmentAddress() const;
 };
 
 } // end namespace macho
 } // end namespace objcopy
 } // end namespace llvm
 
 #endif // LLVM_OBJCOPY_MACHO_OBJECT_H

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

for (const LoadCommand &LC : LoadCommands) {
     default:
       continue;
     }
   }
   return Addr;
 }
 
 template <typename SegmentType>
-static void constructSegment(SegmentType &Seg,
-                             llvm::MachO::LoadCommandType CmdType,
-                             StringRef SegName, uint64_t SegVMAddr) {
+static void
+constructSegment(SegmentType &Seg, llvm::MachO::LoadCommandType CmdType,
+                 StringRef SegName, uint64_t SegVMAddr, uint64_t SegVMSize) {
   assert(SegName.size() <= sizeof(Seg.segname) && "too long segment name");
   memset(&Seg, 0, sizeof(SegmentType));
   Seg.cmd = CmdType;
   strncpy(Seg.segname, SegName.data(), SegName.size());
   Seg.maxprot |=
       (MachO::VM_PROT_READ | MachO::VM_PROT_WRITE | MachO::VM_PROT_EXECUTE);
   Seg.initprot |=
       (MachO::VM_PROT_READ | MachO::VM_PROT_WRITE | MachO::VM_PROT_EXECUTE);
   Seg.vmaddr = SegVMAddr;
+  Seg.vmsize = SegVMSize;
 }
 
-LoadCommand &Object::addSegment(StringRef SegName) {
+LoadCommand &Object::addSegment(StringRef SegName, uint64_t SegVMSize) {
   LoadCommand LC;
   const uint64_t SegVMAddr = nextAvailableSegmentAddress();
   if (is64Bit())
     constructSegment(LC.MachOLoadCommand.segment_command_64_data,
-                     MachO::LC_SEGMENT_64, SegName, SegVMAddr);
+                     MachO::LC_SEGMENT_64, SegName, SegVMAddr, SegVMSize);
   else
     constructSegment(LC.MachOLoadCommand.segment_command_data,
-                     MachO::LC_SEGMENT, SegName, SegVMAddr);
+                     MachO::LC_SEGMENT, SegName, SegVMAddr, SegVMSize);
 
   LoadCommands.push_back(std::move(LC));
   return LoadCommands.back();
 }
 
 /// Extracts a segment name from a string which is possibly non-null-terminated.
 static StringRef extractSegmentName(const char *SegName) {
   return StringRef(SegName,