[NFC][Driver] Change Multilib flag representation

[michaelplatings]

Summary

This new representation means that if a string is a valid command line
option then it can also be used to indicate that the option is required
by a multilib.

To indicate that a flag is required not to be present, its first
character is replaced with '!', which is intended for consistency with
the logical not operator in many programming languages.

[michaelplatings]

@phosek you made some alternative suggestions:

1. Have two separate lists of include and exclude flags.
2. Store flags as a tuple (or a struct?) of string and a tag (that is include or exclude).

The reason for not doing that is to keep multilib selection method conceptually simple: a multilib is a match if its flags are a subset of flags generated from command line arguments. Although historically the Multilib class has had the concept of flags being indicated or contraindicated, I've found that's not needed for the proposed configurable multilib scheme. Therefore I'd prefer not to codify that concept within the Multilib class any more than it already is. Over time I expect "contraindicated" flags like !fexceptions to be used less and less, in favour of real flags like -fno-exceptions.

[simon_tatham]

LGTM, but please wait at least a day for other people to have last-minute thoughts.

[michaelplatings]

Update unit tests

clang/include/clang/Driver/Multilib.h

public:
   /// target variant. Always starts with a '/', unless empty
   const std::string &osSuffix() const { return OSSuffix; }
 
   /// Get the include directory suffix. Always starts with a '/', unless
   /// empty
   const std::string &includeSuffix() const { return IncludeSuffix; }
 
   /// Get the flags that indicate or contraindicate this multilib's use
-  /// All elements begin with either '+' or '-'
+  /// All elements begin with either '-' or '!'
   const flags_list &flags() const { return Flags; }
 
   LLVM_DUMP_METHOD void dump() const;
   /// print summary of the Multilib
   void print(raw_ostream &OS) const;
 
   /// Check whether the default is selected
   bool isDefault() const

clang/include/clang/Driver/MultilibBuilder.h

assert(IncludeSuffix.empty() || (StringRef(IncludeSuffix).front() == '/' &&
                                      IncludeSuffix.size() > 1));
     return IncludeSuffix;
   }
 
   /// Set the include directory suffix
   MultilibBuilder &includeSuffix(StringRef S);
 
   /// Get the flags that indicate or contraindicate this multilib's use
-  /// All elements begin with either '+' or '-'
+  /// All elements begin with either '-' or '!'
   const flags_list &flags() const { return Flags; }
   flags_list &flags() { return Flags; }
 
   /// Add a flag to the flags list
   /// \p Required defines whether the flag is required or disallowed.
   /// \p Flag must be a flag accepted by the driver.
   MultilibBuilder &flag(bool Required, StringRef Flag);
 

clang/lib/Driver/Multilib.cpp

 void Multilib::print(raw_ostream &OS) const {
   if (GCCSuffix.empty())
     OS << ".";
   else {
     OS << StringRef(GCCSuffix).drop_front();
   }
   OS << ";";
   for (StringRef Flag : Flags) {
-    if (Flag.front() == '+')
+    if (Flag.front() == '-')
       OS << "@" << Flag.substr(1);
   }
 }
 
 bool Multilib::operator==(const Multilib &Other) const {
   // Check whether the flags sets match
   // allowing for the match to be order invariant
   llvm::StringSet<> MyFlags;

clang/lib/Driver/MultilibBuilder.cpp

 }
 
 bool MultilibBuilder::isValid() const {
   llvm::StringMap<int> FlagSet;
   for (unsigned I = 0, N = Flags.size(); I != N; ++I) {
     StringRef Flag(Flags[I]);
     llvm::StringMap<int>::iterator SI = FlagSet.find(Flag.substr(1));
 
-    assert(StringRef(Flag).front() == '+' || StringRef(Flag).front() == '-');
+    assert(StringRef(Flag).front() == '-' || StringRef(Flag).front() == '!');
 
     if (SI == FlagSet.end())
       FlagSet[Flag.substr(1)] = I;
     else if (Flags[I] != Flags[SI->getValue()])
       return false;
   }
   return true;
 }
 
 MultilibBuilder &MultilibBuilder::flag(bool Required, StringRef Flag) {
   tools::addMultilibFlag(Required, Flag, Flags);
   return *this;
 }
 
 Multilib MultilibBuilder::makeMultilib() const {
   return Multilib(GCCSuffix, OSSuffix, IncludeSuffix, Flags);
 }
 
 MultilibSetBuilder &MultilibSetBuilder::Maybe(const MultilibBuilder &M) {
   MultilibBuilder Opposite;
-  // Negate any '+' flags
+  // Negate positive flags
   for (StringRef Flag : M.flags()) {
-    if (Flag.front() == '+')
-      Opposite.flags().push_back(("-" + Flag.substr(1)).str());
+    if (Flag.front() == '-')
+      Opposite.flags().push_back(("!" + Flag.substr(1)).str());
   }
   return Either(M, Opposite);
 }
 
 MultilibSetBuilder &MultilibSetBuilder::Either(const MultilibBuilder &M1,
                                                const MultilibBuilder &M2) {
   return Either({M1, M2});
 }

clang/lib/Driver/ToolChains/CommonArgs.cpp

SmallString<128> tools::getStatsFileName(const llvm::opt::ArgList &Args,
   }
   return StatsFile;
 }
 
 void tools::addMultilibFlag(bool Enabled, const StringRef Flag,
                             Multilib::flags_list &Flags) {
   assert(Flag.front() == '-');
   if (Enabled) {
-    Flags.push_back(("+" + Flag.substr(1)).str());
-  } else {
     Flags.push_back(Flag.str());
+  } else {
+    Flags.push_back(("!" + Flag.substr(1)).str());
   }
 }
 
 void tools::addX86AlignBranchArgs(const Driver &D, const ArgList &Args,
                                   ArgStringList &CmdArgs, bool IsLTO,
                                   const StringRef PluginOptPrefix) {
   auto addArg = [&, IsLTO](const Twine &Arg) {
     if (IsLTO) {

clang/lib/Driver/ToolChains/OHOS.cpp

 static bool findOHOSMuslMultilibs(const Multilib::flags_list &Flags,
                                   DetectedMultilibs &Result) {
   MultilibSet Multilibs;
   Multilibs.push_back(Multilib());
   // -mcpu=cortex-a7
   // -mfloat-abi=soft -mfloat-abi=softfp -mfloat-abi=hard
   // -mfpu=neon-vfpv4
   Multilibs.push_back(
-      Multilib("/a7_soft", {}, {}, {"+mcpu=cortex-a7", "+mfloat-abi=soft"}));
+      Multilib("/a7_soft", {}, {}, {"-mcpu=cortex-a7", "-mfloat-abi=soft"}));
 
   Multilibs.push_back(
       Multilib("/a7_softfp_neon-vfpv4", {}, {},
-               {"+mcpu=cortex-a7", "+mfloat-abi=softfp", "+mfpu=neon-vfpv4"}));
+               {"-mcpu=cortex-a7", "-mfloat-abi=softfp", "-mfpu=neon-vfpv4"}));
 
   Multilibs.push_back(
       Multilib("/a7_hard_neon-vfpv4", {}, {},
-               {"+mcpu=cortex-a7", "+mfloat-abi=hard", "+mfpu=neon-vfpv4"}));
+               {"-mcpu=cortex-a7", "-mfloat-abi=hard", "-mfpu=neon-vfpv4"}));
 
   if (Multilibs.select(Flags, Result.SelectedMultilib)) {
     Result.Multilibs = Multilibs;
     return true;
   }
   return false;
 }
 

clang/unittests/Driver/MultilibBuilderTest.cpp

TEST(MultilibBuilderTest, Construction1) {
   ASSERT_TRUE(M.includeSuffix() == "/inc64");
 }
 
 TEST(MultilibBuilderTest, Construction3) {
   MultilibBuilder M =
       MultilibBuilder().flag(true, "-f1").flag(true, "-f2").flag(false, "-f3");
   for (const std::string &A : M.flags()) {
     ASSERT_TRUE(llvm::StringSwitch<bool>(A)
-                    .Cases("+f1", "+f2", "-f3", true)
+                    .Cases("-f1", "-f2", "!f3", true)
                     .Default(false));
   }
 }
 
 TEST(MultilibBuilderTest, SetConstruction1) {
   // Single maybe
   MultilibSet MS = MultilibSetBuilder()
                        .Maybe(MultilibBuilder("64").flag(true, "-m64"))
                        .makeMultilibSet();
   ASSERT_TRUE(MS.size() == 2);
   for (MultilibSet::const_iterator I = MS.begin(), E = MS.end(); I != E; ++I) {
     if (I->gccSuffix() == "/64")
-      ASSERT_TRUE(*I->flags().begin() == "+m64");
-    else if (I->gccSuffix() == "")
       ASSERT_TRUE(*I->flags().begin() == "-m64");
+    else if (I->gccSuffix() == "")
+      ASSERT_TRUE(*I->flags().begin() == "!m64");
     else
       FAIL() << "Unrecognized gccSufix: " << I->gccSuffix();
   }
 }
 
 TEST(MultilibBuilderTest, SetConstruction2) {
   // Double maybe
   MultilibSet MS = MultilibSetBuilder()
                        .Maybe(MultilibBuilder("sof").flag(true, "-sof"))
                        .Maybe(MultilibBuilder("el").flag(true, "-EL"))
                        .makeMultilibSet();
   ASSERT_TRUE(MS.size() == 4);
   for (MultilibSet::const_iterator I = MS.begin(), E = MS.end(); I != E; ++I) {
     ASSERT_TRUE(llvm::StringSwitch<bool>(I->gccSuffix())
                     .Cases("", "/sof", "/el", "/sof/el", true)
                     .Default(false))
         << "Multilib " << *I << " wasn't expected";
     ASSERT_TRUE(llvm::StringSwitch<bool>(I->gccSuffix())
-                    .Case("", is_contained(I->flags(), "-sof"))
-                    .Case("/sof", is_contained(I->flags(), "+sof"))
-                    .Case("/el", is_contained(I->flags(), "-sof"))
-                    .Case("/sof/el", is_contained(I->flags(), "+sof"))
+                    .Case("", is_contained(I->flags(), "!sof"))
+                    .Case("/sof", is_contained(I->flags(), "-sof"))
+                    .Case("/el", is_contained(I->flags(), "!sof"))
+                    .Case("/sof/el", is_contained(I->flags(), "-sof"))
                     .Default(false))
-        << "Multilib " << *I << " didn't have the appropriate {+,-}sof flag";
+        << "Multilib " << *I << " didn't have the appropriate {-,!}sof flag";
     ASSERT_TRUE(llvm::StringSwitch<bool>(I->gccSuffix())
-                    .Case("", is_contained(I->flags(), "-EL"))
-                    .Case("/sof", is_contained(I->flags(), "-EL"))
-                    .Case("/el", is_contained(I->flags(), "+EL"))
-                    .Case("/sof/el", is_contained(I->flags(), "+EL"))
+                    .Case("", is_contained(I->flags(), "!EL"))
+                    .Case("/sof", is_contained(I->flags(), "!EL"))
+                    .Case("/el", is_contained(I->flags(), "-EL"))
+                    .Case("/sof/el", is_contained(I->flags(), "-EL"))
                     .Default(false))
-        << "Multilib " << *I << " didn't have the appropriate {+,-}EL flag";
+        << "Multilib " << *I << " didn't have the appropriate {-,!}EL flag";
   }
 }
 
 TEST(MultilibBuilderTest, SetRegexFilter) {
   MultilibSetBuilder MB;
   MB.Maybe(MultilibBuilder("one"))
       .Maybe(MultilibBuilder("two"))
       .Maybe(MultilibBuilder("three"))
TEST(MultilibBuilderTest, SetFilterObject) {
   }
 }
 
 TEST(MultilibBuilderTest, SetSelection1) {
   MultilibSet MS1 = MultilibSetBuilder()
                         .Maybe(MultilibBuilder("64").flag(true, "-m64"))
                         .makeMultilibSet();
 
-  Multilib::flags_list FlagM64 = {"+m64"};
+  Multilib::flags_list FlagM64 = {"-m64"};
   Multilib SelectionM64;
   ASSERT_TRUE(MS1.select(FlagM64, SelectionM64))
-      << "Flag set was {\"+m64\"}, but selection not found";
+      << "Flag set was {\"-m64\"}, but selection not found";
   ASSERT_TRUE(SelectionM64.gccSuffix() == "/64")
       << "Selection picked " << SelectionM64 << " which was not expected";
 
-  Multilib::flags_list FlagNoM64 = {"-m64"};
+  Multilib::flags_list FlagNoM64 = {"!m64"};
   Multilib SelectionNoM64;
   ASSERT_TRUE(MS1.select(FlagNoM64, SelectionNoM64))
-      << "Flag set was {\"-m64\"}, but selection not found";
+      << "Flag set was {\"!m64\"}, but selection not found";
   ASSERT_TRUE(SelectionNoM64.gccSuffix() == "")
       << "Selection picked " << SelectionNoM64 << " which was not expected";
 }
 
 TEST(MultilibBuilderTest, SetSelection2) {
   MultilibSet MS2 = MultilibSetBuilder()
                         .Maybe(MultilibBuilder("el").flag(true, "-EL"))
                         .Maybe(MultilibBuilder("sf").flag(true, "-SF"))
                         .makeMultilibSet();
 
   for (unsigned I = 0; I < 4; ++I) {
     bool IsEL = I & 0x1;
     bool IsSF = I & 0x2;
     Multilib::flags_list Flags;
     if (IsEL)
-      Flags.push_back("+EL");
-    else
       Flags.push_back("-EL");
+    else
+      Flags.push_back("!EL");
 
     if (IsSF)
-      Flags.push_back("+SF");
-    else
       Flags.push_back("-SF");
+    else
+      Flags.push_back("!SF");
 
     Multilib Selection;
     ASSERT_TRUE(MS2.select(Flags, Selection))
-        << "Selection failed for " << (IsEL ? "+EL" : "-EL") << " "
-        << (IsSF ? "+SF" : "-SF");
+        << "Selection failed for " << (IsEL ? "-EL" : "!EL") << " "
+        << (IsSF ? "-SF" : "!SF");
 
     std::string Suffix;
     if (IsEL)
       Suffix += "/el";
     if (IsSF)
       Suffix += "/sf";
 
     ASSERT_EQ(Selection.gccSuffix(), Suffix)
         << "Selection picked " << Selection << " which was not expected ";
   }
 }