mkovacevic99 wrote:
> I spent a little time digging into the control flow here. If I'm following
> correctly, EmitDeclInit calls EmitScalarInit, which calls EmitScalarExpr,
> which calls ScalarExprEmitter::VisitExprWithCleanups. Which has a
> RunCleanupsScope. But the following seems to fix the crash (not a suggestion,
> just a debugging step):
>
> ```
> diff --git a/clang/lib/CodeGen/CGExprScalar.cpp
> b/clang/lib/CodeGen/CGExprScalar.cpp
> index c8a8ec7b6d92..2009ccc30470 100644
> --- a/clang/lib/CodeGen/CGExprScalar.cpp
> +++ b/clang/lib/CodeGen/CGExprScalar.cpp
> @@ -3226,6 +3226,7 @@ Value *ScalarExprEmitter::VisitStmtExpr(const StmtExpr
> *E) {
> }
>
> Value *ScalarExprEmitter::VisitExprWithCleanups(ExprWithCleanups *E) {
> + CodeGenFunction::RunCleanupsScope ExtraScope(CGF);
> CodeGenFunction::RunCleanupsScope Scope(CGF);
> Value *V = Visit(E->getSubExpr());
> // Defend against dominance problems caused by jumps out of expression
> ```
>
> Following this deeper leads us to CodeGenFunction::EmitBlockLiteral,
> specifically the call to pushLifetimeExtendedDestroy, which intentionally
> dodges the RunCleanupScope. It looks like this goes back to
> [c9a52de](https://github.com/llvm/llvm-project/commit/c9a52de0026093327daedda7ea2eead8b64657b4)
> . CC @ahatanak
>
> We probably don't want to extend the lifetime in the context of a static
> variable. But I'm not sure what the general rule is supposed to look like...
> I'm not sure if this is documented anywhere.
Thanks for digging in on this!
I think that RunCleanupsScope doesn't actually run a lifetime-extended cleanup,
it just hands it up to the enclosing scope — it moves it onto the normal
cleanup stack one level out rather than emitting it (In CGCleanup.cpp: line
482-511). So the scope that's already in VisitExprWithCleanups only passes the
block's destructor upward; it never runs it. That's why your second scope makes
the crash go away — it's the outer scope that finally emits it. My
RunCleanupsScope around EmitDeclInit is doing that same job, just placed around
the variable initializer.
On your bigger question — "should we extend the lifetime at all here?" — I
don't think we can drop the extension in general. It's deliberate (In
CGBlocks.cpp: line1089-1107), from
[c9a52de](https://github.com/llvm/llvm-project/commit/c9a52de0026093327daedda7ea2eead8b64657b4#diff-48b4ac2147eb3be322e2558dc65c7c3ee5817df18d3246c6a5eacd336e710583))
because a stack block can be kept and used after the line that created it:
```
auto b = ^{ use(s); }; // keep a handle
b(); // used later — the captures must still be alive
```
so extending to the end of the scope is correct for normal code.
What I think is the key point: the extension isn't actually what's wrong here —
it happens for plain locals too (drop the static from the repro and it compiles
fine, even though the captures are still extended to end of function). The
thing that makes it crash is that statick init is conditional — it's wrapped in
a "first time?" guard, so the block is only built inside that branch. The
extended cleanup then runs at the end of the enclosing scope, which is reached
on the path where the guard was skipped and the block was never built — hence
the dominance error. With an unconditional initializer the block is always
built before that cleanup, so it's fine.
So I think the right approach is to bound the initializer's cleanups to the
initializer itself, inside the guarded region where the block actually exists —
which is what wrapping EmitDeclInit in a RunCleanupsScope does. It fixes both
the Itanium and Microsoft paths in one spot, and it's safe because nothing can
legally hold onto the block past a static initializer anyway, so running its
captures' destructors at end-of-initializer doesn't change any observable
behavior. (I think it's also why the plain global-init path never hit this —
that init is unconditional, so the cleanup at function exit still has a block
to clean up.)
I did consider fixing it at the source in EmitBlockLiteral — basically skip the
extension when we're inside a static init — but that means teaching block
codegen to know it's in that context, plus pinning down the general rule that,
as you said, doesn't seem to be written down. So I leaned toward the scope fix
since it stays local and changes no existing behavior. Totally happy to go the
other way if you and @ahatanak think the root-cause fix is cleaner — there may
be history here I can't see.
https://github.com/llvm/llvm-project/pull/199508
_______________________________________________
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
