rjmccall added inline comments.
================ Comment at: clang/docs/ReleaseNotes.rst:143 + after leaking the coroutine handle in the await_suspend may be converted to + unconditional access incorrectly. + (`#56301 <https://github.com/llvm/llvm-project/issues/56301>`_) ---------------- Suggestion: ``` - Fixed an issue where accesses to the local variables of a coroutine during ``await_suspend`` could be misoptimized, including accesses to the awaiter object itself. (`#56301 <https://github.com/llvm/llvm-project/issues/56301>`_) ``` ================ Comment at: clang/lib/CodeGen/CGCoroutine.cpp:159 + + // Return false conservatively even if the underlying type is a record type. + if (!Awaiter) ---------------- ================ Comment at: clang/lib/CodeGen/CGCoroutine.cpp:169 + // functions. + return !Awaiter->field_empty(); +} ---------------- Is it possible for the awaiter type to be incomplete here? That shouldn't be possible if the awaiter object is returned as an r-value, but as I understand it, you can also return a reference to an object, which would normally allow the type of that object to be incomplete. But maybe that's disallowed due to higher-level rules with coroutines. ================ Comment at: clang/lib/CodeGen/CodeGenFunction.h:351 + bool maySuspendLeakCoroutineHandle() const { + return isCoroutine() && CurCoro.MaySuspendLeak; ---------------- ChuanqiXu wrote: > rjmccall wrote: > > (1) I'd prefer that we use the term "escape" over "leak" here. > > (2) None of these bugs require us to escape the coroutine handle. > > > > Maybe `mustPreventInliningOfAwaitSuspend`? It's not really a general > > property. > Used the term `escaped` instead of `leak`. > > > (2) None of these bugs require us to escape the coroutine handle. > > I feel like from the perspective of coroutine itself, it looks like the > coroutine handle escapes from the coroutine by await_suspend. So I feel this > may not be a bad name. Also as the above comments shows, we'd like to improve > this in the middle end finally, so these names would be removed in the end of > the day too. Okay, that makes sense to me. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D157833/new/ https://reviews.llvm.org/D157833 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
