[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

5447

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

5447

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

5447

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

5433

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 ↗

(On Diff #451932)

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

5435

Moves to here.

[shafik]

clang/lib/Basic/TypeTraits.cpp

64 ↗

(On Diff #451932)

@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 ↗

(On Diff #451932)

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

[inclyc]

clang/lib/Basic/TypeTraits.cpp

64 ↗

(On Diff #451932)

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 ↗

(On Diff #451932)

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 ↗

(On Diff #451932)

@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 ↗

(On Diff #451932)

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/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>()...);
+    if (Args.empty())
+      return false;
     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())
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) {
   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);
 
   return TypeTraitExpr::Create(Context, ResultType, KWLoc, Kind, Args,

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

+// RUN: %clang_cc1 -fsyntax-only %s
+
+// Shouldn't crash here
+// Reported by https://github.com/llvm/llvm-project/issues/57008
+template<class... Ts> bool b = __is_constructible(Ts...);
+bool x = b<>;