Page MenuHomePhabricator

Fix __has_trivial_destructor crash when the type is incomplete with unknown array bounds.
Needs ReviewPublic

Authored by puneetha on Jun 14 2017, 2:25 AM.

Details

Reviewers
rsmith
rjmccall
Summary

Clang crashes at this test case,
struct I;
int a = __has_trivial_destructor(I[]);

With this error,
clang-5.0: /llvm/tools/clang/include/clang/AST/DeclCXX.h:604: clang::CXXRecordDecl::DefinitionData& clang::CXXRecordDecl::data() const: Assertion `DD && "queried property of class with no definition"' failed.

#9 0xffffffffb737c7c7 clang::CXXRecordDecl::data() const /llvm/tools/clang/include/clang/AST/DeclCXX.h:605:0
#10 0xffffffffb737c877 clang::CXXRecordDecl::hasTrivialDestructor() const /llvm/tools/clang/include/clang/AST/DeclCXX.h:1293:0
#11 0x0a9d3ea2 EvaluateUnaryTypeTrait(clang::Sema&, clang::TypeTrait, clang::SourceLocation, clang::QualType) /llvm/tools/clang/lib/Sema/SemaExprCXX.cpp:4416:0
#12 0x0aa5e589 evaluateTypeTrait(clang::Sema&, clang::TypeTrait, clang::SourceLocation, llvm::ArrayRef<clang::TypeSourceInfo*>, clang::SourceLocation) /llvm/tools/clang/lib/Sema/SemaExprCXX.cpp:4560:0

Analysis:
According to the https://gcc.gnu.org/onlinedocs/gcc/Type-Traits.html, __has_trivial_destructor(type) requires type to be a complete type, (possibly cv-qualified) void, or an array of unknown bound.
When we pass an array of unknown bounds of an incomplete type, it crashes because clang is not able to get the DefinitionData of the type which is empty. But an if the type is array of unknown bounds but is complete type, then the DefinitionData is not empty, so there is no crash.

GCC handles this by giving an error if we pass an array of unknown bounds of an incomplete type. This patch is added to give a similar error when compiled with clang, as well.

Thanks,
Puneetha

Diff Detail

Event Timeline

puneetha created this revision.Jun 14 2017, 2:25 AM
puneetha removed rL LLVM as the repository for this revision.Jun 14 2017, 2:32 AM
puneetha edited the summary of this revision. (Show Details)Jun 14 2017, 3:21 AM
puneetha added reviewers: rsmith, rjmccall.
puneetha added a subscriber: cfe-commits.
puneetha updated this revision to Diff 102516.Jun 14 2017, 3:24 AM

Incorrectly rejecting __is_destructible queries on arrays of unknown bound of incomplete types.

rjmccall edited edge metadata.Jun 14 2017, 9:17 AM

Test cases?

rjmccall requested changes to this revision.Jun 14 2017, 9:17 AM
This revision now requires changes to proceed.Jun 14 2017, 9:17 AM
puneetha updated this revision to Diff 102635.Jun 14 2017, 8:15 PM
puneetha edited edge metadata.

Added testcase.

rjmccall added inline comments.Jun 20 2017, 9:07 AM
lib/Sema/SemaExprCXX.cpp
4125

I don't understand the difference you're creating between traits here. Three specific traits about destructibility allow incomplete array types regardless of whether the base type is incomplete, but the rest do not?

Anyway, I think what you want here is basically just:

if (auto ArrayTy = S.Context.getAsIncompleteArrayType(ArgTy)) {
  ArgTy = ArrayTy->getElementType();
}
rsmith added inline comments.Jun 20 2017, 4:07 PM
lib/Sema/SemaExprCXX.cpp
4088–4089

Please move the UTT_Has* cases up here, since this is the rule that actually applies to them.

4096–4098

Please update this comment to indicate that the GCC documentation is wrong about the constraints that GCC actually imposes on these traits.

puneetha updated this revision to Diff 103337.Jun 21 2017, 3:31 AM

Updated files to address the review comments.

puneetha marked 2 inline comments as done.Jun 21 2017, 3:37 AM
puneetha added inline comments.
lib/Sema/SemaExprCXX.cpp
4125

Of my understanding, these traits are defined by MSVC. There is no mention of them in the GCC type-traits documentation. For these traits, GCC lets us pass an array of incomplete bounds for any base type, complete or incomplete.

Please correct me if I am wrong.

rjmccall added inline comments.Jun 21 2017, 11:16 AM
lib/Sema/SemaExprCXX.cpp
4125

I see. If we're matching GCC bug-for-bug, it doesn't really matter if the behavior seems inconsistent.

rsmith accepted this revision.Jun 21 2017, 2:00 PM
rsmith added inline comments.
lib/Sema/SemaExprCXX.cpp
4125

Generally: when the C++ standard has a trait named std::foo and Clang has a __foo builtin, the latter is a complete implementation of the former (and if other compilers have bugs in their implementations, we are not bug-for-bug compatible). That covers the UTT_Is* traits here.

The other traits are typically extensions from other vendors, often used to implement pre-standard names for traits. Many of those are considered deprecated, and we try to be mostly bug-for-bug compatible with GCC or MSVC when implementing them. That covers the UTT_Has* traits here.

Per commentary in PR33232, MSVC's STL has nearly entirely moved off the deprecated traits, so it's mostly GCC compatibility we care about here.

It'd be good to also add tests for the other traits you're changing.

puneetha updated this revision to Diff 103530.Jun 21 2017, 11:29 PM

Added test cases for all the affecting traits.

rsmith accepted this revision.Jun 27 2017, 12:54 PM

LGTM, thanks. Do you need someone to commit for you?

This is committed by karthik, at r306519.

Thank you.