Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

No EndRegion emitted for DestructionScope #43457

Closed
RalfJung opened this issue Jul 24, 2017 · 5 comments · Fixed by #44082
Closed

No EndRegion emitted for DestructionScope #43457

RalfJung opened this issue Jul 24, 2017 · 5 comments · Fixed by #44082
Assignees
Labels
A-MIR Area: Mid-level IR (MIR) - https://blog.rust-lang.org/2016/04/19/MIR.html C-bug Category: This is a bug.

Comments

@RalfJung
Copy link
Member

RalfJung commented Jul 24, 2017

The following code

fn rc_refcell_test(r: RefCell<i32>) {
    r.borrow_mut();
}

compiles to MIR without any EndRegion (this is right before the EraseRegions pass):

// MIR for `rc_refcell_test`
// source = Fn(NodeId(54))
// pass_name = EraseRegions
// disambiguator = before

fn rc_refcell_test(_1: std::cell::RefCell<i32>) -> () {
    let mut _0: ();                      // return pointer
    scope 1 {
        let _2: std::cell::RefCell<i32>; // "r" in scope 1 at ../tests/run-pass/std.rs:19:20: 19:21
    }
    let mut _3: std::cell::RefMut<ReScope(DestructionScope(NodeId(259))), i32>;
    let mut _4: &ReScope(DestructionScope(NodeId(259))) std::cell::RefCell<i32>;

    bb0: {
        StorageLive(_2);                 // scope 0 at ../tests/run-pass/std.rs:19:20: 19:21
        _2 = _1;                         // scope 0 at ../tests/run-pass/std.rs:19:20: 19:21
        StorageLive(_4);                 // scope 1 at ../tests/run-pass/std.rs:20:5: 20:6
        _4 = &ReScope(DestructionScope(NodeId(259))) _2; // scope 1 at ../tests/run-pass/std.rs:20:5: 20:6
        _3 = const <std::cell::RefCell<T>>::borrow_mut(_4) -> bb1; // scope 1 at ../tests/run-pass/std.rs:20:5: 20:19
    }

    bb1: {
        drop(_3) -> bb2;                 // scope 1 at ../tests/run-pass/std.rs:20:5: 20:19
    }

    bb2: {
        StorageDead(_4);                 // scope 1 at ../tests/run-pass/std.rs:20:19: 20:19
        _0 = ();                         // scope 1 at ../tests/run-pass/std.rs:19:37: 21:2
        StorageDead(_2);                 // scope 0 at ../tests/run-pass/std.rs:21:2: 21:2
        return;                          // scope 1 at ../tests/run-pass/std.rs:21:2: 21:2
    }
}

Cc @pnkfelix

@Mark-Simulacrum Mark-Simulacrum added the A-MIR Area: Mid-level IR (MIR) - https://blog.rust-lang.org/2016/04/19/MIR.html label Jul 24, 2017
@RalfJung
Copy link
Member Author

Cc @nikomatsakis

@Mark-Simulacrum Mark-Simulacrum added the C-bug Category: This is a bug. label Jul 28, 2017
@pnkfelix pnkfelix self-assigned this Aug 22, 2017
@pnkfelix
Copy link
Member

Indeed, even the CleanEndRegions.before.mir shows no EndRegion(_) for that borrow. Looking further now.

@pnkfelix
Copy link
Member

Very weird: It seems like at the time that control reaches this statement in rustc_mir::build::Builder::ast_block_stmts, there is no destruction scope attached to it...? still looking... (I'm not aware of a place where a destruction scope would be added later...)

@pnkfelix
Copy link
Member

A ha!

The destruction scope exists, but during the HIR -> HAIR translation, we fail to find it, because the way we look up the destruction scope is with some code that looks like this:

    let opt_def_id = cx.tcx.hir.opt_local_def_id(block_id);
        ...
        let opt_dxn_ext = opt_def_id.and_then(|def_id| {
            cx.tcx.region_maps(def_id).opt_destruction_extent(stmt.node.id())
        });

but the opt_def_id that we got for that block_id is None, so the lookup doesn't have a chance of working.

Now to figure out what opt_local_def_id is returning None here (or if we perhaps should be using a different key for the region_maps lookup?)

@pnkfelix
Copy link
Member

pnkfelix commented Aug 25, 2017

@eddyb points out that I should not be attempting to do cx.tcx.region_maps(def_id) in the first place here (which then goes south when I fail to find the appropriate def_id to use), because the cx itself now carries the appropriate region_maps directly, thanks to #41662 (specifically 73cd9bd)

Will fix!

alexcrichton added a commit to alexcrichton/rust that referenced this issue Aug 26, 2017
Fix destruction extent lookup during HIR -> HAIR translation

My method for finding the destruction extent, if any, from cbed41a (in rust-lang#39409), was buggy in that it sometimes failed to find an extent that was nonetheless present.

This fixes that, and is cleaner code to boot.

Fix rust-lang#43457
bors added a commit that referenced this issue Aug 26, 2017
Fix destruction extent lookup during HIR -> HAIR translation

My method for finding the destruction extent, if any, from cbed41a (in #39409), was buggy in that it sometimes failed to find an extent that was nonetheless present.

This fixes that, and is cleaner code to boot.

Fix #43457
oli-obk pushed a commit to oli-obk/rust that referenced this issue Sep 19, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-MIR Area: Mid-level IR (MIR) - https://blog.rust-lang.org/2016/04/19/MIR.html C-bug Category: This is a bug.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants