Page MenuHomePhabricator

[llvm-objdump] add support for '--reloc' as an alias of -r (PR39407)
ClosedPublic

Authored by Higuoxing on Oct 28 2018, 8:53 PM.

Diff Detail

Repository
rL LLVM

Event Timeline

Higuoxing created this revision.Oct 28 2018, 8:53 PM
Higuoxing updated this revision to Diff 171859.Oct 30 2018, 7:25 PM
Higuoxing edited the summary of this revision. (Show Details)
Higuoxing added reviewers: eush, MaskRay.

Update with tests

eush accepted this revision.Oct 30 2018, 11:48 PM
eush added a subscriber: llvm-commits.

LGTM

This revision is now accepted and ready to land.Oct 30 2018, 11:48 PM
kristina accepted this revision.Oct 31 2018, 12:09 AM
kristina added a subscriber: kristina.

Hm, maybe it would be a good idea to make the shorthand versions visible (since cl::alias implies cl::Hidden by default)? I feel like these aliases shouldn't be buried all the way in --help-hidden since they're pretty common.

Higuoxing added a comment.EditedOct 31 2018, 12:16 AM

Thank you everyone,

Hm, maybe it would be a good idea to make the shorthand versions visible (since cl::alias implies cl::Hidden by default)? I feel like these aliases shouldn't be buried all the way in --help-hidden since they're pretty common.

I agree with you. What if print help message like gnu-objdump? I think we could make it more like gnu-objdump

And here's another issue complains about --help option: https://bugs.llvm.org/show_bug.cgi?id=37895

I would like to collect some information on this topic

Thank you everyone,

Hm, maybe it would be a good idea to make the shorthand versions visible (since cl::alias implies cl::Hidden by default)? I feel like these aliases shouldn't be buried all the way in --help-hidden since they're pretty common.

I agree with you. What if print help message like gnu-objdump? I think we could make it more like gnu-objdump

And here's another issue complain about --help option: https://bugs.llvm.org/show_bug.cgi?id=37895

That's a bit more complicated due to llvm::cl basically being the main hub for random parts of LLVM to register various hidden options for tinkering most of which you don't usually want to see. I think making it match GNU tools wouldn't serve much purpose but on the other hand I think that relevant shorthand flags shouldn't be hidden (where relevant to the tool in question either).

Would you mind revising this to make the alias non-hidden?

eush added a comment.Oct 31 2018, 12:34 AM

Is there a way to make CommandLine print aliases more compactly? E.g.

-x, -all-headers         - Display all available header information

instead of

-all-headers             - Display all available header information
-x                       - Alias for --all-headers

There are quite a few aliases.

Would you mind revising this to make the alias non-hidden?

Of course, if this is good for community, I'd love to

eush added a comment.Oct 31 2018, 12:37 AM

Would you mind revising this to make the alias non-hidden?

Shouldn't this be addressed separately and for all aliases at once?

Is there a way to make CommandLine print aliases more compactly? E.g.

-x, -all-headers         - Display all available header information

instead of

-all-headers             - Display all available header information
-x                       - Alias for --all-headers

There are quite a few aliases.

It seems that there are only 3 options, Hidden, NotHidden and ReallyHidden, https://llvm.org/docs/CommandLine.html#hiding-an-option-from-help-output

kristina added a comment.EditedOct 31 2018, 1:14 AM

I think in this instance -r should not be hidden. And you'd need to override the default behavior by using cl::NotHidden. I spoke to Chandler and ideally we should start migrating common LLVM tools that substitute for binutils to a more user friendly llvm::Option based interface (kind of similar to what you mention regarding gnu-binutils) instead of the global cl:: registry which is really intended for global LLVM options, but that can be done later as part of larger refactoring effort (which I'll need to bring up on llvm-dev) and these tests are actually going to help a huge deal when that does happen so thank you for including them.

But ideally for now, while we're still using cl:: I think it makes more sense if it's not hidden since it's a tool specific option.

I'll update the one I committed earlier for -t to not be hidden as well, but for now since this is still a diff, would you mind revising it?

Shall we commit this patch and then refactor alias options? I remember I've brought 3 alias options into llvm-objdump (include this one). They are: https://reviews.llvm.org/rL345495 https://reviews.llvm.org/rL345697

Shall we commit this patch and then refactor alias options? I remember I've brought 3 alias options into llvm-objdump (include this one). They are: https://reviews.llvm.org/rL345495 https://reviews.llvm.org/rL345697

Well for llvm-objdump we'd likely use a similar approach to llvm-objcopy or lld where the options are defined in a TableGen file and then parsed and stored in some form of a state object, if that makes sense. But again, I think it's out of the scope of this patch, for just a single option that provides a provisional bugfix for compatibility with GNU tools.

Higuoxing updated this revision to Diff 171871.Oct 31 2018, 1:52 AM

Update with NotHidden flag

Higuoxing updated this revision to Diff 171872.Oct 31 2018, 1:54 AM
Higuoxing updated this revision to Diff 171874.Oct 31 2018, 2:13 AM

Is this version ok? Shall I update other options flag in another patch? Thanks a lot for your advice

This revision was automatically updated to reflect the committed changes.

Is this version ok? Shall I update other options flag in another patch? Thanks a lot for your advice

What you did is fine for now. I do think we should consider working on the command-line options a bit more in llvm-objdump. There are a few issues that I know of already, including https://bugs.llvm.org/show_bug.cgi?id=37895 (which covers the missing single-letter alias options in the help text) and https://bugs.llvm.org/show_bug.cgi?id=31679 (which points out that single-letter options don't combine properly (e.g. you can't do -fsr). The command-line options are also not GNU-compatible in some instances, in that they do different things. I'm going to raise a thread on the mailing list about this, if I get a chance, as to fix it, we'd have to change the meaning of some of the current options.

Is this version ok? Shall I update other options flag in another patch? Thanks a lot for your advice

What you did is fine for now. I do think we should consider working on the command-line options a bit more in llvm-objdump. There are a few issues that I know of already, including https://bugs.llvm.org/show_bug.cgi?id=37895 (which covers the missing single-letter alias options in the help text) and https://bugs.llvm.org/show_bug.cgi?id=31679 (which points out that single-letter options don't combine properly (e.g. you can't do -fsr). The command-line options are also not GNU-compatible in some instances, in that they do different things. I'm going to raise a thread on the mailing list about this, if I get a chance, as to fix it, we'd have to change the meaning of some of the current options.

I'm planning to redo argument parsing for a few common tools using llvm::Option with TableGen generated options, I sent a short proposal on llvm-dev since Chandler suggested moving away from llvm::cl completely as far as actual LLVM tools go and leaving that interface for LLVM components since it serves more as a global registry for tweaking various aspects of LLVM with many passes registering options via that functionality and there are already discussions (on Phab) about problems with that approach even in that regard when multiple in-process instances of LLVM are desired.