[PATCH] D157833: [C++20] [Coroutines] Mark await_suspend as noinline if the awaiter is not empty

[John McCall via Phabricator via cfe-commits]

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