[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
llvm-commits at lists.llvm.org
Tue Jan 11 01:27:56 PST 2022
jhenderson added inline comments.
================
Comment at: llvm/test/tools/llvm-readobj/MachO/stabs-sorted.yaml:1
+## Verify that llvm-readobj can dump files with stab symbols in a sorted order,
+
----------------
I'd name this file `sort-symbols.yaml` to match the option name. Also, is it really relevant that "stabs" are mentioned in the comment? Are all symbols stabs? If not, do non-stab symbols get sorted too (note: not a Mach-O developer, so I may be talking nonsense!).
================
Comment at: llvm/test/tools/llvm-readobj/MachO/stabs-sorted.yaml:4
+# RUN: yaml2obj %s -o %t
+# RUN: llvm-readobj --syms --sort-symbols %t | FileCheck %s --check-prefix=SORTED
+
----------------
As you've now only got one CHECK pattern in this test, you can delete the `--check-prefix` option and just use `CHECK:` and `CHECK-NEXT` below.
However, as you're also playing around with whitespace, I'd recommend adding `--strict-whitespace` and `--match-full-lines` to the FileCheck command - this will ensure that all whitespace on all lines after the `CHECK:` must exactly match that in the output. Take a look at some other examples in other tests.
================
Comment at: llvm/test/tools/llvm-readobj/MachO/stabs-sorted.yaml:54
+
+--- !mach-o
+FileHeader:
----------------
As you've now got a separate YAML, I'd change your symbol names to emphasise the differences, rather than being basically unrelated cruft copied over from the old test.
================
Comment at: llvm/tools/llvm-readobj/MachODumper.cpp:618
+
+ assert(opts::Output != opts::JSON && "not expecting JSON format yet.");
+ // Tuple of <type, name, representation>
----------------
I wouldn't bother with the `assert` here: none of Mach-O output expects JSON format, so adding an assert in one place makes it look like it needs sorting here, but not everywhere else.
================
Comment at: llvm/tools/llvm-readobj/MachODumper.cpp:650
+ // Unindent one level because the Symbols were already padded.
+ W.unindent();
+ for (auto &Tup : SortedSymbols)
----------------
I'm not clear on whether this unindent needs undoing or not after this. Probably you should manually inspect the output, both for multiple symbols, and for multiple operations, where symbol dumping happens before other output.
================
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">;
----------------
jhenderson wrote:
> 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.
Pinging the points in this comment (specifically the inline edit, and points 3 and 4).
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D116787/new/
https://reviews.llvm.org/D116787
More information about the llvm-commits
mailing list