[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

30

Perhaps invalid_argument instead of inconvertibleErrorCode()?

290

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

llvm/test/ObjectYAML/MachO/errors.yaml

2

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

16

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

[alexander-shaposhnikov]

llvm/lib/ObjectYAML/MachOEmitter.cpp

30

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.

290

+ 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 ↗

(On Diff #266395)

generating the -> generating

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

93 ↗

(On Diff #266395)

'##' for comments.

llvm/test/ObjectYAML/MachO/sections.yaml

1 ↗

(On Diff #266395)

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

335 ↗

(On Diff #266395)

"##"
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/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 {
 
+inline Error createError(const Twine &Msg) {
+  return createStringError(inconvertibleErrorCode(), Msg);
+}
+
 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 createError(
+              "wrote too much data somewhere, section offsets don't line up");
+        assert(
+            (OS.tell() - fileStart <= Sec.offset || Sec.offset == (uint32_t)0));
         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 createError("cannot write 'Slices' if not described in 'FatArches'");
+  assert(FatFile.FatArchs.size() >= FatFile.Slices.size());
+
   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/errors.yaml

+## This file contains test cases for error messages in MachOEmitter.
+
+## a) Test yaml2obj emits an error message if the number of 'FatArchs' is less than
+## the number of 'Slices'.
+
+# RUN: not yaml2obj --docnum=1 %s -o %t1.fat-macho 2>&1 | FileCheck %s --check-prefix=FATARCHS
+
+# FATARCHS: yaml2obj: error: cannot write 'Slices' if not described in 'FatArches'
+
+--- !fat-mach-o
+FatHeader:
+  magic:     0xCAFEBABE
+  nfat_arch: 2
+FatArchs:
+  # Should have 2 entries, but none.
+Slices:
+  - FileHeader:
+      magic:      0xFEEDFACE
+      cputype:    0x00000007
+      cpusubtype: 0x00000003
+      filetype:   0x00000002
+      ncmds:      0
+      sizeofcmds: 0
+      flags:      0x01218085
+
+## 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