This is an archive of the discontinued LLVM Phabricator instance.

[clang] fix frontend crash when evaluating type trait
ClosedPublic

Authored by inclyc on Aug 8 2022, 11:53 AM.

Details

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

Diff Detail

Event Timeline

inclyc created this revision.Aug 8 2022, 11:53 AM
Herald added a project: Restricted Project. · View Herald TranscriptAug 8 2022, 11:53 AM
inclyc published this revision for review.Aug 8 2022, 11:57 AM
inclyc added reviewers: aaron.ballman, gribozavr2.
inclyc added a reviewer: NoQ.
inclyc added a subscriber: Restricted Project.
Herald added a project: Restricted Project. · View Herald TranscriptAug 8 2022, 11:57 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript
inclyc added a comment.Aug 8 2022, 9:45 PM

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 added inline comments.Aug 8 2022, 9:45 PM
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 abandoned this revision.Aug 8 2022, 9:47 PM
inclyc reclaimed this revision.Aug 8 2022, 11:07 PM

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 planned changes to this revision.Aug 8 2022, 11:08 PM
inclyc updated this revision to Diff 451056.Aug 8 2022, 11:59 PM

check argument number before evaulating

tbaeder added a subscriber: tbaeder.Aug 9 2022, 7:38 AM
tbaeder added inline comments.
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 added inline comments.Aug 9 2022, 7:42 AM
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 added inline comments.Aug 9 2022, 8:04 AM
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 planned changes to this revision.Aug 9 2022, 5:31 PM
inclyc added inline comments.
clang/lib/Sema/SemaExprCXX.cpp
5431

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

inclyc added a comment.EditedAug 10 2022, 3:38 AM

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

@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 @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 updated this revision to Diff 451931.Aug 11 2022, 11:25 AM

address comments

inclyc edited the summary of this revision. (Show Details)Aug 11 2022, 11:28 AM
inclyc added inline comments.Aug 11 2022, 11:39 AM
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.

inclyc marked 2 inline comments as done.Aug 11 2022, 11:40 AM
shafik added inline comments.Aug 11 2022, 9:20 PM
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 added inline comments.Aug 11 2022, 9:46 PM
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 added inline comments.Aug 11 2022, 9:54 PM
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 updated this revision to Diff 452120.Aug 12 2022, 2:40 AM

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

aaron.ballman accepted this revision.Aug 12 2022, 7:47 AM

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.

This revision is now accepted and ready to land.Aug 12 2022, 7:47 AM
inclyc updated this revision to Diff 452198.Aug 12 2022, 8:49 AM

add release notes

This revision was landed with ongoing or failed builds.Aug 12 2022, 9:03 AM
This revision was automatically updated to reflect the committed changes.