[PATCH] D105532: [llvm-readobj] Switch command line parsing from llvm::cl to OptTable

Wed Jul 7 01:16:24 PDT 2021

jhenderson added a subscriber: gbreynoo.
jhenderson added a comment.

Docs update for llvm-readobj and llvm-readelf?

`check-default-options.txt` in the Support tests is failing due to this change.

> `--demangle=false` and `--demangle=0` cannot be used. Just omit the option.

In limited situations, a user may be unable to easily remove --demangle from earlier in the command-line (e.g. it's part of a script), so I think having a `--no-demangle` option is somewhat important.

> `--section-mapping=false` (D57365 <https://reviews.llvm.org/D57365>) is strange but is kept for now.

I'd like to see the functionality kept (it allows turning off section mapping dumping when using GNU output style to dump the program header table), but I don't mind if the option changes to, e.g. `--no-section-mapping`, in a future change.

> One-dash long options are still supported.

If I follow things correctly, this isn't entirely true. For example, `-dt` no longer means `--dyn-symbols`.

@gbreynoo, another option modification patch for you to look at and be aware of.

Note to self: out of time now, but still need to review the llvm-readobj.cpp changes.

================
Comment at: llvm/test/tools/llvm-readobj/ELF/grouped.test:53-55
+# RUN: llvm-readobj -aeWhSsrnudlVgIS %t.o > %t.grouepd
+# RUN: llvm-readobj -a -e -W -h -S -r -n -u -d -l -V -g -I -s %t.o > %t.not-grouepd
+# RUN: cmp %t.grouepd %t.not-grouepd
----------------

================
Comment at: llvm/tools/llvm-readobj/Opts.td:45
+def symbols : FF<"symbols", "Display the symbol table. Also display the dynamic symbol table when using GNU output style for ELF">;
+def unwind : FF<"unwind", "Displays unwind information">;
+
----------------

================
Comment at: llvm/tools/llvm-readobj/Opts.td:48
+// ELF specific options.
+def grp_elf : OptionGroup<"kind">, HelpText<"llvm-readobj ELF Specific Options">;
+def dynamic_table : FF<"dynamic-table", "Display the dynamic section table">, Group<grp_elf>;
----------------
We should probably drop the "llvm-readobj" bit from these descriptions, as the tool is usually installed as "llvm-readelf" on people's systems, so the name would be incorrect there. Here, we can simply say "ELF Specific Options".

================
Comment at: llvm/tools/llvm-readobj/Opts.td:49
+def grp_elf : OptionGroup<"kind">, HelpText<"llvm-readobj ELF Specific Options">;
+def dynamic_table : FF<"dynamic-table", "Display the dynamic section table">, Group<grp_elf>;
+def elf_hash_histogram : FF<"elf-hash-histogram", "Display bucket list histogram for hash sections">, Group<grp_elf>;
----------------
If I'm not mistaken, you're missing the `--dynamic` alias.

================
Comment at: llvm/tools/llvm-readobj/Opts.td:50
+def dynamic_table : FF<"dynamic-table", "Display the dynamic section table">, Group<grp_elf>;
+def elf_hash_histogram : FF<"elf-hash-histogram", "Display bucket list histogram for hash sections">, Group<grp_elf>;
+def elf_linker_options : FF<"elf-linker-options", "Display the .linker-options section">, Group<grp_elf>;
----------------
Does clang-format have a tablegen formatting option? This and a few other lines in this patch are getting quite long.

================
Comment at: llvm/tools/llvm-readobj/Opts.td:53
+defm elf_output_style : Eq<"elf-output-style", "Specify ELF dump style">, Group<grp_elf>;
+def elf_section_groups : FF<"elf-section-groups", "Display section groups">, Group<grp_elf>;
+def gnu_hash_table : FF<"gnu-hash-table", "Display .gnu.hash section">, Group<grp_elf>;
----------------
You seem to be missing the `--section-groups` alias.

================
Comment at: llvm/tools/llvm-readobj/Opts.td:72
+
+// PE/COFF specific options.
+def grp_coff : OptionGroup<"kind">, HelpText<"llvm-readobj PE/COFF Specific Options">;
----------------
This set isn't ordered alphabetically, unlike the other sets.

================
Comment at: llvm/tools/llvm-readobj/Opts.td:102
+def : FF<"dyn-syms", "Alias for --dyn-symbols">, Alias<dyn_symbols>;
+def : FF<"elf-cg-profile", "Alias for --cg-profile">, Alias<cg_profile>;
+def : FF<"file-headers", "Alias for --file-header">, Alias<file_header>, Flags<[HelpHidden]>;
----------------
I wouldn't be sad if you wanted to hide the "elf-" aliases (and at some point later remove them entirely). I think they are largely a historical mistake.

================
Comment at: llvm/tools/llvm-readobj/Opts.td:104
+def : FF<"file-headers", "Alias for --file-header">, Alias<file_header>, Flags<[HelpHidden]>;
+def : FF<"histogram", "Alias for --elf-hash-histogram">, Alias<elf_hash_histogram>;
+def : FF<"relocations", "Alias for --relocs">, Alias<relocs>;
----------------
I'd make `--histogram` the canonical form, and `--elf-hash-histogram` the alias personally. Also applies to `-I`

================
Comment at: llvm/tools/llvm-readobj/Opts.td:115
+def : F<"e", "Alias for --headers">, Alias<headers>;
+def : F<"g", "Alias for --elf-section-groups">, Alias<elf_section_groups>;
+def : F<"h", "Alias for --file-header">, Alias<file_header>;
----------------
I'd make this refer to `--section-groups`, not `--elf-section-groups` (the former is the GNU name, the latter not - see also above).

================
Comment at: llvm/tools/llvm-readobj/Opts.td:117
+def : F<"h", "Alias for --file-header">, Alias<file_header>;
+def : F<"n", "Alias for --notes">, Alias<notes>;
+def : F<"I", "Alias for --elf-hash-histogram">, Alias<elf_hash_histogram>;
----------------
This should be further down in the list, for alphabetical ordering.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D105532