[Clang] [Sema] Fix a crash when a friend function is redefined as deleted
#135679
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sirraide
added
the
clang:frontend
|
@llvm/pr-subscribers-clang Author: None (Sirraide) ChangesNB: This only fixes the crash introduced in Clang 19; we still accept this code even though we shouldn’t: struct S {
friend int f() { return 3; }
friend int f() = delete;
};I tried figuring out a way to diagnose this redeclaration, but it seems tricky because I kept running into issues around defaulted comparison operators. From my testing, however, this fix here would still be required even once we do start diagnosing this. Fixes #135506. Full diff: https://github.com/llvm/llvm-project/pull/135679.diff 3 Files Affected:
diff --git a/clang/docs/ReleaseNotes.rst b/clang/docs/ReleaseNotes.rst
index 11f62bc881b03..037f9f6fe79e0 100644
--- a/clang/docs/ReleaseNotes.rst
+++ b/clang/docs/ReleaseNotes.rst
@@ -389,7 +389,7 @@ Bug Fixes in This Version
#if 1 ? 1 : 999999999999999999999
#endif
-
+- Fixed a crash when a ``friend`` function is redefined as deleted. (#GH135506)
Bug Fixes to Compiler Builtins
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
diff --git a/clang/lib/Sema/SemaDecl.cpp b/clang/lib/Sema/SemaDecl.cpp
index e9805c345b6af..8fd857d347895 100644
--- a/clang/lib/Sema/SemaDecl.cpp
+++ b/clang/lib/Sema/SemaDecl.cpp
@@ -16178,16 +16178,11 @@ Decl *Sema::ActOnFinishFunctionBody(Decl *dcl, Stmt *Body,
// This is meant to pop the context added in ActOnStartOfFunctionDef().
ExitFunctionBodyRAII ExitRAII(*this, isLambdaCallOperator(FD));
if (FD) {
- // If this is called by Parser::ParseFunctionDefinition() after marking
- // the declaration as deleted, and if the deleted-function-body contains
- // a message (C++26), then a DefaultedOrDeletedInfo will have already been
- // added to store that message; do not overwrite it in that case.
- //
- // Since this would always set the body to 'nullptr' in that case anyway,
- // which is already done when the function decl is initially created,
- // always skipping this irrespective of whether there is a delete message
- // should not be a problem.
- if (!FD->isDeletedAsWritten())
+ // The function body and the DefaultedOrDeletedInfo, if present, use
+ // the same storage; don't overwrite the latter if the former is null
+ // (the body is initialised to null anyway, so even if the latter isn't
+ // present, this would still be a no-op).
+ if (Body)
FD->setBody(Body);
FD->setWillHaveBody(false);
diff --git a/clang/test/SemaCXX/cxx2c-delete-with-message.cpp b/clang/test/SemaCXX/cxx2c-delete-with-message.cpp
index 22e65d902ecd4..5609da18c05aa 100644
--- a/clang/test/SemaCXX/cxx2c-delete-with-message.cpp
+++ b/clang/test/SemaCXX/cxx2c-delete-with-message.cpp
@@ -271,3 +271,33 @@ void operators() {
if (to_int_int) {} // expected-error {{attempt to use a deleted function: deleted (TO<int, int>, operator bool)}}
static_cast<bool>(to_int_int); // expected-error {{static_cast from 'TO<int, int>' to 'bool' uses deleted function: deleted (TO<int, int>, operator bool)}}
};
+
+namespace gh135506 {
+struct a {
+ // FIXME: We currently don't diagnose these invalid redeclarations if the
+ // second declaration is defaulted or deleted. This probably needs to be
+ // handled in ParseCXXInlineMethodDef() after parsing the defaulted/deleted
+ // body.
+ friend consteval int f() { return 3; }
+ friend consteval int f() = delete("foo");
+
+ friend consteval int g() { return 3; }
+ friend consteval int g() = delete;
+
+ friend int h() { return 3; }
+ friend int h() = delete;
+
+ friend consteval int i() = delete; // expected-note {{previous definition is here}}
+ friend consteval int i() { return 3; } // expected-error {{redefinition of 'i'}}
+};
+
+struct b {
+ friend consteval bool operator==(b, b) { return true; } // expected-note {{previous declaration is here}}
+ friend consteval bool operator==(b, b) = default; // expected-error {{defaulting this equality comparison operator is not allowed because it was already declared outside the class}}
+};
+
+struct c {
+ friend consteval bool operator==(c, c) = default; // expected-note {{previous definition is here}}
+ friend consteval bool operator==(c, c) { return true; } // expected-error {{redefinition of 'operator=='}}
+};
+}
|
Fznamznon
approved these changes
Apr 15, 2025
|
|
||
| struct b { | ||
| friend consteval bool operator==(b, b) { return true; } // expected-note {{previous declaration is here}} | ||
| friend consteval bool operator==(b, b) = default; // expected-error {{defaulting this equality comparison operator is not allowed because it was already declared outside the class}} |
There was a problem hiding this comment.
This diagnostic is a bit off, the previous declaration is not outside the class.
There was a problem hiding this comment.
Oh, that’s not ideal either; I’ll make a separate issue for that (this pr doesn’t change anything about how that is handled; I just put it there to check that we don’t crash on it).
There was a problem hiding this comment.
Actually... the wording in the standard around this is a bit unclear (obviously this is ill-formed but I’m talking about the case where the first declaration is not a definition). I’ll investigate this some more.
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/190/builds/18406 Here is the relevant piece of the build log for the reference |
var-const
pushed a commit
to ldionne/llvm-project
that referenced
this pull request
Apr 17, 2025
…eleted (llvm#135679) NB: This only fixes the crash introduced in Clang 19; we still accept this code even though we shouldn’t: ```c++ struct S { friend int f() { return 3; } friend int f() = delete; }; ``` I tried figuring out a way to diagnose this redeclaration, but it seems tricky because I kept running into issues around defaulted comparison operators. From my testing, however, this fix here would still be required even once we do start diagnosing this. Fixes llvm#135506.
shafik
reviewed
Apr 17, 2025
| friend consteval int f() { return 3; } | ||
| friend consteval int f() = delete("foo"); | ||
|
|
||
| friend consteval int g() { return 3; } |
There was a problem hiding this comment.
We should also test constexpr as well.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
NB: This only fixes the crash introduced in Clang 19; we still accept this code even though we shouldn’t:
I tried figuring out a way to diagnose this redeclaration, but it seems tricky because I kept running into issues around defaulted comparison operators. From my testing, however, this fix here would still be required even once we do start diagnosing this.
Fixes #135506.