[Driver] Enable defining multilib flags verbatim

[michaelplatings]

Summary

Historically, a flag beginning - in multilib meant it was not compatible. So if you saw -fexceptions in multilib that meant exceptions were not allowed, which is confusingly the exact opposite of what it means when you see it on the command line.

This change allows doing the obvious thing of just specifying verbatim the flags needed by each multilib. The default of deriving print options from flags beginning '+' remains.

(Without this change, you could just use flags with a - prefix and everything except -print-multi-lib would work, so this change only needs to change how option printing works).

[michaelplatings]

@phosek ping

[peter.smith]

Approach looks good to me. Some suggestions, mostly around documenting the interface.

clang/include/clang/Driver/Multilib.h

57–58

I think it is worth adding some information from the commit message to the comment. In particular what the default behaviour is and what, if any, restrictions are there on the PrintOptionsList.

For example there is a comment in the assert in the constructor body that probably ought to be in the header

// If PrintOptions is something other than List then PrintOptionsList must be
// empty.

Are there any restrictions on the format of PrintOptionsList for it to print correctly? For example does each option need to be prefixed with -?

59

How would someone using makeMultilib() from MultilibBuilder use these parameters? If they can't do they need to? If so we may need something like a makeMultilib overload to supply the extra parameters.

clang/lib/Driver/Multilib.cpp

40

If there are any restrictions on the strings in PrintOptionsList could be worth assertions. Please ignore if there are no restrictions.

[michaelplatings]

Expand constructor comment as suggested by @peter.smith

clang/include/clang/Driver/Multilib.h

59

MultilibBuilder is constructed around the concept of using '+' and '-' to indicate which command line options are compatible and incompatible, so in that case you'd only want to use PlusFlags.

clang/lib/Driver/Multilib.cpp

40

There are restrictions - PrintOptionsList should only contain valid Clang options. However if you violate this then the only consequence is that the -print-multi-lib output will be equally invalid. Since there would be a lot of complexity involved in enforcing the restriction I think it's best to leave it unenforced.

[michaelplatings]

Change code to allow defining multilib flags verbatim. I've removed the option to define flags and print options independently as discussed in https://discourse.llvm.org/t/rfc-multilib/67494

[michaelplatings]

Re-run arc diff with clang-format in PATH

[phosek]

I apologize about the delayed response. I had time to think about this a bit more and it's not really clear to me if we need to preserve the "plus/minus" flag syntax. Looking through the history, it seems that it was introduced in D2538 without any discussion but I always considered it to be confusing and potentially error-prone. Rather than supporting two different syntaxes, which introduces more complexity, I'd prefer having just one. I can of think of two approaches:

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).

Do you have any preference between one these two approaches? Can you think of alternatives? It's going to take a bit of effort to migrate all the existing use cases of the "plus/minus" syntax, but I hope it should be mostly mechanical. I'd be happy to help with the migration once we settle on an API.

[michaelplatings]

Funnily enough I had the same thought coming to work this morning that we don't need 2 syntaxes. I had an idea that we could only print flags beginning "-" but will consider the other options you suggested as well. Thanks.

[michaelplatings]

Abandoning this in favour of D151437 & D151438.

clang/include/clang/Driver/Multilib.h

 /// This corresponds to a single GCC Multilib, or a segment of one controlled
 /// by a command line flag.
 /// See also MultilibBuilder for building a multilib by mutating it
 /// incrementally.
 class Multilib {
 public:
   using flags_list = std::vector<std::string>;
 
+  /// When -print-multi-lib is used, this defines how the printed options must
+  /// be calculated.
+  enum class PrintOptionsType {
+    /// Print Flags verbatim
+    Verbatim,
+    /// Print flags in the Flags list that begin with '+', but with the initial
+    /// '+' replaced with '-'. e.g. "+fno-rtti" becomes "-fno-rtti"
+    PlusFlags,
+  };
+
 private:
   std::string GCCSuffix;
   std::string OSSuffix;
   std::string IncludeSuffix;
   flags_list Flags;
+  PrintOptionsType PrintOptions;
 
 public:
   /// GCCSuffix, OSSuffix & IncludeSuffix will be appended directly to the
   /// sysroot string so they must either be empty or begin with a '/' character.
   /// This is enforced with an assert in the constructor.
+  /// Flags is the set of flags that indicate this multilib's use. See the
+  /// flags() method for more information.
   Multilib(StringRef GCCSuffix = {}, StringRef OSSuffix = {},
-           StringRef IncludeSuffix = {},
-           const flags_list &Flags = flags_list());
+           StringRef IncludeSuffix = {}, const flags_list &Flags = flags_list(),
+           PrintOptionsType PrintOptions = PrintOptionsType::PlusFlags);
 
   /// Get the detected GCC installation path suffix for the multi-arch
   /// target variant. Always starts with a '/', unless empty
   const std::string &gccSuffix() const { return GCCSuffix; }
 
   /// Get the detected os path suffix for the multi-arch
   /// 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 '-'
+  /// Get the set of flags that indicate this multilib's use.
+  /// A multilib is considered compatible if its flags are a subset of the flags
+  /// derived from the Clang command line options.
   const flags_list &flags() const { return Flags; }
 
+  /// Returns the options that should be used for clang -print-multi-lib
+  flags_list getPrintOptions() const;
+
   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
   { return GCCSuffix.empty() && OSSuffix.empty() && IncludeSuffix.empty(); }
 

clang/lib/Driver/Multilib.cpp

 #include "llvm/ADT/StringMap.h"
 #include "llvm/ADT/StringRef.h"
 #include "llvm/ADT/StringSet.h"
 #include "llvm/Support/Compiler.h"
 #include "llvm/Support/ErrorHandling.h"
 #include "llvm/Support/Path.h"
 #include "llvm/Support/Regex.h"
 #include "llvm/Support/raw_ostream.h"
-#include <algorithm>
 #include <cassert>
-#include <string>
 
 using namespace clang;
 using namespace driver;
 using namespace llvm::sys;
 
 Multilib::Multilib(StringRef GCCSuffix, StringRef OSSuffix,
-                   StringRef IncludeSuffix, const flags_list &Flags)
+                   StringRef IncludeSuffix, const flags_list &Flags,
+                   PrintOptionsType PrintOptions)
     : GCCSuffix(GCCSuffix), OSSuffix(OSSuffix), IncludeSuffix(IncludeSuffix),
-      Flags(Flags) {
+      Flags(Flags), PrintOptions(PrintOptions) {
   assert(GCCSuffix.empty() ||
          (StringRef(GCCSuffix).front() == '/' && GCCSuffix.size() > 1));
   assert(OSSuffix.empty() ||
          (StringRef(OSSuffix).front() == '/' && OSSuffix.size() > 1));
   assert(IncludeSuffix.empty() ||
          (StringRef(IncludeSuffix).front() == '/' && IncludeSuffix.size() > 1));
 }
 
+static Multilib::flags_list
+getPrintOptionsFromPlusFlags(const Multilib::flags_list &Flags) {
+  // Derive print options from flags beginning '+', replacing the first
+  // character with '-'.
+  Multilib::flags_list PrintOptions;
+  for (StringRef Flag : Flags) {
+    if (Flag.front() == '+')
+      PrintOptions.push_back(("-" + Flag.substr(1)).str());
+  }
+  return PrintOptions;
+}
+
+Multilib::flags_list Multilib::getPrintOptions() const {
+  switch (PrintOptions) {
+  case PrintOptionsType::Verbatim:
+    return Flags;
+  case PrintOptionsType::PlusFlags:
+    return getPrintOptionsFromPlusFlags(Flags);
+  }
+  llvm_unreachable("Unknown Multilib::PrintOptionsType");
+}
+
 LLVM_DUMP_METHOD void Multilib::dump() const {
   print(llvm::errs());
 }
 
 void Multilib::print(raw_ostream &OS) const {
   if (GCCSuffix.empty())
     OS << ".";
-  else {
+  else
     OS << StringRef(GCCSuffix).drop_front();
-  }
   OS << ";";
-  for (StringRef Flag : Flags) {
-    if (Flag.front() == '+')
-      OS << "@" << Flag.substr(1);
-  }
+  for (StringRef Option : getPrintOptions())
+    OS << "@" << Option.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;
   for (const auto &Flag : Flags)
     MyFlags.insert(Flag);

clang/unittests/Driver/MultilibBuilderTest.cpp

if (IsEL)
       Suffix += "/el";
     if (IsSF)
       Suffix += "/sf";
 
     ASSERT_EQ(Selection.gccSuffix(), Suffix)
         << "Selection picked " << Selection << " which was not expected ";
   }
 }
+
+TEST(MultilibBuilderTest, PrintOptions) {
+  Multilib M = MultilibBuilder()
+                   .flag("+x")
+                   .flag("-y")
+                   .flag("+a")
+                   .flag("-b")
+                   .flag("+c")
+                   .makeMultilib();
+  ASSERT_EQ(Multilib::flags_list({"-x", "-a", "-c"}), M.getPrintOptions());
+}

clang/unittests/Driver/MultilibTest.cpp

TEST(MultilibTest, SelectMultiple) {
   ASSERT_EQ(1u, Selection.size());
   EXPECT_EQ("/b", Selection[0].gccSuffix());
 
   Selection = MS.select({"y", "x"});
   ASSERT_EQ(2u, Selection.size());
   EXPECT_EQ("/a", Selection[0].gccSuffix());
   EXPECT_EQ("/b", Selection[1].gccSuffix());
 }
+
+TEST(MultilibBuilder, PrintOptions) {
+  ASSERT_EQ(Multilib::flags_list(), Multilib().getPrintOptions());
+  ASSERT_EQ(Multilib::flags_list({"-x"}),
+            Multilib({}, {}, {}, {"+x"}).getPrintOptions());
+  ASSERT_EQ(Multilib::flags_list({"-x"}),
+            Multilib({}, {}, {}, {"+x"}, Multilib::PrintOptionsType::PlusFlags)
+                .getPrintOptions());
+  ASSERT_EQ(
+      Multilib::flags_list({"-x", "-a", "-c"}),
+      Multilib({}, {}, {}, {"+x", "-y", "+a", "-b", "+c"}).getPrintOptions());
+  ASSERT_EQ(Multilib::flags_list({"-y"}),
+            Multilib({}, {}, {}, {"-y"}, Multilib::PrintOptionsType::Verbatim)
+                .getPrintOptions());
+}