From 95114f21f5bf1704672dadb45ca7c25efca72e03 Mon Sep 17 00:00:00 2001 From: Jan Svoboda Date: Tue, 8 Dec 2020 18:15:21 +0100 Subject: [clang][cli] Do not marshall only CC1Option flags in BoolOption We cannot be sure whether a flag is CC1Option inside the definition of `BoolOption`. Take the example below: ``` let Flags = [CC1Option] in { defm xxx : BoolOption<...>; } ``` where TableGen applies `Flags = [CC1Option]` to the `xxx` and `no_xxx` records **after** they have been is fully declared by `BoolOption`. For the refactored `-f[no-]debug-pass-manager` flags (see the diff), this means `BoolOption` never adds any marshalling info, as it doesn't see either of the flags as `CC1Option`. For that reason, we should defensively append the marshalling information to both flags inside `BoolOption`. Now the check for `CC1Option` needs to happen later, in the parsing macro, when all TableGen logic has been resolved. However, for some flags defined through `BoolOption`, we can run into issues: ``` // Options.td def fenable_xxx : /* ... */; // Both flags are CC1Option, the first is implied. defm xxx : BoolOption<"xxx, "Opts.Xxx", DefaultsToFalse, ChangedBy, ResetBy>; ``` When parsing `clang -cc1 -fenable-xxx`: * we run parsing for `PosFlag`: * set `Opts.Xxx` to default `false`, * discover `PosFlag` is implied by `-fenable-xxx`: set `Opts.Xxx` to `true`, * don't see `-fxxx` on command line: do nothing, * we run parsing for `NegFlag`: * set `Opts.Xxx` to default `false`, * discover `NegFlag` cannot be implied: do nothing, * don't see `-fno-xxx` on command line: do nothing. Now we ended up with `Opts.Xxx` set to `false` instead of `true`. For this reason, we need to ensure to append the same `ImpliedByAnyOf` instance to both flags. This means both parsing runs now behave identically (they set the same default value, run the same "implied by" check, and call `makeBooleanOptionNormalizer` that already has information on both flags, so it returns the same value in both calls). The solution works well, but what might be confusing is this: you have defined a flag **A** that is not `CC1Option`, but can be implied by another flag **B** that is `CC1Option`: * if **A** is defined manually, it will never get implied, as the code never runs ``` def no_signed_zeros : Flag<["-"], "fno-signed-zeros">, Group, Flags<[]>, MarshallingInfoFlag<"LangOpts->NoSignedZero">, ImpliedByAnyOf<[menable_unsafe_fp_math]>; ``` * if **A** is defined by `BoolOption`, it could get implied, as the code is run by its `CC1Option` counterpart: ``` defm signed_zeros : BoolOption<"signed-zeros", "LangOpts->NoSignedZero", DefaultsToFalse, ChangedBy, ResetBy, "f">, Group; ``` This is a surprising inconsistency. One solution might be to somehow propagate the final `Flags` of the implied flag in `ImpliedByAnyOf` and check whether it has `CC1Option` in the parsing macro. However, I think it doesn't make sense to spend time thinking about a corner case that won't come up in real code. An observation: it is unfortunate that the marshalling information is a part of the flag definition. If we represented it in a separate structure, we could avoid the "double parsing" problem by having a single source of truth. This would require a lot of additional work though. Note: the original patch missed the `CC1Option` check in the parsing macro, making my reasoning here incomplete. Moreover, it contained a change to denormalization that wasn't necessarily related to these changes, so I've extracted that to a follow-up patch: D93094. Reviewed By: dexonsmith, Bigcheese Differential Revision: https://reviews.llvm.org/D93008 --- clang/lib/Frontend/CompilerInvocation.cpp | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) (limited to 'clang/lib/Frontend/CompilerInvocation.cpp') diff --git a/clang/lib/Frontend/CompilerInvocation.cpp b/clang/lib/Frontend/CompilerInvocation.cpp index 23c4ad8..1a863a7 100644 --- a/clang/lib/Frontend/CompilerInvocation.cpp +++ b/clang/lib/Frontend/CompilerInvocation.cpp @@ -869,10 +869,6 @@ static bool ParseCodeGenArgs(CodeGenOptions &Opts, ArgList &Args, InputKind IK, } } - Opts.DebugPassManager = - Args.hasFlag(OPT_fdebug_pass_manager, OPT_fno_debug_pass_manager, - /* Default */ false); - if (Arg *A = Args.getLastArg(OPT_fveclib)) { StringRef Name = A->getValue(); if (Name == "Accelerate") @@ -3767,7 +3763,7 @@ bool CompilerInvocation::parseSimpleArgs(const ArgList &Args, HELPTEXT, METAVAR, VALUES, SPELLING, ALWAYS_EMIT, KEYPATH, DEFAULT_VALUE, \ IMPLIED_CHECK, IMPLIED_VALUE, NORMALIZER, DENORMALIZER, MERGER, EXTRACTOR, \ TABLE_INDEX) \ - { \ + if ((FLAGS)&options::CC1Option) { \ this->KEYPATH = MERGER(this->KEYPATH, DEFAULT_VALUE); \ if (IMPLIED_CHECK) \ this->KEYPATH = MERGER(this->KEYPATH, IMPLIED_VALUE); \ -- cgit v1.1