Skip to content

break stmt confuses intialization-knowledge of borrow checker #24267

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

Closed
pnkfelix opened this issue Apr 10, 2015 · 6 comments · Fixed by #24330
Closed

break stmt confuses intialization-knowledge of borrow checker #24267

pnkfelix opened this issue Apr 10, 2015 · 6 comments · Fixed by #24330
Assignees
Labels
P-medium Medium priority
Milestone

Comments

@pnkfelix
Copy link
Member

This code should not compile:

demo code courtesy of niko's comment below.

struct Foo { n0: i32, n1: i32 }

fn leak_1_brk() -> Foo {
    let ret;
    loop {
        ret = Foo { n0: { break }, n1: 22 };
    }
    // (`ret` is still uninitialized here!)
    ret
}

fn main() { }

Original report:

I was making regression tests for #21486 and ran into this, though @nikomatsakis has pointed out that it is not specific to FRU.

Test case 1:

use std::sync::atomic::{Ordering, AtomicUsize, ATOMIC_USIZE_INIT};

#[derive(Debug)]
struct Noisy(u8);
impl Drop for Noisy {
    fn drop(&mut self) {
        // println!("splat #{}", self.0);
        event(self.0);
    }
}

#[allow(dead_code)]
#[derive(Debug)]
struct Foo { n0: Noisy, n1: Noisy }
impl Foo {
    fn vals(&self) -> (u8, u8) { (self.n0.0, self.n1.0) }
}

fn leak_1_brk() -> Foo {
    let _old_foo = Foo { n0: Noisy(1), n1: Noisy(2) };
    let ret;
    loop {
        ret = Foo { n0: { break }, .._old_foo };
    }
    // ret has not been initialized!!!!!
    ret
}

pub fn main() {
    reset_log();
    // Note: there's not much that's reasonable to expect here...
    let value = leak_1_brk().vals();
    assert_eq!(0x01_02, event_log());
    assert_eq!(value, (1,2));
}

static LOG: AtomicUsize = ATOMIC_USIZE_INIT;

fn reset_log() {
    LOG.store(0, Ordering::SeqCst);
}

fn event_log() -> usize {
    LOG.load(Ordering::SeqCst)
}

fn event(tag: u8) {
    let old_log = LOG.load(Ordering::SeqCst);
    let new_log = (old_log << 8) + tag as usize;
    LOG.store(new_log, Ordering::SeqCst);
}

Test case 2:

use std::sync::atomic::{Ordering, AtomicUsize, ATOMIC_USIZE_INIT};

#[derive(Debug)]
struct Noisy(u8);
impl Drop for Noisy {
    fn drop(&mut self) {
        // println!("splat #{}", self.0);
        event(self.0);
    }
}

#[allow(dead_code)]
#[derive(Debug)]
struct Foo { n0: Noisy, n1: Noisy }
impl Foo {
    fn vals(&self) -> (u8, u8) { (self.n0.0, self.n1.0) }
}

fn leak_1_brk() -> Foo {
    let _old_foo = Foo { n0: Noisy(1), n1: Noisy(2) };
    let ret;
    loop {
        ret = Foo { n0: { break }, n1: { break } };
    }
    // !!!!!
    ret
}


pub fn main() {
    reset_log();
    // Note: there's not much that's reasonable to expect here...
    assert_eq!(leak_1_brk().vals(), (1,2));
    assert_eq!(0x01_02, event_log());
}

static LOG: AtomicUsize = ATOMIC_USIZE_INIT;

fn reset_log() {
    LOG.store(0, Ordering::SeqCst);
}

fn event_log() -> usize {
    LOG.load(Ordering::SeqCst)
}

fn event(tag: u8) {
    let old_log = LOG.load(Ordering::SeqCst);
    let new_log = (old_log << 8) + tag as usize;
    LOG.store(new_log, Ordering::SeqCst);
}
@nikomatsakis
Copy link
Contributor

Here is a vastly reduced test case:

http://is.gd/EtaDNZ

struct Foo { n0: i32, n1: i32 }

fn leak_1_brk() -> Foo {
    let ret;
    loop {
        ret = Foo { n0: { break }, n1: 22 };
    }
    ret
}

fn main() { }

@pnkfelix pnkfelix self-assigned this Apr 10, 2015
@pnkfelix
Copy link
Member Author

triage: P-high (1.0)

@rust-highfive rust-highfive added the P-medium Medium priority label Apr 10, 2015
@rust-highfive rust-highfive added this to the 1.0 milestone Apr 10, 2015
@apasel422
Copy link
Contributor

An example not involving fields:

fn consume(bar: Box<i32>) {
    println!("{:?}", x);
}

fn main() {
    let mut foo = Box::new(1);
    loop {
        foo = { consume(foo); break };
    }
    consume(foo);
}

@apasel422
Copy link
Contributor

I've actually been exploiting this here: https://github.com/apasel422/tree/blob/master/src/node/mod.rs#L254-L269. It's only "worked," apparently, because there is no implementation of Drop for the types involved, which are references.

@pnkfelix
Copy link
Member Author

Based on some cursory investigation, my current hypothesis is that there is a bug in rustc::middle::dataflow, or at least in the interaction between rustc::middle::cfg and rustc::middle::dataflow.

In particular, we have a method, add_kills_from_flow_exits, which is meant to gather all of the effects that need to occur due to a non-local control transfer (like that of break), based on the scopes that it pops through, and assign those effects to the transfer-function attached to the break.

The problem is that it does this by taking the union of all of the kill-effects for every node in the exiting_scopes associated with that edge in the control-flow graph, and even the assignment expression is in the exiting_scopes list. So we end up including the kill-effect of the assignment (which in this case I think kills the record that foo is in the "moved-away" set), and so we end up thinking that foo is initialized in the successor node of the break in the control-flow.

Its possible the fix here may be simple. Not sure yet.

@ftxqxd
Copy link
Contributor

ftxqxd commented Apr 10, 2015

Minimal:

fn main() {
    let x: i32;
    loop {
        x = break;
    }
    println!("{}", x);
}

pnkfelix added a commit to pnkfelix/rust that referenced this issue Apr 11, 2015
…-exits.

Fix borrowck analysis so that it will not treat a break that pops
through an assignment `x = { ... break; ... }` as a kill of the
"moved-out" bit for `x`.

Fix rust-lang#24267.
pnkfelix added a commit to pnkfelix/rust that referenced this issue Apr 14, 2015
Revise rustc::middle::dataflow: one must select kill-kind when calling
add_kill. The current kill-kinds are (1.) kills associated with
ends-of-scopes and (2.) kills associated with the actual action of the
expression/pattern.

Then, use this to fix borrowck analysis so that it will not treat a
break that pops through an assignment `x = { ... break; ... }` as a
kill of the "moved-out" bit for `x`.

Fix rust-lang#24267.
pnkfelix added a commit to pnkfelix/rust that referenced this issue Apr 14, 2015
Revise rustc::middle::dataflow: one must select kill-kind when calling
add_kill. The current kill-kinds are (1.) kills associated with
ends-of-scopes and (2.) kills associated with the actual action of the
expression/pattern.

Then, use this to fix borrowck analysis so that it will not treat a
break that pops through an assignment `x = { ... break; ... }` as a
kill of the "moved-out" bit for `x`.

Fix rust-lang#24267.
pnkfelix added a commit to pnkfelix/rust that referenced this issue Apr 15, 2015
Revise rustc::middle::dataflow: one must select kill-kind when calling
add_kill. The current kill-kinds are (1.) kills associated with
ends-of-scopes and (2.) kills associated with the actual action of the
expression/pattern.

Then, use this to fix borrowck analysis so that it will not treat a
break that pops through an assignment `x = { ... break; ... }` as a
kill of the "moved-out" bit for `x`.

Fix rust-lang#24267.

(incorporated review feedback.)
bors added a commit that referenced this issue Apr 15, 2015
Extend rustc::middle::dataflow to allow filtering kills from flow-exits.

Fix borrowck analysis so that it will not treat a break that pops through an assignment
```rust
x = { ... break; ... }
```
as a kill of the "moved-out" bit for `x`.

Fix #24267.

[breaking-change], but really, its only breaking code that was already buggy.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
P-medium Medium priority
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants