[clang] [Clang] [Sema] Handle placeholders in '.*' expressions (PR #83103)
Sirraide wrote: Hmm, I’m not sure I’m doing this properly; I’ll try and see if I can open a pr manually. https://github.com/llvm/llvm-project/pull/83103 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Clang] [Sema] Handle placeholders in '.*' expressions (PR #83103)
llvmbot wrote: Failed to cherry-pick: https://github.com/Sirraide/llvm-project/commit/74fa05dead4d52eef3c33406d05dd1bbaf10d546 https://github.com/llvm/llvm-project/actions/runs/8147057328 Please manually backport the fix and push it to your github fork. Once this is done, please create a [pull request](https://github.com/llvm/llvm-project/compare) https://github.com/llvm/llvm-project/pull/83103 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Clang] [Sema] Handle placeholders in '.*' expressions (PR #83103)
Sirraide wrote: /cherry-pick https://github.com/Sirraide/llvm-project/commit/74fa05dead4d52eef3c33406d05dd1bbaf10d546 https://github.com/llvm/llvm-project/pull/83103 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Clang] [Sema] Handle placeholders in '.*' expressions (PR #83103)
llvmbot wrote: Failed to cherry-pick: d23ef9ef3685eb42ebf719bc28cfe2e4651932fc https://github.com/llvm/llvm-project/actions/runs/8146857249 Please manually backport the fix and push it to your github fork. Once this is done, please create a [pull request](https://github.com/llvm/llvm-project/compare) https://github.com/llvm/llvm-project/pull/83103 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Clang] [Sema] Handle placeholders in '.*' expressions (PR #83103)
Sirraide wrote: /cherry-pick https://github.com/llvm/llvm-project/commit/d23ef9ef3685eb42ebf719bc28cfe2e4651932fc https://github.com/llvm/llvm-project/pull/83103 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Clang] [Sema] Handle placeholders in '.*' expressions (PR #83103)
https://github.com/Sirraide milestoned https://github.com/llvm/llvm-project/pull/83103 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Clang] [Sema] Handle placeholders in '.*' expressions (PR #83103)
AaronBallman wrote: > @AaronBallman @Sirraide would this patch be reasonable to backport to the > 18.x release branch? It fixes a problem with our application on the > FreeBSD-CURRENT branch. > [SerenityOS/serenity#23365](https://github.com/SerenityOS/serenity/issues/23365) Yeah, I think the changes are sufficiently safe and small enough that we could consider backporting. We have instructions on how to get that going here: https://www.llvm.org/docs/GitHub.html#backporting-fixes-to-the-release-branches https://github.com/llvm/llvm-project/pull/83103 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Clang] [Sema] Handle placeholders in '.*' expressions (PR #83103)
ADKaster wrote: @AaronBallman @Sirraide would this patch be reasonable to backport to the 18.x release branch? It fixes a problem with our application on the FreeBSD-CURRENT branch. https://github.com/SerenityOS/serenity/issues/23365 https://github.com/llvm/llvm-project/pull/83103 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Clang] [Sema] Handle placeholders in '.*' expressions (PR #83103)
https://github.com/AaronBallman closed https://github.com/llvm/llvm-project/pull/83103 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Clang] [Sema] Handle placeholders in '.*' expressions (PR #83103)
https://github.com/AaronBallman updated
https://github.com/llvm/llvm-project/pull/83103
>From 071b6287e89e4601d9e441978f0b4bd53e757c26 Mon Sep 17 00:00:00 2001
From: Sirraide
Date: Tue, 27 Feb 2024 06:19:05 +0100
Subject: [PATCH 1/3] [Clang] [Sema] Handle placeholders in '.*' expressions
---
clang/lib/Sema/SemaOverload.cpp | 22 +-
clang/test/SemaCXX/gh53815.cpp | 21 +
2 files changed, 38 insertions(+), 5 deletions(-)
create mode 100644 clang/test/SemaCXX/gh53815.cpp
diff --git a/clang/lib/Sema/SemaOverload.cpp b/clang/lib/Sema/SemaOverload.cpp
index c46f6338a5a125..f4b67e7b469418 100644
--- a/clang/lib/Sema/SemaOverload.cpp
+++ b/clang/lib/Sema/SemaOverload.cpp
@@ -14474,6 +14474,23 @@ ExprResult Sema::CreateOverloadedBinOp(SourceLocation
OpLoc,
CurFPFeatureOverrides());
}
+ // If this is the .* operator, which is not overloadable, just
+ // create a built-in binary operator.
+ if (Opc == BO_PtrMemD) {
+auto CheckPlaceholder = [&](Expr *) {
+ ExprResult Res = CheckPlaceholderExpr(Arg);
+ if (!Res.isInvalid())
+Arg = Res.get();
+ return Res.isInvalid();
+};
+
+// CreateBuiltinBinOp() doesn't like it if we tell it to create a '.*'
+// expression that contains placeholders (in either the LHS or RHS).
+if (CheckPlaceholder(Args[0]) || CheckPlaceholder(Args[1]))
+ return ExprError();
+return CreateBuiltinBinOp(OpLoc, Opc, Args[0], Args[1]);
+ }
+
// Always do placeholder-like conversions on the RHS.
if (checkPlaceholderForOverload(*this, Args[1]))
return ExprError();
@@ -14493,11 +14510,6 @@ ExprResult Sema::CreateOverloadedBinOp(SourceLocation
OpLoc,
if (Opc == BO_Assign && !Args[0]->getType()->isOverloadableType())
return CreateBuiltinBinOp(OpLoc, Opc, Args[0], Args[1]);
- // If this is the .* operator, which is not overloadable, just
- // create a built-in binary operator.
- if (Opc == BO_PtrMemD)
-return CreateBuiltinBinOp(OpLoc, Opc, Args[0], Args[1]);
-
// Build the overload set.
OverloadCandidateSet CandidateSet(OpLoc, OverloadCandidateSet::CSK_Operator,
OverloadCandidateSet::OperatorRewriteInfo(
diff --git a/clang/test/SemaCXX/gh53815.cpp b/clang/test/SemaCXX/gh53815.cpp
new file mode 100644
index 00..326c911c7bfaf5
--- /dev/null
+++ b/clang/test/SemaCXX/gh53815.cpp
@@ -0,0 +1,21 @@
+// RUN: %clang_cc1 -fsyntax-only -verify -std=c++20 %s
+// expected-no-diagnostics
+
+// Check that we don't crash due to forgetting to check for placeholders
+// in the RHS of '.*'.
+
+template
+static bool has_explicitly_named_overload() {
+ return requires { Fn().*::operator(); };
+}
+
+int main() {
+ has_explicitly_named_overload();
+}
+
+template
+constexpr bool has_explicitly_named_overload_2() {
+ return requires { Fn().*::operator(); };
+}
+
+static_assert(!has_explicitly_named_overload_2());
>From b39abdb1a5d083a59ce818748c0d130501467706 Mon Sep 17 00:00:00 2001
From: Sirraide
Date: Tue, 27 Feb 2024 18:35:22 +0100
Subject: [PATCH 2/3] [Clang] Update release notes
---
clang/docs/ReleaseNotes.rst | 2 ++
1 file changed, 2 insertions(+)
diff --git a/clang/docs/ReleaseNotes.rst b/clang/docs/ReleaseNotes.rst
index c60c5682dbd826..7e16b9f0c67dbd 100644
--- a/clang/docs/ReleaseNotes.rst
+++ b/clang/docs/ReleaseNotes.rst
@@ -288,6 +288,8 @@ Bug Fixes to C++ Support
templates when determining the primary template of an explicit
specialization.
- Fixed a crash in Microsoft compatibility mode where unqualified dependent
base class
lookup searches the bases of an incomplete class.
+- Fix a crash when an unresolved overload set is encountered on the RHS of a
``.*`` operator.
+ (`#53815 `_)
Bug Fixes to AST Handling
^
>From c52a0e91b9949c8ee4a13df11309b230d37ca933 Mon Sep 17 00:00:00 2001
From: Sirraide
Date: Tue, 27 Feb 2024 18:56:23 +0100
Subject: [PATCH 3/3] [NFC] Use isUsable() instead of isInvalid()
---
clang/lib/Sema/SemaOverload.cpp | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/clang/lib/Sema/SemaOverload.cpp b/clang/lib/Sema/SemaOverload.cpp
index 1b1252ba90e560..7d38043890ca20 100644
--- a/clang/lib/Sema/SemaOverload.cpp
+++ b/clang/lib/Sema/SemaOverload.cpp
@@ -14576,9 +14576,9 @@ ExprResult Sema::CreateOverloadedBinOp(SourceLocation
OpLoc,
if (Opc == BO_PtrMemD) {
auto CheckPlaceholder = [&](Expr *) {
ExprResult Res = CheckPlaceholderExpr(Arg);
- if (!Res.isInvalid())
+ if (Res.isUsable())
Arg = Res.get();
- return Res.isInvalid();
+ return !Res.isUsable();
};
// CreateBuiltinBinOp() doesn't like it if we tell it to create a '.*'
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Clang] [Sema] Handle placeholders in '.*' expressions (PR #83103)
AaronBallman wrote: > > LGTM! > > Thanks. Just so you know, I still don’t have commit access, so you’d have to > merge it for me—I should really look into obtaining commit access at this > point perhaps Thank you for mentioning that! It turns out that GitHub is confusing:  I read that to mean you had commit privs. :-D I support you getting commit privs, if you'd like to follow the process outlined here: https://llvm.org/docs/DeveloperPolicy.html#obtaining-commit-access but in the meantime, I'll push the button. https://github.com/llvm/llvm-project/pull/83103 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Clang] [Sema] Handle placeholders in '.*' expressions (PR #83103)
Sirraide wrote: > LGTM! Thanks. Just so you know, I still don’t have commit access, so you’d have to merge it for me—I should really look into obtaining commit access at this point perhaps https://github.com/llvm/llvm-project/pull/83103 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Clang] [Sema] Handle placeholders in '.*' expressions (PR #83103)
https://github.com/AaronBallman approved this pull request. LGTM! https://github.com/llvm/llvm-project/pull/83103 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Clang] [Sema] Handle placeholders in '.*' expressions (PR #83103)
https://github.com/Sirraide updated
https://github.com/llvm/llvm-project/pull/83103
>From 071b6287e89e4601d9e441978f0b4bd53e757c26 Mon Sep 17 00:00:00 2001
From: Sirraide
Date: Tue, 27 Feb 2024 06:19:05 +0100
Subject: [PATCH 1/3] [Clang] [Sema] Handle placeholders in '.*' expressions
---
clang/lib/Sema/SemaOverload.cpp | 22 +-
clang/test/SemaCXX/gh53815.cpp | 21 +
2 files changed, 38 insertions(+), 5 deletions(-)
create mode 100644 clang/test/SemaCXX/gh53815.cpp
diff --git a/clang/lib/Sema/SemaOverload.cpp b/clang/lib/Sema/SemaOverload.cpp
index c46f6338a5a125..f4b67e7b469418 100644
--- a/clang/lib/Sema/SemaOverload.cpp
+++ b/clang/lib/Sema/SemaOverload.cpp
@@ -14474,6 +14474,23 @@ ExprResult Sema::CreateOverloadedBinOp(SourceLocation
OpLoc,
CurFPFeatureOverrides());
}
+ // If this is the .* operator, which is not overloadable, just
+ // create a built-in binary operator.
+ if (Opc == BO_PtrMemD) {
+auto CheckPlaceholder = [&](Expr *) {
+ ExprResult Res = CheckPlaceholderExpr(Arg);
+ if (!Res.isInvalid())
+Arg = Res.get();
+ return Res.isInvalid();
+};
+
+// CreateBuiltinBinOp() doesn't like it if we tell it to create a '.*'
+// expression that contains placeholders (in either the LHS or RHS).
+if (CheckPlaceholder(Args[0]) || CheckPlaceholder(Args[1]))
+ return ExprError();
+return CreateBuiltinBinOp(OpLoc, Opc, Args[0], Args[1]);
+ }
+
// Always do placeholder-like conversions on the RHS.
if (checkPlaceholderForOverload(*this, Args[1]))
return ExprError();
@@ -14493,11 +14510,6 @@ ExprResult Sema::CreateOverloadedBinOp(SourceLocation
OpLoc,
if (Opc == BO_Assign && !Args[0]->getType()->isOverloadableType())
return CreateBuiltinBinOp(OpLoc, Opc, Args[0], Args[1]);
- // If this is the .* operator, which is not overloadable, just
- // create a built-in binary operator.
- if (Opc == BO_PtrMemD)
-return CreateBuiltinBinOp(OpLoc, Opc, Args[0], Args[1]);
-
// Build the overload set.
OverloadCandidateSet CandidateSet(OpLoc, OverloadCandidateSet::CSK_Operator,
OverloadCandidateSet::OperatorRewriteInfo(
diff --git a/clang/test/SemaCXX/gh53815.cpp b/clang/test/SemaCXX/gh53815.cpp
new file mode 100644
index 00..326c911c7bfaf5
--- /dev/null
+++ b/clang/test/SemaCXX/gh53815.cpp
@@ -0,0 +1,21 @@
+// RUN: %clang_cc1 -fsyntax-only -verify -std=c++20 %s
+// expected-no-diagnostics
+
+// Check that we don't crash due to forgetting to check for placeholders
+// in the RHS of '.*'.
+
+template
+static bool has_explicitly_named_overload() {
+ return requires { Fn().*::operator(); };
+}
+
+int main() {
+ has_explicitly_named_overload();
+}
+
+template
+constexpr bool has_explicitly_named_overload_2() {
+ return requires { Fn().*::operator(); };
+}
+
+static_assert(!has_explicitly_named_overload_2());
>From b39abdb1a5d083a59ce818748c0d130501467706 Mon Sep 17 00:00:00 2001
From: Sirraide
Date: Tue, 27 Feb 2024 18:35:22 +0100
Subject: [PATCH 2/3] [Clang] Update release notes
---
clang/docs/ReleaseNotes.rst | 2 ++
1 file changed, 2 insertions(+)
diff --git a/clang/docs/ReleaseNotes.rst b/clang/docs/ReleaseNotes.rst
index c60c5682dbd826..7e16b9f0c67dbd 100644
--- a/clang/docs/ReleaseNotes.rst
+++ b/clang/docs/ReleaseNotes.rst
@@ -288,6 +288,8 @@ Bug Fixes to C++ Support
templates when determining the primary template of an explicit
specialization.
- Fixed a crash in Microsoft compatibility mode where unqualified dependent
base class
lookup searches the bases of an incomplete class.
+- Fix a crash when an unresolved overload set is encountered on the RHS of a
``.*`` operator.
+ (`#53815 `_)
Bug Fixes to AST Handling
^
>From c52a0e91b9949c8ee4a13df11309b230d37ca933 Mon Sep 17 00:00:00 2001
From: Sirraide
Date: Tue, 27 Feb 2024 18:56:23 +0100
Subject: [PATCH 3/3] [NFC] Use isUsable() instead of isInvalid()
---
clang/lib/Sema/SemaOverload.cpp | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/clang/lib/Sema/SemaOverload.cpp b/clang/lib/Sema/SemaOverload.cpp
index 1b1252ba90e560..7d38043890ca20 100644
--- a/clang/lib/Sema/SemaOverload.cpp
+++ b/clang/lib/Sema/SemaOverload.cpp
@@ -14576,9 +14576,9 @@ ExprResult Sema::CreateOverloadedBinOp(SourceLocation
OpLoc,
if (Opc == BO_PtrMemD) {
auto CheckPlaceholder = [&](Expr *) {
ExprResult Res = CheckPlaceholderExpr(Arg);
- if (!Res.isInvalid())
+ if (Res.isUsable())
Arg = Res.get();
- return Res.isInvalid();
+ return !Res.isUsable();
};
// CreateBuiltinBinOp() doesn't like it if we tell it to create a '.*'
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Clang] [Sema] Handle placeholders in '.*' expressions (PR #83103)
Sirraide wrote: > Thank you for the fix! Please be sure to add a release note to > `clang/docs/ReleaseNotes.rst` so users know about the fix (and be sure to > mention the issue # in the release note). Ah yes, I keep forgetting about that https://github.com/llvm/llvm-project/pull/83103 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Clang] [Sema] Handle placeholders in '.*' expressions (PR #83103)
@@ -14474,6 +14474,23 @@ ExprResult Sema::CreateOverloadedBinOp(SourceLocation
OpLoc,
CurFPFeatureOverrides());
}
+ // If this is the .* operator, which is not overloadable, just
+ // create a built-in binary operator.
+ if (Opc == BO_PtrMemD) {
+auto CheckPlaceholder = [&](Expr *) {
+ ExprResult Res = CheckPlaceholderExpr(Arg);
+ if (!Res.isInvalid())
+Arg = Res.get();
+ return Res.isInvalid();
Sirraide wrote:
I was wondering what the difference between those two was, so thanks for
pointing that out. We definitely want that one then.
https://github.com/llvm/llvm-project/pull/83103
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Clang] [Sema] Handle placeholders in '.*' expressions (PR #83103)
@@ -14474,6 +14474,23 @@ ExprResult Sema::CreateOverloadedBinOp(SourceLocation
OpLoc,
CurFPFeatureOverrides());
}
+ // If this is the .* operator, which is not overloadable, just
+ // create a built-in binary operator.
+ if (Opc == BO_PtrMemD) {
+auto CheckPlaceholder = [&](Expr *) {
+ ExprResult Res = CheckPlaceholderExpr(Arg);
+ if (!Res.isInvalid())
+Arg = Res.get();
+ return Res.isInvalid();
AaronBallman wrote:
Do we perhaps want to use `!Res.isUsable()` because `CreateBuiltinBinOp()`
expects the `Expr *`s to be nonnull?
https://github.com/llvm/llvm-project/pull/83103
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Clang] [Sema] Handle placeholders in '.*' expressions (PR #83103)
https://github.com/AaronBallman commented: Thank you for the fix! Please be sure to add a release note to `clang/docs/ReleaseNotes.rst` so users know about the fix (and be sure to mention the issue # in the release note). https://github.com/llvm/llvm-project/pull/83103 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Clang] [Sema] Handle placeholders in '.*' expressions (PR #83103)
https://github.com/AaronBallman edited https://github.com/llvm/llvm-project/pull/83103 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Clang] [Sema] Handle placeholders in '.*' expressions (PR #83103)
Sirraide wrote: Lastly, it also seems weird to me that a function called `CreateOverloadedBinOp()` is called to handle `.*`—an operator that can’t be overloaded—but seeing as this function has apparently been handling this case for over a decade now, I’m probably not going to question this any further. https://github.com/llvm/llvm-project/pull/83103 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Clang] [Sema] Handle placeholders in '.*' expressions (PR #83103)
Sirraide wrote: In my opinion, we ought to get `.*` ought of the way early—as I’m currently doing in this pr—as it makes little sense to do overloading-specific placeholder handling on an operator that isn’t even overloadable—we should instead just handle all placeholders immediately. https://github.com/llvm/llvm-project/pull/83103 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Clang] [Sema] Handle placeholders in '.*' expressions (PR #83103)
Sirraide wrote: Yeah, it seems the change that ultimately caused this to break was made in 2011, which moved the handling of placeholders for this code path up into `SemaOverload.cpp`, and from what I can tell, the case of either operand of `.*` potentially being an overload set when `.*` is not overloadable was simply overlooked (i.e. the `assert()` is there because this case *was* being handled before, but this commit removed that): https://github.com/llvm/llvm-project/commit/526ab47a5573422765ac6c55147dfad00f1d703d#diff-3c28567b5e0c77d68f174541a0b77f5a85d093f58b89cd3675ee04a550a44880L7784-L7800 https://github.com/llvm/llvm-project/pull/83103 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Clang] [Sema] Handle placeholders in '.*' expressions (PR #83103)
Sirraide wrote: It seems like the assertion has been in Clang since 2011, and back then, we *were* checking for placeholders in `CreateBuiltinBinOp()`, so at that point it made sense, but this check seems to have been removed since then. https://github.com/llvm/llvm-project/pull/83103 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Clang] [Sema] Handle placeholders in '.*' expressions (PR #83103)
Sirraide wrote: One more thing: this code seems to not crash and issue a diagnostic just fine if we simply remove the assertion, so that would also be an option, but I didn’t simply want to remove an assertion without fully knowing why it’s there, so I’ve gone with this as an alternative for now (it should be noted that the code path here is shared with `->*`, which of course is overloadable). Alternatively, we could also handle placeholders then and there instead of asserting on them. Thinking about this now, I’m not entirely sure what the best solution would be. https://github.com/llvm/llvm-project/pull/83103 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Clang] [Sema] Handle placeholders in '.*' expressions (PR #83103)
Sirraide wrote: CC @AaronBallman, @cor3ntin https://github.com/llvm/llvm-project/pull/83103 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Clang] [Sema] Handle placeholders in '.*' expressions (PR #83103)
llvmbot wrote:
@llvm/pr-subscribers-clang
Author: None (Sirraide)
Changes
When analysing whether we should handle a binary expression as an overloaded
operator call or a builtin operator, we were calling
`checkPlaceholderForOverload()`, which takes care of any placeholders that are
not overload sets—which would usually make sense since those need to be handled
as part of overload resolution.
Unfortunately, we were also doing that for `.*`, which is not overloadable, and
then proceeding to create a builtin operator anyway, which would crash if the
RHS happened to be an unresolved overload set (due hitting an assertion in
`CreateBuiltinBinOp()`—specifically, in one of its callees—in the `.*` case
that makes sure its arguments aren’t placeholders).
This pr instead makes it so we check for *all* placeholders early if the
operator is `.*`.
It’s worth noting that,
1. In the `.*` case, we now additionally also check for *any* placeholders (not
just non-overload-sets) in the LHS; this shouldn’t make a difference,
however—at least I couldn’t think of a way to trigger the assertion with an
overload set as the LHS of `.*`; it is worth noting that the assertion in
question would also complain if the LHS happened to be of placeholder type,
though.
2. There is another case in which we also don’t perform overload
resolution—namely `=` if the LHS is not of class or enumeration type after
handling non-overload-set placeholders—as in the `.*` case, but similarly to
1., I first couldn’t think of a way of getting this case to crash, and
secondly, `CreateBuiltinBinOp()` doesn’t seem to care about placeholders in the
LHS or RHS in the `=` case (from what I can tell, it, or rather one of its
callees, only checks that the LHS is not a pseudo-object type, but those will
have already been handled by the call to `checkPlaceholderForOverload()` by the
time we get to this function), so I don’t think this case suffers from the same
problem.
This fixes #53815.
---
Full diff: https://github.com/llvm/llvm-project/pull/83103.diff
2 Files Affected:
- (modified) clang/lib/Sema/SemaOverload.cpp (+17-5)
- (added) clang/test/SemaCXX/gh53815.cpp (+21)
``diff
diff --git a/clang/lib/Sema/SemaOverload.cpp b/clang/lib/Sema/SemaOverload.cpp
index c46f6338a5a125..f4b67e7b469418 100644
--- a/clang/lib/Sema/SemaOverload.cpp
+++ b/clang/lib/Sema/SemaOverload.cpp
@@ -14474,6 +14474,23 @@ ExprResult Sema::CreateOverloadedBinOp(SourceLocation
OpLoc,
CurFPFeatureOverrides());
}
+ // If this is the .* operator, which is not overloadable, just
+ // create a built-in binary operator.
+ if (Opc == BO_PtrMemD) {
+auto CheckPlaceholder = [&](Expr *) {
+ ExprResult Res = CheckPlaceholderExpr(Arg);
+ if (!Res.isInvalid())
+Arg = Res.get();
+ return Res.isInvalid();
+};
+
+// CreateBuiltinBinOp() doesn't like it if we tell it to create a '.*'
+// expression that contains placeholders (in either the LHS or RHS).
+if (CheckPlaceholder(Args[0]) || CheckPlaceholder(Args[1]))
+ return ExprError();
+return CreateBuiltinBinOp(OpLoc, Opc, Args[0], Args[1]);
+ }
+
// Always do placeholder-like conversions on the RHS.
if (checkPlaceholderForOverload(*this, Args[1]))
return ExprError();
@@ -14493,11 +14510,6 @@ ExprResult Sema::CreateOverloadedBinOp(SourceLocation
OpLoc,
if (Opc == BO_Assign && !Args[0]->getType()->isOverloadableType())
return CreateBuiltinBinOp(OpLoc, Opc, Args[0], Args[1]);
- // If this is the .* operator, which is not overloadable, just
- // create a built-in binary operator.
- if (Opc == BO_PtrMemD)
-return CreateBuiltinBinOp(OpLoc, Opc, Args[0], Args[1]);
-
// Build the overload set.
OverloadCandidateSet CandidateSet(OpLoc, OverloadCandidateSet::CSK_Operator,
OverloadCandidateSet::OperatorRewriteInfo(
diff --git a/clang/test/SemaCXX/gh53815.cpp b/clang/test/SemaCXX/gh53815.cpp
new file mode 100644
index 00..326c911c7bfaf5
--- /dev/null
+++ b/clang/test/SemaCXX/gh53815.cpp
@@ -0,0 +1,21 @@
+// RUN: %clang_cc1 -fsyntax-only -verify -std=c++20 %s
+// expected-no-diagnostics
+
+// Check that we don't crash due to forgetting to check for placeholders
+// in the RHS of '.*'.
+
+template
+static bool has_explicitly_named_overload() {
+ return requires { Fn().*::operator(); };
+}
+
+int main() {
+ has_explicitly_named_overload();
+}
+
+template
+constexpr bool has_explicitly_named_overload_2() {
+ return requires { Fn().*::operator(); };
+}
+
+static_assert(!has_explicitly_named_overload_2());
``
https://github.com/llvm/llvm-project/pull/83103
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Clang] [Sema] Handle placeholders in '.*' expressions (PR #83103)
https://github.com/Sirraide created
https://github.com/llvm/llvm-project/pull/83103
When analysing whether we should handle a binary expression as an overloaded
operator call or a builtin operator, we were calling
`checkPlaceholderForOverload()`, which takes care of any placeholders that are
not overload sets—which would usually make sense since those need to be handled
as part of overload resolution.
Unfortunately, we were also doing that for `.*`, which is not overloadable, and
then proceeding to create a builtin operator anyway, which would crash if the
RHS happened to be an unresolved overload set (due hitting an assertion in
`CreateBuiltinBinOp()`—specifically, in one of its callees—in the `.*` case
that makes sure its arguments aren’t placeholders).
This pr instead makes it so we check for *all* placeholders early if the
operator is `.*`.
It’s worth noting that,
1. In the `.*` case, we now additionally also check for *any* placeholders (not
just non-overload-sets) in the LHS; this shouldn’t make a difference,
however—at least I couldn’t think of a way to trigger the assertion with an
overload set as the LHS of `.*`; it is worth noting that the assertion in
question would also complain if the LHS happened to be of placeholder type,
though.
2. There is another case in which we also don’t perform overload
resolution—namely `=` if the LHS is not of class or enumeration type after
handling non-overload-set placeholders—as in the `.*` case, but similarly to
1., I first couldn’t think of a way of getting this case to crash, and
secondly, `CreateBuiltinBinOp()` doesn’t seem to care about placeholders in the
LHS or RHS in the `=` case (from what I can tell, it, or rather one of its
callees, only checks that the LHS is not a pseudo-object type, but those will
have already been handled by the call to `checkPlaceholderForOverload()` by the
time we get to this function), so I don’t think this case suffers from the same
problem.
This fixes #53815.
>From 071b6287e89e4601d9e441978f0b4bd53e757c26 Mon Sep 17 00:00:00 2001
From: Sirraide
Date: Tue, 27 Feb 2024 06:19:05 +0100
Subject: [PATCH] [Clang] [Sema] Handle placeholders in '.*' expressions
---
clang/lib/Sema/SemaOverload.cpp | 22 +-
clang/test/SemaCXX/gh53815.cpp | 21 +
2 files changed, 38 insertions(+), 5 deletions(-)
create mode 100644 clang/test/SemaCXX/gh53815.cpp
diff --git a/clang/lib/Sema/SemaOverload.cpp b/clang/lib/Sema/SemaOverload.cpp
index c46f6338a5a125..f4b67e7b469418 100644
--- a/clang/lib/Sema/SemaOverload.cpp
+++ b/clang/lib/Sema/SemaOverload.cpp
@@ -14474,6 +14474,23 @@ ExprResult Sema::CreateOverloadedBinOp(SourceLocation
OpLoc,
CurFPFeatureOverrides());
}
+ // If this is the .* operator, which is not overloadable, just
+ // create a built-in binary operator.
+ if (Opc == BO_PtrMemD) {
+auto CheckPlaceholder = [&](Expr *) {
+ ExprResult Res = CheckPlaceholderExpr(Arg);
+ if (!Res.isInvalid())
+Arg = Res.get();
+ return Res.isInvalid();
+};
+
+// CreateBuiltinBinOp() doesn't like it if we tell it to create a '.*'
+// expression that contains placeholders (in either the LHS or RHS).
+if (CheckPlaceholder(Args[0]) || CheckPlaceholder(Args[1]))
+ return ExprError();
+return CreateBuiltinBinOp(OpLoc, Opc, Args[0], Args[1]);
+ }
+
// Always do placeholder-like conversions on the RHS.
if (checkPlaceholderForOverload(*this, Args[1]))
return ExprError();
@@ -14493,11 +14510,6 @@ ExprResult Sema::CreateOverloadedBinOp(SourceLocation
OpLoc,
if (Opc == BO_Assign && !Args[0]->getType()->isOverloadableType())
return CreateBuiltinBinOp(OpLoc, Opc, Args[0], Args[1]);
- // If this is the .* operator, which is not overloadable, just
- // create a built-in binary operator.
- if (Opc == BO_PtrMemD)
-return CreateBuiltinBinOp(OpLoc, Opc, Args[0], Args[1]);
-
// Build the overload set.
OverloadCandidateSet CandidateSet(OpLoc, OverloadCandidateSet::CSK_Operator,
OverloadCandidateSet::OperatorRewriteInfo(
diff --git a/clang/test/SemaCXX/gh53815.cpp b/clang/test/SemaCXX/gh53815.cpp
new file mode 100644
index 00..326c911c7bfaf5
--- /dev/null
+++ b/clang/test/SemaCXX/gh53815.cpp
@@ -0,0 +1,21 @@
+// RUN: %clang_cc1 -fsyntax-only -verify -std=c++20 %s
+// expected-no-diagnostics
+
+// Check that we don't crash due to forgetting to check for placeholders
+// in the RHS of '.*'.
+
+template
+static bool has_explicitly_named_overload() {
+ return requires { Fn().*::operator(); };
+}
+
+int main() {
+ has_explicitly_named_overload();
+}
+
+template
+constexpr bool has_explicitly_named_overload_2() {
+ return requires { Fn().*::operator(); };
+}
+
+static_assert(!has_explicitly_named_overload_2());
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
