Be a bit more consistent about using ErrorOr when constructing Binary objects.

The constructors of classes deriving from Binary normally take an error_code
as an argument to the constructor. My original intent was to change them
to have a trivial constructor and move the initial parsing logic to a static
method returning an ErrorOr. I changed my mind because:

* A constructor with an error_code out parameter is extremely convenient from
  the implementation side. We can incrementally construct the object and give
  up when we find an error.
* It is very efficient when constructing on the stack or when there is no
  error. The only inefficient case is where heap allocating and an error is
  found (we have to free the memory).

The result is that this is a much smaller patch. It just standardizes the
create* helpers to return an ErrorOr.

Almost no functionality change: The only difference is that this found that
we were trying to read past the end of COFF import library but ignoring the
error.

git-svn-id: https://llvm.org/svn/llvm-project/llvm/trunk@199770 91177308-0d34-0410-b5e6-96231b3b80d8

diff --git a/include/llvm/Object/Archive.h b/include/llvm/Object/Archive.h
index 22c45ccac2be5cfd6ba97d0409bc83c790c15e0a..ce9391e8f0fc775dd75bc36c4c9f2e5f65b79ec5 100644 (file)
--- a/include/llvm/Object/Archive.h
+++ b/include/llvm/Object/Archive.h
@@ -17,6 +17,7 @@
 #include "llvm/ADT/StringRef.h"
 #include "llvm/Object/Binary.h"
 #include "llvm/Support/ErrorHandling.h"
+#include "llvm/Support/ErrorOr.h"
 #include "llvm/Support/FileSystem.h"
 #include "llvm/Support/MemoryBuffer.h"
 
@@ -163,6 +164,7 @@ public:
   };
 
   Archive(MemoryBuffer *source, error_code &ec);
+  static ErrorOr<Archive *> create(MemoryBuffer *Source);
 
   enum Kind {
     K_GNU,

diff --git a/include/llvm/Object/MachOUniversal.h b/include/llvm/Object/MachOUniversal.h
index c5d1359256b0ec7eb3ba7203641f9b68d8349e82..ba02df90714011653ffbc7f91715349084360b6b 100644 (file)
--- a/include/llvm/Object/MachOUniversal.h
+++ b/include/llvm/Object/MachOUniversal.h
@@ -18,6 +18,7 @@
 #include "llvm/ADT/StringRef.h"
 #include "llvm/ADT/Triple.h"
 #include "llvm/Object/Binary.h"
+#include "llvm/Support/ErrorOr.h"
 #include "llvm/Support/MachO.h"
 
 namespace llvm {
@@ -77,6 +78,7 @@ public:
   };
 
   MachOUniversalBinary(MemoryBuffer *Source, error_code &ec);
+  static ErrorOr<MachOUniversalBinary*> create(MemoryBuffer *Source);
 
   object_iterator begin_objects() const {
     return ObjectForArch(this, 0);

diff --git a/include/llvm/Object/ObjectFile.h b/include/llvm/Object/ObjectFile.h
index 9aea639ef0582ce9351077b1f68f2ab82d4e3df0..5ffa4293fbbf6ece5e4776b4e40a62735e64cc38 100644 (file)
--- a/include/llvm/Object/ObjectFile.h
+++ b/include/llvm/Object/ObjectFile.h
@@ -384,9 +384,9 @@ public:
   }
 
 public:
-  static ObjectFile *createCOFFObjectFile(MemoryBuffer *Object);
-  static ObjectFile *createELFObjectFile(MemoryBuffer *Object);
-  static ObjectFile *createMachOObjectFile(MemoryBuffer *Object);
+  static ErrorOr<ObjectFile *> createCOFFObjectFile(MemoryBuffer *Object);
+  static ErrorOr<ObjectFile *> createELFObjectFile(MemoryBuffer *Object);
+  static ErrorOr<ObjectFile *> createMachOObjectFile(MemoryBuffer *Object);
 };
 
 // Inline function definitions.

diff --git a/lib/Object/Archive.cpp b/lib/Object/Archive.cpp
index 3c9eda74b52c0c8abd7aa15d70d192c8163f0781..286e9eebab3b12ff601dbffe91ca6b5e92ac6597 100644 (file)
--- a/lib/Object/Archive.cpp
+++ b/lib/Object/Archive.cpp
@@ -194,6 +194,14 @@ error_code Archive::Child::getAsBinary(OwningPtr<Binary> &Result) const {
   return object_error::success;
 }
 
+ErrorOr<Archive*> Archive::create(MemoryBuffer *Source) {
+  error_code EC;
+  OwningPtr<Archive> Ret(new Archive(Source, EC));
+  if (EC)
+    return EC;
+  return Ret.take();
+}
+
 Archive::Archive(MemoryBuffer *source, error_code &ec)
   : Binary(Binary::ID_Archive, source), SymbolTable(child_end()) {
   // Check for sufficient magic.

diff --git a/lib/Object/Binary.cpp b/lib/Object/Binary.cpp
index f1da11ceab58bb92d4dd90d8b2f528b4b3fe4bc3..4f35d9752626353d49e4029463e7051218f435e9 100644 (file)
--- a/lib/Object/Binary.cpp
+++ b/lib/Object/Binary.cpp
@@ -19,7 +19,6 @@
 
 // Include headers for createBinary.
 #include "llvm/Object/Archive.h"
-#include "llvm/Object/COFF.h"
 #include "llvm/Object/MachOUniversal.h"
 #include "llvm/Object/ObjectFile.h"
 
@@ -45,24 +44,14 @@ StringRef Binary::getFileName() const {
 ErrorOr<Binary *> object::createBinary(MemoryBuffer *Source) {
   OwningPtr<MemoryBuffer> scopedSource(Source);
   sys::fs::file_magic type = sys::fs::identify_magic(Source->getBuffer());
-  error_code EC;
   switch (type) {
-    case sys::fs::file_magic::archive: {
-      OwningPtr<Binary> Ret(new Archive(scopedSource.take(), EC));
-      if (EC)
-        return EC;
-      return Ret.take();
-    }
+    case sys::fs::file_magic::archive:
+      return Archive::create(scopedSource.take());
     case sys::fs::file_magic::elf_relocatable:
     case sys::fs::file_magic::elf_executable:
     case sys::fs::file_magic::elf_shared_object:
-    case sys::fs::file_magic::elf_core: {
-      OwningPtr<Binary> Ret(
-          ObjectFile::createELFObjectFile(scopedSource.take()));
-      if (!Ret)
-        return object_error::invalid_file_type;
-      return Ret.take();
-    }
+    case sys::fs::file_magic::elf_core:
+      return ObjectFile::createELFObjectFile(scopedSource.take());
     case sys::fs::file_magic::macho_object:
     case sys::fs::file_magic::macho_executable:
     case sys::fs::file_magic::macho_fixed_virtual_memory_shared_lib:
@@ -72,28 +61,14 @@ ErrorOr<Binary *> object::createBinary(MemoryBuffer *Source) {
     case sys::fs::file_magic::macho_dynamic_linker:
     case sys::fs::file_magic::macho_bundle:
     case sys::fs::file_magic::macho_dynamically_linked_shared_lib_stub:
-    case sys::fs::file_magic::macho_dsym_companion: {
-      OwningPtr<Binary> Ret(
-          ObjectFile::createMachOObjectFile(scopedSource.take()));
-      if (!Ret)
-        return object_error::invalid_file_type;
-      return Ret.take();
-    }
-    case sys::fs::file_magic::macho_universal_binary: {
-      OwningPtr<Binary> Ret(new MachOUniversalBinary(scopedSource.take(), EC));
-      if (EC)
-        return EC;
-      return Ret.take();
-    }
+    case sys::fs::file_magic::macho_dsym_companion:
+      return ObjectFile::createMachOObjectFile(scopedSource.take());
+    case sys::fs::file_magic::macho_universal_binary:
+      return MachOUniversalBinary::create(scopedSource.take());
     case sys::fs::file_magic::coff_object:
     case sys::fs::file_magic::coff_import_library:
-    case sys::fs::file_magic::pecoff_executable: {
-      OwningPtr<Binary> Ret(
-          ObjectFile::createCOFFObjectFile(scopedSource.take()));
-      if (!Ret)
-        return object_error::invalid_file_type;
-      return Ret.take();
-    }
+    case sys::fs::file_magic::pecoff_executable:
+      return ObjectFile::createCOFFObjectFile(scopedSource.take());
     case sys::fs::file_magic::unknown:
     case sys::fs::file_magic::bitcode:
     case sys::fs::file_magic::windows_resource: {

diff --git a/lib/Object/COFFObjectFile.cpp b/lib/Object/COFFObjectFile.cpp
index ec8989d5c67cb07ffc35abdad0b8e36be568ebdd..a7b06885008100dc45161376beb43bd915cde60b 100644 (file)
--- a/lib/Object/COFFObjectFile.cpp
+++ b/lib/Object/COFFObjectFile.cpp
@@ -514,10 +514,12 @@ COFFObjectFile::COFFObjectFile(MemoryBuffer *Object, error_code &EC)
     CurPtr += COFFHeader->SizeOfOptionalHeader;
   }
 
-  if (!COFFHeader->isImportLibrary())
-    if ((EC = getObject(SectionTable, Data, base() + CurPtr,
-                        COFFHeader->NumberOfSections * sizeof(coff_section))))
-      return;
+  if (COFFHeader->isImportLibrary())
+    return;
+
+  if ((EC = getObject(SectionTable, Data, base() + CurPtr,
+                      COFFHeader->NumberOfSections * sizeof(coff_section))))
+    return;
 
   // Initialize the pointer to the symbol table.
   if (COFFHeader->PointerToSymbolTable != 0)
@@ -1013,9 +1015,10 @@ error_code ExportDirectoryEntryRef::getSymbolName(StringRef &Result) const {
   return object_error::success;
 }
 
-namespace llvm {
-ObjectFile *ObjectFile::createCOFFObjectFile(MemoryBuffer *Object) {
+ErrorOr<ObjectFile *> ObjectFile::createCOFFObjectFile(MemoryBuffer *Object) {
   error_code EC;
-  return new COFFObjectFile(Object, EC);
-}
+  OwningPtr<COFFObjectFile> Ret(new COFFObjectFile(Object, EC));
+  if (EC)
+    return EC;
+  return Ret.take();
 }

diff --git a/lib/Object/ELFObjectFile.cpp b/lib/Object/ELFObjectFile.cpp
index 15bc6be0b46e7971dc3b7b60ac2f98010cbdf3a3..2736bc42f2ea246eedc2bfb7f9e899a600410dab 100644 (file)
--- a/lib/Object/ELFObjectFile.cpp
+++ b/lib/Object/ELFObjectFile.cpp
@@ -17,57 +17,60 @@
 namespace llvm {
 using namespace object;
 
-// Creates an in-memory object-file by default: createELFObjectFile(Buffer)
-ObjectFile *ObjectFile::createELFObjectFile(MemoryBuffer *Object) {
-  std::pair<unsigned char, unsigned char> Ident = getElfArchType(Object);
-  error_code ec;
-
+ErrorOr<ObjectFile *> ObjectFile::createELFObjectFile(MemoryBuffer *Obj) {
+  std::pair<unsigned char, unsigned char> Ident = getElfArchType(Obj);
   std::size_t MaxAlignment =
-    1ULL << countTrailingZeros(uintptr_t(Object->getBufferStart()));
+    1ULL << countTrailingZeros(uintptr_t(Obj->getBufferStart()));
 
+  error_code EC;
+  OwningPtr<ObjectFile> R;
   if (Ident.first == ELF::ELFCLASS32 && Ident.second == ELF::ELFDATA2LSB)
 #if !LLVM_IS_UNALIGNED_ACCESS_FAST
     if (MaxAlignment >= 4)
-      return new ELFObjectFile<ELFType<support::little, 4, false> >(Object, ec);
+      R.reset(new ELFObjectFile<ELFType<support::little, 4, false> >(Obj, EC));
     else
 #endif
     if (MaxAlignment >= 2)
-      return new ELFObjectFile<ELFType<support::little, 2, false> >(Object, ec);
+      R.reset(new ELFObjectFile<ELFType<support::little, 2, false> >(Obj, EC));
     else
       llvm_unreachable("Invalid alignment for ELF file!");
   else if (Ident.first == ELF::ELFCLASS32 && Ident.second == ELF::ELFDATA2MSB)
 #if !LLVM_IS_UNALIGNED_ACCESS_FAST
     if (MaxAlignment >= 4)
-      return new ELFObjectFile<ELFType<support::big, 4, false> >(Object, ec);
+      R.reset(new ELFObjectFile<ELFType<support::big, 4, false> >(Obj, EC));
     else
 #endif
     if (MaxAlignment >= 2)
-      return new ELFObjectFile<ELFType<support::big, 2, false> >(Object, ec);
+      R.reset(new ELFObjectFile<ELFType<support::big, 2, false> >(Obj, EC));
     else
       llvm_unreachable("Invalid alignment for ELF file!");
   else if (Ident.first == ELF::ELFCLASS64 && Ident.second == ELF::ELFDATA2MSB)
 #if !LLVM_IS_UNALIGNED_ACCESS_FAST
     if (MaxAlignment >= 8)
-      return new ELFObjectFile<ELFType<support::big, 8, true> >(Object, ec);
+      R.reset(new ELFObjectFile<ELFType<support::big, 8, true> >(Obj, EC));
     else
 #endif
     if (MaxAlignment >= 2)
-      return new ELFObjectFile<ELFType<support::big, 2, true> >(Object, ec);
+      R.reset(new ELFObjectFile<ELFType<support::big, 2, true> >(Obj, EC));
     else
       llvm_unreachable("Invalid alignment for ELF file!");
   else if (Ident.first == ELF::ELFCLASS64 && Ident.second == ELF::ELFDATA2LSB) {
 #if !LLVM_IS_UNALIGNED_ACCESS_FAST
     if (MaxAlignment >= 8)
-      return new ELFObjectFile<ELFType<support::little, 8, true> >(Object, ec);
+      R.reset(new ELFObjectFile<ELFType<support::little, 8, true> >(Obj, EC));
     else
 #endif
     if (MaxAlignment >= 2)
-      return new ELFObjectFile<ELFType<support::little, 2, true> >(Object, ec);
+      R.reset(new ELFObjectFile<ELFType<support::little, 2, true> >(Obj, EC));
     else
       llvm_unreachable("Invalid alignment for ELF file!");
   }
+  else
+    report_fatal_error("Buffer is not an ELF object file!");
 
-  report_fatal_error("Buffer is not an ELF object file!");
+  if (EC)
+    return EC;
+  return R.take();
 }
 
 } // end namespace llvm

diff --git a/lib/Object/MachOObjectFile.cpp b/lib/Object/MachOObjectFile.cpp
index dc0f9ff6b46326ffcb52165e20c0b725e701ad05..a930117019e7edbfc87228b6f83a373cbe17a5ce 100644 (file)
--- a/lib/Object/MachOObjectFile.cpp
+++ b/lib/Object/MachOObjectFile.cpp
@@ -1582,25 +1582,25 @@ void MachOObjectFile::ReadULEB128s(uint64_t Index,
   }
 }
 
-ObjectFile *ObjectFile::createMachOObjectFile(MemoryBuffer *Buffer) {
+ErrorOr<ObjectFile *> ObjectFile::createMachOObjectFile(MemoryBuffer *Buffer) {
   StringRef Magic = Buffer->getBuffer().slice(0, 4);
-  error_code ec;
-  OwningPtr<ObjectFile> Ret;
+  error_code EC;
+  OwningPtr<MachOObjectFile> Ret;
   if (Magic == "\xFE\xED\xFA\xCE")
-    Ret.reset(new MachOObjectFile(Buffer, false, false, ec));
+    Ret.reset(new MachOObjectFile(Buffer, false, false, EC));
   else if (Magic == "\xCE\xFA\xED\xFE")
-    Ret.reset(new MachOObjectFile(Buffer, true, false, ec));
+    Ret.reset(new MachOObjectFile(Buffer, true, false, EC));
   else if (Magic == "\xFE\xED\xFA\xCF")
-    Ret.reset(new MachOObjectFile(Buffer, false, true, ec));
+    Ret.reset(new MachOObjectFile(Buffer, false, true, EC));
   else if (Magic == "\xCF\xFA\xED\xFE")
-    Ret.reset(new MachOObjectFile(Buffer, true, true, ec));
+    Ret.reset(new MachOObjectFile(Buffer, true, true, EC));
   else {
     delete Buffer;
-    return NULL;
+    return object_error::parse_failed;
   }
 
-  if (ec)
-    return NULL;
+  if (EC)
+    return EC;
   return Ret.take();
 }
 

diff --git a/lib/Object/MachOUniversal.cpp b/lib/Object/MachOUniversal.cpp
index 223dfef8d29ce7af415c94d6afcb18092d0dfba8..0fe06b46b50960e2fdf89404f744d0bbc61ddf04 100644 (file)
--- a/lib/Object/MachOUniversal.cpp
+++ b/lib/Object/MachOUniversal.cpp
@@ -81,16 +81,26 @@ error_code MachOUniversalBinary::ObjectForArch::getAsObjectFile(
         Triple::getArchTypeName(MachOObjectFile::getArch(Header.cputype));
     MemoryBuffer *ObjBuffer = MemoryBuffer::getMemBuffer(
         ObjectData, ObjectName, false);
-    if (ObjectFile *Obj = ObjectFile::createMachOObjectFile(ObjBuffer)) {
-      Result.reset(Obj);
-      return object_error::success;
-    }
+    ErrorOr<ObjectFile *> Obj = ObjectFile::createMachOObjectFile(ObjBuffer);
+    if (error_code EC = Obj.getError())
+      return EC;
+    Result.reset(Obj.get());
+    return object_error::success;
   }
   return object_error::parse_failed;
 }
 
 void MachOUniversalBinary::anchor() { }
 
+ErrorOr<MachOUniversalBinary *>
+MachOUniversalBinary::create(MemoryBuffer *Source) {
+  error_code EC;
+  OwningPtr<MachOUniversalBinary> Ret(new MachOUniversalBinary(Source, EC));
+  if (EC)
+    return EC;
+  return Ret.take();
+}
+
 MachOUniversalBinary::MachOUniversalBinary(MemoryBuffer *Source,
                                            error_code &ec)
   : Binary(Binary::ID_MachOUniversalBinary, Source),

diff --git a/lib/Object/ObjectFile.cpp b/lib/Object/ObjectFile.cpp
index 0e626d650fbb5e3550d9981a164d4fe004e29d23..2397f4b515dd385ce44971e1b415838901998658 100644 (file)
--- a/lib/Object/ObjectFile.cpp
+++ b/lib/Object/ObjectFile.cpp
@@ -56,7 +56,7 @@ ObjectFile *ObjectFile::createObjectFile(MemoryBuffer *Object) {
   case sys::fs::file_magic::elf_executable:
   case sys::fs::file_magic::elf_shared_object:
   case sys::fs::file_magic::elf_core:
-    return createELFObjectFile(Object);
+    return createELFObjectFile(Object).get();
   case sys::fs::file_magic::macho_object:
   case sys::fs::file_magic::macho_executable:
   case sys::fs::file_magic::macho_fixed_virtual_memory_shared_lib:
@@ -67,11 +67,11 @@ ObjectFile *ObjectFile::createObjectFile(MemoryBuffer *Object) {
   case sys::fs::file_magic::macho_bundle:
   case sys::fs::file_magic::macho_dynamically_linked_shared_lib_stub:
   case sys::fs::file_magic::macho_dsym_companion:
-    return createMachOObjectFile(Object);
+    return createMachOObjectFile(Object).get();
   case sys::fs::file_magic::coff_object:
   case sys::fs::file_magic::coff_import_library:
   case sys::fs::file_magic::pecoff_executable:
-    return createCOFFObjectFile(Object);
+    return createCOFFObjectFile(Object).get();
   }
   llvm_unreachable("Unexpected Object File Type");
 }

diff --git a/tools/llvm-objdump/MachODump.cpp b/tools/llvm-objdump/MachODump.cpp
index 86923fd1d7f53091aa4ec2098d100deb6608dca6..52c786f30554b184e62bcc34e5f4b91b020993d5 100644 (file)
--- a/tools/llvm-objdump/MachODump.cpp
+++ b/tools/llvm-objdump/MachODump.cpp
@@ -207,8 +207,8 @@ void llvm::DisassembleInputMachO(StringRef Filename) {
     return;
   }
 
-  OwningPtr<MachOObjectFile> MachOOF(static_cast<MachOObjectFile*>(
-        ObjectFile::createMachOObjectFile(Buff.take())));
+  OwningPtr<MachOObjectFile> MachOOF(static_cast<MachOObjectFile *>(
+      ObjectFile::createMachOObjectFile(Buff.take()).get()));
 
   DisassembleInputMachO2(Filename, MachOOF.get());
 }
@@ -297,7 +297,7 @@ static void DisassembleInputMachO2(StringRef Filename,
         errs() << "llvm-objdump: " << Filename << ": " << ec.message() << '\n';
         return;
       }
-      DbgObj = ObjectFile::createMachOObjectFile(Buf.take());
+      DbgObj = ObjectFile::createMachOObjectFile(Buf.take()).get();
     }
 
     // Setup the DIContext