[PATCH] D105330: [llvm-nm] Switch command line parsing from llvm::cl to OptTable

[James Henderson via Phabricator via llvm-commits]

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

@gbreynoo, could you do a check to see if any of our internal code bases would be broken by the changes made in this patch?

@MaskRay: not that I mind, but any particular reason single-dash long options are still supported for llvm-nm, but not other tools? That could be explained in the patch description, I think.

> The -s collision (-s segment section for Mach-O) is unfortunate. -s means --print-armap in GNU nm.

I wonder if (in a separate patch), we should add a long option and deprecate this short alias? Then, in a future release, we can switch the meaning over, whilst giving people a path forward to migrate any scripts they may have.



================
Comment at: llvm/test/tools/llvm-nm/help.test:5
+CHECK:   --demangle
+CHECK:   --format=<value>
----------------
Perhaps worth adding some check showing the Mach-O option grouping?


================
Comment at: llvm/tools/llvm-nm/Opts.td:33
+def reverse_sort : FF<"reverse-sort", "Sort in reverse order">;
+def quiet : FF<"quiet", "Suppress 'no symbols' diagnostic">;
+def size_sort : FF<"size-sort", "Precede each symbol with the object file it came from">;
----------------
This should be above `radix` for alphabetical ordering.


================
Comment at: llvm/tools/llvm-nm/Opts.td:34-35
+def quiet : FF<"quiet", "Suppress 'no symbols' diagnostic">;
+def size_sort : FF<"size-sort", "Precede each symbol with the object file it came from">;
+def special_syms : FF<"special-syms", "Precede each symbol with the object file it came from">;
+def undefined_only : FF<"undefined-only", "Show only undefined symbols">;
----------------
Help description of these two options is incorrect.


================
Comment at: llvm/tools/llvm-nm/Opts.td:46
+def dyldinfo_only : FF<"dyldinfo-only", "Show only symbols from the dyldinfo">, Group<grp_mach_o>;
+def add_inlinedinfo : FF<"add-inlinedinfo", "Add symbols from the inlined libraries, TBD only">, Group<grp_mach_o>;
+def no_dyldinfo : FF<"no-dyldinfo", "Don't add any symbols from the dyldinfo">, Group<grp_mach_o>;
----------------
This should be above `arch` for alphabetical ordering.


================
Comment at: llvm/tools/llvm-nm/Opts.td:61
+def : Separate<["-"], "f">, HelpText<"Alias for --format">, Alias<format_EQ>;
+def : F<"g", "Alias for --extern-only">, Alias<extern_only>;
+def : F<"j", "Alias for --format=just-symbols">, Alias<format_EQ>, AliasArgs<["just-symbols"]>;
----------------
-h alias for --help?


================
Comment at: llvm/tools/llvm-nm/llvm-nm.cpp:114
 enum Radix { d, o, x };
-cl::opt<Radix>
-    AddressRadix("radix", cl::desc("Radix (o/d/x) for printing symbol Values"),
-                 cl::values(clEnumVal(d, "decimal"), clEnumVal(o, "octal"),
-                            clEnumVal(x, "hexadecimal")),
-                 cl::init(x), cl::cat(NMCat));
-cl::alias RadixAlias("t", cl::desc("Alias for --radix"),
-                     cl::aliasopt(AddressRadix), cl::NotHidden);
-
-cl::opt<bool> JustSymbolName("just-symbol-name",
-                             cl::desc("Alias for --format=just-symbols"),
-                             cl::cat(NMCat), cl::NotHidden);
-cl::alias JustSymbolNames("j", cl::desc("Alias for --format-just-symbols"),
-                          cl::aliasopt(JustSymbolName), cl::Grouping,
-                          cl::NotHidden);
-
-cl::opt<bool>
-    SpecialSyms("special-syms",
-                cl::desc("Do not filter special symbols from the output"),
-                cl::cat(NMCat));
-
-cl::list<std::string> SegSect("s", cl::multi_val(2), cl::ZeroOrMore,
-                              cl::value_desc("segment section"), cl::Hidden,
-                              cl::desc("Dump only symbols from this segment "
-                                       "and section name, Mach-O only"),
-                              cl::cat(NMCat));
-
-cl::opt<bool> FormatMachOasHex("x",
-                               cl::desc("Print symbol entry in hex, "
-                                        "Mach-O only"),
-                               cl::Grouping, cl::cat(NMCat));
-cl::opt<bool> AddDyldInfo("add-dyldinfo",
-                          cl::desc("Add symbols from the dyldinfo not already "
-                                   "in the symbol table, Mach-O only"),
-                          cl::cat(NMCat));
-cl::opt<bool> NoDyldInfo("no-dyldinfo",
-                         cl::desc("Don't add any symbols from the dyldinfo, "
-                                  "Mach-O only"),
-                         cl::cat(NMCat));
-cl::opt<bool> DyldInfoOnly("dyldinfo-only",
-                           cl::desc("Show only symbols from the dyldinfo, "
-                                    "Mach-O only"),
-                           cl::cat(NMCat));
-
-cl::opt<bool> NoLLVMBitcode("no-llvm-bc",
-                            cl::desc("Disable LLVM bitcode reader"),
-                            cl::cat(NMCat));
-
-cl::opt<bool> AddInlinedInfo("add-inlinedinfo",
-                             cl::desc("Add symbols from the inlined libraries, "
-                                      "TBD(Mach-O) only"),
-                             cl::cat(NMCat));
-
-cl::opt<bool> Version("V", cl::desc("Print version info"), cl::cat(NMCat));
-
-cl::extrahelp HelpResponse("\nPass @FILE as argument to read options from FILE.\n");
-
-bool PrintAddress = true;
-
-bool MultipleFiles = false;
-
-bool HadError = false;
-
-std::string ToolName;
-} // anonymous namespace
+}
+static Radix AddressRadix;
----------------
clang-format I think adds this.


================
Comment at: llvm/tools/llvm-nm/llvm-nm.cpp:2143
+    // TODO Replace this with OptTable API once it adds extrahelp support.
+    outs() << "\nPass @FILE as argument to read options from FILE.\n";
+    return 0;
----------------
Perhaps worth adding a check for this (or more specifically probably just `@FILE` to help.test?


================
Comment at: llvm/tools/llvm-nm/llvm-nm.cpp:2220-2222
+  for (const auto *A : Args.filtered(OPT_arch_EQ)) {
+    SmallVector<StringRef, 2> Values;
+    llvm::SplitString(A->getValue(), Values, ",");
----------------
It's unfortunate that we have to hand-roll this additional logic, rather than relying on the command-line parser code to do it for us...


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D105330