PR26449: Fixes for bugs in __builtin_classify_type implementation

[andreybokhanko]

Summary

There are several bugs in __builtin_classify_type implementation:

1. It doesn't support member functions, so the following program crashes clang:

lass cl {
public:
    void bar() {}
};
...
int res = __builtin_classify_type(&cl::bar);

2. It returns enumeral_type_class for enums, function_type_class for functions and array_type_class for arrays in both C and C++ modes. However, gcc returns integer_type_class for enums and pointer_type_class for arrays and functions in C mode -- which is correct, as conversions between enums and ints and function pointers and arrays and pointers can happen everywhere in C. (gcc returns the same as what clang does in C++ mode, however.)

3. It returns string_type_class for chars, which is odd and doesn't match what gcc does.

This patch fixes all these bugs.

Yours,

Andrey

Software Engineer
Intel Compiler Team

[rjmccall]

lib/AST/ExprConstant.cpp

6213 ↗

(On Diff #46759)

I mean, matching GCC exactly on this weird little extension seems reasonable to me, but this is very strange behavior. Sure, there's an implicit conversion to enum types in C, but there are implicit conversions between a lot of types that are distinguished.

6217 ↗

(On Diff #46759)

You can just eliminate this case completely; it's caught by the next line. The comment is good, please add it there.

6223 ↗

(On Diff #46759)

Note that this case is not actually possible; expressions never have reference type, they're just l-values or x-values. It's vaguely possible that GCC actually does something like the C++11 decltype feature, which IIRC recognizes direct decl references and reports their declared type instead of the formal type of the reference expression.

6231 ↗

(On Diff #46759)

I assume member data pointers are supposed to be offset_type_class.

6241 ↗

(On Diff #46759)

We should be able to turn this into an exhaustive switch over the canonical types.

[andreybokhanko]

John, thank you for the review!

I refactored EvaluateBuiltinClassifyType according to your comments; please re-review.

Yours,
Andrey

[rjmccall]

Thank you, that's a lot better. Just a few suggestions to promote exhaustive checking.

lib/AST/ExprConstant.cpp

6246 ↗

(On Diff #46908)

Again, try to avoid adding default cases. There's an x-macro file for BuiltinTypes, too.

6262 ↗

(On Diff #46908)

Please turn the else-if into an else with an assert.

6277 ↗

(On Diff #46908)

You can switch over the tag kind here.

6285 ↗

(On Diff #46908)

Instead of having a default case, you should use an x-macro to exhaustively fill in all the cases that you're assuming can't happen. That way, if somebody adds a new canonical type kind, they'll automatically get a warning here if they don't add it.

In this case, since you're working with a canonical type and it can't be dependent, this would look like:

#define TYPE(ID, BASE)
#define NON_CANONICAL_TYPE(ID, BASE) case Type::ID:
#define NON_CANONICAL_UNLESS_DEPENDENT_TYPE(ID, BASE) case Type::ID:
#define DEPENDENT_TYPE(ID, BASE) case Type::ID:
#include "clang/AST/TypeNodes.def"

[andreybokhanko]

John, thanks for the re-review! -- I did as you advised, and added *a lot* of switches. :-)

Patch updated; please re-review again.

[rjmccall]

Yes, thank you, that looks great. LGTM.

[andreybokhanko]

John, thank you for all these tireless re-reviews -- very much appreciated!

Yours,
Andrey

cfe/trunk/lib/AST/ExprConstant.cpp

else {
       return Success(Val, E);
     }
   }
   return false;
 }
 
 /// EvaluateBuiltinClassifyType - Evaluate __builtin_classify_type the same way
 /// as GCC.
-static int EvaluateBuiltinClassifyType(const CallExpr *E) {
+static int EvaluateBuiltinClassifyType(const CallExpr *E,
+                                       const LangOptions &LangOpts) {
   // The following enum mimics the values returned by GCC.
   // FIXME: Does GCC differ between lvalue and rvalue references here?
   enum gcc_type_class {
     no_type_class = -1,
     void_type_class, integer_type_class, char_type_class,
     enumeral_type_class, boolean_type_class,
     pointer_type_class, reference_type_class, offset_type_class,
     real_type_class, complex_type_class,
     function_type_class, method_type_class,
     record_type_class, union_type_class,
     array_type_class, string_type_class,
     lang_type_class
   };
 
   // If no argument was supplied, default to "no_type_class". This isn't
   // ideal, however it is what gcc does.
   if (E->getNumArgs() == 0)
     return no_type_class;
 
-  QualType ArgTy = E->getArg(0)->getType();
-  if (ArgTy->isVoidType())
+  QualType CanTy = E->getArg(0)->getType().getCanonicalType();
+  const BuiltinType *BT = dyn_cast<BuiltinType>(CanTy);
+
+  switch (CanTy->getTypeClass()) {
+#define TYPE(ID, BASE)
+#define DEPENDENT_TYPE(ID, BASE) case Type::ID:
+#define NON_CANONICAL_TYPE(ID, BASE) case Type::ID:
+#define NON_CANONICAL_UNLESS_DEPENDENT_TYPE(ID, BASE) case Type::ID:
+#include "clang/AST/TypeNodes.def"
+      llvm_unreachable("CallExpr::isBuiltinClassifyType(): unimplemented type");
+
+  case Type::Builtin:
+    switch (BT->getKind()) {
+#define BUILTIN_TYPE(ID, SINGLETON_ID)
+#define SIGNED_TYPE(ID, SINGLETON_ID) case BuiltinType::ID: return integer_type_class;
+#define FLOATING_TYPE(ID, SINGLETON_ID) case BuiltinType::ID: return real_type_class;
+#define PLACEHOLDER_TYPE(ID, SINGLETON_ID) case BuiltinType::ID: break;
+#include "clang/AST/BuiltinTypes.def"
+    case BuiltinType::Void:
       return void_type_class;
-  else if (ArgTy->isEnumeralType())
-    return enumeral_type_class;
-  else if (ArgTy->isBooleanType())
+
+    case BuiltinType::Bool:
       return boolean_type_class;
-  else if (ArgTy->isCharType())
-    return string_type_class; // gcc doesn't appear to use char_type_class
-  else if (ArgTy->isIntegerType())
+
+    case BuiltinType::Char_U: // gcc doesn't appear to use char_type_class
+    case BuiltinType::UChar:
+    case BuiltinType::UShort:
+    case BuiltinType::UInt:
+    case BuiltinType::ULong:
+    case BuiltinType::ULongLong:
+    case BuiltinType::UInt128:
       return integer_type_class;
-  else if (ArgTy->isPointerType())
+
+    case BuiltinType::NullPtr:
       return pointer_type_class;
-  else if (ArgTy->isReferenceType())
-    return reference_type_class;
-  else if (ArgTy->isRealType())
-    return real_type_class;
-  else if (ArgTy->isComplexType())
+
+    case BuiltinType::WChar_U:
+    case BuiltinType::Char16:
+    case BuiltinType::Char32:
+    case BuiltinType::ObjCId:
+    case BuiltinType::ObjCClass:
+    case BuiltinType::ObjCSel:
+    case BuiltinType::OCLImage1d:
+    case BuiltinType::OCLImage1dArray:
+    case BuiltinType::OCLImage2d:
+    case BuiltinType::OCLImage2dArray:
+    case BuiltinType::OCLImage1dBuffer:
+    case BuiltinType::OCLImage2dDepth:
+    case BuiltinType::OCLImage2dArrayDepth:
+    case BuiltinType::OCLImage2dMSAA:
+    case BuiltinType::OCLImage2dArrayMSAA:
+    case BuiltinType::OCLImage2dMSAADepth:
+    case BuiltinType::OCLImage2dArrayMSAADepth:
+    case BuiltinType::OCLImage3d:
+    case BuiltinType::OCLSampler:
+    case BuiltinType::OCLEvent:
+    case BuiltinType::OCLClkEvent:
+    case BuiltinType::OCLQueue:
+    case BuiltinType::OCLNDRange:
+    case BuiltinType::OCLReserveID:
+    case BuiltinType::Dependent:
+      llvm_unreachable("CallExpr::isBuiltinClassifyType(): unimplemented type");
+    };
+
+  case Type::Enum:
+    return LangOpts.CPlusPlus ? enumeral_type_class : integer_type_class;
+    break;
+
+  case Type::Pointer:
+    return pointer_type_class;
+    break;
+
+  case Type::MemberPointer:
+    if (CanTy->isMemberDataPointerType())
+      return offset_type_class;
+    else {
+      // We expect member pointers to be either data or function pointers,
+      // nothing else.
+      assert(CanTy->isMemberFunctionPointerType());
+      return method_type_class;
+    }
+
+  case Type::Complex:
     return complex_type_class;
-  else if (ArgTy->isFunctionType())
-    return function_type_class;
-  else if (ArgTy->isStructureOrClassType())
+
+  case Type::FunctionNoProto:
+  case Type::FunctionProto:
+    return LangOpts.CPlusPlus ? function_type_class : pointer_type_class;
+
+  case Type::Record:
+    if (const RecordType *RT = CanTy->getAs<RecordType>()) {
+      switch (RT->getDecl()->getTagKind()) {
+      case TagTypeKind::TTK_Struct:
+      case TagTypeKind::TTK_Class:
+      case TagTypeKind::TTK_Interface:
         return record_type_class;
-  else if (ArgTy->isUnionType())
-    return union_type_class;
-  else if (ArgTy->isArrayType())
-    return array_type_class;
-  else if (ArgTy->isUnionType())
+
+      case TagTypeKind::TTK_Enum:
+        return LangOpts.CPlusPlus ? enumeral_type_class : integer_type_class;
+
+      case TagTypeKind::TTK_Union:
         return union_type_class;
-  else  // FIXME: offset_type_class, method_type_class, & lang_type_class?
-    llvm_unreachable("CallExpr::isBuiltinClassifyType(): unimplemented type");
+      }
+    }
+    llvm_unreachable("CallExpr::isBuiltinClassifyType(): unimplemented type");
+
+  case Type::ConstantArray:
+  case Type::VariableArray:
+  case Type::IncompleteArray:
+    return LangOpts.CPlusPlus ? array_type_class : pointer_type_class;
+
+  case Type::BlockPointer:
+  case Type::LValueReference:
+  case Type::RValueReference:
+  case Type::Vector:
+  case Type::ExtVector:
+  case Type::Auto:
+  case Type::ObjCObject:
+  case Type::ObjCInterface:
+  case Type::ObjCObjectPointer:
+  case Type::Pipe:
+  case Type::Atomic:
+    llvm_unreachable("CallExpr::isBuiltinClassifyType(): unimplemented type");
+  }
+
 }
 
 /// EvaluateBuiltinConstantPForLValue - Determine the result of
 /// __builtin_constant_p when applied to the given lvalue.
 ///
 /// An lvalue is only "constant" if it is a pointer or reference to the first
 /// character of a string literal.
 template<typename LValue>
case Builtin::BI__builtin_bswap64: {
     APSInt Val;
     if (!EvaluateInteger(E->getArg(0), Val, Info))
       return false;
 
     return Success(Val.byteSwap(), E);
   }
 
   case Builtin::BI__builtin_classify_type:
-    return Success(EvaluateBuiltinClassifyType(E), E);
+    return Success(EvaluateBuiltinClassifyType(E, Info.getLangOpts()), E);
 
   // FIXME: BI__builtin_clrsb
   // FIXME: BI__builtin_clrsbl
   // FIXME: BI__builtin_clrsbll
 
   case Builtin::BI__builtin_clz:
   case Builtin::BI__builtin_clzl:
   case Builtin::BI__builtin_clzll: