Skip to content

[Clang] Fix handling of qualified id-expressions in unevaluated contexts #99807

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
Jul 22, 2024

Conversation

cor3ntin
Copy link
Contributor

In #89713, we made qualified, parenthesized id-expression ill-formed in and address of expressions.

The expected behavior should instead be to form a pointer (rather than a pointer to member)

The fix has been suggested by @zwuis and the tests by @hubert-reinterpretcast.

It is worth pointing out that some of these tests seem rejected by all compilers, however the tests do seem correct.

Fixes #89713
Fixes #40906

In llvm#89713, we made qualified, parenthesized id-expression ill-formed in
and address of expressions.

The expected behavior should instead be to form a pointer (rather
than a pointer to member)

The fix has been suggested by @zwuis and the tests by
@hubert-reinterpretcast.

It is worth pointing out that some of these tests seem rejected
by all compilers, however the tests do seem correct.
@llvmbot llvmbot added clang Clang issues not falling into any other category clang:frontend Language frontend issues, e.g. anything involving "Sema" labels Jul 21, 2024
@llvmbot
Copy link
Member

llvmbot commented Jul 21, 2024

@llvm/pr-subscribers-clang

Author: cor3ntin (cor3ntin)

Changes

In #89713, we made qualified, parenthesized id-expression ill-formed in and address of expressions.

The expected behavior should instead be to form a pointer (rather than a pointer to member)

The fix has been suggested by @zwuis and the tests by @hubert-reinterpretcast.

It is worth pointing out that some of these tests seem rejected by all compilers, however the tests do seem correct.

Fixes #89713
Fixes #40906


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

4 Files Affected:

  • (modified) clang/docs/ReleaseNotes.rst (+2-1)
  • (modified) clang/include/clang/Basic/DiagnosticSemaKinds.td (-3)
  • (modified) clang/lib/Sema/SemaExpr.cpp (+8-17)
  • (modified) clang/test/CXX/expr/expr.unary/expr.unary.op/p4.cpp (+30-12)
diff --git a/clang/docs/ReleaseNotes.rst b/clang/docs/ReleaseNotes.rst
index e0e86af257a19..3dccd85895160 100644
--- a/clang/docs/ReleaseNotes.rst
+++ b/clang/docs/ReleaseNotes.rst
@@ -58,7 +58,8 @@ C++ Specific Potentially Breaking Changes
   versions of clang. The deprecation warning for the negative spelling can be
   disabled with `-Wno-deprecated-no-relaxed-template-template-args`.
 
-- Clang now rejects pointer to member from parenthesized expression in unevaluated context such as ``decltype(&(foo::bar))``. (#GH40906).
+- Clang no longer tries to form pointer-to-members from qualified and parenthesized unevaluated expressions
+  such``decltype(&(foo::bar))``. (#GH40906).
 
 - Clang now performs semantic analysis for unary operators with dependent operands
   that are known to be of non-class non-enumeration type prior to instantiation.
diff --git a/clang/include/clang/Basic/DiagnosticSemaKinds.td b/clang/include/clang/Basic/DiagnosticSemaKinds.td
index d60f32674ca3a..0d6af644f1539 100644
--- a/clang/include/clang/Basic/DiagnosticSemaKinds.td
+++ b/clang/include/clang/Basic/DiagnosticSemaKinds.td
@@ -7596,9 +7596,6 @@ def err_nested_non_static_member_use : Error<
 def warn_cxx98_compat_non_static_member_use : Warning<
   "use of non-static data member %0 in an unevaluated context is "
   "incompatible with C++98">, InGroup<CXX98Compat>, DefaultIgnore;
-def err_form_ptr_to_member_from_parenthesized_expr : Error<
-  "cannot form pointer to member from a parenthesized expression; "
-  "did you mean to remove the parentheses?">;
 def err_invalid_incomplete_type_use : Error<
   "invalid use of incomplete type %0">;
 def err_builtin_func_cast_more_than_one_arg : Error<
diff --git a/clang/lib/Sema/SemaExpr.cpp b/clang/lib/Sema/SemaExpr.cpp
index 8d24e34520e77..1a441d99515f4 100644
--- a/clang/lib/Sema/SemaExpr.cpp
+++ b/clang/lib/Sema/SemaExpr.cpp
@@ -14117,7 +14117,14 @@ QualType Sema::CheckAddressOfOperand(ExprResult &OrigOp, SourceLocation OpLoc) {
       // Okay: we can take the address of a field.
       // Could be a pointer to member, though, if there is an explicit
       // scope qualifier for the class.
-      if (isa<DeclRefExpr>(op) && cast<DeclRefExpr>(op)->getQualifier()) {
+
+      // [C++26] [expr.prim.id.general]
+      // If an id-expression E denotes a non-static non-type member
+      // of some class C [...] and if E is a qualified-id, E is
+      // not the un-parenthesized operand of the unary & operator [...]
+      // the id-expression is transformed into a class member access expression.
+      if (isa<DeclRefExpr>(op) && cast<DeclRefExpr>(op)->getQualifier() &&
+          !isa<ParenExpr>(OrigOp.get())) {
         DeclContext *Ctx = dcl->getDeclContext();
         if (Ctx && Ctx->isRecord()) {
           if (dcl->getType()->isReferenceType()) {
@@ -14127,22 +14134,6 @@ QualType Sema::CheckAddressOfOperand(ExprResult &OrigOp, SourceLocation OpLoc) {
             return QualType();
           }
 
-          // C++11 [expr.unary.op] p4:
-          // A pointer to member is only formed when an explicit & is used and
-          // its operand is a qualified-id not enclosed in parentheses.
-          if (isa<ParenExpr>(OrigOp.get())) {
-            SourceLocation LeftParenLoc = OrigOp.get()->getBeginLoc(),
-                           RightParenLoc = OrigOp.get()->getEndLoc();
-
-            Diag(LeftParenLoc,
-                 diag::err_form_ptr_to_member_from_parenthesized_expr)
-                << SourceRange(OpLoc, RightParenLoc)
-                << FixItHint::CreateRemoval(LeftParenLoc)
-                << FixItHint::CreateRemoval(RightParenLoc);
-
-            // Continuing might lead to better error recovery.
-          }
-
           while (cast<RecordDecl>(Ctx)->isAnonymousStructOrUnion())
             Ctx = Ctx->getParent();
 
diff --git a/clang/test/CXX/expr/expr.unary/expr.unary.op/p4.cpp b/clang/test/CXX/expr/expr.unary/expr.unary.op/p4.cpp
index 162d59439d08e..170ca0a3f1c6b 100644
--- a/clang/test/CXX/expr/expr.unary/expr.unary.op/p4.cpp
+++ b/clang/test/CXX/expr/expr.unary/expr.unary.op/p4.cpp
@@ -43,18 +43,36 @@ namespace test2 {
 }
 
 namespace GH40906 {
-  struct A {
-    int val;
-    void func() {}
-  };
+struct S {
+    int x;
+    void func();
+    static_assert(__is_same_as(decltype((S::x)), int&), "");
+    static_assert(__is_same_as(decltype(&(S::x)), int*), "");
 
-  void test() {
-    decltype(&(A::val)) ptr1; // expected-error {{cannot form pointer to member from a parenthesized expression; did you mean to remove the parentheses?}}
-    int A::* ptr2 = &(A::val); // expected-error {{invalid use of non-static data member 'val'}}
+    // FIXME: provide better error messages
+    static_assert(__is_same_as(decltype((S::func)), int&), ""); // expected-error {{call to non-static member function without an object argument}}
+    static_assert(__is_same_as(decltype(&(S::func)), int*), ""); // expected-error {{call to non-static member function without an object argument}}
+};
+static_assert(__is_same_as(decltype((S::x)), int&), "");
+static_assert(__is_same_as(decltype(&(S::x)), int*), "");
+static_assert(__is_same_as(decltype((S::func)), int&), ""); // expected-error {{call to non-static member function without an object argument}}
+static_assert(__is_same_as(decltype(&(S::func)), int*), ""); // expected-error {{call to non-static member function without an object argument}}
+
+struct A { int x;};
+
+char q(int *);
+short q(int A::*);
+
+template <typename T>
+constexpr int f(char (*)[sizeof(q(&T::x))]) { return 1; }
+
+template <typename T>
+constexpr int f(char (*)[sizeof(q(&(T::x)))]) { return 2; }
+
+constexpr int g(char (*p)[sizeof(char)] = 0) { return f<A>(p); }
+constexpr int h(char (*p)[sizeof(short)] = 0) { return f<A>(p); }
+
+static_assert(g() == 2);
+static_assert(h() == 1);
 
-    // FIXME: Error messages in these cases are less than clear, we can do
-    // better.
-    int size = sizeof(&(A::func)); // expected-error {{call to non-static member function without an object argument}}
-    void (A::* ptr3)() = &(A::func); // expected-error {{call to non-static member function without an object argument}}
-  }
 }

@cor3ntin cor3ntin self-assigned this Jul 21, 2024
Copy link
Collaborator

@AaronBallman AaronBallman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thank you for the fix!

@cor3ntin cor3ntin merged commit 3c459cf into llvm:main Jul 22, 2024
8 checks passed
@cor3ntin cor3ntin deleted the corentin/fix-gh89713 branch July 22, 2024 13:57
@llvm-ci
Copy link
Collaborator

llvm-ci commented Jul 22, 2024

LLVM Buildbot has detected a new failure on builder lldb-aarch64-ubuntu running on linaro-lldb-aarch64-ubuntu while building clang at step 6 "test".

Full details are available at: https://lab.llvm.org/buildbot/#/builders/59/builds/1986

Here is the relevant piece of the build log for the reference:

Step 6 (test) failure: build (failure)
...
PASS: lldb-unit :: ValueObject/./LLDBValueObjectTests/0/3 (1985 of 1994)
PASS: lldb-unit :: ValueObject/./LLDBValueObjectTests/1/3 (1986 of 1994)
PASS: lldb-unit :: ValueObject/./LLDBValueObjectTests/2/3 (1987 of 1994)
PASS: lldb-unit :: tools/lldb-server/tests/./LLDBServerTests/0/2 (1988 of 1994)
PASS: lldb-unit :: Utility/./UtilityTests/5/8 (1989 of 1994)
PASS: lldb-unit :: tools/lldb-server/tests/./LLDBServerTests/1/2 (1990 of 1994)
PASS: lldb-unit :: Utility/./UtilityTests/6/8 (1991 of 1994)
PASS: lldb-shell :: SymbolFile/target-symbols-add-unwind.test (1992 of 1994)
PASS: lldb-unit :: Process/gdb-remote/./ProcessGdbRemoteTests/8/10 (1993 of 1994)
UNRESOLVED: lldb-api :: tools/lldb-server/TestLldbGdbServer.py (1994 of 1994)
******************** TEST 'lldb-api :: tools/lldb-server/TestLldbGdbServer.py' FAILED ********************
Script:
--
/usr/bin/python3.8 /home/tcwg-buildbot/worker/lldb-aarch64-ubuntu/llvm-project/lldb/test/API/dotest.py -u CXXFLAGS -u CFLAGS --env ARCHIVER=/usr/local/bin/llvm-ar --env OBJCOPY=/usr/bin/llvm-objcopy --env LLVM_LIBS_DIR=/home/tcwg-buildbot/worker/lldb-aarch64-ubuntu/build/./lib --env LLVM_INCLUDE_DIR=/home/tcwg-buildbot/worker/lldb-aarch64-ubuntu/build/include --env LLVM_TOOLS_DIR=/home/tcwg-buildbot/worker/lldb-aarch64-ubuntu/build/./bin --arch aarch64 --build-dir /home/tcwg-buildbot/worker/lldb-aarch64-ubuntu/build/lldb-test-build.noindex --lldb-module-cache-dir /home/tcwg-buildbot/worker/lldb-aarch64-ubuntu/build/lldb-test-build.noindex/module-cache-lldb/lldb-api --clang-module-cache-dir /home/tcwg-buildbot/worker/lldb-aarch64-ubuntu/build/lldb-test-build.noindex/module-cache-clang/lldb-api --executable /home/tcwg-buildbot/worker/lldb-aarch64-ubuntu/build/./bin/lldb --compiler /home/tcwg-buildbot/worker/lldb-aarch64-ubuntu/build/./bin/clang --dsymutil /home/tcwg-buildbot/worker/lldb-aarch64-ubuntu/build/./bin/dsymutil --llvm-tools-dir /home/tcwg-buildbot/worker/lldb-aarch64-ubuntu/build/./bin --lldb-obj-root /home/tcwg-buildbot/worker/lldb-aarch64-ubuntu/build/tools/lldb --lldb-libs-dir /home/tcwg-buildbot/worker/lldb-aarch64-ubuntu/build/./lib /home/tcwg-buildbot/worker/lldb-aarch64-ubuntu/llvm-project/lldb/test/API/tools/lldb-server -p TestLldbGdbServer.py
--
Exit Code: 1

Command Output (stdout):
--
lldb version 19.0.0git (https://github.com/llvm/llvm-project.git revision 3c459cfcaebdaf7cabac33a0e18bf6588cef4cdb)
  clang revision 3c459cfcaebdaf7cabac33a0e18bf6588cef4cdb
  llvm revision 3c459cfcaebdaf7cabac33a0e18bf6588cef4cdb
Skipping the following test categories: ['libc++', 'dsym', 'gmodules', 'debugserver', 'objc']

--
Command Output (stderr):
--
Program aborted due to an unhandled Error:
Operation not permitted
PLEASE submit a bug report to https://github.com/llvm/llvm-project/issues/ and include the crash backtrace.
Stack dump:
0.	Program arguments: /home/tcwg-buildbot/worker/lldb-aarch64-ubuntu/build/bin/lldb-server gdbserver --attach=1301674 --reverse-connect [127.0.0.1]:55939
 #0 0x0000aaaad497bfb0 llvm::sys::PrintStackTrace(llvm::raw_ostream&, int) (/home/tcwg-buildbot/worker/lldb-aarch64-ubuntu/build/bin/lldb-server+0xb2bfb0)
 #1 0x0000aaaad4979f2c llvm::sys::RunSignalHandlers() (/home/tcwg-buildbot/worker/lldb-aarch64-ubuntu/build/bin/lldb-server+0xb29f2c)
 #2 0x0000aaaad497c6d0 SignalHandler(int) Signals.cpp:0:0
 #3 0x0000ffffbb9f27dc (linux-vdso.so.1+0x7dc)
 #4 0x0000ffffbb26bd78 raise /build/glibc-Q8DG8B/glibc-2.31/signal/../sysdeps/unix/sysv/linux/raise.c:51:1
 #5 0x0000ffffbb258aac abort /build/glibc-Q8DG8B/glibc-2.31/stdlib/abort.c:81:7
 #6 0x0000aaaad49271e0 llvm::Error::fatalUncheckedError() const (/home/tcwg-buildbot/worker/lldb-aarch64-ubuntu/build/bin/lldb-server+0xad71e0)
 #7 0x0000aaaad49ad8b0 lldb_private::process_linux::NativeProcessLinux::Attach(int) (/home/tcwg-buildbot/worker/lldb-aarch64-ubuntu/build/bin/lldb-server+0xb5d8b0)
 #8 0x0000aaaad49acedc lldb_private::process_linux::NativeProcessLinux::Manager::Attach(unsigned long, lldb_private::NativeProcessProtocol::NativeDelegate&) (/home/tcwg-buildbot/worker/lldb-aarch64-ubuntu/build/bin/lldb-server+0xb5cedc)
 #9 0x0000aaaad4a22df0 lldb_private::process_gdb_remote::GDBRemoteCommunicationServerLLGS::AttachToProcess(unsigned long) (/home/tcwg-buildbot/worker/lldb-aarch64-ubuntu/build/bin/lldb-server+0xbd2df0)
#10 0x0000aaaad4908d50 handle_attach(lldb_private::process_gdb_remote::GDBRemoteCommunicationServerLLGS&, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char>> const&) (/home/tcwg-buildbot/worker/lldb-aarch64-ubuntu/build/bin/lldb-server+0xab8d50)
#11 0x0000aaaad490a9d8 main_gdbserver(int, char**) (/home/tcwg-buildbot/worker/lldb-aarch64-ubuntu/build/bin/lldb-server+0xaba9d8)
#12 0x0000aaaad490e79c main (/home/tcwg-buildbot/worker/lldb-aarch64-ubuntu/build/bin/lldb-server+0xabe79c)
#13 0x0000ffffbb258e10 __libc_start_main /build/glibc-Q8DG8B/glibc-2.31/csu/../csu/libc-start.c:342:3
#14 0x0000aaaad4906ff4 _start (/home/tcwg-buildbot/worker/lldb-aarch64-ubuntu/build/bin/lldb-server+0xab6ff4)
lldb-server exiting...
LLVM ERROR: IO failure on output stream: Bad file descriptor

yuxuanchen1997 pushed a commit that referenced this pull request Jul 25, 2024
…xts (#99807)

Summary:
In #89713, we made qualified, parenthesized id-expression ill-formed in
and address of expressions.

The expected behavior should instead be to form a pointer (rather than a
pointer to member)

The fix has been suggested by @zwuis and the tests by
@hubert-reinterpretcast.

It is worth pointing out that some of these tests seem rejected by all
compilers, however the tests do seem correct.

Fixes #89713
Fixes #40906

---------

Co-authored-by: YanzuoLiu <[email protected]>

Test Plan: 

Reviewers: 

Subscribers: 

Tasks: 

Tags: 


Differential Revision: https://phabricator.intern.facebook.com/D60251286
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
clang:frontend Language frontend issues, e.g. anything involving "Sema" clang Clang issues not falling into any other category
Projects
None yet
Development

Successfully merging this pull request may close these issues.

decltype(&(C::member)) yields pointer-to-member type
5 participants