[llvm-branch-commits] [clang] Backport d23ef9e to release/18.x (PR #83911)

2024-03-04 Thread via llvm-branch-commits 

https://github.com/Sirraide milestoned 
https://github.com/llvm/llvm-project/pull/83911
___
llvm-branch-commits mailing list
llvm-branch-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-branch-commits

[llvm-branch-commits] [clang] Backport d23ef9e to release/18.x (PR #83911)

2024-03-04 Thread via llvm-branch-commits 

https://github.com/Sirraide created 
https://github.com/llvm/llvm-project/pull/83911

Manually cherry-pick and backport 
https://github.com/llvm/llvm-project/commit/d23ef9ef3685eb42ebf719bc28cfe2e4651932fc
 as discussed in https://github.com/llvm/llvm-project/pull/83103.

CC @AaronBallman, @tstellar 

>From 74fa05dead4d52eef3c33406d05dd1bbaf10d546 Mon Sep 17 00:00:00 2001
From: Sirraide 
Date: Tue, 27 Feb 2024 20:19:44 +0100
Subject: [PATCH] [Clang] [Sema] Handle placeholders in '.*' expressions
 (#83103)
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

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 
---
 clang/docs/ReleaseNotes.rst |  2 ++
 clang/lib/Sema/SemaOverload.cpp | 22 +-
 clang/test/SemaCXX/gh53815.cpp  | 21 +
 3 files changed, 40 insertions(+), 5 deletions(-)
 create mode 100644 clang/test/SemaCXX/gh53815.cpp

diff --git a/clang/docs/ReleaseNotes.rst b/clang/docs/ReleaseNotes.rst
index fc27297aea2d6c..101b3a54b9af24 100644
--- a/clang/docs/ReleaseNotes.rst
+++ b/clang/docs/ReleaseNotes.rst
@@ -1103,6 +1103,8 @@ Bug Fixes to C++ Support
   (`#82258 `_)
 - Correctly immediate-escalate lambda conversion functions.
   (`#82258 `_)
+- Fix a crash when an unresolved overload set is encountered on the RHS of a 
``.*`` operator.
+  (`#53815 `_)
 
 Bug Fixes to AST Handling
 ^
diff --git a/clang/lib/Sema/SemaOverload.cpp b/clang/lib/Sema/SemaOverload.cpp
index 940bcccb9e261b..b708272ebe7d87 100644
--- a/clang/lib/Sema/SemaOverload.cpp
+++ b/clang/lib/Sema/SemaOverload.cpp
@@ -14470,6 +14470,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.isUsable())
+Arg = Res.get();
+  return !Res.isUsable();
+};
+
+// 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();
@@ -14489,11 +14506,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]);

[llvm-branch-commits] [clang] Backport d23ef9e to release/18.x (PR #83911)

2024-03-04 Thread via llvm-branch-commits 

llvmbot wrote:




@llvm/pr-subscribers-clang

Author: None (Sirraide)


Changes

Manually cherry-pick and backport 
https://github.com/llvm/llvm-project/commit/d23ef9ef3685eb42ebf719bc28cfe2e4651932fc
 as discussed in https://github.com/llvm/llvm-project/pull/83103.

CC @AaronBallman, @tstellar 

---
Full diff: https://github.com/llvm/llvm-project/pull/83911.diff


3 Files Affected:

- (modified) clang/docs/ReleaseNotes.rst (+2) 
- (modified) clang/lib/Sema/SemaOverload.cpp (+17-5) 
- (added) clang/test/SemaCXX/gh53815.cpp (+21) 


``diff
diff --git a/clang/docs/ReleaseNotes.rst b/clang/docs/ReleaseNotes.rst
index fc27297aea2d6c..101b3a54b9af24 100644
--- a/clang/docs/ReleaseNotes.rst
+++ b/clang/docs/ReleaseNotes.rst
@@ -1103,6 +1103,8 @@ Bug Fixes to C++ Support
   (`#82258 `_)
 - Correctly immediate-escalate lambda conversion functions.
   (`#82258 `_)
+- Fix a crash when an unresolved overload set is encountered on the RHS of a 
``.*`` operator.
+  (`#53815 `_)
 
 Bug Fixes to AST Handling
 ^
diff --git a/clang/lib/Sema/SemaOverload.cpp b/clang/lib/Sema/SemaOverload.cpp
index 940bcccb9e261b..b708272ebe7d87 100644
--- a/clang/lib/Sema/SemaOverload.cpp
+++ b/clang/lib/Sema/SemaOverload.cpp
@@ -14470,6 +14470,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.isUsable())
+Arg = Res.get();
+  return !Res.isUsable();
+};
+
+// 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();
@@ -14489,11 +14506,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().*&Fn::operator(); };
+}
+
+int main() {
+  has_explicitly_named_overload();
+}
+
+template 
+constexpr bool has_explicitly_named_overload_2() {
+  return requires { Fn().*&Fn::operator(); };
+}
+
+static_assert(!has_explicitly_named_overload_2());

``




https://github.com/llvm/llvm-project/pull/83911
___
llvm-branch-commits mailing list
llvm-branch-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-branch-commits

[llvm-branch-commits] [clang] Backport d23ef9e to release/18.x (PR #83911)

2024-03-12 Thread Tom Stellard via llvm-branch-commits 

https://github.com/tstellar updated 
https://github.com/llvm/llvm-project/pull/83911

>From 74fa05dead4d52eef3c33406d05dd1bbaf10d546 Mon Sep 17 00:00:00 2001
From: Sirraide 
Date: Tue, 27 Feb 2024 20:19:44 +0100
Subject: [PATCH] [Clang] [Sema] Handle placeholders in '.*' expressions
 (#83103)
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

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 
---
 clang/docs/ReleaseNotes.rst |  2 ++
 clang/lib/Sema/SemaOverload.cpp | 22 +-
 clang/test/SemaCXX/gh53815.cpp  | 21 +
 3 files changed, 40 insertions(+), 5 deletions(-)
 create mode 100644 clang/test/SemaCXX/gh53815.cpp

diff --git a/clang/docs/ReleaseNotes.rst b/clang/docs/ReleaseNotes.rst
index fc27297aea2d6c..101b3a54b9af24 100644
--- a/clang/docs/ReleaseNotes.rst
+++ b/clang/docs/ReleaseNotes.rst
@@ -1103,6 +1103,8 @@ Bug Fixes to C++ Support
   (`#82258 `_)
 - Correctly immediate-escalate lambda conversion functions.
   (`#82258 `_)
+- Fix a crash when an unresolved overload set is encountered on the RHS of a 
``.*`` operator.
+  (`#53815 `_)
 
 Bug Fixes to AST Handling
 ^
diff --git a/clang/lib/Sema/SemaOverload.cpp b/clang/lib/Sema/SemaOverload.cpp
index 940bcccb9e261b..b708272ebe7d87 100644
--- a/clang/lib/Sema/SemaOverload.cpp
+++ b/clang/lib/Sema/SemaOverload.cpp
@@ -14470,6 +14470,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.isUsable())
+Arg = Res.get();
+  return !Res.isUsable();
+};
+
+// 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();
@@ -14489,11 +14506,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/

[llvm-branch-commits] [clang] Backport d23ef9e to release/18.x (PR #83911)

2024-03-12 Thread Tom Stellard via llvm-branch-commits 

https://github.com/tstellar closed 
https://github.com/llvm/llvm-project/pull/83911
___
llvm-branch-commits mailing list
llvm-branch-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-branch-commits

[llvm-branch-commits] [clang] Backport d23ef9e to release/18.x (PR #83911)

2024-03-12 Thread Tom Stellard via llvm-branch-commits 

tstellar wrote:

Merged: d8352e93c1c8042d9166eab3d76d6c07ef585b6d

https://github.com/llvm/llvm-project/pull/83911
___
llvm-branch-commits mailing list
llvm-branch-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-branch-commits