[llvm-readobj] - Introduce `forEachRelocationDo` helper. NFCI.

[grimar]

Summary

Our printStackSize implementation currently uses
API like RelocationRef, object::symbol_iterator.
It is not ideal as it doesn't allow
to handle possible error conditions properly.

Some time ago I started rewriting it and this NFC patch is
a one more step toward to it. Here I am introducing the
forEachRelocationDo helper. With it it is possible to iterate
over all kinds of relocations, what is helpful for improving
the code it printStackSize and around.

I've made some progress in improving this area and going to post
other related follow-up patches really soon.

[MaskRay]

I don't get why RelRelaReloc is needed. Can you describe a bit about it?

llvm/tools/llvm-readobj/ELFDumper.cpp

803

It is slightly unfortunate that this cannot use function_ref because the variable used for initialization is local.

[grimar]

In D91530#2398695, @MaskRay wrote:

I don't get why RelRelaReloc is needed. Can you describe a bit about it?

Previously the Relocation was the struct which allowed to handle Rel/Rela relocations.
I've reworked it so that it how also is able to keep Relr relocations.
That way it allows to represent 3 types of relocations instead of previous 2 and have a common code for iterations over them.

RelRelaReloc is now a helper class that is used to represent Rel/Rela relocations.

[MaskRay]

In D91530#2398857, @grimar wrote:

In D91530#2398695, @MaskRay wrote:

I don't get why RelRelaReloc is needed. Can you describe a bit about it?

Previously the Relocation was the struct which allowed to handle Rel/Rela relocations.
I've reworked it so that it how also is able to keep Relr relocations.
That way it allows to represent 3 types of relocations instead of previous 2 and have a common code for iterations over them.

RelRelaReloc is now a helper class that is used to represent Rel/Rela relocations.

OK. Does this change make Relr easier to represent? Note that while REL/RELA type relocations are self-contained (the sh_offset is immediately known), RELR is not - it needs a context ((1) is it an address or a bitmap (2) for a bitmap, what is the starting address). Should it just be treated as a special case?

[grimar]

In D91530#2398871, @MaskRay wrote:

In D91530#2398857, @grimar wrote:

In D91530#2398695, @MaskRay wrote:

I don't get why RelRelaReloc is needed. Can you describe a bit about it?

Previously the Relocation was the struct which allowed to handle Rel/Rela relocations.
I've reworked it so that it how also is able to keep Relr relocations.
That way it allows to represent 3 types of relocations instead of previous 2 and have a common code for iterations over them.

RelRelaReloc is now a helper class that is used to represent Rel/Rela relocations.

OK. Does this change make Relr easier to represent? Note that while REL/RELA type relocations are self-contained (the sh_offset is immediately known), RELR is not - it needs a context ((1) is it an address or a bitmap (2) for a bitmap, what is the starting address). Should it just be treated as a special case?

Well. May be. I'll take a look on what can I do for it.

[grimar]

• Use special handling for Elf_Relr as suggested.

Seems looks indeed simpler.

[jhenderson]

llvm/tools/llvm-readobj/ELFDumper.cpp

5679

I don't really mind either way, but what's the motivation for passing in the RawRelr variable, rather than continuing to use opts::RawRelr?

[grimar]

llvm/tools/llvm-readobj/ELFDumper.cpp

5679

In my understanding, the opts::RawRelr option is related to relocation printing:
"Do not decode relocations in SHT_RELR section, display raw contents"

forEachRelocationDo is a more general API which can be used for other things as well.
Perhaps it is not ideal to use such flags inside.

[jhenderson]

Thanks for the answer, I think it makes sense. LGTM, but please wait for @MaskRay.

[MaskRay]

llvm/tools/llvm-readobj/ELFDumper.cpp

3791

this->printRelocationsHelper(Sec); has been expanded into

this->forEachRelocationDo(
        Sec, opts::RawRelr,
        [&](const Relocation<ELFT> &R, unsigned Ndx, const Elf_Shdr &Sec,
            const Elf_Shdr *SymTab) { printReloc(R, Ndx, Sec, SymTab); },
        [&](const Elf_Relr &R) { printRelrReloc(R); });

multiple times. Isn't the previous version clearer?

Can you elaborate on the claim in the description?

Our printStackSize implementation currently uses API like RelocationRef, object::symbol_iterator. It is not ideal as it doesn't allow to handle possible error conditions properly.

[grimar]

llvm/tools/llvm-readobj/ELFDumper.cpp

3791

Isn't the previous version clearer?

Do you compare with the original code or with the previous version of the patch?
Assuming that you compare with the original code: I am using this change in D91624. It allows to avoid
alot of duplication of error handling when iterating over relocations.

Can you elaborate on the claim in the description?

Yeah. I think that API is just totally broken when used for invalid objects. See the description of D91624
about how ELFObjectFile<ELFT>::symbol_end() "works". In the test case for D91624 I've improved one of the messages and
now it provides more details about an error.

One more example:

object::symbol_iterator RelocSym = Reloc.getSymbol();
if (RelocSym != ElfObj.symbol_end()) {

The underlying implementation of Reloc.getSymbol() is:

symbol_iterator
ELFObjectFile<ELFT>::getRelocationSymbol(DataRefImpl Rel) const {
  uint32_t symbolIdx;
  const Elf_Shdr *sec = getRelSection(Rel);
  if (sec->sh_type == ELF::SHT_REL)
    symbolIdx = getRel(Rel)->getSymbol(EF.isMips64EL());
  else
    symbolIdx = getRela(Rel)->getSymbol(EF.isMips64EL());
  if (!symbolIdx)
    return symbol_end();

  // FIXME: error check symbolIdx
  DataRefImpl SymbolData;
  SymbolData.d.a = sec->sh_link;
  SymbolData.d.b = symbolIdx;
  return symbol_iterator(SymbolRef(SymbolData, this));
}

It just doesn't check sec->sh_link or symbolIdx

Another example is that in the ELFObjectFile.h file, the implementation of ELFObjectFile<ELFT> methods
has 5 report_fatal_error calls. They not just doesn't propogate them to user, but also hide real errors behind the
error code messages I think. E.g.:

// Error check sh_link here so that getRelocationSymbol can just use it.
auto SymSecOrErr = EF.getSection(RelSec->sh_link);
if (!SymSecOrErr)
  report_fatal_error(errorToErrorCode(SymSecOrErr.takeError()).message());

[grimar]

• Restored printRelocationsHelper to avoid forEachRelocationDo duplication.

llvm/tools/llvm-readobj/ELFDumper.cpp

3791

this->printRelocationsHelper(Sec); has been expanded into

this->forEachRelocationDo(
        Sec, opts::RawRelr,
        [&](const Relocation<ELFT> &R, unsigned Ndx, const Elf_Shdr &Sec,
            const Elf_Shdr *SymTab) { printReloc(R, Ndx, Sec, SymTab); },
        [&](const Elf_Relr &R) { printRelrReloc(R); });

If the concern is about duplicating of this call, then it is eaily solvable by introducing
a new method to isolate it. I've added the printRelocationsHelper method back,
it is now incapsulates this forEachRelocationDo call.

llvm/tools/llvm-readobj/ELFDumper.cpp

protected:
   void printDependentLibsHelper(
       function_ref<void(const Elf_Shdr &)> OnSectionStart,
       function_ref<void(StringRef, uint64_t)> OnSectionEntry);
 
   virtual void printReloc(const Relocation<ELFT> &R, unsigned RelIndex,
                           const Elf_Shdr &Sec, const Elf_Shdr *SymTab) = 0;
   virtual void printRelrReloc(const Elf_Relr &R) = 0;
   virtual void printDynamicReloc(const Relocation<ELFT> &R) = 0;
+  void forEachRelocationDo(
+      const Elf_Shdr &Sec, bool RawRelr,
+      llvm::function_ref<void(const Relocation<ELFT> &, unsigned,
+                              const Elf_Shdr &, const Elf_Shdr *)>
+          RelRelaFn,
+      llvm::function_ref<void(const Elf_Relr &)> RelrFn);
   void printRelocationsHelper(const Elf_Shdr &Sec);
   void printDynamicRelocationsHelper();
   virtual void printDynamicRelocHeader(unsigned Type, StringRef Name,
                                        const DynRegionInfo &Reg){};
 
   StringRef getPrintableSectionName(const Elf_Shdr &Sec) const;
 
   void reportUniqueWarning(Error Err) const;
 
   StringRef FileName;
   const ELFFile<ELFT> &Obj;
   const ELFObjectFile<ELFT> &ElfObj;
 
 private:
   const ELFDumper<ELFT> &Dumper;
 };
 
 template <typename ELFT> class GNUStyle : public DumpStyle<ELFT> {
   formatted_raw_ostream &OS;
 
 public:
   TYPEDEF_ELF_TYPES(ELFT)
else
           describe(this->Obj, Sec) + ": " + toString(NumOrErr.takeError())));
 
     uintX_t Offset = Sec.sh_offset;
     StringRef Name = this->getPrintableSectionName(Sec);
     OS << "\nRelocation section '" << Name << "' at offset 0x"
        << to_hexString(Offset, false) << " contains " << EntriesNum
        << " entries:\n";
     printRelocHeaderFields<ELFT>(OS, Sec.sh_type);
     this->printRelocationsHelper(Sec);
   }
   if (!HasRelocSections)
     OS << "\nThere are no relocations in this file.\n";
 }
 
 // Print the offset of a particular section from anyone of the ranges:
 // [SHT_LOOS, SHT_HIOS], [SHT_LOPROC, SHT_HIPROC], [SHT_LOUSER, SHT_HIUSER].
 // If 'Type' does not fall within any of those ranges, then a string is
OS << "  " << format_hex(Tag, ELFT::Is64Bits ? 18 : 10)
        << format(ValueFmt.c_str(), Type.c_str()) << Value << "\n";
   }
 }
 
 template <class ELFT> void GNUStyle<ELFT>::printDynamicRelocations() {
   this->printDynamicRelocationsHelper();
 }
 
+template <class ELFT>
+void DumpStyle<ELFT>::printRelocationsHelper(const Elf_Shdr &Sec) {
+  this->forEachRelocationDo(
+      Sec, opts::RawRelr,
+      [&](const Relocation<ELFT> &R, unsigned Ndx, const Elf_Shdr &Sec,
+          const Elf_Shdr *SymTab) { printReloc(R, Ndx, Sec, SymTab); },
+      [&](const Elf_Relr &R) { printRelrReloc(R); });
+}
+
 template <class ELFT> void DumpStyle<ELFT>::printDynamicRelocationsHelper() {
   const bool IsMips64EL = this->Obj.isMips64EL();
   const DynRegionInfo &DynRelaRegion = this->dumper().getDynRelaRegion();
   if (DynRelaRegion.Size > 0) {
     printDynamicRelocHeader(ELF::SHT_RELA, "RELA", DynRelaRegion);
     for (const Elf_Rela &Rela : this->dumper().dyn_relas())
       printDynamicReloc(Relocation<ELFT>(Rela, IsMips64EL));
   }
for (const uint8_t *I = Contents.begin(), *E = Contents.end(); I < E;) {
       StringRef Lib((const char *)I);
       OnLibEntry(Lib, I - Contents.begin());
       I += Lib.size() + 1;
     }
   }
 }
 
 template <class ELFT>
-void DumpStyle<ELFT>::printRelocationsHelper(const Elf_Shdr &Sec) {
+void DumpStyle<ELFT>::forEachRelocationDo(
+    const Elf_Shdr &Sec, bool RawRelr,
+    llvm::function_ref<void(const Relocation<ELFT> &, unsigned,
+                            const Elf_Shdr &, const Elf_Shdr *)>
+        RelRelaFn,
+    llvm::function_ref<void(const Elf_Relr &)> RelrFn) {
   auto Warn = [&](Error &&E,
                   const Twine &Prefix = "unable to read relocations from") {
     this->reportUniqueWarning(createError(Prefix + " " + describe(Obj, Sec) +
                                           ": " + toString(std::move(E))));
   };
 
   // SHT_RELR/SHT_ANDROID_RELR sections do not have an associated symbol table.
   // For them we should not treat the value of the sh_link field as an index of
void DumpStyle<ELFT>::forEachRelocationDo(
   }
 
   unsigned RelNdx = 0;
   const bool IsMips64EL = this->Obj.isMips64EL();
   switch (Sec.sh_type) {
   case ELF::SHT_REL:
     if (Expected<Elf_Rel_Range> RangeOrErr = Obj.rels(Sec)) {
       for (const Elf_Rel &R : *RangeOrErr)
-        printReloc(Relocation<ELFT>(R, IsMips64EL), ++RelNdx, Sec, SymTab);
+        RelRelaFn(Relocation<ELFT>(R, IsMips64EL), ++RelNdx, Sec, SymTab);
     } else {
       Warn(RangeOrErr.takeError());
     }
     break;
   case ELF::SHT_RELA:
     if (Expected<Elf_Rela_Range> RangeOrErr = Obj.relas(Sec)) {
       for (const Elf_Rela &R : *RangeOrErr)
-        printReloc(Relocation<ELFT>(R, IsMips64EL), ++RelNdx, Sec, SymTab);
+        RelRelaFn(Relocation<ELFT>(R, IsMips64EL), ++RelNdx, Sec, SymTab);
     } else {
       Warn(RangeOrErr.takeError());
     }
     break;
   case ELF::SHT_RELR:
   case ELF::SHT_ANDROID_RELR: {
     Expected<Elf_Relr_Range> RangeOrErr = Obj.relrs(Sec);
     if (!RangeOrErr) {
       Warn(RangeOrErr.takeError());
       break;
     }
-    if (opts::RawRelr) {
+    if (RawRelr) {
       for (const Elf_Relr &R : *RangeOrErr)
-        printRelrReloc(R);
+        RelrFn(R);
       break;
     }
 
     for (const Elf_Rel &R : Obj.decode_relrs(*RangeOrErr))
-      printReloc(Relocation<ELFT>(R, IsMips64EL), ++RelNdx, Sec,
-                 /*SymTab=*/nullptr);
+      RelRelaFn(Relocation<ELFT>(R, IsMips64EL), ++RelNdx, Sec,
     break;
   }
   case ELF::SHT_ANDROID_REL:
   case ELF::SHT_ANDROID_RELA:
     if (Expected<std::vector<Elf_Rela>> RelasOrErr = Obj.android_relas(Sec)) {
       for (const Elf_Rela &R : *RelasOrErr)
-        printReloc(Relocation<ELFT>(R, IsMips64EL), ++RelNdx, Sec, SymTab);
+        RelRelaFn(Relocation<ELFT>(R, IsMips64EL), ++RelNdx, Sec, SymTab);
     } else {
       Warn(RelasOrErr.takeError());
     }
     break;
   }
 }
 
 template <class ELFT>