This is an archive of the discontinued LLVM Phabricator instance.

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

Authored by MaskRay on Jul 1 2021, 7:40 PM.
Tokens
"Like" token, awarded by post.kadirselcuk.

Details

Summary

Part of https://lists.llvm.org/pipermail/llvm-dev/2021-July/151622.html
"Binary utilities: switch command line parsing from llvm::cl to OptTable"

Users should generally observe no difference as long as they only use intended
option forms. Behavior changes:

  • -t=d is removed. Use -t d instead.
  • --demangle=0 cannot be used. Omit the option or use --no-demangle instead.
  • --help-list is removed. This is a cl:: specific option.

Note:

  • -t diagnostic gets improved.
  • Avoid cl::opt collision if we decide to support multiplexing for binary utilities
  • One-dash long options are still supported.
  • The -s collision (-s segment section for Mach-O) is unfortunate. -s means --print-armap in GNU nm.
  • This patch removes the last cl::multi_val use case from the llvm/lib/Support/CommandLine.cpp library

-M (--print-armap), -U (--defined-only), and -W (--no-weak)
are now deprecated. They could conflict with future GNU nm options.
(--print-armap has an existing alias -s, so GNU will unlikely add a new one.
--no-weak (not in GNU nm) is rarely used anyway.)

--just-symbol-name is now deprecated in favor of
--format=just-symbols and -j.

Diff Detail

Event Timeline

MaskRay created this revision.Jul 1 2021, 7:40 PM
MaskRay requested review of this revision.Jul 1 2021, 7:40 PM
Herald added a project: Restricted Project. · View Herald TranscriptJul 1 2021, 7:40 PM

@gbreynoo, could you do a check to see if any of our internal code bases would be broken by the changes made in this patch?

@MaskRay: not that I mind, but any particular reason single-dash long options are still supported for llvm-nm, but not other tools? That could be explained in the patch description, I think.

The -s collision (-s segment section for Mach-O) is unfortunate. -s means --print-armap in GNU nm.

I wonder if (in a separate patch), we should add a long option and deprecate this short alias? Then, in a future release, we can switch the meaning over, whilst giving people a path forward to migrate any scripts they may have.

llvm/test/tools/llvm-nm/help.test
5

Perhaps worth adding some check showing the Mach-O option grouping?

llvm/tools/llvm-nm/Opts.td
34

This should be above radix for alphabetical ordering.

35–36

Help description of these two options is incorrect.

47

This should be above arch for alphabetical ordering.

62

-h alias for --help?

llvm/tools/llvm-nm/llvm-nm.cpp
109–128

clang-format I think adds this.

2142

Perhaps worth adding a check for this (or more specifically probably just @FILE to help.test?

2219–2229

It's unfortunate that we have to hand-roll this additional logic, rather than relying on the command-line parser code to do it for us...

xgupta added a subscriber: xgupta.Jul 2 2021, 12:56 AM

Hi @MaskRay! I think you should drop an email on llvm-dev regarding these changes of "Switch command line parsing from llvm::cl to OptTable". Some kind of RFC would be good. (I am not opposing changes btw)

Hi @MaskRay! I think you should drop an email on llvm-dev regarding these changes of "Switch command line parsing from llvm::cl to OptTable". Some kind of RFC would be good. (I am not opposing changes btw)

https://lists.llvm.org/pipermail/llvm-dev/2021-July/151622.html Binary utilities: switch command line parsing from llvm::cl to OptTable (byproduct: drop -long-option?)

MaskRay added a comment.EditedJul 2 2021, 10:28 AM

@gbreynoo, could you do a check to see if any of our internal code bases would be broken by the changes made in this patch?

@MaskRay: not that I mind, but any particular reason single-dash long options are still supported for llvm-nm, but not other tools? That could be explained in the patch description, I think.

I am in favor of dropping support for single-dash long options, but just want to do it separately.
With this patch, I expect most users will not observe any difference. -t=d --demangle=0 are rare.
(For llvm-strings, I combined the two steps because I don't think anyone may use the single-dash form for the few long options in llvm-strings.)

nm -arch is a Mach-O specific option which might be controversial: https://lists.llvm.org/pipermail/llvm-dev/2021-July/151622.html

The -s collision (-s segment section for Mach-O) is unfortunate. -s means --print-armap in GNU nm.

I wonder if (in a separate patch), we should add a long option and deprecate this short alias? Then, in a future release, we can switch the meaning over, whilst giving people a path forward to migrate any scripts they may have.

The problem is that Darwin nm uses -s differently from GNU nm.
For -s it is fine for me because nm -s is not used that much and I can remember to use --print-armap.
To my surprise Darwin nm and GNU nm do share many command line options... otherwise I guess we would need something similar to llvm-libtool-darwin.

-s is indeed very unfortunate. In other places the convention is a comma-separated form, e.g. __TEXT,__text

aganea added a subscriber: aganea.Jul 2 2021, 11:24 AM
aganea added inline comments.
llvm/tools/llvm-nm/llvm-nm.cpp
108

Can you take the opportunity and put all this onto a stack-based state class? So that the tool can be used as-a-lib & be thread-safe? A practical example could be usage of llvm/tools/llvm-shlib/gen-msvc-exports.py, but in-process.

2127–2147

It looks like L2128-L2151 are the same as D104889, llvm/tools/llvm-strings/llvm-strings.cpp, L131-L151. Is it worth putting this boilerplate in a small utility function in llvm/Support/, since we'll probably write the same thing for other bintools?

MaskRay added inline comments.Jul 2 2021, 11:35 AM
llvm/tools/llvm-nm/llvm-nm.cpp
2127–2147

BumpPtrAllocator and StringSaver are the same.

OPT_help can be different (specially the " [options] <input object files>", "llvm string table dumper" lines).
It is hard for code sharing because OPT_help is not special in OptTable. The user code knows this is an help option but OptTable doesn't.

OPT_version can be different (some utilities need GNU libtool workaround like: outs() << "llvm-nm, compatible with GNU nm" << '\n';).

Overall I think code sharing.is difficult for the boilerplate.

Hey, everyone, I guess this revision is up to date right now?

This revision is now accepted and ready to land.Jul 2 2021, 11:39 AM
MaskRay updated this revision to Diff 356306.Jul 2 2021, 5:43 PM

Hide options that are currently hidden, e.g. -M (--print-armap; GNU doesn't have -M), -W

MaskRay updated this revision to Diff 356309.Jul 2 2021, 6:54 PM
MaskRay marked 7 inline comments as done.

address comments

This revision now requires review to proceed.Jul 4 2021, 8:14 PM
jhenderson added inline comments.Jul 5 2021, 12:29 AM
llvm/tools/llvm-nm/Opts.td
39

I don't follow why you've hidden this option?

52

Ditto.

65

FTR: this one I can get behind if GNU doesn't have it, due to the risk of a future name clash. You should remove it from the CommandGuide though. Maybe also add a deprecation note for it to the release notes too.

Same with -W and -U below.

MaskRay updated this revision to Diff 356534.Jul 5 2021, 10:35 AM
MaskRay marked 4 inline comments as done.
MaskRay edited the summary of this revision. (Show Details)

Mark -M/-U/-W deprecated

llvm/tools/llvm-nm/Opts.td
39

--without-aliases is currently hidden. It seems to be for debugging.

The name is confusing: it applies to just LLVM bitcode files but the documentation isn't clear about this fact.

52

The canonical forms are --format=just-symbols and -j now. It seems wasteful to have a third form --just-symbol-name.

llvm/tools/llvm-nm/llvm-nm.cpp
108

Answered in D104889. Not sure localization will help for the oneshot utility.

jhenderson added inline comments.Jul 6 2021, 12:07 AM
llvm/docs/CommandGuide/llvm-nm.rst
135

Up to you, but I personally think it's fine to remove reference to -U (also -W, -M) entirely.

llvm/docs/ReleaseNotes.rst
173

Either "have been deprecated" or "are now deprecated". Perhaps also say "Use the long form versions instead." or some equivalent phrasing.

174

This is the wrong link.

llvm/tools/llvm-nm/Opts.td
39

It's in the command guide text. Either we should remove it from there, or it should be visible in the help text.

52

Fair enough. In that case, can we go through a deprecation process like the short option aliases? (Namely remove it from the command guide and add a release note).

MaskRay updated this revision to Diff 356618.Jul 6 2021, 12:20 AM
MaskRay marked 4 inline comments as done.
MaskRay edited the summary of this revision. (Show Details)

address comments

llvm/tools/llvm-nm/Opts.td
39

Removed --without-aliases from llvm/docs/CommandGuide/llvm-nm.rst

jhenderson accepted this revision.Jul 6 2021, 12:32 AM

Okay, LGTM. Please leave it for a bit before committing though, in case @aganea or anybody else has any more comments.

This revision is now accepted and ready to land.Jul 6 2021, 12:32 AM
MaskRay edited the summary of this revision. (Show Details)Jul 7 2021, 1:03 PM
MaskRay edited the summary of this revision. (Show Details)
dyung added a subscriber: dyung.Jul 7 2021, 2:54 PM

I'm not sure why, but after your change (but strangely not when testing your change) to the test tools/llvm-nm/just-symbols.test, it seems to periodically fail on the PS4 buildbot.

  1. https://lab.llvm.org/buildbot/#/builders/139/builds/6756
  2. https://lab.llvm.org/buildbot/#/builders/139/builds/6759
  3. https://lab.llvm.org/buildbot/#/builders/139/builds/6761
  4. https://lab.llvm.org/buildbot/#/builders/139/builds/6762

Can you take a look?

I'm not sure why, but after your change (but strangely not when testing your change) to the test tools/llvm-nm/just-symbols.test, it seems to periodically fail on the PS4 buildbot.

  1. https://lab.llvm.org/buildbot/#/builders/139/builds/6756
  2. https://lab.llvm.org/buildbot/#/builders/139/builds/6759
  3. https://lab.llvm.org/buildbot/#/builders/139/builds/6761
  4. https://lab.llvm.org/buildbot/#/builders/139/builds/6762

Can you take a look?

Thanks for the report. Fixed by b81aa458afd023323dcd4400164f6a43d981d7de

hubert.reinterpretcast added inline comments.
llvm/test/tools/llvm-nm/just-symbols.test
10

This is causing failures on our AIX builds:

diff: illegal option -- q
Usage: diff [-bitw] [[-C Lines|-D String|-e|-f|-n|-U Lines|-c|-u]|[-h]] File1 File2
       diff [-bilrstw] [[-C Lines|-e|-f|-n|-U Lines|-c|-u]|[-h]] [-S File] Directory1 Directory2
MaskRay marked an inline comment as done.Jul 9 2021, 3:25 PM
MaskRay added inline comments.
llvm/test/tools/llvm-nm/just-symbols.test
10

I removed -q.

post.kadirselcuk added projects: Restricted Project, Restricted Project, Restricted Project, Restricted Project, Restricted Project, Restricted Project, Restricted Project.Jul 11 2021, 12:43 AM
jhenderson removed projects: Restricted Project, Restricted Project, Restricted Project, Restricted Project, Restricted Project, Restricted Project, Restricted Project.Jul 12 2021, 4:13 AM

@MaskRay You say in the commit msg:

One-dash long options are still supported.

Does that mean that -format=sysv should still be supported? I'm not sure if that's a "long option" or what.

It's used in the llvm-testsuite: https://github.com/llvm/llvm-test-suite/blob/main/litsupport/modules/codesize.py

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

@MaskRay You say in the commit msg:

One-dash long options are still supported.

Does that mean that -format=sysv should still be supported? I'm not sure if that's a "long option" or what.

It's used in the llvm-testsuite: https://github.com/llvm/llvm-test-suite/blob/main/litsupport/modules/codesize.py

This is llvm-nm.

The llvm-test-suite you fixed is llvm-size. llvm-size is simple so I just removed one-dash long options.