[llvm] r268694 - Cleanup and refactor of malformedError() in lib/Object/MachOObjectFile.cpp .
David Blaikie via llvm-commits
llvm-commits at lists.llvm.org
Thu May 5 17:18:32 PDT 2016
On Thu, May 5, 2016 at 5:04 PM, Kevin Enderby <enderby at apple.com> wrote:
>
> On May 5, 2016, at 4:51 PM, David Blaikie <dblaikie at gmail.com> wrote:
>
>
>
> On Thu, May 5, 2016 at 4:41 PM, Kevin Enderby via llvm-commits <
> llvm-commits at lists.llvm.org> wrote:
>
>> Author: enderby
>> Date: Thu May 5 18:41:05 2016
>> New Revision: 268694
>>
>> URL: http://llvm.org/viewvc/llvm-project?rev=268694&view=rev
>> Log:
>> Cleanup and refactor of malformedError() in
>> lib/Object/MachOObjectFile.cpp .
>>
>> No functional change.
>>
>> Modified:
>> llvm/trunk/lib/Object/MachOObjectFile.cpp
>>
>> Modified: llvm/trunk/lib/Object/MachOObjectFile.cpp
>> URL:
>> http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Object/MachOObjectFile.cpp?rev=268694&r1=268693&r2=268694&view=diff
>>
>> ==============================================================================
>> --- llvm/trunk/lib/Object/MachOObjectFile.cpp (original)
>> +++ llvm/trunk/lib/Object/MachOObjectFile.cpp Thu May 5 18:41:05 2016
>> @@ -38,19 +38,12 @@ 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, Twine Msg,
>> - object_error ECOverride = object_error::parse_failed) {
>> - return malformedError(Obj.getFileName(), Msg.str(), ECOverride);
>> +malformedError(const MachOObjectFile &Obj, Twine Msg) {
>> + std::string StringMsg = "truncated or malformed object (" + Msg.str()
>> + ")";
>>
>
> This might be marginally more efficient
>
>
> Certainly could be. But all this code is in the error path so I’m not
> sure if being more efficient is the highest goal.
>
Fair enough - I mention it just from a general habits, examples (idioms
that might end up used in other warmer code), etc, perspective.
>
> if you change the precedence:
>
> ("x" + y + "z").str()
>
> That way a new buffer won't have to be created (or at least shuffled
> around) for the ("x" + y) part.
>
> (also, and I know this one isn't so easy - Twine is mostly useful for
> cases where the value may never be manifest (eg: diagnostics
>
>
> Yep that is exactly these cases. As these are all for malformed Mach-O
> files. So my goal is to make it easy to get a good error message and keep
> the code written to be compact and readable.
>
> where the Twine is created, but then some condition is tested and the
> Twine may never be stringified). So I'd somewhat discourage it's use when
> that's not the situation - but I know Twine also provides convenience
> string concatenation, etc, that make it nice to use in places like this.
>
>
> Yep that is exactly why I think this refactoring is worth it.
>
> And the above point I made does demonstrate that it has some potential
> performance benefit here too)
>
>
> I agree with you. But do you feel the performance gain in the error case
> is worth making the code a bit more verbose?
>
Not especially - just figured I'd mention a few of the quirks in case they
seemed relevant/interesting to you. They're certainly marginal issues.
- Dave
>
>
>
>> + return make_error<GenericBinaryError>(std::move(Obj.getFileName()),
>> + std::move(StringMsg),
>> + object_error::parse_failed);
>> }
>>
>> // FIXME: Replace all uses of this function with getStructOrErr.
>> @@ -181,9 +174,8 @@ getLoadCommandInfo(const MachOObjectFile
>> uint32_t LoadCommandIndex) {
>> if (auto CmdOrErr = getStructOrErr<MachO::load_command>(Obj, Ptr)) {
>> if (CmdOrErr->cmdsize < 8)
>> - return malformedError(*Obj, Twine("truncated or malformed object
>> (load "
>> - "command ") + Twine(LoadCommandIndex) +
>> - Twine(" with size less than 8 bytes)"));
>> + return malformedError(*Obj, "load command " +
>> Twine(LoadCommandIndex) +
>> + " with size less than 8 bytes");
>> return MachOObjectFile::LoadCommandInfo({Ptr, *CmdOrErr});
>> } else
>> return CmdOrErr.takeError();
>> @@ -194,9 +186,8 @@ getFirstLoadCommandInfo(const MachOObjec
>> unsigned HeaderSize = Obj->is64Bit() ? sizeof(MachO::mach_header_64)
>> : sizeof(MachO::mach_header);
>> if (sizeof(MachOObjectFile::LoadCommandInfo) >
>> Obj->getHeader().sizeofcmds)
>> - return malformedError(*Obj, "truncated or malformed object (load
>> command "
>> - "0 extends past the end all load commands in
>> the "
>> - "file)");
>> + return malformedError(*Obj, "load command 0 extends past the end all
>> load "
>> + "commands in the file");
>> return getLoadCommandInfo(Obj, getPtr(Obj, HeaderSize), 0);
>> }
>>
>> @@ -207,10 +198,8 @@ getNextLoadCommandInfo(const MachOObject
>> : sizeof(MachO::mach_header);
>> if (L.Ptr + L.C.cmdsize + sizeof(MachOObjectFile::LoadCommandInfo) >
>> Obj->getData().data() + HeaderSize + Obj->getHeader().sizeofcmds)
>> - return malformedError(*Obj, Twine("truncated or malformed object "
>> - "(load command ") + Twine(LoadCommandIndex +
>> 1) +
>> - Twine(" extends past the end all load commands
>> in the "
>> - "file)"));
>> + return malformedError(*Obj, "load command " + Twine(LoadCommandIndex
>> + 1) +
>> + " extends past the end all load commands in
>> the file");
>> return getLoadCommandInfo(Obj, L.Ptr + L.C.cmdsize, LoadCommandIndex +
>> 1);
>> }
>>
>> @@ -218,8 +207,8 @@ template <typename T>
>> static void parseHeader(const MachOObjectFile *Obj, T &Header,
>> Error &Err) {
>> if (sizeof(T) > Obj->getData().size()) {
>> - Err = malformedError(*Obj, "truncated or malformed object (the mach
>> header "
>> - "extends past the end of the file)");
>> + Err = malformedError(*Obj, "the mach header extends past the end of
>> the "
>> + "file");
>> return;
>> }
>> if (auto HeaderOrErr = getStructOrErr<T>(Obj, getPtr(Obj, 0)))
>> @@ -238,19 +227,17 @@ static Error parseSegmentLoadCommand(
>> uint32_t LoadCommandIndex, const char *CmdName) {
>> const unsigned SegmentLoadSize = sizeof(SegmentCmd);
>> if (Load.C.cmdsize < SegmentLoadSize)
>> - return malformedError(*Obj, Twine("truncated or malformed object "
>> - "(load command ") + Twine(LoadCommandIndex) +
>> - Twine(" ") + CmdName + Twine(" cmdsize too
>> small)"));
>> + return malformedError(*Obj, "load command " +
>> Twine(LoadCommandIndex) +
>> + " " + CmdName + " cmdsize 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 malformedError(*Obj, Twine("truncated or malformed object "
>> - "(load command ") + Twine(LoadCommandIndex) +
>> - Twine(" inconsistent cmdsize in ") + CmdName
>> +
>> - Twine(" for the number of sections)"));
>> + return malformedError(*Obj, "load command " +
>> Twine(LoadCommandIndex) +
>> + " inconsistent cmdsize in " + CmdName +
>> + " for the number of sections");
>> for (unsigned J = 0; J < S.nsects; ++J) {
>> const char *Sec = getSectionPtr(Obj, Load, J);
>> Sections.push_back(Sec);
>> @@ -294,8 +281,7 @@ MachOObjectFile::MachOObjectFile(MemoryB
>> return;
>> BigSize += getHeader().sizeofcmds;
>> if (getData().data() + BigSize > getData().end()) {
>> - Err = malformedError(getFileName(), "truncated or malformed object "
>> - "(load commands extend past the end of the
>> file)");
>> + Err = malformedError(*this, "load commands extend past the end of
>> the file");
>> return;
>> }
>>
>> @@ -383,10 +369,8 @@ MachOObjectFile::MachOObjectFile(MemoryB
>> }
>> if (!SymtabLoadCmd) {
>> if (DysymtabLoadCmd) {
>> - Err = malformedError(*this,
>> - "truncated or malformed object (contains "
>> - "LC_DYSYMTAB load command without a LC_SYMTAB
>> load "
>> - "command)");
>> + Err = malformedError(*this, "contains LC_DYSYMTAB load command
>> without a "
>> + "LC_SYMTAB load command");
>> return;
>> }
>> } else if (DysymtabLoadCmd) {
>> @@ -395,51 +379,40 @@ MachOObjectFile::MachOObjectFile(MemoryB
>> MachO::dysymtab_command Dysymtab =
>> getStruct<MachO::dysymtab_command>(this, DysymtabLoadCmd);
>> if (Dysymtab.nlocalsym != 0 && Dysymtab.ilocalsym > Symtab.nsyms) {
>> - Err = malformedError(*this,
>> - "truncated or malformed object (iolocalsym in
>> "
>> - "LC_DYSYMTAB load command extends past the
>> end of "
>> - "the symbol table)");
>> + Err = malformedError(*this, "ilocalsym in LC_DYSYMTAB load command
>> "
>> + "extends past the end of the symbol table");
>> return;
>> }
>> uint64_t BigSize = Dysymtab.ilocalsym;
>> BigSize += Dysymtab.nlocalsym;
>> if (Dysymtab.nlocalsym != 0 && BigSize > Symtab.nsyms) {
>> - Err = malformedError(*this,
>> - "truncated or malformed object (ilocalsym
>> plus "
>> - "nlocalsym in LC_DYSYMTAB load command
>> extends past "
>> - "the end of the symbol table)");
>> + Err = malformedError(*this, "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) {
>> - Err = malformedError(*this,
>> - "truncated or malformed object (nextdefsym in
>> "
>> - "LC_DYSYMTAB load command extends past the
>> end of "
>> - "the symbol table)");
>> + Err = malformedError(*this, "nextdefsym in LC_DYSYMTAB load
>> command "
>> + "extends past the end of the symbol table");
>> return;
>> }
>> BigSize = Dysymtab.iextdefsym;
>> BigSize += Dysymtab.nextdefsym;
>> if (Dysymtab.nextdefsym != 0 && BigSize > Symtab.nsyms) {
>> - Err = malformedError(*this,
>> - "truncated or malformed object (iextdefsym
>> plus "
>> - "nextdefsym in LC_DYSYMTAB load command
>> extends "
>> - "past the end of the symbol table)");
>> + Err = malformedError(*this, "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) {
>> - Err = malformedError(*this,
>> - "truncated or malformed object (nundefsym in "
>> - "LC_DYSYMTAB load command extends past the
>> end of "
>> - "the symbol table)");
>> + Err = malformedError(*this, "nundefsym in LC_DYSYMTAB load command
>> "
>> + "extends past the end of the symbol table");
>> return;
>> }
>> BigSize = Dysymtab.iundefsym;
>> BigSize += Dysymtab.nundefsym;
>> if (Dysymtab.nundefsym != 0 && BigSize > Symtab.nsyms) {
>> - Err = malformedError(*this,
>> - "truncated or malformed object (iundefsym
>> plus "
>> - "nundefsym in LC_DYSYMTAB load command
>> extends past "
>> - "the end of the symbol table");
>> + Err = malformedError(*this, "iundefsym plus nundefsym in
>> LC_DYSYMTAB load "
>> + " command extends past the end of the symbol
>> table");
>> return;
>> }
>> }
>> @@ -460,10 +433,8 @@ Expected<StringRef> MachOObjectFile::get
>> MachO::nlist_base Entry = getSymbolTableEntryBase(this, Symb);
>> const char *Start = &StringTable.data()[Entry.n_strx];
>> if (Start < getData().begin() || Start >= getData().end()) {
>> - return malformedError(*this, Twine("truncated or malformed object
>> (bad "
>> - "string index: ") + Twine(Entry.n_strx) +
>> Twine(" for "
>> - "symbol at index ") +
>> Twine(getSymbolIndex(Symb)) +
>> - Twine(")"));
>> + return malformedError(*this, "bad string index: " +
>> Twine(Entry.n_strx) +
>> + " for symbol at index " +
>> Twine(getSymbolIndex(Symb)));
>> }
>> return StringRef(Start);
>> }
>> @@ -593,10 +564,8 @@ MachOObjectFile::getSymbolSection(DataRe
>> DataRefImpl DRI;
>> DRI.d.a = index - 1;
>> if (DRI.d.a >= Sections.size()){
>> - return malformedError(*this, Twine("truncated or malformed object
>> (bad "
>> - "section index: ") + Twine((int)index) +
>> Twine(" for "
>> - "symbol at index ") +
>> Twine(getSymbolIndex(Symb)) +
>> - Twine(")"));
>> + return malformedError(*this, "bad section index: " +
>> Twine((int)index) +
>> + " for symbol at index " +
>> Twine(getSymbolIndex(Symb)));
>> }
>> return section_iterator(SectionRef(DRI, this));
>> }
>> @@ -2427,6 +2396,7 @@ ObjectFile::createMachOObjectFile(Memory
>> return MachOObjectFile::create(Buffer, false, true);
>> if (Magic == "\xCF\xFA\xED\xFE")
>> return MachOObjectFile::create(Buffer, true, true);
>> - return malformedError(Buffer.getBufferIdentifier(),
>> - "Unrecognized MachO magic number");
>> + return
>> make_error<GenericBinaryError>(std::move(Buffer.getBufferIdentifier()),
>> + std::move("Unrecognized MachO magic
>> number"),
>> + object_error::invalid_file_type);
>> }
>>
>>
>> _______________________________________________
>> 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/20160505/80b50a7a/attachment.html>
More information about the llvm-commits
mailing list