llvm dev - Jul 2021 - Binary utilities: switch command line parsing from llvm::cl to OptTable (byproduct: drop -long-option?)

On Fri, Jul 2, 2021 at 10:15 AM Fāng-ruì Sòng via llvm-dev <
llvm-dev at lists.llvm.org> wrote:
> llvm/tools/ include some binary utilities used as replacement for GNU
> binutils, e.g. llvm-objcopy, llvm-symbolizer, llvm-nm.
> In some old threads people discussed some drawbacks of using cl::opt for
> user-facing utilities (I cannot find them now).
>
Would be good to describe some of the known drawbacks/expected benefits.

One potential one (though I don't recall it being discussed recently) would
be that maybe this addresses the issue of global ctors in cl::opt? Does
OptTable avoid/not use global constructors? That would be nice - it's an
ongoing issue that LLVM library users pay for command line argument support
they have no need for in the form of global ctor execution time.

> Switching to OptTable is an appealing solution. I have prepared two
> patches for two binary utilities: llvm-nm and llvm-strings.
>
> * llvm-strings https://reviews.llvm.org/D104889
> * llvm-nm https://reviews.llvm.org/D105330
>
> llvm-symbolizer was switched last year. llvm-objdump was switched by
> thakis earlier this year.
>
> The switch can fix some corners with lib/Support/CommandLine.cpp. Here is
> a summary:
>
> * -t=d is removed (equal sign after a short option). Use -t d instead.
> * --demangle=0 (=0 to disable a boolean option) is removed. Omit the
> option or use --no-demangle instead.
> * To support boolean options (e.g. --demangle --no-demangle), we don't
> need to compare their positions (if (NoDemangle.getPosition() >
> Demangle.getPosition()) , see llvm-nm.cpp)
> * grouped short options can be specified with one line
> `setGroupedShortOptions`, instead of adding cl::Grouping to every short
> options.
> * We don't need to add cl::cat to every option and call
> `HideUnrelatedOptions` to hide unrelated options from --help. The issue
> would happen with cl::opt tools if linker garbage collection is disabled or
> libLLVM-13git.so is used. (See https://reviews.llvm.org/D104363)
> * If we decide to support binary utility multiplexting (
> https://reviews.llvm.org/D104686), we will not get conflicting options.
> An option may have different meanings in different utilities (especially
> for one-letter options).
>
> *I expect that most users will not observe any difference.*
>
> There is a related topic whether we should disallow the single-dash
> `-long-option` form.
>
(Discussed in 2019:
> https://lists.llvm.org/pipermail/llvm-dev/2019-April/131786.html Accept
> --long-option but not -long-option for llvm binary utilities)
> *I'd like to disallow -long-option but may want to do this in a
separate
> change.*
>
I'd say definitely do this as a separate change. I expect there'd be a
long
tail of users after this change ships in an LLVM release, etc, such that we
may want to undo some amount of it a long time after the change is made.

>
> The main point is that (1) grouped short options have syntax conflict with
> one-dash long options. (2) the GNU getopt_long style two-dash long option
> is much more popular.
>
> I can think of potential pushback for some Mach-O specific options, e.g.
> nm -arch
> http://www.manpagez.com/man/1/nm/osx-10.12.6.php says `-arch` has one
> dash.
> If such options may have problems, we can keep supporting one dash forms.
> With OptTable, allowing one-dash forms for a specific option is easy.
> _______________________________________________
> LLVM Developers mailing list
> llvm-dev at lists.llvm.org
> https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20210702/9b784f4f/attachment.html>

On Fri, Jul 2, 2021 at 11:17 AM David Blaikie <dblaikie at gmail.com>
wrote:
> On Fri, Jul 2, 2021 at 10:15 AM Fāng-ruì Sòng via llvm-dev <
> llvm-dev at lists.llvm.org> wrote:
>
>> llvm/tools/ include some binary utilities used as replacement for GNU
>> binutils, e.g. llvm-objcopy, llvm-symbolizer, llvm-nm.
>> In some old threads people discussed some drawbacks of using cl::opt
for
>> user-facing utilities (I cannot find them now).
>>
>
> Would be good to describe some of the known drawbacks/expected benefits.
>
The summary is the list of benefits:)
The drawback is some initial boilerplate (e.g. llvm-tblgen
-gen-opt-parser-defs in CMakeLists.txt, class NmOptTable in code).
The handling of comma separated options -arch=x86_64,arm64 doesn't have
direct OptTable support. llvm::SplitString is needed (just search for
SplitString in https://reviews.llvm.org/D105330)
But this doesn't tend to increase complexity because the
cl::list<std::string> will need per-value verification anyway.

> One potential one (though I don't recall it being discussed recently)
> would be that maybe this addresses the issue of global ctors in cl::opt?
> Does OptTable avoid/not use global constructors? That would be nice -
it's
> an ongoing issue that LLVM library users pay for command line argument
> support they have no need for in the form of global ctor execution time.
>
OptTable is used as a local variable. So yes, it avoids global
constructors, thus avoiding cl::opt option name collision.
"If we decide to support binary utility multiplexing" below mentioned
this
point.

Switching to OptTable is an appealing solution. I have prepared two
patches
>> for two binary utilities: llvm-nm and llvm-strings.
>>
>> * llvm-strings https://reviews.llvm.org/D104889
>> * llvm-nm https://reviews.llvm.org/D105330
>>
>> llvm-symbolizer was switched last year. llvm-objdump was switched by
>> thakis earlier this year.
>>
>> The switch can fix some corners with lib/Support/CommandLine.cpp. Here
is
>> a summary:
>>
>> * -t=d is removed (equal sign after a short option). Use -t d instead.
>> * --demangle=0 (=0 to disable a boolean option) is removed. Omit the
>> option or use --no-demangle instead.
>> * To support boolean options (e.g. --demangle --no-demangle), we
don't
>> need to compare their positions (if (NoDemangle.getPosition() >
>> Demangle.getPosition()) , see llvm-nm.cpp)
>> * grouped short options can be specified with one line
>> `setGroupedShortOptions`, instead of adding cl::Grouping to every short
>> options.
>> * We don't need to add cl::cat to every option and call
>> `HideUnrelatedOptions` to hide unrelated options from --help. The issue
>> would happen with cl::opt tools if linker garbage collection is
disabled or
>> libLLVM-13git.so is used. (See https://reviews.llvm.org/D104363)
>> * If we decide to support binary utility multiplexting (
>> https://reviews.llvm.org/D104686), we will not get conflicting options.
>> An option may have different meanings in different utilities
(especially
>> for one-letter options).
>>
>> *I expect that most users will not observe any difference.*
>>
>> There is a related topic whether we should disallow the single-dash
>> `-long-option` form.
>>
> (Discussed in 2019:
>> https://lists.llvm.org/pipermail/llvm-dev/2019-April/131786.html Accept
>> --long-option but not -long-option for llvm binary utilities)
>> *I'd like to disallow -long-option but may want to do this in a
separate
>> change.*
>>
>
> I'd say definitely do this as a separate change. I expect there'd
be a
> long tail of users after this change ships in an LLVM release, etc, such
> that we may want to undo some amount of it a long time after the change is
> made.
>
Thanks for chiming in.

>
>> The main point is that (1) grouped short options have syntax conflict
>> with one-dash long options. (2) the GNU getopt_long style two-dash long
>> option is much more popular.
>>
>> I can think of potential pushback for some Mach-O specific options,
e.g.
>> nm -arch
>> http://www.manpagez.com/man/1/nm/osx-10.12.6.php says `-arch` has one
>> dash.
>> If such options may have problems, we can keep supporting one dash
forms.
>> With OptTable, allowing one-dash forms for a specific option is easy.
>>
>>
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20210702/165d4bf4/attachment.html>