Page MenuHomePhabricator

Handle explicitly defaulted consteval special members.
ClosedPublic

Authored by usaxena95 on Aug 9 2022, 3:07 AM.

Details

Summary

Followup patch for D128083

Previously, using a non-consteval constructor from an consteval constructor would code generates the consteval constructor.
Example

template <typename T>
struct S {
  T i;
  consteval S() = default;
};
struct Foo {
    Foo() {}
};
void func() {
  S<Foo> three; // incorrectly accepted by clang.
}

This happened because clang erroneously disregards consteval specifier for a consteval explicitly defaulted special member functions in a class template if it has dependent data members without a consteval default constructor.

According to

C++14 [dcl.constexpr]p6 (CWG DR647/CWG DR1358):
If the instantiated template specialization of a constexpr function
template or member function of a class template would fail to satisfy
the requirements for a constexpr function or constexpr constructor, that
specialization is still a constexpr function or constexpr constructor,
even though a call to such a function cannot appear in a constant
expression.

Therefore the consteval defaulted constructor of a class template should be considered consteval even if the data members' default constructors are not consteval.
Keeping this constructor consteval allows complaining while processing the call to data member constructors.
(Same applies for other special member functions).

This works fine even when we have more than one default constructors since we process the constructors after the templates are instantiated.

This does not address initialization issues raised in
2602 and compiler divergence seen in https://godbolt.org/z/va9EMvvMe

Fixes: https://github.com/llvm/llvm-project/issues/51593

Diff Detail

Event Timeline

usaxena95 created this revision.Aug 9 2022, 3:07 AM
Herald added a project: Restricted Project. · View Herald TranscriptAug 9 2022, 3:07 AM
usaxena95 requested review of this revision.Aug 9 2022, 3:07 AM
Herald added a project: Restricted Project. · View Herald TranscriptAug 9 2022, 3:07 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript
usaxena95 updated this revision to Diff 451333.Aug 9 2022, 6:52 PM

Added tests for multiple constructors in a templated class.

usaxena95 updated this revision to Diff 451353.Aug 9 2022, 9:52 PM

Verify that non-consteval constructors (among others) cannot be used in a consteval context.

usaxena95 edited the summary of this revision. (Show Details)Aug 10 2022, 1:03 AM
ilya-biryukov added a comment.EditedAug 10 2022, 3:24 AM

Should we follow suggestion from CWG Issue 2602 to fix this instead?
I think the trick is to keep the constructor consteval, but complain when processing its call, e.g. somewhere around HandleConstructorCall in ExprConstant.cpp.
WDYT?

clang/lib/Sema/SemaDeclCXX.cpp
7207

The function attempts to check whether the implicitly defined member would be constexpr, which should not depend on what is written in the expliclitly defaulted declaration.

So the check should definitely be performed somewhere outside this function.

usaxena95 updated this revision to Diff 451432.Aug 10 2022, 6:04 AM
usaxena95 marked an inline comment as done.

Addressed comment.

As discussed offline, we are already following the discussion 2602.
We are keeping the constructor consteval to allow complaining when we are actually processing the call to data member constructors (instead of the consteval constructor itself.)

usaxena95 edited the summary of this revision. (Show Details)Aug 10 2022, 6:29 AM
usaxena95 removed subscribers: aaron.ballman, ilya-biryukov.
ilya-biryukov accepted this revision.Aug 10 2022, 7:17 AM

LGTM from my side. Thanks for explaining, I was confused because I looked at the code right after defaulted check and thought we were working around that particular error.
Turns out this is not the case and the change was actually marking the function as consteval, just as was expected.

@aaron.ballman, could you also take a quick look here?

clang/test/SemaCXX/cxx2a-consteval.cpp
857

NIT

This revision is now accepted and ready to land.Aug 10 2022, 7:17 AM

This is generally looking good to me, thank you! Just a few minor points and I think this will be ready.

clang/lib/Sema/SemaDeclCXX.cpp
7551

On my earlier patch, @rsmith had suggested that we check if MD is being instantiated here rather than RD to more closely match the standards wording. I think that's a reasonable suggestion -- WDYT?

clang/test/SemaCXX/cxx2a-consteval.cpp
805

Why do we need to use the regex here (and elsewhere)?

aaron.ballman added inline comments.Aug 10 2022, 11:20 AM
clang/test/SemaCXX/cxx2a-consteval.cpp
812

This likely has nothing to do with your changes here, but what the heck is with that leading & in this already-pretty-surprisingly-weird note? Is that aiming for (&fail1)->operator=(good0) for some reason?

shafik added a subscriber: shafik.Aug 10 2022, 11:35 AM
shafik added inline comments.
clang/test/SemaCXX/cxx2a-consteval.cpp
812

Note, we also see a similar diagnostic in SemaCXX/constant-expression-cxx11.cpp so this looks like it existed before.

usaxena95 marked 3 inline comments as done.

Addressed comments.

usaxena95 added inline comments.Aug 10 2022, 11:06 PM
clang/test/SemaCXX/cxx2a-consteval.cpp
805

I wanted to omit the namespace name in the error messages and make the namespace more descriptive. See the .* in the error messages.

aaron.ballman added inline comments.Aug 11 2022, 5:19 AM
clang/test/SemaCXX/cxx2a-consteval.cpp
805

I think it's better to skip the regex and spell out the diagnostic fully (even with the descriptive namespace name). Regular expression matching is going to be slower than regular matching (generally) and there's no real need for it here except brevity.

812

Good catch! I filed https://github.com/llvm/llvm-project/issues/57081 so that hopefully we go back to make that wording better some day.

usaxena95 updated this revision to Diff 451839.Aug 11 2022, 6:50 AM
usaxena95 marked 3 inline comments as done.

Removed regex matching for diagnostics.

aaron.ballman accepted this revision.Aug 11 2022, 8:19 AM

Thanks, this LGTM! Please be sure to add a release note for the fix before landing.

usaxena95 updated this revision to Diff 452123.Aug 12 2022, 3:10 AM

Added a release note.

This revision was landed with ongoing or failed builds.Aug 12 2022, 3:13 AM
This revision was automatically updated to reflect the committed changes.

This change breaks the libc++ test libcxx/test/std/utilities/optional/optional.object/optional.object.ctor/move.fail.cpp. (https://buildkite.com/llvm-project/libcxx-ci/builds/13149#0182d0d4-341a-4b41-b67f-12937f41e6d5)

Looking at the description of this patch it should only affect consteval functions, but it seems to affect constexpr functions too. Can you have a look?

This change breaks the libc++ test libcxx/test/std/utilities/optional/optional.object/optional.object.ctor/move.fail.cpp. (https://buildkite.com/llvm-project/libcxx-ci/builds/13149#0182d0d4-341a-4b41-b67f-12937f41e6d5)

Looking at the description of this patch it should only affect consteval functions, but it seems to affect constexpr functions too. Can you have a look?

Agreed that we should take a look, but it's interesting to note that libstdc++ seems to have a different behavior than libc++ here: https://godbolt.org/z/6cWhf6GYj (the last three compiler panes show Clang 14 libstd++, Clang 14 libc++, and Clang trunk libc++). How sure are you that the libc++ test is actually correct, because (without checking the standard, so take with a giant grain of salt) this seems like it might have fixed a bug in libc++? The release note says this is expected to impact constexpr and consteval default special member functions, so behavioral changes to constexpr aren't unexpected (though I agree that the commit message didn't mention constexpr so the changes may seem surprising).

This change breaks the libc++ test libcxx/test/std/utilities/optional/optional.object/optional.object.ctor/move.fail.cpp. (https://buildkite.com/llvm-project/libcxx-ci/builds/13149#0182d0d4-341a-4b41-b67f-12937f41e6d5)

Looking at the description of this patch it should only affect consteval functions, but it seems to affect constexpr functions too. Can you have a look?

Agreed that we should take a look, but it's interesting to note that libstdc++ seems to have a different behavior than libc++ here: https://godbolt.org/z/6cWhf6GYj (the last three compiler panes show Clang 14 libstd++, Clang 14 libc++, and Clang trunk libc++). How sure are you that the libc++ test is actually correct, because (without checking the standard, so take with a giant grain of salt) this seems like it might have fixed a bug in libc++? The release note says this is expected to impact constexpr and consteval default special member functions, so behavioral changes to constexpr aren't unexpected (though I agree that the commit message didn't mention constexpr so the changes may seem surprising).

Thanks for confirming it's indeed intended to affect constexpr too! It was a bit confusing since it also referred to C++14, which didn't have consteval. I wasn't entirely sure about the status of libc++, but since the patch gave of a mixed message I wanted make sure that constexpr was intended.

I've done some further digging and according to cppreference the paper P0602R4 was a DR against C++20. This was proposed in the paper.
Looking at the changes in the addressing this paper I see no C++ version checks https://reviews.llvm.org/D32385
In a followup introduces tests using C++ version checks https://reviews.llvm.org/D54772
The failing test was introduced in 308624127fd6cc36558b6eee4d4ffa4e215a074e before the paper was voted in

So I think there's indeed a bug in libc++ which was hidden since Clang hadn't implemented some DRs. I will look at the libc++ side what needs to be done to fix the CI failures.

This change breaks the libc++ test libcxx/test/std/utilities/optional/optional.object/optional.object.ctor/move.fail.cpp. (https://buildkite.com/llvm-project/libcxx-ci/builds/13149#0182d0d4-341a-4b41-b67f-12937f41e6d5)

Looking at the description of this patch it should only affect consteval functions, but it seems to affect constexpr functions too. Can you have a look?

Agreed that we should take a look, but it's interesting to note that libstdc++ seems to have a different behavior than libc++ here: https://godbolt.org/z/6cWhf6GYj (the last three compiler panes show Clang 14 libstd++, Clang 14 libc++, and Clang trunk libc++). How sure are you that the libc++ test is actually correct, because (without checking the standard, so take with a giant grain of salt) this seems like it might have fixed a bug in libc++? The release note says this is expected to impact constexpr and consteval default special member functions, so behavioral changes to constexpr aren't unexpected (though I agree that the commit message didn't mention constexpr so the changes may seem surprising).

Thanks for confirming it's indeed intended to affect constexpr too! It was a bit confusing since it also referred to C++14, which didn't have consteval. I wasn't entirely sure about the status of libc++, but since the patch gave of a mixed message I wanted make sure that constexpr was intended.

It was a good question to bring up!

I've done some further digging and according to cppreference the paper P0602R4 was a DR against C++20. This was proposed in the paper.
Looking at the changes in the addressing this paper I see no C++ version checks https://reviews.llvm.org/D32385
In a followup introduces tests using C++ version checks https://reviews.llvm.org/D54772
The failing test was introduced in 308624127fd6cc36558b6eee4d4ffa4e215a074e before the paper was voted in

So I think there's indeed a bug in libc++ which was hidden since Clang hadn't implemented some DRs. I will look at the libc++ side what needs to be done to fix the CI failures.

Great sleuth work! Thank you for the investigation!

So I think there's indeed a bug in libc++ which was hidden since Clang hadn't implemented some DRs. I will look at the libc++ side what needs to be done to fix the CI failures.

Great sleuth work! Thank you for the investigation!

The issue was indeed in libc++, which has been fixed :-)