Page MenuHomePhabricator

[llvm] [tools] Hide unrelated options from all tools
AbandonedPublic

Authored by tbaeder on Jun 16 2021, 1:29 AM.

Details

Summary

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.

Diff Detail

Event Timeline

tbaeder created this revision.Jun 16 2021, 1:29 AM
tbaeder requested review of this revision.Jun 16 2021, 1:29 AM
Herald added a project: Restricted Project. · View Herald TranscriptJun 16 2021, 1:29 AM

I don't believe this is correct.
This is either not a bug, r should be dealt with by adding grouping, and hiding non-tool cl::opts.

What do you mean by "hiding non-tool cl::opts"? That is what I'm trying to do of course.

This revision now requires changes to proceed.Jun 16 2021, 4:55 AM
ormris removed a subscriber: ormris.Jun 16 2021, 11:43 AM
tbaeder updated this revision to Diff 352631.Jun 17 2021, 12:40 AM

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.

tbaeder updated this revision to Diff 352941.Jun 18 2021, 2:08 AM

@lebedev.ri Is this more like what you imagined?

Something like this yes.
I think someone attempted this previously, and i don't recall why that didn't proceed.
It may be better to do this per-tool.

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?

MaskRay accepted this revision.Jun 23 2021, 10:48 AM

This seems to only happen when the tools are linked against the shared llvm dylib.

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.

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.

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?

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).

jhenderson requested changes to this revision.Jun 24 2021, 12:55 AM

Some test changes aren't correct, so requesting changes.

llvm/test/tools/llvm-strings/help.test
12–15

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.

llvm/tools/llvm-bcanalyzer/llvm-bcanalyzer.cpp
40–45

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.

llvm/tools/llvm-cxxfilt/llvm-cxxfilt.cpp
65–66

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.

llvm/tools/llvm-lto/llvm-lto.cpp
111

There are a load more options here that seem like they syhould have a category?

llvm/tools/llvm-xray/llvm-xray.cpp
26

Seems like you don't need the option category here at all?

This revision now requires changes to proceed.Jun 24 2021, 12:55 AM

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.

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.

tbaeder updated this revision to Diff 354221.Jun 24 2021, 5:22 AM
tbaeder updated this revision to Diff 356137.Fri, Jul 2, 2:44 AM
tbaeder retitled this revision from [llvm] Mark more internal command line optins as cl::Hidden to [llvm] [tools] Hide unrelated options from all tools.
andreadb added inline comments.Fri, Jul 2, 4:48 AM
llvm/tools/llvm-mca/llvm-mca.cpp
71–76 ↗(On Diff #356137)

Is this change (and the other changes to this file) really needed?

If not, then I'd like those to be removed from this patch.

Thanks
-Andrea

tbaeder updated this revision to Diff 356162.Fri, Jul 2, 6:16 AM

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.

llvm/test/tools/llvm-strings/help.test
12–15

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.

llvm/tools/llvm-bcanalyzer/llvm-bcanalyzer.cpp
40–45

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.

It seems like this part of my comment has been missed?

llvm/tools/llvm-extract/llvm-extract.cpp
143

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.

llvm/tools/llvm-gsymutil/llvm-gsymutil.cpp
472

Same as above. Don't add this in this patch.

llvm/tools/llvm-ifs/llvm-ifs.cpp
454

Here and in many other cases, we're already using namespace llvm, so you shouldn't need the llvm prefix before ColorCategory.

llvm/tools/llvm-lto/llvm-lto.cpp
67

LTO is an abbreviation, so should be capitalized in the variable name too (i.e. LTOOptions).

tbaeder abandoned this revision.Thu, Jul 8, 12:34 AM
tbaeder marked an inline comment as done.

I'm just gonna abandon this. The move seems to be to use OptTable instead, and rebasing this every day doesn't make much sense.

I'm just gonna abandon this. The move seems to be to use OptTable instead, and rebasing this every day doesn't make much sense.

@MaskRay - could you state which tools you plan on moving over?

(My suspicion is that it's only a subset of the tools touched by this patch)

I'm just gonna abandon this. The move seems to be to use OptTable instead, and rebasing this every day doesn't make much sense.

@MaskRay - could you state which tools you plan on moving over?

(My suspicion is that it's only a subset of the tools touched by this patch)

The user-facing binary utilities llvm-{cov,cxxfilt,nm,readobj,size,strings} Most are done now.

The patch has a small intersection with the tools.