rjmccall added inline comments.
================ 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. + // ---------------- 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. ================ Comment at: clang/lib/CodeGen/CGCoroutine.cpp:146 +// We can't use (CoroutineSuspendExpr).getCommonExpr()->getType() directly +// since its type may be AutoType, ElaboratedType, ... +class AwaiterTypeFinder : public TypeVisitor<AwaiterTypeFinder> { ---------------- Please just do `getNonReferenceType()->getAsCXXRecordDecl()` and conservatively say it might escape if that's null. And you really don't need to repeat the well-formedness checks about the awaiter type that Sema is presumably already going to have done; that's just asking for extra work if this code ever diverges from Sema, e.g. if the language standard changes. So you should be able to do all of this without a visitor. ================ 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 ---------------- 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`. ================ Comment at: clang/lib/CodeGen/CodeGenFunction.h:351 + bool maySuspendLeakCoroutineHandle() const { + return isCoroutine() && CurCoro.MaySuspendLeak; ---------------- (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. 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