[clang][cli] Prevent double denormalization

[jansvoboda11]

Summary

If both flags created through BoolOption are CC1Option and the keypath has a non-default or non-implied value, the denormalizer gets called twice. If the denormalizer has the ability to generate both flags, we can end up generating the same flag twice.

[jansvoboda11]

clang/unittests/Frontend/CompilerInvocationTest.cpp

272

Is it wise to rely on pointer comparison here? The call to count returns 2 before changing the denormalizer and 1 after, but I'm not sure if it will work on all platforms.

[jansvoboda11]

Rebase

[dexonsmith]

clang/unittests/Frontend/CompilerInvocationTest.cpp

272

Does this compile / avoid the pointer comparison?

ASSERT_EQ(count(GeneratedArgs, StringRef("-fdebug-pass-manager")), 1);

[jansvoboda11]

clang/unittests/Frontend/CompilerInvocationTest.cpp

272

Yes, this forces the const char * from GeneratedArgs to be converted into StringRef, which does comparison via ::memcmp.

If we decide comparing StringRefs is a better solution, I'd be inclined to create a helper function or custom matcher that does the conversion to StringRef. That way, we don't have to worry about doing the right thing in test cases.

[Bigcheese]

lgtm with the test fix.

clang/unittests/Frontend/CompilerInvocationTest.cpp

272

I believe that if you have shared libraries enabled this is guaranteed to be different. We should do a string comparison here, and I think a helper makes sense if we expect to do this more often.

[dexonsmith]

LGTM too, once the test is fixed.

clang/unittests/Frontend/CompilerInvocationTest.cpp

272

Agreed, should be string comparison. IMO passing StringRef("...") into count is clear / easy enough, but a custom matcher would be fine too.

clang/include/clang/Driver/Options.td

 }
 
 // Creates record with a marshalled flag.
 class BoolOptionFlag<FlagDefExpanded flag, FlagDefExpanded other,
                      FlagDefExpanded implied, code keypath, Default default>
   : Flag<["-"], flag.Spelling>, Flags<flag.OptionFlags>, HelpText<flag.Help>,
     MarshallingInfoBooleanFlag<keypath, default.Value, flag.ValueAsCode,
                                flag.RecordName, other.ValueAsCode,
-                               other.RecordName, other.Spelling>,
+                               other.RecordName>,
     ImpliedByAnyOf<implied.ImpliedBy, implied.ValueAsCode> {}
 
 // Generates TableGen records for two command line flags that control the same
 // keypath via the marshalling infrastructure.
 // Names of the records consist of the specified prefix, "no_" for the negative
 // flag, and NAME.
 // BoolOption is the API that should be used most of the time. Use this only
 // when you need more control (e.g. to represent a marshalled option whose

clang/lib/Frontend/CompilerInvocation.cpp

return [Value, OtherValue, OtherOpt](OptSpecifier Opt, unsigned,
                                        DiagnosticsEngine &) -> Optional<bool> {
     if (const Arg *A = Args.getLastArg(Opt, OtherOpt)) {
       return A->getOption().matches(Opt) ? Value : OtherValue;
     }
     return None;
   };
 }
 
-static auto makeBooleanOptionDenormalizer(bool Value,
-                                          const char *OtherSpelling) {
-  return [Value, OtherSpelling](
-             SmallVectorImpl<const char *> &Args, const char *Spelling,
+static auto makeBooleanOptionDenormalizer(bool Value) {
+  return [Value](SmallVectorImpl<const char *> &Args, const char *Spelling,
                  CompilerInvocation::StringAllocator, unsigned, bool KeyPath) {
-    Args.push_back(KeyPath == Value ? Spelling : OtherSpelling);
+    if (KeyPath == Value)
+      Args.push_back(Spelling);
   };
 }
 
 static Optional<SimpleEnumValue>
 findValueTableByName(const SimpleEnumValueTable &Table, StringRef Name) {
   for (int I = 0, E = Table.Size; I != E; ++I)
     if (Name == Table.Table[I].Name)
       return Table.Table[I];

clang/unittests/Frontend/CompilerInvocationTest.cpp

TEST_F(CommandLineTest, BoolOptionCC1ViaLetPresentPos) {
 
   CompilerInvocation::CreateFromArgs(Invocation, Args, *Diags);
 
   ASSERT_FALSE(Diags->hasErrorOccurred());
   ASSERT_TRUE(Invocation.getCodeGenOpts().DebugPassManager);
 
   Invocation.generateCC1CommandLine(GeneratedArgs, *this);
 
-  ASSERT_THAT(GeneratedArgs, Contains(StrEq("-fdebug-pass-manager")));
+  ASSERT_EQ(count(GeneratedArgs, StringRef("-fdebug-pass-manager")), 1);
   ASSERT_THAT(GeneratedArgs, Not(Contains(StrEq("-fno-debug-pass-manager"))));
 }
 
 TEST_F(CommandLineTest, BoolOptionCC1ViaLetPresentNeg) {
   const char *Args[] = {"-fno-debug-pass-manager"};
 
   CompilerInvocation::CreateFromArgs(Invocation, Args, *Diags);
 

llvm/include/llvm/Option/OptParser.td

class MarshallingInfoBitfieldFlag<code keypath, code value>
   : MarshallingInfoFlag<keypath, "0u"> {
   code Normalizer = "makeFlagToValueNormalizer("#value#")";
   code ValueMerger = "mergeMaskValue";
   code ValueExtractor = "(extractMaskValue<unsigned, decltype("#value#"), "#value#">)";
 }
 
 // Marshalling info for booleans. Applied to the flag setting keypath to false.
 class MarshallingInfoBooleanFlag<code keypath, code defaultvalue, code value, code name,
-                                 code other_value, code other_name, string other_spelling>
+                                 code other_value, code other_name>
   : MarshallingInfoFlag<keypath, defaultvalue> {
   code Normalizer = "makeBooleanOptionNormalizer("#value#", "#other_value#", OPT_"#other_name#")";
-  code Denormalizer = "makeBooleanOptionDenormalizer("#value#", \""#other_spelling#"\")";
+  code Denormalizer = "makeBooleanOptionDenormalizer("#value#")";
 }
 
 // Mixins for additional marshalling attributes.
 
 class IsNegative {
   code Normalizer = "normalizeSimpleNegativeFlag";
 }
 class AlwaysEmit { bit ShouldAlwaysEmit = true; }