[ObjectYAML][MachO] Add error handling in MachOEmitter.

[Higuoxing]

Summary

Currently, yaml2macho doesn't support error handling. This patch helps improve it.

[Higuoxing]

Update.

[jhenderson]

llvm/lib/ObjectYAML/MachOEmitter.cpp

31

Perhaps invalid_argument instead of inconvertibleErrorCode()?

287

Here and elsewhere, I don't think you need an assertion if you are reporting an error too.

llvm/test/ObjectYAML/MachO/errors.yaml

1 ↗

(On Diff #266105)

I think these two test cases should each be in their own file, rather than a generic "errors" test file.

15 ↗

(On Diff #266105)

Perhaps worth having one entry here, to show that zero entries is not specifically the issue here.

[alexander-shaposhnikov]

llvm/lib/ObjectYAML/MachOEmitter.cpp

31

nit: not sure if this function is really that necessary, it feels like createStringError is already concise enough.
p.s. https://llvm.org/docs/CodingStandards.html#anonymous-namespaces , this probably should be 'static' instead.

287

+ 1

[Higuoxing]

Addressed comments.

• Use errc::invalid_argument instead of inconvertibleErrorCode()
• Remove assertions
• Move test cases to their own test files.
• Modify the Fat MachO test case according to the comment.

[Higuoxing]

Address @alexshap 's comment.

• Use createStringError()

[jhenderson]

Some comment nits, otherwise LGTM. Please wait for @alexshap to confirm too.

llvm/test/ObjectYAML/MachO/fat_macho_i386_x86_64.yaml

1

generating the -> generating

I think canonically it should be "Mach-O" rather than "MachO" here and elsewhere in comments.

93

'##' for comments.

llvm/test/ObjectYAML/MachO/sections.yaml

1

"in the MachO object file" -> "in Mach-O object files" I think reads better.

335

"##"
the -> an

[Higuoxing]

Address comments.

llvm/lib/ObjectYAML/MachOEmitter.cpp

 /// The Mach component of yaml2obj.
 ///
 //===----------------------------------------------------------------------===//
 
 #include "llvm/BinaryFormat/MachO.h"
 #include "llvm/ObjectYAML/DWARFEmitter.h"
 #include "llvm/ObjectYAML/ObjectYAML.h"
 #include "llvm/ObjectYAML/yaml2obj.h"
+#include "llvm/Support/Errc.h"
+#include "llvm/Support/Error.h"
 #include "llvm/Support/LEB128.h"
 #include "llvm/Support/YAMLTraits.h"
 #include "llvm/Support/raw_ostream.h"
 
 #include "llvm/Support/Format.h"
 
 using namespace llvm;
 
 namespace {
 
 class MachOWriter {
 public:
   MachOWriter(MachOYAML::Object &Obj) : Obj(Obj), is64Bit(true), fileStart(0) {
     is64Bit = Obj.Header.magic == MachO::MH_MAGIC_64 ||
               Obj.Header.magic == MachO::MH_CIGAM_64;
     memset(reinterpret_cast<void *>(&Header), 0, sizeof(MachO::mach_header_64));
   }
 
-  void writeMachO(raw_ostream &OS);
+  Error writeMachO(raw_ostream &OS);
 
 private:
   void writeHeader(raw_ostream &OS);
   void writeLoadCommands(raw_ostream &OS);
-  void writeSectionData(raw_ostream &OS);
+  Error writeSectionData(raw_ostream &OS);
   void writeRelocations(raw_ostream &OS);
   void writeLinkEditData(raw_ostream &OS);
 
   void writeBindOpcodes(raw_ostream &OS,
                         std::vector<MachOYAML::BindOpcode> &BindOpcodes);
   // LinkEdit writers
   void writeRebaseOpcodes(raw_ostream &OS);
   void writeBasicBindOpcodes(raw_ostream &OS);
private:
   uint64_t fileStart;
   MachO::mach_header_64 Header;
 
   // Old PPC Object Files didn't have __LINKEDIT segments, the data was just
   // stuck at the end of the file.
   bool FoundLinkEditSeg = false;
 };
 
-void MachOWriter::writeMachO(raw_ostream &OS) {
+Error MachOWriter::writeMachO(raw_ostream &OS) {
   fileStart = OS.tell();
   writeHeader(OS);
   writeLoadCommands(OS);
-  writeSectionData(OS);
+  if (Error Err = writeSectionData(OS))
+    return Err;
   writeRelocations(OS);
   if (!FoundLinkEditSeg)
     writeLinkEditData(OS);
+  return Error::success();
 }
 
 void MachOWriter::writeHeader(raw_ostream &OS) {
   Header.magic = Obj.Header.magic;
   Header.cputype = Obj.Header.cputype;
   Header.cpusubtype = Obj.Header.cpusubtype;
   Header.filetype = Obj.Header.filetype;
   Header.ncmds = Obj.Header.ncmds;
#include "llvm/BinaryFormat/MachO.def"
     // specified test cases.
     auto BytesRemaining = LC.Data.load_command_data.cmdsize - BytesWritten;
     if (BytesRemaining > 0) {
       ZeroFillBytes(OS, BytesRemaining);
     }
   }
 }
 
-void MachOWriter::writeSectionData(raw_ostream &OS) {
+Error MachOWriter::writeSectionData(raw_ostream &OS) {
   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;
       if (0 ==
           strncmp(&LC.Data.segment_command_data.segname[0], "__LINKEDIT", 16)) {
         FoundLinkEditSeg = true;
         writeLinkEditData(OS);
       }
       for (auto &Sec : LC.Sections) {
         ZeroToOffset(OS, Sec.offset);
         // Zero Fill any data between the end of the last thing we wrote and the
         // start of this section.
-        assert((OS.tell() - fileStart <= Sec.offset ||
-                Sec.offset == (uint32_t)0) &&
-               "Wrote too much data somewhere, section offsets don't line up.");
+        if (OS.tell() - fileStart > Sec.offset && Sec.offset != (uint32_t)0)
+          return createStringError(
+              errc::invalid_argument,
+              "wrote too much data somewhere, section offsets don't line up");
         if (0 == strncmp(&Sec.segname[0], "__DWARF", 16)) {
           if (0 == strncmp(&Sec.sectname[0], "__debug_str", 16)) {
             DWARFYAML::EmitDebugStr(OS, Obj.DWARF);
           } else if (0 == strncmp(&Sec.sectname[0], "__debug_abbrev", 16)) {
             DWARFYAML::EmitDebugAbbrev(OS, Obj.DWARF);
           } else if (0 == strncmp(&Sec.sectname[0], "__debug_aranges", 16)) {
             DWARFYAML::EmitDebugAranges(OS, Obj.DWARF);
           } else if (0 == strncmp(&Sec.sectname[0], "__debug_ranges", 16)) {
case MachO::LC_SEGMENT_64:
         }
       }
       uint64_t segSize = is64Bit ? LC.Data.segment_command_64_data.filesize
                                  : LC.Data.segment_command_data.filesize;
       ZeroToOffset(OS, segOff + segSize);
       break;
     }
   }
+
+  return Error::success();
 }
 
 // The implementation of makeRelocationInfo and makeScatteredRelocationInfo is
 // consistent with how libObject parses MachO binary files. For the reference
 // see getStruct, getRelocation, getPlainRelocationPCRel,
 // getPlainRelocationLength and related methods in MachOObjectFile.cpp
 static MachO::any_relocation_info
 makeRelocationInfo(const MachOYAML::Relocation &R, bool IsLE) {
void MachOWriter::writeStringTable(raw_ostream &OS) {
   }
 }
 
 class UniversalWriter {
 public:
   UniversalWriter(yaml::YamlObjectFile &ObjectFile)
       : ObjectFile(ObjectFile), fileStart(0) {}
 
-  void writeMachO(raw_ostream &OS);
+  Error writeMachO(raw_ostream &OS);
 
 private:
   void writeFatHeader(raw_ostream &OS);
   void writeFatArchs(raw_ostream &OS);
 
   void ZeroToOffset(raw_ostream &OS, size_t offset);
 
   yaml::YamlObjectFile &ObjectFile;
   uint64_t fileStart;
 };
 
-void UniversalWriter::writeMachO(raw_ostream &OS) {
+Error UniversalWriter::writeMachO(raw_ostream &OS) {
   fileStart = OS.tell();
   if (ObjectFile.MachO) {
     MachOWriter Writer(*ObjectFile.MachO);
-    Writer.writeMachO(OS);
-    return;
+    return Writer.writeMachO(OS);
   }
 
   writeFatHeader(OS);
   writeFatArchs(OS);
 
   auto &FatFile = *ObjectFile.FatMachO;
-  assert(FatFile.FatArchs.size() >= FatFile.Slices.size() &&
-         "Cannot write Slices if not decribed in FatArches");
+  if (FatFile.FatArchs.size() < FatFile.Slices.size())
+    return createStringError(
+        errc::invalid_argument,
+        "cannot write 'Slices' if not described in 'FatArches'");
+
   for (size_t i = 0; i < FatFile.Slices.size(); i++) {
     ZeroToOffset(OS, FatFile.FatArchs[i].offset);
     MachOWriter Writer(FatFile.Slices[i]);
-    Writer.writeMachO(OS);
+    if (Error Err = Writer.writeMachO(OS))
+      return Err;
 
     auto SliceEnd = FatFile.FatArchs[i].offset + FatFile.FatArchs[i].size;
     ZeroToOffset(OS, SliceEnd);
   }
+
+  return Error::success();
 }
 
 void UniversalWriter::writeFatHeader(raw_ostream &OS) {
   auto &FatFile = *ObjectFile.FatMachO;
   MachO::fat_header header;
   header.magic = FatFile.Header.magic;
   header.nfat_arch = FatFile.Header.nfat_arch;
   if (sys::IsLittleEndianHost)
if (currOffset < Offset)
     ZeroFillBytes(OS, Offset - currOffset);
 }
 
 } // end anonymous namespace
 
 namespace llvm {
 namespace yaml {
 
-bool yaml2macho(YamlObjectFile &Doc, raw_ostream &Out, ErrorHandler /*EH*/) {
+bool yaml2macho(YamlObjectFile &Doc, raw_ostream &Out, ErrorHandler EH) {
   UniversalWriter Writer(Doc);
-  Writer.writeMachO(Out);
+  if (Error Err = Writer.writeMachO(Out)) {
+    handleAllErrors(std::move(Err),
+                    [&](const ErrorInfoBase &Err) { EH(Err.message()); });
+    return false;
+  }
   return true;
 }
 
 } // namespace yaml
 } // namespace llvm

llvm/test/ObjectYAML/MachO/fat_macho_i386_x86_64.yaml

-# RUN: yaml2obj %s | obj2yaml | FileCheck %s
+## This file contains test cases for generating the Fat MachO binaries.
+
+## a) Test that yaml2obj emits the Fat MachO binary and obj2yaml converts it
+## back to YAML file.
+
+# RUN: yaml2obj --docnum=1 %s | obj2yaml | FileCheck %s
 
 --- !fat-mach-o
 FatHeader:       
   magic:           0xCAFEBABE
   nfat_arch:       2
 FatArchs:        
   - cputype:         0x00000007
     cpusubtype:      0x00000003

 #CHECK:       cputype:         0x01000007
 #CHECK:       cpusubtype:      0x80000003
 #CHECK:       filetype:        0x00000002
 #CHECK:       ncmds:           0
 #CHECK:       sizeofcmds:      0
 #CHECK:       flags:           0x00218085
 #CHECK:       reserved:        0x00000000
 #CHECK: ...
+
+## b) Test that yaml2obj emits an error message if the number of 'FatArchs' is less than
+## the number of 'Slices'.
+
+# RUN: not yaml2obj --docnum=2 %s -o %t2.fat-macho 2>&1 | FileCheck %s --check-prefix=ERROR
+
+# ERROR: yaml2obj: error: cannot write 'Slices' if not described in 'FatArches'
+
+--- !fat-mach-o
+FatHeader:
+  magic:     0xCAFEBABE
+  nfat_arch: 2
+FatArchs:
+  # 2 FatArchs are expected.
+  - cputype:    0x00000007
+    cpusubtype: 0x00000003
+    offset:     0x0000000000001000
+    size:       0
+    align:      0
+Slices:
+  - FileHeader:
+      magic:      0xFEEDFACE
+      cputype:    0x00000007
+      cpusubtype: 0x00000003
+      filetype:   0x00000002
+      ncmds:      0
+      sizeofcmds: 0
+      flags:      0x00000000
+  - FileHeader:
+      magic:      0xFEEDFACE
+      cputype:    0x00000007
+      cpusubtype: 0x00000003
+      filetype:   0x00000002
+      ncmds:      0
+      sizeofcmds: 0
+      flags:      0x00000000

llvm/test/ObjectYAML/MachO/sections.yaml

-# RUN: yaml2obj %s | obj2yaml | FileCheck %s
+## This file contains test cases for generating sections in the MachO object file.
+
+## a) Test that yaml2obj emits sections and obj2yaml converts them back.
+
+# RUN: yaml2obj --docnum=1 %s | obj2yaml | FileCheck %s
 
 --- !mach-o
 FileHeader:      
   magic:           0xFEEDFACF
   cputype:         0x01000007
   cpusubtype:      0x80000003
   filetype:        0x00000002
   ncmds:           16

 #CHECK:     segname:         __DATA
 #CHECK:     Sections:        
 #CHECK:       - sectname:        __got
 #CHECK:         segname:         __DATA
 #CHECK:       - sectname:        __nl_symbol_ptr
 #CHECK:         segname:         __DATA
 #CHECK:       - sectname:        __la_symbol_ptr
 #CHECK:         segname:         __DATA
+
+## b) Test that yaml2obj emits an error message if we specify an offset that
+## makes the current section and the previous one overlap.
+
+# RUN: not yaml2obj --docnum=2 %s -o %t2.macho 2>&1 | FileCheck %s --check-prefix=OVERLAP
+
+# OVERLAP: yaml2obj: error: wrote too much data somewhere, section offsets don't line up
+
+--- !mach-o
+FileHeader:
+  magic:      0xFEEDFACF
+  cputype:    0x01000007
+  cpusubtype: 0x80000003
+  filetype:   0x00000002
+  ncmds:      1
+  sizeofcmds: 1024
+  flags:      0x00000000
+  reserved:   0x00000000
+LoadCommands:
+  - cmd:      LC_SEGMENT_64
+    cmdsize:  0xff
+    segname:  __SEC
+    vmaddr:   0
+    vmsize:   0
+    fileoff:  0
+    filesize: 0
+    maxprot:  0
+    initprot: 0
+    nsects:   2
+    flags:    0
+    Sections:
+      - sectname:  __sec1
+        segname:   __SEC
+        addr:      0x0000000000000000
+        size:      2
+        offset:    0x00000000
+        align:     0
+        reloff:    0x00000000
+        nreloc:    0
+        flags:     0x00000000
+        reserved1: 0x00000000
+        reserved2: 0x00000000
+        reserved3: 0x00000000
+      - sectname:  __sec2
+        segname:   __SEC
+        addr:      0x0000000000000000
+        size:      2
+        offset:    0x00000001 # Specify the offset that makes __sec1 and __sec2 overlap.
+        align:     1
+        reloff:    0x00000000
+        nreloc:    0
+        flags:     0x00000000
+        reserved1: 0x00000000
+        reserved2: 0x00000000
+        reserved3: 0x00000000