BuiltinOperatorOverloadBuilder: Don't consider types that are unavailable on the target (PR35174)

[hans]

Summary

In the PR, Clang ended up in a situation where it tried to mangle the __float128 type, which isn't supported when targetingt MSVC, because it instantiated a variable template with that type when searching for a conversion to use in an arithmetic expression.

[rsmith]

lib/Sema/SemaOverload.cpp

7669 ↗

(On Diff #121409)

Spurious trailing slash.

7731–7746 ↗

(On Diff #121409)

I don't think these asserts are particularly worthwhile now we're not hardcoding the indices.

test/CodeGenCXX/microsoft-cannot-mangle-float128.cpp

3–11 ↗

(On Diff #121409)

This seems like a fragile way of checking for the bug; I'd prefer a test that tests Sema rather than code generation. For instance, how about a class with a conversion operator that converts to T with an enable_if disabling it for float, double, and long double, and a check that an instance of such a class can't be used in floating-point arithmetic?

[hans]

test/CodeGenCXX/microsoft-cannot-mangle-float128.cpp

3–11 ↗

(On Diff #121409)

Thanks! That sounds like a good plan.

[hans]

Addressing comments and changing the test to exercise Sema more directly.

Please take another look.

[rsmith]

lib/Sema/SemaOverload.cpp

7618 ↗

(On Diff #121824)

Can we do better than a magic number here? I don't like that getting this wrong will lead to a silent performance change. (Can we at least assert isSmall() below or something?)

[hans]

lib/Sema/SemaOverload.cpp

7618 ↗

(On Diff #121824)

It turns out isSmall() isn't public, so I can't use that directly, but I've added a constant for the capacity and an assert against that.

I don't actually know if there's any good reason for isSmall() to not be public. If you agree, I can follow up and just use that.

cfe/trunk/lib/Sema/SemaOverload.cpp

class BuiltinOperatorOverloadBuilder {
   // Common instance state available to all overload candidate addition methods.
   Sema &S;
   ArrayRef<Expr *> Args;
   Qualifiers VisibleTypeConversionsQuals;
   bool HasArithmeticOrEnumeralCandidateType;
   SmallVectorImpl<BuiltinCandidateTypeSet> &CandidateTypes;
   OverloadCandidateSet &CandidateSet;
 
-  // Define some constants used to index and iterate over the arithemetic types
-  // provided via the getArithmeticType() method below.
-  // The "promoted arithmetic types" are the arithmetic
+  static constexpr int ArithmeticTypesCap = 24;
+  SmallVector<CanQualType, ArithmeticTypesCap> ArithmeticTypes;
+
+  // Define some indices used to iterate over the arithemetic types in
+  // ArithmeticTypes.  The "promoted arithmetic types" are the arithmetic
   // types are that preserved by promotion (C++ [over.built]p2).
-  static const unsigned FirstIntegralType = 4;
-  static const unsigned LastIntegralType = 21;
-  static const unsigned FirstPromotedIntegralType = 4,
-                        LastPromotedIntegralType = 12;
-  static const unsigned FirstPromotedArithmeticType = 0,
-                        LastPromotedArithmeticType = 12;
-  static const unsigned NumArithmeticTypes = 21;
-
-  /// \brief Get the canonical type for a given arithmetic type index.
-  CanQualType getArithmeticType(unsigned index) {
-    assert(index < NumArithmeticTypes);
-    static CanQualType ASTContext::* const
-      ArithmeticTypes[NumArithmeticTypes] = {
-      // Start of promoted types.
-      &ASTContext::FloatTy,
-      &ASTContext::DoubleTy,
-      &ASTContext::LongDoubleTy,
-      &ASTContext::Float128Ty,
+  unsigned FirstIntegralType,
+           LastIntegralType;
+  unsigned FirstPromotedIntegralType,
+           LastPromotedIntegralType;
+  unsigned FirstPromotedArithmeticType,
+           LastPromotedArithmeticType;
+  unsigned NumArithmeticTypes;
+
+  void InitArithmeticTypes() {
+    FirstPromotedArithmeticType = 0;
+    ArithmeticTypes.push_back(S.Context.FloatTy);
+    ArithmeticTypes.push_back(S.Context.DoubleTy);
+    ArithmeticTypes.push_back(S.Context.LongDoubleTy);
+    if (S.Context.getTargetInfo().hasFloat128Type())
+      ArithmeticTypes.push_back(S.Context.Float128Ty);
 
-      // Start of integral types.
-      &ASTContext::IntTy,
-      &ASTContext::LongTy,
-      &ASTContext::LongLongTy,
-      &ASTContext::Int128Ty,
-      &ASTContext::UnsignedIntTy,
-      &ASTContext::UnsignedLongTy,
-      &ASTContext::UnsignedLongLongTy,
-      &ASTContext::UnsignedInt128Ty,
-      // End of promoted types.
+    FirstIntegralType = ArithmeticTypes.size();
+    FirstPromotedIntegralType = ArithmeticTypes.size();
+    ArithmeticTypes.push_back(S.Context.IntTy);
+    ArithmeticTypes.push_back(S.Context.LongTy);
+    ArithmeticTypes.push_back(S.Context.LongLongTy);
+    if (S.Context.getTargetInfo().hasInt128Type())
+      ArithmeticTypes.push_back(S.Context.Int128Ty);
+    ArithmeticTypes.push_back(S.Context.UnsignedIntTy);
+    ArithmeticTypes.push_back(S.Context.UnsignedLongTy);
+    ArithmeticTypes.push_back(S.Context.UnsignedLongLongTy);
+    if (S.Context.getTargetInfo().hasInt128Type())
+      ArithmeticTypes.push_back(S.Context.UnsignedInt128Ty);
+    LastPromotedIntegralType = ArithmeticTypes.size();
+    LastPromotedArithmeticType = ArithmeticTypes.size();
 
-      &ASTContext::BoolTy,
-      &ASTContext::CharTy,
-      &ASTContext::WCharTy,
-      &ASTContext::Char16Ty,
-      &ASTContext::Char32Ty,
-      &ASTContext::SignedCharTy,
-      &ASTContext::ShortTy,
-      &ASTContext::UnsignedCharTy,
-      &ASTContext::UnsignedShortTy,
-      // End of integral types.
-      // FIXME: What about complex? What about half?
-    };
-    return S.Context.*ArithmeticTypes[index];
+    ArithmeticTypes.push_back(S.Context.BoolTy);
+    ArithmeticTypes.push_back(S.Context.CharTy);
+    ArithmeticTypes.push_back(S.Context.WCharTy);
+    ArithmeticTypes.push_back(S.Context.Char16Ty);
+    ArithmeticTypes.push_back(S.Context.Char32Ty);
+    ArithmeticTypes.push_back(S.Context.SignedCharTy);
+    ArithmeticTypes.push_back(S.Context.ShortTy);
+    ArithmeticTypes.push_back(S.Context.UnsignedCharTy);
+    ArithmeticTypes.push_back(S.Context.UnsignedShortTy);
+    LastIntegralType = ArithmeticTypes.size();
+    NumArithmeticTypes = ArithmeticTypes.size();
+
+    assert(ArithmeticTypes.size() <= ArithmeticTypesCap &&
+           "Enough inline storage for all arithmetic types.");
   }
 
   /// \brief Helper method to factor out the common pattern of adding overloads
   /// for '++' and '--' builtin operators.
   void addPlusPlusMinusMinusStyleOverloads(QualType CandidateTy,
                                            bool HasVolatile,
                                            bool HasRestrict) {
     QualType ParamTypes[2] = {
BuiltinOperatorOverloadBuilder(
     SmallVectorImpl<BuiltinCandidateTypeSet> &CandidateTypes,
     OverloadCandidateSet &CandidateSet)
     : S(S), Args(Args),
       VisibleTypeConversionsQuals(VisibleTypeConversionsQuals),
       HasArithmeticOrEnumeralCandidateType(
         HasArithmeticOrEnumeralCandidateType),
       CandidateTypes(CandidateTypes),
       CandidateSet(CandidateSet) {
-    // Validate some of our static helper constants in debug builds.
-    assert(getArithmeticType(FirstPromotedIntegralType) == S.Context.IntTy &&
-           "Invalid first promoted integral type");
-    assert(getArithmeticType(LastPromotedIntegralType - 1)
-             == S.Context.UnsignedInt128Ty &&
-           "Invalid last promoted integral type");
-    assert(getArithmeticType(FirstPromotedArithmeticType)
-             == S.Context.FloatTy &&
-           "Invalid first promoted arithmetic type");
-    assert(getArithmeticType(LastPromotedArithmeticType - 1)
-             == S.Context.UnsignedInt128Ty &&
-           "Invalid last promoted arithmetic type");
+
+    InitArithmeticTypes();
   }
 
   // C++ [over.built]p3:
   //
   //   For every pair (T, VQ), where T is an arithmetic type, and VQ
   //   is either volatile or empty, there exist candidate operator
   //   functions of the form
   //
public:
   //       T          operator--(VQ T&, int);
   void addPlusPlusMinusMinusArithmeticOverloads(OverloadedOperatorKind Op) {
     if (!HasArithmeticOrEnumeralCandidateType)
       return;
 
     for (unsigned Arith = (Op == OO_PlusPlus? 0 : 1);
          Arith < NumArithmeticTypes; ++Arith) {
       addPlusPlusMinusMinusStyleOverloads(
-        getArithmeticType(Arith),
+        ArithmeticTypes[Arith],
         VisibleTypeConversionsQuals.hasVolatile(),
         VisibleTypeConversionsQuals.hasRestrict());
     }
   }
 
   // C++ [over.built]p5:
   //
   //   For every pair (T, VQ), where T is a cv-qualified or
public:
   //       T         operator+(T);
   //       T         operator-(T);
   void addUnaryPlusOrMinusArithmeticOverloads() {
     if (!HasArithmeticOrEnumeralCandidateType)
       return;
 
     for (unsigned Arith = FirstPromotedArithmeticType;
          Arith < LastPromotedArithmeticType; ++Arith) {
-      QualType ArithTy = getArithmeticType(Arith);
+      QualType ArithTy = ArithmeticTypes[Arith];
       S.AddBuiltinCandidate(&ArithTy, Args, CandidateSet);
     }
 
     // Extension: We also add these operators for vector types.
     for (BuiltinCandidateTypeSet::iterator
               Vec = CandidateTypes[0].vector_begin(),
            VecEnd = CandidateTypes[0].vector_end();
          Vec != VecEnd; ++Vec) {
public:
   //
   //        T         operator~(T);
   void addUnaryTildePromotedIntegralOverloads() {
     if (!HasArithmeticOrEnumeralCandidateType)
       return;
 
     for (unsigned Int = FirstPromotedIntegralType;
          Int < LastPromotedIntegralType; ++Int) {
-      QualType IntTy = getArithmeticType(Int);
+      QualType IntTy = ArithmeticTypes[Int];
       S.AddBuiltinCandidate(&IntTy, Args, CandidateSet);
     }
 
     // Extension: We also add this operator for vector types.
     for (BuiltinCandidateTypeSet::iterator
               Vec = CandidateTypes[0].vector_begin(),
            VecEnd = CandidateTypes[0].vector_end();
          Vec != VecEnd; ++Vec) {
public:
   void addGenericBinaryArithmeticOverloads() {
     if (!HasArithmeticOrEnumeralCandidateType)
       return;
 
     for (unsigned Left = FirstPromotedArithmeticType;
          Left < LastPromotedArithmeticType; ++Left) {
       for (unsigned Right = FirstPromotedArithmeticType;
            Right < LastPromotedArithmeticType; ++Right) {
-        QualType LandR[2] = { getArithmeticType(Left),
-                              getArithmeticType(Right) };
+        QualType LandR[2] = { ArithmeticTypes[Left],
+                              ArithmeticTypes[Right] };
         S.AddBuiltinCandidate(LandR, Args, CandidateSet);
       }
     }
 
     // Extension: Add the binary operators ==, !=, <, <=, >=, >, *, /, and the
     // conditional operator for vector types.
     for (BuiltinCandidateTypeSet::iterator
               Vec1 = CandidateTypes[0].vector_begin(),
public:
   void addBinaryBitwiseArithmeticOverloads(OverloadedOperatorKind Op) {
     if (!HasArithmeticOrEnumeralCandidateType)
       return;
 
     for (unsigned Left = FirstPromotedIntegralType;
          Left < LastPromotedIntegralType; ++Left) {
       for (unsigned Right = FirstPromotedIntegralType;
            Right < LastPromotedIntegralType; ++Right) {
-        QualType LandR[2] = { getArithmeticType(Left),
-                              getArithmeticType(Right) };
+        QualType LandR[2] = { ArithmeticTypes[Left],
+                              ArithmeticTypes[Right] };
         S.AddBuiltinCandidate(LandR, Args, CandidateSet);
       }
     }
   }
 
   // C++ [over.built]p20:
   //
   //   For every pair (T, VQ), where T is an enumeration or
public:
   void addAssignmentArithmeticOverloads(bool isEqualOp) {
     if (!HasArithmeticOrEnumeralCandidateType)
       return;
 
     for (unsigned Left = 0; Left < NumArithmeticTypes; ++Left) {
       for (unsigned Right = FirstPromotedArithmeticType;
            Right < LastPromotedArithmeticType; ++Right) {
         QualType ParamTypes[2];
-        ParamTypes[1] = getArithmeticType(Right);
+        ParamTypes[1] = ArithmeticTypes[Right];
 
         // Add this built-in operator as a candidate (VQ is empty).
         ParamTypes[0] =
-          S.Context.getLValueReferenceType(getArithmeticType(Left));
+          S.Context.getLValueReferenceType(ArithmeticTypes[Left]);
         S.AddBuiltinCandidate(ParamTypes, Args, CandidateSet,
                               /*IsAssigmentOperator=*/isEqualOp);
 
         // Add this built-in operator as a candidate (VQ is 'volatile').
         if (VisibleTypeConversionsQuals.hasVolatile()) {
           ParamTypes[0] =
-            S.Context.getVolatileType(getArithmeticType(Left));
+            S.Context.getVolatileType(ArithmeticTypes[Left]);
           ParamTypes[0] = S.Context.getLValueReferenceType(ParamTypes[0]);
           S.AddBuiltinCandidate(ParamTypes, Args, CandidateSet,
                                 /*IsAssigmentOperator=*/isEqualOp);
         }
       }
     }
 
     // Extension: Add the binary operators =, +=, -=, *=, /= for vector types.
public:
   void addAssignmentIntegralOverloads() {
     if (!HasArithmeticOrEnumeralCandidateType)
       return;
 
     for (unsigned Left = FirstIntegralType; Left < LastIntegralType; ++Left) {
       for (unsigned Right = FirstPromotedIntegralType;
            Right < LastPromotedIntegralType; ++Right) {
         QualType ParamTypes[2];
-        ParamTypes[1] = getArithmeticType(Right);
+        ParamTypes[1] = ArithmeticTypes[Right];
 
         // Add this built-in operator as a candidate (VQ is empty).
         ParamTypes[0] =
-          S.Context.getLValueReferenceType(getArithmeticType(Left));
+          S.Context.getLValueReferenceType(ArithmeticTypes[Left]);
         S.AddBuiltinCandidate(ParamTypes, Args, CandidateSet);
         if (VisibleTypeConversionsQuals.hasVolatile()) {
           // Add this built-in operator as a candidate (VQ is 'volatile').
-          ParamTypes[0] = getArithmeticType(Left);
+          ParamTypes[0] = ArithmeticTypes[Left];
           ParamTypes[0] = S.Context.getVolatileType(ParamTypes[0]);
           ParamTypes[0] = S.Context.getLValueReferenceType(ParamTypes[0]);
           S.AddBuiltinCandidate(ParamTypes, Args, CandidateSet);
         }
       }
     }
   }
 

cfe/trunk/test/SemaCXX/microsoft-vs-float128.cpp

+// RUN: %clang_cc1 -triple x86_64-linux-gnu -fms-compatibility -fms-extensions -fsyntax-only -verify -std=c++11 %s
+// RUN: %clang_cc1 -triple i686-pc-win32 -fms-compatibility -fms-extensions -fsyntax-only -verify -std=c++11 -DMS %s
+
+template <bool> struct enable_if {};
+template<> struct enable_if<true> { typedef void type; };
+
+template <typename, typename> struct is_same { static constexpr bool value = false; };
+template <typename T> struct is_same<T, T> { static constexpr bool value = true; };
+
+
+
+
+struct S {
+  // The only numeric types S can be converted to is __int128 and __float128.
+  template <typename T, typename = typename enable_if<
+                            !((__is_integral(T) && sizeof(T) != 16) ||
+                              is_same<T, float>::value ||
+                              is_same<T, double>::value ||
+                              is_same<T, long double>::value)>::type>
+  operator T() { return T(); }
+};
+
+void f() {
+#ifdef MS
+  // When targeting Win32, __float128 and __int128 do not exist, so the S
+  // object cannot be converted to anything usable in the expression.
+  // expected-error@+2{{invalid operands to binary expression ('S' and 'double')}}
+#endif
+  double d = S() + 1.0;
+#ifndef MS
+  // expected-error@-2{{use of overloaded operator '+' is ambiguous}}
+  // expected-note@-3 36{{built-in candidate operator+}}
+#endif
+}