transform (coroutines): fix memory corruption for tail calls that reference stack allocations #2117
Conversation
…erence stack allocations This change fixes a bug in which `alloca` memory lifetimes would not extend past the suspend of an asynchronous tail call. This would typically manifest as memory corruption, and could happen with or without normal suspending calls within the function.
| @@ -600,11 +600,11 @@ func (c *coroutineLoweringPass) lowerFuncsPass() { | |||
| continue | |||
| } | |||
|
|
|||
| if len(fn.normalCalls) == 0 { | |||
| // No suspend points. Lower without turning it into a coroutine. | |||
| if len(fn.normalCalls) == 0 && fn.fn.FirstBasicBlock().FirstInstruction().IsAAllocaInst().IsNil() { | |||
There was a problem hiding this comment.
I don't think alloca instructions are always at the start of the entry block. I think it would be safer to check the entire entry block for alloca instructions, just in case some are not the first instruction.
(Technically they can be anywhere in the function but we check in other places that this isn't possible).
There was a problem hiding this comment.
If there are any alloca instructions that are not at the start of the entry block, the coroutine lowering pass crashes iirc.
There was a problem hiding this comment.
IIRC it crashes if they are not in the entry block. but I think they are allowed if they are not directly at the start of the entry block.
| // Replace terminator with branch to cleanup. | ||
| // Replace terminator with a branch to the exit. | ||
| var exit llvm.BasicBlock | ||
| if ret.kind == returnNormal || ret.kind == returnVoid || fn.fn.FirstBasicBlock().FirstInstruction().IsAAllocaInst().IsNil() { |
… start of the entry block before lowering
|
I added a pre-processing step which ensures that all allocas in the entry block exist at the start. I had previously made sure that this never happened because a while back we hit an issue where a basic block split moved some allocas out of the entry block. |
| for !inst.IsAAllocaInst().IsNil() { | ||
| inst = llvm.NextInstruction(inst) | ||
| } |
There was a problem hiding this comment.
I believe this won't work correctly if there are no alloca instructions in the entry block: it will eventually reach the last instruction in the block and crash on a nil pointer dereference.
There was a problem hiding this comment.
The loop condition is the other way around. If there are no alloca instructions this will run 0 times.
There was a problem hiding this comment.
Oh, I see. I got confused by the double negation.
| // Find any other alloca instructions and move them after the other allocas. | ||
| c.builder.SetInsertPointBefore(inst) | ||
| for !inst.IsNil() { | ||
| next := llvm.NextInstruction(inst) | ||
| if !inst.IsAAllocaInst().IsNil() { | ||
| inst.RemoveFromParentAsInstruction() | ||
| c.builder.Insert(inst) | ||
| } | ||
| inst = next | ||
| } |
There was a problem hiding this comment.
This moves alloca instructions together, which I guess is fine, but I don't see how this addresses the issue I raised before?
I don't think alloca instructions are always at the start of the entry block. I think it would be safer to check the entire entry block for alloca instructions, just in case some are not the first instruction.
There was a problem hiding this comment.
This moves all alloca instructions to the start of the entry block, so later the check does correctly apply to all allocas in the entry block. This also fixes the potential issues where an alloca could be moved by a later transformation into another block by SplitBasicBlock.
| for !inst.IsAAllocaInst().IsNil() { | ||
| inst = llvm.NextInstruction(inst) | ||
| } |
There was a problem hiding this comment.
Oh, I see. I got confused by the double negation.
This change fixes a bug in which
allocamemory lifetimes would not extend past the suspend of an asynchronous tail call. This would typically manifest as memory corruption, and could happen with or without normal suspending calls within the function.Fixes a bug from #2101