[llvm-dwarfdump] Print =<offset> after --debug-* options in help output.

[JDevlieghere]

Summary

Some of the --debug-* options can take an optional offset. Although the man page does a good job of making that clear it's less discoverable from the help output. Currently the only reference to this is the following sentence:

Where applicable these parameters take an optional =<offset> argument to dump only the entry at the specified offset.

This patch changes the help output from

--debug-info=              - Dump the .debug_info section

to

--debug-info[=<offset>]    - Dump the .debug_info section

rdar://problem/63150066

[JDevlieghere]

Print an error when passing a value to a flag that doesn't take one.

$ ./bin/llvm-dwarfdump --debug-names=0x0
llvm-dwarfdump: for the --debug-names option: this is a flag and does not take a value.

[aprantl]

llvm/include/llvm/BinaryFormat/Dwarf.def

927

I'm not feeling very strongly about it, but I wonder if we should burden everyone's Dwarf.def with an implementation detail of llvm-dwarfdump.

[MaskRay]

llvm/docs/CommandGuide/llvm-dwarfdump.rst

152

I think there should be no space before [=<offset>], because the user specifies --eh-frame=0, instead of --eh-frame =0

[JDevlieghere]

llvm/include/llvm/BinaryFormat/Dwarf.def

927

Yeah, I'm not super happy about it either, but on the other hand we already have the cmdline name and it's easy to ignore.

[MaskRay]

Unfortunately this is still suboptimal. What I'd like to have is

You can patch lib/Support/CommandLine.cpp:1779:

--- a/llvm/lib/Support/CommandLine.cpp
+++ b/llvm/lib/Support/CommandLine.cpp
@@ -1778,9 +1778,10 @@ void basic_parser_impl::printOptionInfo(const Option &O,
   if (!ValName.empty()) {
     if (O.getMiscFlags() & PositionalEatsArgs) {
       outs() << " <" << getValueStr(O, ValName) << ">...";
-    } else {
+    } else if (O.getValueExpectedFlag() == ValueOptional)
+      outs() << "[=<" << getValueStr(O, ValName) << ">]";
+    else
       outs() << "=<" << getValueStr(O, ValName) << '>';
-    }
   }
 
   Option::printHelpStr(O.HelpStr, GlobalWidth, getOptionWidth(O));

[JDevlieghere]

Thanks for the suggestion @MaskRay !

[MaskRay]

Generally looks good, only a couple of nits

llvm/tools/llvm-dwarfdump/llvm-dwarfdump.cpp

117

I think this code path should be impossible.

assert(!Arg.empty() && "this is a flag and does not take a value")

129

Either {} or StringRef() should work.

137

A key function is only useful if the class can be included by multiple files. The whole class is defined in the .cpp file. This should be unnecessary.

[JDevlieghere]

Address code review feedback.

[JDevlieghere]

llvm/tools/llvm-dwarfdump/llvm-dwarfdump.cpp

117

It's not, see the test case for a counter example.

[MaskRay]

LGTM.

[jhenderson]

Couple of post-commit review comments from me.

llvm/docs/CommandGuide/llvm-dwarfdump.rst

152

Unfortunately, if you take a look at the formatted output, if there is no space, the opening square brackets ends up bolded like the option name (see https://llvm.org/docs/CommandGuide/llvm-dwarfdump.html). I think you need to re-add the space before [=<offset>] to fix it (it was the only solution I found with llvm-symbolizer --functions which has a similar problem). If you can come up with an alternative solution without the space, I'd also be happy!

llvm/tools/llvm-dwarfdump/llvm-dwarfdump.cpp

92

Error messages should not end with a full stop (see recent Coding Standards updates) to be consistent with the majority of diagnostics elsewhere.

llvm/docs/CommandGuide/llvm-dwarfdump.rst

.. option:: --verify
             Verify the structure of the DWARF information by verifying the
             compile unit chains, DIE relationships graph, address
             ranges, and more.
 
 .. option:: --version
 
             Display the version of the tool.
 
-.. option:: --debug-abbrev, --debug-addr, --debug-aranges, --debug-cu-index, --debug-frame [=<offset>], --debug-gnu-pubnames, --debug-gnu-pubtypes, --debug-info [=<offset>], --debug-line [=<offset>], --debug-line-str, --debug-loc [=<offset>], --debug-loclists [=<offset>], --debug-macro, --debug-names, --debug-pubnames, --debug-pubtypes, --debug-ranges, --debug-rnglists, --debug-str, --debug-str-offsets, --debug-tu-index, --debug-types, --eh-frame [=<offset>], --gdb-index, --apple-names, --apple-types, --apple-namespaces, --apple-objc
+.. option:: --debug-abbrev, --debug-addr, --debug-aranges, --debug-cu-index, --debug-frame[=<offset>], --debug-gnu-pubnames, --debug-gnu-pubtypes, --debug-info[=<offset>], --debug-line[=<offset>], --debug-line-str, --debug-loc[=<offset>], --debug-loclists[=<offset>], --debug-macro, --debug-names, --debug-pubnames, --debug-pubtypes, --debug-ranges, --debug-rnglists, --debug-str, --debug-str-offsets, --debug-tu-index, --debug-types[=<offset>], --eh-frame[=<offset>], --gdb-index, --apple-names, --apple-types, --apple-namespaces, --apple-objc
 
             Dump the specified DWARF section by name. Only the
             `.debug_info` section is shown by default. Some entries
             support adding an `=<offset>` as a way to provide an
             optional offset of the exact entry to dump within the
             respective section. When an offset is provided, only the
             entry at that offset will be dumped, else the entire
             section will be dumped.

llvm/include/llvm/BinaryFormat/Dwarf.def

 #define HANDLE_DW_APPLE_PROPERTY(ID, NAME)
 #endif
 
 #ifndef HANDLE_DW_UT
 #define HANDLE_DW_UT(ID, NAME)
 #endif
 
 #ifndef HANDLE_DWARF_SECTION
-#define HANDLE_DWARF_SECTION(ENUM_NAME, ELF_NAME, CMDLINE_NAME)
+#define HANDLE_DWARF_SECTION(ENUM_NAME, ELF_NAME, CMDLINE_NAME, OPTION)
 #endif
 
 #ifndef HANDLE_DW_IDX
 #define HANDLE_DW_IDX(ID, NAME)
 #endif
 
 #ifndef HANDLE_DW_END
 #define HANDLE_DW_END(ID, NAME)

 // DWARF v5 Unit Types.
 HANDLE_DW_UT(0x01, compile)
 HANDLE_DW_UT(0x02, type)
 HANDLE_DW_UT(0x03, partial)
 HANDLE_DW_UT(0x04, skeleton)
 HANDLE_DW_UT(0x05, split_compile)
 HANDLE_DW_UT(0x06, split_type)
 
-// DWARF section types. (enum name, ELF name, ELF DWO name, cmdline name)
+// DWARF section types. (enum name, ELF name, ELF DWO name, cmdline name, option)
 // Note that these IDs don't mean anything.
 // TODO: Add Mach-O and COFF names.
 // Official DWARF sections.
-HANDLE_DWARF_SECTION(DebugAbbrev, ".debug_abbrev", "debug-abbrev")
-HANDLE_DWARF_SECTION(DebugAddr, ".debug_addr", "debug-addr")
-HANDLE_DWARF_SECTION(DebugAranges, ".debug_aranges", "debug-aranges")
-HANDLE_DWARF_SECTION(DebugInfo, ".debug_info", "debug-info")
-HANDLE_DWARF_SECTION(DebugTypes, ".debug_types", "debug-types")
-HANDLE_DWARF_SECTION(DebugLine, ".debug_line", "debug-line")
-HANDLE_DWARF_SECTION(DebugLineStr, ".debug_line_str", "debug-line-str")
-HANDLE_DWARF_SECTION(DebugLoc, ".debug_loc", "debug-loc")
-HANDLE_DWARF_SECTION(DebugLoclists, ".debug_loclists", "debug-loclists")
-HANDLE_DWARF_SECTION(DebugFrame, ".debug_frame", "debug-frame")
-HANDLE_DWARF_SECTION(DebugMacro, ".debug_macro", "debug-macro")
-HANDLE_DWARF_SECTION(DebugNames, ".debug_names", "debug-names")
-HANDLE_DWARF_SECTION(DebugPubnames, ".debug_pubnames", "debug-pubnames")
-HANDLE_DWARF_SECTION(DebugPubtypes, ".debug_pubtypes", "debug-pubtypes")
-HANDLE_DWARF_SECTION(DebugGnuPubnames, ".debug_gnu_pubnames", "debug-gnu-pubnames")
-HANDLE_DWARF_SECTION(DebugGnuPubtypes, ".debug_gnu_pubtypes", "debug-gnu-pubtypes")
-HANDLE_DWARF_SECTION(DebugRanges, ".debug_ranges", "debug-ranges")
-HANDLE_DWARF_SECTION(DebugRnglists, ".debug_rnglists", "debug-rnglists")
-HANDLE_DWARF_SECTION(DebugStr, ".debug_str", "debug-str")
-HANDLE_DWARF_SECTION(DebugStrOffsets, ".debug_str_offsets", "debug-str-offsets")
-HANDLE_DWARF_SECTION(DebugCUIndex, ".debug_cu_index", "debug-cu-index")
-HANDLE_DWARF_SECTION(DebugTUIndex, ".debug_tu_index", "debug-tu-index")
+HANDLE_DWARF_SECTION(DebugAbbrev, ".debug_abbrev", "debug-abbrev", BoolOption)
+HANDLE_DWARF_SECTION(DebugAddr, ".debug_addr", "debug-addr", BoolOption)
+HANDLE_DWARF_SECTION(DebugAranges, ".debug_aranges", "debug-aranges", BoolOption)
+HANDLE_DWARF_SECTION(DebugInfo, ".debug_info", "debug-info", OffsetOption)
+HANDLE_DWARF_SECTION(DebugTypes, ".debug_types", "debug-types", OffsetOption)
+HANDLE_DWARF_SECTION(DebugLine, ".debug_line", "debug-line", OffsetOption)
+HANDLE_DWARF_SECTION(DebugLineStr, ".debug_line_str", "debug-line-str", BoolOption)
+HANDLE_DWARF_SECTION(DebugLoc, ".debug_loc", "debug-loc", OffsetOption)
+HANDLE_DWARF_SECTION(DebugLoclists, ".debug_loclists", "debug-loclists", OffsetOption)
+HANDLE_DWARF_SECTION(DebugFrame, ".debug_frame", "debug-frame", OffsetOption)
+HANDLE_DWARF_SECTION(DebugMacro, ".debug_macro", "debug-macro", BoolOption)
+HANDLE_DWARF_SECTION(DebugNames, ".debug_names", "debug-names", BoolOption)
+HANDLE_DWARF_SECTION(DebugPubnames, ".debug_pubnames", "debug-pubnames", BoolOption)
+HANDLE_DWARF_SECTION(DebugPubtypes, ".debug_pubtypes", "debug-pubtypes", BoolOption)
+HANDLE_DWARF_SECTION(DebugGnuPubnames, ".debug_gnu_pubnames", "debug-gnu-pubnames", BoolOption)
+HANDLE_DWARF_SECTION(DebugGnuPubtypes, ".debug_gnu_pubtypes", "debug-gnu-pubtypes", BoolOption)
+HANDLE_DWARF_SECTION(DebugRanges, ".debug_ranges", "debug-ranges", BoolOption)
+HANDLE_DWARF_SECTION(DebugRnglists, ".debug_rnglists", "debug-rnglists", BoolOption)
+HANDLE_DWARF_SECTION(DebugStr, ".debug_str", "debug-str", BoolOption)
+HANDLE_DWARF_SECTION(DebugStrOffsets, ".debug_str_offsets", "debug-str-offsets", BoolOption)
+HANDLE_DWARF_SECTION(DebugCUIndex, ".debug_cu_index", "debug-cu-index", BoolOption)
+HANDLE_DWARF_SECTION(DebugTUIndex, ".debug_tu_index", "debug-tu-index", BoolOption)
 // Vendor extensions.
-HANDLE_DWARF_SECTION(AppleNames, ".apple_names", "apple-names")
-HANDLE_DWARF_SECTION(AppleTypes, ".apple_types", "apple-types")
-HANDLE_DWARF_SECTION(AppleNamespaces, ".apple_namespaces", "apple-namespaces")
-HANDLE_DWARF_SECTION(AppleObjC, ".apple_objc", "apple-objc")
-HANDLE_DWARF_SECTION(GdbIndex, ".gdb_index", "gdb-index")
+HANDLE_DWARF_SECTION(AppleNames, ".apple_names", "apple-names", BoolOption)
+HANDLE_DWARF_SECTION(AppleTypes, ".apple_types", "apple-types", BoolOption)
+HANDLE_DWARF_SECTION(AppleNamespaces, ".apple_namespaces", "apple-namespaces", BoolOption)
+HANDLE_DWARF_SECTION(AppleObjC, ".apple_objc", "apple-objc", BoolOption)
+HANDLE_DWARF_SECTION(GdbIndex, ".gdb_index", "gdb-index", BoolOption)
 
 HANDLE_DW_IDX(0x01, compile_unit)
 HANDLE_DW_IDX(0x02, type_unit)
 HANDLE_DW_IDX(0x03, die_offset)
 HANDLE_DW_IDX(0x04, parent)
 HANDLE_DW_IDX(0x05, type_hash)
 
 // DWARF package file section identifiers.

llvm/include/llvm/DebugInfo/DIContext.h

struct DILineInfoSpecifier {
 
   DILineInfoSpecifier(FileLineInfoKind FLIKind = FileLineInfoKind::RawValue,
                       FunctionNameKind FNKind = FunctionNameKind::None)
       : FLIKind(FLIKind), FNKind(FNKind) {}
 };
 
 /// This is just a helper to programmatically construct DIDumpType.
 enum DIDumpTypeCounter {
-#define HANDLE_DWARF_SECTION(ENUM_NAME, ELF_NAME, CMDLINE_NAME) \
+#define HANDLE_DWARF_SECTION(ENUM_NAME, ELF_NAME, CMDLINE_NAME, OPTION)        \
   DIDT_ID_##ENUM_NAME,
 #include "llvm/BinaryFormat/Dwarf.def"
 #undef HANDLE_DWARF_SECTION
   DIDT_ID_UUID,
   DIDT_ID_Count
 };
 static_assert(DIDT_ID_Count <= 32, "section types overflow storage");
 
 /// Selects which debug sections get dumped.
 enum DIDumpType : unsigned {
   DIDT_Null,
   DIDT_All             = ~0U,
-#define HANDLE_DWARF_SECTION(ENUM_NAME, ELF_NAME, CMDLINE_NAME) \
+#define HANDLE_DWARF_SECTION(ENUM_NAME, ELF_NAME, CMDLINE_NAME, OPTION)        \
   DIDT_##ENUM_NAME = 1U << DIDT_ID_##ENUM_NAME,
 #include "llvm/BinaryFormat/Dwarf.def"
 #undef HANDLE_DWARF_SECTION
   DIDT_UUID = 1 << DIDT_ID_UUID,
 };
 
 /// Container for dump options that control which debug information will be
 /// dumped.

llvm/lib/Support/CommandLine.cpp

 void basic_parser_impl::printOptionInfo(const Option &O,
                                         size_t GlobalWidth) const {
   outs() << PrintArg(O.ArgStr);
 
   auto ValName = getValueName();
   if (!ValName.empty()) {
     if (O.getMiscFlags() & PositionalEatsArgs) {
       outs() << " <" << getValueStr(O, ValName) << ">...";
-    } else {
+    } else if (O.getValueExpectedFlag() == ValueOptional)
+      outs() << "[=<" << getValueStr(O, ValName) << ">]";
+    else
       outs() << "=<" << getValueStr(O, ValName) << '>';
-    }
-  }
 
   Option::printHelpStr(O.HelpStr, GlobalWidth, getOptionWidth(O));
 }
 
 void basic_parser_impl::printOptionName(const Option &O,
                                         size_t GlobalWidth) const {
   outs() << PrintArg(O.ArgStr);
   outs().indent(GlobalWidth - O.ArgStr.size());

llvm/test/tools/llvm-dwarfdump/cmdline.test

 RUN: llvm-dwarfdump -h 2>&1 | FileCheck --check-prefix=HELP %s --implicit-check-not=out-file
 RUN: llvm-dwarfdump --help 2>&1 | FileCheck --check-prefix=HELP %s --implicit-check-not=out-file
 HELP: USAGE: llvm-dwarfdump{{[^ ]*}} [options] <input object files or .dSYM bundles>
 HELP: Color Options
 HELP: --color
 HELP: Generic Options
 HELP: --help
 HELP: --help-list
 HELP: --version
 HELP: Section-specific Dump Options
-HELP: --debug-info            - Dump the .debug_info section
-HELP: --eh-frame
+HELP: --debug-info[=<offset>]    - Dump the .debug_info section
+HELP: --debug-names              - Dump the .debug_names section
+HELP: --eh-frame                 - Alias for -debug-frame
 HELP: Specific Options
 HELP: --diff
 HELP: --find
 HELP: --ignore-case
 HELP: --lookup
 HELP: --name
 HELP: -o=<filename>
 HELP: --parent-recurse-depth=<N>
 HELP: --recurse-depth=<N>
 HELP: --regex
 HELP: --show-children
 HELP: --show-form
 HELP: --show-parents
 HELP: --statistics
 HELP: --summarize-types
 HELP: @FILE
 
 RUN: llvm-dwarfdump --version 2>&1 | FileCheck --check-prefix=VERSION %s
 VERSION: {{ version }}
 
 RUN: llvm-dwarfdump -diff -verbose 2>&1 | FileCheck --check-prefix=INCOMPATIBLE %s
 INCOMPATIBLE: error: incompatible arguments: specifying both -diff and -verbose is currently not supported
+
+RUN: not llvm-dwarfdump --debug-names=0x0 2>&1 | FileCheck --check-prefix=FLAG %s
+FLAG: for the --debug-names option: this is a flag and does not take a value.

llvm/tools/llvm-dwarfdump/llvm-dwarfdump.cpp

 namespace {
 /// Parser for options that take an optional offest argument.
 /// @{
 struct OffsetOption {
   uint64_t Val = 0;
   bool HasValue = false;
   bool IsRequested = false;
 };
+struct BoolOption : public OffsetOption {};
 } // namespace
 
 namespace llvm {
 namespace cl {
 template <>
 class parser<OffsetOption> final : public basic_parser<OffsetOption> {
 public:
   parser(Option &O) : basic_parser(O) {}
bool parse(Option &O, StringRef ArgName, StringRef Arg, OffsetOption &Val) {
     Val.IsRequested = true;
     return false;
   }
 
   enum ValueExpected getValueExpectedFlagDefault() const {
     return ValueOptional;
   }
 
-  StringRef getValueName() const override { return StringRef(); }
+  StringRef getValueName() const override { return StringRef("offset"); }
 
   void printOptionDiff(const Option &O, OffsetOption V, OptVal Default,
                        size_t GlobalWidth) const {
     printOptionName(O, GlobalWidth);
     outs() << "[=offset]";
   }
+};
+
+template <> class parser<BoolOption> final : public basic_parser<BoolOption> {
+public:
+  parser(Option &O) : basic_parser(O) {}
+
+  /// Return true on error.
+  bool parse(Option &O, StringRef ArgName, StringRef Arg, BoolOption &Val) {
+    if (Arg != "")
+      return O.error("this is a flag and does not take a value.");
+    Val.Val = 0;
+    Val.HasValue = false;
+    Val.IsRequested = true;
+    return false;
+  }
+
+  enum ValueExpected getValueExpectedFlagDefault() const {
+    return ValueOptional;
+  }
 
-  // An out-of-line virtual method to provide a 'home' for this class.
-  void anchor() override {};
+  StringRef getValueName() const override { return StringRef(); }
+
+  void printOptionDiff(const Option &O, OffsetOption V, OptVal Default,
+                       size_t GlobalWidth) const {
+    printOptionName(O, GlobalWidth);
+  }
 };
 } // namespace cl
 } // namespace llvm
 
 /// @}
 /// Command line options.
 /// @{
 
 namespace {
 using namespace cl;
 
 OptionCategory DwarfDumpCategory("Specific Options");
 static list<std::string>
     InputFilenames(Positional, desc("<input object files or .dSYM bundles>"),
                    ZeroOrMore, cat(DwarfDumpCategory));
 
 cl::OptionCategory SectionCategory("Section-specific Dump Options",
                                    "These control which sections are dumped. "
                                    "Where applicable these parameters take an "
                                    "optional =<offset> argument to dump only "
                                    "the entry at the specified offset.");
 
 static opt<bool> DumpAll("all", desc("Dump all debug info sections"),
                          cat(SectionCategory));
 static alias DumpAllAlias("a", desc("Alias for -all"), aliasopt(DumpAll));
 
 // Options for dumping specific sections.
 static unsigned DumpType = DIDT_Null;
 static std::array<llvm::Optional<uint64_t>, (unsigned)DIDT_ID_Count>
     DumpOffsets;
-#define HANDLE_DWARF_SECTION(ENUM_NAME, ELF_NAME, CMDLINE_NAME)                \
-  static opt<OffsetOption> Dump##ENUM_NAME(                                    \
-      CMDLINE_NAME, desc("Dump the " ELF_NAME " section"),                     \
+#define HANDLE_DWARF_SECTION(ENUM_NAME, ELF_NAME, CMDLINE_NAME, OPTION)        \
+  static opt<OPTION> Dump##ENUM_NAME(CMDLINE_NAME,                             \
+                                     desc("Dump the " ELF_NAME " section"),    \
                                      cat(SectionCategory));
 #include "llvm/BinaryFormat/Dwarf.def"
 #undef HANDLE_DWARF_SECTION
 
 static alias DumpDebugFrameAlias("eh-frame", desc("Alias for -debug-frame"),
                                  NotHidden, cat(SectionCategory),
                                  aliasopt(DumpDebugFrame));
 static list<std::string>
     ArchFilters("arch",

 
 static void error(StringRef Prefix, std::error_code EC) {
   if (!EC)
     return;
   WithColor::error() << Prefix << ": " << EC.message() << "\n";
   exit(1);
 }
 
-static DIDumpOptions getDumpOpts(DWARFContext& C) { 
+static DIDumpOptions getDumpOpts(DWARFContext &C) {
   DIDumpOptions DumpOpts;
   DumpOpts.DumpType = DumpType;
   DumpOpts.ChildRecurseDepth = ChildRecurseDepth;
   DumpOpts.ParentRecurseDepth = ParentRecurseDepth;
   DumpOpts.ShowAddresses = !Diff;
   DumpOpts.ShowChildren = ShowChildren;
   DumpOpts.ShowParents = ShowParents;
   DumpOpts.ShowForm = ShowForm;
int main(int argc, char **argv) {
   error("Unable to open output file" + OutputFilename, EC);
   // Don't remove output file if we exit with an error.
   OutputFile.keep();
 
   bool OffsetRequested = false;
 
   // Defaults to dumping all sections, unless brief mode is specified in which
   // case only the .debug_info section in dumped.
-#define HANDLE_DWARF_SECTION(ENUM_NAME, ELF_NAME, CMDLINE_NAME)                \
+#define HANDLE_DWARF_SECTION(ENUM_NAME, ELF_NAME, CMDLINE_NAME, OPTION)        \
   if (Dump##ENUM_NAME.IsRequested) {                                           \
     DumpType |= DIDT_##ENUM_NAME;                                              \
     if (Dump##ENUM_NAME.HasValue) {                                            \
       DumpOffsets[DIDT_ID_##ENUM_NAME] = Dump##ENUM_NAME.Val;                  \
       OffsetRequested = true;                                                  \
     }                                                                          \
   }
 #include "llvm/BinaryFormat/Dwarf.def"