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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
