PR for llvm/llvm-project#59723 #637
Conversation
…k coro handle unconditionally any more Close llvm/llvm-project#59723. The fundamental cause of the above issue is that we assumed the memory of coroutine frame can be released by stack unwinding automatically if the allocation of the coroutine frame is elided. But we missed one point: the stack unwinding has different semantics with the explicit coroutine_handle<>::destroy(). Since the latter is explicit so it shows the intention of the user. So we can blame the user to destroy the coroutine frame incorrectly in case of use-after-free happens. But we can't do so with stack unwinding. So after this patch, we won't think the exceptional terminator don't leak the coroutine handle unconditionally. Instead, we think the exceptional terminator will leak the coroutine handle too if the coroutine is leaked somewhere along the search path. Concretely for C++, we can think the exceptional terminator is not special any more. Maybe this may cause some performance regressions. But I've tested the motivating example (std::generator). And on the other side, the coroutine elision is a middle end opitmization and not a language feature. So we don't think we should blame such regressions especially we are correcting the miscompilations. (cherry picked from commit 7037331a2f05990cd59f35a7c0f6ce87c0f3cb5f)
|
I also wanted to tag Erich, but I can't find his github username! |
|
Clang bits look good to me, but I think @ChuanqiXu9 should weigh in on the LLVM bits.
How strange, I can't either! I've messaged him to see if something is up with his account. Thanks for letting me know! |
|
Trying this out manually, CC @erichkeane |
|
As if by magic, I appear :) I'll take a look at this in a little while! |
|
Weird! That ping worked, and now Erich shows up when I @ him. As per usual, I have no clue what GitHub is doing or why. |
|
@tru : I don't spend much time in the optimizer/middle end transforms, so I don't have a good idea what is going on, though it all DOES seem reasonable. Curious what I can contribute otherwise? |
|
It looks OK to me to backport this since it is required by users and the risk is under control (the patch in fact blocks more optimizations). |
resolves llvm/llvm-project#59723