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

[James Henderson via Phabricator via llvm-commits]

jhenderson added a comment.

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.



================
Comment at: llvm/include/llvm/Support/ScopedPrinter.h:233
 
+  virtual void printNumber(StringRef Label, float Value) {
+    startLine() << Label << ": " << format("%5.1f", Value) << "\n";
----------------
This should probably be spun off into a separate patch and have testing in the unit tests.


================
Comment at: llvm/test/tools/llvm-readobj/ELF/hash-histogram.test:15
 
+## Check that LLVM output has the expected format
+# RUN: llvm-readobj --elf-hash-histogram %t1-32.o | FileCheck %s --check-prefix=LLVM-HIST
----------------



================
Comment at: llvm/tools/llvm-readobj/ELFDumper.cpp:2707
+  size_t TotalSyms = 0;
+  // If hash table is correct, we have at least chains with 0 length
+  size_t MaxChain = 1;
----------------
Whilst moving, please add the missing "." to this comment.


================
Comment at: llvm/tools/llvm-readobj/ELFDumper.cpp:2738
+  std::vector<size_t> Count(MaxChain, 0);
+  // Count how long is the chain for each bucket
+  for (size_t B = 0; B < NBucket; B++)
----------------
Ditto.


================
Comment at: llvm/tools/llvm-readobj/ELFDumper.cpp:2742
+  // Print Number of buckets with each chain lengths and their cumulative
+  // coverage of the symbols
+  printHashHistogramStats(NBucket, MaxChain, TotalSyms, Count);
----------------
Ditto.


================
Comment at: llvm/tools/llvm-readobj/ELFDumper.cpp:2788
+  // Print Number of buckets with each chain lengths and their cumulative
+  // coverage of the symbols
+  printGnuHashHistogramStats(NBucket, MaxChain, TotalSyms, Count);
----------------
Ditto.


================
Comment at: llvm/tools/llvm-readobj/ELFDumper.cpp:7130-7132
+void LLVMELFDumper<ELFT>::printHashHistogramStats(
+    size_t NBucket, size_t MaxChain, size_t TotalSyms,
+    const ArrayRef<size_t> Count) const {
----------------
Any particular reason you've made the `Count` member `const` in these two methods?


================
Comment at: llvm/tools/llvm-readobj/ELFDumper.cpp:7137
+  size_t CumulativeNonZero = 0;
+  for (size_t I = 0; I < MaxChain; I++) {
+    CumulativeNonZero += Count[I] * I;
----------------



================
Comment at: llvm/tools/llvm-readobj/ELFDumper.cpp:7155
+  size_t CumulativeNonZero = 0;
+  for (size_t I = 0; I < MaxChain; I++) {
+    CumulativeNonZero += Count[I] * I;
----------------
https://llvm.org/docs/CodingStandards.html#prefer-preincrement


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D137096