[clang] Fix constant evaluation of member pointer access into sibling class. #150829
Conversation
… sibling class. HandleMemberPointerAccess considered whether the lvalue path in a member pointer access matched the bases of the containing class of the member, but neglected to check the same for the containing class of the member itself, thereby ignoring access attempts to members in direct sibling classes. Fixes llvm#150705. Fixes llvm#150709.
|
Thank you for submitting a Pull Request (PR) to the LLVM Project! This PR will be automatically labeled and the relevant teams will be notified. If you wish to, you can add reviewers by using the "Reviewers" section on this page. If this is not working for you, it is probably because you do not have write permissions for the repository. In which case you can instead tag reviewers by name in a comment by using If you have received no comments on your PR for a week, you can request a review by "ping"ing the PR by adding a comment “Ping”. The common courtesy "ping" rate is once a week. Please remember that you are asking for valuable time from other developers. If you have further questions, they may be answered by the LLVM GitHub User Guide. You can also ask questions in a comment on this PR, on the LLVM Discord or on the forums. |
llvmbot
added
clang
|
@llvm/pr-subscribers-clang Author: None (keinflue) ChangesHandleMemberPointerAccess considered whether the lvalue path in a member pointer access matched the bases of the containing class of the member, but neglected to check the same for the containing class of the member itself, thereby ignoring access attempts to members in direct sibling classes. I am new affected code and not 100% sure that the conditions I chose are correct. At least the test cases I added pass. Fixes #150705. Full diff: https://github.com/llvm/llvm-project/pull/150829.diff 3 Files Affected:
diff --git a/clang/lib/AST/ExprConstant.cpp b/clang/lib/AST/ExprConstant.cpp
index 9808298a1b1d0..83096f2d982b8 100644
--- a/clang/lib/AST/ExprConstant.cpp
+++ b/clang/lib/AST/ExprConstant.cpp
@@ -5051,6 +5051,16 @@ static const ValueDecl *HandleMemberPointerAccess(EvalInfo &Info,
return nullptr;
}
}
+ // Consider member in a sibling.
+ const CXXRecordDecl *LastLVDecl =
+ (PathLengthToMember > LV.Designator.MostDerivedPathLength)
+ ? getAsBaseClass(LV.Designator.Entries[PathLengthToMember - 1])
+ : LV.Designator.MostDerivedType->getAsCXXRecordDecl();
+ const CXXRecordDecl *LastMPDecl = MemPtr.getContainingRecord();
+ if (LastLVDecl->getCanonicalDecl() != LastMPDecl->getCanonicalDecl()) {
+ Info.FFDiag(RHS);
+ return nullptr;
+ }
// Truncate the lvalue to the appropriate derived class.
if (!CastToDerivedClass(Info, RHS, LV, MemPtr.getContainingRecord(),
diff --git a/clang/test/SemaCXX/constant-expression-cxx11.cpp b/clang/test/SemaCXX/constant-expression-cxx11.cpp
index 5ecb8c607f59a..4584f53aa16f0 100644
--- a/clang/test/SemaCXX/constant-expression-cxx11.cpp
+++ b/clang/test/SemaCXX/constant-expression-cxx11.cpp
@@ -1210,6 +1210,24 @@ namespace MemberPointer {
return (a.*f)();
}
static_assert(apply(A(2), &A::f) == 5, "");
+
+ struct C { };
+ struct D : C {
+ constexpr int f() const { return 1; };
+ };
+ struct E : C { };
+ struct F : D { };
+ constexpr C c1, c2[2];
+ constexpr D d1, d2[2];
+ constexpr E e1, e2[2];
+ constexpr F f;
+ static_assert((c1.*(static_cast<int (C::*)() const>(&D::f)))() == 1, ""); // expected-error {{constant expression}}
+ static_assert((d1.*(static_cast<int (C::*)() const>(&D::f)))() == 1, "");
+ static_assert((e1.*(static_cast<int (C::*)() const>(&D::f)))() == 1, ""); // expected-error {{constant expression}}
+ static_assert((f.*(static_cast<int (C::*)() const>(&D::f)))() == 1, "");
+ static_assert((c2[0].*(static_cast<int (C::*)() const>(&D::f)))() == 1, ""); // expected-error {{constant expression}}
+ static_assert((d2[0].*(static_cast<int (C::*)() const>(&D::f)))() == 1, "");
+ static_assert((e2[0].*(static_cast<int (C::*)() const>(&D::f)))() == 1, ""); // expected-error {{constant expression}}
}
namespace ArrayBaseDerived {
diff --git a/clang/test/SemaCXX/constant-expression-cxx2a.cpp b/clang/test/SemaCXX/constant-expression-cxx2a.cpp
index ffb7e633c2919..b22a82c57ef06 100644
--- a/clang/test/SemaCXX/constant-expression-cxx2a.cpp
+++ b/clang/test/SemaCXX/constant-expression-cxx2a.cpp
@@ -1497,3 +1497,16 @@ namespace GH67317 {
// expected-note {{subobject of type 'const unsigned char' is not initialized}}
__builtin_bit_cast(unsigned char, *new char[3][1]);
};
+
+namespace GH150705 {
+ struct A { };
+ struct B : A { };
+ struct C : A {
+ constexpr virtual int foo() const { return 0; }
+ };
+ constexpr auto p = &C::foo;
+ constexpr auto q = static_cast<int (A::*)() const>(p);
+ constexpr B b;
+ constexpr const A& a = b;
+ constexpr auto x = (a.*q)(); // expected-error {{constant expression}}
+}
|
erichkeane
left a comment
erichkeane
left a comment
There was a problem hiding this comment.
Not quite sure I get what is going on but:
1- we need a release note
2- I need a better description of what is happening here. Your tests doesn't make it clear that it is only the e1 and e2 expressions that are changing the behavior? Is that correct?
3- Please put a comment that explains what is happening. Consider member in a sibling is not nearly good enough. A standards quite in particular is preferred.
|
ping and question: I have addressed the last review via commit and gotten approval. Should I wait for the second review or should I proceed in a different way? |
|
I think you're good to go, feel free to click the merge button. Let me know if you don't have commit rights yet, and I'll click it for you :) |
|
@erichkeane I do not have commit access, so I would appreciate if you could do it for me. The state of the PR should be fine, unless I should squash and rebase first myself. |
| of the accessed object but is instead located directly in a sibling class to one of the classes | ||
| along the inheritance hierarchy of the most-derived object as ill-formed. | ||
| Other scenarios in which the member is not member of the most derived object were already | ||
| diagnosed previously. (#GH150709) |
There was a problem hiding this comment.
Thanks for adding a release note. I am fine with how this paragraph is written but I must say that first run-on sentence reminded me of this short video 😅
The handling of the member pointer access looks good and the tests look great. LGTM.
|
@keinflue Congratulations on having your first Pull Request (PR) merged into the LLVM Project! Your changes will be combined with recent changes from other authors, then tested by our build bots. If there is a problem with a build, you may receive a report in an email or a comment on this PR. Please check whether problems have been caused by your change specifically, as the builds can include changes from many authors. It is not uncommon for your change to be included in a build that fails due to someone else's changes, or infrastructure issues. How to do this, and the rest of the post-merge process, is covered in detail here. If your change does cause a problem, it may be reverted, or you can revert it yourself. This is a normal part of LLVM development. You can fix your changes and open a new PR to merge them again. If you don't get any reports, no action is required from you. Your changes are working as expected, well done! |
|
@erichkeane Thank you. I realized that both issues fixed by this PR are still present in similar form if Given that the option is experimental, is this expected to work at the moment? Should that be a new issue or should the issues be reopened? |
cc @tbaederr |
|
I fixed the worst offender in github.com/llvm/llvm-project/commit/193995d5a21dc8b923e19d9370aa8e1f374cd940, now |
5 participants
HandleMemberPointerAccess considered whether the lvalue path in a member pointer access matched the bases of the containing class of the member, but neglected to check the same for the containing class of the member itself, thereby ignoring access attempts to members in direct sibling classes.
Fixes #150705.
Fixes #150709.