[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-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:    ]
----------------
oontvoo wrote:
> jhenderson wrote:
> > oontvoo wrote:
> > > 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.
> > Whitespace padding isn't relevant to this test: the formatting should be tested in a basic symbol printing test, not in a test that is about the `--sort-symbols` option. Besides: without the `--strict-whitespace` and `--match-full-lines` FileCheck options, this doesn't actually verify the whitespace properly.
> the comment about white  was out of date (no longer needed) 
You can still simplify the checks as described in my original comment.


================
Comment at: llvm/tools/llvm-readobj/MachODumper.cpp:647
 
-void MachODumper::printDynamicSymbols() {
+void MachODumper::printDynamicSymbols(const SymbolComparator * /*unused*/) {
   ListScope Group(W, "DynamicSymbols");
----------------
jhenderson wrote:
> I might be mistaken, but I believe LLVM usually doesn't bother commenting out unused parameter names.
Unaddressed but marked as done?


================
Comment at: llvm/tools/llvm-readobj/llvm-readobj.cpp:359
   ObjDumper *Dumper;
+  std::unique_ptr<SymbolComparator> SymComp;
   Expected<std::unique_ptr<ObjDumper>> DumperOrErr = createDumper(Obj, Writer);
----------------
jhenderson wrote:
> `llvm::Optional` would be the more expressive form here. You could then pass it directly, rather than via the pointer, and have a `None` check instead of a `nullptr` check.
I was mistaken to put the `llvm::` qualifier before `Optional`. It looks like many (most?) instances don't use the qualifier for it or `None`, so please remove it.


================
Comment at: llvm/tools/llvm-readobj/llvm-readobj.cpp:371
+        case NAME:
+          SymComp->addPredicate([Dumper](SymbolRef LHS, SymbolRef RHS) -> bool {
+            return Dumper->compareSymbolsByName(LHS, RHS);
----------------
jhenderson wrote:
> Here and below, I don't think you need the trailing return types.
Not addressed?


================
Comment at: llvm/tools/llvm-readobj/llvm-readobj.cpp:386
+    } else {
+      error("--sort-symbols is not supported yet for this format.");
+    }
----------------
oontvoo wrote:
> jhenderson wrote:
> > 1) Error and warning messages shouldn't end with a "."
> > 2) I have concerns here: this will stop dumping as soon as an unsupported file type is encountered (even in an archive), yet that is unlikely what the user wants to happen. They more likely want to continue dumping and just not sort in this case (imagine if they had mutliple different file format objects in their input). This should be at most a warning (including the input file name), although I could even see an argument for not having it at all. It also needs testing.
> 1. Fixed
> 2. If the format doesn't support --sort-symbols then the users shouldn't specify --sort-symbols.  (Keeping it as error for consistency - similarly to how it handles unknown sort-key above, which is that if it doesn't understand it, then it's an error, rather than trying to guess. This file also doesn't have precedence for "warning")
You're making the potentially incorrect assumption that all input are of the same format. If a user has two objects of different formats, they might want the symbols sorted for the ones that can be:
```
$ llvm-readobj elf.o macho.o --sort-symbols --symbols
```
Would result in an error, and nothing printed (not even macho.o's symbols).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D116787