[llvm-dwarfdump] Remove unnecessary explicit -h behaviour

[jhenderson]

Summary

--help and -h are automatically supported by the command-line parser, unless overridden by the tool. The behaviour of the PrintHelpMessage being used for -h prior to this patch is subtly different to that provided by --help automatically (it omits certain elements of help text and options, such as --help-list), so overriding the default is not desirable, without good reason. This patch removes the explicit specification of -h and its behaviour, so that the default behaviour is used.

[hintonda]

LGTM, but you'll also want to fix llvm/test/Support/check-default-options.txt. It checks your special handling to make sure the default -h support didn't break it.

[jhenderson]

In D63565#1551983, @hintonda wrote:

LGTM, but you'll also want to fix llvm/test/Support/check-default-options.txt. It checks your special handling to make sure the default -h support didn't break it.

Thanks, I wasn't aware of that one (and hadn't yet bothered running every lit test for this). Just to be clear, are you recommending that I just delete the bits of that test relating to llvm-dwarfdump, since it will now follow the default process, or do you think there should be something else to replace it? I can replace it with llvm-opt-report, which seems to do something similar to llvm-dwarfdump.

[hintonda]

In D63565#1552004, @jhenderson wrote:

In D63565#1551983, @hintonda wrote:

LGTM, but you'll also want to fix llvm/test/Support/check-default-options.txt. It checks your special handling to make sure the default -h support didn't break it.

Thanks, I wasn't aware of that one (and hadn't yet bothered running every lit test for this). Just to be clear, are you recommending that I just delete the bits of that test relating to llvm-dwarfdump, since it will now follow the default process, or do you think there should be something else to replace it? I can replace it with llvm-opt-report, which seems to do something similar to llvm-dwarfdump.

Yes, it can be removed. If you want, you can add it to the ones above that do use the new default -h, but I don't think that's necessary. And yes, if there's another one that does something special, adding it, i.e., s/llvm-dwarfdump/llvm-opt-report/ makes sense.

[jhenderson]

In D63565#1552064, @hintonda wrote:

In D63565#1552004, @jhenderson wrote:

In D63565#1551983, @hintonda wrote:

LGTM, but you'll also want to fix llvm/test/Support/check-default-options.txt. It checks your special handling to make sure the default -h support didn't break it.

Thanks, I wasn't aware of that one (and hadn't yet bothered running every lit test for this). Just to be clear, are you recommending that I just delete the bits of that test relating to llvm-dwarfdump, since it will now follow the default process, or do you think there should be something else to replace it? I can replace it with llvm-opt-report, which seems to do something similar to llvm-dwarfdump.

Yes, it can be removed. If you want, you can add it to the ones above that do use the new default -h, but I don't think that's necessary. And yes, if there's another one that does something special, adding it, i.e., s/llvm-dwarfdump/llvm-opt-report/ makes sense.

Okay, I'm going to remove it and then commit, without replacing the removed test part. My initial evaluation was using an out-of-date llvm-opt-report, and I was unable to find any other suitable candidates. I reviewed the other tools, and the only tools I could find with explicit -h help handling are dsymutil and clang-offload-bundler. In both cases, I think the existing handling is a bug (in dsymutil at least, you actually have to provide a positional argument before it will process '-h'!).

[jhenderson]

Removed references to llvm-dwarfdump from test/Support/check-default-options.txt.

[hintonda]

In D63565#1553263, @jhenderson wrote:

In D63565#1552064, @hintonda wrote:

In D63565#1552004, @jhenderson wrote:

In D63565#1551983, @hintonda wrote:

LGTM, but you'll also want to fix llvm/test/Support/check-default-options.txt. It checks your special handling to make sure the default -h support didn't break it.

Thanks, I wasn't aware of that one (and hadn't yet bothered running every lit test for this). Just to be clear, are you recommending that I just delete the bits of that test relating to llvm-dwarfdump, since it will now follow the default process, or do you think there should be something else to replace it? I can replace it with llvm-opt-report, which seems to do something similar to llvm-dwarfdump.

Yes, it can be removed. If you want, you can add it to the ones above that do use the new default -h, but I don't think that's necessary. And yes, if there's another one that does something special, adding it, i.e., s/llvm-dwarfdump/llvm-opt-report/ makes sense.

Okay, I'm going to remove it and then commit, without replacing the removed test part. My initial evaluation was using an out-of-date llvm-opt-report, and I was unable to find any other suitable candidates. I reviewed the other tools, and the only tools I could find with explicit -h help handling are dsymutil and clang-offload-bundler. In both cases, I think the existing handling is a bug (in dsymutil at least, you actually have to provide a positional argument before it will process '-h'!).

That's because they're processing -h after cl::ParseCommandLineOptions returns. But as you say, InputFiles requires at least one argument, so cl::ParseCommandLineOptions never does return if this condition isn't met. The only way to fix that is to remove the special -h processing.

static list<std::string> InputFiles(Positional, OneOrMore,
                                    desc("<input files>"), cat(DsymCategory));

test/Support/check-default-options.txt

 # RUN: llvm-objdump --help-hidden %t | FileCheck --check-prefix=CHECK-OBJDUMP %s
 # RUN: llvm-readobj --help-hidden %t | FileCheck --check-prefix=CHECK-READOBJ %s
 # RUN: llvm-tblgen --help-hidden %t | FileCheck --check-prefix=CHECK-TBLGEN %s
 # RUN: llvm-opt-report --help-hidden %t | FileCheck --check-prefix=CHECK-OPT-RPT %s
-# RUN: llvm-dwarfdump --help-hidden %t | FileCheck --check-prefix=CHECK-DWARF %s
-# RUN: llvm-dwarfdump -h %t | FileCheck --check-prefix=CHECK-DWARF-H %s
-
 
 # CHECK-OBJDUMP: -h  - Alias for --section-headers
 # CHECK-READOBJ: -h  - Alias for --file-headers
 # CHECK-TBLGEN:  -h  - Alias for --help
 # CHECK-OPT-RPT: -h  - Alias for --help
-# CHECK-DWARF:   -h  - Alias for -help
-
-# llvm-dwarfdump declares `-h` option and prints special help in that case,
-# which is weird, but makes for a good test, i.e., shows the default `-h`
-# wasn't used.
-# CHECK-DWARF-H-NOT: --help-list  - Display list of available options (--help-list-hidden for more)

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: Specific Options
 HELP: -diff
 HELP: -find
 HELP: -ignore-case
 HELP: -lookup

tools/llvm-dwarfdump/llvm-dwarfdump.cpp

 /// @}
 /// Command line options.
 /// @{
 
 namespace {
 using namespace cl;
 
 OptionCategory DwarfDumpCategory("Specific Options");
-static opt<bool> Help("h", desc("Alias for -help"), Hidden,
-                      cat(DwarfDumpCategory));
 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 "
int main(int argc, char **argv) {
   llvm::InitializeAllTargetMCs();
 
   HideUnrelatedOptions({&DwarfDumpCategory, &SectionCategory, &ColorCategory});
   cl::ParseCommandLineOptions(
       argc, argv,
       "pretty-print DWARF debug information in object files"
       " and debug info archives.\n");
 
-  if (Help) {
-    PrintHelpMessage(/*Hidden =*/false, /*Categorized =*/true);
-    return 0;
-  }
-
   // FIXME: Audit interactions between these two options and make them
   //        compatible.
   if (Diff && Verbose) {
     WithColor::error() << "incompatible arguments: specifying both -diff and "
                           "-verbose is currently not supported";
     return 0;
   }