[llvm] r264425 - [Object] Start threading Error through MachOObjectFile construction.
Lang Hames via llvm-commits
llvm-commits at lists.llvm.org
Fri Mar 25 11:25:53 PDT 2016
Hi Dave,
> drop the empty "private:" section?
Good call. Done in r264436.
+char BinaryError::ID = 0;
>
> +char GenericBinaryError::ID = 0;
>
>
> I'm assuming these don't really need values( just their address is used)
> so perhaps leave them uninitialized? (rather than giving the impression
> their value is important)
Nope. As long as they end up in a zero-init section we're fine. I usually
zero-init explicitly to avoid any uninitialized warnings, but I guess those
don't trigger on variables if we only take the address?
+GenericBinaryError::GenericBinaryError(std::string FileName, Twine Msg)
>
>
Might as well pass Msg as a string instead of a twine, if there's no
> opportunity to delay stringification?
Twine also composes more freely: StringRef + {StringRef, std::string,
char*} all work, and since error paths can be expensive anyway I didn't
mind the overhead of creating the twine just to stringify it. I don't have
strong feelings here though - switching to std::string will only mean a
little bit more boilerplate at call sites.
You can just "return {Ptr, *CmdOrErr};" if you're going to use braced init
> anyway/if you like.
The return value is an Expected<LoadCommand>, not a LoadCommand, so I have
to be explicit.
Else-after-return
CmdOrErr is scoped to the if - the else has to be there.
Branch-to-unreachable should be assert. But then the error wouldn't be
> checked? Perhaps need a more explicit discard operation for that.
Yeah. I grappled with that for a while (I'm thinking 'assertIsSuccess'),
but decided to just go with this for now and have more of a think about it.
If we're going to use Err out parameters, perhaps the caller should have
> some way of constructing an Err in the checked state instead of callers
> having to clear it?
That's a really good idea. The 'Error as out parameter' idiom really only
turns up in constructors (everything else can use Expected), so we can
chose something an ugly name that people won't be tempted to abuse. How
about
Error Err = Error::preCheckedSuccess();
?
Why the change to put the error checking inside the ifs here - it seems
> verbose/unfortunate?
Yep. I think the right way to fix this is to use a static named constructor
to minimize the number of sites using 'error as out parameter'. I'm about
to add that in a follow-up patch.
- Lang.
On Fri, Mar 25, 2016 at 10:53 AM, David Blaikie <dblaikie at gmail.com> wrote:
>
>
> On Fri, Mar 25, 2016 at 10:25 AM, Lang Hames via llvm-commits <
> llvm-commits at lists.llvm.org> wrote:
>
>> Author: lhames
>> Date: Fri Mar 25 12:25:34 2016
>> New Revision: 264425
>>
>> URL: http://llvm.org/viewvc/llvm-project?rev=264425&view=rev
>> Log:
>> [Object] Start threading Error through MachOObjectFile construction.
>>
>> Modified:
>> llvm/trunk/include/llvm/Object/Error.h
>> llvm/trunk/include/llvm/Object/MachO.h
>> llvm/trunk/lib/Object/Error.cpp
>> llvm/trunk/lib/Object/MachOObjectFile.cpp
>> llvm/trunk/tools/llvm-rtdyld/llvm-rtdyld.cpp
>>
>> Modified: llvm/trunk/include/llvm/Object/Error.h
>> URL:
>> http://llvm.org/viewvc/llvm-project/llvm/trunk/include/llvm/Object/Error.h?rev=264425&r1=264424&r2=264425&view=diff
>>
>> ==============================================================================
>> --- llvm/trunk/include/llvm/Object/Error.h (original)
>> +++ llvm/trunk/include/llvm/Object/Error.h Fri Mar 25 12:25:34 2016
>> @@ -14,11 +14,15 @@
>> #ifndef LLVM_OBJECT_ERROR_H
>> #define LLVM_OBJECT_ERROR_H
>>
>> +#include "llvm/ADT/Twine.h"
>> +#include "llvm/Support/Error.h"
>> #include <system_error>
>>
>> namespace llvm {
>> namespace object {
>>
>> +class Binary;
>> +
>> const std::error_category &object_category();
>>
>> enum class object_error {
>> @@ -39,6 +43,41 @@ inline std::error_code make_error_code(o
>> return std::error_code(static_cast<int>(e), object_category());
>> }
>>
>> +/// Base class for all errors indicating malformed binary files.
>> +///
>> +/// Having a subclass for all malformed binary files allows
>> archive-walking
>> +/// code to skip malformed files without having to understand every
>> possible
>> +/// way that a binary file might be malformed.
>> +///
>> +/// Currently inherits from ECError for easy interoperability with
>> +/// std::error_code, but this will be removed in the future.
>> +class BinaryError : public ErrorInfo<BinaryError, ECError> {
>>
>
> Might as well just make this class a struct? (its publicly inheriting, has
> only public members) or at least drop the empty "private:" section?
>
>
>> +public:
>> + static char ID;
>> + BinaryError() {
>> + // Default to parse_failed, can be overridden with setErrorCode.
>> + setErrorCode(make_error_code(object_error::parse_failed));
>> + }
>> +private:
>> +};
>> +
>> +/// Generic binary error.
>> +///
>> +/// For errors that don't require their own specific sub-error (most
>> errors)
>> +/// this class can be used to describe the error via a string message.
>> +class GenericBinaryError : public ErrorInfo<GenericBinaryError,
>> BinaryError> {
>> +public:
>> + static char ID;
>> + GenericBinaryError(std::string FileName, Twine Msg);
>> + GenericBinaryError(std::string FileName, Twine Msg, object_error
>> ECOverride);
>> + const std::string &getFileName() const { return FileName; }
>> + const std::string &getMessage() const { return Msg; }
>> + void log(raw_ostream &OS) const override;
>> +private:
>> + std::string FileName;
>> + std::string Msg;
>> +};
>> +
>> } // end namespace object.
>>
>> } // end namespace llvm.
>>
>> Modified: llvm/trunk/include/llvm/Object/MachO.h
>> URL:
>> http://llvm.org/viewvc/llvm-project/llvm/trunk/include/llvm/Object/MachO.h?rev=264425&r1=264424&r2=264425&view=diff
>>
>> ==============================================================================
>> --- llvm/trunk/include/llvm/Object/MachO.h (original)
>> +++ llvm/trunk/include/llvm/Object/MachO.h Fri Mar 25 12:25:34 2016
>> @@ -194,7 +194,7 @@ public:
>> typedef LoadCommandList::const_iterator load_command_iterator;
>>
>> MachOObjectFile(MemoryBufferRef Object, bool IsLittleEndian, bool
>> Is64Bits,
>> - std::error_code &EC);
>> + Error &Err);
>>
>> void moveSymbolNext(DataRefImpl &Symb) const override;
>>
>>
>> Modified: llvm/trunk/lib/Object/Error.cpp
>> URL:
>> http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Object/Error.cpp?rev=264425&r1=264424&r2=264425&view=diff
>>
>> ==============================================================================
>> --- llvm/trunk/lib/Object/Error.cpp (original)
>> +++ llvm/trunk/lib/Object/Error.cpp Fri Mar 25 12:25:34 2016
>> @@ -58,6 +58,21 @@ std::string _object_error_category::mess
>> "defined.");
>> }
>>
>> +char BinaryError::ID = 0;
>> +char GenericBinaryError::ID = 0;
>>
>
> I'm assuming these don't really need values( just their address is used)
> so perhaps leave them uninitialized? (rather than giving the impression
> their value is important)
>
>
>> +
>> +GenericBinaryError::GenericBinaryError(std::string FileName, Twine Msg)
>>
>
> Might as well pass Msg as a string instead of a twine, if there's no
> opportunity to delay stringification? (Twine is mostly used for cases where
> it's passed into an API that only conditionally uses it, so the
> concatenation can be avoided in the cases where it's not needed) This would
> allow moves/avoid copies, etc.
>
>
>> + : FileName(std::move(FileName)), Msg(Msg.str()) {}
>> +
>> +GenericBinaryError::GenericBinaryError(std::string FileName, Twine Msg,
>> object_error ECOverride)
>> + : FileName(std::move(FileName)), Msg(Msg.str()) {
>
> + setErrorCode(make_error_code(ECOverride));
>> +}
>> +
>> +void GenericBinaryError::log(raw_ostream &OS) const {
>> + OS << "Error in " << FileName << ": " << Msg;
>> +}
>> +
>> static ManagedStatic<_object_error_category> error_category;
>>
>> const std::error_category &object::object_category() {
>>
>> Modified: llvm/trunk/lib/Object/MachOObjectFile.cpp
>> URL:
>> http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Object/MachOObjectFile.cpp?rev=264425&r1=264424&r2=264425&view=diff
>>
>> ==============================================================================
>> --- llvm/trunk/lib/Object/MachOObjectFile.cpp (original)
>> +++ llvm/trunk/lib/Object/MachOObjectFile.cpp Fri Mar 25 12:25:34 2016
>> @@ -38,6 +38,22 @@ namespace {
>> };
>> }
>>
>> +// FIXME: Remove ECOverride once Error has been plumbed down to obj tool
>> code.
>
> +static Error
>> +malformedError(std::string FileName, std::string Msg,
>> + object_error ECOverride = object_error::parse_failed) {
>> + return make_error<GenericBinaryError>(std::move(FileName),
>> std::move(Msg),
>> + ECOverride);
>> +}
>> +
>> +
>> +// FIXME: Remove ECOverride once Error has been plumbed down to obj tool
>> code.
>> +static Error
>> +malformedError(const MachOObjectFile &Obj, std::string Msg,
>> + object_error ECOverride = object_error::parse_failed) {
>> + return malformedError(Obj.getFileName(), std::move(Msg), ECOverride);
>> +}
>> +
>> // FIXME: Replace all uses of this function with getStructOrErr.
>> template <typename T>
>> static T getStruct(const MachOObjectFile *O, const char *P) {
>> @@ -53,10 +69,10 @@ static T getStruct(const MachOObjectFile
>> }
>>
>> template <typename T>
>> -static ErrorOr<T> getStructOrErr(const MachOObjectFile *O, const char
>> *P) {
>> +static Expected<T> getStructOrErr(const MachOObjectFile *O, const char
>> *P) {
>> // Don't read before the beginning or past the end of the file
>> if (P < O->getData().begin() || P + sizeof(T) > O->getData().end())
>> - return object_error::parse_failed;
>> + return malformedError(*O, "Structure read out-of-range");
>>
>> T Cmd;
>> memcpy(&Cmd, P, sizeof(T));
>> @@ -161,27 +177,25 @@ static uint32_t getSectionFlags(const Ma
>> return Sect.flags;
>> }
>>
>> -static ErrorOr<MachOObjectFile::LoadCommandInfo>
>> +static Expected<MachOObjectFile::LoadCommandInfo>
>> getLoadCommandInfo(const MachOObjectFile *Obj, const char *Ptr) {
>> - auto CmdOrErr = getStructOrErr<MachO::load_command>(Obj, Ptr);
>> - if (!CmdOrErr)
>> - return CmdOrErr.getError();
>> - if (CmdOrErr->cmdsize < 8)
>> - return object_error::macho_small_load_command;
>> - MachOObjectFile::LoadCommandInfo Load;
>> - Load.Ptr = Ptr;
>> - Load.C = CmdOrErr.get();
>> - return Load;
>> + if (auto CmdOrErr = getStructOrErr<MachO::load_command>(Obj, Ptr)) {
>> + if (CmdOrErr->cmdsize < 8)
>> + return malformedError(*Obj, "Mach-O load command with size < 8
>> bytes",
>> + object_error::macho_small_load_command);
>> + return MachOObjectFile::LoadCommandInfo({Ptr, *CmdOrErr});
>>
>
> You can just "return {Ptr, *CmdOrErr};" if you're going to use braced init
> anyway/if you like.
>
>
>> + } else
>>
>
> Else-after-return
>
>
>> + return CmdOrErr.takeError();
>> }
>>
>> -static ErrorOr<MachOObjectFile::LoadCommandInfo>
>> +static Expected<MachOObjectFile::LoadCommandInfo>
>> getFirstLoadCommandInfo(const MachOObjectFile *Obj) {
>> unsigned HeaderSize = Obj->is64Bit() ? sizeof(MachO::mach_header_64)
>> : sizeof(MachO::mach_header);
>> return getLoadCommandInfo(Obj, getPtr(Obj, HeaderSize));
>> }
>>
>> -static ErrorOr<MachOObjectFile::LoadCommandInfo>
>> +static Expected<MachOObjectFile::LoadCommandInfo>
>> getNextLoadCommandInfo(const MachOObjectFile *Obj,
>> const MachOObjectFile::LoadCommandInfo &L) {
>> return getLoadCommandInfo(Obj, L.Ptr + L.C.cmdsize);
>> @@ -189,92 +203,104 @@ getNextLoadCommandInfo(const MachOObject
>>
>> template <typename T>
>> static void parseHeader(const MachOObjectFile *Obj, T &Header,
>> - std::error_code &EC) {
>> - auto HeaderOrErr = getStructOrErr<T>(Obj, getPtr(Obj, 0));
>> - if (HeaderOrErr)
>> - Header = HeaderOrErr.get();
>> + Error &Err) {
>> + if (auto HeaderOrErr = getStructOrErr<T>(Obj, getPtr(Obj, 0)))
>> + Header = *HeaderOrErr;
>> else
>> - EC = HeaderOrErr.getError();
>> + Err = HeaderOrErr.takeError();
>> }
>>
>> // Parses LC_SEGMENT or LC_SEGMENT_64 load command, adds addresses of all
>> // sections to \param Sections, and optionally sets
>> // \param IsPageZeroSegment to true.
>> template <typename SegmentCmd>
>> -static std::error_code parseSegmentLoadCommand(
>> +static Error parseSegmentLoadCommand(
>> const MachOObjectFile *Obj, const MachOObjectFile::LoadCommandInfo
>> &Load,
>> SmallVectorImpl<const char *> &Sections, bool &IsPageZeroSegment) {
>> const unsigned SegmentLoadSize = sizeof(SegmentCmd);
>> if (Load.C.cmdsize < SegmentLoadSize)
>> - return object_error::macho_load_segment_too_small;
>> - auto SegOrErr = getStructOrErr<SegmentCmd>(Obj, Load.Ptr);
>> - if (!SegOrErr)
>> - return SegOrErr.getError();
>> - SegmentCmd S = SegOrErr.get();
>> - const unsigned SectionSize =
>> + return malformedError(*Obj,
>> + "Mach-O segment load command size is too
>> small",
>> + object_error::macho_load_segment_too_small);
>> + if (auto SegOrErr = getStructOrErr<SegmentCmd>(Obj, Load.Ptr)) {
>> + SegmentCmd S = SegOrErr.get();
>> + const unsigned SectionSize =
>> Obj->is64Bit() ? sizeof(MachO::section_64) :
>> sizeof(MachO::section);
>> - if (S.nsects > std::numeric_limits<uint32_t>::max() / SectionSize ||
>> - S.nsects * SectionSize > Load.C.cmdsize - SegmentLoadSize)
>> - return object_error::macho_load_segment_too_many_sections;
>> - for (unsigned J = 0; J < S.nsects; ++J) {
>> - const char *Sec = getSectionPtr(Obj, Load, J);
>> - Sections.push_back(Sec);
>> - }
>> - IsPageZeroSegment |= StringRef("__PAGEZERO").equals(S.segname);
>> - return std::error_code();
>> + if (S.nsects > std::numeric_limits<uint32_t>::max() / SectionSize ||
>> + S.nsects * SectionSize > Load.C.cmdsize - SegmentLoadSize)
>> + return malformedError(*Obj,
>> + "Mach-O segment load command contains too
>> many "
>> + "sections",
>> +
>> object_error::macho_load_segment_too_many_sections);
>> + for (unsigned J = 0; J < S.nsects; ++J) {
>> + const char *Sec = getSectionPtr(Obj, Load, J);
>> + Sections.push_back(Sec);
>> + }
>> + IsPageZeroSegment |= StringRef("__PAGEZERO").equals(S.segname);
>> + } else
>> + return SegOrErr.takeError();
>> +
>> + return Error::success();
>> }
>>
>> MachOObjectFile::MachOObjectFile(MemoryBufferRef Object, bool
>> IsLittleEndian,
>> - bool Is64bits, std::error_code &EC)
>> + bool Is64bits, Error &Err)
>> : ObjectFile(getMachOType(IsLittleEndian, Is64bits), Object),
>> SymtabLoadCmd(nullptr), DysymtabLoadCmd(nullptr),
>> DataInCodeLoadCmd(nullptr), LinkOptHintsLoadCmd(nullptr),
>> DyldInfoLoadCmd(nullptr), UuidLoadCmd(nullptr),
>> HasPageZeroSegment(false) {
>> +
>> + // We have to check Err before it's assigned to.
>> + if (Err)
>> + llvm_unreachable("Err should be in success state at entry to
>> constructor.");
>>
>
> Branch-to-unreachable should be assert. But then the error wouldn't be
> checked? Perhaps need a more explicit discard operation for that.
>
> If we're going to use Err out parameters, perhaps the caller should have
> some way of constructing an Err in the checked state instead of callers
> having to clear it?
>
> (alternatively this API could be refactored into a named ctor style?
> (returning Expected<MachOObjectFile> and doing most of this logic in a
> static function rather than in the ctor? (& a private ctor that allows the
> processed data to be passed in to initialize the object)))
>
>
>> +
>> if (is64Bit())
>> - parseHeader(this, Header64, EC);
>> + parseHeader(this, Header64, Err);
>> else
>> - parseHeader(this, Header, EC);
>> - if (EC)
>> + parseHeader(this, Header, Err);
>> + if (Err)
>> return;
>>
>> uint32_t LoadCommandCount = getHeader().ncmds;
>> if (LoadCommandCount == 0)
>> return;
>>
>> - auto LoadOrErr = getFirstLoadCommandInfo(this);
>> - if (!LoadOrErr) {
>> - EC = LoadOrErr.getError();
>> + LoadCommandInfo Load;
>> + if (auto LoadOrErr = getFirstLoadCommandInfo(this))
>> + Load = *LoadOrErr;
>> + else {
>> + Err = LoadOrErr.takeError();
>> return;
>> }
>> - LoadCommandInfo Load = LoadOrErr.get();
>> +
>> for (unsigned I = 0; I < LoadCommandCount; ++I) {
>> LoadCommands.push_back(Load);
>> if (Load.C.cmd == MachO::LC_SYMTAB) {
>> // Multiple symbol tables
>> if (SymtabLoadCmd) {
>> - EC = object_error::parse_failed;
>> + Err = malformedError(*this, "Multiple symbol tables");
>> return;
>> }
>> SymtabLoadCmd = Load.Ptr;
>> } else if (Load.C.cmd == MachO::LC_DYSYMTAB) {
>> // Multiple dynamic symbol tables
>> if (DysymtabLoadCmd) {
>> - EC = object_error::parse_failed;
>> + Err = malformedError(*this, "Multiple dynamic symbol tables");
>> return;
>> }
>> DysymtabLoadCmd = Load.Ptr;
>> } else if (Load.C.cmd == MachO::LC_DATA_IN_CODE) {
>> // Multiple data in code tables
>> if (DataInCodeLoadCmd) {
>> - EC = object_error::parse_failed;
>> + Err = malformedError(*this, "Multiple data-in-code tables");
>> return;
>> }
>> DataInCodeLoadCmd = Load.Ptr;
>> } else if (Load.C.cmd == MachO::LC_LINKER_OPTIMIZATION_HINT) {
>> // Multiple linker optimization hint tables
>> if (LinkOptHintsLoadCmd) {
>> - EC = object_error::parse_failed;
>> + Err = malformedError(*this, "Multiple linker optimization hint
>> tables");
>> return;
>> }
>> LinkOptHintsLoadCmd = Load.Ptr;
>> @@ -282,24 +308,24 @@ MachOObjectFile::MachOObjectFile(MemoryB
>> Load.C.cmd == MachO::LC_DYLD_INFO_ONLY) {
>> // Multiple dyldinfo load commands
>> if (DyldInfoLoadCmd) {
>> - EC = object_error::parse_failed;
>> + Err = malformedError(*this, "Multiple dyldinfo load commands");
>> return;
>> }
>> DyldInfoLoadCmd = Load.Ptr;
>> } else if (Load.C.cmd == MachO::LC_UUID) {
>> // Multiple UUID load commands
>> if (UuidLoadCmd) {
>> - EC = object_error::parse_failed;
>> + Err = malformedError(*this, "Multiple UUID load commands");
>> return;
>> }
>> UuidLoadCmd = Load.Ptr;
>> } else if (Load.C.cmd == MachO::LC_SEGMENT_64) {
>> - if ((EC = parseSegmentLoadCommand<MachO::segment_command_64>(
>> - this, Load, Sections, HasPageZeroSegment)))
>> + if ((Err = parseSegmentLoadCommand<MachO::segment_command_64>(
>> + this, Load, Sections, HasPageZeroSegment)))
>> return;
>> } else if (Load.C.cmd == MachO::LC_SEGMENT) {
>> - if ((EC = parseSegmentLoadCommand<MachO::segment_command>(
>> - this, Load, Sections, HasPageZeroSegment)))
>> + if ((Err = parseSegmentLoadCommand<MachO::segment_command>(
>> + this, Load, Sections, HasPageZeroSegment)))
>> return;
>> } else if (Load.C.cmd == MachO::LC_LOAD_DYLIB ||
>> Load.C.cmd == MachO::LC_LOAD_WEAK_DYLIB ||
>> @@ -309,19 +335,20 @@ MachOObjectFile::MachOObjectFile(MemoryB
>> Libraries.push_back(Load.Ptr);
>> }
>> if (I < LoadCommandCount - 1) {
>> - auto LoadOrErr = getNextLoadCommandInfo(this, Load);
>> - if (!LoadOrErr) {
>> - EC = LoadOrErr.getError();
>> + if (auto LoadOrErr = getNextLoadCommandInfo(this, Load))
>> + Load = *LoadOrErr;
>> + else {
>> + Err = LoadOrErr.takeError();
>> return;
>> }
>> - Load = LoadOrErr.get();
>> }
>> }
>> if (!SymtabLoadCmd) {
>> if (DysymtabLoadCmd) {
>> - // Diagnostic("truncated or malformed object (contains LC_DYSYMTAB
>> load "
>> - // "command without a LC_SYMTAB load command)");
>> - EC = object_error::parse_failed;
>> + Err = malformedError(*this,
>> + "truncated or malformed object (contains "
>> + "LC_DYSYMTAB load command without a LC_SYMTAB
>> load "
>> + "command)");
>> return;
>> }
>> } else if (DysymtabLoadCmd) {
>
> @@ -330,49 +357,57 @@ MachOObjectFile::MachOObjectFile(MemoryB
>> MachO::dysymtab_command Dysymtab =
>> getStruct<MachO::dysymtab_command>(this, DysymtabLoadCmd);
>> if (Dysymtab.nlocalsym != 0 && Dysymtab.ilocalsym > Symtab.nsyms) {
>> - // Diagnostic("truncated or malformed object (ilocalsym in
>> LC_DYSYMTAB "
>> - // "load command extends past the end of the symbol table)"
>> - EC = object_error::parse_failed;
>> + Err = malformedError(*this,
>> + "truncated or malformed object (iolocalsym in
>> "
>> + "LC_DYSYMTAB load command extends past the
>> end of "
>> + "the symbol table)");
>> return;
>> }
>> uint64_t big_size = Dysymtab.ilocalsym;
>> big_size += Dysymtab.nlocalsym;
>> if (Dysymtab.nlocalsym != 0 && big_size > Symtab.nsyms) {
>> - // Diagnostic("truncated or malformed object (ilocalsym plus
>> nlocalsym "
>> - // "in LC_DYSYMTAB load command extends past the end of the symbol
>> table)"
>> - EC = object_error::parse_failed;
>> + Err = malformedError(*this,
>> + "truncated or malformed object (ilocalsym
>> plus "
>> + "nlocalsym in LC_DYSYMTAB load command
>> extends past "
>> + "the end of the symbol table)");
>> return;
>> }
>> if (Dysymtab.nextdefsym != 0 && Dysymtab.ilocalsym > Symtab.nsyms) {
>> - // Diagnostic("truncated or malformed object (nextdefsym in
>> LC_DYSYMTAB "
>> - // "load command extends past the end of the symbol table)"
>> - EC = object_error::parse_failed;
>> + Err = malformedError(*this,
>> + "truncated or malformed object (nextdefsym in
>> "
>> + "LC_DYSYMTAB load command extends past the
>> end of "
>> + "the symbol table)");
>> return;
>> }
>> big_size = Dysymtab.iextdefsym;
>> big_size += Dysymtab.nextdefsym;
>> if (Dysymtab.nextdefsym != 0 && big_size > Symtab.nsyms) {
>> - // Diagnostic("truncated or malformed object (iextdefsym plus
>> nextdefsym "
>> - // "in LC_DYSYMTAB load command extends past the end of the symbol
>> table)"
>> - EC = object_error::parse_failed;
>> + Err = malformedError(*this,
>> + "truncated or malformed object (iextdefsym
>> plus "
>> + "nextdefsym in LC_DYSYMTAB load command
>> extends "
>> + "past the end of the symbol table)");
>> return;
>> }
>> if (Dysymtab.nundefsym != 0 && Dysymtab.iundefsym > Symtab.nsyms) {
>> - // Diagnostic("truncated or malformed object (nundefsym in
>> LC_DYSYMTAB "
>> - // "load command extends past the end of the symbol table)"
>> - EC = object_error::parse_failed;
>> + Err = malformedError(*this,
>> + "truncated or malformed object (nundefsym in "
>> + "LC_DYSYMTAB load command extends past the
>> end of "
>> + "the symbol table)");
>> return;
>> }
>> big_size = Dysymtab.iundefsym;
>> big_size += Dysymtab.nundefsym;
>> if (Dysymtab.nundefsym != 0 && big_size > Symtab.nsyms) {
>> - // Diagnostic("truncated or malformed object (iundefsym plus
>> nundefsym "
>> - // "in LC_DYSYMTAB load command extends past the end of the symbol
>> table)"
>> - EC = object_error::parse_failed;
>> + Err = malformedError(*this,
>> + "truncated or malformed object (iundefsym
>> plus "
>> + "nundefsym in LC_DYSYMTAB load command
>> extends past "
>> + "the end of the symbol table");
>> return;
>> }
>> }
>> assert(LoadCommands.size() == LoadCommandCount);
>> +
>> + Err = Error::success();
>> }
>>
>> void MachOObjectFile::moveSymbolNext(DataRefImpl &Symb) const {
>> @@ -2381,20 +2416,29 @@ bool MachOObjectFile::isRelocatableObjec
>> ErrorOr<std::unique_ptr<MachOObjectFile>>
>> ObjectFile::createMachOObjectFile(MemoryBufferRef Buffer) {
>> StringRef Magic = Buffer.getBuffer().slice(0, 4);
>> - std::error_code EC;
>> std::unique_ptr<MachOObjectFile> Ret;
>> - if (Magic == "\xFE\xED\xFA\xCE")
>> - Ret.reset(new MachOObjectFile(Buffer, false, false, EC));
>> - else if (Magic == "\xCE\xFA\xED\xFE")
>> - Ret.reset(new MachOObjectFile(Buffer, true, false, EC));
>> - else if (Magic == "\xFE\xED\xFA\xCF")
>> - Ret.reset(new MachOObjectFile(Buffer, false, true, EC));
>> - else if (Magic == "\xCF\xFA\xED\xFE")
>> - Ret.reset(new MachOObjectFile(Buffer, true, true, EC));
>> - else
>> + if (Magic == "\xFE\xED\xFA\xCE") {
>> + Error Err;
>> + Ret.reset(new MachOObjectFile(Buffer, false, false, Err));
>>
>
> I would generally favor MakeUnique, but I see this is existing code (&
> perhaps the ctor is private/protected/somesuch)
>
> Why the change to put the error checking inside the ifs here - it seems
> verbose/unfortunate?
>
> (I guess if you put it outside then you end up prematurely raising the
> "checked" flag on the error before it's returned? Is there some way to fix
> that, API-wise? have a way to check-without-clearing?)
>
>
>> + if (Err)
>> + return errorToErrorCode(std::move(Err));
>> + } else if (Magic == "\xCE\xFA\xED\xFE") {
>> + Error Err;
>> + Ret.reset(new MachOObjectFile(Buffer, true, false, Err));
>> + if (Err)
>> + return errorToErrorCode(std::move(Err));
>> + } else if (Magic == "\xFE\xED\xFA\xCF") {
>> + Error Err;
>> + Ret.reset(new MachOObjectFile(Buffer, false, true, Err));
>> + if (Err)
>> + return errorToErrorCode(std::move(Err));
>> + } else if (Magic == "\xCF\xFA\xED\xFE") {
>> + Error Err;
>> + Ret.reset(new MachOObjectFile(Buffer, true, true, Err));
>> + if (Err)
>> + return errorToErrorCode(std::move(Err));
>> + } else
>> return object_error::parse_failed;
>>
>> - if (EC)
>> - return EC;
>> return std::move(Ret);
>> }
>>
>> Modified: llvm/trunk/tools/llvm-rtdyld/llvm-rtdyld.cpp
>> URL:
>> http://llvm.org/viewvc/llvm-project/llvm/trunk/tools/llvm-rtdyld/llvm-rtdyld.cpp?rev=264425&r1=264424&r2=264425&view=diff
>>
>> ==============================================================================
>> --- llvm/trunk/tools/llvm-rtdyld/llvm-rtdyld.cpp (original)
>> +++ llvm/trunk/tools/llvm-rtdyld/llvm-rtdyld.cpp Fri Mar 25 12:25:34 2016
>> @@ -254,9 +254,9 @@ uint8_t *TrivialMemoryManager::allocateD
>>
>> static const char *ProgramName;
>>
>> -static int Error(const Twine &Msg) {
>> +static int ErrorAndExit(const Twine &Msg) {
>> errs() << ProgramName << ": error: " << Msg << "\n";
>> - return 1;
>> + exit(1);
>> }
>>
>> static void loadDylibs() {
>> @@ -290,13 +290,13 @@ static int printLineInfoForInput(bool Lo
>> ErrorOr<std::unique_ptr<MemoryBuffer>> InputBuffer =
>> MemoryBuffer::getFileOrSTDIN(File);
>> if (std::error_code EC = InputBuffer.getError())
>> - return Error("unable to read input: '" + EC.message() + "'");
>> + ErrorAndExit("unable to read input: '" + EC.message() + "'");
>>
>> ErrorOr<std::unique_ptr<ObjectFile>> MaybeObj(
>> ObjectFile::createObjectFile((*InputBuffer)->getMemBufferRef()));
>>
>> if (std::error_code EC = MaybeObj.getError())
>> - return Error("unable to create object file: '" + EC.message() +
>> "'");
>> + ErrorAndExit("unable to create object file: '" + EC.message() +
>> "'");
>>
>> ObjectFile &Obj = **MaybeObj;
>>
>> @@ -309,7 +309,7 @@ static int printLineInfoForInput(bool Lo
>> Dyld.loadObject(Obj);
>>
>> if (Dyld.hasError())
>> - return Error(Dyld.getErrorString());
>> + ErrorAndExit(Dyld.getErrorString());
>>
>> // Resolve all the relocations we can.
>> Dyld.resolveRelocations();
>> @@ -400,19 +400,19 @@ static int executeInput() {
>> ErrorOr<std::unique_ptr<MemoryBuffer>> InputBuffer =
>> MemoryBuffer::getFileOrSTDIN(File);
>> if (std::error_code EC = InputBuffer.getError())
>> - return Error("unable to read input: '" + EC.message() + "'");
>> + ErrorAndExit("unable to read input: '" + EC.message() + "'");
>> ErrorOr<std::unique_ptr<ObjectFile>> MaybeObj(
>> ObjectFile::createObjectFile((*InputBuffer)->getMemBufferRef()));
>>
>> if (std::error_code EC = MaybeObj.getError())
>> - return Error("unable to create object file: '" + EC.message() +
>> "'");
>> + ErrorAndExit("unable to create object file: '" + EC.message() +
>> "'");
>>
>> ObjectFile &Obj = **MaybeObj;
>>
>> // Load the object file
>> Dyld.loadObject(Obj);
>> if (Dyld.hasError()) {
>> - return Error(Dyld.getErrorString());
>> + ErrorAndExit(Dyld.getErrorString());
>> }
>> }
>>
>> @@ -423,7 +423,7 @@ static int executeInput() {
>> // Get the address of the entry point (_main by default).
>> void *MainAddress = Dyld.getSymbolLocalAddress(EntryPoint);
>> if (!MainAddress)
>> - return Error("no definition for '" + EntryPoint + "'");
>> + ErrorAndExit("no definition for '" + EntryPoint + "'");
>>
>> // Invalidate the instruction cache for each loaded function.
>> for (auto &FM : MemMgr.FunctionMemory) {
>> @@ -432,7 +432,7 @@ static int executeInput() {
>> // setExecutable will call InvalidateInstructionCache.
>> std::string ErrorStr;
>> if (!sys::Memory::setExecutable(FM, &ErrorStr))
>> - return Error("unable to mark function executable: '" + ErrorStr +
>> "'");
>> + ErrorAndExit("unable to mark function executable: '" + ErrorStr +
>> "'");
>> }
>>
>> // Dispatch to _main().
>> @@ -452,12 +452,12 @@ static int checkAllExpressions(RuntimeDy
>> ErrorOr<std::unique_ptr<MemoryBuffer>> CheckerFileBuf =
>> MemoryBuffer::getFileOrSTDIN(CheckerFileName);
>> if (std::error_code EC = CheckerFileBuf.getError())
>> - return Error("unable to read input '" + CheckerFileName + "': " +
>> + ErrorAndExit("unable to read input '" + CheckerFileName + "': " +
>> EC.message());
>>
>> if (!Checker.checkAllRulesInBuffer("# rtdyld-check:",
>> CheckerFileBuf.get().get()))
>> - return Error("some checks in '" + CheckerFileName + "' failed");
>> + ErrorAndExit("some checks in '" + CheckerFileName + "' failed");
>> }
>> return 0;
>> }
>> @@ -606,7 +606,7 @@ static int linkAndVerify() {
>>
>> // Check for missing triple.
>> if (TripleName == "")
>> - return Error("-triple required when running in -verify mode.");
>> + ErrorAndExit("-triple required when running in -verify mode.");
>>
>> // Look up the target and build the disassembler.
>> Triple TheTriple(Triple::normalize(TripleName));
>> @@ -614,29 +614,29 @@ static int linkAndVerify() {
>> const Target *TheTarget =
>> TargetRegistry::lookupTarget("", TheTriple, ErrorStr);
>> if (!TheTarget)
>> - return Error("Error accessing target '" + TripleName + "': " +
>> ErrorStr);
>> + ErrorAndExit("Error accessing target '" + TripleName + "': " +
>> ErrorStr);
>>
>> TripleName = TheTriple.getTriple();
>>
>> std::unique_ptr<MCSubtargetInfo> STI(
>> TheTarget->createMCSubtargetInfo(TripleName, MCPU, ""));
>> if (!STI)
>> - return Error("Unable to create subtarget info!");
>> + ErrorAndExit("Unable to create subtarget info!");
>>
>> std::unique_ptr<MCRegisterInfo>
>> MRI(TheTarget->createMCRegInfo(TripleName));
>> if (!MRI)
>> - return Error("Unable to create target register info!");
>> + ErrorAndExit("Unable to create target register info!");
>>
>> std::unique_ptr<MCAsmInfo> MAI(TheTarget->createMCAsmInfo(*MRI,
>> TripleName));
>> if (!MAI)
>> - return Error("Unable to create target asm info!");
>> + ErrorAndExit("Unable to create target asm info!");
>>
>> MCContext Ctx(MAI.get(), MRI.get(), nullptr);
>>
>> std::unique_ptr<MCDisassembler> Disassembler(
>> TheTarget->createMCDisassembler(*STI, Ctx));
>> if (!Disassembler)
>> - return Error("Unable to create disassembler!");
>> + ErrorAndExit("Unable to create disassembler!");
>>
>> std::unique_ptr<MCInstrInfo> MII(TheTarget->createMCInstrInfo());
>>
>> @@ -663,20 +663,20 @@ static int linkAndVerify() {
>> MemoryBuffer::getFileOrSTDIN(Filename);
>>
>> if (std::error_code EC = InputBuffer.getError())
>> - return Error("unable to read input: '" + EC.message() + "'");
>> + ErrorAndExit("unable to read input: '" + EC.message() + "'");
>>
>> ErrorOr<std::unique_ptr<ObjectFile>> MaybeObj(
>> ObjectFile::createObjectFile((*InputBuffer)->getMemBufferRef()));
>>
>> if (std::error_code EC = MaybeObj.getError())
>> - return Error("unable to create object file: '" + EC.message() +
>> "'");
>> + ErrorAndExit("unable to create object file: '" + EC.message() +
>> "'");
>>
>> ObjectFile &Obj = **MaybeObj;
>>
>> // Load the object file
>> Dyld.loadObject(Obj);
>> if (Dyld.hasError()) {
>> - return Error(Dyld.getErrorString());
>> + ErrorAndExit(Dyld.getErrorString());
>> }
>> }
>>
>> @@ -692,7 +692,7 @@ static int linkAndVerify() {
>>
>> int ErrorCode = checkAllExpressions(Checker);
>> if (Dyld.hasError())
>> - return Error("RTDyld reported an error applying relocations:\n " +
>> + ErrorAndExit("RTDyld reported an error applying relocations:\n " +
>> Dyld.getErrorString());
>>
>> return ErrorCode;
>>
>>
>> _______________________________________________
>> llvm-commits mailing list
>> llvm-commits at lists.llvm.org
>> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits
>>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20160325/6bc3b01f/attachment.html>
More information about the llvm-commits
mailing list