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

6208–6209

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

6208–6209

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.

6278–6279

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.

6292

I assume member data pointers are supposed to be offset_type_class.

6329–6332

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

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

6285

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

6339

You can switch over the tag kind here.

6347

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

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"
+      break;
+
+  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 {
+      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>
 static bool EvaluateBuiltinConstantPForLValue(const LValue &LV) {
   const Expr *E = LV.getLValueBase().template dyn_cast<const Expr*>();
   return E && isa<StringLiteral>(E) && LV.getLValueOffset().isZero();
 }
 
 /// EvaluateBuiltinConstantP - Evaluate __builtin_constant_p as similarly to
 /// GCC as we can manage.
 static bool EvaluateBuiltinConstantP(ASTContext &Ctx, const Expr *Arg) {
   QualType ArgType = Arg->getType();
 
   // __builtin_constant_p always has one operand. The rules which gcc follows
   // are not precisely documented, but are as follows:
   //
   //  - If the operand is of integral, floating, complex or enumeration type,
   //    and can be folded to a known value of that type, it returns 1.
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:

test/Sema/builtin-classify-type.c

+// RUN: %clang_cc1 -fsyntax-only -verify %s
+
+// expected-no-diagnostics
+
+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
+};
+
+void foo() {
+  int i;
+  char c;
+  enum { red, green, blue } enum_obj;
+  int *p;
+  double d;
+  _Complex double cc;
+  extern void f();
+  struct { int a; float b; } s_obj;
+  union { int a; float b; } u_obj;
+  int arr[10];
+
+  int a1[__builtin_classify_type(f()) == void_type_class ? 1 : -1];
+  int a2[__builtin_classify_type(i) == integer_type_class ? 1 : -1];
+  int a3[__builtin_classify_type(c) == integer_type_class ? 1 : -1];
+  int a4[__builtin_classify_type(enum_obj) == integer_type_class ? 1 : -1];
+  int a5[__builtin_classify_type(p) == pointer_type_class ? 1 : -1];
+  int a6[__builtin_classify_type(d) == real_type_class ? 1 : -1];
+  int a7[__builtin_classify_type(cc) == complex_type_class ? 1 : -1];
+  int a8[__builtin_classify_type(f) == pointer_type_class ? 1 : -1];
+  int a0[__builtin_classify_type(s_obj) == record_type_class ? 1 : -1];
+  int a10[__builtin_classify_type(u_obj) == union_type_class ? 1 : -1];
+  int a11[__builtin_classify_type(arr) == pointer_type_class ? 1 : -1];
+  int a12[__builtin_classify_type("abc") == pointer_type_class ? 1 : -1];
+}
+

test/SemaCXX/builtin-classify-type.cpp

+// RUN: %clang_cc1 -fsyntax-only -verify %s
+
+// expected-no-diagnostics
+
+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
+};
+
+class cl {
+public:
+    void bar() {}
+    int baz;
+};
+
+int builtin_result;
+
+void foo() {
+  int i;
+  char c;
+  enum { red, green, blue} enum_obj;
+  bool b;
+  int *p;
+  int &r = i;
+  double d;
+  extern void f();
+  cl cl_obj;
+  union { int a; float b; } u_obj;
+  int arr[10];
+
+  int a1[__builtin_classify_type(f()) == void_type_class ? 1 : -1];
+  int a2[__builtin_classify_type(i) == integer_type_class ? 1 : -1];
+  int a3[__builtin_classify_type(c) == integer_type_class ? 1 : -1];
+  int a4[__builtin_classify_type(enum_obj) == enumeral_type_class ? 1 : -1];
+  int a5[__builtin_classify_type(b) == boolean_type_class ? 1 : -1];
+  int a6[__builtin_classify_type(p) == pointer_type_class ? 1 : -1];
+  int a7[__builtin_classify_type(r) == integer_type_class ? 1 : -1];
+  int a8[__builtin_classify_type(&cl::baz) == offset_type_class ? 1 : -1];
+  int a9[__builtin_classify_type(d) == real_type_class ? 1 : -1];
+  int a10[__builtin_classify_type(f) == function_type_class ? 1 : -1];
+  int a11[__builtin_classify_type(&cl::bar) == method_type_class ? 1 : -1];
+  int a12[__builtin_classify_type(cl_obj) == record_type_class ? 1 : -1];
+  int a13[__builtin_classify_type(u_obj) == union_type_class ? 1 : -1];
+  int a14[__builtin_classify_type(arr) == array_type_class ? 1 : -1];
+  int a15[__builtin_classify_type("abc") == array_type_class ? 1 : -1];
+}
+