[llvm-objcopy][MachO] Fix segment's vmsize

[alexander-shaposhnikov]

Summary

Fix the calculation of vmsize.

Test plan: make check-all

[alexander-shaposhnikov]

Fix typo

[MaskRay]

Fix the calculation of vmsize.

Hope this can be expanded a bit to describe why the current code is wrong.

llvm/test/tools/llvm-objcopy/MachO/segments-vmsize.test

26

The long test (200+) makes it really unwieldy when we later want to reorganize tests. See ELF/ for some short examples. Can we simplify the YAML input a bit?

[alexander-shaposhnikov]

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

161

@MaskRay: the issue was here. (VMSize += Sec->Size) - this is not correct (and it results in producing a broken binary),
also you can take a look at the newly added test and the comment therein. I've already mentioned in the past reviews that for tests containing sections it's error-prone to modify MachO yaml manually, even removing one seemingly unrelated load command triggers the recalculation of the offsets mentioned inside other load commands + the test is not really large, it's the minimal example which triggers the issue (because of the page alignment). Having a valid binary also enables us to use the existing tools from Xcode to validate / introspect the binary if necessary.

[alexander-shaposhnikov]

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

161

to the best of my knowledge this test is not supposed to be reorganized, and if i'm not mistaken it should not even change (because it copies the binary without modifications). P.S. the test fails (as expected) without the fix.

[alexander-shaposhnikov]

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

161

just to be clear: (what i mentioned on one of the previous diffs) i think one can refactor / consolidate the tests which verify that some load commands are supported and simply copied-over. Tests like this one (which are supposed to catch a particular issue
(on some reduced example) and the tests which verify a particular functionality should not be merged etc.

[smeenai]

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

169

What purpose is the std::max serving here, given that VMSize is set to 0 before the loop and isn't modified inside the loop, as far as I can see?

[alexander-shaposhnikov]

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

169

@smeenai - good question,
(if I'm not mistaken this code was influenced by the old LLD for MachO)
the sections listed in one LC_SEGMENT_64 load command in general appear to be not ordered by their virtual addresses.

P.S. I've also done a small experiment - one can see this happens for some object files created by llvm-mc

llvm-mc -assemble -triple x86_64-apple-darwin9 -filetype=obj Inputs/various-symbols.s -o real-world-input-copy.test.tmp.various-symbols.o
llvm-readobj --sections real-world-input-copy.test.tmp.various-symbols.o ...

[smeenai]

This LGTM. I'll let @MaskRay sign off too.

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

169

Ah, that makes sense.

[MaskRay]

LGTM as well.

llvm/test/tools/llvm-objcopy/MachO/segments-vmsize.test

+# RUN: yaml2obj %s -o %t 
+
+## The binary used in this test can be built (on OSX) as follows:
+## main.c:
+##   __attribute__((used)) static int a[1021];
+##   __attribute__((used)) static char b = 1;
+##   int main() {
+##     return 0;
+##   }
+## build command:
+##   clang -fno-exceptions -fno-unwind-tables main.c -o main.exe
+## Note, that the variable "b" (whose size is 1) is placed into __DATA,__data section,
+## the array "a" (whose size is 4 * 1021 = 4084) is placed into __DATA,__bss and
+## __DATA,__bss is 16-byte aligned. In this case the total vmsize narrowly crosses
+## the border of one page (4096) and the final value should be equal to 8192.
+
+# RUN: llvm-objcopy %t %t.copy
+# RUN: cmp %t %t.copy
+
+## This additionally verifies that the binary created by llvm-objcopy/yaml2obj is valid.
+# RUN: llvm-readobj --sections %t.copy
+
+--- !mach-o
+FileHeader:
+  magic:           0xFEEDFACF
+  cputype:         0x01000007
+  cpusubtype:      0x80000003
+  filetype:        0x00000002
+  ncmds:           15
+  sizeofcmds:      976
+  flags:           0x00200085
+  reserved:        0x00000000
+LoadCommands:
+  - cmd:             LC_SEGMENT_64
+    cmdsize:         72
+    segname:         __PAGEZERO
+    vmaddr:          0
+    vmsize:          4294967296
+    fileoff:         0
+    filesize:        0
+    maxprot:         0
+    initprot:        0
+    nsects:          0
+    flags:           0
+  - cmd:             LC_SEGMENT_64
+    cmdsize:         232
+    segname:         __TEXT
+    vmaddr:          4294967296
+    vmsize:          4096
+    fileoff:         0
+    filesize:        4096
+    maxprot:         5
+    initprot:        5
+    nsects:          2
+    flags:           0
+    Sections:
+      - sectname:        __text
+        segname:         __TEXT
+        addr:            0x0000000100000FA0
+        size:            15
+        offset:          0x00000FA0
+        align:           4
+        reloff:          0x00000000
+        nreloc:          0
+        flags:           0x80000400
+        reserved1:       0x00000000
+        reserved2:       0x00000000
+        reserved3:       0x00000000
+        content:         554889E531C0C745FC000000005DC3
+      - sectname:        __unwind_info
+        segname:         __TEXT
+        addr:            0x0000000100000FB0
+        size:            72
+        offset:          0x00000FB0
+        align:           2
+        reloff:          0x00000000
+        nreloc:          0
+        flags:           0x00000000
+        reserved1:       0x00000000
+        reserved2:       0x00000000
+        reserved3:       0x00000000
+        content:         010000001C000000000000001C000000000000001C00000002000000A00F00003400000034000000B00F00000000000034000000030000000C000100100001000000000000000000
+  - cmd:             LC_SEGMENT_64
+    cmdsize:         232
+    segname:         __DATA
+    vmaddr:          4294971392
+    vmsize:          8192
+    fileoff:         4096
+    filesize:        4096
+    maxprot:         3
+    initprot:        3
+    nsects:          2
+    flags:           0
+    Sections:
+      - sectname:        __data
+        segname:         __DATA
+        addr:            0x0000000100001000
+        size:            1
+        offset:          0x00001000
+        align:           0
+        reloff:          0x00000000
+        nreloc:          0
+        flags:           0x00000000
+        reserved1:       0x00000000
+        reserved2:       0x00000000
+        reserved3:       0x00000000
+        content:         '01'
+      - sectname:        __bss
+        segname:         __DATA
+        addr:            0x0000000100001010
+        size:            4084
+        offset:          0x00000000
+        align:           4
+        reloff:          0x00000000
+        nreloc:          0
+        flags:           0x00000001
+        reserved1:       0x00000000
+        reserved2:       0x00000000
+        reserved3:       0x00000000
+  - cmd:             LC_SEGMENT_64
+    cmdsize:         72
+    segname:         __LINKEDIT
+    vmaddr:          4294979584
+    vmsize:          4096
+    fileoff:         8192
+    filesize:        128
+    maxprot:         1
+    initprot:        1
+    nsects:          0
+    flags:           0
+  - cmd:             LC_DYLD_INFO_ONLY
+    cmdsize:         48
+    rebase_off:      0
+    rebase_size:     0
+    bind_off:        0
+    bind_size:       0
+    weak_bind_off:   0
+    weak_bind_size:  0
+    lazy_bind_off:   0
+    lazy_bind_size:  0
+    export_off:      8192
+    export_size:     48
+  - cmd:             LC_SYMTAB
+    cmdsize:         24
+    symoff:          8248
+    nsyms:           2
+    stroff:          8280
+    strsize:         40
+  - cmd:             LC_DYSYMTAB
+    cmdsize:         80
+    ilocalsym:       0
+    nlocalsym:       0
+    iextdefsym:      0
+    nextdefsym:      1
+    iundefsym:       1
+    nundefsym:       1
+    tocoff:          0
+    ntoc:            0
+    modtaboff:       0
+    nmodtab:         0
+    extrefsymoff:    0
+    nextrefsyms:     0
+    indirectsymoff:  0
+    nindirectsyms:   0
+    extreloff:       0
+    nextrel:         0
+    locreloff:       0
+    nlocrel:         0
+  - cmd:             LC_LOAD_DYLINKER
+    cmdsize:         32
+    name:            12
+    PayloadString:   '/usr/lib/dyld'
+    ZeroPadBytes:    7
+  - cmd:             LC_UUID
+    cmdsize:         24
+    uuid:            11362E8D-D70B-33B3-96F8-A70ADF2583C4
+  - cmd:             LC_BUILD_VERSION
+    cmdsize:         32
+    platform:        1
+    minos:           658944
+    sdk:             658944
+    ntools:          1
+    Tools:
+      - tool:            3
+        version:         34734080
+  - cmd:             LC_SOURCE_VERSION
+    cmdsize:         16
+    version:         0
+  - cmd:             LC_MAIN
+    cmdsize:         24
+    entryoff:        4000
+    stacksize:       0
+  - cmd:             LC_LOAD_DYLIB
+    cmdsize:         56
+    dylib:
+      name:            24
+      timestamp:       2
+      current_version: 82115073
+      compatibility_version: 65536
+    PayloadString:   '/usr/lib/libSystem.B.dylib'
+    ZeroPadBytes:    6
+  - cmd:             LC_FUNCTION_STARTS
+    cmdsize:         16
+    dataoff:         8240
+    datasize:        8
+  - cmd:             LC_DATA_IN_CODE
+    cmdsize:         16
+    dataoff:         8248
+    datasize:        0
+LinkEditData:
+  ExportTrie:
+    TerminalSize:    0
+    NodeOffset:      0
+    Name:            ''
+    Flags:           0x0000000000000000
+    Address:         0x0000000000000000
+    Other:           0x0000000000000000
+    ImportName:      ''
+    Children:
+      - TerminalSize:    0
+        NodeOffset:      5
+        Name:            _
+        Flags:           0x0000000000000000
+        Address:         0x0000000000000000
+        Other:           0x0000000000000000
+        ImportName:      ''
+        Children:
+          - TerminalSize:    2
+            NodeOffset:      33
+            Name:            _mh_execute_header
+            Flags:           0x0000000000000000
+            Address:         0x0000000000000000
+            Other:           0x0000000000000000
+            ImportName:      ''
+          - TerminalSize:    3
+            NodeOffset:      37
+            Name:            main
+            Flags:           0x0000000000000000
+            Address:         0x0000000000000FA0
+            Other:           0x0000000000000000
+            ImportName:      ''
+  NameList:
+    - n_strx:          18
+      n_type:          0x0F
+      n_sect:          1
+      n_desc:          16
+      n_value:         4294967296
+    - n_strx:          1
+      n_type:          0x01
+      n_sect:          0
+      n_desc:          256
+      n_value:         0
+  StringTable:
+    - ''
+    - dyld_stub_binder
+    - __mh_execute_header
+    - ''
+    - ''
+...

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

if (Segname == "__LINKEDIT") {
       continue;
     }
 
     // Update file offsets and sizes of sections.
     uint64_t SegOffset = Offset;
     uint64_t SegFileSize = 0;
     uint64_t VMSize = 0;
     for (std::unique_ptr<Section> &Sec : LC.Sections) {
+      assert(SegmentVmAddr <= Sec->Addr &&
+             "Section's address cannot be smaller than Segment's one");
+      uint32_t SectOffset = Sec->Addr - SegmentVmAddr;
       if (IsObjectFile) {
         if (Sec->isVirtualSection()) {
           Sec->Offset = 0;
         } else {
           uint64_t PaddingSize =
               offsetToAlignment(SegFileSize, Align(1ull << Sec->Align));
           Sec->Offset = SegOffset + SegFileSize + PaddingSize;
           Sec->Size = Sec->Content.size();
           SegFileSize += PaddingSize + Sec->Size;
         }
-        VMSize = std::max(VMSize, Sec->Addr + Sec->Size);
       } else {
         if (Sec->isVirtualSection()) {
           Sec->Offset = 0;
-          VMSize += Sec->Size;
         } else {
-          uint32_t SectOffset = Sec->Addr - SegmentVmAddr;
           Sec->Offset = SegOffset + SectOffset;
           Sec->Size = Sec->Content.size();
           SegFileSize = std::max(SegFileSize, SectOffset + Sec->Size);
-          VMSize = std::max(VMSize, SegFileSize);
         }
       }
+      VMSize = std::max(VMSize, SectOffset + Sec->Size);
     }
 
     if (IsObjectFile) {
       Offset += SegFileSize;
     } else {
       Offset = alignTo(Offset + SegFileSize, PageSize);
       SegFileSize = alignTo(SegFileSize, PageSize);
       // Use the original vmsize if the segment is __PAGEZERO.