modocache created this revision.
modocache added reviewers: GorNishanov, tks2103, rsmith.
Herald added a subscriber: EricWF.
The test case added in this diff would incorrectly warn that control
flow may fall through without returning. Here's a standalone example:
https://godbolt.org/z/dCwXEi
The same program, but using `return` instead of `co_return`, does not
produce a warning: https://godbolt.org/z/mVldqQ
The issue was in how Clang analysis would structure its representation
of the control-flow graph. Specifically, when constructing the CFG,
`CFGBuilder::Visit` had special handling of a `ReturnStmt`, in which it
would place object destructors in the same CFG block as a `return` statement,
immediately after it. Doing so would allow the logic in
`lib/Sema/AnalysisBasedWarning.cpp` `CheckFallThrough` to work properly in the
program that used `return`, correctly determining that no "plain edges" preceded
the exit block of the function.
Because a `co_return` statement would not enjoy the same treatment when
it was being built into the control-flow graph, object destructors
would not be placed in the same CFG block as the `co_return`, thus
resulting in a "plain edge" preceding the exit block of the function,
and so the warning logic would be triggered.
Add special casing for `co_return` to Clang analysis, thereby
remedying the mistaken warning.
Test Plan: `check-clang`
Repository:
rC Clang
https://reviews.llvm.org/D54075
Files:
lib/Analysis/CFG.cpp
test/SemaCXX/coreturn-eh.cpp
Index: test/SemaCXX/coreturn-eh.cpp
===================================================================
--- /dev/null
+++ test/SemaCXX/coreturn-eh.cpp
@@ -0,0 +1,45 @@
+// RUN: %clang_cc1 -triple x86_64-apple-darwin9 %s -std=c++14 -fcoroutines-ts -fcxx-exceptions -fsyntax-only -Wignored-qualifiers -Wno-error=return-type -verify -fblocks -Wall -Wextra -Wno-error=unreachable-code
+// expected-no-diagnostics
+
+#include "Inputs/std-coroutine.h"
+
+using std::experimental::suspend_always;
+using std::experimental::suspend_never;
+
+struct awaitable {
+ bool await_ready();
+ void await_suspend(std::experimental::coroutine_handle<>); // FIXME: coroutine_handle
+ void await_resume();
+} a;
+
+struct object { ~object() {} };
+
+struct promise_void_return_value {
+ void get_return_object();
+ suspend_always initial_suspend();
+ suspend_always final_suspend();
+ void unhandled_exception();
+ void return_value(object);
+};
+
+struct VoidTagReturnValue {
+ struct promise_type {
+ VoidTagReturnValue get_return_object();
+ suspend_always initial_suspend();
+ suspend_always final_suspend();
+ void unhandled_exception();
+ void return_value(object);
+ };
+};
+
+template <typename T1>
+struct std::experimental::coroutine_traits<void, T1> { using promise_type = promise_void_return_value; };
+
+VoidTagReturnValue test() {
+ object x = {};
+ try {
+ co_return {};
+ } catch (...) {
+ throw;
+ }
+}
Index: lib/Analysis/CFG.cpp
===================================================================
--- lib/Analysis/CFG.cpp
+++ lib/Analysis/CFG.cpp
@@ -571,7 +571,7 @@
CFGBlock *VisitObjCForCollectionStmt(ObjCForCollectionStmt *S);
CFGBlock *VisitObjCMessageExpr(ObjCMessageExpr *E, AddStmtChoice asc);
CFGBlock *VisitPseudoObjectExpr(PseudoObjectExpr *E);
- CFGBlock *VisitReturnStmt(ReturnStmt *R);
+ CFGBlock *VisitReturnStmt(Stmt *S);
CFGBlock *VisitSEHExceptStmt(SEHExceptStmt *S);
CFGBlock *VisitSEHFinallyStmt(SEHFinallyStmt *S);
CFGBlock *VisitSEHLeaveStmt(SEHLeaveStmt *S);
@@ -2147,7 +2147,8 @@
return VisitPseudoObjectExpr(cast<PseudoObjectExpr>(S));
case Stmt::ReturnStmtClass:
- return VisitReturnStmt(cast<ReturnStmt>(S));
+ case Stmt::CoreturnStmtClass:
+ return VisitReturnStmt(S);
case Stmt::SEHExceptStmtClass:
return VisitSEHExceptStmt(cast<SEHExceptStmt>(S));
@@ -2877,31 +2878,33 @@
return LastBlock;
}
-CFGBlock *CFGBuilder::VisitReturnStmt(ReturnStmt *R) {
+CFGBlock *CFGBuilder::VisitReturnStmt(Stmt *S) {
// If we were in the middle of a block we stop processing that block.
//
- // NOTE: If a "return" appears in the middle of a block, this means that the
- // code afterwards is DEAD (unreachable). We still keep a basic block
- // for that code; a simple "mark-and-sweep" from the entry block will be
- // able to report such dead blocks.
+ // NOTE: If a "return" or "co_return" appears in the middle of a block, this
+ // means that the code afterwards is DEAD (unreachable). We still keep
+ // a basic block for that code; a simple "mark-and-sweep" from the entry
+ // block will be able to report such dead blocks.
+ assert(isa<ReturnStmt>(S) || isa<CoreturnStmt>(S));
// Create the new block.
Block = createBlock(false);
- addAutomaticObjHandling(ScopePos, LocalScope::const_iterator(), R);
+ addAutomaticObjHandling(ScopePos, LocalScope::const_iterator(), S);
- findConstructionContexts(
- ConstructionContextLayer::create(cfg->getBumpVectorContext(), R),
- R->getRetValue());
+ if (auto *R = dyn_cast<ReturnStmt>(S))
+ findConstructionContexts(
+ ConstructionContextLayer::create(cfg->getBumpVectorContext(), R),
+ R->getRetValue());
// If the one of the destructors does not return, we already have the Exit
// block as a successor.
if (!Block->hasNoReturnElement())
addSuccessor(Block, &cfg->getExit());
// Add the return statement to the block. This may create new blocks if R
// contains control-flow (short-circuit operations).
- return VisitStmt(R, AddStmtChoice::AlwaysAdd);
+ return VisitStmt(S, AddStmtChoice::AlwaysAdd);
}
CFGBlock *CFGBuilder::VisitSEHExceptStmt(SEHExceptStmt *ES) {
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
