[PATCH] D105881: [flang][driver] Switch to `BoolFOption` for boolean options

Mon Aug 2 04:38:16 PDT 2021

jansvoboda11 added a comment.

In D105881#2906955 <https://reviews.llvm.org/D105881#2906955>, @awarzynski wrote:

> OK, that's fair enough. But what about the output from `clang --help` and `clang -cc1 --help`? Let's look at -feliminate-unused-debug-types <https://github.com/llvm/llvm-project/blob/2e27c4e1f187446c84220f75e492f16807d21b12/clang/include/clang/Driver/Options.td#L1392-L1393> (the only instance of `OptOutFFLag`):
>
>   # Only -fno-eliminate-unusued-debug-types is printed
>   clang -cc1 --help | grep eliminate-unused
>     -fno-eliminate-unused-debug-types
>   # Both -fno-eliminate-unusued-debug-types and -feliminate-unusued-debug-types printed?
>   clang --help | grep eliminate-unused
>     -feliminate-unused-debug-types
>     -fno-eliminate-unused-debug-types
>
> To me this behavior is counter-intuitive. So are these `clang` options? `clang -cc1` options? Both? What's the point of adding `CC1Option` if both `clang` and `clang -cc1` accept them. The definition of `CC1Option` from Options.td:
>
>   // CC1Option - This option should be accepted by clang -cc1.
>   def CC1Option : OptionFlag;
>
> OK, so currently `-fno-eliminate-unused-debug-types` is a `CC1Option` and _should_ be accepted by `clang -cc1`. How about `-feliminate-unused-debug-types`?
>
>   clang -cc1 -feliminate-unused-debug-types file.c
>   error: unknown argument: '-feliminate-unused-debug-types'
>
> Makes sense, but:
>
>   clang -c -feliminate-unused-debug-types file.c
>   clang-13: warning: argument unused during compilation: '-feliminate-unused-debug-types' [-Wunused-command-line-argument]
>
> Shouldn't it be rejected by `clang` with an error as well? Shouldn't `CC1Option` be used consistently for `-feliminate-unused-debug-types`  and `-fno-eliminate-unused-debug-types`?
>
>>> In fact, only the contents of `clang -cc1 --help` are being tweaked by using `OptInFFlag/OptOutFFlag`.
>>
>> That's incorrect, `OptInFFlag` and `OptOutFFlag` both set `HelpText` for the positive and negative driver flags.
>
> Yes, but in `clang --cc1 --help`, only the variant with `CC1Option` is being printed and the other one is ignored. So why bother adding the other one?
>
> Btw, now I see that `-cc1` does reject `-feliminate-unused-debug-types`, so it's not only about the contents of `clang -cc1 --help`. I was wrong, sorry about that and thanks for pointing this out!

The `clang` driver always accepts both the positive and negative options, and it always only considers the argument that appeared last on the command-line (using `ArgList::getLastArg(Pos, Neg)`). This allows users to take a random `clang` driver command-line, append `-feliminate-unused-debug-types` and get the desired behavior, even though the original command-line might have contained `-fno-eliminate-unused-debug-types`.

For the pair of options you mentioned, the `clang` driver knows they are "opt-out" in `clang -cc1`. The behavior is enabled by default in `-cc1` and can be disabled/opted-out through the negative `-fno-eliminate-unused-debug-types`. The positive option is **not** accepted by `clang -cc1`, since explicitly enabling a behavior that's already enabled by default is redundant.

If driver sees `-fno-eliminate-unused-debug-types` as the last option, it will forward it to `clang -cc1`. If it sees `-feliminate-unused-debug-types`, it will ignore it.

Does this make sense?

If neither the positive nor negative options have `CC1Option`, they are driver-only options. If you add `CC1Option` to both, both are accepted by `clang -cc1`. But the most common scenario is that only one of the two options is marked with `CC1Option`.

The `HelpText` doesn't play a role in option being accepted or not in `clang` or `clang -cc1`.

> **Content of `--help`**
>
>>> In Flang, we rely on the fact that "no help text? don't include it in `flang-new -fc1 --help`" instead. This behavior is implemented in `clangDriver` and applies to all drivers that use it (so this is nothing specific to Flang).
>>
>> I see. Is this intentional?
>>
>>> This way, we can always mark Flang options with the relevant flags (e.g. `FC1Option`). In other words, we can claim them and make it clear that other drivers can ignore them.
>>
>> I don't understand this. Can you elaborate? Are you relying on the absence/presence of a help text to mark flags with `FC1Option`? When does this happen? (In `Options.td`? The TableGen backend? The consumer of `Options.inc`?)
>
> See the implementation of printHelp <https://github.com/llvm/llvm-project/blob/2e27c4e1f187446c84220f75e492f16807d21b12/llvm/lib/Option/OptTable.cpp#L627-L671> from OptTable.cpp. Basically: "No help text? Not printing". We rely on this **not to print** options supported by Flang, but for which we have no help text. We only used that for boolean options (so we have a help text for e.g. `-ffoo`, but not for `-fno-foo`). This is a minor thing and I am not concerned about loosing it.

Okay, this makes sense, thanks! I think we're in agreement that the current distinction between `HelpText<"">` and no `HelpText` is not particularly useful when printing help.

> As for `FC1Option` (and `FlangOption`), we always add these explicitly in Options.td. This helps to identify options that are either shared or Flang-only and to avoid polluting `clang --help` with Flang/Fortran specific options. And that's what we rely on when calling printHelp <https://github.com/llvm/llvm-project/blob/2e27c4e1f187446c84220f75e492f16807d21b12/flang/lib/FrontendTool/ExecuteCompilerInvocation.cpp#L106-L110>.
>
>> I'd need to double-check if we particularly care about showing/hiding flags without help text.
>
> We should. `clang --help` should not be printing any Flang options, and vice-versa.

I agree we shouldn't mix Clang and Flang options together in `-help`. But to make sure we're on the same page: this doesn't have to do anything with the presence/absence of `HelpText`, this is controlled by `FlangOption`, `FC1Option`, `CC1Option` etc.

> I'm under the impression that the contents of `clang --help` and `clang --cc1 --help` got a bit out of control. We did discuss this briefly on cfe-dev <https://lists.llvm.org/pipermail/cfe-dev/2020-October/066953.html>. I guess that with ~1000 options supported by Clang, this is rarely noticed. Does anyone use `clang --help` these days? But in Flang, we have only ~50 options and if we are not careful, `flang --help` would be printing hundreds of options that are not supported (and in fact, will never be).
>
> We try to be strict about what is and what is not included in `flang --help` as that's currently our main end-user-facing documentation. We do this by adding a Flang flag (e.g. `FlangOption` or `FC1Option`) to every option that we support. Options that don't contain flags (e.g. `feliminate-unused-debug-types`) are very problematic for us. What is Flang meant to do with such options? This is an issue for every tool that relies on Options.td. This also works the other way - if we don't flag Flang options, `clang --help` and `clang -cc1 --help` will start printing options added by us.  Same mechanism applies to auto-completion for command line options or for finding suggestions when an option is misspelled.
>
> **Splitting Options.td**
>
>>> In general, I think that Options.td would become a bit cleaner and easier to reason about if all options specified all their flags. That wouldn't be easy to achieve though!
>>
>> I think that the way we define flags for the Clang driver and `-cc1` in one go is already bit confusing. But it's convenient, so we roll with it.
>
> I find it very hard to reason about things in Options.td. IMHO, we should have a separate Options.td for generic driver options (shared between Clang and Flang), and then separate Options.td for `cc1` and `fc1`. This was brought up when we discussed implementing the Flang driver in terms of `clangDriver`. It would help in forcing every driver to clearly define what options it supports. Also, the long term goal is to move `clangDriver` out of Clang, at which point that's going to be required anyway.

It's cool to see people are thinking about improving `Options.td`!

> IMO, `OptInFFlang`/`OptOutFFlag` are generic enough and should be shared.
>
> **Next steps**
>
>>> It would be nicer to have the `opt-in`/`opt-out` semantics for Flang in TableGen through something similar to `BoolFOption`. No need for this just now - one step at a time :)
>>
>> I suggest to find another means of achieving the semantics you want. You can always copy the `BoolFOption` implementation and remove the marshalling stuff, if that's really the semantics you want.
>
> The updated `OptInFFlang`/`OptOutFFlag` work just fine for us.  Flang does not need `EmptyKPM` and `BoolFOption` just now. I'm not suggesting any changes here.

Ah, okay. Can you remove `EmptyKPM` from this patch then?

> As there's only a handful of options using `OptInFFlang`/`OptOutFFlag` right now, I was hoping that the current revision would be non-controversial. We definitely need to use `FC1Option` (or other Flang flag) for both `-ffoo` and `-fno-foo`. Otherwise, `clang --help` will start printing/supporting the version without a flag. If adding a flag to both variants is not acceptable in `-cc1`, then I suggest splitting `OptInFFlag`/`OptOutFFlag` into
>
> - `OptInCC1FFlang`/`OptOutCC1FFlag`, and
> - `OptInFC1FFlang`/`OptOutFC1FFlag`.

Agreed. What was controversial from my point of view was the addition of `EmptyKPM` and the wording that I interpreted as "the content of `HelpText` influences whether a flag is accepted by driver or the front-end".

> As for the help text, what would be best for Clang and `-cc1`? Add it to both `-ffoo` and `-fno-foo`? Just one of them?

Add it to both TableGen definitions. Whether each of the options will be printed only in `clang -help` or also `clang -cc1 -help` is determined by the presence of `CC1Option`.

> Thanks for reviewing this and all your comments! I hope that we can figure something out that will work for both Clang and Flang.

No problem!

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D105881/new/

https://reviews.llvm.org/D105881