[OptTable] Make explicitly included options override excluded ones

[bogner]

Summary

When we have both explicitly included and excluded option sets, we
were excluding anything from the latter set regardless of what was in
the former. This doesn't compose well and led to an overly complicated
design around DXC options where a third flag was introduced to handle
options that overlapped between DXC and CL.

With this change we check the included options before excluding
anything from the exclude list, which allows for options that are in
multiple categories to be handled in a sensible way. This allows us to
remove CLDXCOption but should otherwise be NFC.

[python3kgae]

This is much cleaner.

[bob80905]

clang/include/clang/Driver/Options.h

40

Given that the id for these flags have changed, does it make sense to write a test that makes sure these flags with these bits still behave as expected? (Ignored and target specific, specifically).

[bogner]

clang/include/clang/Driver/Options.h

40

The actual values of the flags aren't really visible anywhere (and in the case of ignored, it's actually just changing the value back to what it was before https://reviews.llvm.org/D128462). I'm not sure that there's much we can do to write such a test.

[MaskRay]

Thank you! The clean up is nice. I missed the original change that added the DXCOption complexity..

clang-tools-extra/clangd/CompileCommands.cpp

void CommandMangler::operator()(tooling::CompileCommand &Command,
   // everything it could parse in ArgList. So we just ignore those counts.
   unsigned IgnoredCount;
   // Drop the executable name, as ParseArgs doesn't expect it. This means
   // indices are actually of by one between ArgList and OriginalArgs.
   llvm::opt::InputArgList ArgList;
   ArgList = OptTable.ParseArgs(
       llvm::ArrayRef(OriginalArgs).drop_front(), IgnoredCount, IgnoredCount,
       /*FlagsToInclude=*/
-      IsCLMode ? (driver::options::CLOption | driver::options::CoreOption |
-                  driver::options::CLDXCOption)
+      IsCLMode ? (driver::options::CLOption | driver::options::CoreOption)
                : /*everything*/ 0,
       /*FlagsToExclude=*/driver::options::NoDriverOption |
-          (IsCLMode
-               ? 0
-               : (driver::options::CLOption | driver::options::CLDXCOption)));
+          (IsCLMode ? 0 : driver::options::CLOption));
 
   llvm::SmallVector<unsigned, 1> IndicesToDrop;
   // Having multiple architecture options (e.g. when building fat binaries)
   // results in multiple compiler jobs, which clangd cannot handle. In such
   // cases strip all the `-arch` options and fallback to default architecture.
   // As there are no signals to figure out which one user actually wants. They
   // can explicitly specify one through `CompileFlags.Add` if need be.
   unsigned ArchOptCount = 0;
unsigned char getModes(const llvm::opt::Option &Opt) {
   // Why is this so complicated?!
   // Reference is clang::driver::Driver::getIncludeExcludeOptionFlagMasks()
   unsigned char Result = DM_None;
   if (Opt.hasFlag(driver::options::CC1Option))
     Result |= DM_CC1;
   if (!Opt.hasFlag(driver::options::NoDriverOption)) {
     if (Opt.hasFlag(driver::options::CLOption)) {
       Result |= DM_CL;
-    } else if (Opt.hasFlag(driver::options::CLDXCOption)) {
-      Result |= DM_CL;
     } else {
       Result |= DM_GCC;
       if (Opt.hasFlag(driver::options::CoreOption)) {
         Result |= DM_CL;
       }
     }
   }
   return Result;

clang/include/clang/Driver/Options.h

enum ClangFlags {
   CC1Option = (1 << 10),
   CC1AsOption = (1 << 11),
   NoDriverOption = (1 << 12),
   LinkOption = (1 << 13),
   FlangOption = (1 << 14),
   FC1Option = (1 << 15),
   FlangOnlyOption = (1 << 16),
   DXCOption = (1 << 17),
-  CLDXCOption = (1 << 18),
-  Ignored = (1 << 19),
-  TargetSpecific = (1 << 20),
+  Ignored = (1 << 18),
+  TargetSpecific = (1 << 19),
 };
 
 enum ID {
     OPT_INVALID = 0, // This is not an option ID.
 #define OPTION(PREFIX, NAME, ID, KIND, GROUP, ALIAS, ALIASARGS, FLAGS, PARAM,  \
                HELPTEXT, METAVAR, VALUES)                                      \
   OPT_##ID,
 #include "clang/Driver/Options.inc"

clang/include/clang/Driver/Options.td

 
 // CC1AsOption - This option should be accepted by clang -cc1as.
 def CC1AsOption : OptionFlag;
 
 // DXCOption - This is a dxc.exe compatibility option. Options with this flag
 // are made available when the driver is running in DXC compatibility mode.
 def DXCOption : OptionFlag;
 
-// CLDXCOption - This is a cl.exe/dxc.exe compatibility option. Options with this flag
-// are made available when the driver is running in CL/DXC compatibility mode.
-def CLDXCOption : OptionFlag;
-
 // NoDriverOption - This option should not be accepted by the driver.
 def NoDriverOption : OptionFlag;
 
 // If an option affects linking, but has a primary group (so Link_Group cannot
 // be used), add this flag.
 def LinkOption : OptionFlag;
 
 // FlangOption - This is considered a "core" Flang option, available in
def defsym : Separate<["-"], "defsym">,
   HelpText<"Define a value for a symbol">;
 
 } // let Flags = [CC1AsOption]
 
 //===----------------------------------------------------------------------===//
 // clang-cl Options
 //===----------------------------------------------------------------------===//
 
-def cl_Group : OptionGroup<"<clang-cl options>">, Flags<[CLDXCOption]>,
+def cl_Group : OptionGroup<"<clang-cl options>">,
   HelpText<"CL.EXE COMPATIBILITY OPTIONS">;
 
 def cl_compile_Group : OptionGroup<"<clang-cl compile-only options>">,
   Group<cl_Group>;
 
 def cl_ignored_Group : OptionGroup<"<clang-cl ignored options>">,
   Group<cl_Group>;
 
-class CLFlag<string name> : Option<["/", "-"], name, KIND_FLAG>,
-  Group<cl_Group>, Flags<[CLOption, NoXarchOption]>;
-
-class CLDXCFlag<string name> : Option<["/", "-"], name, KIND_FLAG>,
-  Group<cl_Group>, Flags<[CLDXCOption, NoXarchOption]>;
-
-class CLCompileFlag<string name> : Option<["/", "-"], name, KIND_FLAG>,
-  Group<cl_compile_Group>, Flags<[CLOption, NoXarchOption]>;
-
-class CLIgnoredFlag<string name> : Option<["/", "-"], name, KIND_FLAG>,
-  Group<cl_ignored_Group>, Flags<[CLOption, NoXarchOption]>;
-
-class CLJoined<string name> : Option<["/", "-"], name, KIND_JOINED>,
-  Group<cl_Group>, Flags<[CLOption, NoXarchOption]>;
-
-class CLDXCJoined<string name> : Option<["/", "-"], name, KIND_JOINED>,
-  Group<cl_Group>, Flags<[CLDXCOption, NoXarchOption]>;
-
-class CLCompileJoined<string name> : Option<["/", "-"], name, KIND_JOINED>,
-  Group<cl_compile_Group>, Flags<[CLOption, NoXarchOption]>;
-
-class CLIgnoredJoined<string name> : Option<["/", "-"], name, KIND_JOINED>,
-  Group<cl_ignored_Group>, Flags<[CLOption, NoXarchOption, HelpHidden]>;
-
-class CLJoinedOrSeparate<string name> : Option<["/", "-"], name,
-  KIND_JOINED_OR_SEPARATE>, Group<cl_Group>, Flags<[CLOption, NoXarchOption]>;
-
-class CLDXCJoinedOrSeparate<string name> : Option<["/", "-"], name,
-  KIND_JOINED_OR_SEPARATE>, Group<cl_Group>, Flags<[CLDXCOption, NoXarchOption]>;
-
-class CLCompileJoinedOrSeparate<string name> : Option<["/", "-"], name,
-  KIND_JOINED_OR_SEPARATE>, Group<cl_compile_Group>,
-  Flags<[CLOption, NoXarchOption]>;
+class CLFlagOpts<list<OptionFlag> flags>
+  : Flags<!listconcat(flags, [NoXarchOption])>;
 
-class CLRemainingArgsJoined<string name> : Option<["/", "-"], name,
-  KIND_REMAINING_ARGS_JOINED>, Group<cl_Group>, Flags<[CLOption, NoXarchOption]>;
+class CLFlag<string name, list<OptionFlag> flags = [CLOption]>
+  : Option<["/", "-"], name, KIND_FLAG>,
+    Group<cl_Group>, CLFlagOpts<flags>;
+
+class CLCompileFlag<string name, list<OptionFlag> flags = [CLOption]>
+  : Option<["/", "-"], name, KIND_FLAG>,
+    Group<cl_compile_Group>, CLFlagOpts<flags>;
+
+class CLIgnoredFlag<string name, list<OptionFlag> flags = [CLOption]>
+  : Option<["/", "-"], name, KIND_FLAG>,
+    Group<cl_ignored_Group>, CLFlagOpts<flags>;
+
+class CLJoined<string name, list<OptionFlag> flags = [CLOption]>
+  : Option<["/", "-"], name, KIND_JOINED>,
+    Group<cl_Group>, CLFlagOpts<flags>;
+
+class CLCompileJoined<string name, list<OptionFlag> flags = [CLOption]>
+  : Option<["/", "-"], name, KIND_JOINED>,
+    Group<cl_compile_Group>, CLFlagOpts<flags>;
+
+class CLIgnoredJoined<string name, list<OptionFlag> flags = [CLOption]>
+  : Option<["/", "-"], name, KIND_JOINED>,
+    Group<cl_ignored_Group>, CLFlagOpts<!listconcat(flags, [HelpHidden])>;
+
+class CLJoinedOrSeparate<string name, list<OptionFlag> flags = [CLOption]>
+  : Option<["/", "-"], name, KIND_JOINED_OR_SEPARATE>,
+    Group<cl_Group>, CLFlagOpts<flags>;
+
+class CLCompileJoinedOrSeparate<string name,
+                                list<OptionFlag> flags = [CLOption]>
+  : Option<["/", "-"], name, KIND_JOINED_OR_SEPARATE>,
+    Group<cl_compile_Group>, CLFlagOpts<flags>;
+
+class CLRemainingArgsJoined<string name, list<OptionFlag> flags = [CLOption]>
+  : Option<["/", "-"], name, KIND_REMAINING_ARGS_JOINED>,
+    Group<cl_Group>, CLFlagOpts<flags>;
 
 // Aliases:
 // (We don't put any of these in cl_compile_Group as the options they alias are
 // already in the right group.)
 
 def _SLASH_Brepro : CLFlag<"Brepro">,
   HelpText<"Do not write current time into COFF output (breaks link.exe /incremental)">,
   Alias<mno_incremental_linker_compatible>;

 def _SLASH_Gw_ : CLFlag<"Gw-">,
   HelpText<"Do not put each data item in its own section (default)">,
   Alias<fno_data_sections>;
 def _SLASH_help : CLFlag<"help">, Alias<help>,
   HelpText<"Display available options">;
 def _SLASH_HELP : CLFlag<"HELP">, Alias<help>;
 def _SLASH_hotpatch : CLFlag<"hotpatch">, Alias<fms_hotpatch>,
   HelpText<"Create hotpatchable image">;
-def _SLASH_I : CLDXCJoinedOrSeparate<"I">,
+def _SLASH_I : CLJoinedOrSeparate<"I", [CLOption, DXCOption]>,
   HelpText<"Add directory to include search path">, MetaVarName<"<dir>">,
   Alias<I>;
 def _SLASH_J : CLFlag<"J">, HelpText<"Make char type unsigned">,
   Alias<funsigned_char>;
 
 // The _SLASH_O option handles all the /O flags, but we also provide separate
 // aliased options to provide separate help messages.
-def _SLASH_O : CLDXCJoined<"O">,
+def _SLASH_O : CLJoined<"O", [CLOption, DXCOption]>,
   HelpText<"Set multiple /O flags at once; e.g. '/O2y-' for '/O2 /Oy-'">,
   MetaVarName<"<flags>">;
 def : CLFlag<"O1">, Alias<_SLASH_O>, AliasArgs<["1"]>,
   HelpText<"Optimize for size  (like /Og     /Os /Oy /Ob2 /GF /Gy)">;
 def : CLFlag<"O2">, Alias<_SLASH_O>, AliasArgs<["2"]>,
   HelpText<"Optimize for speed (like /Og /Oi /Ot /Oy /Ob2 /GF /Gy)">;
 def : CLFlag<"Ob0">, Alias<_SLASH_O>, AliasArgs<["b0"]>,
   HelpText<"Disable function inlining">;
 def : CLFlag<"Ob1">, Alias<_SLASH_O>, AliasArgs<["b1"]>,
   HelpText<"Only inline functions explicitly or implicitly marked inline">;
 def : CLFlag<"Ob2">, Alias<_SLASH_O>, AliasArgs<["b2"]>,
   HelpText<"Inline functions as deemed beneficial by the compiler">;
-def : CLDXCFlag<"Od">, Alias<_SLASH_O>, AliasArgs<["d"]>,
+def : CLFlag<"Od", [CLOption, DXCOption]>, Alias<_SLASH_O>, AliasArgs<["d"]>,
   HelpText<"Disable optimization">;
 def : CLFlag<"Og">, Alias<_SLASH_O>, AliasArgs<["g"]>,
   HelpText<"No effect">;
 def : CLFlag<"Oi">, Alias<_SLASH_O>, AliasArgs<["i"]>,
   HelpText<"Enable use of builtin functions">;
 def : CLFlag<"Oi-">, Alias<_SLASH_O>, AliasArgs<["i-"]>,
   HelpText<"Disable use of builtin functions">;
 def : CLFlag<"Os">, Alias<_SLASH_O>, AliasArgs<["s"]>,

clang/lib/Driver/Driver.cpp

 std::pair<unsigned, unsigned>
 Driver::getIncludeExcludeOptionFlagMasks(bool IsClCompatMode) const {
   unsigned IncludedFlagsBitmask = 0;
   unsigned ExcludedFlagsBitmask = options::NoDriverOption;
 
   if (IsClCompatMode) {
     // Include CL and Core options.
     IncludedFlagsBitmask |= options::CLOption;
-    IncludedFlagsBitmask |= options::CLDXCOption;
     IncludedFlagsBitmask |= options::CoreOption;
   } else {
     ExcludedFlagsBitmask |= options::CLOption;
   }
   if (IsDXCMode()) {
     // Include DXC and Core options.
     IncludedFlagsBitmask |= options::DXCOption;
-    IncludedFlagsBitmask |= options::CLDXCOption;
     IncludedFlagsBitmask |= options::CoreOption;
   } else {
     ExcludedFlagsBitmask |= options::DXCOption;
   }
-  if (!IsClCompatMode && !IsDXCMode())
-    ExcludedFlagsBitmask |= options::CLDXCOption;
 
   return std::make_pair(IncludedFlagsBitmask, ExcludedFlagsBitmask);
 }
 
 const char *Driver::getExecutableForDriverMode(DriverMode Mode) {
   switch (Mode) {
   case GCCMode:
     return "clang";

clang/lib/Tooling/InterpolatingCompilationDatabase.cpp

TransferableCommand(CompileCommand C)
     if (!OldArgs.empty())
       Cmd.CommandLine.emplace_back(OldArgs.front());
     for (unsigned Pos = 1; Pos < OldArgs.size();) {
       using namespace driver::options;
 
       const unsigned OldPos = Pos;
       std::unique_ptr<llvm::opt::Arg> Arg(OptTable.ParseOneArg(
           ArgList, Pos,
-          /* Include */ ClangCLMode ? CoreOption | CLOption | CLDXCOption : 0,
-          /* Exclude */ ClangCLMode ? 0 : CLOption | CLDXCOption));
+          /* Include */ ClangCLMode ? CoreOption | CLOption : 0,
+          /* Exclude */ ClangCLMode ? 0 : CLOption));
 
       if (!Arg)
         continue;
 
       const llvm::opt::Option &Opt = Arg->getOption();
 
       // Strip input and output files.
       if (Opt.matches(OPT_INPUT) || Opt.matches(OPT_o) ||

llvm/lib/Option/OptTable.cpp

for (; Start != End; ++Start) {
     if (Start == End)
       break;
 
     Option Opt(Start, this);
 
     if (FlagsToInclude && !Opt.hasFlag(FlagsToInclude))
       continue;
     if (Opt.hasFlag(FlagsToExclude))
+      if (!FlagsToInclude || !Opt.hasFlag(FlagsToInclude))
         continue;
 
     // See if this option matches.
     if (std::unique_ptr<Arg> A =
             Opt.accept(Args, StringRef(Args.getArgString(Index), ArgSize),
                        /*GroupedShortOption=*/false, Index))
       return A;
 
     // Otherwise, see if this argument was missing values.
for (unsigned Id = 1, e = getNumOptions() + 1; Id != e; ++Id) {
     // FIXME: Split out option groups.
     if (getOptionKind(Id) == Option::GroupClass)
       continue;
 
     unsigned Flags = getInfo(Id).Flags;
     if (FlagsToInclude && !(Flags & FlagsToInclude))
       continue;
     if (Flags & FlagsToExclude)
+      if (!FlagsToInclude || !(Flags & FlagsToInclude))
         continue;
 
     // If an alias doesn't have a help text, show a help text for the aliased
     // option instead.
     const char *HelpText = getOptionHelpText(Id);
     if (!HelpText && ShowAllAliases) {
       const Option Alias = getOption(Id).getAlias();
       if (Alias.isValid())
         HelpText = getOptionHelpText(Alias.getID());