[llvm-debuginfod][NFC] Switch to OptTable

[mysterymath]

llvm/tools/llvm-debuginfod/llvm-debuginfod.cpp

81–85

This should actually check that the integer is nonnegative, and the name should reflect this (NonNegInt?)

[avillega]

llvm/tools/llvm-debuginfod/llvm-debuginfod.cpp

81–85

What are your thoughts on me changing the types of the Options to also be non-negative types. in this case ScanInterval and MaxConcurrency will become unsigned and usize_t respectivelly, which will catch the error of passing non-negative integers at parse time.

[mysterymath]

llvm/tools/llvm-debuginfod/llvm-debuginfod.cpp

81–85

Hmm, actually if this is intended to be a NFC change (oh, which should probably have the [NFC] commit label, as a nit-pick), it would probably be better to just change the error message to ": expected an integer...".

[avillega]

Review comments

[avillega]

Comments done

[abrachet]

It would also be nice to update the bazel build. Technically we (because we don't work in google3) aren't required to keep the bazel build clean, though Google LLVM integrators will often see a commit was broken by a @ google.com address and ask that you fix it.

In these cases, it might not be immediately obvious to the integrator how to fix this issues so it would certainly be courteous to fix it before landing. You can opt to join the bazel-build group https://reviews.llvm.org/project/profile/107/ which will add precommit checks to ensure the bazel build built cleanly.

Though feel free to ignore

llvm/tools/llvm-debuginfod/CMakeLists.txt

13

[avillega]

@mysterymath I do not have commit access yet, as the reviewer, if you think everything is ready to go could you please commit it for me? Thanks

[avillega]

In D151273#4374565, @abrachet wrote:

It would also be nice to update the bazel build. Technically we (because we don't work in google3) aren't required to keep the bazel build clean, though Google LLVM integrators will often see a commit was broken by a @ google.com address and ask that you fix it.

In these cases, it might not be immediately obvious to the integrator how to fix this issues so it would certainly be courteous to fix it before landing. You can opt to join the bazel-build group https://reviews.llvm.org/project/profile/107/ which will add precommit checks to ensure the bazel build built cleanly.

Though feel free to ignore

I couldn't find usages of llvm-debuginfod in the bazel files, I am assuming we currently do not build llvm-debuginfod with bazel given that it is not part of the default targets.

llvm/tools/llvm-debuginfod/CMakeLists.txt

13

You are very right, I copy pasted it from another

[avillega]

Fix review comments

[mgorny]

This is causing a build failure for me, using gcc-13 on 32-bit x86:

samu: job failed: /usr/lib/ccache/bin/x86_64-pc-linux-gnu-g++ -m32 -mfpmath=sse -DCPPHTTPLIB_OPENSSL_SUPPORT -DCPPHTTPLIB_ZLIB_SUPPORT 
-D_DEBUG -D_FILE_OFFSET_BITS=64 -D_GLIBCXX_ASSERTIONS -D_GNU_SOURCE -D_LARGEFILE_SOURCE -D_LIBCPP_ENABLE_ASSERTIONS -D__STDC_CONSTANT_M
ACROS -D__STDC_FORMAT_MACROS -D__STDC_LIMIT_MACROS -I/var/tmp/portage/sys-devel/llvm-17.0.0_pre20230527/work/llvm_build-abi_x86_32.x86/
tools/llvm-debuginfod -I/var/tmp/portage/sys-devel/llvm-17.0.0_pre20230527/work/llvm/tools/llvm-debuginfod -I/var/tmp/portage/sys-devel
/llvm-17.0.0_pre20230527/work/llvm_build-abi_x86_32.x86/include -I/var/tmp/portage/sys-devel/llvm-17.0.0_pre20230527/work/llvm/include 
 -march=znver2 --param=l1-cache-size=32 --param=l1-cache-line-size=64 -O2 -pipe -frecord-gcc-switches -fPIC -fno-semantic-interposition
 -fvisibility-inlines-hidden -Werror=date-time -fno-lifetime-dse -Wall -Wextra -Wno-unused-parameter -Wwrite-strings -Wcast-qual -Wno-m
issing-field-initializers -pedantic -Wno-long-long -Wimplicit-fallthrough -Wno-maybe-uninitialized -Wno-class-memaccess -Wno-redundant-
move -Wno-pessimizing-move -Wno-noexcept-type -Wdelete-non-virtual-dtor -Wsuggest-override -Wno-comment -Wno-misleading-indentation -Wc
tad-maybe-unsupported -fdiagnostics-color -ffunction-sections -fdata-sections -std=c++17 -UNDEBUG -MD -MT tools/llvm-debuginfod/CMakeFi
les/llvm-debuginfod.dir/llvm-debuginfod.cpp.o -MF tools/llvm-debuginfod/CMakeFiles/llvm-debuginfod.dir/llvm-debuginfod.cpp.o.d -o tools
/llvm-debuginfod/CMakeFiles/llvm-debuginfod.dir/llvm-debuginfod.cpp.o -c /var/tmp/portage/sys-devel/llvm-17.0.0_pre20230527/work/llvm/t
ools/llvm-debuginfod/llvm-debuginfod.cpp
/var/tmp/portage/sys-devel/llvm-17.0.0_pre20230527/work/llvm/tools/llvm-debuginfod/llvm-debuginfod.cpp: In function ‘void parseArgs(int
, char**)’:
/var/tmp/portage/sys-devel/llvm-17.0.0_pre20230527/work/llvm/tools/llvm-debuginfod/llvm-debuginfod.cpp:113:14: error: no matching funct
ion for call to ‘parseIntArg(llvm::opt::InputArgList&, {anonymous}::ID, size_t&, long unsigned int)’
  113 |   parseIntArg(Args, OPT_max_concurrency, MaxConcurrency, 0ul);
      |   ~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/var/tmp/portage/sys-devel/llvm-17.0.0_pre20230527/work/llvm/tools/llvm-debuginfod/llvm-debuginfod.cpp:77:13: note: candidate: ‘templat
e<class T> void parseIntArg(const llvm::opt::InputArgList&, int, T&, T)’
   77 | static void parseIntArg(const opt::InputArgList &Args, int ID, T &Value,
      |             ^~~~~~~~~~~
/var/tmp/portage/sys-devel/llvm-17.0.0_pre20230527/work/llvm/tools/llvm-debuginfod/llvm-debuginfod.cpp:77:13: note:   template argument deduction/substitution failed:
/var/tmp/portage/sys-devel/llvm-17.0.0_pre20230527/work/llvm/tools/llvm-debuginfod/llvm-debuginfod.cpp:113:14: note:   deduced conflicting types for parameter ‘T’ (‘unsigned int’ and ‘long unsigned int’)
  113 |   parseIntArg(Args, OPT_max_concurrency, MaxConcurrency, 0ul);
      |   ~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
samu: subcommand failed

[thakis]

This breaks building on windows: http://45.33.8.238/win/79161/step_4.txt

Please take a look and revert for now if it takes a while to fix.

[avillega]

In D151273#4378039, @thakis wrote:

This breaks building on windows: http://45.33.8.238/win/79161/step_4.txt

Please take a look and revert for now if it takes a while to fix.

I want to revert it but I don't have commit access, if you do have commit access could you please revert on my behalf? Thanks!

[avillega]

This is a reland of https://reviews.llvm.org/D151273 which
had to be reverted because size_t type is implementation
dependant and the original change was assuming unsigned long
for the default value of one the command line arguments.

[avillega]

llvm/tools/llvm-debuginfod/llvm-debuginfod.cpp

113

In the previous version, this was using 0ul as the default value which was breaking some targets.

llvm/tools/llvm-debuginfod/CMakeLists.txt

 set(LLVM_LINK_COMPONENTS
+  Option
   Support
   )
+set(LLVM_TARGET_DEFINITIONS Opts.td)
+tablegen(LLVM Opts.inc -gen-opt-parser-defs)
+add_public_tablegen_target(DebugInfodOptsTableGen)
+
 add_llvm_tool(llvm-debuginfod
   llvm-debuginfod.cpp
+
+  DEPENDS
+  DebugInfodOptsTableGen
   )
 target_link_libraries(llvm-debuginfod PRIVATE LLVMDebuginfod)
 if(LLVM_INSTALL_BINUTILS_SYMLINKS)
   add_llvm_tool_symlink(debuginfod llvm-debuginfod)
 endif()

llvm/tools/llvm-debuginfod/Opts.td

+include "llvm/Option/OptParser.td"
+
+class F<string name, string help> : Flag<["-"], name>, HelpText<help>;
+class FF<string name, string help>: Flag<["--"], name>, HelpText<help>;
+class S<string name, string meta, string help>: Separate<["-"], name>, HelpText<help>, MetaVarName<meta>;
+
+def help : FF<"help", "Display available options">;
+def : F<"h", "Alias for --help">, Alias<help>;
+def max_concurrency :
+   S<"c", "<ulong>", "Maximum number of files to scan concurrently. "
+                     "If 0, use the hardware concurrency.">;
+def host_interface : S<"i", "<string>", "Host interface to bind to.">;
+def min_interval :
+    S<"m", "<number>", "Minimum number of seconds to wait before an on-demand update can be"
+                       "triggered by a request for a buildid which is not in the collection.">;
+def port : S<"p", "<uint>", "Port to listen on. Set to 0 to bind to any available port.">;
+def scan_interval :
+    S<"t", "<int>", "Number of seconds to wait between subsequent "
+                    "automated scans of the filesystem.">;
+def verbose_logging : F<"v", "Enable verbose logging.">;

llvm/tools/llvm-debuginfod/llvm-debuginfod.cpp

 /// This file contains the llvm-debuginfod tool, which serves the debuginfod
 /// protocol over HTTP. The tool periodically scans zero or more filesystem
 /// directories for ELF binaries to serve, and federates requests for unknown
 /// build IDs to the debuginfod servers set in the DEBUGINFOD_URLS environment
 /// variable.
 ///
 //===----------------------------------------------------------------------===//
 
+#include "llvm/ADT/StringRef.h"
 #include "llvm/Debuginfod/Debuginfod.h"
 #include "llvm/Debuginfod/HTTPClient.h"
+#include "llvm/Option/ArgList.h"
+#include "llvm/Option/Option.h"
 #include "llvm/Support/CommandLine.h"
 #include "llvm/Support/InitLLVM.h"
 #include "llvm/Support/ThreadPool.h"
 
 using namespace llvm;
 
-cl::OptionCategory DebuginfodCategory("llvm-debuginfod Options");
-
-static cl::list<std::string> ScanPaths(cl::Positional,
-                                       cl::desc("<Directories to scan>"),
-                                       cl::cat(DebuginfodCategory));
-
-static cl::opt<unsigned>
-    Port("p", cl::init(0),
-         cl::desc("Port to listen on. Set to 0 to bind to any available port."),
-         cl::cat(DebuginfodCategory));
-
-static cl::opt<std::string>
-    HostInterface("i", cl::init("0.0.0.0"),
-                  cl::desc("Host interface to bind to."),
-                  cl::cat(DebuginfodCategory));
-
-static cl::opt<int>
-    ScanInterval("t", cl::init(300),
-                 cl::desc("Number of seconds to wait between subsequent "
-                          "automated scans of the filesystem."),
-                 cl::cat(DebuginfodCategory));
-
-static cl::opt<double> MinInterval(
-    "m", cl::init(10),
-    cl::desc(
-        "Minimum number of seconds to wait before an on-demand update can be "
-        "triggered by a request for a buildid which is not in the collection."),
-    cl::cat(DebuginfodCategory));
-
-static cl::opt<size_t>
-    MaxConcurrency("c", cl::init(0),
-                   cl::desc("Maximum number of files to scan concurrently. If "
-                            "0, use the hardware concurrency."),
-                   cl::cat(DebuginfodCategory));
-
-static cl::opt<bool> VerboseLogging("v", cl::init(false),
-                                    cl::desc("Enable verbose logging."),
-                                    cl::cat(DebuginfodCategory));
+// Command-line option boilerplate.
+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)                                                    \
+  static constexpr StringLiteral NAME##_init[] = VALUE;                        \
+  static constexpr ArrayRef<StringLiteral> NAME(NAME##_init,                   \
+                                                std::size(NAME##_init) - 1);
+#include "Opts.inc"
+#undef PREFIX
+
+static constexpr 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 DebuginfodOptTable : public opt::GenericOptTable {
+public:
+  DebuginfodOptTable() : GenericOptTable(InfoTable) {}
+};
+} // end anonymous namespace
+
+// Options
+static unsigned Port;
+static std::string HostInterface;
+static int ScanInterval;
+static double MinInterval;
+static size_t MaxConcurrency;
+static bool VerboseLogging;
+static std::vector<std::string> ScanPaths;
 
 ExitOnError ExitOnErr;
 
+template <typename T>
+static void parseIntArg(const opt::InputArgList &Args, int ID, T &Value,
+                        T Default) {
+  if (const opt::Arg *A = Args.getLastArg(ID)) {
+    StringRef V(A->getValue());
+    if (!llvm::to_integer(V, Value, 0)) {
+      errs() << A->getSpelling() + ": expected an integer, but got '" + V + "'";
+      exit(1);
+    }
+  } else {
+    Value = Default;
+  }
+}
+
+static void parseArgs(int argc, char **argv) {
+  DebuginfodOptTable Tbl;
+  llvm::StringRef ToolName = argv[0];
+  llvm::BumpPtrAllocator A;
+  llvm::StringSaver Saver{A};
+  opt::InputArgList Args =
+      Tbl.parseArgs(argc, argv, OPT_UNKNOWN, Saver, [&](StringRef Msg) {
+        llvm::errs() << Msg << '\n';
+        std::exit(1);
+      });
+
+  if (Args.hasArg(OPT_help)) {
+    Tbl.printHelp(llvm::outs(),
+                  "llvm-debuginfod [options] <Directories to scan>",
+                  ToolName.str().c_str());
+    std::exit(0);
+  }
+
+  VerboseLogging = Args.hasArg(OPT_verbose_logging);
+  ScanPaths = Args.getAllArgValues(OPT_INPUT);
+
+  parseIntArg(Args, OPT_port, Port, 0u);
+  parseIntArg(Args, OPT_scan_interval, ScanInterval, 300);
+  parseIntArg(Args, OPT_max_concurrency, MaxConcurrency, size_t(0));
+
+  if (const opt::Arg *A = Args.getLastArg(OPT_min_interval)) {
+    StringRef V(A->getValue());
+    if (!llvm::to_float(V, MinInterval)) {
+      errs() << A->getSpelling() + ": expected a number, but got '" + V + "'";
+      exit(1);
+    }
+  } else {
+    MinInterval = 10.0;
+  }
+
+  HostInterface = Args.getLastArgValue(OPT_host_interface, "0.0.0.0");
+}
+
 int main(int argc, char **argv) {
   InitLLVM X(argc, argv);
   HTTPClient::initialize();
-  cl::HideUnrelatedOptions({&DebuginfodCategory});
-  cl::ParseCommandLineOptions(argc, argv);
+  parseArgs(argc, argv);
 
   SmallVector<StringRef, 1> Paths;
   for (const std::string &Path : ScanPaths)
     Paths.push_back(Path);
 
   ThreadPool Pool(hardware_concurrency(MaxConcurrency));
   DebuginfodLog Log;
   DebuginfodCollection Collection(Paths, Log, Pool, MinInterval);

llvm/utils/gn/secondary/llvm/tools/llvm-debuginfod/BUILD.gn

 import("//llvm/tools/binutils_symlinks.gni")
+import("//llvm/utils/TableGen/tablegen.gni")
 import("//llvm/utils/gn/build/symlink_or_copy.gni")
 
+tablegen("Opts") {
+  visibility = [ ":llvm-debuginfod" ]
+  args = [ "-gen-opt-parser-defs" ]
+}
+
 if (llvm_install_binutils_symlinks) {
   symlink_or_copy("debuginfod") {
     deps = [ ":llvm-debuginfod" ]
     source = "llvm-debuginfod"
     output = "$root_out_dir/bin/debuginfod"
   }
 }
 
 # //:llvm-debuginfod depends on this symlink target, see comment in //BUILD.gn.
 group("symlinks") {
   deps = [ ":llvm-debuginfod" ]
   if (llvm_install_binutils_symlinks) {
     deps += [ ":debuginfod" ]
   }
 }
 
 executable("llvm-debuginfod") {
   deps = [
+    ":Opts",
     "//llvm/lib/Debuginfod",
+    "//llvm/lib/Option",
     "//llvm/lib/Support",
   ]
   sources = [ "llvm-debuginfod.cpp" ]
 }