ChuanqiXu marked 4 inline comments as done.
ChuanqiXu added a comment.
Address comments. Thanks for reviewing.
================
Comment at: clang/lib/CodeGen/CGCall.cpp:5496
+ // execution of the await_suspend. To achieve this, we need to prevent the
+ // await_suspend get inlined before CoroSplit pass.
+ //
----------------
rjmccall wrote:
> Suggestion:
>
> > The `await_suspend` call performed by `co_await` is essentially asynchronous
> > to the execution of the coroutine. Inlining it normally into an unsplit
> > coroutine
> > can cause miscompilation because the coroutine CFG misrepresents the true
> > control flow of the program: things that happen in the `await_suspend` are
> > not
> > guaranteed to happen prior to the resumption of the coroutine, and things
> > that
> > happen after the resumption of the coroutine (including its exit and the
> > potential deallocation of the coroutine frame) are not guaranteed to happen
> > only after the end of `await_suspend`.
> >
> > The short-term solution to this problem is to mark the call as uninlinable.
> > But we don't want to do this if the call is known to be trivial, which is
> > very
> > common.
>
> We don't need to get into the long-term solution here, but I'd suggest a
> pattern
> like:
>
> ```
> call @llvm.coro.await_suspend(token %suspend, ptr %awaiter, ptr
> @awaitSuspendFn)
> ```
>
> and then we just replace that with the normal call during function splitting.
> I guess we'd have to make sure we did everything right with the return
> values and exceptions, which might be a pain. But it does have the really
> nice property that we could move all of this "is it trivial" analysis into
> the LLVM passes: if we decide that it's safe to inline the function before
> splitting (e.g. `awaitSuspendFn` doesn't contain any uses of `this`), then
> the optimization is as simple as doing the replacement in CoroEarly instead
> of after splitting.
Got it. The proposed long term solution looks pretty good indeed.
================
Comment at: clang/lib/CodeGen/CGCall.cpp:5504
+ // pass.
+ if (inSuspendBlock() && maySuspendLeakCoroutineHandle())
+ Attrs = Attrs.addFnAttribute(getLLVMContext(), llvm::Attribute::NoInline);
----------------
nit: I thought to simplify this to `maySuspendLeakCoroutineHandle()`. But I
feel the current combination reads better. I read it as "we are in a suspend
block and it may leak the coroutine handle". I just feel it has better
readability.
================
Comment at: clang/lib/CodeGen/CGCoroutine.cpp:165
+ // to give users better user experience. It doesn't matter with the
+ // correctness but 1 byte memory overhead.
+#ifdef NDEBUG
----------------
rjmccall wrote:
> I'm sorry, but I'm really confused about what you're doing here. In general,
> we really don't want compiler behavior to change based on whether we're
> running a release version of the compiler. And this would be disabling the
> optimization completely in release builds?
>
> Are you maybe trying to check whether we're doing an unoptimized build of the
> *program*? That's `CodeGenOpts.OptimizationLevel == 0`.
> I'm sorry, but I'm really confused about what you're doing here. In general,
> we really don't want compiler behavior to change based on whether we're
> running a release version of the compiler. And this would be disabling the
> optimization completely in release builds?
I was trying to generate the better code at much as I can. But as your advice
shows, it is clearly much better to do the analysis in the middle end. So let's
try to improve it in the middle end and leave it as is in the frontend.
> Are you maybe trying to check whether we're doing an unoptimized build of the
> *program*? That's CodeGenOpts.OptimizationLevel == 0.
Given now it is pretty simple, I think we can omit the check.
================
Comment at: clang/lib/CodeGen/CodeGenFunction.h:351
+ bool maySuspendLeakCoroutineHandle() const {
+ return isCoroutine() && CurCoro.MaySuspendLeak;
----------------
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.
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
