From 3d218156f8eb62693c2ab31029859096a3b2e4a2 Mon Sep 17 00:00:00 2001 From: Rafael Espindola Date: Tue, 21 Jan 2014 23:06:54 +0000 Subject: 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 --- include/llvm/Object/Archive.h | 2 ++ include/llvm/Object/MachOUniversal.h | 2 ++ include/llvm/Object/ObjectFile.h | 6 ++--- lib/Object/Archive.cpp | 8 +++++++ lib/Object/Binary.cpp | 45 ++++++++---------------------------- lib/Object/COFFObjectFile.cpp | 19 ++++++++------- lib/Object/ELFObjectFile.cpp | 33 ++++++++++++++------------ lib/Object/MachOObjectFile.cpp | 20 ++++++++-------- lib/Object/MachOUniversal.cpp | 18 +++++++++++---- lib/Object/ObjectFile.cpp | 6 ++--- tools/llvm-objdump/MachODump.cpp | 6 ++--- 11 files changed, 84 insertions(+), 81 deletions(-) diff --git a/include/llvm/Object/Archive.h b/include/llvm/Object/Archive.h index 22c45ccac2..ce9391e8f0 100644 --- 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 create(MemoryBuffer *Source); enum Kind { K_GNU, diff --git a/include/llvm/Object/MachOUniversal.h b/include/llvm/Object/MachOUniversal.h index c5d1359256..ba02df9071 100644 --- 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 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 9aea639ef0..5ffa4293fb 100644 --- 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 createCOFFObjectFile(MemoryBuffer *Object); + static ErrorOr createELFObjectFile(MemoryBuffer *Object); + static ErrorOr createMachOObjectFile(MemoryBuffer *Object); }; // Inline function definitions. diff --git a/lib/Object/Archive.cpp b/lib/Object/Archive.cpp index 3c9eda74b5..286e9eebab 100644 --- a/lib/Object/Archive.cpp +++ b/lib/Object/Archive.cpp @@ -194,6 +194,14 @@ error_code Archive::Child::getAsBinary(OwningPtr &Result) const { return object_error::success; } +ErrorOr Archive::create(MemoryBuffer *Source) { + error_code EC; + OwningPtr 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 f1da11ceab..4f35d97526 100644 --- 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 object::createBinary(MemoryBuffer *Source) { OwningPtr 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 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 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 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 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 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 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 ec8989d5c6..a7b0688500 100644 --- 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::createCOFFObjectFile(MemoryBuffer *Object) { error_code EC; - return new COFFObjectFile(Object, EC); -} + OwningPtr 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 15bc6be0b4..2736bc42f2 100644 --- 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 Ident = getElfArchType(Object); - error_code ec; - +ErrorOr ObjectFile::createELFObjectFile(MemoryBuffer *Obj) { + std::pair Ident = getElfArchType(Obj); std::size_t MaxAlignment = - 1ULL << countTrailingZeros(uintptr_t(Object->getBufferStart())); + 1ULL << countTrailingZeros(uintptr_t(Obj->getBufferStart())); + error_code EC; + OwningPtr R; if (Ident.first == ELF::ELFCLASS32 && Ident.second == ELF::ELFDATA2LSB) #if !LLVM_IS_UNALIGNED_ACCESS_FAST if (MaxAlignment >= 4) - return new ELFObjectFile >(Object, ec); + R.reset(new ELFObjectFile >(Obj, EC)); else #endif if (MaxAlignment >= 2) - return new ELFObjectFile >(Object, ec); + R.reset(new ELFObjectFile >(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 >(Object, ec); + R.reset(new ELFObjectFile >(Obj, EC)); else #endif if (MaxAlignment >= 2) - return new ELFObjectFile >(Object, ec); + R.reset(new ELFObjectFile >(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 >(Object, ec); + R.reset(new ELFObjectFile >(Obj, EC)); else #endif if (MaxAlignment >= 2) - return new ELFObjectFile >(Object, ec); + R.reset(new ELFObjectFile >(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 >(Object, ec); + R.reset(new ELFObjectFile >(Obj, EC)); else #endif if (MaxAlignment >= 2) - return new ELFObjectFile >(Object, ec); + R.reset(new ELFObjectFile >(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 dc0f9ff6b4..a930117019 100644 --- 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::createMachOObjectFile(MemoryBuffer *Buffer) { StringRef Magic = Buffer->getBuffer().slice(0, 4); - error_code ec; - OwningPtr Ret; + error_code EC; + OwningPtr 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 223dfef8d2..0fe06b46b5 100644 --- 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 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::create(MemoryBuffer *Source) { + error_code EC; + OwningPtr 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 0e626d650f..2397f4b515 100644 --- 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 86923fd1d7..52c786f305 100644 --- a/tools/llvm-objdump/MachODump.cpp +++ b/tools/llvm-objdump/MachODump.cpp @@ -207,8 +207,8 @@ void llvm::DisassembleInputMachO(StringRef Filename) { return; } - OwningPtr MachOOF(static_cast( - ObjectFile::createMachOObjectFile(Buff.take()))); + OwningPtr MachOOF(static_cast( + 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 -- cgit v1.2.3