[llvm-strings] Switch command line parsing from llvm::cl to OptTable

[MaskRay]

Summary

Some behavior changes:

• -t=d is removed. Use -t d instead.
• one-dash long options like -all are supported. Use --all instead.
• --all=0 or --all=false cannot be used. (Note: --all is silently ignored anyway)
• --help-list is removed. This is a cl:: specific option.

Nobody is likely leveraging any of the above.

Advantages:

• -t diagnostic gets improved.
• in the absence of HideUnrelatedOptions, --help will not list unrelated options if linking against libLLVM-13git.so or linker GC is not used.
• Decrease the probability of cl::opt collision if we do decide to support multiplexing

Note: because the tool is so simple, used more for forensics instead of a building
tool, and its long options are unlikely used in one-dash form, I just drop the
one-dash form in this patch.

[jhenderson]

one-dash long options like -all are supported

"are no longer"?

Generally seems reasonable to me. However, I am currently assigned to work outside our downstream binutils usage, so don't have time to investigate any ramifications. @gbreynoo, could you please check our downstream internal code bases and see if any of the behaviour changes @MaskRay mentions are going to impact them? If not, then it's okay by me. You'll want an internal ticket to track this change too, since it will require a downstream doc update.

The CommandGuide mentions --help-list, so will need updating as part of this change.

llvm/tools/llvm-strings/Opts.td

19

This should probably list the valid values.

23

llvm/tools/llvm-strings/llvm-strings.cpp

186

If the command guide is correct, I think you can drop this assignment and the corresponding variable (but not the option). The command guide says: Silently ignored. Present for GNU strings compatibility. Also, update the help text accordingly, I'd recommend.

[jhenderson]

Oh, I think this also loses the --color option, which turns off the error message colouring, if I'm not mistaken. I'm not sure either way how important this is, but thought I'd best flag it up.

[gbreynoo]

I suggest the llvm-strings User Guide also needs updating to reflect these changes, I think only --help-list needs removing.

[MaskRay]

remove --help-list from doc

[MaskRay]

In D104889#2840283, @jhenderson wrote:

Oh, I think this also loses the --color option, which turns off the error message colouring, if I'm not mistaken. I'm not sure either way how important this is, but thought I'd best flag it up.

--color is provided by a Support/WithColor.cpp cl::opt which doesn't do anything in llvm-strings.
I guess it cannot be discarded by linker GC due to some references from Support/ .

[jhenderson]

In D104889#2841696, @MaskRay wrote:

In D104889#2840283, @jhenderson wrote:

Oh, I think this also loses the --color option, which turns off the error message colouring, if I'm not mistaken. I'm not sure either way how important this is, but thought I'd best flag it up.

--color is provided by a Support/WithColor.cpp cl::opt which doesn't do anything in llvm-strings.
I guess it cannot be discarded by linker GC due to some references from Support/ .

I dug into the code more, rather than just going off of faulty memory. In other tools at least, --color controls the output colour of error message prefixes, i.e. when printing an error message via WithColor, e.g. "error: some error message" the "error:" prefix is coloured in console output, but can be turned off/on in other contexts etc. llvm-strings however, just prints straight to errs() without going via WithColor. This is actually a bug in llvm-strings in my opinion (we should be consistent with how our messages are printed across the tools). It is however somewhat tangential to this patch, and whilst this bug is present, there's little point in the --color option to my knowledge.

[MaskRay]

In D104889#2843423, @jhenderson wrote:

In D104889#2841696, @MaskRay wrote:

In D104889#2840283, @jhenderson wrote:

Oh, I think this also loses the --color option, which turns off the error message colouring, if I'm not mistaken. I'm not sure either way how important this is, but thought I'd best flag it up.

--color is provided by a Support/WithColor.cpp cl::opt which doesn't do anything in llvm-strings.
I guess it cannot be discarded by linker GC due to some references from Support/ .

I dug into the code more, rather than just going off of faulty memory. In other tools at least, --color controls the output colour of error message prefixes, i.e. when printing an error message via WithColor, e.g. "error: some error message" the "error:" prefix is coloured in console output, but can be turned off/on in other contexts etc. llvm-strings however, just prints straight to errs() without going via WithColor. This is actually a bug in llvm-strings in my opinion (we should be consistent with how our messages are printed across the tools). It is however somewhat tangential to this patch, and whilst this bug is present, there's little point in the --color option to my knowledge.

If people care about the stderr error messages, they can create a separate patch to fix it....

if (std::error_code EC = Buffer.getError())
  errs() << File << ": " << EC.message() << '\n';

Regarding -all vs --all, if a downstream group needs time for migration, we can add ["-"] temporarily in def NAME: Flag<["--"], name>, HelpText<help1>;
But I prefer not to do that in this patch, as the failing scenario seems unlikely (strings is more for forensics, not for being used as a build tool).

[aganea]

llvm/tools/llvm-strings/llvm-strings.cpp

70–71

Same comment as D105330, it would be nice if the globals were part a stack-based state instead.

[MaskRay]

llvm/tools/llvm-strings/llvm-strings.cpp

70–71

I think the global options are fine. They are trivially copyable types. The variables are referenced by more than one function so not localizable straightforwardly.

This tool is not used as a library so global states don't matter as long as they don't collide with other tools.

[MaskRay]

address comments

[jhenderson]

Looks reasonable to me, with one remaining suggestion.

llvm/tools/llvm-strings/Opts.td

17

Perhaps "Print the offset within the file, with the specified radix."?

[MaskRay]

address comments

llvm/docs/CommandGuide/llvm-strings.rst

 
  Set the minimum number of printable ASCII characters required for a sequence of
  bytes to be considered a string. The default value is 4.
 
 .. option:: --help, -h
 
  Display a summary of command line options.
 
-.. option:: --help-list
-
- Display an uncategorized summary of command line options.
-
 .. option:: --print-file-name, -f
 
  Display the name of the containing file before each string.
 
  Example:
 
  .. code-block:: console
 

llvm/test/tools/llvm-strings/grouped.test

+## Show that short options can be grouped.
+
+RUN: echo abcd | llvm-strings -af | FileCheck %s
+CHECK: {standard input}: abcd

llvm/test/tools/llvm-strings/help.test

 ## Show that help text is printed correctly when requested.
 
-RUN: llvm-strings -h | FileCheck %s --check-prefixes=CHECK,CATEG
-RUN: llvm-strings --help | FileCheck %s --check-prefixes=CHECK,CATEG
-RUN: llvm-strings --help-list \
-RUN:   | FileCheck %s --check-prefixes=CHECK,LIST
+RUN: llvm-strings -h | FileCheck %s
+RUN: llvm-strings --help | FileCheck %s
 
 CHECK: OVERVIEW: llvm string dumper
-CHECK: USAGE: llvm-strings{{(.exe)?}} [options] <input object files>{{$}}
+CHECK: USAGE: llvm-strings [options] <input object files>{{$}}
 CHECK: OPTIONS:
-CATEG:    General options:
-LIST-NOT: General options:
-CATEG:    Generic Options:
-LIST-NOT: Generic Options:
-CHECK: @FILE
+CHECK:   --all
+CHECK:   -a
+CHECK: Pass @FILE as argument to read options from FILE.

llvm/test/tools/llvm-strings/length.test

 ## Default is equivalent to -n 4.
 RUN: llvm-strings %t | FileCheck --check-prefix CHECK-4 %s --implicit-check-not={{.}}
 
 ## Show --bytes works too.
 RUN: llvm-strings --bytes 2 %t | FileCheck --check-prefix CHECK-2 %s --implicit-check-not={{.}}
 
 ## Show different syntaxes work.
 RUN: llvm-strings --bytes=2 %t | FileCheck --check-prefix CHECK-2 %s --implicit-check-not={{.}}
-RUN: llvm-strings -n=2 %t | FileCheck --check-prefix CHECK-2 %s --implicit-check-not={{.}}
+RUN: llvm-strings -n 2 %t | FileCheck --check-prefix CHECK-2 %s --implicit-check-not={{.}}
 
-CHECK-0: invalid minimum string length 0
+CHECK-0: llvm-strings: error: expected a positive integer, but got '0'
 
 CHECK-1:      a
 CHECK-1-NEXT: ab
 CHECK-1-NEXT: abc
 CHECK-1-NEXT: abcd
 CHECK-1-NEXT: abcde
 
 CHECK-2:      ab
 CHECK-2-NEXT: abc
 CHECK-2-NEXT: abcd
 CHECK-2-NEXT: abcde
 
 CHECK-4:      abcd
 CHECK-4-NEXT: abcde
 
 CHECK-5:      abcde
 
 ## Show that a non-numeric argument is rejected.
 RUN: not llvm-strings -n foo %t 2>&1 | FileCheck %s --check-prefix=ERR
-ERR: llvm-strings{{.*}}: for the --bytes option: 'foo' value invalid for integer argument!
+ERR: llvm-strings: error: expected a positive integer, but got 'foo'

llvm/test/tools/llvm-strings/radix.test

 
 ## Show --radix works too.
 RUN: llvm-strings --radix d %t/a.txt | FileCheck %s -check-prefix CHECK-DEC --strict-whitespace
 RUN: llvm-strings --radix o %t/a.txt | FileCheck %s -check-prefix CHECK-OCT --strict-whitespace
 RUN: llvm-strings --radix x %t/a.txt | FileCheck %s -check-prefix CHECK-HEX --strict-whitespace
 
 ## Show different syntaxes work.
 RUN: llvm-strings --radix=d %t/a.txt | FileCheck %s -check-prefix CHECK-DEC --strict-whitespace
-RUN: llvm-strings -t=d %t/a.txt | FileCheck %s -check-prefix CHECK-DEC --strict-whitespace
+RUN: llvm-strings -t d %t/a.txt | FileCheck %s -check-prefix CHECK-DEC --strict-whitespace
 
 CHECK-NONE: {{^}}three
 CHECK-NONE: {{^}}four
 CHECK-NONE: {{^}}five
 CHECK-NONE: {{^}}seven
 CHECK-NONE: {{^}}eight
 CHECK-NONE: {{^}}nine
 

 CHECK-HEX: {{^}}      e four
 CHECK-HEX: {{^}}     13 five
 CHECK-HEX: {{^}}     1c seven
 CHECK-HEX: {{^}}     22 eight
 CHECK-HEX: {{^}}     28 nine
 
 ## Show that an invalid value is rejected.
 RUN: not llvm-strings --radix z %t/a.txt 2>&1 | FileCheck %s --check-prefix=INVALID
-INVALID: llvm-strings{{.*}}: for the --radix option: Cannot find option named 'z'!
+INVALID: llvm-strings: error: --radix value should be one of: '' (no offset), 'o' (octal), 'd' (decimal), 'x' (hexadecimal)

llvm/tools/llvm-strings/CMakeLists.txt

 set(LLVM_LINK_COMPONENTS
   Core
   Object
+  Option
   Support
   )
 
+set(LLVM_TARGET_DEFINITIONS Opts.td)
+tablegen(LLVM Opts.inc -gen-opt-parser-defs)
+add_public_tablegen_target(StringsOptsTableGen)
+
 add_llvm_tool(llvm-strings
   llvm-strings.cpp
+  DEPENDS
+  StringsOptsTableGen
   )
 
 if(LLVM_INSTALL_BINUTILS_SYMLINKS)
   add_llvm_tool_symlink(strings llvm-strings)
 endif()

llvm/tools/llvm-strings/Opts.td

+include "llvm/Option/OptParser.td"
+
+class F<string letter, string help> : Flag<["-"], letter>, HelpText<help>;
+class FF<string name, string help> : Flag<["--"], name>, HelpText<help>;
+
+multiclass Eq<string name, string help> {
+  def NAME #_EQ : Joined<["--"], name #"=">,
+                  HelpText<help>;
+  def : Separate<["--"], name>, Alias<!cast<Joined>(NAME #_EQ)>;
+}
+
+def all : FF<"all", "Silently ignored. Present for GNU strings compatibility">;
+defm bytes : Eq<"bytes", "Print sequences of the specified length">;
+def help : FF<"help", "Display this help">;
+def print_file_name : Flag<["--"], "print-file-name">, HelpText<"Print the name of the file before each string">;
+defm radix : Eq<"radix", "Print the offset within the file with the specified radix: o (octal), d (decimal), x (hexadecimal)">, MetaVarName<"<radix>">;
+def version : FF<"version", "Display the version">;
+
+def : F<"a", "Alias for --all">, Alias<all>;
+def : F<"f", "Alias for --print-file-name">, Alias<print_file_name>;
+def : F<"h", "Alias for --help">, Alias<help>;
+def : JoinedOrSeparate<["-"], "n">, Alias<bytes_EQ>, HelpText<"Alias for --bytes">;
+def : JoinedOrSeparate<["-"], "t">, Alias<radix_EQ>, HelpText<"Alias for --radix">, MetaVarName<"<radix>">;

llvm/tools/llvm-strings/llvm-strings.cpp

 //===-- llvm-strings.cpp - Printable String dumping utility ---------------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 //
 // This program is a utility that works like binutils "strings", that is, it
 // prints out printable strings in a binary, objdump, or archive file.
 //
 //===----------------------------------------------------------------------===//
 
+#include "Opts.inc"
 #include "llvm/Object/Binary.h"
+#include "llvm/Option/Arg.h"
+#include "llvm/Option/ArgList.h"
+#include "llvm/Option/Option.h"
 #include "llvm/Support/CommandLine.h"
 #include "llvm/Support/Error.h"
 #include "llvm/Support/Format.h"
 #include "llvm/Support/InitLLVM.h"
 #include "llvm/Support/MemoryBuffer.h"
 #include "llvm/Support/Program.h"
+#include "llvm/Support/WithColor.h"
 #include <cctype>
 #include <string>
 
 using namespace llvm;
 using namespace llvm::object;
 
+namespace {
+enum ID {
+  OPT_INVALID = 0, // This is not an option ID.
+#define OPTION(PREFIX, NAME, ID, KIND, GROUP, ALIAS, ALIASARGS, FLAGS, PARAM,  \
+               HELPTEXT, METAVAR, VALUES)                                      \
+  OPT_##ID,
+#include "Opts.inc"
+#undef OPTION
+};
+
+#define PREFIX(NAME, VALUE) const char *const NAME[] = VALUE;
+#include "Opts.inc"
+#undef PREFIX
+
+static const opt::OptTable::Info InfoTable[] = {
+#define OPTION(PREFIX, NAME, ID, KIND, GROUP, ALIAS, ALIASARGS, FLAGS, PARAM,  \
+               HELPTEXT, METAVAR, VALUES)                                      \
+  {                                                                            \
+      PREFIX,      NAME,      HELPTEXT,                                        \
+      METAVAR,     OPT_##ID,  opt::Option::KIND##Class,                        \
+      PARAM,       FLAGS,     OPT_##GROUP,                                     \
+      OPT_##ALIAS, ALIASARGS, VALUES},
+#include "Opts.inc"
+#undef OPTION
+};
+
+class StringsOptTable : public opt::OptTable {
+public:
+  StringsOptTable() : OptTable(InfoTable) { setGroupedShortOptions(true); }
+};
+} // namespace
+
+const char ToolName[] = "llvm-strings";
+
 static cl::list<std::string> InputFileNames(cl::Positional,
                                             cl::desc("<input object files>"),
                                             cl::ZeroOrMore);
 
-static cl::opt<bool>
-    PrintFileName("print-file-name",
-                  cl::desc("Print the name of the file before each string"));
-static cl::alias PrintFileNameShort("f", cl::desc(""),
-                                    cl::aliasopt(PrintFileName));
-
-static cl::opt<int>
-    MinLength("bytes", cl::desc("Print sequences of the specified length"),
-              cl::init(4));
-static cl::alias MinLengthShort("n", cl::desc(""), cl::aliasopt(MinLength));
-
-static cl::opt<bool>
-    AllSections("all",
-                  cl::desc("Check all sections, not just the data section"));
-static cl::alias AllSectionsShort("a", cl::desc(""),
-                                    cl::aliasopt(AllSections));
+static int MinLength = 4;
+static bool PrintFileName;
 
 enum radix { none, octal, hexadecimal, decimal };
-static cl::opt<radix>
-    Radix("radix", cl::desc("print the offset within the file"),
-          cl::values(clEnumValN(octal, "o", "octal"),
-                     clEnumValN(hexadecimal, "x", "hexadecimal"),
-                     clEnumValN(decimal, "d", "decimal")),
-          cl::init(none));
-static cl::alias RadixShort("t", cl::desc(""), cl::aliasopt(Radix));
+static radix Radix;
+
+LLVM_ATTRIBUTE_NORETURN static void reportCmdLineError(const Twine &Message) {
+  WithColor::error(errs(), ToolName) << Message << "\n";
+  exit(1);
+}
 
-static cl::extrahelp
-    HelpResponse("\nPass @FILE as argument to read options from FILE.\n");
+template <typename T>
+static void parseIntArg(const opt::InputArgList &Args, int ID, T &Value) {
+  if (const opt::Arg *A = Args.getLastArg(ID)) {
+    StringRef V(A->getValue());
+    if (!llvm::to_integer(V, Value, 0) || Value <= 0)
+      reportCmdLineError("expected a positive integer, but got '" + V + "'");
+  }
+}
 
 static void strings(raw_ostream &OS, StringRef FileName, StringRef Contents) {
   auto print = [&OS, FileName](unsigned Offset, StringRef L) {
     if (L.size() < static_cast<size_t>(MinLength))
       return;
     if (PrintFileName)
       OS << FileName << ": ";
     switch (Radix) {
for (P = Contents.begin(), E = Contents.end(); P < E; ++P) {
     }
   }
   if (S)
     print(S - B, StringRef(S, E - S));
 }
 
 int main(int argc, char **argv) {
   InitLLVM X(argc, argv);
+  BumpPtrAllocator A;
+  StringSaver Saver(A);
+  StringsOptTable Tbl;
+  opt::InputArgList Args =
+      Tbl.parseArgs(argc, argv, OPT_UNKNOWN, Saver,
+                    [&](StringRef Msg) { reportCmdLineError(Msg); });
+  if (Args.hasArg(OPT_help)) {
+    Tbl.printHelp(
+        outs(),
+        (Twine(ToolName) + " [options] <input object files>").str().c_str(),
+        "llvm string dumper");
+    // TODO Replace this with OptTable API once it adds extrahelp support.
+    outs() << "\nPass @FILE as argument to read options from FILE.\n";
+    return 0;
+  }
+  if (Args.hasArg(OPT_version)) {
+    outs() << ToolName << '\n';
+    cl::PrintVersionMessage();
+    return 0;
+  }
+
+  parseIntArg(Args, OPT_bytes_EQ, MinLength);
+  PrintFileName = Args.hasArg(OPT_print_file_name);
+  StringRef R = Args.getLastArgValue(OPT_radix_EQ);
+  if (R.empty())
+    Radix = none;
+  else if (R == "o")
+    Radix = octal;
+  else if (R == "d")
+    Radix = decimal;
+  else if (R == "x")
+    Radix = hexadecimal;
+  else
+    reportCmdLineError("--radix value should be one of: '' (no offset), 'o' "
+                       "(octal), 'd' (decimal), 'x' (hexadecimal)");
 
-  cl::ParseCommandLineOptions(argc, argv, "llvm string dumper\n");
   if (MinLength == 0) {
     errs() << "invalid minimum string length 0\n";
     return EXIT_FAILURE;
   }
 
+  std::vector<std::string> InputFileNames = Args.getAllArgValues(OPT_INPUT);
   if (InputFileNames.empty())
     InputFileNames.push_back("-");
 
   for (const auto &File : InputFileNames) {
     ErrorOr<std::unique_ptr<MemoryBuffer>> Buffer =
         MemoryBuffer::getFileOrSTDIN(File);
     if (std::error_code EC = Buffer.getError())
       errs() << File << ": " << EC.message() << '\n';
     else
       strings(llvm::outs(), File == "-" ? "{standard input}" : File,
               Buffer.get()->getMemBufferRef().getBuffer());
   }
 
   return EXIT_SUCCESS;
 }