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

Jump to bottom

Fix ICE on using result of index on a constant to index into a constant #30714

Merged
merged 1 commit into from
Feb 16, 2016
Merged

Fix ICE on using result of index on a constant to index into a constant #30714

merged 1 commit into from
Feb 16, 2016

Conversation

wesleywiser
Copy link
Member

The issue was that the const evaluator was returning an error because
the feature flag const_indexing wasn't turned on. The error was then
reported as a bug.

Fixes #29914

@rust-highfive
Copy link
Collaborator

r? @arielb1

(rust_highfive has picked a reviewer for you, use r? to override)

@wesleywiser
Copy link
Member Author

First time fixing an ICE so any feedback is greatly appreciated 😄

@wesleywiser
Copy link
Member Author

I'll squash those up later

@nagisa
Copy link
Member

nagisa commented Jan 5, 2016

cc @oli-obk @eddyb

@oli-obk
Copy link
Contributor

oli-obk commented Jan 5, 2016

Good idea. We should probably report all other errors, too. It should only be a bug when it returns a non int ok value.

oli-obk
oli-obk reviewed Jan 5, 2016
View reviewed changes
src/test/run-pass/issue-29914.rs
const ARR: [usize; 5] = [5, 4, 3, 2, 1];

fn main() {
assert_eq!(2, ARR[ARR[3]]);
Copy link
Contributor

Choose a reason for hiding this comment

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

Doesn't this require the feature gate to be turned on?

Ah nvrmind. This runs the normal eval then. Please add a second test with the feature gate enabled.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done

@wesleywiser wesleywiser force-pushed the fix_29914 branch 3 times, most recently from 3d4d377 to 11c16db Compare January 6, 2016 00:32
@wesleywiser
Copy link
Member Author

@oli-obk Done

@arielb1
Copy link
Contributor

arielb1 commented Jan 6, 2016

@bors r+

@bors
Copy link
Contributor

bors commented Jan 6, 2016

📌 Commit 11c16db has been approved by arielb1

@bors
Copy link
Contributor

bors commented Jan 6, 2016

⌛ Testing commit 11c16db with merge 1f86f80...

bors added a commit that referenced this pull request Jan 6, 2016
@bors
Auto merge of #30714 - wesleywiser:fix_29914, r=arielb1
1f86f80 
The issue was that the const evaluator was returning an error because
the feature flag const_indexing wasn't turned on. The error was then
reported as a bug.

Fixes #29914
@bors
Copy link
Contributor

bors commented Jan 6, 2016

💔 Test failed - auto-mac-64-nopt-t

@wesleywiser
Copy link
Member Author 

failures:

---- [run-pass] run-pass/issue-29914.rs stdout ----

error: test run failed!
status: signal: 11
command: x86_64-apple-darwin/test/run-pass/issue-29914.stage2-x86_64-apple-darwin 
stdout:
------------------------------------------

------------------------------------------
stderr:
------------------------------------------

------------------------------------------

thread '[run-pass] run-pass/issue-29914.rs' panicked at 'explicit panic', /Users/rustbuild/src/rust-buildbot/slave/auto-mac-64-nopt-t/build/src/compiletest/runtest.rs:1505


failures:
    [run-pass] run-pass/issue-29914.rs

test result: FAILED. 2335 passed; 1 failed; 3 ignored; 0 measured

@wesleywiser
Copy link
Member Author

I just retested this on OS X 10.11 and the test passes. Not sure what's going on

@wesleywiser
Copy link
Member Author

So I tried merging again and retesting and it works on my machine:

run rpass [x86_64-apple-darwin]: x86_64-apple-darwin/stage2/bin/compiletest

running 2 tests
test [run-pass] run-pass/issue-29914.rs ... ok
test [run-pass] run-pass/issue-29914-2.rs ... ok

test result: ok. 2 passed; 0 failed; 0 ignored; 0 measured

@arielb1 maybe @bors just needs to be retried?

@eddyb
Copy link
Member

eddyb commented Jan 8, 2016

@wesleywiser Signal 11 is SIGSEGV AFAIK, which is worrisome.

@wesleywiser
Copy link
Member Author

@eddyb Agreed I'm just not sure where to go from here. I can't even repro the issue on my computer.

@nagisa
Copy link
Member

nagisa commented Jan 8, 2016

AFAIR I also had SIGSEGV on buildbots once (a few months ago; also on OSX IIRC), which turned out to be “temporary” and not “reproducible” even on buildbots when retesting again.

@alexcrichton
Copy link
Member

@bors: retry

Looks like this was likely spurious

@bors
Copy link
Contributor

bors commented Jan 11, 2016

⌛ Testing commit 11c16db with merge 97b0e0e...

@bors
Copy link
Contributor

bors commented Jan 11, 2016

💔 Test failed - auto-win-gnu-64-nopt-t

@alexcrichton
Copy link
Member

@bors: retry

On Mon, Jan 11, 2016 at 3:44 PM, bors [email protected] wrote:

[image: 💔] Test failed - auto-win-gnu-64-nopt-t
http://buildbot.rust-lang.org/builders/auto-win-gnu-64-nopt-t/builds/2647

—
Reply to this email directly or view it on GitHub
#30714 (comment).

@bors
Copy link
Contributor

bors commented Jan 12, 2016

⌛ Testing commit 11c16db with merge cd251f8...

@oli-obk
Copy link
Contributor

oli-obk commented Jan 31, 2016

Jup

@wesleywiser
Copy link
Member Author

Got it. Thanks!

@ia0
Copy link
Contributor

ia0 commented Feb 7, 2016

It seems this PR also fixes #23513 and a similar ICE with const fn I got today:

#![feature(const_fn)]

const fn get(x: &[u8], i: usize) -> u8 {
    x[i]
}

fn main() {
    let _ = get(&[0], 0);
}

Theerror: internal compiler error: index is not an integer-constant expression is changed into error: non-constant path in constant expression.

wesleywiser
wesleywiser reviewed Feb 10, 2016
View reviewed changes
src/librustc_trans/trans/consts.rs
@@ -664,12 +662,19 @@ fn const_expr_unadjusted<'a, 'tcx>(cx: &CrateContext<'a, 'tcx>,
adt::const_get_field(cx, &*brepr, bv, vinfo.discr, idx.node)
},
hir::ExprIndex(ref base, ref index) => {
//honor the const_indexing feature if this is in a CONST expression
if !cx.sess().features.borrow().const_indexing && trueconst == TrueConst::Yes {
Copy link
Member Author

Choose a reason for hiding this comment

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

I had to add this check in order to get the compile-fail test I added to pass. Maybe this is the wrong place to add the check though?

Copy link
Contributor

Choose a reason for hiding this comment

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

Const indexing is allowed in statics, just not in true constants. So this check is wrong here.

@wesleywiser
Copy link
Member Author

@oli-obk When you get a minute, could you look at my changes? Thanks!

@oli-obk
Copy link
Contributor

oli-obk commented Feb 10, 2016

With the change from eval_const_expr_partial to running purely in trans/consts, the compile-fail test is actually a run-pass test. This is not a breaking change, since previously failing code now works, and all previously working code will still work, because trans/consts can eval more things than eval_const_expr_partial, and there's nothing that eval_const_expr_partial can eval while trans/consts cannot.

oli-obk
oli-obk reviewed Feb 10, 2016
View reviewed changes
src/librustc_trans/trans/consts.rs
Ok(ConstVal::Uint(u)) => u,
_ => cx.sess().span_bug(index.span,
"index is not an integer-constant expression")
let iv = try!(const_expr(cx, &**index, param_substs, fn_args, trueconst)).0;
Copy link
Contributor

Choose a reason for hiding this comment

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

trueconst should probably be TrueConst::Yes (definitely change this just to be on the safe side).

@wesleywiser
Copy link
Member Author

the compile-fail test is actually a run-pass test

I think I'm missing something then. Isn't it supposed to fail because it's performing a constant index operation without using #![feature(const_indexing)]?

@oli-obk
Copy link
Contributor

oli-obk commented Feb 10, 2016

not quite: the const_indexing feature exists solely to prevent constant indexing in array length calculations. Indexing was allowed in statics since 1.0.

@wesleywiser
Copy link
Member Author

Ah ok. That makes perfect sense now. Thanks! I'll fix this up tonight

@wesleywiser
Copy link
Member Author

@oli-obk Fixed

oli-obk
oli-obk reviewed Feb 11, 2016
View reviewed changes
src/librustc_trans/trans/consts.rs
_ => cx.sess().span_bug(index.span,
"index is not an integer-constant expression")
let (bv, bt) = try!(const_expr(cx, &**base, param_substs, fn_args, TrueConst::Yes));
let iv = try!(const_expr(cx, &**index, param_substs, fn_args, trueconst)).0;
Copy link
Contributor

Choose a reason for hiding this comment

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

the TrueConst::Yes should be on the index, not the array

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed, thanks

@oli-obk
Copy link
Contributor

oli-obk commented Feb 12, 2016

lgtm

@bors
Copy link
Contributor

bors commented Feb 13, 2016

☔ The latest upstream changes (presumably #31524) made this pull request unmergeable. Please resolve the merge conflicts.

@wesleywiser
Fix ICE on using result of index on a constant to index into a constant
271777c 
The issue was that the const evaluator was returning an error because
the feature flag const_indexing wasn't turned on. The error was then
reported as a bug.

Fixes rust-lang#29914
@wesleywiser
Copy link
Member Author

Rebased

@wesleywiser
Copy link
Member Author

@arielb1 I think this is ready to go

@arielb1
Copy link
Contributor

arielb1 commented Feb 16, 2016

@bors r+

@bors
Copy link
Contributor

bors commented Feb 16, 2016

📌 Commit 271777c has been approved by arielb1

@bors
Copy link
Contributor

bors commented Feb 16, 2016

⌛ Testing commit 271777c with merge 18f8143...

bors added a commit that referenced this pull request Feb 16, 2016
@bors
Auto merge of #30714 - wesleywiser:fix_29914, r=arielb1
18f8143 
The issue was that the const evaluator was returning an error because
the feature flag const_indexing wasn't turned on. The error was then
reported as a bug.

Fixes #29914
@bors
Copy link
Contributor

bors commented Feb 16, 2016

☀️ Test successful - auto-linux-32-nopt-t, auto-linux-32-opt, auto-linux-64-debug-opt, auto-linux-64-nopt-t, auto-linux-64-opt, auto-linux-64-x-android-t, auto-linux-cross-opt, auto-linux-musl-64-opt, auto-mac-32-opt, auto-mac-64-nopt-t, auto-mac-64-opt, auto-mac-ios-opt, auto-win-gnu-32-nopt-t, auto-win-gnu-32-opt, auto-win-gnu-64-nopt-t, auto-win-gnu-64-opt, auto-win-msvc-32-opt, auto-win-msvc-64-opt

@bors bors merged commit 271777c into rust-lang:master Feb 16, 2016
@steveklabnik steveklabnik mentioned this pull request Feb 17, 2016
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@arielb1 arielb1

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
9 participants
@wesleywiser @rust-highfive @nagisa @oli-obk @arielb1 @bors @eddyb @alexcrichton @ia0