[Clang] [Sema] Handle placeholders in '.*' expressions #83103

Sirraide · 2024-02-27T05:35:50Z

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,

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.
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.

llvmbot · 2024-02-27T05:36:20Z

@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,

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.
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 --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 *&Arg) {
+      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 00000000000000..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 <typename Fn>
+static bool has_explicitly_named_overload() {
+  return requires { Fn().*&Fn::operator(); };
+}
+
+int main() {
+  has_explicitly_named_overload<decltype([](auto){})>();
+}
+
+template <typename Fn>
+constexpr bool has_explicitly_named_overload_2() {
+  return requires { Fn().*&Fn::operator(); };
+}
+
+static_assert(!has_explicitly_named_overload_2<decltype([](auto){})>());

Sirraide · 2024-02-27T05:38:49Z

CC @AaronBallman, @cor3ntin

Sirraide · 2024-02-27T05:53:54Z

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.

Sirraide · 2024-02-27T06:37:48Z

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.

Sirraide · 2024-02-27T06:52:12Z

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):
526ab47#diff-3c28567b5e0c77d68f174541a0b77f5a85d093f58b89cd3675ee04a550a44880L7784-L7800

Sirraide · 2024-02-27T06:54:03Z

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.

Sirraide · 2024-02-27T07:40:26Z

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.

AaronBallman

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).

clang/lib/Sema/SemaOverload.cpp

Sirraide · 2024-02-27T17:19:49Z

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

AaronBallman

LGTM!

Sirraide · 2024-02-27T19:05:22Z

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

AaronBallman · 2024-02-27T19:19:21Z

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.

ADKaster · 2024-03-01T22:27:19Z

@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

AaronBallman · 2024-03-04T20:23:27Z

@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

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

Sirraide · 2024-03-04T20:54:13Z

/cherry-pick d23ef9e

llvmbot · 2024-03-04T20:59:33Z

Failed to cherry-pick: d23ef9e

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

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 llvm#53815. --------- Co-authored-by: Aaron Ballman <aaron@aaronballman.com>

Sirraide · 2024-03-04T21:11:42Z

/cherry-pick Sirraide@74fa05d

llvmbot · 2024-03-04T21:16:31Z

Failed to cherry-pick: Sirraide@74fa05d

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

Sirraide · 2024-03-04T21:19:45Z

Hmm, I’m not sure I’m doing this properly; I’ll try and see if I can open a pr manually.

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. --------- Co-authored-by: Aaron Ballman <aaron@aaronballman.com>

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 llvm#53815. --------- Co-authored-by: Aaron Ballman <aaron@aaronballman.com> (cherry picked from commit d23ef9e)

[Clang] [Sema] Handle placeholders in '.*' expressions (llvm#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 llvm#53815. --------- Co-authored-by: Aaron Ballman <aaron@aaronballman.com> (cherry picked from commit d23ef9e)

🍒 [Clang] [Sema] Handle placeholders in '.*' expressions (llvm#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 llvm#53815. --------- Co-authored-by: Aaron Ballman <aaron@aaronballman.com>

[Clang] [Sema] Handle placeholders in '.*' expressions

071b628

llvmbot added clang Clang issues not falling into any other category clang:frontend Language frontend issues, e.g. anything involving "Sema" labels Feb 27, 2024

ADKaster mentioned this pull request Feb 27, 2024

AK: AK::Detail::VisitImpl::has_explicitly_named_overload causes ICE in clang assertions build SerenityOS/serenity#23365

Closed

AaronBallman reviewed Feb 27, 2024

View reviewed changes

clang/lib/Sema/SemaOverload.cpp Outdated Show resolved Hide resolved

Sirraide added 3 commits February 27, 2024 18:30

Merge branch 'main' into 53815

f1ca53f

[Clang] Update release notes

b39abdb

[NFC] Use isUsable() instead of isInvalid()

c52a0e9

AaronBallman approved these changes Feb 27, 2024

View reviewed changes

Merge branch 'main' into 53815

daeade2

AaronBallman merged commit d23ef9e into llvm:main Feb 27, 2024

Sirraide deleted the 53815 branch February 27, 2024 19:27

Sirraide added this to the LLVM 18.X Release milestone Mar 4, 2024

llvmbot added the release:cherry-pick-failed label Mar 4, 2024

Sirraide mentioned this pull request Mar 4, 2024

Backport d23ef9e to release/18.x #83911

Closed

ADKaster mentioned this pull request Apr 18, 2024

[ladybird] clang 18.1.4 ICE #89157

Closed

pointhex mentioned this pull request May 7, 2024

getStyleDiagHandler #91314

Closed

aemerson mentioned this pull request May 9, 2024

release/18.x: [AArc64][GlobalISel] Fix legalizer assert for G_INSERT_VECTOR_ELT - manual merge #91672

Merged

ADKaster mentioned this pull request Jul 22, 2024

Please backport d8352e93c1c8042d9166eab3d76d6c07ef585b6d swiftlang/llvm-project#8998

Closed

compnerd added a commit to swiftlang/llvm-project that referenced this pull request Aug 12, 2024

Merge pull request #9089 from compnerd/53815

57564d4

[Clang] [Sema] Handle placeholders in '.*' expressions (llvm#83103)

DougGregor added a commit to swiftlang/llvm-project that referenced this pull request Aug 20, 2024

Merge pull request #9038 from ADKaster/backport-llvm-53815-to-swift-6x

392fe16

🍒 [Clang] [Sema] Handle placeholders in '.*' expressions (llvm#83103)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Clang] [Sema] Handle placeholders in '.*' expressions #83103

[Clang] [Sema] Handle placeholders in '.*' expressions #83103

Sirraide commented Feb 27, 2024

llvmbot commented Feb 27, 2024

Sirraide commented Feb 27, 2024

Sirraide commented Feb 27, 2024

Sirraide commented Feb 27, 2024

Sirraide commented Feb 27, 2024

Sirraide commented Feb 27, 2024

Sirraide commented Feb 27, 2024

AaronBallman left a comment

Sirraide commented Feb 27, 2024

AaronBallman left a comment

Sirraide commented Feb 27, 2024

AaronBallman commented Feb 27, 2024

ADKaster commented Mar 1, 2024

AaronBallman commented Mar 4, 2024

Sirraide commented Mar 4, 2024

llvmbot commented Mar 4, 2024

Sirraide commented Mar 4, 2024

llvmbot commented Mar 4, 2024

Sirraide commented Mar 4, 2024

[Clang] [Sema] Handle placeholders in '.*' expressions #83103

[Clang] [Sema] Handle placeholders in '.*' expressions #83103

Conversation

Sirraide commented Feb 27, 2024

llvmbot commented Feb 27, 2024

Sirraide commented Feb 27, 2024

Sirraide commented Feb 27, 2024

Sirraide commented Feb 27, 2024

Sirraide commented Feb 27, 2024

Sirraide commented Feb 27, 2024

Sirraide commented Feb 27, 2024

AaronBallman left a comment

Choose a reason for hiding this comment

Sirraide commented Feb 27, 2024

AaronBallman left a comment

Choose a reason for hiding this comment

Sirraide commented Feb 27, 2024

AaronBallman commented Feb 27, 2024

ADKaster commented Mar 1, 2024

AaronBallman commented Mar 4, 2024

Sirraide commented Mar 4, 2024

llvmbot commented Mar 4, 2024

Sirraide commented Mar 4, 2024

llvmbot commented Mar 4, 2024

Sirraide commented Mar 4, 2024