Author: ericwf Date: Wed May 24 21:16:53 2017 New Revision: 303831 URL: http://llvm.org/viewvc/llvm-project?rev=303831&view=rev Log: [coroutines] Fix fallthrough diagnostics for coroutines
Summary: This patch fixes a number of issues with the analysis warnings emitted when a coroutine may reach the end of the function w/o returning. * Fix bug where coroutines with `return_value` are incorrectly diagnosed as missing `co_return`'s. * Rework diagnostic message to no longer say "non-void coroutine", because that implies the coroutine doesn't have a void return type, which it might. In this case a non-void coroutine is one who's promise type does not contain `return_void()` As a side-effect of this patch, coroutine bodies that contain an invalid coroutine promise objects are marked as invalid. Reviewers: GorNishanov, rsmith, aaron.ballman, majnemer Reviewed By: GorNishanov Subscribers: cfe-commits Differential Revision: https://reviews.llvm.org/D33532 Modified: cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td cfe/trunk/include/clang/Sema/ScopeInfo.h cfe/trunk/lib/Analysis/AnalysisDeclContext.cpp cfe/trunk/lib/Sema/AnalysisBasedWarnings.cpp cfe/trunk/lib/Sema/SemaCoroutine.cpp cfe/trunk/lib/Sema/SemaDecl.cpp cfe/trunk/test/SemaCXX/coreturn.cpp cfe/trunk/test/SemaCXX/coroutines.cpp Modified: cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td?rev=303831&r1=303830&r2=303831&view=diff ============================================================================== --- cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td (original) +++ cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td Wed May 24 21:16:53 2017 @@ -537,10 +537,10 @@ def err_maybe_falloff_nonvoid_block : Er def err_falloff_nonvoid_block : Error< "control reaches end of non-void block">; def warn_maybe_falloff_nonvoid_coroutine : Warning< - "control may reach end of non-void coroutine">, + "control may reach end of coroutine; which is undefined behavior because the promise type %0 does not declare 'return_void()'">, InGroup<ReturnType>; def warn_falloff_nonvoid_coroutine : Warning< - "control reaches end of non-void coroutine">, + "control reaches end of coroutine; which is undefined behavior because the promise type %0 does not declare 'return_void()'">, InGroup<ReturnType>; def warn_suggest_noreturn_function : Warning< "%select{function|method}0 %1 could be declared with attribute 'noreturn'">, Modified: cfe/trunk/include/clang/Sema/ScopeInfo.h URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Sema/ScopeInfo.h?rev=303831&r1=303830&r2=303831&view=diff ============================================================================== --- cfe/trunk/include/clang/Sema/ScopeInfo.h (original) +++ cfe/trunk/include/clang/Sema/ScopeInfo.h Wed May 24 21:16:53 2017 @@ -388,6 +388,8 @@ public: (HasBranchProtectedScope && HasBranchIntoScope)); } + bool isCoroutine() const { return !FirstCoroutineStmtLoc.isInvalid(); } + void setFirstCoroutineStmt(SourceLocation Loc, StringRef Keyword) { assert(FirstCoroutineStmtLoc.isInvalid() && "first coroutine statement location already set"); Modified: cfe/trunk/lib/Analysis/AnalysisDeclContext.cpp URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Analysis/AnalysisDeclContext.cpp?rev=303831&r1=303830&r2=303831&view=diff ============================================================================== --- cfe/trunk/lib/Analysis/AnalysisDeclContext.cpp (original) +++ cfe/trunk/lib/Analysis/AnalysisDeclContext.cpp Wed May 24 21:16:53 2017 @@ -92,6 +92,8 @@ Stmt *AnalysisDeclContext::getBody(bool IsAutosynthesized = false; if (const FunctionDecl *FD = dyn_cast<FunctionDecl>(D)) { Stmt *Body = FD->getBody(); + if (auto *CoroBody = dyn_cast_or_null<CoroutineBodyStmt>(Body)) + Body = CoroBody->getBody(); if (Manager && Manager->synthesizeBodies()) { Stmt *SynthesizedBody = getBodyFarm(getASTContext(), Manager->Injector.get()).getBody(FD); Modified: cfe/trunk/lib/Sema/AnalysisBasedWarnings.cpp URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Sema/AnalysisBasedWarnings.cpp?rev=303831&r1=303830&r2=303831&view=diff ============================================================================== --- cfe/trunk/lib/Sema/AnalysisBasedWarnings.cpp (original) +++ cfe/trunk/lib/Sema/AnalysisBasedWarnings.cpp Wed May 24 21:16:53 2017 @@ -334,10 +334,6 @@ static ControlFlowKind CheckFallThrough( bool HasPlainEdge = false; bool HasAbnormalEdge = false; - // In a coroutine, only co_return statements count as normal returns. Remember - // if we are processing a coroutine or not. - const bool IsCoroutine = isa<CoroutineBodyStmt>(AC.getBody()); - // Ignore default cases that aren't likely to be reachable because all // enums in a switch(X) have explicit case statements. CFGBlock::FilterOptions FO; @@ -379,7 +375,7 @@ static ControlFlowKind CheckFallThrough( CFGStmt CS = ri->castAs<CFGStmt>(); const Stmt *S = CS.getStmt(); - if ((isa<ReturnStmt>(S) && !IsCoroutine) || isa<CoreturnStmt>(S)) { + if (isa<ReturnStmt>(S) || isa<CoreturnStmt>(S)) { HasLiveReturn = true; continue; } @@ -546,6 +542,7 @@ static void CheckFallThroughForBody(Sema bool ReturnsVoid = false; bool HasNoReturn = false; + bool IsCoroutine = S.getCurFunction() && S.getCurFunction()->isCoroutine(); if (const auto *FD = dyn_cast<FunctionDecl>(D)) { if (const auto *CBody = dyn_cast<CoroutineBodyStmt>(Body)) @@ -574,8 +571,13 @@ static void CheckFallThroughForBody(Sema // Short circuit for compilation speed. if (CD.checkDiagnostics(Diags, ReturnsVoid, HasNoReturn)) return; - SourceLocation LBrace = Body->getLocStart(), RBrace = Body->getLocEnd(); + auto EmitDiag = [&](SourceLocation Loc, unsigned DiagID) { + if (IsCoroutine) + S.Diag(Loc, DiagID) << S.getCurFunction()->CoroutinePromise->getType(); + else + S.Diag(Loc, DiagID); + }; // Either in a function body compound statement, or a function-try-block. switch (CheckFallThrough(AC)) { case UnknownFallThrough: @@ -583,15 +585,15 @@ static void CheckFallThroughForBody(Sema case MaybeFallThrough: if (HasNoReturn) - S.Diag(RBrace, CD.diag_MaybeFallThrough_HasNoReturn); + EmitDiag(RBrace, CD.diag_MaybeFallThrough_HasNoReturn); else if (!ReturnsVoid) - S.Diag(RBrace, CD.diag_MaybeFallThrough_ReturnsNonVoid); + EmitDiag(RBrace, CD.diag_MaybeFallThrough_ReturnsNonVoid); break; case AlwaysFallThrough: if (HasNoReturn) - S.Diag(RBrace, CD.diag_AlwaysFallThrough_HasNoReturn); + EmitDiag(RBrace, CD.diag_AlwaysFallThrough_HasNoReturn); else if (!ReturnsVoid) - S.Diag(RBrace, CD.diag_AlwaysFallThrough_ReturnsNonVoid); + EmitDiag(RBrace, CD.diag_AlwaysFallThrough_ReturnsNonVoid); break; case NeverFallThroughOrReturn: if (ReturnsVoid && !HasNoReturn && CD.diag_NeverFallThroughOrReturn) { @@ -2031,12 +2033,6 @@ AnalysisBasedWarnings::IssueWarnings(sem // Warning: check missing 'return' if (P.enableCheckFallThrough) { - auto IsCoro = [&]() { - if (auto *FD = dyn_cast<FunctionDecl>(D)) - if (FD->getBody() && isa<CoroutineBodyStmt>(FD->getBody())) - return true; - return false; - }; const CheckFallThroughDiagnostics &CD = (isa<BlockDecl>(D) ? CheckFallThroughDiagnostics::MakeForBlock() @@ -2044,7 +2040,7 @@ AnalysisBasedWarnings::IssueWarnings(sem cast<CXXMethodDecl>(D)->getOverloadedOperator() == OO_Call && cast<CXXMethodDecl>(D)->getParent()->isLambda()) ? CheckFallThroughDiagnostics::MakeForLambda() - : (IsCoro() + : (fscope->isCoroutine() ? CheckFallThroughDiagnostics::MakeForCoroutine(D) : CheckFallThroughDiagnostics::MakeForFunction(D))); CheckFallThroughForBody(S, D, Body, blkExpr, CD, AC); Modified: cfe/trunk/lib/Sema/SemaCoroutine.cpp URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Sema/SemaCoroutine.cpp?rev=303831&r1=303830&r2=303831&view=diff ============================================================================== --- cfe/trunk/lib/Sema/SemaCoroutine.cpp (original) +++ cfe/trunk/lib/Sema/SemaCoroutine.cpp Wed May 24 21:16:53 2017 @@ -719,13 +719,16 @@ static FunctionDecl *findDeleteForPromis void Sema::CheckCompletedCoroutineBody(FunctionDecl *FD, Stmt *&Body) { FunctionScopeInfo *Fn = getCurFunction(); - assert(Fn && Fn->CoroutinePromise && "not a coroutine"); - + assert(Fn && Fn->isCoroutine() && "not a coroutine"); if (!Body) { assert(FD->isInvalidDecl() && "a null body is only allowed for invalid declarations"); return; } + // We have a function that uses coroutine keywords, but we failed to build + // the promise type. + if (!Fn->CoroutinePromise) + return FD->setInvalidDecl(); if (isa<CoroutineBodyStmt>(Body)) { // Nothing todo. the body is already a transformed coroutine body statement. Modified: cfe/trunk/lib/Sema/SemaDecl.cpp URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Sema/SemaDecl.cpp?rev=303831&r1=303830&r2=303831&view=diff ============================================================================== --- cfe/trunk/lib/Sema/SemaDecl.cpp (original) +++ cfe/trunk/lib/Sema/SemaDecl.cpp Wed May 24 21:16:53 2017 @@ -12179,7 +12179,7 @@ Decl *Sema::ActOnFinishFunctionBody(Decl sema::AnalysisBasedWarnings::Policy WP = AnalysisWarnings.getDefaultPolicy(); sema::AnalysisBasedWarnings::Policy *ActivePolicy = nullptr; - if (getLangOpts().CoroutinesTS && getCurFunction()->CoroutinePromise) + if (getLangOpts().CoroutinesTS && getCurFunction()->isCoroutine()) CheckCompletedCoroutineBody(FD, Body); if (FD) { Modified: cfe/trunk/test/SemaCXX/coreturn.cpp URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/test/SemaCXX/coreturn.cpp?rev=303831&r1=303830&r2=303831&view=diff ============================================================================== --- cfe/trunk/test/SemaCXX/coreturn.cpp (original) +++ cfe/trunk/test/SemaCXX/coreturn.cpp Wed May 24 21:16:53 2017 @@ -18,6 +18,43 @@ struct promise_void { void unhandled_exception(); }; +struct promise_void_return_value { + void get_return_object(); + suspend_always initial_suspend(); + suspend_always final_suspend(); + void unhandled_exception(); + void return_value(int); +}; + +struct VoidTagNoReturn { + struct promise_type { + VoidTagNoReturn get_return_object(); + suspend_always initial_suspend(); + suspend_always final_suspend(); + void unhandled_exception(); + }; +}; + +struct VoidTagReturnValue { + struct promise_type { + VoidTagReturnValue get_return_object(); + suspend_always initial_suspend(); + suspend_always final_suspend(); + void unhandled_exception(); + void return_value(int); + }; +}; + +struct VoidTagReturnVoid { + struct promise_type { + VoidTagReturnVoid get_return_object(); + suspend_always initial_suspend(); + suspend_always final_suspend(); + void unhandled_exception(); + void return_void(); + }; +}; + struct promise_float { float get_return_object(); suspend_always initial_suspend(); @@ -34,8 +71,11 @@ struct promise_int { void unhandled_exception(); }; -template <typename... T> -struct std::experimental::coroutine_traits<void, T...> { using promise_type = promise_void; }; +template <> +struct std::experimental::coroutine_traits<void> { using promise_type = promise_void; }; + +template <typename T1> +struct std::experimental::coroutine_traits<void, T1> { using promise_type = promise_void_return_value; }; template <typename... T> struct std::experimental::coroutine_traits<float, T...> { using promise_type = promise_float; }; @@ -48,10 +88,66 @@ float test1() { co_await a; } int test2() { co_await a; -} // expected-warning {{control reaches end of non-void coroutine}} +} // expected-warning {{control reaches end of coroutine; which is undefined behavior because the promise type 'std::experimental::coroutine_traits<int>::promise_type' (aka 'promise_int') does not declare 'return_void()'}} + +int test2a(bool b) { + if (b) + co_return 42; +} // expected-warning {{control may reach end of coroutine; which is undefined behavior because the promise type 'std::experimental::coroutine_traits<int, bool>::promise_type' (aka 'promise_int') does not declare 'return_void()'}} int test3() { co_await a; b: goto b; } + +int test4() { + co_return 42; +} + +void test5(int) { + co_await a; +} // expected-warning {{control reaches end of coroutine; which is undefined behavior because}} + +void test6(int x) { + if (x) + co_return 42; +} // expected-warning {{control may reach end of coroutine; which is undefined behavior because}} + +void test7(int y) { + if (y) + co_return 42; + else + co_return 101; +} + +VoidTagReturnVoid test8() { + co_await a; +} + +VoidTagReturnVoid test9(bool b) { + if (b) + co_return; +} + +VoidTagReturnValue test10() { + co_await a; +} // expected-warning {{control reaches end of coroutine}} + +VoidTagReturnValue test11(bool b) { + if (b) + co_return 42; +} // expected-warning {{control may reach end of coroutine}} + +// FIXME: Promise types that declare neither return_value nor return_void +// should be ill-formed. This test should be removed when that is fixed. +VoidTagNoReturn test12() { + co_await a; +} // expected-warning {{control reaches end of coroutine}} + +// FIXME: Promise types that declare neither return_value nor return_void +// should be ill-formed. This test should be removed when that is fixed. +VoidTagNoReturn test13(bool b) { + if (b) + co_await a; +} // expected-warning {{control reaches end of coroutine}} Modified: cfe/trunk/test/SemaCXX/coroutines.cpp URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/test/SemaCXX/coroutines.cpp?rev=303831&r1=303830&r2=303831&view=diff ============================================================================== --- cfe/trunk/test/SemaCXX/coroutines.cpp (original) +++ cfe/trunk/test/SemaCXX/coroutines.cpp Wed May 24 21:16:53 2017 @@ -818,3 +818,14 @@ extern "C" int f(mismatch_gro_type_tag4) co_return; //expected-note {{function is a coroutine due to use of 'co_return' here}} } +struct bad_promise_no_return_func { + coro<bad_promise_no_return_func> get_return_object(); + suspend_always initial_suspend(); + suspend_always final_suspend(); + void unhandled_exception(); +}; +// FIXME: Make this ill-formed because the promise type declares +// neither return_value(...) or return_void(). +coro<bad_promise_no_return_func> no_return_value_or_return_void() { + co_await a; +} _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
