Page MenuHomePhabricator

[CommandLineParser] Add DefaultOption flag
ClosedPublic

Authored by hintonda on Mar 24 2019, 1:10 AM.

Details

Summary

Add DefaultOption flag to CommandLineParser which provides a default option or alias, but allows users to override it for some other purpose as needed.

Also, add -h as a default alias to -help, which can be seamlessly overridden by applications like llvm-objdump and llvm-readobj which use -h as an alias for other options.

Diff Detail

Repository
rL LLVM

Event Timeline

hintonda created this revision.Mar 24 2019, 1:10 AM
Herald added a project: Restricted Project. · View Herald TranscriptMar 24 2019, 1:10 AM

A better alternative would have been to add a cl::aliasopt for '-h' in llvm's CommandLineParser when '-help' was first added. However, that's no longer possible since some llvm based tools already use '-h' for other purposes.

@arphaman, since you added the -h option in D37618, could you let me know if this change is okay with you?

alexfh added a comment.Fri, Apr 5, 5:47 AM

Can you give a specific example of how this problem manifests?

Can you give a specific example of how this problem manifests?

Any tool using tooling::CommonOptionsParser with the llvm::cl::OneOrMore flag will have this problem, i.e., the -h flag will be seen, but no action taken because it was never wired up. Please note that we changed clang-tidy to use cl::ZeroOrMore a few years ago, so it'll spit out help with or without any arguments, including -h. However, if you pass a bad argument with -h you'll get an error, but no help.

Here are a few you can verify (first try with -h, then -help):

$ bin/clang-tidy -h -x
LLVM ERROR: CommonOptionsParser: failed to parse command-line arguments. [CommonOptionsParser]: clang-tidy: Unknown command line argument '-x'.  Try: 'bin/clang-tidy -help'
clang-tidy: Did you mean '-h'?

$ bin/clang-tidy -help -x
USAGE: clang-tidy [options] <source0> [... <sourceN>]
<snip>


$ bin/clang-refactor -h
error: no refactoring action given
note: the following actions are supported:
  local-rename
  extract

$ bin/clang-refactor -help
OVERVIEW: Clang-based refactoring tool for C, C++ and Objective-C
USAGE: clang-refactor [options] <source0> [... <sourceN>]
<snip>


$ bin/clang-apply-replacements -h
clang-apply-replacements: Unknown command line argument '-h'.  Try: 'bin/clang-apply-replacements -help'
clang-apply-replacements: Did you mean '-help'?
clang-apply-replacements: Not enough positional command line arguments specified!
Must specify at least 1 positional argument: See: bin/clang-apply-replacements -help

$ bin/clang-apply-replacements -help
USAGE: clang-apply-replacements [options] <Search Root Directory>
<snip>


$ bin/clang-change-namespace -h
clang-change-namespace: for the -old_namespace option: must be specified at least once!
clang-change-namespace: for the -new_namespace option: must be specified at least once!
clang-change-namespace: for the -file_pattern option: must be specified at least once!
LLVM ERROR: CommonOptionsParser: failed to parse command-line arguments. [CommonOptionsParser]: clang-change-namespace: Not enough positional command line arguments specified!
Must specify at least 1 positional argument: See: bin/clang-change-namespace -help

$ bin/clang-change-namespace -help
USAGE: clang-change-namespace [options] <source0> [... <sourceN>]
<snip>
klimek added a comment.Mon, Apr 8, 1:58 AM

A better alternative would have been to add a cl::aliasopt for '-h' in llvm's CommandLineParser when '-help' was first added. However, that's no longer possible since some llvm based tools already use '-h' for other purposes.

Is that intentional? Can you point at samples?

A better alternative would have been to add a cl::aliasopt for '-h' in llvm's CommandLineParser when '-help' was first added. However, that's no longer possible since some llvm based tools already use '-h' for other purposes.

Is that intentional? Can you point at samples?

A quick grep found these -- there could be more.

llvm-opt-report adds -h for help and handles it after ParseCommandLineOptions returns.
llvm-objdump adds -h as an alias for --section-headers.
dsymutil adds -h for help and handles it after ParseCommandLineOptions returns.
llvm-readobj adds -h as an alias for --file-headers.
llvm-dwarfdump adds -h for help and handles it after ParseCommandLineOptions returns.
llvm-mt rolls its own via llvm-tblgen.

So, the only ones I found that aren't essentially aliases for help are in llvm-objdump and llvm-readobj.

klimek added a comment.Mon, Apr 8, 8:07 AM

A better alternative would have been to add a cl::aliasopt for '-h' in llvm's CommandLineParser when '-help' was first added. However, that's no longer possible since some llvm based tools already use '-h' for other purposes.

Is that intentional? Can you point at samples?

A quick grep found these -- there could be more.

llvm-opt-report adds -h for help and handles it after ParseCommandLineOptions returns.
llvm-objdump adds -h as an alias for --section-headers.
dsymutil adds -h for help and handles it after ParseCommandLineOptions returns.
llvm-readobj adds -h as an alias for --file-headers.
llvm-dwarfdump adds -h for help and handles it after ParseCommandLineOptions returns.
llvm-mt rolls its own via llvm-tblgen.

So, the only ones I found that aren't essentially aliases for help are in llvm-objdump and llvm-readobj.

If we make it an alias by default, can somebody overwrite that?

A better alternative would have been to add a cl::aliasopt for '-h' in llvm's CommandLineParser when '-help' was first added. However, that's no longer possible since some llvm based tools already use '-h' for other purposes.

Is that intentional? Can you point at samples?

A quick grep found these -- there could be more.

llvm-opt-report adds -h for help and handles it after ParseCommandLineOptions returns.
llvm-objdump adds -h as an alias for --section-headers.
dsymutil adds -h for help and handles it after ParseCommandLineOptions returns.
llvm-readobj adds -h as an alias for --file-headers.
llvm-dwarfdump adds -h for help and handles it after ParseCommandLineOptions returns.
llvm-mt rolls its own via llvm-tblgen.

So, the only ones I found that aren't essentially aliases for help are in llvm-objdump and llvm-readobj.

If we make it an alias by default, can somebody overwrite that?

Unfortunately, that produces a runtime error:

lan1:/Users/dhinton/projects/llvm_project/monorepo/build/Debug $ bin/llvm-objdump -h
: CommandLine Error: Option 'h' registered more than once!
LLVM ERROR: inconsistency in registered CommandLine options

The operative lines:

llvm/tools/llvm-objdump/llvm-objdump.cpp:187:static cl::alias SectionHeadersShorter("h",
llvm/lib/Support/CommandLine.cpp:2149:static cl::alias HOpA("h", cl::desc("Alias for -help"), cl::aliasopt(HOp));

If we make it an alias by default, can somebody overwrite that?

Unfortunately, that produces a runtime error:

lan1:/Users/dhinton/projects/llvm_project/monorepo/build/Debug $ bin/llvm-objdump -h
: CommandLine Error: Option 'h' registered more than once!
LLVM ERROR: inconsistency in registered CommandLine options

The operative lines:

llvm/tools/llvm-objdump/llvm-objdump.cpp:187:static cl::alias SectionHeadersShorter("h",
llvm/lib/Support/CommandLine.cpp:2149:static cl::alias HOpA("h", cl::desc("Alias for -help"), cl::aliasopt(HOp));

The other problem is that these are statics, so you can't count on the order, i.e., did the user overwrite get processed before or after the one in CommandLine.cpp?

klimek added a comment.Tue, Apr 9, 6:02 AM

If we make it an alias by default, can somebody overwrite that?

Unfortunately, that produces a runtime error:

lan1:/Users/dhinton/projects/llvm_project/monorepo/build/Debug $ bin/llvm-objdump -h
: CommandLine Error: Option 'h' registered more than once!
LLVM ERROR: inconsistency in registered CommandLine options

The operative lines:

llvm/tools/llvm-objdump/llvm-objdump.cpp:187:static cl::alias SectionHeadersShorter("h",
llvm/lib/Support/CommandLine.cpp:2149:static cl::alias HOpA("h", cl::desc("Alias for -help"), cl::aliasopt(HOp));

The other problem is that these are statics, so you can't count on the order, i.e., did the user overwrite get processed before or after the one in CommandLine.cpp?

If we can extend cl::alias to support this, we could give it a priority and take the highest prio :)

If we make it an alias by default, can somebody overwrite that?

Unfortunately, that produces a runtime error:

lan1:/Users/dhinton/projects/llvm_project/monorepo/build/Debug $ bin/llvm-objdump -h
: CommandLine Error: Option 'h' registered more than once!
LLVM ERROR: inconsistency in registered CommandLine options

The operative lines:

llvm/tools/llvm-objdump/llvm-objdump.cpp:187:static cl::alias SectionHeadersShorter("h",
llvm/lib/Support/CommandLine.cpp:2149:static cl::alias HOpA("h", cl::desc("Alias for -help"), cl::aliasopt(HOp));

The other problem is that these are statics, so you can't count on the order, i.e., did the user overwrite get processed before or after the one in CommandLine.cpp?

If we can extend cl::alias to support this, we could give it a priority and take the highest prio :)

Okay, I'll give it that old college try, and see if I can come up with something not too kludgy. ;-)

If we make it an alias by default, can somebody overwrite that?

Unfortunately, that produces a runtime error:

lan1:/Users/dhinton/projects/llvm_project/monorepo/build/Debug $ bin/llvm-objdump -h
: CommandLine Error: Option 'h' registered more than once!
LLVM ERROR: inconsistency in registered CommandLine options

The operative lines:

llvm/tools/llvm-objdump/llvm-objdump.cpp:187:static cl::alias SectionHeadersShorter("h",
llvm/lib/Support/CommandLine.cpp:2149:static cl::alias HOpA("h", cl::desc("Alias for -help"), cl::aliasopt(HOp));

The other problem is that these are statics, so you can't count on the order, i.e., did the user overwrite get processed before or after the one in CommandLine.cpp?

If we can extend cl::alias to support this, we could give it a priority and take the highest prio :)

Okay, I'll give it that old college try, and see if I can come up with something not too kludgy. ;-)

Btw, do you just want default aliases, or default options in general?

If we can extend cl::alias to support this, we could give it a priority and take the highest prio :)

Okay, I'll give it that old college try, and see if I can come up with something not too kludgy. ;-)

Btw, do you just want default aliases, or default options in general?

I hope you don't mind, but I've got a few more design questions:

  • should the default api be public, usable by Clang Tooling, or just usable within CommandLine.cpp?
  • which options should be hard-coded, i.e., status quo, and which should be defaulted.
  • should we provide some sort of diagnostic to user to alert them that they have overwritten a defaulted options -- note this has to happen at runtime. Guess it could be a help option, a la hidden-help, that shows status of each option. I actually like the idea of option origin in general.

Have a working prototype for aliases, but underneath, an option is an option, so should be able to support both without any trouble. Will upload as soon as I clean it up and add tests, etc...

Just chiming in from a LLVM binutils developer perspective. Some of the binutils are missing -h as an alias, when they really should have it for compatibility (specifically I checked llvm-nm and llvm-symbolizer). As a result, a solution that auto-adds -h as an alias would be good, as long as we have the ability to override it somehow. I wouldn't mind having logic in llvm-objdump/llvm-readobj to explicitly override the short alias if it is required.

hintonda updated this revision to Diff 194539.Wed, Apr 10, 9:44 AM
  • Add DefaultOption logic.

Still needs tests, but wanted to get early feedback on basic approach.

Herald added a project: Restricted Project. · View Herald TranscriptWed, Apr 10, 9:44 AM
hintonda marked an inline comment as done.Wed, Apr 10, 10:33 AM
hintonda added inline comments.
llvm/lib/Support/CommandLine.cpp
1187 ↗(On Diff #194539)

Looks like this is a bug in the way sub commands are handled, but I'd like to get feedback on how it *should* work.

The problem is that if an option is added with cl::sub(*AllSubCommands) it gets added to all registered subcommands, including TopLevelSubCommand. However, TopLevelSubCommand isn't included in Option::Subs, so I have to update/remove via two commands, one at the parser level for the ChosenSubCommand, which happens to be TopLevelSubCommand in this case, and another for the Option itself, which doesn't have TopLevelSubCommand in it's subs.

Should CommandLineParser::addOption specifically exclude TopLevelSubCommand when it add subs to an Option? That seems like a bug to me, but I'm not sure I grok the reason it was excluded.

hintonda retitled this revision from [LibTooling] Fix '-h' option to [CommandLineParser] Add DefaultOption flag.Wed, Apr 10, 11:11 AM
hintonda edited the summary of this revision. (Show Details)
hintonda updated this revision to Diff 194633.Wed, Apr 10, 7:51 PM
hintonda edited the summary of this revision. (Show Details)
  • Fix bug in updateArgStr() where it didn't handle isInAllSubCommands() correctly, and refactored code based on how SubCommands work.

I've not looked at the implementation in depth, but cl::DefaultOption seems like a nice way of handling this. Please make sure that there is testing in llvm-readobj and llvm-objdump that test their own short alias interpretation somewhere though.

hintonda updated this revision to Diff 194669.Thu, Apr 11, 6:02 AM
  • Delay actually adding default options until ParseCommandLineOptions which simplifies handling and makes it easy to only add them to the correct SubCommand.

    Add simple tests.
hintonda marked 2 inline comments as done.Thu, Apr 11, 6:07 AM
hintonda added inline comments.
llvm/lib/Support/CommandLine.cpp
282 ↗(On Diff #194669)

Will move this bug fix to its own patch and add test.

I've not looked at the implementation in depth, but cl::DefaultOption seems like a nice way of handling this. Please make sure that there is testing in llvm-readobj and llvm-objdump that test their own short alias interpretation somewhere though.

While I'm still working on unittests, I have added a few tests that should cover this. Please let me know if you have any concerns.

hintonda updated this revision to Diff 194708.Thu, Apr 11, 9:13 AM
  • Add unittest.
hintonda updated this revision to Diff 194729.Thu, Apr 11, 11:49 AM

Enhance Option::reset to reset the default value and remove the option
if it's a cl::DefaultOption.

Also, make sure removeOption only removes an option if both the name
and the Option matches, not just the name -- different Options with
the same name in different SubCommands have always been possible, but
are more common with DefaultOptions.

@klimek, this patch is now ready for review. Thanks...

klimek added inline comments.Fri, Apr 12, 1:47 AM
llvm/lib/Support/CommandLine.cpp
101 ↗(On Diff #194729)

A comment explaining what this contains and how it'll be used would help.

152 ↗(On Diff #194729)

This is used multiple times, add isDefault() to Option?

196 ↗(On Diff #194729)

I'd delete this comment, I don't think it helps.

1199 ↗(On Diff #194729)

Same here, this comment doesn't say anything non-obvious.

hintonda updated this revision to Diff 194899.Fri, Apr 12, 8:51 AM
  • Fix comments and add isDefaultOption per review comments.
This revision is now accepted and ready to land.Sat, Apr 13, 8:13 AM

lg

Thanks again for the help and the suggestion.

Closed by commit rL358337: [CommandLineParser] Add DefaultOption flag (authored by dhinton, committed by ). · Explain WhySat, Apr 13, 9:56 AM
This revision was automatically updated to reflect the committed changes.
hintonda reopened this revision.Mon, Apr 15, 9:10 AM

Reopen to track fix after buildbot failure and revert -- r358414.

http://lists.llvm.org/pipermail/llvm-commits/Week-of-Mon-20190415/644037.html

This revision is now accepted and ready to land.Mon, Apr 15, 9:10 AM
hintonda updated this revision to Diff 195217.Mon, Apr 15, 10:07 AM
  • Original patch, r358337, that was reverted.
  • Make sure to clear DefaultOptions on reset.
hintonda updated this revision to Diff 195218.Mon, Apr 15, 10:10 AM
  • Add new test back.
Harbormaster completed remote builds in B30575: Diff 195218.
Closed by commit rL358428: [CommandLineParser] Add DefaultOption flag (authored by dhinton, committed by ). · Explain WhyMon, Apr 15, 10:17 AM
This revision was automatically updated to reflect the committed changes.