Author: Georgii Rymar Date: 2020-12-16T13:14:23+03:00 New Revision: 407d42002904ce541f732ce4300913ef57cff232
URL: https://github.com/llvm/llvm-project/commit/407d42002904ce541f732ce4300913ef57cff232 DIFF: https://github.com/llvm/llvm-project/commit/407d42002904ce541f732ce4300913ef57cff232.diff LOG: [lib/Object] - Make ELFObjectFile::getSymbol() return Expected<>. This was requested in comments for D93209: https://reviews.llvm.org/D93209#inline-871192 D93209 fixes an issue with `ELFFile<ELFT>::getEntry`, after what `getSymbol` starts calling `report_fatal_error` for previously missed invalid cases. This patch makes it return `Expected<>` and updates callers. For few of them I had to add new `report_fatal_error` calls. But I see no way to avoid it currently. The change would affects too many places, e.g: `getSymbolBinding` and other methods are used from `ELFSymbolRef` which is used in too many places across LLVM. Differential revision: https://reviews.llvm.org/D93297 Added: Modified: llvm/include/llvm/Object/ELFObjectFile.h llvm/tools/llvm-objdump/ELFDump.cpp llvm/tools/llvm-objdump/llvm-objdump.cpp llvm/unittests/Object/ELFObjectFileTest.cpp Removed: ################################################################################ diff --git a/llvm/include/llvm/Object/ELFObjectFile.h b/llvm/include/llvm/Object/ELFObjectFile.h index ae4521624077..ca4363572d90 100644 --- a/llvm/include/llvm/Object/ELFObjectFile.h +++ b/llvm/include/llvm/Object/ELFObjectFile.h @@ -408,11 +408,8 @@ template <class ELFT> class ELFObjectFile : public ELFObjectFileBase { const Elf_Rel *getRel(DataRefImpl Rel) const; const Elf_Rela *getRela(DataRefImpl Rela) const; - const Elf_Sym *getSymbol(DataRefImpl Sym) const { - auto Ret = EF.template getEntry<Elf_Sym>(Sym.d.a, Sym.d.b); - if (!Ret) - report_fatal_error(errorToErrorCode(Ret.takeError()).message()); - return *Ret; + Expected<const Elf_Sym *> getSymbol(DataRefImpl Sym) const { + return EF.template getEntry<Elf_Sym>(Sym.d.a, Sym.d.b); } /// Get the relocation section that contains \a Rel. @@ -499,7 +496,9 @@ template <class ELFT> Error ELFObjectFile<ELFT>::initContent() { template <class ELFT> Expected<StringRef> ELFObjectFile<ELFT>::getSymbolName(DataRefImpl Sym) const { - const Elf_Sym *ESym = getSymbol(Sym); + Expected<const Elf_Sym *> SymOrErr = getSymbol(Sym); + if (!SymOrErr) + return SymOrErr.takeError(); auto SymTabOrErr = EF.getSection(Sym.d.a); if (!SymTabOrErr) return SymTabOrErr.takeError(); @@ -511,12 +510,12 @@ Expected<StringRef> ELFObjectFile<ELFT>::getSymbolName(DataRefImpl Sym) const { auto SymStrTabOrErr = EF.getStringTable(*StringTableSec); if (!SymStrTabOrErr) return SymStrTabOrErr.takeError(); - Expected<StringRef> Name = ESym->getName(*SymStrTabOrErr); + Expected<StringRef> Name = (*SymOrErr)->getName(*SymStrTabOrErr); if (Name && !Name->empty()) return Name; // If the symbol name is empty use the section name. - if (ESym->getType() == ELF::STT_SECTION) { + if ((*SymOrErr)->getType() == ELF::STT_SECTION) { if (Expected<section_iterator> SecOrErr = getSymbolSection(Sym)) { consumeError(Name.takeError()); return (*SecOrErr)->getName(); @@ -542,15 +541,18 @@ uint64_t ELFObjectFile<ELFT>::getSectionOffset(DataRefImpl Sec) const { template <class ELFT> uint64_t ELFObjectFile<ELFT>::getSymbolValueImpl(DataRefImpl Symb) const { - const Elf_Sym *ESym = getSymbol(Symb); - uint64_t Ret = ESym->st_value; - if (ESym->st_shndx == ELF::SHN_ABS) + Expected<const Elf_Sym *> SymOrErr = getSymbol(Symb); + if (!SymOrErr) + report_fatal_error(SymOrErr.takeError()); + + uint64_t Ret = (*SymOrErr)->st_value; + if ((*SymOrErr)->st_shndx == ELF::SHN_ABS) return Ret; const Elf_Ehdr &Header = EF.getHeader(); // Clear the ARM/Thumb or microMIPS indicator flag. if ((Header.e_machine == ELF::EM_ARM || Header.e_machine == ELF::EM_MIPS) && - ESym->getType() == ELF::STT_FUNC) + (*SymOrErr)->getType() == ELF::STT_FUNC) Ret &= ~1; return Ret; @@ -565,8 +567,11 @@ ELFObjectFile<ELFT>::getSymbolAddress(DataRefImpl Symb) const { return SymbolValueOrErr.takeError(); uint64_t Result = *SymbolValueOrErr; - const Elf_Sym *ESym = getSymbol(Symb); - switch (ESym->st_shndx) { + Expected<const Elf_Sym *> SymOrErr = getSymbol(Symb); + if (!SymOrErr) + return SymOrErr.takeError(); + + switch ((*SymOrErr)->st_shndx) { case ELF::SHN_COMMON: case ELF::SHN_UNDEF: case ELF::SHN_ABS: @@ -589,7 +594,7 @@ ELFObjectFile<ELFT>::getSymbolAddress(DataRefImpl Symb) const { } Expected<const Elf_Shdr *> SectionOrErr = - EF.getSection(*ESym, *SymTabOrErr, ShndxTable); + EF.getSection(**SymOrErr, *SymTabOrErr, ShndxTable); if (!SectionOrErr) return SectionOrErr.takeError(); const Elf_Shdr *Section = *SectionOrErr; @@ -602,9 +607,11 @@ ELFObjectFile<ELFT>::getSymbolAddress(DataRefImpl Symb) const { template <class ELFT> uint32_t ELFObjectFile<ELFT>::getSymbolAlignment(DataRefImpl Symb) const { - const Elf_Sym *Sym = getSymbol(Symb); - if (Sym->st_shndx == ELF::SHN_COMMON) - return Sym->st_value; + Expected<const Elf_Sym *> SymOrErr = getSymbol(Symb); + if (!SymOrErr) + report_fatal_error(SymOrErr.takeError()); + if ((*SymOrErr)->st_shndx == ELF::SHN_COMMON) + return (*SymOrErr)->st_value; return 0; } @@ -619,35 +626,49 @@ template <class ELFT> uint16_t ELFObjectFile<ELFT>::getEType() const { template <class ELFT> uint64_t ELFObjectFile<ELFT>::getSymbolSize(DataRefImpl Sym) const { - return getSymbol(Sym)->st_size; + Expected<const Elf_Sym *> SymOrErr = getSymbol(Sym); + if (!SymOrErr) + report_fatal_error(SymOrErr.takeError()); + return (*SymOrErr)->st_size; } template <class ELFT> uint64_t ELFObjectFile<ELFT>::getCommonSymbolSizeImpl(DataRefImpl Symb) const { - return getSymbol(Symb)->st_size; + return getSymbolSize(Symb); } template <class ELFT> uint8_t ELFObjectFile<ELFT>::getSymbolBinding(DataRefImpl Symb) const { - return getSymbol(Symb)->getBinding(); + Expected<const Elf_Sym *> SymOrErr = getSymbol(Symb); + if (!SymOrErr) + report_fatal_error(SymOrErr.takeError()); + return (*SymOrErr)->getBinding(); } template <class ELFT> uint8_t ELFObjectFile<ELFT>::getSymbolOther(DataRefImpl Symb) const { - return getSymbol(Symb)->st_other; + Expected<const Elf_Sym *> SymOrErr = getSymbol(Symb); + if (!SymOrErr) + report_fatal_error(SymOrErr.takeError()); + return (*SymOrErr)->st_other; } template <class ELFT> uint8_t ELFObjectFile<ELFT>::getSymbolELFType(DataRefImpl Symb) const { - return getSymbol(Symb)->getType(); + Expected<const Elf_Sym *> SymOrErr = getSymbol(Symb); + if (!SymOrErr) + report_fatal_error(SymOrErr.takeError()); + return (*SymOrErr)->getType(); } template <class ELFT> Expected<SymbolRef::Type> ELFObjectFile<ELFT>::getSymbolType(DataRefImpl Symb) const { - const Elf_Sym *ESym = getSymbol(Symb); + Expected<const Elf_Sym *> SymOrErr = getSymbol(Symb); + if (!SymOrErr) + return SymOrErr.takeError(); - switch (ESym->getType()) { + switch ((*SymOrErr)->getType()) { case ELF::STT_NOTYPE: return SymbolRef::ST_Unknown; case ELF::STT_SECTION: @@ -667,8 +688,11 @@ ELFObjectFile<ELFT>::getSymbolType(DataRefImpl Symb) const { template <class ELFT> Expected<uint32_t> ELFObjectFile<ELFT>::getSymbolFlags(DataRefImpl Sym) const { - const Elf_Sym *ESym = getSymbol(Sym); + Expected<const Elf_Sym *> SymOrErr = getSymbol(Sym); + if (!SymOrErr) + return SymOrErr.takeError(); + const Elf_Sym *ESym = *SymOrErr; uint32_t Result = SymbolRef::SF_None; if (ESym->getBinding() != ELF::STB_LOCAL) @@ -760,12 +784,14 @@ ELFObjectFile<ELFT>::getSymbolSection(const Elf_Sym *ESym, template <class ELFT> Expected<section_iterator> ELFObjectFile<ELFT>::getSymbolSection(DataRefImpl Symb) const { - const Elf_Sym *Sym = getSymbol(Symb); + Expected<const Elf_Sym *> SymOrErr = getSymbol(Symb); + if (!SymOrErr) + return SymOrErr.takeError(); + auto SymTabOrErr = EF.getSection(Symb.d.a); if (!SymTabOrErr) return SymTabOrErr.takeError(); - const Elf_Shdr *SymTab = *SymTabOrErr; - return getSymbolSection(Sym, SymTab); + return getSymbolSection(*SymOrErr, *SymTabOrErr); } template <class ELFT> diff --git a/llvm/tools/llvm-objdump/ELFDump.cpp b/llvm/tools/llvm-objdump/ELFDump.cpp index 031edb37f5cd..1c4d59179cc7 100644 --- a/llvm/tools/llvm-objdump/ELFDump.cpp +++ b/llvm/tools/llvm-objdump/ELFDump.cpp @@ -85,8 +85,13 @@ static Error getRelocationValueString(const ELFObjectFile<ELFT> *Obj, if (!Undef) { symbol_iterator SI = RelRef.getSymbol(); - const typename ELFT::Sym *Sym = Obj->getSymbol(SI->getRawDataRefImpl()); - if (Sym->getType() == ELF::STT_SECTION) { + Expected<const typename ELFT::Sym *> SymOrErr = + Obj->getSymbol(SI->getRawDataRefImpl()); + // TODO: test this error. + if (!SymOrErr) + return SymOrErr.takeError(); + + if ((*SymOrErr)->getType() == ELF::STT_SECTION) { Expected<section_iterator> SymSI = SI->getSection(); if (!SymSI) return SymSI.takeError(); diff --git a/llvm/tools/llvm-objdump/llvm-objdump.cpp b/llvm/tools/llvm-objdump/llvm-objdump.cpp index 96e936ec4e8f..6fda093351b9 100644 --- a/llvm/tools/llvm-objdump/llvm-objdump.cpp +++ b/llvm/tools/llvm-objdump/llvm-objdump.cpp @@ -1340,13 +1340,21 @@ PrettyPrinter &selectPrettyPrinter(Triple const &Triple) { static uint8_t getElfSymbolType(const ObjectFile *Obj, const SymbolRef &Sym) { assert(Obj->isELF()); if (auto *Elf32LEObj = dyn_cast<ELF32LEObjectFile>(Obj)) - return Elf32LEObj->getSymbol(Sym.getRawDataRefImpl())->getType(); + return unwrapOrError(Elf32LEObj->getSymbol(Sym.getRawDataRefImpl()), + Obj->getFileName()) + ->getType(); if (auto *Elf64LEObj = dyn_cast<ELF64LEObjectFile>(Obj)) - return Elf64LEObj->getSymbol(Sym.getRawDataRefImpl())->getType(); + return unwrapOrError(Elf64LEObj->getSymbol(Sym.getRawDataRefImpl()), + Obj->getFileName()) + ->getType(); if (auto *Elf32BEObj = dyn_cast<ELF32BEObjectFile>(Obj)) - return Elf32BEObj->getSymbol(Sym.getRawDataRefImpl())->getType(); + return unwrapOrError(Elf32BEObj->getSymbol(Sym.getRawDataRefImpl()), + Obj->getFileName()) + ->getType(); if (auto *Elf64BEObj = cast<ELF64BEObjectFile>(Obj)) - return Elf64BEObj->getSymbol(Sym.getRawDataRefImpl())->getType(); + return unwrapOrError(Elf64BEObj->getSymbol(Sym.getRawDataRefImpl()), + Obj->getFileName()) + ->getType(); llvm_unreachable("Unsupported binary format"); } @@ -1362,7 +1370,9 @@ addDynamicElfSymbols(const ELFObjectFile<ELFT> *Obj, // ELFSymbolRef::getAddress() returns size instead of value for common // symbols which is not desirable for disassembly output. Overriding. if (SymbolType == ELF::STT_COMMON) - Address = Obj->getSymbol(Symbol.getRawDataRefImpl())->st_value; + Address = unwrapOrError(Obj->getSymbol(Symbol.getRawDataRefImpl()), + Obj->getFileName()) + ->st_value; StringRef Name = unwrapOrError(Symbol.getName(), Obj->getFileName()); if (Name.empty()) diff --git a/llvm/unittests/Object/ELFObjectFileTest.cpp b/llvm/unittests/Object/ELFObjectFileTest.cpp index 30be24cef66a..aabb07c40559 100644 --- a/llvm/unittests/Object/ELFObjectFileTest.cpp +++ b/llvm/unittests/Object/ELFObjectFileTest.cpp @@ -397,3 +397,75 @@ TEST(ELFObjectFileTest, InvalidLoadSegmentsOrderTest) { EXPECT_EQ((const char *)Data - Buf.getBufferStart(), 0x4000); EXPECT_EQ(Data[0], 0x99); } + +// This is a test for API that is related to symbols. +// We check that errors are properly reported here. +TEST(ELFObjectFileTest, InvalidSymbolTest) { + SmallString<0> Storage; + Expected<ELFObjectFile<ELF64LE>> ElfOrErr = toBinary<ELF64LE>(Storage, R"( +--- !ELF +FileHeader: + Class: ELFCLASS64 + Data: ELFDATA2LSB + Type: ET_DYN + Machine: EM_X86_64 +Sections: + - Name: .symtab + Type: SHT_SYMTAB +)"); + + ASSERT_THAT_EXPECTED(ElfOrErr, Succeeded()); + const ELFFile<ELF64LE> &Elf = ElfOrErr->getELFFile(); + const ELFObjectFile<ELF64LE> &Obj = *ElfOrErr; + + Expected<const typename ELF64LE::Shdr *> SymtabSecOrErr = Elf.getSection(1); + ASSERT_THAT_EXPECTED(SymtabSecOrErr, Succeeded()); + ASSERT_EQ((*SymtabSecOrErr)->sh_type, ELF::SHT_SYMTAB); + + // We create a symbol with an index that is too large to exist in the object. + constexpr unsigned BrokenSymIndex = 0xFFFFFFFF; + ELFSymbolRef BrokenSym = Obj.toSymbolRef(*SymtabSecOrErr, BrokenSymIndex); + + const char *ErrMsg = "unable to access section [index 1] data at " + "0x1800000028: offset goes past the end of file"; + // 1) Check the behavior of ELFObjectFile<ELFT>::getSymbolName(). + // SymbolRef::getName() calls it internally. We can't test it directly, + // because it is protected. + EXPECT_THAT_ERROR(BrokenSym.getName().takeError(), FailedWithMessage(ErrMsg)); + + // 2) Check the behavior of ELFObjectFile<ELFT>::getSymbol(). + EXPECT_THAT_ERROR(Obj.getSymbol(BrokenSym.getRawDataRefImpl()).takeError(), + FailedWithMessage(ErrMsg)); + + // 3) Check the behavior of ELFObjectFile<ELFT>::getSymbolSection(). + // SymbolRef::getSection() calls it internally. We can't test it directly, + // because it is protected. + EXPECT_THAT_ERROR(BrokenSym.getSection().takeError(), + FailedWithMessage(ErrMsg)); + + // 4) Check the behavior of ELFObjectFile<ELFT>::getSymbolFlags(). + // SymbolRef::getFlags() calls it internally. We can't test it directly, + // because it is protected. + EXPECT_THAT_ERROR(BrokenSym.getFlags().takeError(), + FailedWithMessage(ErrMsg)); + + // 5) Check the behavior of ELFObjectFile<ELFT>::getSymbolType(). + // SymbolRef::getType() calls it internally. We can't test it directly, + // because it is protected. + EXPECT_THAT_ERROR(BrokenSym.getType().takeError(), FailedWithMessage(ErrMsg)); + + // 6) Check the behavior of ELFObjectFile<ELFT>::getSymbolAddress(). + // SymbolRef::getAddress() calls it internally. We can't test it directly, + // because it is protected. + EXPECT_THAT_ERROR(BrokenSym.getAddress().takeError(), + FailedWithMessage(ErrMsg)); + + // Finally, check the `ELFFile<ELFT>::getEntry` API. This is an underlying + // method that generates errors for all cases above. + EXPECT_THAT_EXPECTED(Elf.getEntry<typename ELF64LE::Sym>(**SymtabSecOrErr, 0), + Succeeded()); + EXPECT_THAT_ERROR( + Elf.getEntry<typename ELF64LE::Sym>(**SymtabSecOrErr, BrokenSymIndex) + .takeError(), + FailedWithMessage(ErrMsg)); +} _______________________________________________ llvm-branch-commits mailing list llvm-branch-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-branch-commits
