Page MenuHomePhabricator

[llvm-readobj] Switch command line parsing from llvm::cl to OptTable
ClosedPublic

Authored by MaskRay on Jul 6 2021, 11:34 PM.

Details

Summary

Users should generally observe no difference as long as they don't use
unintended option forms. Behavior changes:

  • -t=d is removed. Use -t d instead.
  • --demangle=false and --demangle=0 cannot be used. Omit the option or use --no-demangle. Other flag-style options don't have --no- forms.
  • --help-list is removed. This is a cl:: specific option.
  • llvm-readobj now supports grouped short options as well.
  • --color is removed. This is generally not useful (only apply to errors/warnings) but was inherited from Support.

Some adjustment to the canonical forms
(usually from GNU readelf; currently llvm-readobj has too many redundant aliases):

  • --dyn-syms is canonical. --dyn-symbols is a hidden alias
  • --file-header is canonical. --file-headers is a hidden alias
  • --histogram is canonical. --elf-hash-histogram is a hidden alias
  • --relocs is canonical. --relocations is a hidden alias
  • --section-groups is canonical. --elf-section-groups is a hidden alias

OptTable avoids global option collision if we decide to support multiplexing for binary utilities.

  • Most one-dash long options are still supported. -dt, -sd, -st, -sr are dropped due to their conflict with grouped short options.
  • --section-mapping=false (D57365) is strange but is kept for now.
  • Many cl::opt variables were unnecessarily external. I added static whenever appropriate.

Diff Detail

Event Timeline

MaskRay created this revision.Jul 6 2021, 11:34 PM
MaskRay requested review of this revision.Jul 6 2021, 11:34 PM
Herald added a project: Restricted Project. · View Herald TranscriptJul 6 2021, 11:34 PM
MaskRay edited the summary of this revision. (Show Details)Jul 6 2021, 11:39 PM
MaskRay edited the summary of this revision. (Show Details)Jul 6 2021, 11:41 PM

Docs update for llvm-readobj and llvm-readelf?

check-default-options.txt in the Support tests is failing due to this change.

--demangle=false and --demangle=0 cannot be used. Just omit the option.

In limited situations, a user may be unable to easily remove --demangle from earlier in the command-line (e.g. it's part of a script), so I think having a --no-demangle option is somewhat important.

--section-mapping=false (D57365) is strange but is kept for now.

I'd like to see the functionality kept (it allows turning off section mapping dumping when using GNU output style to dump the program header table), but I don't mind if the option changes to, e.g. --no-section-mapping, in a future change.

One-dash long options are still supported.

If I follow things correctly, this isn't entirely true. For example, -dt no longer means --dyn-symbols.

@gbreynoo, another option modification patch for you to look at and be aware of.

Note to self: out of time now, but still need to review the llvm-readobj.cpp changes.

llvm/test/tools/llvm-readobj/ELF/grouped.test
53–55
llvm/tools/llvm-readobj/Opts.td
46
49

We should probably drop the "llvm-readobj" bit from these descriptions, as the tool is usually installed as "llvm-readelf" on people's systems, so the name would be incorrect there. Here, we can simply say "ELF Specific Options".

50

If I'm not mistaken, you're missing the --dynamic alias.

51

Does clang-format have a tablegen formatting option? This and a few other lines in this patch are getting quite long.

54

You seem to be missing the --section-groups alias.

73

This set isn't ordered alphabetically, unlike the other sets.

103

I wouldn't be sad if you wanted to hide the "elf-" aliases (and at some point later remove them entirely). I think they are largely a historical mistake.

105

I'd make --histogram the canonical form, and --elf-hash-histogram the alias personally. Also applies to -I

116

I'd make this refer to --section-groups, not --elf-section-groups (the former is the GNU name, the latter not - see also above).

118

This should be further down in the list, for alphabetical ordering.

MaskRay added inline comments.Jul 7 2021, 1:19 AM
llvm/tools/llvm-readobj/Opts.td
49

The option groups are sorted alphabetically, so "ELF Specific Options" would be before "OPTIONS"... That's how the llvm-readobj prefix helps, OPTIONS < llvm-readobj ...

MaskRay updated this revision to Diff 357024.Jul 7 2021, 11:37 AM
MaskRay marked 10 inline comments as done.
MaskRay edited the summary of this revision. (Show Details)

address comments

many refinements

llvm/tools/llvm-readobj/Opts.td
49

I am going to use the OPTIONS (ELF specific) style for now.

51

It does but the result isn't appealing. It may use many lines for some defs and the continuation positions are not consistent.

def section_mapping_EQ_false                                                                                
    : FF<"section-mapping=false",                                                                           
         "Don't display the section to segment mapping">,                                                           
      Flags<[HelpHidden]>;
...
def elf_hash_histogram : FF<"elf-hash-histogram",         
                            "Display bucket list histogram for hash sections">,                                     
                         Group<grp_elf>;                  
def elf_linker_options                                    
    : FF<"elf-linker-options", "Display the .linker-options section">,                                              
      Group<grp_elf>;                                     
defm elf_output_style : Eq<"elf-output-style", "Specify ELF dump style">,                                           
                        Group<grp_elf>;
jhenderson added inline comments.Jul 8 2021, 1:35 AM
llvm/tools/llvm-readobj/Opts.td
49

Thanks, I was going to suggest something along the same lines, but you beat me to it!

93

Any particular reason --wide is hidden, but -W isn't? Both are in the help text of GNU readelf. I don't really mind which way you jump here. I just think they should both be the same (hidden or visible). If push comes to shove, I'd leave them as visible.

104

These elf-* options are all in the docs and should be removed if they are being marked as hidden.

108

I'm not sure about hiding this one. If you really feel it should be hidden, remove it from both docs (currently it's only missing from one).

jhenderson added inline comments.Jul 9 2021, 1:05 AM
llvm/tools/llvm-readobj/Opts.td
54

This is no longer in alphabetical order (ditto histogram above).

llvm/tools/llvm-readobj/llvm-readobj.cpp
228

Reorder alphabetically.

566

Might want to fix this line ;) (actually do you need it at all?)

MaskRay marked 7 inline comments as done.Jul 9 2021, 4:10 PM
MaskRay added inline comments.
llvm/tools/llvm-readobj/llvm-readobj.cpp
566

There was no llvm-readobj line previously. So I don't add it.

Previously there was a buggy line: the "registered targets" is always "none" because InitializeAll* is done too late. I assume this lines don't matter so I delete them.

Registered Targets:
  (none)
MaskRay updated this revision to Diff 357661.Jul 9 2021, 4:14 PM
MaskRay marked an inline comment as done.

address comments

This revision is now accepted and ready to land.Jul 12 2021, 3:46 AM
xgupta added a subscriber: xgupta.Jul 14 2021, 4:58 AM
xgupta added inline comments.
llvm/docs/CommandGuide/llvm-readelf.rst
83

Documentation build on my system is failing with this change.

Warning, treated as error:
/home/user/llvm-project/llvm/docs/CommandGuide/llvm-readelf.rst:101:unknown option: --file-headers

MaskRay marked an inline comment as done.Jul 14 2021, 12:39 PM
MaskRay added inline comments.
llvm/docs/CommandGuide/llvm-readelf.rst
83

Thanks for the report. Fixed by 3bda1c4e22cdf54f899f0330a98213337df9b3d4

Hi MaskRay,

I saw the following options are now hidden for llvm-readelf but are still present in the documentation:

  • elf-hash-histogram
  • elf-section-groups

Thanks

MaskRay marked an inline comment as done.Jul 16 2021, 10:41 AM

Hi MaskRay,

I saw the following options are now hidden for llvm-readelf but are still present in the documentation:

  • elf-hash-histogram
  • elf-section-groups

Thanks

Thanks. Pushed ca012627cd9352d5ff5eccfadeaeabc083c8f704 to change docs/CommandGuide/llvm-readelf.rst. The two .rst files have other differences that I don't change.