[Lldb-commits] [PATCH] D68361: [dsymutil] Tablegenify option parsing

[Jonas Devlieghere via Phabricator via lldb-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL373622: [dsymutil] Tablegenify option parsing (authored by 
JDevlieghere, committed by ).

Changed prior to commit:
  https://reviews.llvm.org/D68361?vs=222934=223042#toc

Repository:
  rL LLVM

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

https://reviews.llvm.org/D68361

Files:
  llvm/trunk/test/tools/dsymutil/cmdline.test
  llvm/trunk/tools/dsymutil/CMakeLists.txt
  llvm/trunk/tools/dsymutil/Options.td
  llvm/trunk/tools/dsymutil/dsymutil.cpp

Index: llvm/trunk/test/tools/dsymutil/cmdline.test
===
--- llvm/trunk/test/tools/dsymutil/cmdline.test
+++ llvm/trunk/test/tools/dsymutil/cmdline.test
@@ -1,21 +1,19 @@
 RUN: dsymutil -help 2>&1 | FileCheck --check-prefix=HELP %s
 HELP: OVERVIEW: manipulate archived DWARF debug symbol files.
-HELP: USAGE: dsymutil{{[^ ]*}} [options] 
+HELP: USAGE: {{.*}}dsymutil{{[^ ]*}} [options] 
 HELP-NOT: -reverse-iterate
-HELP: Color Options
-HELP: -color
-HELP: Specific Options:
+HELP: Dsymutil Options:
 HELP: -accelerator
-HELP: -arch=
+HELP: -arch 
 HELP: -dump-debug-map
 HELP: -flat
 HELP: -minimize
 HELP: -no-odr
 HELP: -no-output
 HELP: -no-swiftmodule-timestamp
-HELP: -num-threads=
-HELP: -o=
-HELP: -oso-prepend-path=
+HELP: -num-threads 
+HELP: -oso-prepend-path 
+HELP: -o 
 HELP: -papertrail
 HELP: -symbol-map
 HELP: -symtab
Index: llvm/trunk/tools/dsymutil/Options.td
===
--- llvm/trunk/tools/dsymutil/Options.td
+++ llvm/trunk/tools/dsymutil/Options.td
@@ -0,0 +1,146 @@
+include "llvm/Option/OptParser.td"
+
+class F: Flag<["--", "-"], name>;
+
+def grp_general : OptionGroup<"Dsymutil">, HelpText<"Dsymutil Options">;
+
+def help: F<"help">,
+  HelpText<"Prints this help output.">,
+  Group;
+def: Flag<["-"], "h">,
+  Alias,
+  HelpText<"Alias for --help">,
+  Group;
+
+def version: F<"version">,
+  HelpText<"Prints the dsymutil version.">,
+  Group;
+def: Flag<["-"], "v">,
+  Alias,
+  HelpText<"Alias for --version">,
+  Group;
+
+def verbose: F<"verbose">,
+  HelpText<"Enable verbose mode.">,
+  Group;
+
+def verify: F<"verify">,
+  HelpText<"Run the DWARF verifier on the linked DWARF debug info.">,
+  Group;
+
+def no_output: F<"no-output">,
+  HelpText<"Do the link in memory, but do not emit the result file.">,
+  Group;
+
+def no_swiftmodule_timestamp: F<"no-swiftmodule-timestamp">,
+  HelpText<"Don't check timestamp for swiftmodule files.">,
+  Group;
+
+def no_odr: F<"no-odr">,
+  HelpText<"Do not use ODR (One Definition Rule) for type uniquing.">,
+  Group;
+
+def dump_debug_map: F<"dump-debug-map">,
+  HelpText<"Parse and dump the debug map to standard output. Not DWARF link will take place.">,
+  Group;
+
+def yaml_input: F<"y">,
+  HelpText<"Treat the input file is a YAML debug map rather than a binary.">,
+  Group;
+
+def papertrail: F<"papertrail">,
+  HelpText<"Embed warnings in the linked DWARF debug info.">,
+  Group;
+
+def assembly: F<"S">,
+  HelpText<"Output textual assembly instead of a binary dSYM companion file.">,
+  Group;
+
+def symtab: F<"symtab">,
+  HelpText<"Dumps the symbol table found in executable or object file(s) and exits.">,
+  Group;
+def: Flag<["-"], "s">,
+  Alias,
+  HelpText<"Alias for --symtab">,
+  Group;
+
+def flat: F<"flat">,
+  HelpText<"Produce a flat dSYM file (not a bundle).">,
+  Group;
+def: Flag<["-"], "f">,
+  Alias,
+  HelpText<"Alias for --flat">,
+  Group;
+
+def minimize: F<"minimize">,
+  HelpText<"When used when creating a dSYM file with Apple accelerator tables, "
+   "this option will suppress the emission of the .debug_inlines, "
+   ".debug_pubnames, and .debug_pubtypes sections since dsymutil "
+   "has better equivalents: .apple_names and .apple_types. When used in "
+   "conjunction with --update option, this option will cause redundant "
+   "accelerator tables to be removed.">,
+  Group;
+def: Flag<["-"], "z">,
+  Alias,
+  HelpText<"Alias for --minimize">,
+  Group;
+
+def update: F<"update">,
+  HelpText<"Updates existing dSYM files to contain the latest accelerator tables and other DWARF optimizations.">,
+  Group;
+def: Flag<["-"], "u">,
+  Alias,
+  HelpText<"Alias for --update">,
+  Group;
+
+def output: Separate<["--", "-"], "o">,
+  MetaVarName<"">,
+  HelpText<"Specify the output file. Defaults to .dwarf">,
+  Group;
+def: Separate<["-"], "out">,
+  Alias,
+  HelpText<"Alias for --o">,
+  Group;
+def: Joined<["--", "-"], "out=">, Alias;
+def: Joined<["--", "-"], "o=">, Alias;
+
+def oso_prepend_path: Separate<["--", "-"], "oso-prepend-path">,
+  MetaVarName<"">,
+  HelpText<"Specify a directory to prepend to the paths of object files.">,
+  Group;
+def: Joined<["--", "-"], "oso-prepend-path=">, Alias;
+
+def symbolmap: Separate<["--", "-"], "symbol-map">,
+  MetaVarName<"">,
+  HelpText<"Updates the existing dSYMs 

[Lldb-commits] [PATCH] D68361: [dsymutil] Tablegenify option parsing

[Jonas Devlieghere via Phabricator via lldb-commits]

JDevlieghere added a comment.

In D68361#1692988 , @thegameg wrote:

> I noticed a bunch of explicit `llvm::` prefixes like `llvm::Error`, 
> `llvm::StringRef`, etc. Did you intentionally leave that there?


No, that's definitely unintentional. I'll fix the inconsistency in a follow-up 
commit! :-)


Repository:
  rLLDB LLDB

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

https://reviews.llvm.org/D68361



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D68361: [dsymutil] Tablegenify option parsing

[Frederic Riss via Phabricator via lldb-commits]

friss accepted this revision.
friss added a comment.

This seems a little heavyweight, but it reads nicely and gets rid of global 
state. Go for it.


Repository:
  rLLDB LLDB

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

https://reviews.llvm.org/D68361



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D68361: [dsymutil] Tablegenify option parsing

[Francis Visoiu Mistrih via Phabricator via lldb-commits]

thegameg accepted this revision.
thegameg added a comment.
This revision is now accepted and ready to land.

I noticed a bunch of explicit `llvm::` prefixes like `llvm::Error`, 
`llvm::StringRef`, etc. Did you intentionally leave that there?

Otherwise, this LGTM, thanks for the nice cleanup!




Comment at: llvm/tools/dsymutil/dsymutil.cpp:161
+"standard input cannot be used as input for a dSYM update.",
+inconvertibleErrorCode());
+  }

Would it make sense for all these error codes to be 
`std::errc::invalid_argument`?



Comment at: llvm/tools/dsymutil/dsymutil.cpp:233
+  getInputs(Args, Options.LinkOptions.Update)) {
+Options.InputFiles = *InputFiles;
+  } else {

`=  std::move(*InputFiles);`?


Repository:
  rLLDB LLDB

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

https://reviews.llvm.org/D68361



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D68361: [dsymutil] Tablegenify option parsing

[Jonas Devlieghere via Phabricator via lldb-commits]

JDevlieghere created this revision.
JDevlieghere added reviewers: friss, aprantl, thegameg.
Herald added subscribers: llvm-commits, mgorny.
Herald added projects: LLDB, LLVM.

This patch reimplements command line option parsing in dsymutil with Tablegen 
and libOption. The main motivation for this change is to prevent clashes with 
other cl::opt options defined in llvm. Although it's a bit more heavyweight, it 
has some nice advantages such as no global static initializers and better 
separation between the code and the option definitions.

I also used this opportunity to improve how dsymutil deals with incompatible 
options. Instead of having checks spread across the code, everything is now 
grouped together in `verifyOptions`. The fact that the options are no longer 
global means that we need to pass them around a bit more, but I think it's 
worth the tradeoff.


Repository:
  rLLDB LLDB

https://reviews.llvm.org/D68361

Files:
  llvm/test/tools/dsymutil/cmdline.test
  llvm/tools/dsymutil/CMakeLists.txt
  llvm/tools/dsymutil/Options.td
  llvm/tools/dsymutil/dsymutil.cpp

Index: llvm/tools/dsymutil/dsymutil.cpp
===
--- llvm/tools/dsymutil/dsymutil.cpp
+++ llvm/tools/dsymutil/dsymutil.cpp
@@ -20,12 +20,16 @@
 #include "llvm/ADT/SmallVector.h"
 #include "llvm/ADT/StringExtras.h"
 #include "llvm/ADT/StringRef.h"
+#include "llvm/ADT/StringSwitch.h"
 #include "llvm/ADT/Triple.h"
 #include "llvm/DebugInfo/DIContext.h"
 #include "llvm/DebugInfo/DWARF/DWARFContext.h"
 #include "llvm/DebugInfo/DWARF/DWARFVerifier.h"
 #include "llvm/Object/Binary.h"
 #include "llvm/Object/MachO.h"
+#include "llvm/Option/Arg.h"
+#include "llvm/Option/ArgList.h"
+#include "llvm/Option/Option.h"
 #include "llvm/Support/CommandLine.h"
 #include "llvm/Support/FileSystem.h"
 #include "llvm/Support/InitLLVM.h"
@@ -43,137 +47,227 @@
 #include 
 
 using namespace llvm;
-using namespace llvm::cl;
 using namespace llvm::dsymutil;
 using namespace object;
 
-static OptionCategory DsymCategory("Specific Options");
-static opt Help("h", desc("Alias for -help"), Hidden);
-static opt Version("v", desc("Alias for -version"), Hidden);
-
-static list InputFiles(Positional, OneOrMore,
-desc(""), cat(DsymCategory));
-
-static opt
-OutputFileOpt("o",
-  desc("Specify the output file. default: .dwarf"),
-  value_desc("filename"), cat(DsymCategory));
-static alias OutputFileOptA("out", desc("Alias for -o"),
-aliasopt(OutputFileOpt));
-
-static opt OsoPrependPath(
-"oso-prepend-path",
-desc("Specify a directory to prepend to the paths of object files."),
-value_desc("path"), cat(DsymCategory));
-
-static opt Assembly(
-"S",
-desc("Output textual assembly instead of a binary dSYM companion file."),
-init(false), cat(DsymCategory), cl::Hidden);
-
-static opt DumpStab(
-"symtab",
-desc("Dumps the symbol table found in executable or object file(s) and\n"
- "exits."),
-init(false), cat(DsymCategory));
-static alias DumpStabA("s", desc("Alias for --symtab"), aliasopt(DumpStab));
-
-static opt FlatOut("flat",
- desc("Produce a flat dSYM file (not a bundle)."),
- init(false), cat(DsymCategory));
-static alias FlatOutA("f", desc("Alias for --flat"), aliasopt(FlatOut));
-
-static opt Minimize(
-"minimize",
-desc("When used when creating a dSYM file with Apple accelerator tables,\n"
- "this option will suppress the emission of the .debug_inlines, \n"
- ".debug_pubnames, and .debug_pubtypes sections since dsymutil \n"
- "has better equivalents: .apple_names and .apple_types. When used in\n"
- "conjunction with --update option, this option will cause redundant\n"
- "accelerator tables to be removed."),
-init(false), cat(DsymCategory));
-static alias MinimizeA("z", desc("Alias for --minimize"), aliasopt(Minimize));
-
-static opt Update(
-"update",
-desc("Updates existing dSYM files to contain the latest accelerator\n"
- "tables and other DWARF optimizations."),
-init(false), cat(DsymCategory));
-static alias UpdateA("u", desc("Alias for --update"), aliasopt(Update));
-
-static opt SymbolMap(
-"symbol-map",
-desc("Updates the existing dSYMs inplace using symbol map specified."),
-value_desc("bcsymbolmap"), cat(DsymCategory));
-
-static cl::opt AcceleratorTable(
-"accelerator", cl::desc("Output accelerator tables."),
-cl::values(clEnumValN(AccelTableKind::Default, "Default",
-  "Default for input."),
-   clEnumValN(AccelTableKind::Apple, "Apple", "Apple"),
-   clEnumValN(AccelTableKind::Dwarf, "Dwarf", "DWARF")),
-cl::init(AccelTableKind::Default), cat(DsymCategory));
-
-static opt NumThreads(
-"num-threads",
-desc("Specifies the maximum number (n) of simultaneous