-
Notifications
You must be signed in to change notification settings - Fork 13.1k
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
[coroutines/c++] Return object is being prematurely converted. #56532
Comments
If someone needs a minimal reproducer I can provide one, but I'm hoping that it won't be necessary. |
I guess so. But I am not sure if it is incorrect. @dpacbach I don't understand how this is related to |
@tstellar Hi, may you love to create a new label like |
@ChuanqiXu9: Done, but you could add labels yourself. |
@ChuanqiXu9 Here is a demonstration of the change: https://godbolt.org/z/jfrqsKsnb There you can see clang trunk vs. clang 14 vs gcc 12.1. Clang 14 and gcc 12.1 agree in that they perform the implicit conversion after the Note that on the cppreference coroutines page it says this:
To me that implies that it should do the implicit conversion only when necessary (as late as possible), though I am not sure what the C++ standard says officially. At the very least it seems a bit suspicious that clang trunk disagrees with the other two major compilers. |
@ChuanqiXu9 Here is another library that will be broken by this change, as it relies on the deferred implicit conversion of the
|
Here is another issue from Lewis Baker (who I'd think is a reliable source on this) who seems to imply that the conversion should be deferred: |
I got it. It surprises me but it looks like the intentional behavior indeed. I just failed to find the corresponding wording. I would try to fix it after I understand how to handle it properly. |
@dpacbach from https://cplusplus.github.io/CWG/issues/2563.html, it looks like the current behavior of clang is fine. |
@ChuanqiXu9 Although I am not an expert on the standard, I think you may be correct that the current C++ standard does not mandate the deferred implicit conversion (someone more expert than me would need to comment on that). My guess is that people didn't really appreciate the use cases that deferred conversion enables when writing the original C++20 standard, but which have developed since. Since deferred conversion seems to be essential for a few common programming patterns that have emerged, I suspect someday that people will try to change the wording of the standard to require deferred conversion in the future, especially if the major compilers start to disagree about it. So if clang breaks that now, then it may be harder to fix in the future. Thanks for taking a look. |
My feeling is that is an undefined behavior now. So both behavior are fine. It might change according to the issue. The suggested change is not the final decision. Let's wait for the decision made. |
Question: is it possible to change your implementation of coroutine RVO so that it can defer conversion and still work correctly? I'd think that, even if the return type needs to be implicitly converted, you can still do RVO with the result even if you defer that conversion. If so, then that seems like it would be ideal. Even if the standard doesn't mandate it now, it does seem a bit worrying that clang now disagrees with the other major compilers (and past clang versions). I think that ignoring this may turn out to be a mistake. @lewissbaker Given lewissbaker/cppcoro#43 that you raised, would you have any comments or advice here? |
Oh, your words make sense. I close it too hurry. I would check if it is possible to do RVO with the deferred conversion. But it might be a relative long time since there are too many TODOs and the priority of the issue looks not high to me. |
A good option would be for compilers to agree on the behavior and for compilers and the community to standardize the agreed-on behavior. Up until recently, the major compilers - GCC, MSVC, and Clang - all agreed on the behavior. And the agreed-on behavior prior was the behavior with slightly more flexibility. It would have been convenient to merge a technical paper that resolves a technical ambiguity in the standard and which merely standardizes what all major compilers already do anyway. Clang regressing the order here creates the potential for the ambiguity to stay in the standard for the long term. Clang fixing the order here makes it easier to resolve the ambiguity in the standard, which seems to me to be the best option overall. Leaving this ambiguous in the standard and having the compilers choose different orders seems to me to be a bad option because it requires implementations of some coroutines to be sensitive to, and somehow magically to know, what the order is for any given platform - and to have two implementations to handle the two orders. This looks like it would negatively affects coroutine implementations for In particular, the coroutine implementation for Beyond the technical points, Clang having divergent behavior is a new feeling. I am used to MSVC having divergent behavior and Clang having non-divergent behavior - that is, I am used to having custom patches to handle ways in which MSVC has missing or surprising behavior that is unlike the behavior of other compilers, but I am not used to having to do that for Clang. Fixing this regression in Clang and fixing the ambiguity in the standard strikes me as a good option because it allows implementations of all coroutines to be able to work with only one order - the order which leads to the least complexity in relevant coroutine implementations. |
While we're talking about implementation divergence of With Clang/MSVC, it constructs the return value directly from the reference returned from For example: https://godbolt.org/z/vbcbofxvs |
@lewissbaker Thanks, though perhaps that should be raised as a new issue. I don't recall that ever causing trouble for the |
@yfeldblum thanks for the written-up. But I just saw @lewissbaker kicked off a discussion in wg21. So it may be better to wait for the result from the committee. |
Hey folks, the discussion in EWG reached consensus that the
@ChuanqiXu9 do you have plans to work on this soon? If you're not already planning to work on this I'm happy to do the leg work - we've been reverting D117087 internally for almost an year now, probably a good moment to improve this. From a first look at D117087, it's not clear to me yet:
Thanks! |
@bcardosolopes Hi, it is great to hear that you want to work on this. Although I planned to work it myself, I don't have a lot of time though.
Yes. Since now the GRO may be local variable inside the function (coroutine), we need to handle the cleanup of the GRO in case of exceptions. So we need to re-introduce GetReturnObjectManager.
I think the best approach is to revert D117087 and implement RVO for the case Thanks! |
Patches are up:
I've separated this into two to give our internal CIs time to digest this in pieces. We're current facing some crashes related to RVO with the upstream implementation, which (2) might solve. But by independently fixing (1) we detach correctness from optimization. |
Thanks @bcardosolopes, I tested the fix with my |
Fix llvm/llvm-project#56532 Effectively, this reverts behavior introduced in https://reviews.llvm.org/D117087, which did two things: 1. Change delayed to early conversion of return object. 2. Introduced RVO possibilities because of early conversion. This patches fixes (1) and removes (2). I already worked on a follow up for (2) in a separated patch. I believe it's important to split these two because if the RVO causes any problems we can explore reverting (2) while maintaining (1). Notes on some testcase changes: - `pr59221.cpp` changed to `-O1` so we can check that the front-end honors the value checked for. Sounds like `-O3` without RVO is more likely to work with LLVM optimizations... - Comment out delete members `coroutine-no-move-ctor.cpp` since behavior now requires copies again. Differential Revision: https://reviews.llvm.org/D145639
Note that https://reviews.llvm.org/D145639 just got reverted (see comments in the diff), will re-land it again next week, altogether with https://reviews.llvm.org/D145641. |
…tain conditions - The cwg2563 issue is fixed by delaying GRO initialization only when the types mismatch between GRO and function return. - When the types match directly initialize, which indirectly enables RVO to kick in, partially restores behavior introduced in https://reviews.llvm.org/D117087. - Add entry to release notes. Background: #56532 https://cplusplus.github.io/CWG/issues/2563.html cplusplus/papers#1414 Differential Revision: https://reviews.llvm.org/D145641
@bcardosolopes Thanks a lot! |
using llvm/clang main built on 2022-07-14.
There appears to be a regression in how coroutine return objects are implicitly converted prior to returning them.
First as background, the
get_return_object
customization point is allowed to return an object that can be implicitly converted to the real return type. This part is still working fine.The regression concerns when this implicit conversion happens.
Clang traditionally waited to perform this implicit conversion until after the
return_value
customization point was called. This allowed doing some tricks that are useful in implementing e.g. astd::optional
coroutine. So if you were to print out the order of calls, it would looks like this traditionally:get_return_object
return_value
This may have changed at some point in the last few months, but the version of clang that I built today does it in a different order:
get_return object
return_value
I'm not sure if the standard guarantees that it will happen in a certain order... but I know that traditionally it used to be done according to the first ordering (which gcc also does), but now that changed, and it is breaking my implementation of the
std::optional
coroutine.It seems sensible to wait as long as possible to perform the implicit conversion, since it affords flexibility to the user in that it permits the object that
get_return_object
returns to remain alive and accessible toreturn_value
(or other customization point methods) until the very end when it is implicitly converted just before ending the coroutine.The text was updated successfully, but these errors were encountered: