[llvm-readobj] Provide Hash Histogram for all ELFDumper implementations

[paulkirth]

Summary

Previously, the GNUELFDumper was the only implementer for Hash Histograms.
This patch moves the implementation for printHashHistogram and
printGnuHashHistogram into the ELFDumper base class, and allows each
derived class to specialize how it outputs that information.

This change also prevents the JSONELFDumper from emitting invalid JSON,
since the shared implementation in LLVMELFDumper no longer emits a
warning message to the output stream.

[jhenderson]

I'd rather not do this, and would strongly prefer we simply do it right the first time.

[paulkirth]

Rebase

• Replace empty implementation with a shared impl in the ELFDumper base class and printing APIs for derived classes to specialize.
• Add test to check the formatting for the LLVM style output

[paulkirth]

Rebase to fix patch application

[paulkirth]

Fix formatting

[paulkirth]

Rebase

[paulkirth]

Rebase

[paulkirth]

Rebase.

[paulkirth]

Rebase.

[paulkirth]

Rebase.

[paulkirth]

Rebase.

[jhenderson]

So this patch appears to add support for the hash histogram printing for both GNU and non-GNU hash sections, but the testing only appears to test one of these two cases. I think you need tests for both cases.

llvm/include/llvm/Support/ScopedPrinter.h

233 ↗

(On Diff #500343)

This should probably be spun off into a separate patch and have testing in the unit tests.

llvm/test/tools/llvm-readobj/ELF/hash-histogram.test

15 ↗

(On Diff #500343)

llvm/tools/llvm-readobj/ELFDumper.cpp

2665

Whilst moving, please add the missing "." to this comment.

2696

Ditto.

2700

Ditto.

2746

Ditto.

6999–7001

Any particular reason you've made the Count member const in these two methods?

7006

7024

https://llvm.org/docs/CodingStandards.html#prefer-preincrement

[paulkirth]

Rebase.

• split out ScopedPrinter changes.
• remove const from ArrayRef parameters
• fix typos in comments
• use preincrement in loops
• add test for both types of HashHistogram

[paulkirth]

llvm/tools/llvm-readobj/ELFDumper.cpp

6999–7001

Thanks for pointing that out. I think it was just habit from writing const Foo& forever, but that doesn't make sense in this case.

7024

This loop is taken directly from the GNUELFDumper, should I update this in the existing implementations too?

[paulkirth]

Rebase

[paulkirth]

Rebase

[paulkirth]

Fix formatting

[jhenderson]

llvm/test/tools/llvm-readobj/ELF/hash-histogram.test

16–17 ↗

(On Diff #502710)

There's no need for these two extra cases: they are there for the earlier llvm-readelf check to ensure the --histogram and -I aliases are equivalent to --elf-hash-histogram.

llvm/tools/llvm-readobj/ELFDumper.cpp

7002

Looking at this again, this and the GNU hash histogram version immediately below are practically identical. Can you try sharing the code rather than duplicating it, please?

Whilst you're at it, you might want to do the same for the GNU output format versions of these two functions.

7024

I wouldn't change code that isn't already changing/moving, but if it is, you can make the tidy-up at the same time.

[paulkirth]

Address comments.

• Use preincrement in moved code.
• Share more code between Gnu and non-Gnu versions
• Remove redundant test cases

[jhenderson]

Two minor points, otherwise LGTM.

llvm/tools/llvm-readobj/ELFDumper.cpp

2701

4830

I have a marginal preference to make this argument StringRef TableDescription (with values of "" and `.gnu.hash' respectively, since there's only one place the behaviour is different between the two output styles, as far as I can tell.

[paulkirth]

llvm/tools/llvm-readobj/ELFDumper.cpp

4830

Sure, but if they have the same interface, then they can be overrides from the base class, as they are now.

[jhenderson]

llvm/tools/llvm-readobj/ELFDumper.cpp

4830

Okay, that's fair.

[paulkirth]

Rebase.

• fix comment string for /*IsGnu=*/

llvm/tools/llvm-readobj/ELFDumper.cpp

public:
 
   void printRelRelaReloc(const Relocation<ELFT> &R,
                          const RelSymbol<ELFT> &RelSym) override;
 
   void printRelocations() override;
 
   void printGroupSections() override;
 
+  void printHashHistograms() override{};
+
 private:
   std::unique_ptr<DictScope> FileScope;
 };
 
 } // end anonymous namespace
 
 namespace llvm {
 
case EM_ARM:
     break;
   case EM_RISCV:
     if (Obj.isLE())
       printAttributes(ELF::SHT_RISCV_ATTRIBUTES,
                       std::make_unique<RISCVAttributeParser>(&W),
                       support::little);
     else
       reportUniqueWarning("attribute printing not implemented for big-endian "
                           "RISC-V objects");
     break;
   case EM_MSP430:
     printAttributes(ELF::SHT_MSP430_ATTRIBUTES,
                     std::make_unique<MSP430AttributeParser>(&W),
                     support::little);
     break;
   case EM_MIPS: {
     printMipsABIFlags();
template <class ELFT> void ELFDumper<ELFT>::printArchSpecificInfo() {
   default:
     break;
   }
 }
 
 template <class ELFT>
 void ELFDumper<ELFT>::printAttributes(
     unsigned AttrShType, std::unique_ptr<ELFAttributeParser> AttrParser,
     support::endianness Endianness) {
   assert((AttrShType != ELF::SHT_NULL) && AttrParser &&
          "Incomplete ELF attribute implementation");
   DictScope BA(W, "BuildAttributes");
   for (const Elf_Shdr &Sec : cantFail(Obj.sections())) {
     if (Sec.sh_type != AttrShType)
       continue;
 
     ArrayRef<uint8_t> Contents;
     if (Expected<ArrayRef<uint8_t>> ContentOrErr =
             Obj.getSectionContents(Sec)) {
       Contents = *ContentOrErr;
       if (Contents.empty()) {
         reportUniqueWarning("the " + describe(Sec) + " is empty");
public:
   MipsGOTParser(const ELFDumper<ELFT> &D);
   Error findGOT(Elf_Dyn_Range DynTable, Elf_Sym_Range DynSyms);
   Error findPLT(Elf_Dyn_Range DynTable);
 
   bool isGotEmpty() const { return GotEntries.empty(); }
   bool isPltEmpty() const { return PltEntries.empty(); }
 
   uint64_t getGp() const;
 
   const Entry *getGotLazyResolver() const;
   const Entry *getGotModulePointer() const;
   const Entry *getPltLazyResolver() const;
   const Entry *getPltModulePointer() const;
 
   Entries getLocalEntries() const;
   Entries getGlobalEntries() const;
   Entries getOtherEntries() const;
void GNUELFDumper<ELFT>::printGnuHashHistogram(
 
   ArrayRef<Elf_Word> Chains = *ChainsOrErr;
   size_t Symndx = GnuHashTable.symndx;
   size_t TotalSyms = 0;
   size_t MaxChain = 1;
   size_t CumulativeNonZero = 0;
 
   size_t NBucket = GnuHashTable.nbuckets;
   if (Chains.empty() || NBucket == 0)
     return;
 
   ArrayRef<Elf_Word> Buckets = GnuHashTable.buckets();
   std::vector<size_t> ChainLen(NBucket, 0);
   for (size_t B = 0; B < NBucket; B++) {
     if (!Buckets[B])
       continue;
     size_t Len = 1;
for (const VernAux &Aux : VN.AuxV) {
       W.printNumber("Index", Aux.Other);
       W.printString("Name", Aux.Name.c_str());
     }
   }
 }
 
 template <class ELFT> void LLVMELFDumper<ELFT>::printHashHistograms() {
   W.startLine() << "Hash Histogram not implemented!\n";
 }
 
 // Returns true if rel/rela section exists, and populates SymbolIndices.
 // Otherwise returns false.
 template <class ELFT>
 static bool getSymbolIndices(const typename ELFT::Shdr *CGRelSection,
                              const ELFFile<ELFT> &Obj,
                              const LLVMELFDumper<ELFT> *Dumper,
                              SmallVector<uint32_t, 128> &SymbolIndices) {
   if (!CGRelSection) {
     Dumper->reportUniqueWarning(
         "relocation section for a call graph section doesn't exist");
     return false;
   }
 
   if (CGRelSection->sh_type == SHT_REL) {
     typename ELFT::RelRange CGProfileRel;
     Expected<typename ELFT::RelRange> CGProfileRelOrError =
         Obj.rels(*CGRelSection);
     if (!CGProfileRelOrError) {
       Dumper->reportUniqueWarning("unable to load relocations for "
                                   "SHT_LLVM_CALL_GRAPH_PROFILE section: " +
                                   toString(CGProfileRelOrError.takeError()));
       return false;
     }
 
     CGProfileRel = *CGProfileRelOrError;
     for (const typename ELFT::Rel &Rel : CGProfileRel)
       SymbolIndices.push_back(Rel.getSymbol(Obj.isMips64EL()));
   } else {
     // MC unconditionally produces SHT_REL, but GNU strip/objcopy may convert
     // the format to SHT_RELA
     // (https://sourceware.org/bugzilla/show_bug.cgi?id=28035)
     typename ELFT::RelaRange CGProfileRela;