[llvm] r268694 - Cleanup and refactor of malformedError() in lib/Object/MachOObjectFile.cpp .
Kevin Enderby via llvm-commits
llvm-commits at lists.llvm.org
Thu May 5 17:04:05 PDT 2016
> 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 <mailto: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 <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 <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.
> 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?
>
> + 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 <mailto:llvm-commits at lists.llvm.org>
> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits <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/b2cfa625/attachment.html>
More information about the llvm-commits
mailing list