[llvm] r264425 - [Object] Start threading Error through MachOObjectFile construction.
David Blaikie via llvm-commits
llvm-commits at lists.llvm.org
Fri Mar 25 11:35:31 PDT 2016
On Fri, Mar 25, 2016 at 11:25 AM, Lang Hames <lhames at gmail.com> wrote:
> 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.
>
Not sure I follow - why do they need to end up in a zero-init section? (it
seems like/I assume their value doesn't matter at all, but perhaps I'm
misunderstanding)
Also shouldn't they be const? (well, if their value doesn't matter it
shouldn't matter if their value is changed - but might want to lock it down
a bit - and at that point maybe the language will force you to give it an
initializer?)
> 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?
>
Yeah, it'd be a pretty clear false positive if we complained about init of
a variable whose value is never used (& init warnings on globals are
generally hard anyway - potentially accessed from anywhere, etc)
>
> +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.
>
Yeah, mildly unfortunate that that's the tradeoff. But I can't think of any
improvements to twine or string concatenation to help that greatly. It's
weird that callers get infected by whether their callers are likely to have
a concatenation expression or not...
>
> 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.
>
OK. Are we going to want a make_expected? I suppose not, since the element
has to be movable in general anyway.
>
> 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.
>
What I mean is that "branch to unreachable -> assert" is a simple transform
someone would make, so I probably wouldn't leave some code like that that
looks so easy to simplify & isn't. (our coding convention explicitly
suggests unreachable over assert(false) but doesn't currently have guidance
for this transformation (branch to unreachable -> assert) but I suspect it
should)
>
> 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();
>
Not sure of the best name... :/ I feel like something like "Error::Quiet()"
or something, but that's pretty vague/subtle, probably.
>
> ?
>
> 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.
>
Alrighty, will keep an eye out for it.
>
> - 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/0b17beb9/attachment-0001.html>
More information about the llvm-commits
mailing list