Fix ICE on using result of index on a constant to index into a constant by wesleywiser · Pull Request #30714 · rust-lang/rust

wesleywiser · 2016-01-05T14:10:11Z

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 · 2016-01-05T14:10:16Z

r? @arielb1

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

wesleywiser · 2016-01-05T14:10:53Z

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

wesleywiser · 2016-01-05T14:46:10Z

I'll squash those up later

nagisa · 2016-01-05T15:20:55Z

cc @oli-obk @eddyb

oli-obk · 2016-01-05T17:55:59Z

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 · 2016-01-05T18:03:33Z

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.

wesleywiser · 2016-01-06T00:50:05Z

@oli-obk Done

arielb1 · 2016-01-06T15:26:55Z

@bors r+

bors · 2016-01-06T15:26:59Z

📌 Commit 11c16db has been approved by arielb1

bors · 2016-01-06T22:52:21Z

⌛ Testing commit 11c16db with merge 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 · 2016-01-06T23:35:13Z

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

wesleywiser · 2016-01-06T23:39:08Z

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 · 2016-01-07T00:50:05Z

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

wesleywiser · 2016-01-08T02:00:23Z

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 · 2016-01-08T02:20:03Z

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

wesleywiser · 2016-01-08T02:28:28Z

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

nagisa · 2016-01-08T12:26:26Z

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 · 2016-01-11T01:41:59Z

@bors: retry

Looks like this was likely spurious

bors · 2016-01-11T02:42:47Z

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

bors · 2016-01-11T02:44:28Z

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

alexcrichton · 2016-01-11T04:12:44Z

@bors: retry

On Mon, Jan 11, 2016 at 3:44 PM, bors notifications@github.com 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 · 2016-01-12T15:44:08Z

⌛ Testing commit 11c16db with merge cd251f8...

oli-obk · 2016-01-31T22:03:04Z

Jup

wesleywiser · 2016-01-31T22:24:46Z

Got it. Thanks!

ia0 · 2016-02-07T00:50:23Z

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 · 2016-02-10T00:48:02Z

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?

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

wesleywiser · 2016-02-10T00:51:41Z

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

oli-obk · 2016-02-10T08:30:45Z

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 · 2016-02-10T08:33:11Z

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

wesleywiser · 2016-02-10T14:56:31Z

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 · 2016-02-10T16:04:56Z

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 · 2016-02-10T16:28:26Z

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

wesleywiser · 2016-02-11T15:04:53Z

@oli-obk Fixed

oli-obk · 2016-02-11T15:08:16Z

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

Fixed, thanks

oli-obk · 2016-02-12T08:08:47Z

lgtm

bors · 2016-02-13T03:29:07Z

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

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 · 2016-02-13T21:31:00Z

Rebased

wesleywiser · 2016-02-16T15:41:18Z

@arielb1 I think this is ready to go

arielb1 · 2016-02-16T16:10:08Z

@bors r+

bors · 2016-02-16T16:10:10Z

📌 Commit 271777c has been approved by arielb1

bors · 2016-02-16T17:13:47Z

⌛ Testing commit 271777c with merge 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 · 2016-02-16T19:19:36Z

☀️ 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

rust-highfive assigned arielb1 Jan 5, 2016

oli-obk reviewed Jan 5, 2016
View reviewed changes

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

wesleywiser force-pushed the fix_29914 branch from 38de158 to f31cb44 Compare February 6, 2016 19:10

wesleywiser reviewed Feb 10, 2016
View reviewed changes

oli-obk reviewed Feb 10, 2016
View reviewed changes

wesleywiser force-pushed the fix_29914 branch from f31cb44 to 94cc410 Compare February 11, 2016 02:20

oli-obk reviewed Feb 11, 2016
View reviewed changes

wesleywiser force-pushed the fix_29914 branch from 94cc410 to 31fd057 Compare February 11, 2016 23:42

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 force-pushed the fix_29914 branch from 31fd057 to 271777c Compare February 13, 2016 20:13

bors merged commit 271777c into rust-lang:master Feb 16, 2016

steveklabnik mentioned this pull request Feb 17, 2016

ICE: "index is not an integer-constant expression" using enum as array index. #23513

Closed

Uh oh!

Uh oh!

Conversation

wesleywiser commented Jan 5, 2016

Uh oh!

rust-highfive commented Jan 5, 2016

Uh oh!

wesleywiser commented Jan 5, 2016

Uh oh!

wesleywiser commented Jan 5, 2016

Uh oh!

nagisa commented Jan 5, 2016

Uh oh!

oli-obk commented Jan 5, 2016

Uh oh!

oli-obk Jan 5, 2016

Choose a reason for hiding this comment

Uh oh!

wesleywiser Jan 5, 2016

Choose a reason for hiding this comment

Uh oh!

wesleywiser commented Jan 6, 2016

Uh oh!

arielb1 commented Jan 6, 2016

Uh oh!

bors commented Jan 6, 2016

Uh oh!

bors commented Jan 6, 2016

Uh oh!

bors commented Jan 6, 2016

Uh oh!

wesleywiser commented Jan 6, 2016

Uh oh!

wesleywiser commented Jan 7, 2016

Uh oh!

wesleywiser commented Jan 8, 2016

Uh oh!

eddyb commented Jan 8, 2016

Uh oh!

wesleywiser commented Jan 8, 2016

Uh oh!

nagisa commented Jan 8, 2016

Uh oh!

alexcrichton commented Jan 11, 2016

Uh oh!

bors commented Jan 11, 2016

Uh oh!

bors commented Jan 11, 2016

Uh oh!

alexcrichton commented Jan 11, 2016

Uh oh!

bors commented Jan 12, 2016

Uh oh!

oli-obk commented Jan 31, 2016

Uh oh!

wesleywiser commented Jan 31, 2016

Uh oh!

ia0 commented Feb 7, 2016

Uh oh!

wesleywiser Feb 10, 2016

Choose a reason for hiding this comment

Uh oh!

oli-obk Feb 10, 2016

Choose a reason for hiding this comment

Uh oh!

wesleywiser commented Feb 10, 2016

Uh oh!

oli-obk commented Feb 10, 2016

Uh oh!

oli-obk Feb 10, 2016

Choose a reason for hiding this comment

Uh oh!

wesleywiser commented Feb 10, 2016

Uh oh!

oli-obk commented Feb 10, 2016

Uh oh!

wesleywiser commented Feb 10, 2016

Uh oh!

wesleywiser commented Feb 11, 2016

Uh oh!