[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

llvm/include/llvm/Object/ELFObjectFile.h

@@ -408,11 +408,8 @@ public:
   const Elf_Rel *getRel(DataRefImpl Rel) const;
   const Elf_Rela *getRela(DataRefImpl Rela) const;
 
-  const Elf_Sym *getSymbol(DataRefImpl Sym) const {
+  Expected<const Elf_Sym *> getSymbol(DataRefImpl Sym) const {
-    auto Ret = EF.template getEntry<Elf_Sym>(Sym.d.a, Sym.d.b);
+    return EF.template getEntry<Elf_Sym>(Sym.d.a, Sym.d.b);
-    if (!Ret)
-      report_fatal_error(errorToErrorCode(Ret.takeError()).message());
-    return *Ret;
   }
 
   /// 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);
+  Expected<const Elf_Sym *> SymOrErr = getSymbol(Symb);
-  uint64_t Ret = ESym->st_value;
+  if (!SymOrErr)
-  if (ESym->st_shndx == ELF::SHN_ABS)
+    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);
+  Expected<const Elf_Sym *> SymOrErr = getSymbol(Symb);
-  switch (ESym->st_shndx) {
+  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);
+  Expected<const Elf_Sym *> SymOrErr = getSymbol(Symb);
-  if (Sym->st_shndx == ELF::SHN_COMMON)
+  if (!SymOrErr)
-    return Sym->st_value;
+    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(*SymOrErr, *SymTabOrErr);
-  return getSymbolSection(Sym, SymTab);
 }
 
 template <class ELFT>

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());
+    Expected<const typename ELFT::Sym *> SymOrErr =
-    if (Sym->getType() == ELF::STT_SECTION) {
+        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();

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())

llvm/unittests/Object/ELFObjectFileTest.cpp

@@ -397,3 +397,75 @@ ProgramHeaders:
   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));
+}