-
Notifications
You must be signed in to change notification settings - Fork 11.8k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[Clang][Sema] fix crash in codegen stage when an lambda expression declared in an unevaluated context #80802
[Clang][Sema] fix crash in codegen stage when an lambda expression declared in an unevaluated context #80802
Conversation
@llvm/pr-subscribers-clang Author: Qizhi Hu (jcsxky) ChangesTry to fix issue Full diff: https://github.com/llvm/llvm-project/pull/80802.diff 3 Files Affected:
diff --git a/clang/docs/ReleaseNotes.rst b/clang/docs/ReleaseNotes.rst
index 4d57ea4fd55b8..7ad575e4f57fa 100644
--- a/clang/docs/ReleaseNotes.rst
+++ b/clang/docs/ReleaseNotes.rst
@@ -204,6 +204,8 @@ Bug Fixes to C++ Support
parameter where we did an incorrect specialization of the initialization of
the default parameter.
Fixes (`#68490 <https://github.com/llvm/llvm-project/issues/68490>`_)
+- Fix a crash in codegen when lambdas declared in an unevaluated context.
+ Fixes (`#76674 <https://github.com/llvm/llvm-project/issues/76674>`_)
Bug Fixes to AST Handling
^^^^^^^^^^^^^^^^^^^^^^^^^
diff --git a/clang/lib/Sema/SemaTemplateInstantiate.cpp b/clang/lib/Sema/SemaTemplateInstantiate.cpp
index 6d59180bc446d..383163ea969a9 100644
--- a/clang/lib/Sema/SemaTemplateInstantiate.cpp
+++ b/clang/lib/Sema/SemaTemplateInstantiate.cpp
@@ -1613,8 +1613,8 @@ namespace {
bool TemplateInstantiator::AlreadyTransformed(QualType T) {
if (T.isNull())
return true;
-
- if (T->isInstantiationDependentType() || T->isVariablyModifiedType())
+ if (T->isInstantiationDependentType() || T->isVariablyModifiedType() ||
+ (SemaRef.getLangOpts().CPlusPlus20 && T->isDecltypeType()))
return false;
getSema().MarkDeclarationsReferencedInType(Loc, T);
@@ -2685,7 +2685,8 @@ QualType Sema::SubstType(QualType T,
// If T is not a dependent type or a variably-modified type, there
// is nothing to do.
- if (!T->isInstantiationDependentType() && !T->isVariablyModifiedType())
+ if (!T->isInstantiationDependentType() && !T->isVariablyModifiedType() &&
+ (getLangOpts().CPlusPlus20 && !T->isDecltypeType()))
return T;
TemplateInstantiator Instantiator(*this, TemplateArgs, Loc, Entity);
diff --git a/clang/test/SemaTemplate/PR76674.cpp b/clang/test/SemaTemplate/PR76674.cpp
new file mode 100644
index 0000000000000..50e9053e41e0f
--- /dev/null
+++ b/clang/test/SemaTemplate/PR76674.cpp
@@ -0,0 +1,11 @@
+// RUN: %clang_cc1 -c -std=c++20 -verify=cxx20 -o /dev/null %s
+// expected-no-diagnostics
+
+template <class>
+struct A {
+ template <class U>
+ using Func = decltype([] {return U{};});
+};
+
+A<int>::Func<int> f{};
+int i{f()};
|
d767344
to
741793c
Compare
|
||
if (T->isInstantiationDependentType() || T->isVariablyModifiedType()) | ||
if (T->isInstantiationDependentType() || T->isVariablyModifiedType() || | ||
(SemaRef.getLangOpts().CPlusPlus20 && T->isDecltypeType())) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you please add the relevant standards-quote here, particularly for the C++20 vs pre-C++20 change here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note that this would at best retransform all the decltype
which does sound wrong?
I'd rather try to understand why isInstantiationDependentType
gives a bogus result, although that might be involved.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@cor3ntin I use this test case locally:
A<double>::Func<long long> f{};
AST looks like:
Breakpoint 42, clang::DecltypeType::DecltypeType (this=0x39d190, E=0x39cfd0, underlyingType=..., can=...) at /home/huqizhi/Downloads/llvm-project/clang/lib/AST/Type.cpp:3797
3797 E(E), UnderlyingType(underlyingType) {}
(gdb-ChatDBG) p E->dumpColor()
CXXBindTemporaryExpr 0x39cfd0 'class A<double>::(lambda at /home/huqizhi/K.cpp:18:27)' (CXXTemporary 0x39cfd0)
`-LambdaExpr 0x39cfa0 'class A<double>::(lambda at /home/huqizhi/K.cpp:18:27)'
|-CXXRecordDecl 0x39ca60 implicit class definition
| |-DefinitionData lambda pass_in_registers empty standard_layout trivially_copyable trivial literal has_constexpr_non_copy_move_ctor can_const_default_init
| | |-DefaultConstructor exists trivial constexpr needs_implicit defaulted_is_constexpr
| | |-CopyConstructor simple trivial has_const_param needs_implicit implicit_has_const_param
| | |-MoveConstructor exists simple trivial needs_implicit
| | |-CopyAssignment simple trivial has_const_param needs_implicit implicit_has_const_param
| | |-MoveAssignment exists simple trivial needs_implicit
| | `-Destructor simple irrelevant trivial constexpr
| |-CXXMethodDecl 0x39cbc8 constexpr operator() 'auto (void) const -> auto' implicit_instantiation inline instantiated_from 0x37cc18
| | `-CompoundStmt 0x39cd58
| | `-ReturnStmt 0x39cd48
| | `-CXXUnresolvedConstructExpr 0x39cd08 'U' 'U' list
| | `-InitListExpr 0x39ccc8 'void'
| |-CXXConversionDecl 0x39cdf0 implicit constexpr operator auto (*)() 'auto (*(void) const noexcept)(void) -> auto' inline
| |-CXXMethodDecl 0x39ceb0 implicit __invoke 'auto (void) -> auto' static inline
| `-CXXDestructorDecl 0x39cff8 implicit referenced constexpr ~(lambda at /home/huqizhi/K.cpp:18:27) 'void (void) noexcept' inline default trivial
`-CompoundStmt 0x39cd58
`-ReturnStmt 0x39cd48
`-CXXUnresolvedConstructExpr 0x39cd08 'U' 'U' list
`-InitListExpr 0x39ccc8 'void'
CXXBindTemporaryExpr
and LambdaExpr
both are not InstantiationDependentType
(gdb-ChatDBG) p E->isInstantiationDependent()
$306 = false
(gdb-ChatDBG) p ((CXXBindTemporaryExpr*)E)->getSubExpr()->isInstantiationDependent()
$308 = false
This is because type of the lambda is not InstantiationDependentType
:
(gdb-ChatDBG) p ((LambdaExpr*)(((CXXBindTemporaryExpr*)E)->getSubExpr()))->getType().dump()
RecordType 0x39cba0 'class A<double>::(lambda at /home/huqizhi/K.cpp:18:27)'
`-CXXRecord 0x39ca60 ''
$312 = void
I think retransforming all the decltype
should be right because retransformation should not change the result. But, this may involve some efficiency problem.
Since CXXUnresolvedConstructExpr
in the lambda expression is InstantiationDependentType
, Maybe visiting the lambda expression and checking whether it has InstantiationDependentType
child is a better approach (I think, but more complex).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you please add the relevant standards-quote here, particularly for the C++20 vs pre-C++20 change here?
Sorry, I couldn't get what you mean of relevant standards-quote and I am not very familiar with cpp standards. I add SemaRef.getLangOpts().CPlusPlus20
just because command args contains -std=c++20
and clang diagnose says 'lambda expression in an unevaluated operand' and don't retransform decltype
all the time(issue only exists in code of cpp20 and later).
1706840
to
376d5b0
Compare
59fb684
to
0063efb
Compare
@cor3ntin @erichkeane Could you please take another look at this PR? I think current version would be a reasonable fix since it transforms type only when it's needed. Or you have any other opinions? Thank you for guidance! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A few suggestions, but I REALLY would like @cor3ntin to take a final pass, it mostly LGTM.
@@ -1614,7 +1614,19 @@ bool TemplateInstantiator::AlreadyTransformed(QualType T) { | |||
if (T.isNull()) | |||
return true; | |||
|
|||
if (T->isInstantiationDependentType() || T->isVariablyModifiedType()) | |||
bool DependentLambdaType = false; | |||
QualType DesugaredType = T.getDesugaredType(SemaRef.getASTContext()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this step necessary? getAsCXXRecordDecl
should at least canonicalize.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Indeed, code has been fixed.
bool DependentLambdaType = false; | ||
QualType DesugaredType = T.getDesugaredType(SemaRef.getASTContext()); | ||
CXXRecordDecl *RD = DesugaredType->getAsCXXRecordDecl(); | ||
if (RD && RD->isLambda()) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if (auto *RD = T->getAsCXXRecordDecl(); RD && RD->isLambda())
??
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed
2f09be5
to
5430d07
Compare
…clared in an unevaluated context
5430d07
to
842f848
Compare
This was fixed by #82310 |
Try to fix issue
When transform a lambda expression which is declared in an unevaluated context,
isInstantiationDependentType()
andisVariablyModifiedType()
both return false and lead to skip transforming the lambda expression. On the other hand,AlreadyTransformed
also skip transformation in this case. Add the condition to check whether it's in decltype makes it work.