[clang] [Clang][Sema] Do not attempt to instantiate a deleted move constructor (PR #80959)
@@ -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;
Sirraide wrote:
> 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.
https://github.com/llvm/llvm-project/pull/80959
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Clang][Sema] Do not attempt to instantiate a deleted move constructor (PR #80959)
@@ -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;
erichkeane wrote:
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.
https://github.com/llvm/llvm-project/pull/80959
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Clang][Sema] Do not attempt to instantiate a deleted move constructor (PR #80959)
AaronBallman wrote: > > which also begs the question as to what change actually caused this to break > > 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 `git blame` on the surrounding code to see how the code evolved and what comments there were on the reviews. https://github.com/llvm/llvm-project/pull/80959 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Clang][Sema] Do not attempt to instantiate a deleted move constructor (PR #80959)
Sirraide wrote: > which also begs the question as to what change actually caused this to break 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... https://github.com/llvm/llvm-project/pull/80959 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Clang][Sema] Do not attempt to instantiate a deleted move constructor (PR #80959)
Sirraide wrote: CC @shafik, @erichkeane, @AaronBallman https://github.com/llvm/llvm-project/pull/80959 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Clang][Sema] Do not attempt to instantiate a deleted move constructor (PR #80959)
llvmbot wrote:
@llvm/pr-subscribers-clang
Author: None (Sirraide)
Changes
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
1. This problem did not exist in earlier versions of Clang (e.g. the release
branch of Clang 17 diagnoses this correctly with an overload resolution error).
2. There are some other code paths that seem very similar to this one that that
should probably also be investigated (specifically, other special member
functions and potentially also defaulted comparison operators; I’ll take a look
at adding some tests for those too if need be, but I wanted to document the
current state of this here before that).
3. There may be a better place to put this check seeing as the functions that
are changed by this do not seem to have been modified compared to the release
branch of Clang 17, which also begs the question as to what change actually
caused this to break.
This fixes #80869.
---
Full diff: https://github.com/llvm/llvm-project/pull/80959.diff
3 Files Affected:
- (modified) clang/include/clang/Sema/Sema.h (+2-1)
- (modified) clang/lib/Sema/SemaDeclCXX.cpp (+12-6)
- (modified) clang/test/SemaCXX/cxx0x-defaulted-functions.cpp (+20)
``diff
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(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
+++
[clang] [Clang][Sema] Do not attempt to instantiate a deleted move constructor (PR #80959)
https://github.com/Sirraide created
https://github.com/llvm/llvm-project/pull/80959
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
1. This problem did not exist in earlier versions of Clang (e.g. the release
branch of Clang 17 diagnoses this correctly with an overload resolution error).
2. There are some other code paths that seem very similar to this one that that
should probably also be investigated (specifically, other special member
functions and potentially also defaulted comparison operators; I’ll take a look
at adding some tests for those too if need be, but I wanted to document the
current state of this here before that).
3. There may be a better place to put this check seeing as the functions that
are changed by this do not seem to have been modified compared to the release
branch of Clang 17, which also begs the question as to what change actually
caused this to break.
This fixes #80869.
>From 9224a09244d48b630358ef8659c4c28436d6d28a Mon Sep 17 00:00:00 2001
From: Sirraide
Date: Wed, 7 Feb 2024 10:07:32 +0100
Subject: [PATCH] [Clang][Sema] Do not attempt to instantiate a deleted move
constructor
---
clang/include/clang/Sema/Sema.h | 3 ++-
clang/lib/Sema/SemaDeclCXX.cpp| 18 +++--
.../SemaCXX/cxx0x-defaulted-functions.cpp | 20 +++
3 files changed, 34 insertions(+), 7 deletions(-)
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(FD);
-if (CheckExplicitlyDefaultedSpecialMember(MD, DefKind.asSpecialMember(),
- DefaultLoc))
+if (CheckExplicitlyDefaultedSpecialMember(
+MD, DefKind.asSpecialMember(), DefaultLoc, /*ForDefinition=*/true))
MD->setInvalidDecl();
else
DefineDefaultedFunction(*this, MD, DefaultLoc);
diff --git
