mkovacevic99 wrote:

Thank you both for giving significant insides on this issue. Here's the 
solution I'd propose (I attached it here), and the reasoning behind it.

1. Model the guarded initializer as a ConditionalEvaluation (generic, both 
ABIs).
A guarded init only runs on the pass/thread that wins the guard, so it's 
genuinely conditional code — but the cleanup machinery wasn't being told that, 
which is the root of the ICE. Marking the region conditional makes the existing 
machinery adjust the block's lifetime-extended capture destructors in two ways: 
it saves each capture's address into a slot that's valid on all paths (so the 
end-of-scope destructor reloads it instead of recomputing it from the 
in-guard-only %block — this is what resolves Instruction does not dominate all 
uses), and it adds an "init ran?" flag that guards the destructor (so it's 
skipped on the path where init was already done and the block was never built). 
I went with this because it fixes the crash at its actual cause and is 
ABI-independent.

2. Remove the guard-abort cleanup by identity instead of popping the top 
(per-ABI).
Enabling (1) exposed a second, pre-existing problem. EmitGuardedInit drops its 
guard-abort cleanup with PopCleanupBlock(), assuming it's on top — but by then 
the initializer has promoted the capture cleanups above it, so the wrong 
cleanup gets emitted inside the guarded region, destroying a capture before the 
block is used. That's exactly the non-ARC use-after-destruction @ahatanak 
flagged. I fix it by recording the guard cleanup's iterator and removing that 
one specifically (DeactivateCleanupBlock when it's no longer on top), so the 
capture destructors stay at end-of-scope, after the use. I think this part is 
necessary precisely because of @ahatanak's point: without it, fixing (1) 
doesn't just fail to help the escape case — it introduces a silent 
use-after-destroy where there was previously an ICE.

Why I stop here. Together these make the reported case and the 
single-activation block-capture case correct on every path, in both ABIs. I 
deliberately don't try to make the non-ARC escape case work across repeated 
calls — as @efriedma-quic notes, that's fundamentally broken because b is 
static but the block is on the stack, and the only real fix is giving the block 
itself static/heap lifetime (block lifetime extension), which is a separate 
feature rather than a codegen-correctness fix.

On the ARC path. I tested @efriedma-quic's heap-promotion point with an 
-fobjc-arc -fblocks case (Darwin/Itanium triple). Confirmed: the store emits 
llvm.objc.retainBlock (the heap copy), b and the call both use the heap copy, 
and my change only destroys the stack block's captures — once, after the call. 
No double-destroy, no interference with the heap copy, and the module verifies. 
So immediate vs. after-the-call destruction of the stack copy doesn't matter 
for ARC (the call uses the heap version), and my change is consistent with that.
[guarded-init-block-capture.patch](https://github.com/user-attachments/files/29043114/guarded-init-block-capture.patch)


https://github.com/llvm/llvm-project/pull/199508
_______________________________________________
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Reply via email to