-
Notifications
You must be signed in to change notification settings - Fork 10.7k
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] Do not attempt to instantiate a deleted move constructor #80959
base: main
Are you sure you want to change the base?
Conversation
@llvm/pr-subscribers-clang Author: None (Sirraide) ChangesSema would incorrectly skip diagnosing a malformed use of This fixes that by always erroring in that case. However, there are some things to note here
This fixes #80869. Full diff: https://github.com/llvm/llvm-project/pull/80959.diff 3 Files Affected:
diff --git a/clang/include/clang/Sema/Sema.h b/clang/include/clang/Sema/Sema.h
index 3c26003b5bda7f..7e36b3f1771a71 100644
--- a/clang/include/clang/Sema/Sema.h
+++ b/clang/include/clang/Sema/Sema.h
@@ -7999,7 +7999,8 @@ class Sema final {
bool CheckExplicitlyDefaultedSpecialMember(CXXMethodDecl *MD,
CXXSpecialMember CSM,
- SourceLocation DefaultLoc);
+ SourceLocation DefaultLoc,
+ bool ForDefinition = false);
void CheckDelayedMemberExceptionSpecs();
bool CheckExplicitlyDefaultedComparison(Scope *S, FunctionDecl *MD,
diff --git a/clang/lib/Sema/SemaDeclCXX.cpp b/clang/lib/Sema/SemaDeclCXX.cpp
index ab8a967b06a456..8f355e1e86519d 100644
--- a/clang/lib/Sema/SemaDeclCXX.cpp
+++ b/clang/lib/Sema/SemaDeclCXX.cpp
@@ -7676,7 +7676,8 @@ void Sema::CheckExplicitlyDefaultedFunction(Scope *S, FunctionDecl *FD) {
bool Sema::CheckExplicitlyDefaultedSpecialMember(CXXMethodDecl *MD,
CXXSpecialMember CSM,
- SourceLocation DefaultLoc) {
+ SourceLocation DefaultLoc,
+ bool ForDefinition) {
CXXRecordDecl *RD = MD->getParent();
assert(MD->isExplicitlyDefaulted() && CSM != CXXInvalid &&
@@ -7894,13 +7895,18 @@ bool Sema::CheckExplicitlyDefaultedSpecialMember(CXXMethodDecl *MD,
if (ShouldDeleteForTypeMismatch || ShouldDeleteSpecialMember(MD, CSM)) {
if (First) {
SetDeclDeleted(MD, MD->getLocation());
- if (!inTemplateInstantiation() && !HadError) {
- Diag(MD->getLocation(), diag::warn_defaulted_method_deleted) << CSM;
+ if ((ForDefinition || !inTemplateInstantiation()) && !HadError) {
+ // Always error if we're about to generate a definition.
+ HadError = ForDefinition;
+ Diag(MD->getLocation(), ForDefinition
+ ? diag::err_out_of_line_default_deletes
+ : diag::warn_defaulted_method_deleted)
+ << CSM;
if (ShouldDeleteForTypeMismatch) {
Diag(MD->getLocation(), diag::note_deleted_type_mismatch) << CSM;
} else if (ShouldDeleteSpecialMember(MD, CSM, nullptr,
/*Diagnose*/ true) &&
- DefaultLoc.isValid()) {
+ DefaultLoc.isValid() && !ForDefinition) {
Diag(DefaultLoc, diag::note_replace_equals_default_to_delete)
<< FixItHint::CreateReplacement(DefaultLoc, "delete");
}
@@ -18285,8 +18291,8 @@ void Sema::SetDeclDefaulted(Decl *Dcl, SourceLocation DefaultLoc) {
} else {
auto *MD = cast<CXXMethodDecl>(FD);
- if (CheckExplicitlyDefaultedSpecialMember(MD, DefKind.asSpecialMember(),
- DefaultLoc))
+ if (CheckExplicitlyDefaultedSpecialMember(
+ MD, DefKind.asSpecialMember(), DefaultLoc, /*ForDefinition=*/true))
MD->setInvalidDecl();
else
DefineDefaultedFunction(*this, MD, DefaultLoc);
diff --git a/clang/test/SemaCXX/cxx0x-defaulted-functions.cpp b/clang/test/SemaCXX/cxx0x-defaulted-functions.cpp
index 0c3dd1ea7aa274..112bb4488de063 100644
--- a/clang/test/SemaCXX/cxx0x-defaulted-functions.cpp
+++ b/clang/test/SemaCXX/cxx0x-defaulted-functions.cpp
@@ -286,3 +286,23 @@ struct B {
auto operator = (RM<B>) -> RV<B> = delete;
};
}
+
+namespace GH80869 {
+ struct F {F(F&&)=delete;}; // expected-note {{has been explicitly marked deleted here}}
+
+ template<int=0>
+ struct M {
+ F f; // expected-note {{field 'f' has a deleted move constructor}}
+ M();
+ M(const M&);
+ M(M&&);
+ };
+
+ template<int I>
+ M<I>::M(M&&)=default; // expected-error {{would delete it after its first declaration}}
+
+ M<> f() {
+ M<> m;
+ return m; // expected-note {{in instantiation of}}
+ }
+}
|
Since I’m not sure how that is usually done here, is bisecting a reasonable approach? I’m asking because rebuilding a project the size of Clang unfortunately ends up taking a rather substantial amount of time... |
Bisecting is one useful approach (with the drawback of how resource-intensive it is), but you can also sometimes do well with |
Diag(MD->getLocation(), diag::warn_defaulted_method_deleted) << CSM; | ||
if ((ForDefinition || !inTemplateInstantiation()) && !HadError) { | ||
// Always error if we're about to generate a definition. | ||
HadError = ForDefinition; |
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.
This does not seem right here... I would expect us to still want to instantiate the function, just skip doing that earlier. I think the bisect as suggested by others to see if we can figure out what the difference is for the cause is a better way forward than this.
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.
I think the bisect as suggested by others to see if we can figure out what the difference is for the cause is a better way forward than this
That makes sense. I’ll take another look at this.
Sema would incorrectly skip diagnosing a malformed use of
= default
on an implcitly deleted move constructor while performing template instantiation, even if we were about to then generate a definition of said move constructor, which caused a crash.This fixes that by always erroring in that case. However, there are some things to note here
This fixes #80869.