Drop is called more than once for the same object

[glennw]

use std::mem;

struct Fragment {
    dummy: int
}

impl Fragment {
    fn new(d: int) -> Fragment {
        Fragment {
            dummy: d,
        }
    }
}

impl Drop for Fragment {
    fn drop(&mut self) {
        println!("drop {}", self.dummy);
    }
}

fn main() {
    let mut fragments = vec!();
    fragments.push(Fragment::new(1));
    fragments.push(Fragment::new(2));

    let new_fragments: Vec<Fragment> = mem::replace(&mut fragments, vec![])
        .move_iter()
        .skip_while(|fragment| {
            true
        }).collect();
}

Produces the following output:

drop 1
drop 2
drop 2

[metajack]

@zwarich reported this differs for him based on -O.

[lilyball]

On my machine (OS X) the presence of -O is irrelevant.

rustc 0.12.0-pre (7a25cf3f3 2014-07-30 17:06:18 +0000)

[metajack]

Discovered on rust @ 0582421.

[brson]

@dotdash Could this be caused by incorrect LLVM lifetime annotations?

[zwarich]

@brson The Rust we're using for the upgrade doesn't have the LLVM lifetime intrinsic changes.

[zwarich]

@metajack The example that differs for me depending on optimization settings was my further reduction, not the original:

struct Fragment;

impl Drop for Fragment {
    fn drop(&mut self) {
        println!("drop");
    }
}

fn main() {
    let mut fragments = Vec::new();
    fragments.push(Fragment);
    fragments.push(Fragment);

    fragments.move_iter()
             .skip_while(|fragment| {
                 true
             }).collect::<Vec<Fragment>>();
}

[brson]

@pnkfelix or @nick29581 do either of you have the bandwith to look into this? It's blocking servo.

[dotdash]

Adding some debug output, it looks like fragment 2 is dropped too early.

use std::mem;

struct Fragment {
    dummy: int
}

impl Fragment {
    fn new(d: int) -> Fragment {
        Fragment {
            dummy: d,
        }
    }
}

impl Drop for Fragment {
    fn drop(&mut self) {
        println!("drop {}", self.dummy);
    }
}

fn main() {
    let mut fragments = vec!();
    fragments.push(Fragment::new(1));
    fragments.push(Fragment::new(2));

    let new_fragments: Vec<Fragment> = mem::replace(&mut fragments, vec![])
        .move_iter()
        .skip_while(|fragment| {
            println!("Skip {}", fragment.dummy);
            true
        }).collect();
    std::io::println("End");
}

Gives:

Skip 1
drop 1
drop 2
Skip 0
drop 2
End

[lilyball]

After reading through the LLVM IR, the issue is in SkipWhile::next().

Here's the source of SkipWhile::next():

    fn next(&mut self) -> Option<A> {
        let mut next = self.iter.next();
        if self.flag {
            next
        } else {
            loop {
                match next {
                    Some(x) => {
                        if (self.predicate)(&x) {
                            next = self.iter.next();
                            continue
                        } else {
                            self.flag = true;
                            return Some(x)
                        }
                    }
                    None => return None
                }
            }
        }
    }

In the match case Some(x), instead of moving the Fragment out of the Option, it merely takes a pointer. Then when the predicate passes, on the line next = self.iter.next(), it drops the next value before reassigning. And then it drops x. But since x was merely a pointer into the old next, it actually ends up dropping the fragment from the new next value.

I'm not quite sure what the expected sequence here is. In the else block, both x and next get dropped, but x gets dropped after moving it into the return slot, and the drop flag is set, which causes the subsequent drop of next to do nothing. But in the then block, next gets dropped first, and x ends up pointing into the new value (and thus, very appropriately, doesn't have its zero flag set).

I suppose a fix would be to actually move the value out of next and into x (i.e. making x no longer a pointer), setting the drop flag on the fragment in next in the process. I assume the use of a pointer is deliberate though.

[lilyball]

I strongly suspect this was caused by #15076, which removed the extra alloca slot that was used to move the value out of the llmatch.

[lilyball]

cc @luqmana

[dotdash]

Yeah, that makes sense, as that is the only place that does zero-after-drop, while explains the "Skip 0" in the debugging output.