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

[Vy Nguyen via Phabricator via llvm-commits]

oontvoo marked an inline comment as done.
oontvoo added a comment.

In D116787#3345140 <https://reviews.llvm.org/D116787#3345140>, @jhenderson wrote:

> I feel like there's an awful lot more code than is required for this patch. I stopped commenting on inidividual things partway through. I think the following outline should be sufficient. It probably isn't 100% correct, but you should be able to get the general idea from it.
>
>   // This code could all live in generic area, since this is generic behaviour.
>   bool compareSymName(SymbolRef LHS, SymbolRef RHS) {
>     // Implementation left as an exercise for the reader. In essence:
>     // return LHS.Name < RHS.Name
>   }
>   
>   bool compareSymType(SymbolRef LHS, SymbolRef RHS) {
>     // Implementation left as an exercise for the reader. In essence:
>     // return LHS.Type < RHS.Type
>   }
>   
>   class SymbolComparer {
>   public:
>     using ComparePred = function_ref<bool(SymbolRef, SymbolRef)>;
>     void add(ComparePred Pred) { Predicates.push_back(Pred); }
>   
>     bool operator()(SymbolRef LHS, SymbolRef RHS) {
>       for(ComparePred Pred : Predicates) {
>         if (Pred(LHS, RHS))
>           return true;
>         if (Pred(RHS, LHS))
>           return false;
>       }
>       // All considered parameters are equal. This means that a SymbolComparer
>       // taking an empty vector in the constructor will treat all symbols as equal.
>       return false;
>     }
>   
>   private:
>     SmallVector<ComparePred, 2> Predicates;
>   };
>   
>   // Code in MachODumper.cpp, possibly even other files too.
>   void MachODumper::printSymbols(const SymbolComparer &SymCmp) {
>     ListScope Group(W, "Symbols");
>     auto SymbolRange = Obj->symbol();
>     std::vector<SymbolRef> SortedSymbols(SymbolRange.begin(), SymbolRange.end());
>     stable_sort(SortedSymbols, SymCmp);
>     for (SymbolRef Symbol : SortedSymbols)
>       printSymbol(Symbol);
>   }
>   
>   // In main
>   SymbolComparer SymCmp;
>   if (Arg *A = Args.getLastArg(OPT_sort_symbols_EQ)) {
>     const StringMap <SymbolComparer::ComparePred> KeyToPredMap =
>       {{"name", compareSymName}, {"type", compareSymType}};
>     for (StringRef KeyStr : llvm::split(A->getValue(), ",")) {
>       auto Found = KeyToPredMap.find(KeyStr);
>       if(Found == KeyToPredMap.end())
>         error("--sort-symbols value should be 'name' or 'type', but was '" +
>               Twine(KeyStr) + "'");
>       else
>         SymCmp.add(Found->getValue());
>     }
>   }

I'm not sure that's much less code - the only substantial chunk of code right now is in MachOODumper.cpp, line ~602 to 722.
The rest is just one or two line of plumping the args through.

(Also there were some unrelated formatting changes when I ran clang-format ... will revert those)



================
Comment at: llvm/test/tools/llvm-readobj/MachO/stabs-sorted.yaml:23-31
+# TYPE-NAME-NEXT:  Symbol {
+# TYPE-NAME-NEXT:    Name: _a (19)
+# TYPE-NAME-NEXT:    Type: Section (0xE)
+# TYPE-NAME-NEXT:    Section: __data (0x3)
+# TYPE-NAME-NEXT:    RefType: UndefinedNonLazy (0x0)
+# TYPE-NAME-NEXT:    Flags [ (0x0)
+# TYPE-NAME-NEXT:    ]
----------------
jhenderson wrote:
> Related to my above comment re. formatting, we don't need to show that the entire symbol printing is correct - that's the responsibility of a test that is testing the `--symbols` option rather than the `--sort-symbols` option. Instead, I'd just check the name and type fields. Something like this:
> 
> ```
> TYPE-NAME:      Name: _a
> TYPE-NAME-NEXT: Type: Section
> TYPE-NAME:      Name: _d
> TYPE-NAME-NEXT: Type: Section
> ```
i can drop this in the other test (the NAME-TYPE one) but this was needed here to ensure the whitespace padding was done correctly.


================
Comment at: llvm/tools/llvm-readobj/MachODumper.cpp:680
+  if (SortKeys && !SortKeys->empty()) {
+    // The references returned by calling Obj->symbols() are temporary,
+    // and we can't hold onto them after the loop. In order to sort the
----------------
jhenderson wrote:
> Are you sure about this? They don't look all that temporary (as long as the object file is still in memory)...
> Are you sure about this? They don't look all that temporary (as long as the object file is still in memory)...

Yes - I've run the code with asan(previous version which simply stored the returned values from `->symbols()` to a vector then sorted it) and got errors


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D116787