[clang] Fix crash when declaring invalid lambda member #74110
Conversation
llvmbot
added
clang
|
@llvm/pr-subscribers-clang Author: Nikolas Klauser (philnik777) ChangesI've come across this while working on implementing lambdas in C++03. Full diff: https://github.com/llvm/llvm-project/pull/74110.diff 2 Files Affected:
diff --git a/clang/lib/AST/DeclCXX.cpp b/clang/lib/AST/DeclCXX.cpp
index c944862fcefeeef..e9e62fea9fb376a 100644
--- a/clang/lib/AST/DeclCXX.cpp
+++ b/clang/lib/AST/DeclCXX.cpp
@@ -1526,10 +1526,9 @@ bool CXXRecordDecl::isGenericLambda() const {
#ifndef NDEBUG
static bool allLookupResultsAreTheSame(const DeclContext::lookup_result &R) {
- for (auto *D : R)
- if (!declaresSameEntity(D, R.front()))
- return false;
- return true;
+ return llvm::all_of(R, [&](NamedDecl *D) {
+ return D->isInvalidDecl() || declaresSameEntity(D, R.front());
+ });
}
#endif
diff --git a/clang/test/SemaCXX/lambda-expressions.cpp b/clang/test/SemaCXX/lambda-expressions.cpp
index 1797eef320b8659..1921d02b1a9cf8b 100644
--- a/clang/test/SemaCXX/lambda-expressions.cpp
+++ b/clang/test/SemaCXX/lambda-expressions.cpp
@@ -1,3 +1,4 @@
+// RUN: %clang_cc1 -std=c++11 -Wno-unused-value -fsyntax-only -verify=expected,expected-cxx14,cxx11 -fblocks %s
// RUN: %clang_cc1 -std=c++14 -Wno-unused-value -fsyntax-only -verify -verify=expected-cxx14 -fblocks %s
// RUN: %clang_cc1 -std=c++17 -Wno-unused-value -verify -ast-dump -fblocks %s | FileCheck %s
@@ -558,8 +559,8 @@ struct B {
int x;
A a = [&] { int y = x; };
A b = [&] { [&] { [&] { int y = x; }; }; };
- A d = [&](auto param) { int y = x; };
- A e = [&](auto param) { [&] { [&](auto param2) { int y = x; }; }; };
+ A d = [&](auto param) { int y = x; }; // cxx11-error {{'auto' not allowed in lambda parameter}}
+ A e = [&](auto param) { [&] { [&](auto param2) { int y = x; }; }; }; // cxx11-error 2 {{'auto' not allowed in lambda parameter}}
};
B<int> b;
@@ -589,6 +590,7 @@ struct S1 {
void foo1() {
auto s0 = S1{[name=]() {}}; // expected-error 2 {{expected expression}}
auto s1 = S1{[name=name]() {}}; // expected-error {{use of undeclared identifier 'name'; did you mean 'name1'?}}
+ // cxx11-warning@-1 {{initialized lambda captures are a C++14 extension}}
}
}
@@ -604,7 +606,7 @@ namespace PR25627_dont_odr_use_local_consts {
namespace ConversionOperatorDoesNotHaveDeducedReturnType {
auto x = [](int){};
- auto y = [](auto &v) -> void { v.n = 0; };
+ auto y = [](auto &v) -> void { v.n = 0; }; // cxx11-error {{'auto' not allowed in lambda parameter}} cxx11-note {{candidate function not viable}} cxx11-note {{conversion candidate}}
using T = decltype(x);
using U = decltype(y);
using ExpectedTypeT = void (*)(int);
@@ -624,14 +626,14 @@ namespace ConversionOperatorDoesNotHaveDeducedReturnType {
template<typename T>
friend constexpr U::operator ExpectedTypeU<T>() const noexcept;
#else
- friend auto T::operator()(int) const;
+ friend auto T::operator()(int) const; // cxx11-error {{'auto' return without trailing return type; deduced return types are a C++14 extension}}
friend T::operator ExpectedTypeT() const;
template<typename T>
- friend void U::operator()(T&) const;
+ friend void U::operator()(T&) const; // cxx11-error {{friend declaration of 'operator()' does not match any declaration}}
// FIXME: This should not match, as above.
template<typename T>
- friend U::operator ExpectedTypeU<T>() const;
+ friend U::operator ExpectedTypeU<T>() const; // cxx11-error {{friend declaration of 'operator void (*)(type-parameter-0-0 &)' does not match any declaration}}
#endif
private:
@@ -639,7 +641,7 @@ namespace ConversionOperatorDoesNotHaveDeducedReturnType {
};
// Should be OK: lambda's call operator is a friend.
- void use(X &x) { y(x); }
+ void use(X &x) { y(x); } // cxx11-error {{no matching function for call to object}}
// This used to crash in return type deduction for the conversion opreator.
struct A { int n; void f() { +[](decltype(n)) {}; } };
|
| return llvm::all_of(R, [&](NamedDecl *D) { | ||
| return D->isInvalidDecl() || declaresSameEntity(D, R.front()); | ||
| }); |
There was a problem hiding this comment.
Should we move the isInvalidDecl check to declaresSameEntity?
I doubt this is the only place this might happen
There was a problem hiding this comment.
From what I can tell nowhere is checking what they pass to declaresSameEntity(...) but I did not dig too deeply to see if we know they are valid earlier in some way.
|
Can you add a little more details in the description on the root cause of the bug? |
They mostly are. I found this while trying to check C++03 with the test and noticed that C++11 also crashes, so I updated the test to also run in C++11. |
|
gentle ping |
|
ping |
|
Ping (@AaronBallman maybe?) |
There was a problem hiding this comment.
The changes should come with a release note in clang/docs/ReleaseNotes.rst.
I'm confused -- the patch says that it's fixing a crash, but none of those test cases crash today. Can you add some new test coverage that demonstrates the crash? Also, the changes are to code that's only run in asserts builds... so why are we getting more diagnostics after these changes?
| // Should be OK: lambda's call operator is a friend. | ||
| void use(X &x) { y(x); } | ||
| void use(X &x) { y(x); } // cxx11-error {{no matching function for call to object}} |
There was a problem hiding this comment.
The comment above no longer matches the behavior, the comment should be updated slightly.
| // FIXME: This should not match, as above. | ||
| template<typename T> | ||
| friend U::operator ExpectedTypeU<T>() const; | ||
| friend U::operator ExpectedTypeU<T>() const; // cxx11-error {{friend declaration of 'operator void (*)(type-parameter-0-0 &)' does not match any declaration}} |
There was a problem hiding this comment.
Oof, that's a bit of a gross diagnostic (not the fault of your patch).
There was a problem hiding this comment.
Oooh, I see now, there's a new RUN line added to the test and that's where the new diagnostics are coming from.
The tests are unrelated to the patch and should be split out into a separate change, and you should add test coverage where we were crashing and we no longer do so. That may need to go into its own file which // REQUIRES: asserts.
|
@AaronBallman Thanks for the review. Sorry for responding so late - I've missed your review. The tests aren't actually unrelated to the path. This is the file where I discovered the crash, so I just used it to demonstrate that it's fixed. I'm not sure how to split things, since I can't add the run line without fixing the bug. |
Ah, I see now what's going on, and yeah, the tests are not unrelated to the patch (the added RUN line is what causes the crash today), sorry for missing that. |
|
Looks like the included test still makes clang crash: http://45.33.8.238/linux/132722/step_7.txt |
|
Also on the official waterfall, eg here: https://lab.llvm.org/buildbot/#/builders/188/builds/42935 Can you take a look, and revert for now if it takes a while to fix? |
This reverts commit dc567a2.
This re-applies llvm#74110 with the crashing code disabled in C++03. I'll try to fix the new crash in it's own patch.
) This re-applies #74110 with the crashing code disabled in C++11. I'll try to fix the new crash in it's own patch.
In valid code, there should only be a very specific set of members in a lambda definition. If the user tries to define something inside the lambda class, this assumption is violated and causes an assertion error. This can be fixed by checking whether the members are valid, and if not, ignore that the class members are potentially unexpected.
I've come across this while working on implementing lambdas in C++03.