[clang] fix frontend crash when evaluating type trait

[inclyc]

Summary

Before this patch type traits are checked in Parser, so use type traits
directly did not cause assertion faults. However if type traits are initialized
from a template, we didn't perform arity checks before evaluating. This
patch moves arity checks from Parser to Sema, and performing arity
checks in Sema actions, so type traits get checked corretly.

Crash input:

template<class... Ts> bool b = __is_constructible(Ts...);
bool x = b<>;

After this patch:

clang/test/SemaCXX/type-trait-eval-crash-issue-57008.cpp:5:32: error: type trait requires 1 or more arguments; have 0 arguments
template<class... Ts> bool b = __is_constructible(Ts...);
                               ^~~~~~~~~~~~~~~~~~
clang/test/SemaCXX/type-trait-eval-crash-issue-57008.cpp:6:10: note: in instantiation of variable template specialization 'b<>' requested here
bool x = b<>;
         ^
1 error generated.

See https://godbolt.org/z/q39W78hsK.

Fixes https://github.com/llvm/llvm-project/issues/57008

[inclyc]

GCC MSVC ICC seems to have some different behavior, clang after introducing this patch will think there is no problem with the code and output binary files, and x evaluates to false

https://godbolt.org/z/hvbqTYMsx

[shafik]

clang/lib/Sema/SemaExprCXX.cpp

5307

I don't believe this is the right fix, the assert below is saying that we should not be here if Args.empty() so we are doing something wrong before this point.

This will prevent the crash but cover up the real issue.

Unless we have some foundation for saying the assert is not correct but I don't see that.

[inclyc]

It seems that we are missing checks about if there is no enough parameter to intrinsic template __is_constructible here, and we should report an error. [WIP]

[inclyc]

check argument number before evaulating

[tbaeder]

clang/lib/Sema/SemaExprCXX.cpp

5445

This function copies the two conditions for the if-statements from evaluateTypeTrait(). Is just doing it in evaluateTypeTrait() too late? Duplicating that seems bad.

[inclyc]

clang/lib/Sema/SemaExprCXX.cpp

5445

evaluateTypeTrait() should return evaluate result, i.e. bool value true / false, if we do this in evaluateTypeTrait() , how can we express that we encountered some error? I think it is reasonable to split these conditions here.

[inclyc]

clang/lib/Sema/SemaExprCXX.cpp

5445

Do you think we should add a condition checks to the evaluate function evaluateTypeTrait() and return false if it is not satisfied?

[inclyc]

clang/lib/Sema/SemaExprCXX.cpp

5431

I think we should report a warning or an error if this function fails.

[inclyc]

@aaron.ballman @shafik (Help wanted). These type traits will not cause clang to crash if current patch applied, but the bool x in the test case will evaluate to false. I think the problem behind the github issue is that we didn't check the number of arguments for builtin type traits like __is_constructible

I want to ask what is our expected behavior for this, give an error like using std::is_constructible or just consider __is_constructible should always have a bool value? Should we report a warning but compile translation unit, or report an error and terminate?

[shafik]

In D131423#3712083, @inclyc wrote:

@aaron.ballman @shafik (Help wanted). These type traits will not cause clang to crash if current patch applied, but the bool x in the test case will evaluate to false. I think the problem behind the github issue is that we didn't check the number of arguments for builtin type traits like __is_constructible

I want to ask what is our expected behavior for this, give an error like using std::is_constructible or just consider __is_constructible should always have a bool value? Should we report a warning but compile translation unit, or report an error and terminate?

I think it should be an error but @aaron.ballman may have a different idea.

[aaron.ballman]

In D131423#3714318, @shafik wrote:

In D131423#3712083, @inclyc wrote:

@aaron.ballman @shafik (Help wanted). These type traits will not cause clang to crash if current patch applied, but the bool x in the test case will evaluate to false. I think the problem behind the github issue is that we didn't check the number of arguments for builtin type traits like __is_constructible

I want to ask what is our expected behavior for this, give an error like using std::is_constructible or just consider __is_constructible should always have a bool value? Should we report a warning but compile translation unit, or report an error and terminate?

I think it should be an error but @aaron.ballman may have a different idea.

I agree; I think it should be an error.

[inclyc]

address comments

[inclyc]

rebase

[inclyc]

clang/lib/Parse/ParseExprCXX.cpp

3783

Before this patch, we check the Arity here, so using type traits with unexpected number of arguments does not crash clang

See https://godbolt.org/z/afGxvqdEh

clang/lib/Sema/SemaExprCXX.cpp

5433

Moves to here.

[shafik]

clang/lib/Basic/TypeTraits.cpp

64

@aaron.ballman do we really have to include this three times? We are defining different macros so shouldn't we be able to include is just once?

I see we do this in several other places but a few we don't.

clang/lib/Parse/ParseExprCXX.cpp

3789

Why doesn't this catch our case but moving the check into evaluateTypeTrait does?

[inclyc]

clang/lib/Basic/TypeTraits.cpp

64

I've tried

#define TYPE_TRAIT(N, I, K) N,
#include "clang/Basic/TokenKinds.def"

But using enum TypeTrait as array index seems to have incorrect mapping.

clang/lib/Parse/ParseExprCXX.cpp

3789

In this case:

template<class... Ts> bool b = __is_constructible(Ts...); // Parser: 1 argument `Ts`, no problem
bool x = b<>; // After template template instantiation tree transformation: 0 argument, but missing checks!

Ts... is considered as 1 argument in Parsing, so passed this check.

Why doesn't this catch our case but moving the check into evaluateTypeTrait does?

This patch moved the check to Sema::BuildTypeTrait, this function will be invoked by tree transformation procedure.

invoked by tree transformation procedure

In fact: clang::TreeTransform<(anonymous namespace)::TemplateInstantiator>::TransformTypeTraitExpr(clang::TypeTraitExpr*) SemaTemplateInstantiate.cpp).

I think it is not a good idea to move the check into evaluateTypeTrait, because this function might be designed to return the final evaluation result, we should perform the check _before_ this.

[inclyc]

clang/lib/Basic/TypeTraits.cpp

64

@aaron.ballman do we really have to include this three times? We are defining different macros so shouldn't we be able to include is just once?

I see we do this in several other places but a few we don't.

Type trait definitions in #include "clang/Basic/TokenKinds.def" may not be sorted by the number of arguments.

Example:

#define TYPE_TRAIT_1(some_stuff)
#define TYPE_TRAIT_N(some_stuff)
#define TYPE_TRAIT_2(some_stuff)

Might be necessary to include this 3 times to get sorted layouts, like

[1, 1, 1, 2, 2, 2, 0, 0]

[inclyc]

move implemention of CheckTypeTraitArity into SemaExprCXX, since type traits are C++ specific language extension

[aaron.ballman]

LGTM, but you should add a release note for the change.

clang/lib/Basic/TypeTraits.cpp

64

Yeah, I think we ultimately either need to reorder the TokenKinds.def file to be in sorted order (fragile, surprising, tight coupling) or we have to do this dance because we need the initialization itself to be sorted.

[inclyc]

add release notes

[inclyc]

.

clang/include/clang/Basic/TypeTraits.h

 const char *getTraitName(TypeTrait T) LLVM_READONLY;
 const char *getTraitName(ArrayTypeTrait T) LLVM_READONLY;
 const char *getTraitName(UnaryExprOrTypeTrait T) LLVM_READONLY;
 
 /// Return the spelling of the type trait \p TT. Never null.
 const char *getTraitSpelling(TypeTrait T) LLVM_READONLY;
 const char *getTraitSpelling(ArrayTypeTrait T) LLVM_READONLY;
 const char *getTraitSpelling(UnaryExprOrTypeTrait T) LLVM_READONLY;
+
+/// Return the arity of the type trait \p T. Never null.
+unsigned getTypeTraitArity(TypeTrait T) LLVM_READONLY;
+
 } // namespace clang
 
 #endif

clang/include/clang/Sema/Sema.h

public:
   ExprResult BuildUnaryOp(Scope *S, SourceLocation OpLoc,
                           UnaryOperatorKind Opc, Expr *Input);
   ExprResult ActOnUnaryOp(Scope *S, SourceLocation OpLoc,
                           tok::TokenKind Op, Expr *Input);
 
   bool isQualifiedMemberAccess(Expr *E);
   QualType CheckAddressOfOperand(ExprResult &Operand, SourceLocation OpLoc);
 
+  bool CheckTypeTraitArity(unsigned Arity, SourceLocation Loc, size_t N);
+
   ExprResult CreateUnaryExprOrTypeTraitExpr(TypeSourceInfo *TInfo,
                                             SourceLocation OpLoc,
                                             UnaryExprOrTypeTrait ExprKind,
                                             SourceRange R);
   ExprResult CreateUnaryExprOrTypeTraitExpr(Expr *E, SourceLocation OpLoc,
                                             UnaryExprOrTypeTrait ExprKind);
   ExprResult
     ActOnUnaryExprOrTypeTraitExpr(SourceLocation OpLoc,

clang/lib/Basic/TypeTraits.cpp

 };
 
 static constexpr const char *UnaryExprOrTypeTraitSpellings[] = {
 #define UNARY_EXPR_OR_TYPE_TRAIT(Spelling, Name, Key) #Spelling,
 #define CXX11_UNARY_EXPR_OR_TYPE_TRAIT(Spelling, Name, Key) #Spelling,
 #include "clang/Basic/TokenKinds.def"
 };
 
+static constexpr const unsigned TypeTraitArities[] = {
+#define TYPE_TRAIT_1(Spelling, Name, Key) 1,
+#include "clang/Basic/TokenKinds.def"
+#define TYPE_TRAIT_2(Spelling, Name, Key) 2,
+#include "clang/Basic/TokenKinds.def"
+#define TYPE_TRAIT_N(Spelling, Name, Key) 0,
+#include "clang/Basic/TokenKinds.def"
+};
+
 const char *clang::getTraitName(TypeTrait T) {
   assert(T <= TT_Last && "invalid enum value!");
   return TypeTraitNames[T];
 }
 
 const char *clang::getTraitName(ArrayTypeTrait T) {
   assert(T <= ATT_Last && "invalid enum value!");
   return ArrayTypeTraitNames[T];
const char *clang::getTraitSpelling(ArrayTypeTrait T) {
   assert(T <= ATT_Last && "invalid enum value!");
   return ArrayTypeTraitSpellings[T];
 }
 
 const char *clang::getTraitSpelling(UnaryExprOrTypeTrait T) {
   assert(T <= UETT_Last && "invalid enum value!");
   return UnaryExprOrTypeTraitSpellings[T];
 }
+
+unsigned clang::getTypeTraitArity(TypeTrait T) {
+  assert(T <= TT_Last && "invalid enum value!");
+  return TypeTraitArities[T];
+}

clang/lib/Parse/ParseExprCXX.cpp

default:
     llvm_unreachable("Not a known unary expression trait.");
 #define EXPRESSION_TRAIT(Spelling, Name, Key)                                  \
   case tok::kw_##Spelling:                                                     \
     return ET_##Name;
 #include "clang/Basic/TokenKinds.def"
   }
 }
 
-static unsigned TypeTraitArity(tok::TokenKind kind) {
-  switch (kind) {
-    default: llvm_unreachable("Not a known type trait");
-#define TYPE_TRAIT(N,Spelling,K) case tok::kw_##Spelling: return N;
-#include "clang/Basic/TokenKinds.def"
-  }
-}
-
 /// Parse the built-in type-trait pseudo-functions that allow
 /// implementation of the TR1/C++11 type traits templates.
 ///
 ///       primary-expression:
 ///          unary-type-trait '(' type-id ')'
 ///          binary-type-trait '(' type-id ',' type-id ')'
 ///          type-trait '(' type-id-seq ')'
 ///
 ///       type-id-seq:
 ///          type-id ...[opt] type-id-seq[opt]
 ///
 ExprResult Parser::ParseTypeTrait() {
   tok::TokenKind Kind = Tok.getKind();
-  unsigned Arity = TypeTraitArity(Kind);
 
   SourceLocation Loc = ConsumeToken();
 
   BalancedDelimiterTracker Parens(*this, tok::l_paren);
   if (Parens.expectAndConsume())
     return ExprError();
 
   SmallVector<ParsedType, 2> Args;
do {
     Args.push_back(Ty.get());
   } while (TryConsumeToken(tok::comma));
 
   if (Parens.consumeClose())
     return ExprError();
 
   SourceLocation EndLoc = Parens.getCloseLocation();
 
-  if (Arity && Args.size() != Arity) {
-    Diag(EndLoc, diag::err_type_trait_arity)
-      << Arity << 0 << (Arity > 1) << (int)Args.size() << SourceRange(Loc);
-    return ExprError();
-  }
-
-  if (!Arity && Args.empty()) {
-    Diag(EndLoc, diag::err_type_trait_arity)
-      << 1 << 1 << 1 << (int)Args.size() << SourceRange(Loc);
-    return ExprError();
-  }
-
   return Actions.ActOnTypeTrait(TypeTraitFromTokKind(Kind), Loc, Args, EndLoc);
 }
 
 /// ParseArrayTypeTrait - Parse the built-in array type-trait
 /// pseudo-functions.
 ///
 ///       primary-expression:
 /// [Embarcadero]     '__array_rank' '(' type-id ')'

clang/lib/Sema/SemaExpr.cpp

case Type::TypeOfExpr:
       break;
     case Type::Atomic:
       T = cast<AtomicType>(Ty)->getValueType();
       break;
     }
   } while (!T.isNull() && T->isVariablyModifiedType());
 }
 
+bool Sema::CheckTypeTraitArity(unsigned Arity, SourceLocation Loc, size_t N) {
+  if (Arity && N != Arity) {
+    Diag(Loc, diag::err_type_trait_arity)
+        << Arity << 0 << (Arity > 1) << (int)N << SourceRange(Loc);
+    return false;
+  }
+
+  if (!Arity && N == 0) {
+    Diag(Loc, diag::err_type_trait_arity)
+        << 1 << 1 << 1 << (int)N << SourceRange(Loc);
+    return false;
+  }
+  return true;
+}
+
 /// Build a sizeof or alignof expression given a type operand.
 ExprResult
 Sema::CreateUnaryExprOrTypeTraitExpr(TypeSourceInfo *TInfo,
                                      SourceLocation OpLoc,
                                      UnaryExprOrTypeTrait ExprKind,
                                      SourceRange R) {
   if (!TInfo)
     return ExprError();

clang/lib/Sema/SemaExprCXX.cpp

case clang::TT_IsTriviallyConstructible: {
     //     no operation that is not trivial.
     //
     //   The predicate condition for a template specialization
     //   is_constructible<T, Args...> shall be satisfied if and only if the
     //   following variable definition would be well-formed for some invented
     //   variable t:
     //
     //     T t(create<Args>()...);
     assert(!Args.empty());
 
     // Precondition: T and all types in the parameter pack Args shall be
     // complete types, (possibly cv-qualified) void, or arrays of
     // unknown bound.
     for (const auto *TSI : Args) {
       QualType ArgTy = TSI->getType();
       if (ArgTy->isVoidType() || ArgTy->isIncompleteArrayType())
         continue;
default:
       return;
   }
   S.Diag(KWLoc, diag::warn_deprecated_builtin)
     << getTraitSpelling(Kind) << getTraitSpelling(Replacement);
 }
 }
 
 ExprResult Sema::BuildTypeTrait(TypeTrait Kind, SourceLocation KWLoc,
                                 ArrayRef<TypeSourceInfo *> Args,
                                 SourceLocation RParenLoc) {
+  if (!CheckTypeTraitArity(getTypeTraitArity(Kind), KWLoc, Args.size()))
+    return ExprError();
   QualType ResultType = Context.getLogicalOperationType();
 
   if (Kind <= UTT_Last && !CheckUnaryTypeTraitTypeCompleteness(
                                *this, Kind, KWLoc, Args[0]->getType()))
     return ExprError();
 
   DiagnoseBuiltinDeprecation(*this, Kind, KWLoc);
 
   bool Dependent = false;
   for (unsigned I = 0, N = Args.size(); I != N; ++I) {
     if (Args[I]->getType()->isDependentType()) {
       Dependent = true;
       break;
     }
   }
 
   bool Result = false;
   if (!Dependent)
     Result = evaluateTypeTrait(*this, Kind, KWLoc, Args, RParenLoc);

clang/test/SemaCXX/type-trait-eval-crash-issue-57008.cpp

+// RUN: %clang_cc1 -fsyntax-only %s -verify
+
+// Shouldn't crash here
+// Reported by https://github.com/llvm/llvm-project/issues/57008
+template<class... Ts> bool b = __is_constructible(Ts...); // expected-error{{type trait requires 1 or more arguments}}
+bool x = b<>; // expected-note{{in instantiation of variable template specialization}}