[PATCH] D105884: [llvm-readobj] Display multiple function names for stack size entries

[James Henderson via Phabricator via llvm-commits]

jhenderson added inline comments.


================
Comment at: llvm/test/tools/llvm-readobj/ELF/stack-sizes-multiple-symbols.test:1
+# RUN: yaml2obj --docnum=1 %s -o %t
+# RUN: llvm-readelf --stack-sizes %t 2>&1 \
----------------
It probably makes more sense to add this test case to the existing stack-sizes.test test.


================
Comment at: llvm/test/tools/llvm-readobj/ELF/stack-sizes.test:180
 ## object file. This also shows that the sh_link field is ignored in this situation
-## without warning.
+## without warning and that if two symbols have the same value, both are output,
 
----------------
Multiple symbols with the same value is not specific to executables, so combining these test cases sounds like it risks confusion. It probably makes more sense to just take the multiple symbols case you've written in a separate file and add it to this file.


================
Comment at: llvm/tools/llvm-readobj/ELFDumper.cpp:65
 #include <memory>
+#include <numeric>
 #include <string>
----------------
What's this for?


================
Comment at: llvm/tools/llvm-readobj/ELFDumper.cpp:306-308
+  SmallVector<uint32_t>
+  getSymbolIndexesForFunctionAddress(uint64_t SymValue,
                                    Optional<const Elf_Shdr *> FunctionSec);
----------------
Make sure to clang-format new/modified code.


================
Comment at: llvm/tools/llvm-readobj/ELFDumper.cpp:5814
+    SmallVector<std::string> FuncSymNames;
+    for (uint32_t index : FuncSymIndexes)
+      FuncSymNames.push_back(this->getStaticSymbolName(index));
----------------
Nit: fix all the clang-tidy warnings.


================
Comment at: llvm/tools/llvm-readobj/ELFDumper.cpp:5824
+void GNUELFDumper<ELFT>::printStackSizeEntry(
+    uint64_t Size, const SmallVector<std::string> &FuncNames) {
   OS.PadToColumn(2);
----------------
Use `ArrayRef` for passing around vectors by reference. Same elsewhere.


================
Comment at: llvm/tools/llvm-readobj/ELFDumper.cpp:5831
+      std::next(FuncNames.begin()), FuncNames.end(), FuncNames.front(),
+      [this](const std::string &AllNames, const std::string &FuncNames) {
+        return AllNames + ", " + FuncNames;
----------------
Use `StringRef`, not `const std::string &`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D105884