Segfault in an unconditional drop call after a match

[sfackler]

This code will segfault running drop glue for Box<Box<Fn()>>, even though a value of that type is not actually created.

enum Handler {
    Default,
    Custom(*mut Box<Fn()>),
}

fn main() {
    take();
}

fn take() -> Box<Fn()> {
    unsafe {
        match Handler::Default {
            Handler::Default => Box::new(main),
            Handler::Custom(ptr) => *Box::from_raw(ptr),
        }
    }
}

Interestingly, it exits successfully if the second match body is wrapped in a block.

[eddyb]

It looks like Box<Trait> noop coercions result in non-trivial codegen. Not sure if it's related to the actual cause, but maybe we shouldn't generate 20 instructions for nothing.

[nikomatsakis]

Seems like a case where we failed to initialize the temporary storage slot to 0, even though it is only conditionally initialized.

cc @pnkfelix

[dotdash]

Looks like it might be the same problem as #29092

[pnkfelix]

triage: P-high

[pnkfelix]

Seems like a case where we failed to initialize the temporary storage slot to 0, even though it is only conditionally initialized.

Yes, this looks right.

In particular, the code in trans is structured in a somewhat misleading way: there's this populate callback on lvalue_scratch_datum, where populate is supposed to initialize the scratch data that has been created via an alloca().

The problem, I think, is that there is no guarantee that the populate call will dominates the cleanup code generated at the end of the scope. The bcx (Block context) and the scope are separate inputs to lvalue_scratch_datum, and I think in examples like the one from this bug, the bcx corresponds to the match arm, while the scope is the parent of the match itself. :(

I'm not completely sure the best way to fix this quickly. My current plan is to try to inject initialization code (for types that need the drop flag) that will run right after the alloca() does, and hopefully LLVM will be smart enough to eliminate that initialization when it is effectively dead-code due to the code generated by populate.

---

Update: (not sure if the above hypothesis is 100% right; I'm reviewing the logic from lvalue_scratch_datum and below, and the populate-generated code seems like it must be generated immediately after the alloca; in the same bcx, etc)

[pnkfelix]

oh:

mod build { ... fn Alloca(cx, ty, name) -> ValueRef { AllocaFcx(cx.fcx, ty, name) } ... }

[nikomatsakis]

@pnkfelix and I had some discussion on IRC about this -- there used to be a flag (zero) that was used to indicate when the initialization might not dominate the drop, but it was removed in commit f580412

[eddyb]

@pnkfelix Yeah, we put all the allocas at the top of the entry BB. Doing otherwise results in dynamic stack manipulation which isn't the best in LLVM (IIRC you can stack overflow by having an alloca in a loop - presumably missing a llvm.lifetime.end call).

[pnkfelix]

@eddyb right, its actually perfectly sensible; I just worry (or worried) that some people may have the wrong impression based on the function signature(s).