[yaml2obj][MachO] Don't fill dummy data for virtual sections

[seiya]

Summary

Currently, MachOWriter::writeSectionData writes dummy data (0xdeadbeef) to fill section data areas in the file even if the section is a virtual one. Since virtual sections don't occupy any space in the file, writing dummy data could results the "OS.tell() - fileStart <= Sec.offset" assertion failure.

This patch fixes the bug by simply not writing any dummy data for virtual sections.

[seiya]

Added reviewers who may be relevant to part and my GSoC mentors.

[alexander-shaposhnikov]

khm, could you please explain the rationale for this change ? If yaml2obj completely ignores virtual sections we won't be able to create a MachO binary containing them, moreover, even in some simple cases the round trip conversion yaml -> obj -> yaml will create a completely different binary. Having ability to create binaries with virtual sections overall seems to be useful for testing various tools etc.

[seiya]

In D62991#1533463, @alexshap wrote:

khm, could you please explain the rationale for this change ? If yaml2obj completely ignores virtual sections we won't be able to create a MachO binary containing them, moreover, even in some simple cases the round trip conversion yaml -> obj -> yaml will create a completely different binary. Having ability to create binaries with virtual sections overall seems to be useful for testing various tools etc.

The description was insufficient and updated the summary. This patch is intended for fixing the problem so that we can create a MachO binary containing virtual sections

Currently, an assertion (OS.tell() - fileStart <= Sec.offset ...) in yaml2macho.cpp could fails if there are virtual sections in the yaml file: it writes Sec.size-sized dummy data as section data even if the section is virtual and finally OS.tell() - fileStart goes beyond the file offset of the next section (Sec.offset).

This patch fixes it by simply not (what "ignore" meant in the previous summary) writing dummy data if the section is virtual.

[alexander-shaposhnikov]

ok, this makes sense to me now, thanks for the explanation. LGTM with a couple of inline comments

llvm/tools/yaml2obj/yaml2macho.cpp

265 ↗

(On Diff #203474)

I have a couple of comments here:

1. in general i think this should be moved out of yaml2macho.cpp into the library (libObject ?)

because I believe it's not the only place where this might be useful. I think this can be done as a follow up though.

2. the test probably can be a bit more robust (and check all these cases at once), but I don't insist.

[compnerd]

llvm/test/ObjectYAML/MachO/virtual_section.yaml

85 ↗

(On Diff #203474)

I think that the __TEXT,__eh_frame is unnecessary for the test as is __LD,__compact_unwind and ___DATA,__common. It would be nice to have the TLS and the large zero-filled section in the test.

[seiya]

llvm/tools/yaml2obj/yaml2macho.cpp

265 ↗

(On Diff #203474)

I was wondering about the first comment. Some methods like MachOObjectFile::isSectionVirtual in libObject are what we need but ObjectYAML employs its own data structure MachOYAML::Object.

Adding some macros or inline functions into include/llvm/BinaryFormat/MachO.h and refactoring libObject as well as here using them would be better way I suppose. This should be separate patch by the way.

[seiya]

• Added a TLS section in the test.
• Removed irrelevant sections such as __TEXT,__eh_frame.

[seiya]

llvm/test/ObjectYAML/MachO/virtual_section.yaml

85 ↗

(On Diff #203474)

Hmm, in my environment (macOS 10.14.5), clang -m32 emits a corrupted object file if the input has large (4GiB) zero-filled section. Do you know an example of S_GB_ZEROFILL sections?

That said, possibly S_GB_ZEROFILL is no longer used because we use 64-bit binaries.

[alexander-shaposhnikov]

llvm/tools/yaml2obj/yaml2macho.cpp

265 ↗

(On Diff #203474)

yeah

[alexander-shaposhnikov]

i think we can commit this

[seiya]

Thank you I'll commit this.

llvm/trunk/test/ObjectYAML/MachO/virtual_section.yaml

+# RUN: yaml2obj %s | obj2yaml | FileCheck %s
+
+--- !mach-o
+FileHeader:
+  magic:           0xFEEDFACE
+  cputype:         0x00000007
+  cpusubtype:      0x00000003
+  filetype:        0x00000001
+  ncmds:           4
+  sizeofcmds:      660
+  flags:           0x00002000
+LoadCommands:
+  - cmd:             LC_SEGMENT
+    cmdsize:         532
+    segname:         ''
+    vmaddr:          0
+    vmsize:          175
+    fileoff:         688
+    filesize:        160
+    maxprot:         7
+    initprot:        7
+    nsects:          7
+    flags:           0
+    Sections:
+      - sectname:        __text
+        segname:         __TEXT
+        addr:            0x0000000000000000
+        size:            72
+        offset:          0x000002B0
+        align:           4
+        reloff:          0x00000350
+        nreloc:          7
+        flags:           0x80000400
+        reserved1:       0x00000000
+        reserved2:       0x00000000
+        reserved3:       0x00000000
+      - sectname:        __data
+        segname:         __DATA
+        addr:            0x0000000000000048
+        size:            4
+        offset:          0x000002F8
+        align:           2
+        reloff:          0x00000000
+        nreloc:          0
+        flags:           0x00000000
+        reserved1:       0x00000000
+        reserved2:       0x00000000
+        reserved3:       0x00000000
+      - sectname:        __bss
+        segname:         __DATA
+        addr:            0x00000000000000A0
+        size:            8
+        offset:          0x00000000
+        align:           2
+        reloff:          0x00000000
+        nreloc:          0
+        flags:           0x00000001
+        reserved1:       0x00000000
+        reserved2:       0x00000000
+        reserved3:       0x00000000
+      - sectname:        __thread_bss
+        segname:         __DATA
+        addr:            0x00000000000000A8
+        size:            7
+        offset:          0x00000000
+        align:           0
+        reloff:          0x00000000
+        nreloc:          0
+        flags:           0x00000012
+        reserved1:       0x00000000
+        reserved2:       0x00000000
+        reserved3:       0x00000000
+      - sectname:        __thread_vars
+        segname:         __DATA
+        addr:            0x000000000000004C
+        size:            12
+        offset:          0x000002FC
+        align:           0
+        reloff:          0x00000388
+        nreloc:          2
+        flags:           0x00000013
+        reserved1:       0x00000000
+        reserved2:       0x00000000
+        reserved3:       0x00000000
+  - cmd:             LC_BUILD_VERSION
+    cmdsize:         24
+    platform:        1
+    minos:           658944
+    sdk:             0
+    ntools:          0
+  - cmd:             LC_SYMTAB
+    cmdsize:         24
+    symoff:          928
+    nsyms:           7
+    stroff:          1012
+    strsize:         48
+  - cmd:             LC_DYSYMTAB
+    cmdsize:         80
+    ilocalsym:       0
+    nlocalsym:       4
+    iextdefsym:      4
+    nextdefsym:      2
+    iundefsym:       6
+    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
+LinkEditData:
+  NameList:
+    - n_strx:          44
+      n_type:          0x0E
+      n_sect:          2
+      n_desc:          0
+      n_value:         72
+    - n_strx:          41
+      n_type:          0x0E
+      n_sect:          3
+      n_desc:          0
+      n_value:         160
+    - n_strx:          38
+      n_type:          0x0E
+      n_sect:          3
+      n_desc:          0
+      n_value:         164
+    - n_strx:          1
+      n_type:          0x0E
+      n_sect:          4
+      n_desc:          0
+      n_value:         168
+    - n_strx:          35
+      n_type:          0x0F
+      n_sect:          5
+      n_desc:          0
+      n_value:         76
+    - n_strx:          29
+      n_type:          0x0F
+      n_sect:          1
+      n_desc:          0
+      n_value:         0
+    - n_strx:          13
+      n_type:          0x01
+      n_sect:          0
+      n_desc:          0
+      n_value:         0
+  StringTable:
+    - ''
+    - '_d$tlv$init'
+    - __tlv_bootstrap
+    - _main
+    - _d
+    - _c
+    - _b
+    - _a
+    - ''
+...
+
+# CHECK:         Sections:
+# CHECK-NEXT:      - sectname:        __text
+# CHECK-NEXT:        segname:         __TEXT
+# CHECK-NEXT:        addr:            0x0000000000000000
+# CHECK-NEXT:        size:            72
+# CHECK-NEXT:        offset:          0x000002B0
+# CHECK-NEXT:        align:           4
+# CHECK-NEXT:        reloff:          0x00000350
+# CHECK-NEXT:        nreloc:          7
+# CHECK-NEXT:        flags:           0x80000400
+# CHECK-NEXT:        reserved1:       0x00000000
+# CHECK-NEXT:        reserved2:       0x00000000
+# CHECK-NEXT:        reserved3:       0x00000000
+# CHECK-NEXT:      - sectname:        __data
+# CHECK-NEXT:        segname:         __DATA
+# CHECK-NEXT:        addr:            0x0000000000000048
+# CHECK-NEXT:        size:            4
+# CHECK-NEXT:        offset:          0x000002F8
+# CHECK-NEXT:        align:           2
+# CHECK-NEXT:        reloff:          0x00000000
+# CHECK-NEXT:        nreloc:          0
+# CHECK-NEXT:        flags:           0x00000000
+# CHECK-NEXT:        reserved1:       0x00000000
+# CHECK-NEXT:        reserved2:       0x00000000
+# CHECK-NEXT:        reserved3:       0x00000000
+# CHECK-NEXT:      - sectname:        __bss
+# CHECK-NEXT:        segname:         __DATA
+# CHECK-NEXT:        addr:            0x00000000000000A0
+# CHECK-NEXT:        size:            8
+# CHECK-NEXT:        offset:          0x00000000
+# CHECK-NEXT:        align:           2
+# CHECK-NEXT:        reloff:          0x00000000
+# CHECK-NEXT:        nreloc:          0
+# CHECK-NEXT:        flags:           0x00000001
+# CHECK-NEXT:        reserved1:       0x00000000
+# CHECK-NEXT:        reserved2:       0x00000000
+# CHECK-NEXT:        reserved3:       0x00000000
+# CHECK-NEXT:      - sectname:        __thread_bss
+# CHECK-NEXT:        segname:         __DATA
+# CHECK-NEXT:        addr:            0x00000000000000A8
+# CHECK-NEXT:        size:            7
+# CHECK-NEXT:        offset:          0x00000000
+# CHECK-NEXT:        align:           0
+# CHECK-NEXT:        reloff:          0x00000000
+# CHECK-NEXT:        nreloc:          0
+# CHECK-NEXT:        flags:           0x00000012
+# CHECK-NEXT:        reserved1:       0x00000000
+# CHECK-NEXT:        reserved2:       0x00000000
+# CHECK-NEXT:        reserved3:       0x00000000
+# CHECK-NEXT:      - sectname:        __thread_vars
+# CHECK-NEXT:        segname:         __DATA
+# CHECK-NEXT:        addr:            0x000000000000004C
+# CHECK-NEXT:        size:            12
+# CHECK-NEXT:        offset:          0x000002FC
+# CHECK-NEXT:        align:           0
+# CHECK-NEXT:        reloff:          0x00000388
+# CHECK-NEXT:        nreloc:          2
+# CHECK-NEXT:        flags:           0x00000013
+# CHECK-NEXT:        reserved1:       0x00000000
+# CHECK-NEXT:        reserved2:       0x00000000
+# CHECK-NEXT:        reserved3:       0x00000000

llvm/trunk/tools/yaml2obj/yaml2macho.cpp

#include "llvm/BinaryFormat/MachO.def"
     auto BytesRemaining = LC.Data.load_command_data.cmdsize - BytesWritten;
     if (BytesRemaining > 0) {
       ZeroFillBytes(OS, BytesRemaining);
     }
   }
   return Error::success();
 }
 
+static bool isVirtualSection(uint8_t type) {
+  return (type == MachO::S_ZEROFILL ||
+          type == MachO::S_GB_ZEROFILL ||
+          type == MachO::S_THREAD_LOCAL_ZEROFILL);
+}
+
 Error MachOWriter::writeSectionData(raw_ostream &OS) {
   bool FoundLinkEditSeg = false;
   for (auto &LC : Obj.LoadCommands) {
     switch (LC.Data.load_command_data.cmd) {
     case MachO::LC_SEGMENT:
     case MachO::LC_SEGMENT_64:
       uint64_t segOff = is64Bit ? LC.Data.segment_command_64_data.fileoff
                                 : LC.Data.segment_command_data.fileoff;
case MachO::LC_SEGMENT_64:
           } else if (0 == strncmp(&Sec.sectname[0], "__debug_pubtypes", 16)) {
             DWARFYAML::EmitPubSection(OS, Obj.DWARF.PubTypes,
                                       Obj.IsLittleEndian);
           } else if (0 == strncmp(&Sec.sectname[0], "__debug_info", 16)) {
             DWARFYAML::EmitDebugInfo(OS, Obj.DWARF);
           } else if (0 == strncmp(&Sec.sectname[0], "__debug_line", 16)) {
             DWARFYAML::EmitDebugLine(OS, Obj.DWARF);
           }
-        } else {
-          // Fills section data with 0xDEADBEEF
-          Fill(OS, Sec.size, 0xDEADBEEFu);
+
+          continue;
         }
+
+        // Skip if it's a virtual section.
+        if (isVirtualSection(Sec.flags & MachO::SECTION_TYPE))
+          continue;
+
+        // Fill section data with 0xDEADBEEF
+        Fill(OS, Sec.size, 0xDEADBEEFu);
       }
       uint64_t segSize = is64Bit ? LC.Data.segment_command_64_data.filesize
                                  : LC.Data.segment_command_data.filesize;
       ZeroToOffset(OS, segOff + segSize);
       break;
     }
   }
   // Old PPC Object Files didn't have __LINKEDIT segments, the data was just