[PATCH] D75498: [llvm-objdump] Add option to sort symbols during disassembly

[Jordan Rupprecht via Phabricator via llvm-commits]

rupprecht added reviewers: MaskRay, rupprecht.
rupprecht added a comment.

Is there a particular use case for this feature?

The loop that prints each symbol might have some more assumptions that it's printed in address order; it's pretty complicated so I didn't fully review it, but I'm not confident this won't introduce some subtle bug. For an initial test, you might want run `llvm-objdump -d --sort` on some large binary, and make sure that it's equivalent to `llvm-objdump -d` after you've reordered the sections with a different post-processing script.



================
Comment at: llvm/test/tools/llvm-objdump/X86/Inputs/sort-test.yaml:1
+--- !ELF
+FileHeader:      
----------------
This doesn't appear to be an actual test. Can you add some test assertions, e.g. `RUN:` lines?


================
Comment at: llvm/tools/llvm-objdump/llvm-objdump.cpp:299
+static cl::opt<bool> Sort(
+    "sort",
+    cl::desc(
----------------
Just `--sort` does not seem to be a specific flag. e.g. should we sort section headers by name? Or what about symbols *not* when disassembling, i.e. `llvm-objdump --syms --sort`


================
Comment at: llvm/tools/llvm-objdump/llvm-objdump.cpp:1332-1335
+    SortedSectionSymbolsTy sortedSymbols;
     for (unsigned SI = 0, SE = Symbols.size(); SI != SE; ++SI) {
+      sortedSymbols.emplace_back(Symbols[SI].Name, SI);
+    }
----------------
`sortedSymbols` should be preallocated (`sortedSymbols.resize(Symbols.size())`) to avoid the performance hit when increasing the size every time.


================
Comment at: llvm/tools/llvm-objdump/llvm-objdump.cpp:1343-1344
       std::string SymbolName = Symbols[SI].Name.str();
       if (Demangle)
         SymbolName = demangle(SymbolName);
 
----------------
Demangling should happen earlier, so that the symbols are also sorted when demangled


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D75498