[lib/Object] - Make ELFObjectFile::getSymbol() return Expected<>.

[grimar]

Summary

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 too many places across LLVM.

[MaskRay]

LG

llvm/include/llvm/Object/ELFObjectFile.h

414

Just return EF.template getEntry<Elf_Sym>(Sym.d.a, Sym.d.b);

[grimar]

llvm/include/llvm/Object/ELFObjectFile.h

414

Right, thanks!

[jhenderson]

LGTM, with @MaskRay's suggestion. We can work on bubbling up the Error/Expected in due course to get rid of the report_fatal_error calls.

llvm/include/llvm/Object/ELFObjectFile.h

 public:
   ELFObjectFile(ELFObjectFile<ELFT> &&Other);
   static Expected<ELFObjectFile<ELFT>> create(MemoryBufferRef Object,
                                               bool InitContent = true);
 
   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.
   const Elf_Shdr *getRelSection(DataRefImpl Rel) const {
     auto RelSecOrErr = EF.getSection(Rel.d.a);
     if (!RelSecOrErr)
       report_fatal_error(errorToErrorCode(RelSecOrErr.takeError()).message());
     return *RelSecOrErr;
   }
 
template <class ELFT> Error ELFObjectFile<ELFT>::initContent() {
   }
 
   ContentValid = true;
   return Error::success();
 }
 
 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();
   const Elf_Shdr *SymTableSec = *SymTabOrErr;
   auto StrTabOrErr = EF.getSection(SymTableSec->sh_link);
   if (!StrTabOrErr)
     return StrTabOrErr.takeError();
   const Elf_Shdr *StringTableSec = *StrTabOrErr;
   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();
     }
   }
   return Name;
 }
 

 
 template <class ELFT>
 uint64_t ELFObjectFile<ELFT>::getSectionOffset(DataRefImpl Sec) const {
   return getSection(Sec)->sh_offset;
 }
 
 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;
 }
 
 template <class ELFT>
 Expected<uint64_t>
 ELFObjectFile<ELFT>::getSymbolAddress(DataRefImpl Symb) const {
   Expected<uint64_t> SymbolValueOrErr = getSymbolValue(Symb);
   if (!SymbolValueOrErr)
     // TODO: Test this error.
     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:
     return Result;
   }
 
   auto SymTabOrErr = EF.getSection(Symb.d.a);
   if (!SymTabOrErr)
     return SymTabOrErr.takeError();
 
   if (EF.getHeader().e_type == ELF::ET_REL) {
     ArrayRef<Elf_Word> ShndxTable;
     if (DotSymtabShndxSec) {
       // TODO: Test this error.
       if (Expected<ArrayRef<Elf_Word>> ShndxTableOrErr =
               EF.getSHNDXTable(*DotSymtabShndxSec))
         ShndxTable = *ShndxTableOrErr;
       else
         return ShndxTableOrErr.takeError();
     }
 
     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;
     if (Section)
       Result += Section->sh_addr;
   }
 
   return Result;
 }
 
 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;
 }
 
 template <class ELFT>
 uint16_t ELFObjectFile<ELFT>::getEMachine() const {
   return EF.getHeader().e_machine;
 }
 
 template <class ELFT> uint16_t ELFObjectFile<ELFT>::getEType() const {
   return EF.getHeader().e_type;
 }
 
 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:
     return SymbolRef::ST_Debug;
   case ELF::STT_FILE:
     return SymbolRef::ST_File;
   case ELF::STT_FUNC:
     return SymbolRef::ST_Function;
   case ELF::STT_OBJECT:
   case ELF::STT_COMMON:
   case ELF::STT_TLS:
     return SymbolRef::ST_Data;
   default:
     return SymbolRef::ST_Other;
   }
 }
 
 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)
     Result |= SymbolRef::SF_Global;
 
   if (ESym->getBinding() == ELF::STB_WEAK)
     Result |= SymbolRef::SF_Weak;
 
ELFObjectFile<ELFT>::getSymbolSection(const Elf_Sym *ESym,
   DataRefImpl Sec;
   Sec.p = reinterpret_cast<intptr_t>(ESec);
   return section_iterator(SectionRef(Sec, this));
 }
 
 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>
 void ELFObjectFile<ELFT>::moveSectionNext(DataRefImpl &Sec) const {
   const Elf_Shdr *ESec = getSection(Sec);
   Sec = toDRI(++ESec);
 }
 

llvm/tools/llvm-objdump/ELFDump.cpp

static Error getRelocationValueString(const ELFObjectFile<ELFT> *Obj,
 
   // Default scheme is to print Target, as well as "+ <addend>" for nonzero
   // addend. Should be acceptable for all normal purposes.
   std::string FmtBuf;
   raw_string_ostream Fmt(FmtBuf);
 
   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();
       const typename ELFT::Shdr *SymSec =
           Obj->getSection((*SymSI)->getRawDataRefImpl());
       auto SecName = EF.getSectionName(*SymSec);
       if (!SecName)
         return SecName.takeError();

llvm/tools/llvm-objdump/llvm-objdump.cpp

case Triple::bpfeb:
     return BPFPrettyPrinterInst;
   }
 }
 }
 
 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");
 }
 
 template <class ELFT> static void
 addDynamicElfSymbols(const ELFObjectFile<ELFT> *Obj,
                      std::map<SectionRef, SectionSymbolsTy> &AllSymbols) {
   for (auto Symbol : Obj->getDynamicSymbolIterators()) {
     uint8_t SymbolType = Symbol.getELFType();
     if (SymbolType == ELF::STT_SECTION)
       continue;
 
     uint64_t Address = unwrapOrError(Symbol.getAddress(), Obj->getFileName());
     // 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())
       continue;
 
     section_iterator SecI =
         unwrapOrError(Symbol.getSection(), Obj->getFileName());
     if (SecI == Obj->section_end())

llvm/unittests/Object/ELFObjectFileTest.cpp

)");
   EXPECT_EQ(Data[0], 0x11);
 
   Data = ToMappedAddr(0x2000);
   ASSERT_TRUE(Data);
   Buf = ExpectedFile->getMemoryBufferRef();
   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));
+}