[llvm-readobj] - Move out the common code from printRelocations() methods.

[grimar]

Summary

This introduces the printRelocationsHelper() which now contains the common
code used by both GNU and LLVM output styles.

Depends on https://reviews.llvm.org/D83871

[grimar]

• Minor simplification.

[MaskRay]

llvm/tools/llvm-readobj/ELFDumper.cpp

5592

I am a bit unsure this simplifies the code (number of lines increased...).

We changed two dispatching switches to 3 lambdas plus 2 dispatching switches? Did not see how the new organization simplifies code or improves readability...

[grimar]

llvm/tools/llvm-readobj/ELFDumper.cpp

5592

This method has 6 unwrapOrError calls and now shares the logic which previously was duplicated.
With this patch we will be able to replace these unwrapOrError calls with a proper error reporting code in a single shared place.

[grimar]

• Slightly changed the approavh used (3 lambdas -> 1 lambda).

[jhenderson]

llvm/tools/llvm-readobj/ELFDumper.cpp

6238–6243

This lambda (and the identical one in printSectionHeaders) seems a little unfortunate to me, if I'm honest. The printRelocationsHelper function switches on the section header type, whilst this switches on the relocation type, which can be inferred from the section header type. Would it not be simpler to call printRelocation inline? I'm not sure I see how the lambda improves things.

[grimar]

• Addressed review comments.

llvm/tools/llvm-readobj/ELFDumper.cpp

6238–6243

The printRelocationsHelper function switches on the section header type, whilst this switches on the relocation type, which can be inferred from the section header type.

Inferring the relocation type is not that straightforward. It can be Rel/Relr depending on the opts::RawRelr switch, for example:

case ELF::SHT_RELR:
case ELF::SHT_ANDROID_RELR: {
  Elf_Relr_Range Relrs = unwrapOrError(this->FileName, Obj->relrs(&Sec));
  if (opts::RawRelr) {
    for (const Elf_Relr &R : Relrs)
      printRelrReloc(R);
    break;
  }
  std::vector<Elf_Rel> RelrRels =
      unwrapOrError(this->FileName, Obj->decode_relrs(Relrs));
  for (const Elf_Rel &R : RelrRels)
    printRelReloc(Obj, SecNdx, SymTab, R, ++RelNdx);
  break;
}

And we want to avoid the duplication of the inferring logic probably, it is not that easy/convenient to share it I think.

Would it not be simpler to call printRelocation inline?

It is possible, but we have different implementations of printRelocation for GNU/LLVM style.
And it is a template function that only handles Rel/Rela currently, but not Relr:

template <class RelTy>
void printRelocation(const ELFO *Obj, unsigned SecIndex, const RelTy &Rel,
                     unsigned RelIndex, const Elf_Shdr *SymTab);

So we can't jsut call/inline the existent logic. To be able to remove lambda, I see 2 ways:

1. Introduce 3 more virtual functions that will be overriden by GNU/LLVM dumpers:

printRelReloc, printRelaReloc and printRelrReloc.

2. Introduce a Relocation struct/union (like was done in the diff 3) and a single new virtual function.

I've updated the code to remove lambda and implemented (1). What do you think?

[jhenderson]

llvm/tools/llvm-readobj/ELFDumper.cpp

3622–3633

It seems to me like this pair of functions (and the equivalent LLVM pair) is unnecessary. You should be able to call printRelRelaReloc directly.

[grimar]

llvm/tools/llvm-readobj/ELFDumper.cpp

3622–3633

printRelRelaReloc is a non-virtual template method, defined for both LLVMStyle<ELFT> and GNUStyle<ELFT> output styles.

template <class ELFT>
template <class RelTy>
void LLVMStyle<ELFT>::printRelRelaReloc(const ELFO *Obj, unsigned SecIndex,
                                        const RelTy &Rel, unsigned RelIndex,
                                        const Elf_Shdr *SymTab)

template <class ELFT>
template <class RelTy>
void GNUStyle<ELFT>::printRelRelaReloc(const ELFO *Obj, unsigned SecIndex,
                                       const Elf_Shdr *SymTab, const RelTy &R,
                                       unsigned RelIndex)

The only way I can imagine how to call it directly from the base class is to detemplate it, make it virtual and return to having Elf_Rela R, bool IsRela or something alike in the signature, what didn't work good (D83871).
Or, may be we can pass both Elf_Rela and Elf_Rel at the same time. E.g:

void GNUStyle<ELFT>::printRelRelaReloc(const ELFO *Obj, unsigned SecIndex,
                                       const Elf_Shdr *SymTab, unsigned RelIndex, const Elf_Rela *Rela, const Elf_Rela *Rel) {
assert((Rela || Rel) && !(Rela && Rel));
...
}

or introduce a helper struct to store either both relocations ot Elf_Rel + optional addend. In all that cases this needs printRelRelaReloc to be detemplated and virtualized. Want me to try this way?

[jhenderson]

LGTM, I think, but it would be wise to get @MaskRay's thoughts too.

llvm/tools/llvm-readobj/ELFDumper.cpp

3622–3633

No, it's okay. I hadn't quite registered templates + virtual methods not combining well in this way.

[grimar]

llvm/tools/llvm-readobj/ELFDumper.cpp

3622–3633

I've experimented with de-templating and it looks not so bad as I thought it seems.
I've posted the D84323 as a possible follow-up that we can land on top if we decide to detemplate it.

[grimar]

@MaskRay, are you OK to land it?

[grimar]

There was no new objections/comments for about 7 days after I got LGTM, so I've committed it as 08a265435bc359841c8618081db6a698cef1f668 (phab does not autocloses any reviews currently it seems).

llvm/tools/llvm-readobj/ELFDumper.cpp

for (const auto &Sym : Syms)
     ELFDumperStyle->printSymbol(Obj, &Sym, Syms.begin(), StrTable, IsDynamic,
                                 NonVisibilityBitsUsed);
 }
 
 template <class ELFT> class MipsGOTParser;
 
 template <typename ELFT> class DumpStyle {
 public:
-  using Elf_Shdr = typename ELFT::Shdr;
-  using Elf_Sym = typename ELFT::Sym;
-  using Elf_Addr = typename ELFT::Addr;
+  TYPEDEF_ELF_TYPES(ELFT)
 
   DumpStyle(ELFDumper<ELFT> *Dumper) : Dumper(Dumper) {
     FileName = this->Dumper->getElfObject()->getFileName();
   }
 
   virtual ~DumpStyle() = default;
 
   virtual void printFileHeaders(const ELFFile<ELFT> *Obj) = 0;
public:
   const ELFDumper<ELFT> *dumper() const { return Dumper; }
 
 protected:
   void printDependentLibsHelper(
       const ELFFile<ELFT> *Obj,
       function_ref<void(const Elf_Shdr &)> OnSectionStart,
       function_ref<void(StringRef, uint64_t)> OnSectionEntry);
 
+  using PrintRelFn = function_ref<void(const ELFO *, unsigned, unsigned,
+                                       const Elf_Shdr *, const Elf_Rel &)>;
+  using PrintRelaFn = function_ref<void(const ELFO *, unsigned, unsigned,
+                                        const Elf_Shdr *, const Elf_Rela &)>;
+  using PrintRelrFn = function_ref<void(const Elf_Relr &)>;
+  void printRelocationsHelper(const ELFFile<ELFT> *Obj, const Elf_Shdr &Sec,
+                              PrintRelFn PrintRel, PrintRelaFn PrintRela,
+                              PrintRelrFn PrintRelr);
+
   void reportUniqueWarning(Error Err) const;
   StringRef FileName;
 
 private:
   const ELFDumper<ELFT> *Dumper;
 };
 
 template <typename ELFT> class GNUStyle : public DumpStyle<ELFT> {
public:
   TYPEDEF_ELF_TYPES(ELFT)
 
   LLVMStyle(ScopedPrinter &W, ELFDumper<ELFT> *Dumper)
       : DumpStyle<ELFT>(Dumper), W(W) {}
 
   void printFileHeaders(const ELFO *Obj) override;
   void printGroupSections(const ELFFile<ELFT> *Obj) override;
   void printRelocations(const ELFO *Obj) override;
-  void printRelocations(const Elf_Shdr *Sec, const ELFO *Obj);
   void printSectionHeaders(const ELFO *Obj) override;
   void printSymbols(const ELFO *Obj, bool PrintSymbols,
                     bool PrintDynamicSymbols) override;
   void printDependentLibs(const ELFFile<ELFT> *Obj) override;
   void printDynamic(const ELFFile<ELFT> *Obj) override;
   void printDynamicRelocations(const ELFO *Obj) override;
   void printProgramHeaders(const ELFO *Obj, bool PrintProgramHeaders,
                            cl::boolOrDefault PrintSectionMapping) override;
for (const GroupSection &G : V) {
     }
   }
 
   if (V.empty())
     OS << "There are no section groups in this file.\n";
 }
 
 template <class ELFT>
 template <class RelTy>
 void GNUStyle<ELFT>::printRelocation(const ELFO *Obj, unsigned SecIndex,
                                      const Elf_Shdr *SymTab, const RelTy &R,
                                      unsigned RelIndex) {
   Expected<std::pair<const typename ELFT::Sym *, std::string>> Target =
       this->dumper()->getRelocationTarget(SymTab, R);
   if (!Target)
     this->reportUniqueWarning(createError(
         "unable to print relocation " + Twine(RelIndex) + " in section " +
         Twine(SecIndex) + ": " + toString(Target.takeError())));
   else
     printRelocation(Obj, /*Sym=*/Target->first, /*Name=*/Target->second, R);
 }
 
 template <class ELFT>
 static Optional<int64_t> getAddend(const typename ELFT::Rela &R) {
   return (int64_t)R.r_addend;
 }
 
 template <class ELFT>
OS << "             Info             Type"
        << "               Symbol's Value  Symbol's Name";
   else
     OS << "     Info    Type                Sym. Value  Symbol's Name";
   if (IsRela)
     OS << " + Addend";
   OS << "\n";
 }
 
+template <class ELFT>
+static bool isRelocationSec(const typename ELFT::Shdr &Sec) {
+  return Sec.sh_type == ELF::SHT_REL || Sec.sh_type == ELF::SHT_RELA ||
+         Sec.sh_type == ELF::SHT_RELR || Sec.sh_type == ELF::SHT_ANDROID_REL ||
+         Sec.sh_type == ELF::SHT_ANDROID_RELA ||
+         Sec.sh_type == ELF::SHT_ANDROID_RELR;
+}
+
 template <class ELFT> void GNUStyle<ELFT>::printRelocations(const ELFO *Obj) {
   bool HasRelocSections = false;
   for (const Elf_Shdr &Sec : cantFail(Obj->sections())) {
-    if (Sec.sh_type != ELF::SHT_REL && Sec.sh_type != ELF::SHT_RELA &&
-        Sec.sh_type != ELF::SHT_RELR && Sec.sh_type != ELF::SHT_ANDROID_REL &&
-        Sec.sh_type != ELF::SHT_ANDROID_RELA &&
-        Sec.sh_type != ELF::SHT_ANDROID_RELR)
+    if (!isRelocationSec<ELFT>(Sec))
       continue;
     HasRelocSections = true;
+
     StringRef Name = unwrapOrError(this->FileName, Obj->getSectionName(&Sec));
-    unsigned Entries = Sec.getEntityCount();
-    std::vector<Elf_Rela> AndroidRelas;
-    if (Sec.sh_type == ELF::SHT_ANDROID_REL ||
-        Sec.sh_type == ELF::SHT_ANDROID_RELA) {
-      // Android's packed relocation section needs to be unpacked first
-      // to get the actual number of entries.
-      AndroidRelas = unwrapOrError(this->FileName, Obj->android_relas(&Sec));
-      Entries = AndroidRelas.size();
-    }
-    std::vector<Elf_Rel> RelrRels;
-    if (!opts::RawRelr && (Sec.sh_type == ELF::SHT_RELR ||
+    unsigned Entries;
+    if (Sec.sh_type == ELF::SHT_ANDROID_REL ||
+        Sec.sh_type == ELF::SHT_ANDROID_RELA) {
+      Entries = unwrapOrError(this->FileName, Obj->android_relas(&Sec)).size();
+    } else if (!opts::RawRelr && (Sec.sh_type == ELF::SHT_RELR ||
                                   Sec.sh_type == ELF::SHT_ANDROID_RELR)) {
-      // .relr.dyn relative relocation section needs to be unpacked first
-      // to get the actual number of entries.
       Elf_Relr_Range Relrs = unwrapOrError(this->FileName, Obj->relrs(&Sec));
-      RelrRels = unwrapOrError(this->FileName, Obj->decode_relrs(Relrs));
-      Entries = RelrRels.size();
+      Entries = unwrapOrError(this->FileName, Obj->decode_relrs(Relrs)).size();
+    } else {
+      Entries = Sec.getEntityCount();
     }
+
     uintX_t Offset = Sec.sh_offset;
     OS << "\nRelocation section '" << Name << "' at offset 0x"
        << to_hexString(Offset, false) << " contains " << Entries
        << " entries:\n";
     printRelocHeader(Sec.sh_type);
-    const Elf_Shdr *SymTab =
-        unwrapOrError(this->FileName, Obj->getSection(Sec.sh_link));
-    unsigned SecNdx = &Sec - &cantFail(Obj->sections()).front();
-    unsigned RelNdx = 0;
-
-    switch (Sec.sh_type) {
-    case ELF::SHT_REL:
-      for (const Elf_Rel &R : unwrapOrError(this->FileName, Obj->rels(&Sec)))
-        printRelocation(Obj, SecNdx, SymTab, R, ++RelNdx);
-      break;
-    case ELF::SHT_RELA:
-      for (const Elf_Rela &R : unwrapOrError(this->FileName, Obj->relas(&Sec)))
-        printRelocation(Obj, SecNdx, SymTab, R, ++RelNdx);
-      break;
-    case ELF::SHT_RELR:
-    case ELF::SHT_ANDROID_RELR:
-      if (opts::RawRelr)
-        for (const Elf_Relr &R :
-             unwrapOrError(this->FileName, Obj->relrs(&Sec)))
+    this->printRelocationsHelper(
+        Obj, Sec,
+        [&](const ELFO *Obj, unsigned SecIndex, unsigned RelIndex,
+            const Elf_Shdr *SymTab, const Elf_Rel &R) {
+          printRelocation(Obj, SecIndex, SymTab, R, RelIndex);
+        },
+        [&](const ELFO *Obj, unsigned SecIndex, unsigned RelIndex,
+            const Elf_Shdr *SymTab, const Elf_Rela &R) {
+          printRelocation(Obj, SecIndex, SymTab, R, RelIndex);
+        },
+        [&](const Elf_Relr &R) {
           OS << to_string(format_hex_no_prefix(R, ELFT::Is64Bits ? 16 : 8))
              << "\n";
-      else
-        for (const Elf_Rel &R : RelrRels)
-          printRelocation(Obj, SecNdx, SymTab, R, ++RelNdx);
-      break;
-    case ELF::SHT_ANDROID_REL:
-      for (const Elf_Rela &R : AndroidRelas)
-        printRelocation(Obj, SecNdx, SymTab, (const Elf_Rel &)R, ++RelNdx);
-      break;
-    case ELF::SHT_ANDROID_RELA:
-      for (const Elf_Rela &R : AndroidRelas)
-        printRelocation(Obj, SecNdx, SymTab, R, ++RelNdx);
-      break;
-    }
+        });
   }
   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
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 ELFFile<ELFT> *Obj,
+                                             const Elf_Shdr &Sec,
+                                             PrintRelFn PrintRel,
+                                             PrintRelaFn PrintRela,
+                                             PrintRelrFn PrintRelr) {
+  const Elf_Shdr *SymTab =
+      unwrapOrError(this->FileName, Obj->getSection(Sec.sh_link));
+  unsigned SecNdx = &Sec - &cantFail(Obj->sections()).front();
+  unsigned RelNdx = 0;
+
+  switch (Sec.sh_type) {
+  case ELF::SHT_REL:
+    for (const Elf_Rel &R : unwrapOrError(this->FileName, Obj->rels(&Sec)))
+      PrintRel(Obj, SecNdx, ++RelNdx, SymTab, R);
+    break;
+  case ELF::SHT_RELA:
+    for (const Elf_Rela &R : unwrapOrError(this->FileName, Obj->relas(&Sec)))
+      PrintRela(Obj, SecNdx, ++RelNdx, SymTab, R);
+    break;
+  case ELF::SHT_RELR:
+  case ELF::SHT_ANDROID_RELR: {
+    Elf_Relr_Range Relrs = unwrapOrError(this->FileName, Obj->relrs(&Sec));
+    if (opts::RawRelr) {
+      for (const Elf_Relr &R : Relrs)
+        PrintRelr(R);
+      break;
+    }
+    std::vector<Elf_Rel> RelrRels =
+        unwrapOrError(this->FileName, Obj->decode_relrs(Relrs));
+    for (const Elf_Rel &R : RelrRels)
+      PrintRel(Obj, SecNdx, ++RelNdx, SymTab, R);
+    break;
+  }
+  case ELF::SHT_ANDROID_REL:
+  case ELF::SHT_ANDROID_RELA:
+    for (const Elf_Rela &R :
+         unwrapOrError(this->FileName, Obj->android_relas(&Sec)))
+      PrintRela(Obj, SecNdx, ++RelNdx, SymTab, R);
+    break;
+  }
+}
+
+template <class ELFT>
 void GNUStyle<ELFT>::printDependentLibs(const ELFFile<ELFT> *Obj) {
   bool SectionStarted = false;
   struct NameOffset {
     StringRef Name;
     uint64_t Offset;
   };
   std::vector<NameOffset> SecEntries;
   NameOffset Current;
void LLVMStyle<ELFT>::printGroupSections(const ELFO *Obj) {
 
   if (V.empty())
     W.startLine() << "There are no group sections in the file.\n";
 }
 
 template <class ELFT> void LLVMStyle<ELFT>::printRelocations(const ELFO *Obj) {
   ListScope D(W, "Relocations");
 
-  int SectionNumber = -1;
   for (const Elf_Shdr &Sec : cantFail(Obj->sections())) {
-    ++SectionNumber;
-
-    if (Sec.sh_type != ELF::SHT_REL && Sec.sh_type != ELF::SHT_RELA &&
-        Sec.sh_type != ELF::SHT_RELR && Sec.sh_type != ELF::SHT_ANDROID_REL &&
-        Sec.sh_type != ELF::SHT_ANDROID_RELA &&
-        Sec.sh_type != ELF::SHT_ANDROID_RELR)
+    if (!isRelocationSec<ELFT>(Sec))
       continue;
 
     StringRef Name = unwrapOrError(this->FileName, Obj->getSectionName(&Sec));
-
-    W.startLine() << "Section (" << SectionNumber << ") " << Name << " {\n";
+    unsigned SecNdx = &Sec - &cantFail(Obj->sections()).front();
+    W.startLine() << "Section (" << SecNdx << ") " << Name << " {\n";
     W.indent();
-
-    printRelocations(&Sec, Obj);
-
+    this->printRelocationsHelper(
+        Obj, Sec,
+        [&](const ELFO *Obj, unsigned SecIndex, unsigned RelIndex,
+            const Elf_Shdr *SymTab, const Elf_Rel &R) {
+          printRelocation(Obj, SecIndex, R, RelIndex, SymTab);
+        },
+        [&](const ELFO *Obj, unsigned SecIndex, unsigned RelIndex,
+            const Elf_Shdr *SymTab, const Elf_Rela &R) {
+          printRelocation(Obj, SecIndex, R, RelIndex, SymTab);
+        },
+        [&](const Elf_Relr &R) { W.startLine() << W.hex(R) << "\n"; });
     W.unindent();
     W.startLine() << "}\n";
   }
 }
 
 template <class ELFT>
-void LLVMStyle<ELFT>::printRelocations(const Elf_Shdr *Sec, const ELFO *Obj) {
-  const Elf_Shdr *SymTab =
-      unwrapOrError(this->FileName, Obj->getSection(Sec->sh_link));
-  unsigned SecNdx = Sec - &cantFail(Obj->sections()).front();
-  unsigned RelNdx = 0;
-
-  switch (Sec->sh_type) {
-  case ELF::SHT_REL:
-    for (const Elf_Rel &R : unwrapOrError(this->FileName, Obj->rels(Sec)))
-      printRelocation(Obj, SecNdx, R, ++RelNdx, SymTab);
-    break;
-  case ELF::SHT_RELA:
-    for (const Elf_Rela &R : unwrapOrError(this->FileName, Obj->relas(Sec)))
-      printRelocation(Obj, SecNdx, R, ++RelNdx, SymTab);
-    break;
-  case ELF::SHT_RELR:
-  case ELF::SHT_ANDROID_RELR: {
-    Elf_Relr_Range Relrs = unwrapOrError(this->FileName, Obj->relrs(Sec));
-    if (opts::RawRelr) {
-      for (const Elf_Relr &R : Relrs)
-        W.startLine() << W.hex(R) << "\n";
-    } else {
-      std::vector<Elf_Rel> RelrRels =
-          unwrapOrError(this->FileName, Obj->decode_relrs(Relrs));
-      for (const Elf_Rel &R : RelrRels)
-        printRelocation(Obj, SecNdx, R, ++RelNdx, SymTab);
-    }
-    break;
-  }
-  case ELF::SHT_ANDROID_REL:
-  case ELF::SHT_ANDROID_RELA:
-    for (const Elf_Rela &R :
-         unwrapOrError(this->FileName, Obj->android_relas(Sec)))
-      printRelocation(Obj, SecNdx, R, ++RelNdx, SymTab);
-    break;
-  }
-}
-
-template <class ELFT>
 template <class RelTy>
 void LLVMStyle<ELFT>::printRelocation(const ELFO *Obj, unsigned SecIndex,
                                       const RelTy &Rel, unsigned RelIndex,
                                       const Elf_Shdr *SymTab) {
   Expected<std::pair<const typename ELFT::Sym *, std::string>> Target =
       this->dumper()->getRelocationTarget(SymTab, Rel);
   if (!Target) {
     this->reportUniqueWarning(createError(
for (const Elf_Shdr &Sec : cantFail(Obj->sections())) {
     W.printNumber("Size", Sec.sh_size);
     W.printNumber("Link", Sec.sh_link);
     W.printNumber("Info", Sec.sh_info);
     W.printNumber("AddressAlignment", Sec.sh_addralign);
     W.printNumber("EntrySize", Sec.sh_entsize);
 
     if (opts::SectionRelocations) {
       ListScope D(W, "Relocations");
-      printRelocations(&Sec, Obj);
+      this->printRelocationsHelper(
+          Obj, Sec,
+          [&](const ELFO *Obj, unsigned SecIndex, unsigned RelIndex,
+              const Elf_Shdr *SymTab, const Elf_Rel &R) {
+            printRelocation(Obj, SecIndex, R, RelIndex, SymTab);
+          },
+          [&](const ELFO *Obj, unsigned SecIndex, unsigned RelIndex,
+              const Elf_Shdr *SymTab, const Elf_Rela &R) {
+            printRelocation(Obj, SecIndex, R, RelIndex, SymTab);
+          },
+          [&](const Elf_Relr &R) { W.startLine() << W.hex(R) << "\n"; });
     }
 
     if (opts::SectionSymbols) {
       ListScope D(W, "Symbols");
       const Elf_Shdr *Symtab = this->dumper()->getDotSymtabSec();
       StringRef StrTable =
           unwrapOrError(this->FileName, Obj->getStringTableForSymtab(*Symtab));