This is an archive of the discontinued LLVM Phabricator instance.

[llvm/OptTable] Print options without documentation
Needs ReviewPublic

Authored by j0le on Jul 27 2023, 7:34 AM.

Details

Summary

When printing the help (most likely triggered by the tool’s option
"--help"), then list also those options, that don’t have a
documentation, i.e. no help text. That makes those options more
discoverable. The user is then able to search online, for example in an
alternate, non-official documentation, for example in the GCC or MSVC
documentaion in case of Clang.

An example of an option without help text is Clang’s option "-static",
which is well established, but couldn’t be found previously in the help
output.

If we want to hide specific options without help text, we should flag
them with llvm::opt::DriverFlag::HelpHidden, instead of relying on them
being hidden, because they have no help text.

With these changes:

$ ./clang.exe --help | wc -l
2451

$ ./clang.exe --help-hidden | wc -l
2556

Without these changes (on commit 2854852f4f0f):

$ ./clang.exe --help | wc -l
1358

$ ./clang.exe --help-hidden | wc -l
1427

Diff Detail

Event Timeline

j0le created this revision.Jul 27 2023, 7:34 AM
Herald added a project: Restricted Project. · View Herald TranscriptJul 27 2023, 7:34 AM
Herald added a subscriber: hiraditya. · View Herald Transcript
j0le requested review of this revision.Jul 27 2023, 7:34 AM
Herald added a project: Restricted Project. · View Herald TranscriptJul 27 2023, 7:34 AM

Haven't looked closely(on my phone), but the new behavior makes sense. We probably need a unit test in llvm.

j0le added a comment.Jul 27 2023, 1:55 PM

Haven't looked closely(on my phone), but the new behavior makes sense.

I'm glad to hear that ☺.

We probably need a unit test in llvm.

Seems like we already have a unit test for Flang:

Failed Tests (2):
  Flang :: Driver/driver-help-hidden.f90
  Flang :: Driver/driver-help.f90

Sorry, that I didn't noticed earlier. I will look into it tomorrow.

I said “unit test for Flang”, but is it a unit test or regression test?

LLVMOption used to be part of Clang and certain features are heavily coupled with Clang Driver behaviors.
Ideally we should have llvm/ tests (llvm/unittests/Option, or somewhere in llvm/test) for better layering (https://maskray.me/blog/2021-08-08-toolchain-testing#the-test-checks-at-the-wrong-layer), but if not applicable, somewhere in clang/unittests/Driver is fine as well.
Many contributors don't build flang and consider clang more "core", so having clang tests seems important if we don't have a good place in llvm/

llvm/include/llvm/Option/OptTable.h
260–261
j0le updated this revision to Diff 545142.Jul 28 2023, 7:02 AM

I added the missing undocumented options of Flang to the tests:

  • flang/test/Driver/driver-help-hidden.f90
  • flang/test/Driver/driver-help.f90

For the most part that are options starting with "-fno-".

I also changed quite a lot of white space in these tests, so that the help texts are aligned. I don't know if that's good or bad.
With git show --ignore-all-space one can see the relevant lines.

Furthermore I remove the comma, as suggested by @serge-sans-paille

j0le added a comment.Jul 28 2023, 7:21 AM

Now that the Flang tests are fixed, I can think about how to write the unit tests.

Should we add a little executable, that uses the Option support library and has all sorts of options (hidden, aliases, long, short, ...)? The output of that executable can then be checked by FileCheck. Or should we use gtest.h? (I guess that's Google Test.)

I see there is already llvm/unittests/Option/Opts.td. Maybe that can be reused.

I believe checking with FileCheck is the easiest.

flang/test/Driver/driver-help-hidden.f90
102

How do check for the end of file (EOF) with FileCheck? I didn't find anything better that CHECK-EMPTY in https://www.llvm.org/docs/CommandGuide/FileCheck.html . Maybe we should use a special regex.

The reason, why we should check for EOF is, that we don't want to miss new options that start with a letter at the end of the alphabet.

j0le marked an inline comment as done.Jul 28 2023, 7:26 AM
awarzynski added inline comments.Jul 28 2023, 7:54 AM
flang/test/Driver/driver-help-hidden.f90
51

IMHO, printing (no help text for the -fno-<option> variant):

-fno-backslash

on top of:

-fbackslash   Specify that backslash in string introduces an escape character

is of little value. To me this is noise that we should avoid. If both -fbackslash and -fno-backslash are printed, then both should contain "help" text.

@j0le, thanks for caring about --help - it could really benefit from a bit of love. However, I'm not convinced that this is the right direction:

With these changes:

$ ./clang.exe --help | wc -l
2451

$ ./clang.exe --help-hidden | wc -l
2556

We should focus on quality rather than the quantity of the help text being printed. Personally, I find clang --help of little help because it is just a very long dump of unfiltered/uncategorized text. As pointed out in my inline comment, it's perfectly fine to skip some options from the --help dump if they don't really contribute any new information. However, categorizing options so that clang --help would greatly improve the user experience. At least for Clang. In the case of Flang the list of supported options is still very short and --help is quite "manageable".

j0le added inline comments.Jul 28 2023, 8:54 AM
flang/test/Driver/driver-help-hidden.f90
51

I agree, it is of little value. But the value is, that the user now knows, that she/he/they/... can toggle of the -fbackslash option explicitly. Maybe some wrapper around flang (like a build system), that the user has little control of, sets the option -fbackslash, then the user can turn it of with -fno-backslash.

But if we realy want to hide -fno-backslash and other -fno-<option> options, but still show options that have no help text (but don't have a related negated option), we can come up with some ideas.

But first let's analyze the implementation:
If I understand this correctly, the flang options also come from the clang source tree, i.e. from the file "clang/include/clang/Driver/Options.td".

Both -fbackslash and -fno-backslash are defined with this line:

defm backslash : OptInFC1FFlag<"backslash", "Specify that backslash in string introduces an escape character">;

That corresponds to the following lines in the generated "Options.inc" file:

OPTION(prefix_1, llvm::StringLiteral("fbackslash"), fbackslash, Flag, f_Group, INVALID, nullptr, FC1Option | FlangOption | FlangOnlyOption, 0, "Specify that backslash in string introduces an escape character", nullptr, nullptr)
OPTION(prefix_1, llvm::StringLiteral("fno-backslash"), fno_backslash, Flag, f_Group, INVALID, nullptr, FC1Option | FlangOption | FlangOnlyOption, 0, "", nullptr, nullptr)

I don't totally understand how this table generation works. And I don't know how we can tell (programmatically) from this table that fbackslash and fno_backslash are related. But maybe there is a way, or we have to change the table generation.

Possible solutions:

  1. For OptInFC1FFlag the "-fno-..." options are marked as hidden.
  2. We detect in OptTable::printHelp that those options are related and group them together
  3. A help text for the "-fno-..." option is generated from help text of "-f..." option. Either in table generation or in C++ code.
j0le added a comment.Jul 28 2023, 9:35 AM

@j0le, thanks for caring about --help - it could really benefit from a bit of love. However, I'm not convinced that this is the right direction:

[...]

We should focus on quality rather than the quantity of the help text being printed.
Personally, I find clang --help of little help because it is just a very long dump of unfiltered/uncategorized text. As pointed out in my inline comment, it's perfectly fine to skip some options from the --help dump if they don't really contribute any new information. However, categorizing options so that clang --help would greatly improve the user experience. At least for Clang. In the case of Flang the list of supported options is still very short and --help is quite "manageable".

Yeah, I would also find it better, if we grouped the options some how. Maybe I will find some time for that some day, because this a little bit more work than this small patch.

I'm fine with discarding this patch, because I can still search for options in https://clang.llvm.org/docs/ClangCommandLineReference.html (But it is a little bit broken at the end.)
But I wanna say: The user might be fooled and think the output of "--help" is complete, or identical to https://clang.llvm.org/docs/ClangCommandLineReference.html, but that's not the case.

Thanks for your feedback, @awarzynski!

I agree that displaying all options will be useful. And with OptInFC1FFlag, displaying just the negative option in --help will be nice.

lichray has a point that (a) this allows determining whether an option is accepted by a specific version of Clang and (b) without a string like "(Undocumented)", an option without a message can be confused with a preceding option.

flang/test/Driver/driver-help.f90
17

Thanks for the reply and apologies for the delay.

@j0le, thanks for caring about --help - it could really benefit from a bit of love. However, I'm not convinced that this is the right direction:

[...]

We should focus on quality rather than the quantity of the help text being printed.
Personally, I find clang --help of little help because it is just a very long dump of unfiltered/uncategorized text. As pointed out in my inline comment, it's perfectly fine to skip some options from the --help dump if they don't really contribute any new information. However, categorizing options so that clang --help would greatly improve the user experience. At least for Clang. In the case of Flang the list of supported options is still very short and --help is quite "manageable".

Yeah, I would also find it better, if we grouped the options some how. Maybe I will find some time for that some day, because this a little bit more work than this small patch.

This would be a non-trivial task - you'd need to "properly" categorise all options in Options.td. But, IMHO, that would be hugely beneficial to the community.

But I wanna say: The user might be fooled and think the output of "--help" is complete, or identical to https://clang.llvm.org/docs/ClangCommandLineReference.html, but that's not the case.

Yes, but I wouldn't put -f<option and -fno-<option> into this category. Folks usually know that often both forms are possible. As for -static specifically, you could simply add a HelpText. In fact, your patch should allow you to identify all options for which a HelpText is missing.

I'm fine with discarding this patch,

That would be my suggestion. I really appreciate the effort and, in general, Clang/Flang would greatly benefit from somebody taking greater care of Options.td. However, I don't believe that this patch is the direction that we should be taking. At least not without some additional refactoring that would take care of duplication in the --help output (printing -fbackslash and -fno-backslash is, IMHO, duplication).

If you do decide to purse this further, I will happily review your TableGen changes.

flang/test/Driver/driver-help-hidden.f90
51

But the value is, that the user now knows, that she/he/they/... can toggle of the -fbackslash option explicitly.

But to document that, do we really need:

-fno-backslash
-fbackslash   Specify that backslash in string introduces an escape character

? Why not something like this:

-fbackslash   Specify that backslash in string introduces an escape character (default)

And then, somewhere in the documentation:

Options with default values (marked with `(default)` at the end of their help text) support both positive (`-f<option>`) and negative (`-fno-<option>`) versions.

Or something similar.

With things like this it's often very helpful to see what GNU do (https://gcc.gnu.org/onlinedocs/gcc/Invoking-GCC.html):

Many options have long names starting with ‘-f’ or with ‘-W’—for example, -fmove-loop-invariants, -Wformat and so on. Most of these have both positive and negative forms; the negative form of -ffoo is -fno-foo. This manual documents only one of these two forms, whichever one is not the default.

Following GNU would the path of least surprises :)

If I understand this correctly, the flang options also come from the clang source tree, i.e. from the file "clang/include/clang/Driver/Options.td".

Yes, Flang's driver is implemented in terms of Clang's clangDriver library and these options are defined within that library. Historically, only Clang would use it. However, with the introduction of Flang, that library has gained a new client - Flang's compiler driver :) This helps us create more consistence experience for end users (there are many other benefits too). Note that the long term plan is to extract the driver library out of Clang.

I don't totally understand how this table generation works.

It's a bit tricky, yes :) Have you checked whether this is a problem specific to Flang options? I would imagine that your change would affect Clang options in a similar way. It just happens that for Flang it's more visible as there's fewer options.

Possible solutions:

Like hinted above, I suggest that in cases like this, instead of printing two options:

-fno-backslash <potentially some help text>
-fbackslash   Specify that backslash in string introduces an escape character

Clang and Flang print only one:

-fbackslash   Specify that backslash in string introduces an escape character (perhaps some extra marker here?)

As for your suggestions:

  • Option 1. makes sense to me, though it would pollute -help-hidden. You could, though, use some other flag instead of HelpHidden.
  • Option 3 means duplication that I would rather avoid.
  • Not sure about Option 2 without seeing a prototype/PoC.

And I don't know how we can tell (programmatically) from this table that fbackslash and fno_backslash are related.

I don't believe they are.

But maybe there is a way, or we have to change the table generation.

There's always a way :) I think that in this case it would require a bit of design work, but nothing too extensive.

MaskRay added 1 blocking reviewer(s): awarzynski.Aug 30 2023, 11:06 PM