[coroutines] Remove assert on CoroutineParameterMoves in Sema::buildCoroutineParameterMoves

[junparser]

Summary

The assertion of CoroutineParameterMoves happens when build coroutine function with arguments multiple time while fails to build promise type.

Fix: use return false instead.

Test Plan: check-clang

[junparser]

gental ping~ @modocache @GorNishanov

[rjmccall]

Despite generally knowing about coroutines and generally knowing about Clang, I actually don't know the Clang coroutine code and can't review this properly.

[junparser]

In D69022#1720645, @rjmccall wrote:

Despite generally knowing about coroutines and generally knowing about Clang, I actually don't know the Clang coroutine code and can't review this properly.

thanks anyway~

[modocache]

Sorry for the slow response here, @junparser!

The test case you came up with here is great! I can see the issue is that ScopeInfo->CoroutineParameterMoves are built up when Clang parses the first co_await a, but are not cleared when lookupPromiseType results in an error. As a result, when Clang hits the second co_await a, it's in a state that the current code didn't anticipate. Your test case does a great job demonstrating this. Your fix for the problem also looks good to me! My only suggestion is to make the test case just a little clearer, as I'll explain...

(Also, in the future could you please upload your patches with full context? You can read https://llvm.org/docs/Phabricator.html for more details. I think the section explaining the web interface might be relevant to you, where it suggests git show HEAD -U999999 > mypatch.patch. The reason I ask is because on Phabricator I can see what lines you're proposing should be added, but not the surrounding source lines, which made this more difficult to review.)

test/SemaCXX/coroutines.cpp

93 ↗

(On Diff #225167)

This is a great test case, thanks for coming up with it! However, I think it could be a little clearer, though: right now the int a variable is shadowing the awaitable a that's declared further up in this file. At first, I wasn't sure whether the shadowing had something to do with the test, but in fact it doesn't. This test case demonstrates the issue but without the confusion, I think:

int no_promise_type_multiple_awaits(int) { // expected-error {{this function cannot be a coroutine: 'std::experimental::coroutine_traits<int, int>' has no member named 'promise_type'}}
  co_await a;
  co_await a;
}

[junparser]

update as @modocache's suggestion

[junparser]

In D69022#1755636, @modocache wrote:

Sorry for the slow response here, @junparser!

The test case you came up with here is great! I can see the issue is that ScopeInfo->CoroutineParameterMoves are built up when Clang parses the first co_await a, but are not cleared when lookupPromiseType results in an error. As a result, when Clang hits the second co_await a, it's in a state that the current code didn't anticipate. Your test case does a great job demonstrating this. Your fix for the problem also looks good to me! My only suggestion is to make the test case just a little clearer, as I'll explain...

(Also, in the future could you please upload your patches with full context? You can read https://llvm.org/docs/Phabricator.html for more details. I think the section explaining the web interface might be relevant to you, where it suggests git show HEAD -U999999 > mypatch.patch. The reason I ask is because on Phabricator I can see what lines you're proposing should be added, but not the surrounding source lines, which made this more difficult to review.)

Thanks so much for reviewing the patch and giving the helpful advise.

[modocache]

LGTM, thanks! Please let me know if you'd like me to commit this change.

[junparser]

In D69022#1756138, @modocache wrote:

LGTM, thanks! Please let me know if you'd like me to commit this change.

commit it please, thanks!

[modocache]

Thanks again for the patch @junparser! And sorry the review took so long!

clang/lib/Sema/SemaCoroutine.cpp

 
 // Build statements that move coroutine function parameters to the coroutine
 // frame, and store them on the function scope info.
 bool Sema::buildCoroutineParameterMoves(SourceLocation Loc) {
   assert(isa<FunctionDecl>(CurContext) && "not in a function scope");
   auto *FD = cast<FunctionDecl>(CurContext);
 
   auto *ScopeInfo = getCurFunction();
-  assert(ScopeInfo->CoroutineParameterMoves.empty() &&
-         "Should not build parameter moves twice");
+  if (!ScopeInfo->CoroutineParameterMoves.empty())
+    return false;
 
   for (auto *PD : FD->parameters()) {
     if (PD->getType()->isDependentType())
       continue;
 
     ExprResult PDRefExpr =
         BuildDeclRefExpr(PD, PD->getType().getNonReferenceType(),
                          ExprValueKind::VK_LValue, Loc); // FIXME: scope?

clang/test/SemaCXX/coroutines.cpp

 
 template <typename... T>
 struct std::experimental::coroutine_traits<int, T...> {};
 
 int no_promise_type() { // expected-error {{this function cannot be a coroutine: 'std::experimental::coroutine_traits<int>' has no member named 'promise_type'}}
   co_await a;
 }
 
+int no_promise_type_multiple_awaits(int) { // expected-error {{this function cannot be a coroutine: 'std::experimental::coroutine_traits<int, int>' has no member named 'promise_type'}}
+  co_await a;
+  co_await a;
+}
+
 template <>
 struct std::experimental::coroutine_traits<double, double> { typedef int promise_type; };
 double bad_promise_type(double) { // expected-error {{this function cannot be a coroutine: 'experimental::coroutine_traits<double, double>::promise_type' (aka 'int') is not a class}}
   co_await a;
 }
 
 template <>
 struct std::experimental::coroutine_traits<double, int> {