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.

Sign up for GitHub

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

Don't run cleanups twice in "if true" blocks #10735

Merged
merged 1 commit into from
Nov 30, 2013
Merged

Don't run cleanups twice in "if true" blocks #10735

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 11 additions & 7 deletions src/librustc/middle/trans/controlflow.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -77,7 +77,7 @@ pub fn trans_if(bcx: @mut Block,
return with_scope(bcx, thn.info(), "if_true_then", |bcx| {
let bcx_out = trans_block(bcx, thn, dest);
debuginfo::clear_source_location(bcx.fcx);
trans_block_cleanups(bcx_out, block_cleanups(bcx))
bcx_out
})
} else {
let mut trans = TransItemVisitor { ccx: bcx.fcx.ccx } ;
Expand All @@ -90,9 +90,9 @@ pub fn trans_if(bcx: @mut Block,
elexpr.info(),
"if_false_then",
|bcx| {
let bcx_out = trans_if_else(bcx, elexpr, dest);
let bcx_out = trans_if_else(bcx, elexpr, dest, false);
debuginfo::clear_source_location(bcx.fcx);
trans_block_cleanups(bcx_out, block_cleanups(bcx))
bcx_out
})
}
// if false { .. }
Expand All @@ -116,7 +116,7 @@ pub fn trans_if(bcx: @mut Block,
let (else_bcx_in, next_bcx) = match els {
Some(elexpr) => {
let else_bcx_in = scope_block(bcx, elexpr.info(), "else");
let else_bcx_out = trans_if_else(else_bcx_in, elexpr, dest);
let else_bcx_out = trans_if_else(else_bcx_in, elexpr, dest, true);
(else_bcx_in, join_blocks(bcx, [then_bcx_out, else_bcx_out]))
}
_ => {
Expand All @@ -138,7 +138,7 @@ pub fn trans_if(bcx: @mut Block,

// trans `else [ if { .. } ... | { .. } ]`
fn trans_if_else(else_bcx_in: @mut Block, elexpr: @ast::Expr,
dest: expr::Dest) -> @mut Block {
dest: expr::Dest, cleanup: bool) -> @mut Block {
let else_bcx_out = match elexpr.node {
ast::ExprIf(_, _, _) => {
let elseif_blk = ast_util::block_from_expr(elexpr);
Expand All @@ -150,8 +150,12 @@ pub fn trans_if(bcx: @mut Block,
// would be nice to have a constraint on ifs
_ => else_bcx_in.tcx().sess.bug("strange alternative in if")
};
debuginfo::clear_source_location(else_bcx_in.fcx);
trans_block_cleanups(else_bcx_out, block_cleanups(else_bcx_in))
if cleanup {
debuginfo::clear_source_location(else_bcx_in.fcx);
trans_block_cleanups(else_bcx_out, block_cleanups(else_bcx_in))
} else {
else_bcx_out
}
}
}

Expand Down
20 changes: 20 additions & 0 deletions src/test/run-pass/issue-10734.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
// Copyright 2013 The Rust Project Developers. See the COPYRIGHT
// file at the top-level directory of this distribution and at
// http://rust-lang.org/COPYRIGHT.
//
// Licensed under the Apache License, Version 2.0 <LICENSE-APACHE or
// http://www.apache.org/licenses/LICENSE-2.0> or the MIT license
// <LICENSE-MIT or http://opensource.org/licenses/MIT>, at your
// option. This file may not be copied, modified, or distributed
// except according to those terms.

pub fn main() {
if true {
let _a = ~3;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems not quite ideal to rely on throwing a pointer being freed was not allocated malloc error to indicate the bug exists. I would suggest something like the following:

#[unsafe_no_drop_flag]
struct Foo {
    dropped: bool
}

impl Drop for Foo {
    fn drop(&mut self) {
        assert!(!self.dropped);
        self.dropped = true;
    }
}

pub fn main() {
    if true {
        let _a = Foo{ dropped: false };
    }

    if false {
        fail!();
    } else {
        let _a = Foo{ dropped: false };
    }
}

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After chatting with @cmr, it might actually be nice to assert that it's dropping at all, just to kill two birds with one stone (i.e. define this test as checking that it drops the correct number of times, rather than checking that it hasn't dropped too many times).

static mut drop_count: uint = 0;

#[unsafe_no_drop_flag]
struct Foo {
    dropped: bool
}

impl Drop for Foo {
    fn drop(&mut self) {
        assert!(!self.dropped);
        self.dropped = true;
        unsafe { drop_count += 1; }
    }
}

pub fn main() {
    if true {
        let _a = Foo{ dropped: false };
    }
    unsafe { assert!(drop_count == 1); }

    if false {
        fail!();
    } else {
        let _a = Foo{ dropped: false };
    }
    unsafe { assert!(drop_count == 2); }
}

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Although, come to think about it, this is not quite right as it's making a (probably bad) assumption about where the drops occur. It should probably be something like this instead:

pub fn main() {
    {
        if true {
            let _a = Foo{ dropped: false };
        }

        if false {
            fail!();
        } else {
            let _a = Foo{ dropped: false };
        }
    }
    unsafe { assert!(drop_count == 2); }
}

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I feel like I'm having a conversation with myself.

After thinking some more, the previous version (with the two asserts) isn't so bad, because that's also checking to make sure that if true { expr } behaves like { expr } rather than behaving like expr, which seems to me to be the correct interpretation. FWIW, that version (again, the one with the two asserts) passes with this patch applied.

}
if false {
fail!()
} else {
let _a = ~3;
}
}