[llvm] 407d420 - [lib/Object] - Make ELFObjectFile::getSymbol() return Expected<>.
Georgii Rymar via llvm-commits
llvm-commits at lists.llvm.org
Wed Dec 16 02:26:19 PST 2020
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));
+}
More information about the llvm-commits
mailing list