[PATCH] D93900: [llvm-readef/obj] - Change the design structure of ELF dumper. NFCI.

[James Henderson via Phabricator via llvm-commits]

jhenderson added a subscriber: khemant.
jhenderson added a comment.

The original split of the two styles came in D14128 <https://reviews.llvm.org/D14128>, by @khemant. I can't see any explanation in that review for taking the route of using a separate style class as opposed to simply doing what you're doing now. Possibly it was just easier to retrofit it in that way. The style/dumper split would make sense, if the styles were shared between the different dumper kinds, but they aren't, and I suspect the dumping functionality for other tools may be too divergent for them to be easily reusable.

Overall, this looks reasonable. Could you highlight the major changes you've had to make to achieve the refactor? This patch is quite large and tricky for me to review in depth.



================
Comment at: llvm/tools/llvm-readobj/ELFDumper.cpp:730
 
-template <class ELFT> class MipsGOTParser;
-
-template <typename ELFT> class DumpStyle {
-public:
-  LLVM_ELF_IMPORT_TYPES_ELFT(ELFT)
-
-  DumpStyle(const ELFDumper<ELFT> &Dumper)
-      : Obj(Dumper.getElfObject().getELFFile()), ElfObj(Dumper.getElfObject()),
-        Dumper(Dumper) {
-    FileName = ElfObj.getFileName();
-  }
-
-  virtual ~DumpStyle() = default;
-
-  virtual void printFileHeaders() = 0;
-  virtual void printGroupSections() = 0;
-  virtual void printRelocations() = 0;
-  virtual void printSectionHeaders() = 0;
-  virtual void printSymbols(bool PrintSymbols, bool PrintDynamicSymbols) = 0;
-  virtual void printHashSymbols() {}
-  virtual void printSectionDetails() {}
-  virtual void printDependentLibs() = 0;
-  virtual void printDynamic() {}
-  virtual void printDynamicRelocations() = 0;
-  virtual void printSymtabMessage(const Elf_Shdr *Symtab, size_t Offset,
-                                  bool NonVisibilityBitsUsed) {}
-  virtual void printSymbol(const Elf_Sym &Symbol, unsigned SymIndex,
-                           Optional<StringRef> StrTable, bool IsDynamic,
-                           bool NonVisibilityBitsUsed) = 0;
-  virtual void printProgramHeaders(bool PrintProgramHeaders,
-                                   cl::boolOrDefault PrintSectionMapping) = 0;
-  virtual void printVersionSymbolSection(const Elf_Shdr *Sec) = 0;
-  virtual void printVersionDefinitionSection(const Elf_Shdr *Sec) = 0;
-  virtual void printVersionDependencySection(const Elf_Shdr *Sec) = 0;
-  virtual void printHashHistograms() = 0;
-  virtual void printCGProfile() = 0;
-  virtual void printAddrsig() = 0;
-  virtual void printNotes() = 0;
-  virtual void printELFLinkerOptions() = 0;
-  virtual void printStackSizes() = 0;
-  void printNonRelocatableStackSizes(std::function<void()> PrintHeader);
-  void printRelocatableStackSizes(std::function<void()> PrintHeader);
-  bool printFunctionStackSize(uint64_t SymValue,
-                              Optional<const Elf_Shdr *> FunctionSec,
-                              const Elf_Shdr &StackSizeSec, DataExtractor Data,
-                              uint64_t *Offset);
-  void printStackSize(const Relocation<ELFT> &R, const Elf_Shdr &RelocSec,
-                      unsigned Ndx, const Elf_Shdr *SymTab,
-                      const Elf_Shdr *FunctionSec, const Elf_Shdr &StackSizeSec,
-                      const RelocationResolver &Resolver, DataExtractor Data);
-  virtual void printStackSizeEntry(uint64_t Size, StringRef FuncName) = 0;
-  virtual void printMipsGOT(const MipsGOTParser<ELFT> &Parser) = 0;
-  virtual void printMipsPLT(const MipsGOTParser<ELFT> &Parser) = 0;
-  virtual void printMipsABIFlags() = 0;
-  const ELFDumper<ELFT> &dumper() const { return Dumper; }
-  void reportUniqueWarning(Error Err) const;
-  void reportUniqueWarning(const Twine &Msg) const;
-
-protected:
-  std::vector<GroupSection> getGroups();
-
-  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;
-
-  StringRef FileName;
-  const ELFFile<ELFT> &Obj;
-  const ELFObjectFile<ELFT> &ElfObj;
-
-private:
-  const ELFDumper<ELFT> &Dumper;
-};
-
-template <typename ELFT> class GNUStyle : public DumpStyle<ELFT> {
+template <typename ELFT> class GNUStyleELFDumper : public ELFDumper<ELFT> {
   formatted_raw_ostream &OS;
----------------
Maybe the name could be simplified slightly to `GNUELFDumper` (and the LLVM one to `LLVMELFDumper`)?


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D93900/new/

https://reviews.llvm.org/D93900