[clang][PR55406] co_return CFG fix

[urnathan]

Summary

PR55406 is an issue where an incorrect CFG is constructed containing
duplicate references to subtrees. That's due to co_return keeping the
original expression and the promise call. It needs the former for (a)
instantiation and (b) co_returns with void expressions.

Add a flag to CoreturnStmt indicating whether the operand member is
needed for the void-return case. This simplifies code-generation but
also allows the CFG generator to look at that flag to determine
whether it needs to emit the operand in addition to the promise call.

[ChuanqiXu]

This one looks like a workaround for me. It tries to skip a CFG check for the operand of return_value to avoid a potential crash. The true solution should be to construct the CFG correctly.

[urnathan]

Please clarify. This is fixing a bug where the CFG was being incorrectly constructed.

[ChuanqiXu]

In D125541#3518571, @urnathan wrote:

Please clarify. This is fixing a bug where the CFG was being incorrectly constructed.

I thought we could avoid the crash by a simple fix (see the comment) in CFG.cpp instead of touching coroutine things. And today I've tried we could pass all the tests by checking the promise call only. But I just realize that we might miss a check for the expressions in return_void but it is not covered by tests. I think it makes sense to fix the crash first (don't add IsVoid in CoreturnStmt) and checking the expressions in later patches if it is needed. Since expressions generally wouldn't break the CFG and the CFG analysis for coroutines are not matured too.

clang/lib/Analysis/CFG.cpp

3110–3111

This is not true for co_return statement. According to http://eel.is/c++draft/stmt.return.coroutine, after co_return we need to execute codes in final suspend points. So the CFG analysis might not be accurate for coroutines. Maybe it is good to leave a FIXME here.

3142

Why we don't use Block here?

3143–3148

According to http://eel.is/c++draft/stmt.return.coroutine#2.2, the expression is sequenced before p.return_void(); Also, this is not covered by tests. We could pass the current test by write the following:

return Visit(CRS->getPromiseCall());

clang/include/clang/AST/StmtCXX.h

 ///   <promise>.return_void()
 /// which we name the "promise call".
 class CoreturnStmt : public Stmt {
   SourceLocation CoreturnLoc;
 
   enum SubStmt { Operand, PromiseCall, Count };
   Stmt *SubStmts[SubStmt::Count];
 
+  bool IsVoid : 1; // A void return with non-null operand.
   bool IsImplicit : 1;
 
   friend class ASTStmtReader;
 public:
   CoreturnStmt(SourceLocation CoreturnLoc, Stmt *Operand, Stmt *PromiseCall,
-               bool IsImplicit = false)
-      : Stmt(CoreturnStmtClass), CoreturnLoc(CoreturnLoc),
+               bool IsVoid, bool IsImplicit = false)
+      : Stmt(CoreturnStmtClass), CoreturnLoc(CoreturnLoc), IsVoid(IsVoid),
         IsImplicit(IsImplicit) {
     SubStmts[SubStmt::Operand] = Operand;
     SubStmts[SubStmt::PromiseCall] = PromiseCall;
   }
 
-  CoreturnStmt(EmptyShell) : CoreturnStmt({}, {}, {}) {}
+  CoreturnStmt(EmptyShell) : CoreturnStmt({}, {}, {}, {}) {}
 
   SourceLocation getKeywordLoc() const { return CoreturnLoc; }
 
   /// Retrieve the operand of the 'co_return' statement. Will be nullptr
   /// if none was specified.
   Expr *getOperand() const { return static_cast<Expr*>(SubStmts[Operand]); }
 
   /// Retrieve the promise call that results from this 'co_return'
-  /// statement. Will be nullptr if either the coroutine has not yet been
-  /// finalized or the coroutine has no eventual return type.
+  /// statement. Will encapsulate the operand, unless IsVoid is true.
   Expr *getPromiseCall() const {
     return static_cast<Expr*>(SubStmts[PromiseCall]);
   }
 
   bool isImplicit() const { return IsImplicit; }
   void setIsImplicit(bool value = true) { IsImplicit = value; }
 
+  bool getIsVoid() const { return IsVoid; }
+  void setIsVoid(bool value = true) { IsVoid = value; }
+
   SourceLocation getBeginLoc() const LLVM_READONLY { return CoreturnLoc; }
   SourceLocation getEndLoc() const LLVM_READONLY {
     return getOperand() ? getOperand()->getEndLoc() : getBeginLoc();
   }
 
   child_range children() {
     if (!getOperand())
       return child_range(SubStmts + SubStmt::PromiseCall,

clang/lib/Analysis/CFG.cpp

CFGBlock *CFGBuilder::VisitIfStmt(IfStmt *I) {
   }
 
   return LastBlock;
 }
 
 CFGBlock *CFGBuilder::VisitReturnStmt(Stmt *S) {
   // If we were in the middle of a block we stop processing that block.
   //
   // 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(), S);
CFGBlock *CFGBuilder::VisitReturnStmt(Stmt *S) {
   appendStmt(Block, S);
 
   // Visit children
   if (ReturnStmt *RS = dyn_cast<ReturnStmt>(S)) {
     if (Expr *O = RS->getRetValue())
       return Visit(O, AddStmtChoice::AlwaysAdd, /*ExternallyDestructed=*/true);
     return Block;
   }
-  // co_return
-  return VisitChildren(S);
+
+  CoreturnStmt *CRS = cast<CoreturnStmt>(S);
+  auto *B = Block;
+  if (CFGBlock *R = Visit(CRS->getPromiseCall()))
+    B = R;
+
+  if (CRS->getIsVoid())
+    if (CFGBlock *R = Visit(CRS->getOperand()))
+      B = R;
+
+  return B;
 }
 
 CFGBlock *CFGBuilder::VisitSEHExceptStmt(SEHExceptStmt *ES) {
   // SEHExceptStmt are treated like labels, so they are the first statement in a
   // block.
 
   // Save local scope position because in case of exception variable ScopePos
   // won't be restored when traversing AST.

clang/lib/CodeGen/CGCoroutine.cpp

RValue CodeGenFunction::EmitCoyieldExpr(const CoyieldExpr &E,
                                         AggValueSlot aggSlot,
                                         bool ignoreResult) {
   return emitSuspendExpression(*this, *CurCoro.Data, E, AwaitKind::Yield,
                                aggSlot, ignoreResult, /*forLValue*/false).RV;
 }
 
 void CodeGenFunction::EmitCoreturnStmt(CoreturnStmt const &S) {
   ++CurCoro.Data->CoreturnCount;
-  const Expr *RV = S.getOperand();
-  if (RV && RV->getType()->isVoidType() && !isa<InitListExpr>(RV)) {
-    // Make sure to evaluate the non initlist expression of a co_return
-    // with a void expression for side effects.
+  if (S.getIsVoid()) {
+    // Evaluate the void expression for side effects.
     RunCleanupsScope cleanupScope(*this);
-    EmitIgnoredExpr(RV);
+    EmitIgnoredExpr(S.getOperand());
   }
   EmitStmt(S.getPromiseCall());
   EmitBranchThroughCleanup(CurCoro.Data->FinalJD);
 }
 
 
 #ifndef NDEBUG
 static QualType getCoroutineSuspendExprReturnType(const ASTContext &Ctx,

clang/lib/Sema/SemaCoroutine.cpp

StmtResult Sema::BuildCoreturnStmt(SourceLocation Loc, Expr *E,
   if (E && E->hasPlaceholderType() &&
       !E->hasPlaceholderType(BuiltinType::Overload)) {
     ExprResult R = CheckPlaceholderExpr(E);
     if (R.isInvalid()) return StmtError();
     E = R.get();
   }
 
   Expr *PCE = nullptr;
+  bool IsVoid = false;
   VarDecl *Promise = FSI->CoroutinePromise;
   if ((E && E->getType()->isDependentType()) ||
       Promise->getType()->isDependentType()) {
     // It is dependent, do nothing until instantiation time.
   } else {
     ExprResult PC;
     if (E && (isa<InitListExpr>(E) || !E->getType()->isVoidType())) {
       getNamedReturnInfo(E, SimplerImplicitMoveMode::ForceOn);
       PC = buildPromiseCall(*this, Promise, Loc, "return_value", E);
     } else {
       E = MakeFullDiscardedValueExpr(E).get();
+      IsVoid = bool(E);
       PC = buildPromiseCall(*this, Promise, Loc, "return_void", None);
     }
     if (PC.isInvalid())
       return StmtError();
 
     PCE = ActOnFinishFullExpr(PC.get(), /*DiscardedValue*/ false).get();
   }
 
-  Stmt *Res = new (Context) CoreturnStmt(Loc, E, PCE, IsImplicit);
+  Stmt *Res = new (Context) CoreturnStmt(Loc, E, PCE, IsVoid, IsImplicit);
   return Res;
 }
 
 /// Look up the std::nothrow object.
 static Expr *buildStdNoThrowDeclRef(Sema &S, SourceLocation Loc) {
   NamespaceDecl *Std = S.getStdNamespace();
   assert(Std && "Should already be diagnosed");
 

clang/test/SemaCXX/PR55406.cpp

+// RUN: %clang_cc1 -fsyntax-only -verify -Wthread-safety -std=c++17 -fcoroutines-ts %s
+
+// expected-no-diagnostics
+
+namespace std {
+template <typename _Result, typename...>
+struct coroutine_traits {
+  using promise_type = typename _Result::promise_type;
+};
+
+template <typename _Promise = void>
+struct coroutine_handle;
+
+template <>
+struct coroutine_handle<void> {
+  static coroutine_handle from_address(void *__a) noexcept;
+  void resume() const noexcept;
+  void destroy() const noexcept;
+};
+
+template <typename _Promise>
+struct coroutine_handle : coroutine_handle<> {};
+
+struct suspend_always {
+  bool await_ready() const noexcept;
+  void await_suspend(coroutine_handle<>) const noexcept;
+  void await_resume() const noexcept;
+};
+} // namespace std
+
+class Task {
+public:
+  struct promise_type {
+  public:
+    std::suspend_always initial_suspend() noexcept;
+    std::suspend_always final_suspend() noexcept;
+
+    Task get_return_object() noexcept;
+    void unhandled_exception() noexcept;
+    void return_value(int value) noexcept;
+  };
+};
+
+Task Foo() noexcept {
+  // ICE'd
+  co_return({ int frame = 0; 0; });
+}