[PATCH] D116787: [llvm-readobj][MachO] Add option to sort the symbol table before dumping (MachO only, for now).

[James Henderson via Phabricator via llvm-commits]

jhenderson added inline comments.


================
Comment at: llvm/test/tools/llvm-readobj/MachO/stabs.yaml:56
 
+# RUN: llvm-readobj --syms --sort-symbols %t | FileCheck %s --check-prefix=SORTED
+
----------------
It's probably worth pulling this into a new test file entirely, so that you can have additional cases that trigger different tie-breaking behaviours.


================
Comment at: llvm/tools/llvm-readobj/MachODumper.cpp:611
+void MachODumper::printSymbols(bool SortSymbols) {
+
+  if (SortSymbols) {
----------------
Don't add a blank line at the start of a function.


================
Comment at: llvm/tools/llvm-readobj/MachODumper.cpp:613-616
+    // The references returned by calling Obj->symbols() is temporary
+    // and we can't hold on to them after the loop.
+    // So in order to sort the symbols, we have to print their representation
+    // to string and sort them later.
----------------



================
Comment at: llvm/tools/llvm-readobj/MachODumper.cpp:621
+    std::vector<std::tuple<uint8_t, std::string, std::string>> SortedSymbols;
+    for (const SymbolRef &Symbol : Obj->symbols()) {
+      std::string SymRep;
----------------
Entirely up to you, as it's existing code, but you could probably just drop the `const &`, since `SymbolRef` is designed to be easily copyable.


================
Comment at: llvm/tools/llvm-readobj/MachODumper.cpp:624
+      raw_string_ostream StringOs(SymRep);
+      llvm::formatted_raw_ostream FormattedOs(StringOs);
+      std::unique_ptr<ScopedPrinter> Printer;
----------------



================
Comment at: llvm/tools/llvm-readobj/MachODumper.cpp:627-630
+      if (opts::Output == opts::JSON)
+        Printer = std::make_unique<JSONScopedPrinter>(
+            FormattedOs, opts::PrettyPrint ? 2 : 0,
+            std::make_unique<ListScope>());
----------------
I'm surprised you need this here, as JSON output format isn't supported yet for non-ELF formats. Probably remove it for now (especially as this code path is currently untested).


================
Comment at: llvm/tools/llvm-readobj/MachODumper.cpp:634-638
+      // Hack!
+      // Added a fake list scope so that the Symbol have the right indentation.
+      // Without it, each symbol will have zero-indent (because we print them
+      // individually)
+      ListScope Group(*Printer, "");
----------------
I think there might be a cleaner way of doing this: `ScopedPrinter` has a `getIndentLevel` method, which you could use to set the indentation of the new printer you have here, based on that of the existing one. This will make this code more reusable too, since in different contexts, the indentation will be different.

It would look something like this: `Printer.indent(W.getIndentLevel());`, although you might need a "+ 1" or similar, I'm not sure.


================
Comment at: llvm/tools/llvm-readobj/MachODumper.cpp:650-651
+    llvm::stable_sort(SortedSymbols,
+                      [](std::tuple<uint8_t, std::string, std::string> LHS,
+                         std::tuple<uint8_t, std::string, std::string> RHS) {
+                        uint8_t LType = std::get<0>(LHS);
----------------
These should be `const &`, right? Otherwise, you end up copying the tuple contents.


================
Comment at: llvm/tools/llvm-readobj/MachODumper.cpp:658
+                        return LType < RType;
+                        // FIXME: Maybe use Symbol's addresses as tie-breaker if
+                        // names are the same.
----------------
This isn't a bug, merely a possible improvement.


================
Comment at: llvm/tools/llvm-readobj/MachODumper.cpp:662
+
+    ListScope Group(W, "Symbols");
+    for (auto &Tup : SortedSymbols)
----------------
Note that if you move this `ListScope` earlier, outside the `if`, you a) avoid the duplication with the `else`, and b), shouldn't need the `+ 1` I mentioned in my earlier comment about indentation.


================
Comment at: llvm/tools/llvm-readobj/MachODumper.cpp:668
 
-  for (const SymbolRef &Symbol : Obj->symbols()) {
-    printSymbol(Symbol);
+    for (const SymbolRef &Symbol : Obj->symbols())
+      printSymbol(Symbol);
----------------



================
Comment at: llvm/tools/llvm-readobj/Opts.td:46
 def unwind : FF<"unwind", "Display unwind information">;
+def sort_symbols : FF<"sort-symbols", "Sort symbol before outputing symtab">;
 
----------------
1) "displaying" is the term used for other options.
2) This option is (currently) Mach-O specific. Unless you plan on implementing it for other formats too, please move it to the Mach-O specific options block.
3) These options are listed alphabetically within each block. Please maintain that order.
4) Please remember to update the documentation for llvm-readobj (and llvm-readelf, if you are planning on this being a generic option), located at llvm/docs/CommandGuide.
5) If this is going to be Mach-O specific (for now), I'd name the variable name accordingly (i.e. something like `macho_sort_symbols`. Also, it will need to have the `grp_macho` Group, like the other Mach-O specific options.


================
Comment at: llvm/tools/llvm-readobj/llvm-readobj.cpp:102
 static std::vector<std::string> HexDump;
-static bool PrettyPrint;
+bool PrettyPrint;
 static bool PrintStackMap;
----------------
Noting that this change isn't needed if you drop the reference to JSON output style from elsewhere in this patch.


================
Comment at: llvm/tools/llvm-readobj/llvm-readobj.cpp:258
   opts::VersionInfo = Args.hasArg(OPT_version_info);
+  opts::SortSymbols = Args.hasArg(OPT_sort_symbols);
 
----------------
This option is currently Mach-O specific, so move it accordingly, unless you plan on implementing other formats. Also, these lists are in alphabetical order. Please maintain that order.


================
Comment at: llvm/tools/llvm-readobj/llvm-readobj.h:42
 extern bool Demangle;
+extern bool PrettyPrint;
 enum OutputStyleTy { LLVM, GNU, JSON, UNKNOWN };
----------------
Noting that this change isn't needed if you drop the reference to JSON output style from elsewhere in this patch.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D116787