They otherwise show up in the --help output of some llvm tools, e.g. llvm-bcanalyzer. This seems to only happen when the tools are linked against the shared llvm dylib.
New version with the proposed way of using cl::HideUnrelatedOptions() instead.
Unfortunately this makes the changes difficult in some of the tools since they declare cl::opts in files other than their main file.
This isn't an issue for the static linking approach, so why does it happen for the dynamic library approach? It seems like fixing this aspect should be the way forward, as it will be more robust generally.
The options are all declared as global variables and called from __cxx_global_var_init. I was assuming that the linker removes the unused command line options when linking against llvm statically but leaves all of them in when linking against the libLLVM.so.
IMO this is simply a clash between "user-facing command line options" and "internally used command line options, for tests and benchmarking". I was trying to mark the internally used ones as auch by making them cl::Hidden.
The email thread about the llvm busybox implementation contains this paragraph:
One implementation detail we think will be an issue is merging arguments in individual tools that use cl::opt. cl::opt works by maintaining a global state of flags, but we aren’t confident of what the resulting behavior will be when merging them together in the dispatching main. What we would like to avoid is having flags used by one specific tool available on other tools. To address this issue, we would like to migrate all tools to use OptTable which doesn't have this issue and has been the general direction most tools have been already moving into.
So, should all tools use OptTable instead?
This seems to only happen when the tools are linked against the shared llvm dylib.
How many options are displayed depends on linker garbage collection.
With the default -DLLVM_LINK_LLVM_DYLIB=off, --gc-sections can garbage collection almost all code generation/middle-end optimization options, so --help looks clean.
With -DLLVM_LINK_LLVM_DYLIB=on, cl::opt (and its friends) options are all retained in libLLVM-13git.so, so a tool linking against libLLVM-13git.so may see many unrelated options.
Adding cl::cat(ToolOptions) annotations does solve the problem, so LGTM. For some tools like llvm-mc, they can use arbitrary MC layer cl::opt options.
Some tests may use some obscure options.
IIUC cl::HideUnrelatedOptions(&ToolOptions); will make the options cl::ReallyHidden which will disappear from --help-all-hidden. If any tool turns out to want the obscure options, we can think of other options or just dropping HideUnrelatedOptions for the particular tool.
Many llvm/tools/* tools are really internal testing tools. Some are user-facing tools (llvm-objdump, llvm-readelf, llvm-objcopy, llvm-nm, ...).
For user-facing tools, I think it makes sense to convert them to OptTable. cl::opt can emulate GNU style getopt_long which we may be familiar with but there are some rough corners (see the description of D83530).
Some test changes aren't correct, so requesting changes.
This isn't a correct change.
Before, the intent was that the LIST-NOT checks showed that each of the checked-for categories appeared in --help output, but not --help-hidden.
The checks should be:
CATEG: Generic Options: LIST-NOT: Generic Options: CATEG: Tool Options: LIST-NOT: Tool Options:
Additionally, you can add --implicit-check-not="General options:" to the --help FileCheck, to show that the "unrelated" options are not present in the output.
Finally, I don't know if it's still present, but in the llvm-strings copy I have installed on my machine, there's a "Color Options:" category that is also listed in the help text. It should probably be tested here too.
In other tools, we either do, or used to at least, call the tool-specific option category after the tool name. Here, that would mean something like BCAnalyzerOptions("BC Analyzer Options").
It may also be a good idea to add testing similar to the llvm-strings test to show that only the expected options categories are present.
The same comments apply to each of the other tools.
In other cases, you've added the option category to positionals as well. I don't know if you need to, but we should be consistent either way, and either remove it from all the others, or add it here.
There are a load more options here that seem like they syhould have a category?
Seems like you don't need the option category here at all?
Oh, one other thing: does HideUnrelatedOptions hide the --color option (which is in its own category)? This option should be in the help text usually, because error/warning messages at least are printed with colours usually.
Yes, it also hides the --color option. I guess I need to include &llvm::ColorCategory everywhere.
But this all seems like a lot of work in all the tools (not to mention external ones linking against libLLVM.so I guess?) for something that should just work. Why not mark all the llvm internal options as cl::Hidden instead? That way thy would also still be available via --help-hidden.
I don't think that's particularly useful, and at the very least needs discussing on the mailing lists. LLVM developers may well want to know about some of these options when using tools like opt and llc, and hiding them means newer LLVM develpers may never find them.
Where you've added the ColorCategory, how sure are you that it actually has an impact on the tool you've modified. My suspicion is that in many cases it actually has no effect whatsoever, as the tool in question never uses the WithColor code. Please make sure you add it only where the option has some visible effect.
This still isn't correct for the code this patch is based on.
That being said, @MaskRay has just landed a patch to completely redo llvm-strings command-line handling, so it's moot - please rebase.
It seems like this part of my comment has been missed?
Seems like this is only tangentially related to this patch. Please move it to another patch. Also check to see if --color actually should appear in the otuput - if it has no impact, it shouldn't.
Same as above. Don't add this in this patch.
Here and in many other cases, we're already using namespace llvm, so you shouldn't need the llvm prefix before ColorCategory.
LTO is an abbreviation, so should be capitalized in the variable name too (i.e. LTOOptions).