[clang] [llvm] [Coroutines] Mark parameter allocas with coro.outside.frame metadata (PR #127653)

[Chuanqi Xu via cfe-commits]

ChuanqiXu9 wrote:

> > In the issue (#127499) you pointed out that this issue applies to the MSVC ABI where all parameters are destroyed in the callee. Is it reasonable to construct the analogous test case in that environment?
> 
> Yes, in fact the reproducer on that issue will trigger use-after-free on x86-64 Windows also without the `trivial_abi` attribute.
> 
> I've updated coro-params.cpp to cover this as well.
> 
> > My instinct reaction is, it may block some optimizations to optimize the alloca out.
> 
> The intrinsic gets dropped by CoroSplit, so after that it's not a problem. Before that, I'm not sure much interesting optimization can happen to allocas anyway? Especially since it's not known if they will be part of the coro frame or not at that point.

As far as I know, a lot of allocas related optimizations/transformation happens before CoroSplit. e.g., mem2reg. And this is the reason why we put CoroSplit in the middle of passes.

> 
> > I believe we shouldn't access the coroutine frame after coro.end. If the doc doesn't make it clear, let's try to make it.
> 
> How would we motivate that change though?
> 
> Also, would that solve the GRO issue (#49843) which led to adding the existing metadata?

I think so.

> 
> > > In fact, my change broke a test which seems to be doing exactly that. Just because it's old doesn't mean it was wrong. So the patch didn't feel solid.
> 
> > I do feel it is wrong since it accesses the frame after it ends. It makes no sense.
> 
> (To make it easier to follow, we're talking about [this access](https://github.com/llvm/llvm-project/blob/llvmorg-20.1.0-rc2/llvm/test/Transforms/Coroutines/coro-debug.ll#L75).)
> 
> Looking closer, I think that code might be fine. The code after `coro.end` will go at the end of the ramp function. At that point the coro frame is still alive. And since that alloca is also [accessed in the resume function](https://github.com/llvm/llvm-project/blob/llvmorg-20.1.0-rc2/llvm/test/Transforms/Coroutines/coro-debug.ll#L67) (after a suspend), it _needs_ to be in the frame.

On the one hand, I still suspect to the validness of the old test. On the other hand, I think we should improve our algorithm to identify the area after coro.end as special area when calculating whether or not to put an alloca to the frame.

The short term solution may consider the case the coroutine won't suspend specially.

https://github.com/llvm/llvm-project/pull/127653