<div dir="ltr"><div>Regarding the contents of your t.patch:</div><div><br></div>isSectionRequiredForExecution, isSectionZeroInit and isSectionReadOnlyData have no callers from LLVM (and are therefore untestable), they can probably be removed from lib/Object altogether.<div><br></div><div>I replaced the check in COFFObjectFile::toSymb with a much more concise assertion using checkOffset.</div></div><div class="gmail_extra"><br><div class="gmail_quote">On Mon, Nov 24, 2014 at 11:00 PM, Rafael Espíndola <span dir="ltr"><<a href="mailto:rafael.espindola@gmail.com" target="_blank">rafael.espindola@gmail.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">This, like the other patches, seems almost entirely untested.<br>
<br>
No tests fails with the attached partial revert.<br>
<br>
Please don't add any more untested code to lib/Object. In particular,<br>
please don't add untested error handling code. lib/Object has a<br>
history of truly horrible error checking interfaces and we really have<br>
to test the error paths to have confidence in refactoring them.<br>
<div class="HOEnZb"><div class="h5"><br>
<br>
<br>
On 17 November 2014 at 06:17, David Majnemer <<a href="mailto:david.majnemer@gmail.com">david.majnemer@gmail.com</a>> wrote:<br>
> Author: majnemer<br>
> Date: Mon Nov 17 05:17:17 2014<br>
> New Revision: 222124<br>
><br>
> URL: <a href="http://llvm.org/viewvc/llvm-project?rev=222124&view=rev" target="_blank">http://llvm.org/viewvc/llvm-project?rev=222124&view=rev</a><br>
> Log:<br>
> Object, COFF: Tighten the object file parser<br>
><br>
> We were a little lax in a few areas:<br>
> - We pretended that import libraries were like any old COFF file, they<br>
> are not. In fact, they aren't really COFF files at all, we should<br>
> probably grow some specialized functionality to handle them smarter.<br>
> - Our symbol iterators were more than happy to attempt to go past the<br>
> end of the symbol table if you had a symbol with a bad list of<br>
> auxiliary symbols.<br>
><br>
> Modified:<br>
> llvm/trunk/include/llvm/Object/COFF.h<br>
> llvm/trunk/lib/Object/COFFObjectFile.cpp<br>
> llvm/trunk/test/tools/llvm-readobj/file-headers.test<br>
> llvm/trunk/tools/llvm-objdump/llvm-objdump.cpp<br>
> llvm/trunk/tools/llvm-readobj/COFFDumper.cpp<br>
><br>
> Modified: llvm/trunk/include/llvm/Object/COFF.h<br>
> URL: <a href="http://llvm.org/viewvc/llvm-project/llvm/trunk/include/llvm/Object/COFF.h?rev=222124&r1=222123&r2=222124&view=diff" target="_blank">http://llvm.org/viewvc/llvm-project/llvm/trunk/include/llvm/Object/COFF.h?rev=222124&r1=222123&r2=222124&view=diff</a><br>
> ==============================================================================<br>
> --- llvm/trunk/include/llvm/Object/COFF.h (original)<br>
> +++ llvm/trunk/include/llvm/Object/COFF.h Mon Nov 17 05:17:17 2014<br>
> @@ -508,7 +508,8 @@ public:<br>
> }<br>
> uint16_t getSizeOfOptionalHeader() const {<br>
> if (COFFHeader)<br>
> - return COFFHeader->SizeOfOptionalHeader;<br>
> + return COFFHeader->isImportLibrary() ? 0<br>
> + : COFFHeader->SizeOfOptionalHeader;<br>
> // bigobj doesn't have this field.<br>
> if (COFFBigObjHeader)<br>
> return 0;<br>
> @@ -516,7 +517,7 @@ public:<br>
> }<br>
> uint16_t getCharacteristics() const {<br>
> if (COFFHeader)<br>
> - return COFFHeader->Characteristics;<br>
> + return COFFHeader->isImportLibrary() ? 0 : COFFHeader->Characteristics;<br>
> // bigobj doesn't have characteristics to speak of,<br>
> // editbin will silently lie to you if you attempt to set any.<br>
> if (COFFBigObjHeader)<br>
> @@ -532,21 +533,22 @@ public:<br>
> }<br>
> uint32_t getNumberOfSections() const {<br>
> if (COFFHeader)<br>
> - return COFFHeader->NumberOfSections;<br>
> + return COFFHeader->isImportLibrary() ? 0 : COFFHeader->NumberOfSections;<br>
> if (COFFBigObjHeader)<br>
> return COFFBigObjHeader->NumberOfSections;<br>
> llvm_unreachable("no COFF header!");<br>
> }<br>
> uint32_t getPointerToSymbolTable() const {<br>
> if (COFFHeader)<br>
> - return COFFHeader->PointerToSymbolTable;<br>
> + return COFFHeader->isImportLibrary() ? 0<br>
> + : COFFHeader->PointerToSymbolTable;<br>
> if (COFFBigObjHeader)<br>
> return COFFBigObjHeader->PointerToSymbolTable;<br>
> llvm_unreachable("no COFF header!");<br>
> }<br>
> uint32_t getNumberOfSymbols() const {<br>
> if (COFFHeader)<br>
> - return COFFHeader->NumberOfSymbols;<br>
> + return COFFHeader->isImportLibrary() ? 0 : COFFHeader->NumberOfSymbols;<br>
> if (COFFBigObjHeader)<br>
> return COFFBigObjHeader->NumberOfSymbols;<br>
> llvm_unreachable("no COFF header!");<br>
> @@ -657,7 +659,7 @@ public:<br>
> return EC;<br>
> return COFFSymbolRef(Symb);<br>
> }<br>
> - llvm_unreachable("no symbol table pointer!");<br>
> + return object_error::parse_failed;<br>
> }<br>
> template <typename T><br>
> std::error_code getAuxSymbol(uint32_t index, const T *&Res) const {<br>
><br>
> Modified: llvm/trunk/lib/Object/COFFObjectFile.cpp<br>
> URL: <a href="http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Object/COFFObjectFile.cpp?rev=222124&r1=222123&r2=222124&view=diff" target="_blank">http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Object/COFFObjectFile.cpp?rev=222124&r1=222123&r2=222124&view=diff</a><br>
> ==============================================================================<br>
> --- llvm/trunk/lib/Object/COFFObjectFile.cpp (original)<br>
> +++ llvm/trunk/lib/Object/COFFObjectFile.cpp Mon Nov 17 05:17:17 2014<br>
> @@ -54,7 +54,7 @@ static std::error_code checkOffset(Memor<br>
> template <typename T><br>
> static std::error_code getObject(const T *&Obj, MemoryBufferRef M,<br>
> const void *Ptr,<br>
> - const size_t Size = sizeof(T)) {<br>
> + const uint64_t Size = sizeof(T)) {<br>
> uintptr_t Addr = uintptr_t(Ptr);<br>
> if (std::error_code EC = checkOffset(M, Addr, Size))<br>
> return EC;<br>
> @@ -101,13 +101,10 @@ const coff_symbol_type *COFFObjectFile::<br>
> const coff_symbol_type *Addr =<br>
> reinterpret_cast<const coff_symbol_type *>(Ref.p);<br>
><br>
> + assert(!checkOffset(Data, uintptr_t(Addr), sizeof(*Addr)));<br>
> #ifndef NDEBUG<br>
> // Verify that the symbol points to a valid entry in the symbol table.<br>
> uintptr_t Offset = uintptr_t(Addr) - uintptr_t(base());<br>
> - if (Offset < getPointerToSymbolTable() ||<br>
> - Offset >= getPointerToSymbolTable() +<br>
> - (getNumberOfSymbols() * sizeof(coff_symbol_type)))<br>
> - report_fatal_error("Symbol was outside of symbol table.");<br>
><br>
> assert((Offset - getPointerToSymbolTable()) % sizeof(coff_symbol_type) == 0 &&<br>
> "Symbol did not point to the beginning of a symbol");<br>
> @@ -133,14 +130,15 @@ const coff_section *COFFObjectFile::toSe<br>
> }<br>
><br>
> void COFFObjectFile::moveSymbolNext(DataRefImpl &Ref) const {<br>
> + auto End = reinterpret_cast<uintptr_t>(StringTable);<br>
> if (SymbolTable16) {<br>
> const coff_symbol16 *Symb = toSymb<coff_symbol16>(Ref);<br>
> Symb += 1 + Symb->NumberOfAuxSymbols;<br>
> - Ref.p = reinterpret_cast<uintptr_t>(Symb);<br>
> + Ref.p = std::min(reinterpret_cast<uintptr_t>(Symb), End);<br>
> } else if (SymbolTable32) {<br>
> const coff_symbol32 *Symb = toSymb<coff_symbol32>(Ref);<br>
> Symb += 1 + Symb->NumberOfAuxSymbols;<br>
> - Ref.p = reinterpret_cast<uintptr_t>(Symb);<br>
> + Ref.p = std::min(reinterpret_cast<uintptr_t>(Symb), End);<br>
> } else {<br>
> llvm_unreachable("no symbol table pointer!");<br>
> }<br>
> @@ -365,8 +363,12 @@ bool COFFObjectFile::isSectionBSS(DataRe<br>
> }<br>
><br>
> bool COFFObjectFile::isSectionRequiredForExecution(DataRefImpl Ref) const {<br>
> - // FIXME: Unimplemented<br>
> - return true;<br>
> + // Sections marked 'Info', 'Remove', or 'Discardable' aren't required for<br>
> + // execution.<br>
> + const coff_section *Sec = toSec(Ref);<br>
> + return !(Sec->Characteristics &<br>
> + (COFF::IMAGE_SCN_LNK_INFO | COFF::IMAGE_SCN_LNK_REMOVE |<br>
> + COFF::IMAGE_SCN_MEM_DISCARDABLE));<br>
> }<br>
><br>
> bool COFFObjectFile::isSectionVirtual(DataRefImpl Ref) const {<br>
> @@ -375,13 +377,22 @@ bool COFFObjectFile::isSectionVirtual(Da<br>
> }<br>
><br>
> bool COFFObjectFile::isSectionZeroInit(DataRefImpl Ref) const {<br>
> - // FIXME: Unimplemented.<br>
> - return false;<br>
> + const coff_section *Sec = toSec(Ref);<br>
> + return Sec->Characteristics & COFF::IMAGE_SCN_CNT_UNINITIALIZED_DATA;<br>
> }<br>
><br>
> bool COFFObjectFile::isSectionReadOnlyData(DataRefImpl Ref) const {<br>
> - // FIXME: Unimplemented.<br>
> - return false;<br>
> + const coff_section *Sec = toSec(Ref);<br>
> + // Check if it's any sort of data section.<br>
> + if (!(Sec->Characteristics & (COFF::IMAGE_SCN_CNT_UNINITIALIZED_DATA |<br>
> + COFF::IMAGE_SCN_CNT_INITIALIZED_DATA)))<br>
> + return false;<br>
> + // If it's writable or executable or contains code, it isn't read-only data.<br>
> + if (Sec->Characteristics &<br>
> + (COFF::IMAGE_SCN_CNT_CODE | COFF::IMAGE_SCN_MEM_EXECUTE |<br>
> + COFF::IMAGE_SCN_MEM_WRITE))<br>
> + return false;<br>
> + return true;<br>
> }<br>
><br>
> bool COFFObjectFile::sectionContainsSymbol(DataRefImpl SecRef,<br>
> @@ -446,15 +457,15 @@ relocation_iterator COFFObjectFile::sect<br>
> // Initialize the pointer to the symbol table.<br>
> std::error_code COFFObjectFile::initSymbolTablePtr() {<br>
> if (COFFHeader)<br>
> - if (std::error_code EC =<br>
> - getObject(SymbolTable16, Data, base() + getPointerToSymbolTable(),<br>
> - getNumberOfSymbols() * getSymbolTableEntrySize()))<br>
> + if (std::error_code EC = getObject(<br>
> + SymbolTable16, Data, base() + getPointerToSymbolTable(),<br>
> + (uint64_t)getNumberOfSymbols() * getSymbolTableEntrySize()))<br>
> return EC;<br>
><br>
> if (COFFBigObjHeader)<br>
> - if (std::error_code EC =<br>
> - getObject(SymbolTable32, Data, base() + getPointerToSymbolTable(),<br>
> - getNumberOfSymbols() * getSymbolTableEntrySize()))<br>
> + if (std::error_code EC = getObject(<br>
> + SymbolTable32, Data, base() + getPointerToSymbolTable(),<br>
> + (uint64_t)getNumberOfSymbols() * getSymbolTableEntrySize()))<br>
> return EC;<br>
><br>
> // Find string table. The first four byte of the string table contains the<br>
> @@ -681,13 +692,20 @@ COFFObjectFile::COFFObjectFile(MemoryBuf<br>
> }<br>
><br>
> if ((EC = getObject(SectionTable, Data, base() + CurPtr,<br>
> - getNumberOfSections() * sizeof(coff_section))))<br>
> + (uint64_t)getNumberOfSections() * sizeof(coff_section))))<br>
> return;<br>
><br>
> // Initialize the pointer to the symbol table.<br>
> - if (getPointerToSymbolTable() != 0)<br>
> + if (getPointerToSymbolTable() != 0) {<br>
> if ((EC = initSymbolTablePtr()))<br>
> return;<br>
> + } else {<br>
> + // We had better not have any symbols if we don't have a symbol table.<br>
> + if (getNumberOfSymbols() != 0) {<br>
> + EC = object_error::parse_failed;<br>
> + return;<br>
> + }<br>
> + }<br>
><br>
> // Initialize the pointer to the beginning of the import table.<br>
> if ((EC = initImportTablePtr()))<br>
> @@ -843,15 +861,15 @@ COFFObjectFile::getDataDirectory(uint32_<br>
><br>
> std::error_code COFFObjectFile::getSection(int32_t Index,<br>
> const coff_section *&Result) const {<br>
> - // Check for special index values.<br>
> + Result = nullptr;<br>
> if (COFF::isReservedSectionNumber(Index))<br>
> - Result = nullptr;<br>
> - else if (Index > 0 && static_cast<uint32_t>(Index) <= getNumberOfSections())<br>
> + return object_error::success;<br>
> + if (static_cast<uint32_t>(Index) <= getNumberOfSections()) {<br>
> // We already verified the section table data, so no need to check again.<br>
> Result = SectionTable + (Index - 1);<br>
> - else<br>
> - return object_error::parse_failed;<br>
> - return object_error::success;<br>
> + return object_error::success;<br>
> + }<br>
> + return object_error::parse_failed;<br>
> }<br>
><br>
> std::error_code COFFObjectFile::getString(uint32_t Offset,<br>
> @@ -1001,6 +1019,8 @@ std::error_code COFFObjectFile::getReloc<br>
> symbol_iterator COFFObjectFile::getRelocationSymbol(DataRefImpl Rel) const {<br>
> const coff_relocation *R = toRel(Rel);<br>
> DataRefImpl Ref;<br>
> + if (R->SymbolTableIndex >= getNumberOfSymbols())<br>
> + return symbol_end();<br>
> if (SymbolTable16)<br>
> Ref.p = reinterpret_cast<uintptr_t>(SymbolTable16 + R->SymbolTableIndex);<br>
> else if (SymbolTable32)<br>
><br>
> Modified: llvm/trunk/test/tools/llvm-readobj/file-headers.test<br>
> URL: <a href="http://llvm.org/viewvc/llvm-project/llvm/trunk/test/tools/llvm-readobj/file-headers.test?rev=222124&r1=222123&r2=222124&view=diff" target="_blank">http://llvm.org/viewvc/llvm-project/llvm/trunk/test/tools/llvm-readobj/file-headers.test?rev=222124&r1=222123&r2=222124&view=diff</a><br>
> ==============================================================================<br>
> --- llvm/trunk/test/tools/llvm-readobj/file-headers.test (original)<br>
> +++ llvm/trunk/test/tools/llvm-readobj/file-headers.test Mon Nov 17 05:17:17 2014<br>
> @@ -335,12 +335,11 @@ COFF-IMPORTLIB-NEXT: Arch: unknown<br>
> COFF-IMPORTLIB-NEXT: AddressSize: 32bit<br>
> COFF-IMPORTLIB-NEXT: ImageFileHeader {<br>
> COFF-IMPORTLIB-NEXT: Machine: IMAGE_FILE_MACHINE_UNKNOWN (0x0)<br>
> -COFF-IMPORTLIB-NEXT: SectionCount: 65535<br>
> +COFF-IMPORTLIB-NEXT: SectionCount: 0<br>
> COFF-IMPORTLIB-NEXT: TimeDateStamp: 1970-09-09 19:52:32 (0x14C0000)<br>
> -COFF-IMPORTLIB-NEXT: PointerToSymbolTable: 0x528542EB<br>
> -COFF-IMPORTLIB-NEXT: SymbolCount: 20<br>
> +COFF-IMPORTLIB-NEXT: PointerToSymbolTable: 0x0<br>
> +COFF-IMPORTLIB-NEXT: SymbolCount: 0<br>
> COFF-IMPORTLIB-NEXT: OptionalHeaderSize: 0<br>
> -COFF-IMPORTLIB-NEXT: Characteristics [ (0x8)<br>
> -COFF-IMPORTLIB-NEXT: IMAGE_FILE_LOCAL_SYMS_STRIPPED (0x8)<br>
> +COFF-IMPORTLIB-NEXT: Characteristics [ (0x0)<br>
> COFF-IMPORTLIB-NEXT: ]<br>
> COFF-IMPORTLIB-NEXT: }<br>
><br>
> Modified: llvm/trunk/tools/llvm-objdump/llvm-objdump.cpp<br>
> URL: <a href="http://llvm.org/viewvc/llvm-project/llvm/trunk/tools/llvm-objdump/llvm-objdump.cpp?rev=222124&r1=222123&r2=222124&view=diff" target="_blank">http://llvm.org/viewvc/llvm-project/llvm/trunk/tools/llvm-objdump/llvm-objdump.cpp?rev=222124&r1=222123&r2=222124&view=diff</a><br>
> ==============================================================================<br>
> --- llvm/trunk/tools/llvm-objdump/llvm-objdump.cpp (original)<br>
> +++ llvm/trunk/tools/llvm-objdump/llvm-objdump.cpp Mon Nov 17 05:17:17 2014<br>
> @@ -307,8 +307,7 @@ static void DisassembleObject(const Obje<br>
> }<br>
><br>
> for (const SectionRef &Section : Obj->sections()) {<br>
> - bool Text = Section.isText();<br>
> - if (!Text)<br>
> + if (!Section.isText() || Section.isVirtual())<br>
> continue;<br>
><br>
> uint64_t SectionAddr = Section.getAddress();<br>
><br>
> Modified: llvm/trunk/tools/llvm-readobj/COFFDumper.cpp<br>
> URL: <a href="http://llvm.org/viewvc/llvm-project/llvm/trunk/tools/llvm-readobj/COFFDumper.cpp?rev=222124&r1=222123&r2=222124&view=diff" target="_blank">http://llvm.org/viewvc/llvm-project/llvm/trunk/tools/llvm-readobj/COFFDumper.cpp?rev=222124&r1=222123&r2=222124&view=diff</a><br>
> ==============================================================================<br>
> --- llvm/trunk/tools/llvm-readobj/COFFDumper.cpp (original)<br>
> +++ llvm/trunk/tools/llvm-readobj/COFFDumper.cpp Mon Nov 17 05:17:17 2014<br>
> @@ -825,22 +825,22 @@ void COFFDumper::printSymbols() {<br>
><br>
> void COFFDumper::printDynamicSymbols() { ListScope Group(W, "DynamicSymbols"); }<br>
><br>
> -static StringRef getSectionName(const llvm::object::COFFObjectFile *Obj,<br>
> - COFFSymbolRef Symbol,<br>
> - const coff_section *Section) {<br>
> +static ErrorOr<StringRef><br>
> +getSectionName(const llvm::object::COFFObjectFile *Obj, int32_t SectionNumber,<br>
> + const coff_section *Section) {<br>
> if (Section) {<br>
> StringRef SectionName;<br>
> - Obj->getSectionName(Section, SectionName);<br>
> + if (std::error_code EC = Obj->getSectionName(Section, SectionName))<br>
> + return EC;<br>
> return SectionName;<br>
> }<br>
> - int32_t SectionNumber = Symbol.getSectionNumber();<br>
> if (SectionNumber == llvm::COFF::IMAGE_SYM_DEBUG)<br>
> - return "IMAGE_SYM_DEBUG";<br>
> + return StringRef("IMAGE_SYM_DEBUG");<br>
> if (SectionNumber == llvm::COFF::IMAGE_SYM_ABSOLUTE)<br>
> - return "IMAGE_SYM_ABSOLUTE";<br>
> + return StringRef("IMAGE_SYM_ABSOLUTE");<br>
> if (SectionNumber == llvm::COFF::IMAGE_SYM_UNDEFINED)<br>
> - return "IMAGE_SYM_UNDEFINED";<br>
> - return "";<br>
> + return StringRef("IMAGE_SYM_UNDEFINED");<br>
> + return StringRef("");<br>
> }<br>
><br>
> void COFFDumper::printSymbol(const SymbolRef &Sym) {<br>
> @@ -858,7 +858,11 @@ void COFFDumper::printSymbol(const Symbo<br>
> if (Obj->getSymbolName(Symbol, SymbolName))<br>
> SymbolName = "";<br>
><br>
> - StringRef SectionName = getSectionName(Obj, Symbol, Section);<br>
> + StringRef SectionName = "";<br>
> + ErrorOr<StringRef> Res =<br>
> + getSectionName(Obj, Symbol.getSectionNumber(), Section);<br>
> + if (Res)<br>
> + SectionName = *Res;<br>
><br>
> W.printString("Name", SymbolName);<br>
> W.printNumber("Value", Symbol.getValue());<br>
> @@ -929,10 +933,14 @@ void COFFDumper::printSymbol(const Symbo<br>
> if (Section && Section->Characteristics & COFF::IMAGE_SCN_LNK_COMDAT<br>
> && Aux->Selection == COFF::IMAGE_COMDAT_SELECT_ASSOCIATIVE) {<br>
> const coff_section *Assoc;<br>
> - StringRef AssocName;<br>
> - std::error_code EC;<br>
> - if ((EC = Obj->getSection(AuxNumber, Assoc)) ||<br>
> - (EC = Obj->getSectionName(Assoc, AssocName))) {<br>
> + StringRef AssocName = "";<br>
> + std::error_code EC = Obj->getSection(AuxNumber, Assoc);<br>
> + ErrorOr<StringRef> Res = getSectionName(Obj, AuxNumber, Assoc);<br>
> + if (Res)<br>
> + AssocName = *Res;<br>
> + if (!EC)<br>
> + EC = Res.getError();<br>
> + if (EC) {<br>
> AssocName = "";<br>
> error(EC);<br>
> }<br>
><br>
><br>
> _______________________________________________<br>
> llvm-commits mailing list<br>
> <a href="mailto:llvm-commits@cs.uiuc.edu">llvm-commits@cs.uiuc.edu</a><br>
> <a href="http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits" target="_blank">http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits</a><br>
</div></div></blockquote></div><br></div>