This is an archive of the discontinued LLVM Phabricator instance.

PR26449: Fixes for bugs in __builtin_classify_type implementation
ClosedPublic

Authored by andreybokhanko on Feb 3 2016, 3:48 AM.

Details

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

Diff Detail

Event Timeline

andreybokhanko retitled this revision from to PR26449: Fixes for bugs in __builtin_classify_type implementation.
andreybokhanko updated this object.
andreybokhanko updated this object.
andreybokhanko added a subscriber: cfe-commits.
rjmccall added inline comments.Feb 3 2016, 9:57 AM
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.

John, thank you for the review!

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

Yours,
Andrey

rjmccall edited edge metadata.Feb 4 2016, 10:38 AM

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 edited edge metadata.

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

Patch updated; please re-review again.

Yes, thank you, that looks great. LGTM.

This revision was automatically updated to reflect the committed changes.

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

Yours,
Andrey