Page MenuHomePhabricator

[clang] Apply P1825 as Defect Report from C++11 up to C++20.
ClosedPublic

Authored by mizvekov on Jun 17 2021, 6:44 PM.

Details

Summary

This extends the effects of P1825 to all C++ standards from C++11 up to C++20.

According to Motion 23 from Cologne 2019, P1825R0 was accepted as a Defect Report, so we retroactively apply this all the way back to C++11.

Note that we also remove implicit moves from C++98 as an extension
altogether, since the expanded first overload resolution from P1825
can cause some meaning changes in C++98.
For example it can change which copy constructor is picked when both const
and non-const ones are available.

This also rips out warn_return_std_move since there are no cases where it would be worthwhile to suggest it.

This also fixes a bug with bailing into the second overload resolution
when encountering a non-rvref qualified conversion operator.
This was unnoticed until now, so two new test cases cover these.

Signed-off-by: Matheus Izvekov <mizvekov@gmail.com>

Diff Detail

Event Timeline

mizvekov requested review of this revision.Jun 17 2021, 6:44 PM
mizvekov created this revision.
Herald added a project: Restricted Project. · View Herald TranscriptJun 17 2021, 6:44 PM
Herald added a subscriber: cfe-commits. · View Herald Transcript
mizvekov updated this revision to Diff 352894.Jun 17 2021, 6:48 PM

set parent revision.

Quuxplusone added inline comments.
clang/test/CXX/class/class.init/class.copy.elision/p3.cpp
59–61

Personally, I don't think the world will accept applying P0527 unconditionally in pre-C++20 modes. But I guess we'll find out. :P

Patch is missing description

Patch is missing description

Yes sorry for the noise, I do that sometimes just to let the bots test my patch before it is fully ready for review.
If you have any tips for me so I can upload a diff and have it tested, without having to create the DR for it, would appreciate.

clang/test/CXX/class/class.init/class.copy.elision/p3.cpp
59–61

Indeed :)
By the way, I have not found a counter example of strict C++98 code (not using extensions) breaking with implicit moves enabled.
So it looks like we can have one mode for everything here.

mizvekov edited the summary of this revision. (Show Details)Jun 18 2021, 1:15 PM
mizvekov updated this revision to Diff 353127.Jun 18 2021, 4:40 PM

rip out warn_return_std_move completely.

mizvekov retitled this revision from DRAFT: [clang] Apply P1825 as DR for all modes below C++20. to [clang] Apply P1825 as Defect Report for all C++ standards up to C++20..Jun 18 2021, 4:53 PM
mizvekov edited the summary of this revision. (Show Details)
mizvekov edited the summary of this revision. (Show Details)Jun 18 2021, 4:56 PM
mizvekov edited the summary of this revision. (Show Details)Jun 18 2021, 5:05 PM
mizvekov edited the summary of this revision. (Show Details)Jun 18 2021, 5:12 PM
mizvekov added inline comments.Jun 18 2021, 5:30 PM
clang/test/CXX/class/class.init/class.copy.elision/p3.cpp
59–61

Oh never mind I was wrong :)

Found your example: https://godbolt.org/z/T374o518W

Too bad we had not put it in our test suite.

mizvekov retitled this revision from [clang] Apply P1825 as Defect Report for all C++ standards up to C++20. to [clang] Apply P1825 as Defect Report for all C++ standards from C++11 up to C++20..Jun 18 2021, 5:30 PM
mizvekov edited the summary of this revision. (Show Details)
mizvekov edited the summary of this revision. (Show Details)Jun 18 2021, 6:47 PM
mizvekov updated this revision to Diff 353151.Jun 18 2021, 6:50 PM

Don't apply implicit moves to C++98, and add tests which demonstrate incompatibilities this would cause.

mizvekov edited the summary of this revision. (Show Details)Jun 19 2021, 3:35 PM
mizvekov updated this revision to Diff 353215.Jun 19 2021, 3:37 PM

tests: improve consistency of expcted messages..

mizvekov retitled this revision from [clang] Apply P1825 as Defect Report for all C++ standards from C++11 up to C++20. to [clang] Apply P1825 as Defect Report from C++11 up to C++20..Jun 19 2021, 3:40 PM
mizvekov updated this revision to Diff 353320.Jun 21 2021, 3:33 AM

remove unnecessary qualification.

rsmith accepted this revision.Thu, Jun 24, 12:29 PM
This revision is now accepted and ready to land.Thu, Jun 24, 12:29 PM
mizvekov updated this revision to Diff 354715.Sat, Jun 26, 4:28 PM

Simplify checking first overload reslution result.

rsmith added inline comments.Sat, Jun 26, 5:04 PM
clang/lib/Sema/SemaStmt.cpp
3469

Looks like you lost a word in this comment.

3476–3479

Is the removal of this check for an rvalue ref qualifier a bugfix? If so, please add a test. I suppose it would look something like this:

struct B;
struct A { A(B&) = delete; };
struct B { operator A(); };
A f() { B b; return b; }

... which would be ambiguous if we don't implicitly convert to xvalue, but if we do, it selects a conversion function that is not rvalue-ref-qualified.

mizvekov added inline comments.Sat, Jun 26, 5:59 PM
clang/lib/Sema/SemaStmt.cpp
3476–3479

Yeah you are right. I was reading that section of the standard again in bed to try to get some sleep, and it suddenly clicked me that we were just supposed to be doing that first overload resolution with the expression as an xvalue and that is it. There was no reason to be anything else in there.
I just could not resist the urge to get up and fix it thought :)
I'll leave the test case for tomorrow though!

mizvekov updated this revision to Diff 354773.Sun, Jun 27, 2:32 PM

This introduces two new test cases that cover the bug fixed by the previous diff:

  • The one suggested by rsmith in the comments above.
  • Another one that shows that when both const and non-const lvref conversion operators where available, we would not pick the const one on the first overload resolution, and end up picking the non-const one on the second.

Trying to think of explanations of how we got this bug in the first place,
I think it boils down to the fact that the first overload resolution was
jerry-rigged into (trying to, unsuccessfully) bailing out in cases where it
would have succceeded without the xvalueness of the expression mattering, and
so the second overload would have hopefully done the same operation, without
affecting the meaning of the program.

The reason for this was diagnostics, it was necessary to get information on whether
casting the expression to rvref would have helped in order to implement the std-move
suggestions. As specified in the standard, the first overload resolution would often
just succeed where it would not have made a difference if the cast was there or not.

mizvekov marked 2 inline comments as done.Sun, Jun 27, 2:34 PM
mizvekov added inline comments.
clang/lib/Sema/SemaStmt.cpp
3544

If there was a IsDiagnosticsCheck && check here just as there is above for the constructor case, this bug could have been confined to a diagnostic issue only.

mizvekov edited the summary of this revision. (Show Details)Sun, Jun 27, 3:03 PM
rsmith added inline comments.Wed, Jun 30, 2:22 PM
clang/test/CXX/class/class.init/class.copy.elision/p3.cpp
435

The "{{is ambiguous}}" here is not part of the expected-error and will be ignored. If you want to ignore the text in between those two strings, you can do that with // cxx98-error-re {{conversion {{.*}} is ambiguous}} or similar. (The -re enables regular expression checking, and the nested {{...}} switches to regular expression matching rather than literal matching.)

mizvekov updated this revision to Diff 355714.Wed, Jun 30, 4:01 PM
  • fix test diagnostic expectation using regular expressions.
mizvekov marked an inline comment as done.Thu, Jul 1, 3:12 AM
sbc100 added a subscriber: sbc100.Wed, Jul 7, 3:57 PM

This commit seems to have broken libc++ in C++98 mode, as it appears to have depended upon the implicit-move extension.

Reproduction is simple. Build this with -stdlib=libc++ -std=c++98:

#include <set>
void foo (std::set<int> *s) {
   s->insert(5);
}

(https://godbolt.org/z/vKWKhE34x)

The root cause appears to be that libc++ emulates unique_ptr in c++98 mode, and this emulation stopped working. E.g. this no longer works (compiled with same flags):

#include <memory>
std::unique_ptr<int> foo() {
  std::unique_ptr<int> x;
  return x;
}

To simplify further, removing the stdlib headers: returning a type with a move constructor but no copy constructor used to work, and now doesn't (with -std=c++98).

struct uncopyable {
  uncopyable() = default;
  uncopyable(uncopyable const&) = delete;
  uncopyable(uncopyable&&) = default;
};
uncopyable bar() {
  uncopyable x;
  return x;
}

Now, of course, the above is invalid c++98, because rvalue references aren't even a thing there...but clang _does_ implement rvalue references in c++98 mode, and libc++ depends on it.

So -- I think either libc++ needs to stop depending on this nonstandard functionality, or we need to keep implementing it in Clang. I don't know which option would be more desirable.

CC+=ldionne, for libc++ expertise.

This commit seems to have broken libc++ in C++98 mode, as it appears to have depended upon the implicit-move extension.

The root cause appears to be that libc++ emulates unique_ptr in c++98 mode, and this emulation stopped working.

Thank you for the report!

It's not that hard, right now, to put that support back in.
But with C++11 up to C++20 sharing the same code, this is a lot of burden to have yet another implicit move mechanism just for C++98.

AFAIK, rvalue references in C++98 is a clang-only thing, none of the other C++ compilers implement this extension.
And this was also never officially supported, it is a best effort kind of deal.

And I think libc++ is in the process of ditching support for C++98 completely, though I might be mistaken about the extent here.

This commit seems to have broken libc++ in C++98 mode, as it appears to have depended upon the implicit-move extension.
The root cause appears to be that libc++ emulates unique_ptr in c++98 mode, and this emulation stopped working.

It's not that hard, right now, to put that support back in.
But with C++11 up to C++20 sharing the same code, this is a lot of burden to have yet another implicit move mechanism just for C++98.

@mizvekov, I don't understand what you mean by "yet another" mechanism. This is just asking to restore Clang's C++98/03 extension (that supports move constructors even in C++03). It's asking to remove special-case code you added, right? Find the line where you're saying "oh but if it's C++03 then don't do the usual rvalue lookup," and remove that.

My naive guess is line 3459:

if (getLangOpts().CPlusPlus11 && !getLangOpts().CPlusPlus2b &&
    NRInfo.isMoveEligible()) {

should maybe be just

if (!getLangOpts().CPlusPlus2b && NRInfo.isMoveEligible()) {

Could you look into that?

And I think libc++ is in the process of ditching support for C++98 completely, though I might be mistaken about the extent here.

Nope, untrue. I've been making some patches to ditch libc++'s _LIBCPP_CXX03_LANG macro, but I'm doing that precisely because the only compiler we support in C++03 mode is Clang, and on Clang we can actually use rvalue references and variadic templates and so on, even in C++03 mode. That is, this extension (that was broken by this patch) is actually the only reason libc++ works in C++03 mode at all! So it's important to preserve the extension. (And presumably there might be clients relying on the extension, besides just libc++.)

@mizvekov, I don't understand what you mean by "yet another" mechanism. This is just asking to restore Clang's C++98/03 extension (that supports move constructors even in C++03). It's asking to remove special-case code you added, right? Find the line where you're saying "oh but if it's C++03 then don't do the usual rvalue lookup," and remove that.

My naive guess is line 3459:

if (getLangOpts().CPlusPlus11 && !getLangOpts().CPlusPlus2b &&
    NRInfo.isMoveEligible()) {

should maybe be just

if (!getLangOpts().CPlusPlus2b && NRInfo.isMoveEligible()) {

Could you look into that?

With that change, you get the full P1825 implicit move in C++98, which modifies the semantics of pure C++98 programs.
Like I said in the commit message, this can change which copy constructor is picked, between non-const and const ones.

To continue supporting C++98, we would need to keep the old C++11 implicit move code around.

And I think libc++ is in the process of ditching support for C++98 completely, though I might be mistaken about the extent here.

Nope, untrue. I've been making some patches to ditch libc++'s _LIBCPP_CXX03_LANG macro, but I'm doing that precisely because the only compiler we support in C++03 mode is Clang, and on Clang we can actually use rvalue references and variadic templates and so on, even in C++03 mode. That is, this extension (that was broken by this patch) is actually the only reason libc++ works in C++03 mode at all! So it's important to preserve the extension. (And presumably there might be clients relying on the extension, besides just libc++.)

Well I thought that meant exactly that libc++ does not support C++98, it only works on clang because it provides so much of C++11 as an extension.

I did not remove this just on my own whim by the way, the information I had and the reason the patch was approved as is, is because as I said, those extensions were considered best effort and not officially supported.
@rsmith thoughts?

rsmith added a comment.Thu, Jul 8, 5:44 PM

@mizvekov, I don't understand what you mean by "yet another" mechanism. This is just asking to restore Clang's C++98/03 extension (that supports move constructors even in C++03). It's asking to remove special-case code you added, right? Find the line where you're saying "oh but if it's C++03 then don't do the usual rvalue lookup," and remove that.

My naive guess is line 3459:

if (getLangOpts().CPlusPlus11 && !getLangOpts().CPlusPlus2b &&
    NRInfo.isMoveEligible()) {

should maybe be just

if (!getLangOpts().CPlusPlus2b && NRInfo.isMoveEligible()) {

Could you look into that?

With that change, you get the full P1825 implicit move in C++98, which modifies the semantics of pure C++98 programs.
Like I said in the commit message, this can change which copy constructor is picked, between non-const and const ones.

To continue supporting C++98, we would need to keep the old C++11 implicit move code around.

And I think libc++ is in the process of ditching support for C++98 completely, though I might be mistaken about the extent here.

Nope, untrue. I've been making some patches to ditch libc++'s _LIBCPP_CXX03_LANG macro, but I'm doing that precisely because the only compiler we support in C++03 mode is Clang, and on Clang we can actually use rvalue references and variadic templates and so on, even in C++03 mode. That is, this extension (that was broken by this patch) is actually the only reason libc++ works in C++03 mode at all! So it's important to preserve the extension. (And presumably there might be clients relying on the extension, besides just libc++.)

Well I thought that meant exactly that libc++ does not support C++98, it only works on clang because it provides so much of C++11 as an extension.

I did not remove this just on my own whim by the way, the information I had and the reason the patch was approved as is, is because as I said, those extensions were considered best effort and not officially supported.
@rsmith thoughts?

I wonder if we can restore the extension behavior for the types that want it, while still using the C++98 rules for the rest of the cases? Perhaps we could use the new C++11-to-C++20 logic even in C++98 if the return type declares a move constructor?

I wonder if we can restore the extension behavior for the types that want it, while still using the C++98 rules for the rest of the cases? Perhaps we could use the new C++11-to-C++20 logic even in C++98 if the return type declares a move constructor?

Well if we want to keep supporting it, then my plan would be:

It comes back, but it's not exactly the same "C++98 implicit moves" that you used to know, it now has a few cybernetic enhancements:

  • As far as the rules of what id-expressions are considered move eligible, we keep the same ones as C++20 has. The old one used to apply only to the subset where we can do copy elision.
  • We jerry rig the first overload resolution, by iterating the construction steps and aborting it in case we pick a constructor or conversion operator which does not take an rvalue reference.

I think this way would be a balance where we don't add back too much code, but at the same time we keep compatibility with pure C++98 programs. I think this is in line with the idea of best effort.

Though if I may suggest, if we ever hope to sunset C++98, then to stop providing these C++11 extensions would be a good first step...

Well I thought that meant exactly that libc++ does not support C++98, it only works on clang because it provides so much of C++11 as an extension.

I did not remove this just on my own whim by the way, the information I had and the reason the patch was approved as is, is because as I said, those extensions were considered best effort and not officially supported.
@rsmith thoughts?

I don't know what the state of those extensions is from Clang's perspective, however one thing is clear - we use those extensions in libc++ very heavily. If any such extension is removed, libc++ will stop working in C++03 mode, which effectively means that most of the code compiled with Clang is going to break. That's a pretty serious problem.

Though if I may suggest, if we ever hope to sunset C++98, then to stop providing these C++11 extensions would be a good first step...

Yes, I agree those extensions are terrible. However, libc++ has supported C++11 library features with an approximative C++03 language mode ever since it was created. Removing support for something as fundamental as std::unique_ptr in C++03 mode is likely going to break a huge number of things, so we can't really do that. If there is a desire to sunset C++03, I could get behind that, but it would be an effort on its own and I don't think reducing what C++11 features we implement in C++03 mode would be part of that.

In the summary, you say:

Note that we also remove implicit moves from C++98 as an extension altogether, since the expanded first overload resolution from P1825 can cause some meaning changes in C++98. For example it can change which copy constructor is picked when both const and non-const ones are available.

So basically, you decided to downright remove the extension because it caused some code to change meaning? However, in doing so, all the code that was previously moving implicitly will now perform a copy instead (unless the copy constructor is deleted, in which case the code breaks, as in libc++). That's a pretty big behavior change. Is that accurate, or am I misunderstanding something?

mizvekov added a comment.EditedSat, Jul 10, 2:40 PM

I don't know what the state of those extensions is from Clang's perspective, however one thing is clear - we use those extensions in libc++ very heavily. If any such extension is removed, libc++ will stop working in C++03 mode, which effectively means that most of the code compiled with Clang is going to break. That's a pretty serious problem.

That was not clear from the clang side, we had those extensions as non officially supported.
Now since another project under the same umbrella depends on it, it makes sense to continue supporting this, at least until we agree on something else.

So basically, you decided to downright remove the extension because it caused some code to change meaning? However, in doing so, all the code that was previously moving implicitly will now perform a copy instead (unless the copy constructor is deleted, in which case the code breaks, as in libc++). That's a pretty big behavior change. Is that accurate, or am I misunderstanding something?

Well, considering from my perspective, which had those extensions as not officially supported and "best effort" only, it would make sense not to provide an extension which can break pure C++98 programs.
The options where:

  1. DR P1825 down to C++11 as the comittee decided, but get almost no benefit from implementation complexity because we would still have to keep most of the code around, but this time only for C++98 (which is not even supposed to have it).
  2. Leave C++98 without implicit moves. This gives following benefits:
    • Drop huge amount of code, almost no new code.
    • Pure C++98 programs don't get broken.
    • Less work, I can spend more time doing other things.
  3. DR P1825 down to C++98. This gives almost all the benefits from the option above, except that has risk of breaking valid C++98 code.
  4. Invent yet another implicit move rule.

We tried 2, and failed here.
Number 3 would be cool if we could get away with it, but I don't know if we should take the chances.
So I present my take on option 4: https://reviews.llvm.org/D105756