[llvm/Object] - Make ELFObjectFile::getRelocatedSection return Expected<section_iterator>.

[grimar]

Summary

It returns just a section_iterator currently and have a report_fatal_error call inside.
This change adds a way to return errors and handle them on caller sides.

The patch also changes/improves current users and adds test cases.

Tools/places affected: llvm-readobj, llvm-objdump, llvm-cxxdump, llvm-dwarfdump, RuntimeDyld

[dblaikie]

Generally makes sense. Hopefully some other folks chime in too, though.

(I wonder if it makes sense not to necessarily test this codepath in every tool - seems like maybe too much test coverage)

[grimar]

In D69167#1714967, @dblaikie wrote:

Generally makes sense. Hopefully some other folks chime in too, though.

(I wonder if it makes sense not to necessarily test this codepath in every tool - seems like maybe too much test coverage)

Yes. I thought about the same.

I think it is good to have in llvm-readobj, llvm-objdump and llvm-dwarfdump, because these are tools used for inspecting corrupt
objects more often and it worth to check their error message text reported and the behavior. Adding tests for them was my primary
intention.

I added a test for llvm-cxxdump because it was easy, but I never used this tool before or wrote patches for it. I do not know how
much it is excessive.

I didn't add a test for RuntimeDyld because I am not familiar with it too and did not find how to do it quickly. Also I did not see any
other error checking tests around, so I supposed it might be not so important (to check a one more possible broken input case).

Given all the above I am going to commit this tomorrow. but will be happy to address any post-review comments/requests if any.

[jhenderson]

llvm/test/tools/llvm-readobj/stack-sizes.test

646

Nit: "Here the sh_info..."

llvm/include/llvm/Object/ELFObjectFile.h

protected:
   bool isSectionData(DataRefImpl Sec) const override;
   bool isSectionBSS(DataRefImpl Sec) const override;
   bool isSectionVirtual(DataRefImpl Sec) const override;
   bool isBerkeleyText(DataRefImpl Sec) const override;
   bool isBerkeleyData(DataRefImpl Sec) const override;
   relocation_iterator section_rel_begin(DataRefImpl Sec) const override;
   relocation_iterator section_rel_end(DataRefImpl Sec) const override;
   std::vector<SectionRef> dynamic_relocation_sections() const override;
-  section_iterator getRelocatedSection(DataRefImpl Sec) const override;
+  Expected<section_iterator>
+  getRelocatedSection(DataRefImpl Sec) const override;
 
   void moveRelocationNext(DataRefImpl &Rel) const override;
   uint64_t getRelocationOffset(DataRefImpl Rel) const override;
   symbol_iterator getRelocationSymbol(DataRefImpl Rel) const override;
   uint64_t getRelocationType(DataRefImpl Rel) const override;
   void getRelocationTypeName(DataRefImpl Rel,
                              SmallVectorImpl<char> &Result) const override;
 
ELFObjectFile<ELFT>::section_rel_end(DataRefImpl Sec) const {
   if (!SymSecOrErr)
     report_fatal_error(errorToErrorCode(SymSecOrErr.takeError()).message());
 
   RelData.d.b += S->sh_size / S->sh_entsize;
   return relocation_iterator(RelocationRef(RelData, this));
 }
 
 template <class ELFT>
-section_iterator
+Expected<section_iterator>
 ELFObjectFile<ELFT>::getRelocatedSection(DataRefImpl Sec) const {
   if (EF.getHeader()->e_type != ELF::ET_REL)
     return section_end();
 
   const Elf_Shdr *EShdr = getSection(Sec);
   uintX_t Type = EShdr->sh_type;
   if (Type != ELF::SHT_REL && Type != ELF::SHT_RELA)
     return section_end();
 
-  auto R = EF.getSection(EShdr->sh_info);
-  if (!R)
-    report_fatal_error(errorToErrorCode(R.takeError()).message());
-  return section_iterator(SectionRef(toDRI(*R), this));
+  Expected<const Elf_Shdr *> SecOrErr = EF.getSection(EShdr->sh_info);
+  if (!SecOrErr)
+    return SecOrErr.takeError();
+  return section_iterator(SectionRef(toDRI(*SecOrErr), this));
 }
 
 // Relocations
 template <class ELFT>
 void ELFObjectFile<ELFT>::moveRelocationNext(DataRefImpl &Rel) const {
   ++Rel.d.b;
 }
 

llvm/include/llvm/Object/ObjectFile.h

public:
 
   bool containsSymbol(SymbolRef S) const;
 
   relocation_iterator relocation_begin() const;
   relocation_iterator relocation_end() const;
   iterator_range<relocation_iterator> relocations() const {
     return make_range(relocation_begin(), relocation_end());
   }
-  section_iterator getRelocatedSection() const;
+  Expected<section_iterator> getRelocatedSection() const;
 
   DataRefImpl getRawDataRefImpl() const;
   const ObjectFile *getObject() const;
 };
 
 struct SectionedAddress {
   const static uint64_t UndefSection = UINT64_MAX;
 
protected:
   // A section is 'virtual' if its contents aren't present in the object image.
   virtual bool isSectionVirtual(DataRefImpl Sec) const = 0;
   virtual bool isSectionBitcode(DataRefImpl Sec) const;
   virtual bool isSectionStripped(DataRefImpl Sec) const;
   virtual bool isBerkeleyText(DataRefImpl Sec) const;
   virtual bool isBerkeleyData(DataRefImpl Sec) const;
   virtual relocation_iterator section_rel_begin(DataRefImpl Sec) const = 0;
   virtual relocation_iterator section_rel_end(DataRefImpl Sec) const = 0;
-  virtual section_iterator getRelocatedSection(DataRefImpl Sec) const;
+  virtual Expected<section_iterator> getRelocatedSection(DataRefImpl Sec) const;
 
   // Same as above for RelocationRef.
   friend class RelocationRef;
   virtual void moveRelocationNext(DataRefImpl &Rel) const = 0;
   virtual uint64_t getRelocationOffset(DataRefImpl Rel) const = 0;
   virtual symbol_iterator getRelocationSymbol(DataRefImpl Rel) const = 0;
   virtual uint64_t getRelocationType(DataRefImpl Rel) const = 0;
   virtual void getRelocationTypeName(DataRefImpl Rel,

 inline relocation_iterator SectionRef::relocation_begin() const {
   return OwningObject->section_rel_begin(SectionPimpl);
 }
 
 inline relocation_iterator SectionRef::relocation_end() const {
   return OwningObject->section_rel_end(SectionPimpl);
 }
 
-inline section_iterator SectionRef::getRelocatedSection() const {
+inline Expected<section_iterator> SectionRef::getRelocatedSection() const {
   return OwningObject->getRelocatedSection(SectionPimpl);
 }
 
 inline DataRefImpl SectionRef::getRawDataRefImpl() const {
   return SectionPimpl;
 }
 
 inline const ObjectFile *SectionRef::getObject() const {

llvm/lib/DebugInfo/DWARF/DWARFContext.cpp

for (const SectionRef &Section : Obj.sections()) {
       if (Section.isBSS() || Section.isVirtual())
         continue;
 
       // Skip sections stripped by dsymutil.
       if (Section.isStripped())
         continue;
 
       StringRef Data;
-      section_iterator RelocatedSection = Section.getRelocatedSection();
+      Expected<section_iterator> SecOrErr = Section.getRelocatedSection();
+      if (!SecOrErr) {
+        ErrorPolicy EP = HandleError(createError(
+            "failed to get relocated section: ", SecOrErr.takeError()));
+        if (EP == ErrorPolicy::Halt)
+          return;
+        continue;
+      }
+
       // Try to obtain an already relocated version of this section.
       // Else use the unrelocated section from the object file. We'll have to
       // apply relocations ourselves later.
+      section_iterator RelocatedSection = *SecOrErr;
       if (!L || !L->getLoadedSectionContents(*RelocatedSection, Data)) {
         Expected<StringRef> E = Section.getContents();
         if (E)
           Data = *E;
         else
           // maybeDecompress below will error.
           consumeError(E.takeError());
       }

llvm/lib/ExecutionEngine/RuntimeDyld/RuntimeDyld.cpp

if (auto Err = emitCommonSymbols(Obj, CommonSymbolsToAllocate, CommonSize,
                                    CommonAlign))
     return std::move(Err);
 
   // Parse and process relocations
   LLVM_DEBUG(dbgs() << "Parse relocations:\n");
   for (section_iterator SI = Obj.section_begin(), SE = Obj.section_end();
        SI != SE; ++SI) {
     StubMap Stubs;
-    section_iterator RelocatedSection = SI->getRelocatedSection();
 
+    Expected<section_iterator> RelSecOrErr = SI->getRelocatedSection();
+    if (!RelSecOrErr)
+      return RelSecOrErr.takeError();
+
+    section_iterator RelocatedSection = *RelSecOrErr;
     if (RelocatedSection == SE)
       continue;
 
     relocation_iterator I = SI->relocation_begin();
     relocation_iterator E = SI->relocation_end();
 
     if (I == E && !ProcessAllSections)
       continue;
if (StubSize == 0) {
     return 0;
   }
   // FIXME: this is an inefficient way to handle this. We should computed the
   // necessary section allocation size in loadObject by walking all the sections
   // once.
   unsigned StubBufSize = 0;
   for (section_iterator SI = Obj.section_begin(), SE = Obj.section_end();
        SI != SE; ++SI) {
-    section_iterator RelSecI = SI->getRelocatedSection();
+
+    Expected<section_iterator> RelSecOrErr = SI->getRelocatedSection();
+    if (!RelSecOrErr)
+      report_fatal_error(toString(RelSecOrErr.takeError()));
+
+    section_iterator RelSecI = *RelSecOrErr;
     if (!(RelSecI == Section))
       continue;
 
     for (const RelocationRef &Reloc : SI->relocations())
       if (relocationNeedsStub(Reloc))
         StubBufSize += StubSize;
   }
 

llvm/lib/ExecutionEngine/RuntimeDyld/RuntimeDyldELF.cpp

 // by Symbol.
 Error RuntimeDyldELF::findOPDEntrySection(const ELFObjectFileBase &Obj,
                                           ObjSectionToIDMap &LocalSections,
                                           RelocationValueRef &Rel) {
   // Get the ELF symbol value (st_value) to compare with Relocation offset in
   // .opd entries
   for (section_iterator si = Obj.section_begin(), se = Obj.section_end();
        si != se; ++si) {
-    section_iterator RelSecI = si->getRelocatedSection();
+
+    Expected<section_iterator> RelSecOrErr = si->getRelocatedSection();
+    if (!RelSecOrErr)
+      report_fatal_error(toString(RelSecOrErr.takeError()));
+
+    section_iterator RelSecI = *RelSecOrErr;
     if (RelSecI == Obj.section_end())
       continue;
 
     Expected<StringRef> NameOrErr = RelSecI->getName();
     if (!NameOrErr)
       return NameOrErr.takeError();
     StringRef RelSectionName = *NameOrErr;
 
if (GOTSectionID != 0) {
     // needed when GOT-based relocations are applied.
     memset(Addr, 0, TotalSize);
     if (IsMipsN32ABI || IsMipsN64ABI) {
       // To correctly resolve Mips GOT relocations, we need a mapping from
       // object's sections to GOTs.
       for (section_iterator SI = Obj.section_begin(), SE = Obj.section_end();
            SI != SE; ++SI) {
         if (SI->relocation_begin() != SI->relocation_end()) {
-          section_iterator RelocatedSection = SI->getRelocatedSection();
+          Expected<section_iterator> RelSecOrErr = SI->getRelocatedSection();
+          if (!RelSecOrErr)
+            return make_error<RuntimeDyldError>(
+                toString(RelSecOrErr.takeError()));
+
+          section_iterator RelocatedSection = *RelSecOrErr;
           ObjSectionToIDMap::iterator i = SectionMap.find(*RelocatedSection);
           assert (i != SectionMap.end());
           SectionToGOTMap[i->second] = GOTSectionID;
         }
       }
       GOTSymbolOffsets.clear();
     }
   }

llvm/lib/Object/ObjectFile.cpp

 bool ObjectFile::isBerkeleyText(DataRefImpl Sec) const {
   return isSectionText(Sec);
 }
 
 bool ObjectFile::isBerkeleyData(DataRefImpl Sec) const {
   return isSectionData(Sec);
 }
 
-section_iterator ObjectFile::getRelocatedSection(DataRefImpl Sec) const {
+Expected<section_iterator>
+ObjectFile::getRelocatedSection(DataRefImpl Sec) const {
   return section_iterator(SectionRef(Sec, this));
 }
 
 Triple ObjectFile::makeTriple() const {
   Triple TheTriple;
   auto Arch = getArch();
   TheTriple.setArch(Triple::ArchType(Arch));
 

llvm/test/tools/llvm-cxxdump/broken-reloc-sec.test

+## Check we report an error when trying to dump an object
+## which has a relocation section that has a broken sh_info
+## field, which is larger than the number of sections.
+
+# RUN: yaml2obj %s -o %t
+# RUN: not llvm-cxxdump %t 2>&1 | FileCheck %s
+# CHECK: error: reading file: invalid section index: 255
+
+--- !ELF
+FileHeader:
+  Class:   ELFCLASS64
+  Data:    ELFDATA2LSB
+  Type:    ET_REL
+  Machine: EM_X86_64
+Sections:
+- Name: .rela.foo
+  Type: SHT_RELA
+  Link: 0
+  Info: 0xFF
+  Relocations: []

llvm/test/tools/llvm-dwarfdump/elf-broken-reloc-target.yaml

+## Check we report an error if the relocated section identified by the
+## sh_info field of a relocation section is invalid.
+
+# RUN: yaml2obj %s -o %t
+# RUN: llvm-dwarfdump %t 2>&1 | FileCheck %s -DFILE=%t --check-prefix=ERR
+
+# ERR: error: failed to get relocated section: invalid section index: 255
+
+--- !ELF
+FileHeader:
+  Class:   ELFCLASS64
+  Data:    ELFDATA2LSB
+  Type:    ET_REL
+  Machine: EM_X86_64
+Sections:
+- Name: .rela.debug_info
+  Type: SHT_RELA
+  Link: 0
+  Info: 0xFF
+  Relocations: []

llvm/test/tools/llvm-objdump/X86/elf-disassemble-relocs.test

 ## Show that --disassemble + --reloc prints relocations inline and does not dump
 ## the relocation sections.
 
-# RUN: yaml2obj %s -o %t.o
-# RUN: llvm-objdump %t.o -d -r | FileCheck %s --implicit-check-not="RELOCATION RECORDS"
+# RUN: yaml2obj %s --docnum=1 -o %t1.o
+# RUN: llvm-objdump %t1.o -d -r | FileCheck %s --implicit-check-not="RELOCATION RECORDS"
 
 # CHECK:      0: e8 00 00 00 00                callq   0 <.text+0x5>
 # CHECK-NEXT:          0000000000000001:  R_X86_64_PC32        foo-4
 # CHECK-NEXT:          0000000000000002:  R_X86_64_NONE        bar+8
 # CHECK-NEXT: 5: e8 00 00 00 00                callq   0 <.text+0xa>
 # CHECK-NEXT:          0000000000000006:  R_X86_64_PLT32       foo+1
 
 --- !ELF
Relocations:
         Addend: 8
       - Offset: 6
         Symbol: foo
         Type:   R_X86_64_PLT32
         Addend: 1
 Symbols:
   - Name: foo
   - Name: bar
+
+## Check we report an error if the relocated section identified by the
+## sh_info field of a relocation section is invalid.
+
+# RUN: yaml2obj %s --docnum=2 -o %t2.o
+# RUN: not llvm-objdump %t2.o --disassemble --reloc 2>&1 | FileCheck %s -DFILE=%t2.o --check-prefix=ERR
+
+# ERR: error: '[[FILE]]': section (1): failed to get a relocated section: invalid section index: 255
+
+--- !ELF
+FileHeader:
+  Class:   ELFCLASS64
+  Data:    ELFDATA2LSB
+  Type:    ET_REL
+  Machine: EM_X86_64
+Sections:
+- Name: .rela.debug_info
+  Type: SHT_RELA
+  Link: 0
+  Info: 0xFF
+  Relocations: []

llvm/test/tools/llvm-objdump/relocations-elf.test

- Name:    glob1
     Size:    4
     Binding: STB_GLOBAL
   - Name:    glob2
     Binding: STB_GLOBAL
 
 ## Check we report an error if the relocated section identified by the
 ## sh_info field of a relocation section is invalid.
 # RUN: yaml2obj --docnum=2 %s > %t2
-# RUN: not llvm-objdump --reloc %t2 2>&1 | FileCheck %s --check-prefix=ERR
-# ERR: LLVM ERROR: Invalid data was encountered while parsing the file
+# RUN: not llvm-objdump --reloc %t2 2>&1 | FileCheck %s -DFILE=%t2 --check-prefix=ERR
+# ERR: error: '[[FILE]]': section (1): unable to get a relocation target: invalid section index: 255
 
 --- !ELF
 FileHeader:
   Class: ELFCLASS64
   Data: ELFDATA2LSB
   Type: ET_REL
   Machine: EM_X86_64
 Sections:
 - Name: .rela.foo
   Type: SHT_RELA
-  Info: 0x255
+  Info: 0xFF
   Relocations:
     - Offset: 0x1
       Type: R_X86_64_NONE

llvm/test/tools/llvm-readobj/stack-sizes.test

- Name: .stack_sizes
     Type: SHT_PROGBITS
     Content: "00"
   - Name: .rela.stack_sizes
     Type: SHT_RELA
     Info: .stack_sizes
     Relocations:
       - Offset: 0
         Type:   R_X86_64_64
+
+## Check we report an error when dumping stack sizes if the relocated section
+## identified by the sh_info field is invalid. Here sh_info value is larger than
+## the number of sections.
+
+# RUN: yaml2obj --docnum=13 %s > %t18
+# RUN: not llvm-readelf --stack-sizes %t18 2>&1 | FileCheck %s -DFILE=%t18 --check-prefix=INVALID-TARGET
+# RUN: not llvm-readobj --stack-sizes %t18 2>&1 | FileCheck %s -DFILE=%t18 --check-prefix=INVALID-TARGET
+
+# INVALID-TARGET: error: '[[FILE]]': .rela.stack_sizes: failed to get a relocated section: invalid section index: 255
+
+--- !ELF
+FileHeader:
+  Class:   ELFCLASS32
+  Data:    ELFDATA2MSB
+  Type:    ET_REL
+  Machine: EM_X86_64
+Sections:
+  - Name: .rela.stack_sizes
+    Type: SHT_RELA
+    Link: 0
+    Info: 0xFF
+    Relocations: []

llvm/tools/llvm-cxxdump/llvm-cxxdump.cpp

static void dumpCXXData(const ObjectFile *Obj) {
 
   std::map<std::pair<StringRef, uint64_t>, StringRef> VTableSymEntries;
   std::map<std::pair<StringRef, uint64_t>, int64_t> VTableDataEntries;
   std::map<std::pair<StringRef, uint64_t>, StringRef> VTTEntries;
   std::map<StringRef, StringRef> TINames;
 
   SectionRelocMap.clear();
   for (const SectionRef &Section : Obj->sections()) {
-    section_iterator Sec2 = Section.getRelocatedSection();
+    Expected<section_iterator> ErrOrSec = Section.getRelocatedSection();
+    if (!ErrOrSec)
+      error(ErrOrSec.takeError());
+
+    section_iterator Sec2 = *ErrOrSec;
     if (Sec2 != Obj->section_end())
       SectionRelocMap[*Sec2].push_back(Section);
   }
 
   uint8_t BytesInAddress = Obj->getBytesInAddress();
 
   std::vector<std::pair<SymbolRef, uint64_t>> SymAddr =
       object::computeSymbolSizes(*Obj);

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

static size_t countSkippableZeroBytes(ArrayRef<uint8_t> Buf) {
   // instruction if it starts with a zero byte.
   return N & ~0x3;
 }
 
 // Returns a map from sections to their relocations.
 static std::map<SectionRef, std::vector<RelocationRef>>
 getRelocsMap(object::ObjectFile const &Obj) {
   std::map<SectionRef, std::vector<RelocationRef>> Ret;
+  uint64_t I = (uint64_t)-1;
   for (SectionRef Sec : Obj.sections()) {
-    section_iterator Relocated = Sec.getRelocatedSection();
+    ++I;
+    Expected<section_iterator> RelocatedOrErr = Sec.getRelocatedSection();
+    if (!RelocatedOrErr)
+      reportError(Obj.getFileName(),
+                  "section (" + Twine(I) +
+                      "): failed to get a relocated section: " +
+                      toString(RelocatedOrErr.takeError()));
+
+    section_iterator Relocated = *RelocatedOrErr;
     if (Relocated == Obj.section_end() || !checkSectionFilter(*Relocated).Keep)
       continue;
     std::vector<RelocationRef> &V = Ret[*Relocated];
     for (const RelocationRef &R : Sec.relocations())
       V.push_back(R);
     // Sort relocations by address.
     llvm::stable_sort(V, isRelocAddressLess);
   }
void printRelocations(const ObjectFile *Obj) {
   // files.
   if (!Obj->isRelocatableObject())
     return;
 
   // Build a mapping from relocation target to a vector of relocation
   // sections. Usually, there is an only one relocation section for
   // each relocated section.
   MapVector<SectionRef, std::vector<SectionRef>> SecToRelSec;
-  for (const SectionRef &Section : ToolSectionFilter(*Obj)) {
+  uint64_t Ndx;
+  for (const SectionRef &Section : ToolSectionFilter(*Obj, &Ndx)) {
     if (Section.relocation_begin() == Section.relocation_end())
       continue;
-    const SectionRef TargetSec = *Section.getRelocatedSection();
-    SecToRelSec[TargetSec].push_back(Section);
+    Expected<section_iterator> SecOrErr = Section.getRelocatedSection();
+    if (!SecOrErr)
+      reportError(Obj->getFileName(),
+                  "section (" + Twine(Ndx) +
+                      "): unable to get a relocation target: " +
+                      toString(SecOrErr.takeError()));
+    SecToRelSec[**SecOrErr].push_back(Section);
   }
 
   for (std::pair<SectionRef, std::vector<SectionRef>> &P : SecToRelSec) {
     StringRef SecName = unwrapOrError(P.first.getName(), Obj->getFileName());
     outs() << "RELOCATION RECORDS FOR [" << SecName << "]:\n";
 
     for (SectionRef Section : P.second) {
       for (const RelocationRef &Reloc : Section.relocations()) {

llvm/tools/llvm-readobj/ELFDumper.cpp

for (const SectionRef &Sec : Obj->sections()) {
 
     // Check relocation sections if they are relocating contents of a
     // stack sizes section.
     const Elf_Shdr *ElfSec = Obj->getSection(Sec.getRawDataRefImpl());
     uint32_t SectionType = ElfSec->sh_type;
     if (SectionType != ELF::SHT_RELA && SectionType != ELF::SHT_REL)
       continue;
 
-    SectionRef Contents = *Sec.getRelocatedSection();
-    const Elf_Shdr *ContentsSec = Obj->getSection(Contents.getRawDataRefImpl());
+    Expected<section_iterator> RelSecOrErr = Sec.getRelocatedSection();
+    if (!RelSecOrErr)
+      reportError(createStringError(object_error::parse_failed,
+                                    "%s: failed to get a relocated section: %s",
+                                    SectionName.data(),
+                                    toString(RelSecOrErr.takeError()).c_str()),
+                  Obj->getFileName());
+
+    const Elf_Shdr *ContentsSec =
+        Obj->getSection((*RelSecOrErr)->getRawDataRefImpl());
     Expected<StringRef> ContentsSectionNameOrErr =
         EF->getSectionName(ContentsSec);
     if (!ContentsSectionNameOrErr) {
       consumeError(ContentsSectionNameOrErr.takeError());
       continue;
     }
     if (*ContentsSectionNameOrErr != ".stack_sizes")
       continue;