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

Segfault in a compiler-generated Drop impl #29092

Closed
emberian opened this issue Oct 16, 2015 · 10 comments · Fixed by #30823
Closed

Segfault in a compiler-generated Drop impl #29092

emberian opened this issue Oct 16, 2015 · 10 comments · Fixed by #30823
Labels
A-codegen Area: Code generation I-crash Issue: The compiler crashes (SIGSEGV, SIGABRT, etc). Use I-ICE instead when the compiler panics.

Comments

@emberian
Copy link
Member

I've tried reducing this code in various ways, and unfortunately the original seems to already be the smallest that fails! Removing variants from Term causes it to not exhibit itself, as does removing cases from the outer match in small_eval. It reproduces on latest nightly only if debuginfo=2, but on older nightlies (@Aatch reports a repro on 2015-09-22) or 1.3.0 it repros for debuginfo=1. On all versions, doing optimizations causes the issue to disappear.

use self::Term::*;

#[derive(Clone)]
pub enum Term {
    True,
    False,
    If(Box<Term>, Box<Term>, Box<Term>),
    Succ(Box<Term>),
    Pred(Box<Term>),
    IsZero(Box<Term>),
}

fn isnumeric(v: &Term) -> bool {
    match *v {
        Pred(ref f) | Succ(ref f) => isnumeric(f),
        _ => false
    }
}

// a small-step evaluator
pub fn small_eval(v: &Term) -> Term {
    match *v {
        If(ref con, ref the, ref els) => match **con {
            True => *the.clone(),
            False => *els.clone(),
            _ => If(Box::new(small_eval(con)), the.clone(), els.clone()),
        },
        Succ(ref s) => Succ(Box::new(small_eval(s))),
        Pred(ref s) => match **s {
            Succ(ref v) if isnumeric(v) => {
                *v.clone()
            },
            _ => Pred(Box::new(small_eval(s))),
        },
        IsZero(ref s) => match **s {
            Succ(ref s) if isnumeric(s) => False,
            _ => IsZero(Box::new(small_eval(s))),
        },
        _ => v.clone()
    }
}

fn main() {
    let t = If(Box::new(True), Box::new(True), Box::new(True));
    small_eval(&t);
}

I suspect that an alloca (forced by debuginfo) is getting trampled somewhere.

GDB backtrace (from rustc 1.5.0-nightly (eafe106 2015-10-15)):

(gdb) run
Starting program: /tmp/foo 
[Thread debugging using libthread_db enabled]
Using host libthread_db library "/usr/lib/libthread_db.so.1".

Program received signal SIGSEGV, Segmentation fault.
0x0000555555558b14 in Term::drop.3510::ha59bcdf39bbe7497 ()
(gdb) where
#0  0x0000555555558b14 in Term::drop.3510::ha59bcdf39bbe7497 ()
#1  0x0000555555558af2 in Box$LT$Term$GT$::drop.3508::h48a2557aadc5bcd2 ()
#2  0x00005555555592c1 in foo::small_eval (v=0x7fffffffdb08) at foo.rs:25
#3  0x0000555555559a06 in foo::main () at foo.rs:49
#4  0x000055555555f525 in sys_common::unwind::try::try_fn::h16668888951888304395 ()
#5  0x000055555555d1b9 in __rust_try ()
#6  0x000055555555f1c0 in rt::lang_start::ha93496369c7c61cfF5w ()
#7  0x0000555555559b1a in main ()
@emberian emberian added I-crash Issue: The compiler crashes (SIGSEGV, SIGABRT, etc). Use I-ICE instead when the compiler panics. A-debuginfo Area: Debugging information in compiled programs (DWARF, PDB, etc.) A-codegen Area: Code generation labels Oct 16, 2015
@emberian
Copy link
Member Author

Doing If(Box::new(False), ...) causes it to not segfault, but other cases do. So I believe that it is the that gets clobbered.

@dotdash
Copy link
Contributor

dotdash commented Oct 16, 2015

Reproduces for me only without any debuginfo. Both level 1 and 2 make the problem disappear, as do optimizations.

@dotdash
Copy link
Contributor

dotdash commented Oct 16, 2015

use self::Term::*;

#[derive(Clone)]
pub enum Term {
    True,
    False,
    If(Box<Term>),
}

// a small-step evaluator
pub fn small_eval(v: Term) -> Term {
    match v {
        If(ref con) => match **con {
            True => True,
            False => *con.clone(),
            _ => True,
        },
        _ => True,
    }
}

fn main() {
    small_eval(If(Box::new(True)));
}

still segfaults for me.

@dotdash
Copy link
Contributor

dotdash commented Oct 16, 2015

Minimized even further:

use self::Term::*;

#[derive(Clone)]
pub enum Term {
    True,
    False,
    If(Box<Term>),
}

// a small-step evaluator
pub fn small_eval(v: Term) -> Term {
    match v {
        If(ref con) => match **con {
            True => True,
            _ => *con.clone(),
        },
        _ => True,
    }
}

fn main() {
    small_eval(If(Box::new(True)));
}

The crash happens because the drop for the Box<Term> returned by the clone call is executed even though that match arm isn't taken.

@dotdash
Copy link
Contributor

dotdash commented Oct 16, 2015

Adding braces around *con.clone() makes the error disappear, and moves the drop to the correct block. Maybe a missing AST scope?

cc @nikomatsakis

@emberian
Copy link
Member Author

@dotdash The smallest example you posted repros in the same manner (that is, only no-op without debuginfo).

@dotdash
Copy link
Contributor

dotdash commented Oct 20, 2015

Eliminated the need for the nested match.

use self::Term::*;

#[derive(Clone)]
pub enum Term {
    Dummy,
    A(Box<Term>),
    B(Box<Term>),
}

// a small-step evaluator
pub fn small_eval(v: Term) -> Term {
    match v {
        A(t) => *t.clone(),
        B(t) => *t.clone(),
        _ => Dummy,
    }
}

fn main() {
    small_eval(Dummy);
}

@nikomatsakis You said that you think that we might be missing a drop initializer, but I don't think that's the case. It's not the match binding that's being dropped, but the expression in the match arm. Initializing that to a dropped value wouldn't do anything because the initialization would also happen in the match arm, which is never executed.

@michaelwoerister michaelwoerister removed the A-debuginfo Area: Debugging information in compiled programs (DWARF, PDB, etc.) label Oct 30, 2015
@pnkfelix pnkfelix changed the title Segfault with debuginfo in a compiler-generated Drop impl Segfault in a compiler-generated Drop impl Jan 8, 2016
@pnkfelix
Copy link
Member

pnkfelix commented Jan 8, 2016

(revised title based on @dotdash observations)

@dotdash
Copy link
Contributor

dotdash commented Jan 12, 2016

I cannot reproduce this anymore, and in any case, I suspect it would be fixed by #30823

@pnkfelix
Copy link
Member

(The given test still readily reproduces the bug on the playpen, so I'll throw a regression test for this into the mix for #30823.)

pnkfelix added a commit to pnkfelix/rust that referenced this issue Jan 13, 2016
…st-lang#30530, rust-lang#30822.

Note that the test for rust-lang#30822 is folded into the test for rust-lang#30530 (but
the file name mentions only 30530).
Manishearth added a commit to Manishearth/rust that referenced this issue Jan 14, 2016
…r-issue-30530, r=dotdash

Put back alloca zeroing for issues rust-lang#29092, rust-lang#30018, rust-lang#30530; inject zeroing for rust-lang#30822.

----

Background context: `fn alloca_zeroed` was removed in PR rust-lang#22969, so we haven't been "zero'ing" (\*) the alloca's since at least that point, but the logic behind that PR seems sound, so its not entirely obvious how *long* the underlying bug has actually been present.  In other words, I have not yet done a survey to see when the new `alloc_ty` and `lvalue_scratch_datum` calls were introduced that should have had "zero'ing" the alloca's.

----

I first fixed rust-lang#30018, then decided to do a survey of `alloc_ty` calls to see if they needed similar treatment, which quickly led to a rediscovery of rust-lang#30530.

While making the regression test for the latter, I discovered rust-lang#30822, which is a slightly different bug (in terms of where the "zero'ing" needs to go), but still relevant.

I haven't finished the aforementioned survey of `fn alloc_ty` calls, but I decided I wanted to get this up for review in its current state (namely to see if my attempt to force developers to include a justification for passing `Uninit` can possibly fly, or if I should abandon that path of action).

----

(*): I am putting quotation marks around "zero'ing" because we no longer use zero as our "dropped" marker value.

Fix rust-lang#29092
Fix rust-lang#30018
Fix rust-lang#30530
Fix rust-lang#30822
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-codegen Area: Code generation I-crash Issue: The compiler crashes (SIGSEGV, SIGABRT, etc). Use I-ICE instead when the compiler panics.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants