Mehdi AMINI via llvm-dev
2021-Jul-02 21:03 UTC
[llvm-dev] Binary utilities: switch command line parsing from llvm::cl to OptTable (byproduct: drop -long-option?)
On Fri, Jul 2, 2021 at 1:27 PM Fāng-ruì Sòng <maskray at google.com> wrote:> On Fri, Jul 2, 2021 at 1:10 PM Mehdi AMINI <joker.eph at gmail.com> wrote: > >> >> >> On Fri, Jul 2, 2021 at 12:49 PM Alexandre Ganea < >> alexandre.ganea at ubisoft.com> wrote: >> >>> The API benefits sound nice, though presumably some could be retrofitted >>> to cl::opt if that was the only goal. Side benefits in addition to removing >>> global ctors are nice to have. >>> >>> >>> 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, >>> >>> >>> >>> Nice :) >>> >>> >>> >>> Note that MLIR is using cl::opt without global ctor (we build with >>> `-Werror=global-constructors`). >>> >>> >>> >>> The pattern we use to write a tool with cl::opt and avoid global ctor >>> (and can be used to avoid collision) looks like: >>> https://github.com/llvm/llvm-project/blob/main/mlir/lib/IR/MLIRContext.cpp#L57-L83 >>> >>> >>> >>> The tool that wants to expose the MLIRContext options to the command >>> line calls registerMLIRContextCLOptions() before parsing the command line. >>> >>> Wouldn't this translate directly to LLVM tools as well with some minor >>> refactoring? >>> >>> >>> >>> The same applies to all of the infrastructure in MLIR, passes are >>> registered explicitly, etc. This decouples the "is this code linked in" >>> from "options are loaded" annoying part of the global constructors. >>> >>> >>> >>> -- >>> >>> Mehdi >>> >>> >>> >>> *[Alexandre Ganea] *I think one other issue with cl::opt is that it >>> aggregates the “command-line argument definition” and the “runtime >>> parameter” *de facto* in a single object (unless cl::location is >>> manually specified to every cl::opt). What MLIR does solves the issue >>> mentioned by David, the fact that every tool pulls/initializes every >>> cl::opt out there. However OptTable solves both problems, and makes the >>> entry point thread-safe. >>> >>> >>> >> >> I agree that removing the global state would be great! >> Right now what I see proposed with OptTable (like >> https://reviews.llvm.org/D104889) seems to just address the >> tools-specific options, and the value isn't clear to me for these cases, >> since these options aren't exposed through library entry points. >> I don't quite get right now how OptTable would compose at the LLVM scale? >> Are there examples of libraries exposing pluggable hooks for a tool to >> aggregate multiple libraries' options and expose them on the command line? >> >> > The first message listed: > > * -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) > > To me *these points are all usability issues of cl::opt*. I care about > not exposing unnecessary interfaces so cl::opt accepting the weird -t=d > looks a downside to me. > > --demangle=0 is weird and some llvm/llvm/test tests do use cl::opt options > this way, so we cannot just remove this usage. As a workaround, we could > add a cl::foobar_toggle to a cl::opt to disallow =0. > We would end with more customization for one option, cl::cat (for hiding > unrelated options), cl::foobar_toggle (for disallowing =0), and potentially > others for other ad-hoc tasks. > > I can highlight another thing about the global state of cl::opt => *library > cl::opt and binary utility cl::opt share the same namespace*. > So cl::opt options (usually for debugging or testing) in library code can > end up in a tool's list of command line options. > This is usually undesired (e.g. llvm-objdump --x86-asm-syntax in > https://reviews.llvm.org/D100433). > People may not notice this if they always use -DLLVM_LINK_LLVM_DYLIB=off > and don't use linker garbage collection. >You're not answering my question here, are you? Are you answering to what I mentioned 3 emails before in an answer to David when I wrote "Indeed: it isn't clear to me that these are outright "benefits"."? Because I still don't see clearly how to build something like `opt` with all the pass and the options with OptTable, how does it all compose? Thanks, -- Mehdi -------------- next part -------------- An HTML attachment was scrubbed... URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20210702/14f17d76/attachment.html>
Fāng-ruì Sòng via llvm-dev
2021-Jul-02 21:09 UTC
[llvm-dev] Binary utilities: switch command line parsing from llvm::cl to OptTable (byproduct: drop -long-option?)
On Fri, Jul 2, 2021 at 2:03 PM Mehdi AMINI <joker.eph at gmail.com> wrote:> > > On Fri, Jul 2, 2021 at 1:27 PM Fāng-ruì Sòng <maskray at google.com> wrote: > >> On Fri, Jul 2, 2021 at 1:10 PM Mehdi AMINI <joker.eph at gmail.com> wrote: >> >>> >>> >>> On Fri, Jul 2, 2021 at 12:49 PM Alexandre Ganea < >>> alexandre.ganea at ubisoft.com> wrote: >>> >>>> The API benefits sound nice, though presumably some could be >>>> retrofitted to cl::opt if that was the only goal. Side benefits in addition >>>> to removing global ctors are nice to have. >>>> >>>> >>>> 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, >>>> >>>> >>>> >>>> Nice :) >>>> >>>> >>>> >>>> Note that MLIR is using cl::opt without global ctor (we build with >>>> `-Werror=global-constructors`). >>>> >>>> >>>> >>>> The pattern we use to write a tool with cl::opt and avoid global ctor >>>> (and can be used to avoid collision) looks like: >>>> https://github.com/llvm/llvm-project/blob/main/mlir/lib/IR/MLIRContext.cpp#L57-L83 >>>> >>>> >>>> >>>> The tool that wants to expose the MLIRContext options to the command >>>> line calls registerMLIRContextCLOptions() before parsing the command line. >>>> >>>> Wouldn't this translate directly to LLVM tools as well with some minor >>>> refactoring? >>>> >>>> >>>> >>>> The same applies to all of the infrastructure in MLIR, passes are >>>> registered explicitly, etc. This decouples the "is this code linked in" >>>> from "options are loaded" annoying part of the global constructors. >>>> >>>> >>>> >>>> -- >>>> >>>> Mehdi >>>> >>>> >>>> >>>> *[Alexandre Ganea] *I think one other issue with cl::opt is that it >>>> aggregates the “command-line argument definition” and the “runtime >>>> parameter” *de facto* in a single object (unless cl::location is >>>> manually specified to every cl::opt). What MLIR does solves the issue >>>> mentioned by David, the fact that every tool pulls/initializes every >>>> cl::opt out there. However OptTable solves both problems, and makes the >>>> entry point thread-safe. >>>> >>>> >>>> >>> >>> I agree that removing the global state would be great! >>> Right now what I see proposed with OptTable (like >>> https://reviews.llvm.org/D104889) seems to just address the >>> tools-specific options, and the value isn't clear to me for these cases, >>> since these options aren't exposed through library entry points. >>> I don't quite get right now how OptTable would compose at the LLVM >>> scale? Are there examples of libraries exposing pluggable hooks for a tool >>> to aggregate multiple libraries' options and expose them on the command >>> line? >>> >>> >> The first message listed: >> >> * -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) >> >> To me *these points are all usability issues of cl::opt*. I care about >> not exposing unnecessary interfaces so cl::opt accepting the weird -t=d >> looks a downside to me. >> >> --demangle=0 is weird and some llvm/llvm/test tests do use cl::opt >> options this way, so we cannot just remove this usage. As a workaround, we >> could add a cl::foobar_toggle to a cl::opt to disallow =0. >> We would end with more customization for one option, cl::cat (for hiding >> unrelated options), cl::foobar_toggle (for disallowing =0), and potentially >> others for other ad-hoc tasks. >> >> I can highlight another thing about the global state of cl::opt => *library >> cl::opt and binary utility cl::opt share the same namespace*. >> So cl::opt options (usually for debugging or testing) in library code can >> end up in a tool's list of command line options. >> This is usually undesired (e.g. llvm-objdump --x86-asm-syntax in >> https://reviews.llvm.org/D100433). >> People may not notice this if they always use -DLLVM_LINK_LLVM_DYLIB=off >> and don't use linker garbage collection. >> > > You're not answering my question here, are you? Are you answering to what > I mentioned 3 emails before in an answer to David when I wrote "Indeed: it > isn't clear to me that these are outright "benefits"."? >OptTable doesn't have the listed usability issues. Not having the issues is a large benefit to me.> Because I still don't see clearly how to build something like `opt` with > all the pass and the options with OptTable, how does it all compose? >The proposed changes are specific to binary utilities: llvm-{ar,cxxfilt,nm,objcopy,objdump,readobj,size,strings,symbolizer}. 'opt', 'llc', etc are not in the scope. (I guess I should have named the utilities more specifically to not cause confusion.) For llvm-{ar,cxxfilt,nm,objcopy,objdump,readobj,size,strings,symbolizer}, we don't want a random cl::opt in lib/Transforms, lib/MC, orlib/LTO to be accidentally specifiable on the command line. (In the rare debugging case where such functionality is needed, it needs a -mllvm prefix like what ld.lld does.) -------------- next part -------------- An HTML attachment was scrubbed... URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20210702/e2fef9c6/attachment.html>