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

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

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

6223

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

I assume member data pointers are supposed to be offset_type_class.

6241

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

6232

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

6251

You can switch over the tag kind here.

6257

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

6259

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())
     return void_type_class;
   else if (ArgTy->isEnumeralType())
-    return enumeral_type_class;
+    return LangOpts.CPlusPlus ? enumeral_type_class : integer_type_class;
   else if (ArgTy->isBooleanType())
     return boolean_type_class;
   else if (ArgTy->isCharType())
-    return string_type_class; // gcc doesn't appear to use char_type_class
+    return integer_type_class; // gcc doesn't appear to use char_type_class
   else if (ArgTy->isIntegerType())
     return integer_type_class;
   else if (ArgTy->isPointerType())
     return pointer_type_class;
   else if (ArgTy->isReferenceType())
     return reference_type_class;
   else if (ArgTy->isRealType())
     return real_type_class;
   else if (ArgTy->isComplexType())
     return complex_type_class;
   else if (ArgTy->isFunctionType())
-    return function_type_class;
+    return LangOpts.CPlusPlus ? function_type_class : pointer_type_class;
+  else if (ArgTy->isMemberFunctionPointerType())
+    return method_type_class;
   else if (ArgTy->isStructureOrClassType())
     return record_type_class;
   else if (ArgTy->isUnionType())
     return union_type_class;
   else if (ArgTy->isArrayType())
-    return array_type_class;
+    return LangOpts.CPlusPlus ? array_type_class : pointer_type_class;
   else if (ArgTy->isUnionType())
     return union_type_class;
-  else  // FIXME: offset_type_class, method_type_class, & lang_type_class?
+  else  // FIXME: offset_type_class & lang_type_class?
     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.
   //  - If the operand and can be folded to a pointer to the first character
   //    of a string literal (or such a pointer cast to an integral 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 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(d) == real_type_class ? 1 : -1];
+  int a9[__builtin_classify_type(f) == function_type_class ? 1 : -1];
+  int a10[__builtin_classify_type(&cl::bar) == method_type_class ? 1 : -1];
+  int a11[__builtin_classify_type(cl_obj) == record_type_class ? 1 : -1];
+  int a12[__builtin_classify_type(u_obj) == union_type_class ? 1 : -1];
+  int a13[__builtin_classify_type(arr) == array_type_class ? 1 : -1];
+  int a14[__builtin_classify_type("abc") == array_type_class ? 1 : -1];
+}
+