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

getting parity of signed integer maximum or minimum results in stack overflow #37991

Closed
est31 opened this issue Nov 25, 2016 · 13 comments
Closed
Assignees
Labels
A-mir Area: Mid-level IR (MIR) - https://blog.rust-lang.org/2016/04/19/MIR.html E-easy Call for participation: Easy difficulty. Experience needed to fix: Not much. Good first issue. P-high High priority regression-from-stable-to-stable Performance or correctness regression from one stable version to another. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@est31
Copy link
Member

est31 commented Nov 25, 2016

Some examples that cause rustc to overflow its stack:

assert_eq!(i64::max_value() % 2, 1);
assert_eq!(i64::min_value() % 2, 0);

Give error message at compile time:

thread 'rustc' has overflowed its stack

Gives a stack overflow too if you use i8,i16,i32, but it can't be reproduced with unsigned types.

Also, this doesn't reproduce the stack overflow either:

let m = i64::min_value();
assert_eq!(m % 2, 0);

This dates back to at least rustc 1.12 stable, and is reproducible with recent nightly as well.

@est31
Copy link
Member Author

est31 commented Nov 25, 2016

Note that literals don't invoke the bug:

assert_eq!(127i8 % 2, 1);
assert_eq!(-9223372036854775808i64 % 2, 0);

@est31
Copy link
Member Author

est31 commented Nov 25, 2016

When I look at the stack, it seems to look like a MIR issue...

@est31
Copy link
Member Author

est31 commented Nov 25, 2016

The stack trace looks like this:

rustc_mir::transform::promote_consts::Promoter::promote_temp
<rustc_mir::transform::promote_consts::Promoter<'a, 'tcx> as rustc::mir::visit::MutVisitor<'tcx>>::visit_lvalue
[...] (lots and lots of these two functions calling each other)
<rustc_mir::transform::qualify_consts::QualifyAndPromoteConstants as rustc::mir::transform::MirMapPass<'tcx>>::run_pass
rustc::mir::transform::Passes::run_passes
rustc_driver::driver::phase_3_run_analysis_passes::{{closure}}::{{closure}}
[...] (I hope from here on its the same for each rustc invocation)

@jonas-schievink
Copy link
Contributor

cc @eddyb

@eddyb eddyb added A-mir Area: Mid-level IR (MIR) - https://blog.rust-lang.org/2016/04/19/MIR.html I-nominated regression-from-stable-to-stable Performance or correctness regression from one stable version to another. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Nov 25, 2016
@eddyb
Copy link
Member

eddyb commented Nov 25, 2016

The minimal reproduction is &(i64::max_value() % 2), with &i64::max_value() compiling fine.
Any macro that does formatting (including assert_eq) should be avoided from minimal examples, as the MIR and LLVM IR they generate for the formatting machinery is much noisier than the buggy part, usually.

The stack trace may suggest some cycle at the MIR level, which would be weird, or accidental recursion.

EDIT: ah, here's the signedness' relevance: a % b checks b != 0 and a != i64::MIN & b != -1:

    bb0: {
        StorageLive(_2);
        _2 = core::num::<impl i64>::max_value() -> bb1;
    }

    bb1: {
        _3 = Eq(const 2i64, const 0i64);
        assert(!_3, "attempt to calculate the remainder with a divisor of zero") -> bb2;
    }

    bb2: {
        _4 = Eq(const 2i64, const -1i64);
        _5 = Eq(_2, const -9223372036854775808i64);
        _6 = BitAnd(_4, _5);
        assert(!_6, "attempt to calculate the remainder with overflow") -> bb3;
    }

    bb3: {
        _1 = Rem(_2, const 2i64);
        StorageDead(_2);
    }

@brson brson added P-high High priority and removed P-high High priority labels Dec 1, 2016
@nikomatsakis
Copy link
Contributor

triage: P-high

@rust-highfive rust-highfive added P-high High priority and removed I-nominated labels Dec 1, 2016
@nikomatsakis
Copy link
Contributor

@eddyb any luck tracking this down?

@eddyb
Copy link
Member

eddyb commented Dec 11, 2016

Found the culprit: when we don't want to steal the call, we're not taking the destination out of the clone.
I don't think this can succeed, seems like it would always infinitely recurse as the destination is always there, but even if it did, the resulting MIR may have a malformed cleanup edge left around.

The reason the "stealing" case is complex in the first place is to get the target for the goto, I think the infinite recursion bug was avoided by accident there, not by design.

@eddyb eddyb added the E-easy Call for participation: Easy difficulty. Experience needed to fix: Not much. Good first issue. label Dec 11, 2016
@brson
Copy link
Contributor

brson commented Dec 15, 2016

@eddyb will backport for 1.15 on feb 2.

@nikomatsakis
Copy link
Contributor

#38408 is a duplicate, so whoever fixes this should check that out too.

@brson
Copy link
Contributor

brson commented Dec 29, 2016

This needs to be fixed and backported soon. Release is feb 2. I'd like this backported by the third week of jan at latest.

@est31
Copy link
Member Author

est31 commented Dec 29, 2016

I'll give it a try later today.

@est31
Copy link
Member Author

est31 commented Dec 30, 2016

Sorry, I think the code is too unfamiliar to me. I don't want to fix it anymore :)

@arielb1 do you want to take over?

@arielb1 arielb1 assigned arielb1 and unassigned eddyb Jan 4, 2017
arielb1 added a commit to arielb1/rust that referenced this issue Jan 4, 2017
promotion of MIR terminators used to try to promote the destination it
is trying to promote, leading to stack overflow.

Fixes rust-lang#37991.
bors added a commit that referenced this issue Jan 6, 2017
fix promotion of MIR terminators

promotion of MIR terminators used to try to promote the destination it
is trying to promote, leading to stack overflow.

Also clean up the code in `promote_temp` a bit to make it more understandable.

Fixes #37991.

cc @nikomatsakis
r? @eddyb
nikomatsakis pushed a commit to nikomatsakis/rust that referenced this issue Jan 6, 2017
promotion of MIR terminators used to try to promote the destination it
is trying to promote, leading to stack overflow.

Fixes rust-lang#37991.
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 E-easy Call for participation: Easy difficulty. Experience needed to fix: Not much. Good first issue. P-high High priority regression-from-stable-to-stable Performance or correctness regression from one stable version to another. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests

7 participants