<html><head><meta http-equiv="Content-Type" content="text/html charset=utf-8"></head><body style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space;" class=""><br class=""><div><blockquote type="cite" class=""><div class="">On May 5, 2016, at 4:51 PM, David Blaikie <<a href="mailto:dblaikie@gmail.com" class="">dblaikie@gmail.com</a>> wrote:</div><br class="Apple-interchange-newline"><div class=""><div dir="ltr" class=""><br class=""><div class="gmail_extra"><br class=""><div class="gmail_quote">On Thu, May 5, 2016 at 4:41 PM, Kevin Enderby via llvm-commits <span dir="ltr" class=""><<a href="mailto:llvm-commits@lists.llvm.org" target="_blank" class="">llvm-commits@lists.llvm.org</a>></span> wrote:<br class=""><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Author: enderby<br class="">
Date: Thu May 5 18:41:05 2016<br class="">
New Revision: 268694<br class="">
<br class="">
URL: <a href="http://llvm.org/viewvc/llvm-project?rev=268694&view=rev" rel="noreferrer" target="_blank" class="">http://llvm.org/viewvc/llvm-project?rev=268694&view=rev</a><br class="">
Log:<br class="">
Cleanup and refactor of malformedError() in lib/Object/MachOObjectFile.cpp .<br class="">
<br class="">
No functional change.<br class="">
<br class="">
Modified:<br class="">
llvm/trunk/lib/Object/MachOObjectFile.cpp<br class="">
<br class="">
Modified: llvm/trunk/lib/Object/MachOObjectFile.cpp<br class="">
URL: <a href="http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Object/MachOObjectFile.cpp?rev=268694&r1=268693&r2=268694&view=diff" rel="noreferrer" target="_blank" class="">http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Object/MachOObjectFile.cpp?rev=268694&r1=268693&r2=268694&view=diff</a><br class="">
==============================================================================<br class="">
--- llvm/trunk/lib/Object/MachOObjectFile.cpp (original)<br class="">
+++ llvm/trunk/lib/Object/MachOObjectFile.cpp Thu May 5 18:41:05 2016<br class="">
@@ -38,19 +38,12 @@ namespace {<br class="">
};<br class="">
}<br class="">
<br class="">
-// FIXME: Remove ECOverride once Error has been plumbed down to obj tool code.<br class="">
static Error<br class="">
-malformedError(std::string FileName, std::string Msg,<br class="">
- object_error ECOverride = object_error::parse_failed) {<br class="">
- return make_error<GenericBinaryError>(std::move(FileName), std::move(Msg),<br class="">
- ECOverride);<br class="">
-}<br class="">
-<br class="">
-// FIXME: Remove ECOverride once Error has been plumbed down to obj tool code.<br class="">
-static Error<br class="">
-malformedError(const MachOObjectFile &Obj, Twine Msg,<br class="">
- object_error ECOverride = object_error::parse_failed) {<br class="">
- return malformedError(Obj.getFileName(), Msg.str(), ECOverride);<br class="">
+malformedError(const MachOObjectFile &Obj, Twine Msg) {<br class="">
+ std::string StringMsg = "truncated or malformed object (" + Msg.str() + ")";<br class=""></blockquote><div class=""><br class=""></div><div class="">This might be marginally more efficient</div></div></div></div></div></blockquote><div><br class=""></div>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.</div><div><br class=""><blockquote type="cite" class=""><div class=""><div dir="ltr" class=""><div class="gmail_extra"><div class="gmail_quote"><div class=""> if you change the precedence:<br class=""><br class="">("x" + y + "z").str()<br class=""><br class="">That way a new buffer won't have to be created (or at least shuffled around) for the ("x" + y) part.<br class=""><br class="">(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</div></div></div></div></div></blockquote><div><br class=""></div>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.</div><div><br class=""><blockquote type="cite" class=""><div class=""><div dir="ltr" class=""><div class="gmail_extra"><div class="gmail_quote"><div class=""> 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.</div></div></div></div></div></blockquote><div><br class=""></div>Yep that is exactly why I think this refactoring is worth it.</div><div><br class=""><blockquote type="cite" class=""><div class=""><div dir="ltr" class=""><div class="gmail_extra"><div class="gmail_quote"><div class=""> And the above point I made does demonstrate that it has some potential performance benefit here too)</div></div></div></div></div></blockquote><div><br class=""></div>I agree with you. But do you feel the performance gain in the error case is worth making the code a bit more verbose?</div><div><br class=""><blockquote type="cite" class=""><div class=""><div dir="ltr" class=""><div class="gmail_extra"><div class="gmail_quote"><div class=""> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
+ return make_error<GenericBinaryError>(std::move(Obj.getFileName()),<br class="">
+ std::move(StringMsg),<br class="">
+ object_error::parse_failed);<br class="">
}<br class="">
<br class="">
// FIXME: Replace all uses of this function with getStructOrErr.<br class="">
@@ -181,9 +174,8 @@ getLoadCommandInfo(const MachOObjectFile<br class="">
uint32_t LoadCommandIndex) {<br class="">
if (auto CmdOrErr = getStructOrErr<MachO::load_command>(Obj, Ptr)) {<br class="">
if (CmdOrErr->cmdsize < 8)<br class="">
- return malformedError(*Obj, Twine("truncated or malformed object (load "<br class="">
- "command ") + Twine(LoadCommandIndex) +<br class="">
- Twine(" with size less than 8 bytes)"));<br class="">
+ return malformedError(*Obj, "load command " + Twine(LoadCommandIndex) +<br class="">
+ " with size less than 8 bytes");<br class="">
return MachOObjectFile::LoadCommandInfo({Ptr, *CmdOrErr});<br class="">
} else<br class="">
return CmdOrErr.takeError();<br class="">
@@ -194,9 +186,8 @@ getFirstLoadCommandInfo(const MachOObjec<br class="">
unsigned HeaderSize = Obj->is64Bit() ? sizeof(MachO::mach_header_64)<br class="">
: sizeof(MachO::mach_header);<br class="">
if (sizeof(MachOObjectFile::LoadCommandInfo) > Obj->getHeader().sizeofcmds)<br class="">
- return malformedError(*Obj, "truncated or malformed object (load command "<br class="">
- "0 extends past the end all load commands in the "<br class="">
- "file)");<br class="">
+ return malformedError(*Obj, "load command 0 extends past the end all load "<br class="">
+ "commands in the file");<br class="">
return getLoadCommandInfo(Obj, getPtr(Obj, HeaderSize), 0);<br class="">
}<br class="">
<br class="">
@@ -207,10 +198,8 @@ getNextLoadCommandInfo(const MachOObject<br class="">
: sizeof(MachO::mach_header);<br class="">
if (L.Ptr + L.C.cmdsize + sizeof(MachOObjectFile::LoadCommandInfo) ><br class="">
Obj->getData().data() + HeaderSize + Obj->getHeader().sizeofcmds)<br class="">
- return malformedError(*Obj, Twine("truncated or malformed object "<br class="">
- "(load command ") + Twine(LoadCommandIndex + 1) +<br class="">
- Twine(" extends past the end all load commands in the "<br class="">
- "file)"));<br class="">
+ return malformedError(*Obj, "load command " + Twine(LoadCommandIndex + 1) +<br class="">
+ " extends past the end all load commands in the file");<br class="">
return getLoadCommandInfo(Obj, L.Ptr + L.C.cmdsize, LoadCommandIndex + 1);<br class="">
}<br class="">
<br class="">
@@ -218,8 +207,8 @@ template <typename T><br class="">
static void parseHeader(const MachOObjectFile *Obj, T &Header,<br class="">
Error &Err) {<br class="">
if (sizeof(T) > Obj->getData().size()) {<br class="">
- Err = malformedError(*Obj, "truncated or malformed object (the mach header "<br class="">
- "extends past the end of the file)");<br class="">
+ Err = malformedError(*Obj, "the mach header extends past the end of the "<br class="">
+ "file");<br class="">
return;<br class="">
}<br class="">
if (auto HeaderOrErr = getStructOrErr<T>(Obj, getPtr(Obj, 0)))<br class="">
@@ -238,19 +227,17 @@ static Error parseSegmentLoadCommand(<br class="">
uint32_t LoadCommandIndex, const char *CmdName) {<br class="">
const unsigned SegmentLoadSize = sizeof(SegmentCmd);<br class="">
if (Load.C.cmdsize < SegmentLoadSize)<br class="">
- return malformedError(*Obj, Twine("truncated or malformed object "<br class="">
- "(load command ") + Twine(LoadCommandIndex) +<br class="">
- Twine(" ") + CmdName + Twine(" cmdsize too small)"));<br class="">
+ return malformedError(*Obj, "load command " + Twine(LoadCommandIndex) +<br class="">
+ " " + CmdName + " cmdsize too small");<br class="">
if (auto SegOrErr = getStructOrErr<SegmentCmd>(Obj, Load.Ptr)) {<br class="">
SegmentCmd S = SegOrErr.get();<br class="">
const unsigned SectionSize =<br class="">
Obj->is64Bit() ? sizeof(MachO::section_64) : sizeof(MachO::section);<br class="">
if (S.nsects > std::numeric_limits<uint32_t>::max() / SectionSize ||<br class="">
S.nsects * SectionSize > Load.C.cmdsize - SegmentLoadSize)<br class="">
- return malformedError(*Obj, Twine("truncated or malformed object "<br class="">
- "(load command ") + Twine(LoadCommandIndex) +<br class="">
- Twine(" inconsistent cmdsize in ") + CmdName +<br class="">
- Twine(" for the number of sections)"));<br class="">
+ return malformedError(*Obj, "load command " + Twine(LoadCommandIndex) +<br class="">
+ " inconsistent cmdsize in " + CmdName +<br class="">
+ " for the number of sections");<br class="">
for (unsigned J = 0; J < S.nsects; ++J) {<br class="">
const char *Sec = getSectionPtr(Obj, Load, J);<br class="">
Sections.push_back(Sec);<br class="">
@@ -294,8 +281,7 @@ MachOObjectFile::MachOObjectFile(MemoryB<br class="">
return;<br class="">
BigSize += getHeader().sizeofcmds;<br class="">
if (getData().data() + BigSize > getData().end()) {<br class="">
- Err = malformedError(getFileName(), "truncated or malformed object "<br class="">
- "(load commands extend past the end of the file)");<br class="">
+ Err = malformedError(*this, "load commands extend past the end of the file");<br class="">
return;<br class="">
}<br class="">
<br class="">
@@ -383,10 +369,8 @@ MachOObjectFile::MachOObjectFile(MemoryB<br class="">
}<br class="">
if (!SymtabLoadCmd) {<br class="">
if (DysymtabLoadCmd) {<br class="">
- Err = malformedError(*this,<br class="">
- "truncated or malformed object (contains "<br class="">
- "LC_DYSYMTAB load command without a LC_SYMTAB load "<br class="">
- "command)");<br class="">
+ Err = malformedError(*this, "contains LC_DYSYMTAB load command without a "<br class="">
+ "LC_SYMTAB load command");<br class="">
return;<br class="">
}<br class="">
} else if (DysymtabLoadCmd) {<br class="">
@@ -395,51 +379,40 @@ MachOObjectFile::MachOObjectFile(MemoryB<br class="">
MachO::dysymtab_command Dysymtab =<br class="">
getStruct<MachO::dysymtab_command>(this, DysymtabLoadCmd);<br class="">
if (Dysymtab.nlocalsym != 0 && Dysymtab.ilocalsym > Symtab.nsyms) {<br class="">
- Err = malformedError(*this,<br class="">
- "truncated or malformed object (iolocalsym in "<br class="">
- "LC_DYSYMTAB load command extends past the end of "<br class="">
- "the symbol table)");<br class="">
+ Err = malformedError(*this, "ilocalsym in LC_DYSYMTAB load command "<br class="">
+ "extends past the end of the symbol table");<br class="">
return;<br class="">
}<br class="">
uint64_t BigSize = Dysymtab.ilocalsym;<br class="">
BigSize += Dysymtab.nlocalsym;<br class="">
if (Dysymtab.nlocalsym != 0 && BigSize > Symtab.nsyms) {<br class="">
- Err = malformedError(*this,<br class="">
- "truncated or malformed object (ilocalsym plus "<br class="">
- "nlocalsym in LC_DYSYMTAB load command extends past "<br class="">
- "the end of the symbol table)");<br class="">
+ Err = malformedError(*this, "ilocalsym plus nlocalsym in LC_DYSYMTAB load "<br class="">
+ "command extends past the end of the symbol table");<br class="">
return;<br class="">
}<br class="">
if (Dysymtab.nextdefsym != 0 && Dysymtab.ilocalsym > Symtab.nsyms) {<br class="">
- Err = malformedError(*this,<br class="">
- "truncated or malformed object (nextdefsym in "<br class="">
- "LC_DYSYMTAB load command extends past the end of "<br class="">
- "the symbol table)");<br class="">
+ Err = malformedError(*this, "nextdefsym in LC_DYSYMTAB load command "<br class="">
+ "extends past the end of the symbol table");<br class="">
return;<br class="">
}<br class="">
BigSize = Dysymtab.iextdefsym;<br class="">
BigSize += Dysymtab.nextdefsym;<br class="">
if (Dysymtab.nextdefsym != 0 && BigSize > Symtab.nsyms) {<br class="">
- Err = malformedError(*this,<br class="">
- "truncated or malformed object (iextdefsym plus "<br class="">
- "nextdefsym in LC_DYSYMTAB load command extends "<br class="">
- "past the end of the symbol table)");<br class="">
+ Err = malformedError(*this, "iextdefsym plus nextdefsym in LC_DYSYMTAB "<br class="">
+ "load command extends past the end of the symbol "<br class="">
+ "table");<br class="">
return;<br class="">
}<br class="">
if (Dysymtab.nundefsym != 0 && Dysymtab.iundefsym > Symtab.nsyms) {<br class="">
- Err = malformedError(*this,<br class="">
- "truncated or malformed object (nundefsym in "<br class="">
- "LC_DYSYMTAB load command extends past the end of "<br class="">
- "the symbol table)");<br class="">
+ Err = malformedError(*this, "nundefsym in LC_DYSYMTAB load command "<br class="">
+ "extends past the end of the symbol table");<br class="">
return;<br class="">
}<br class="">
BigSize = Dysymtab.iundefsym;<br class="">
BigSize += Dysymtab.nundefsym;<br class="">
if (Dysymtab.nundefsym != 0 && BigSize > Symtab.nsyms) {<br class="">
- Err = malformedError(*this,<br class="">
- "truncated or malformed object (iundefsym plus "<br class="">
- "nundefsym in LC_DYSYMTAB load command extends past "<br class="">
- "the end of the symbol table");<br class="">
+ Err = malformedError(*this, "iundefsym plus nundefsym in LC_DYSYMTAB load "<br class="">
+ " command extends past the end of the symbol table");<br class="">
return;<br class="">
}<br class="">
}<br class="">
@@ -460,10 +433,8 @@ Expected<StringRef> MachOObjectFile::get<br class="">
MachO::nlist_base Entry = getSymbolTableEntryBase(this, Symb);<br class="">
const char *Start = &StringTable.data()[Entry.n_strx];<br class="">
if (Start < getData().begin() || Start >= getData().end()) {<br class="">
- return malformedError(*this, Twine("truncated or malformed object (bad "<br class="">
- "string index: ") + Twine(Entry.n_strx) + Twine(" for "<br class="">
- "symbol at index ") + Twine(getSymbolIndex(Symb)) +<br class="">
- Twine(")"));<br class="">
+ return malformedError(*this, "bad string index: " + Twine(Entry.n_strx) +<br class="">
+ " for symbol at index " + Twine(getSymbolIndex(Symb)));<br class="">
}<br class="">
return StringRef(Start);<br class="">
}<br class="">
@@ -593,10 +564,8 @@ MachOObjectFile::getSymbolSection(DataRe<br class="">
DataRefImpl DRI;<br class="">
DRI.d.a = index - 1;<br class="">
if (DRI.d.a >= Sections.size()){<br class="">
- return malformedError(*this, Twine("truncated or malformed object (bad "<br class="">
- "section index: ") + Twine((int)index) + Twine(" for "<br class="">
- "symbol at index ") + Twine(getSymbolIndex(Symb)) +<br class="">
- Twine(")"));<br class="">
+ return malformedError(*this, "bad section index: " + Twine((int)index) +<br class="">
+ " for symbol at index " + Twine(getSymbolIndex(Symb)));<br class="">
}<br class="">
return section_iterator(SectionRef(DRI, this));<br class="">
}<br class="">
@@ -2427,6 +2396,7 @@ ObjectFile::createMachOObjectFile(Memory<br class="">
return MachOObjectFile::create(Buffer, false, true);<br class="">
if (Magic == "\xCF\xFA\xED\xFE")<br class="">
return MachOObjectFile::create(Buffer, true, true);<br class="">
- return malformedError(Buffer.getBufferIdentifier(),<br class="">
- "Unrecognized MachO magic number");<br class="">
+ return make_error<GenericBinaryError>(std::move(Buffer.getBufferIdentifier()),<br class="">
+ std::move("Unrecognized MachO magic number"),<br class="">
+ object_error::invalid_file_type);<br class="">
}<br class="">
<br class="">
<br class="">
_______________________________________________<br class="">
llvm-commits mailing list<br class="">
<a href="mailto:llvm-commits@lists.llvm.org" class="">llvm-commits@lists.llvm.org</a><br class="">
<a href="http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits" rel="noreferrer" target="_blank" class="">http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits</a><br class="">
</blockquote></div><br class=""></div></div>
</div></blockquote></div><br class=""></body></html>