Report const index out-of-bound condition earlier

[graydon]

Currently if you try indexing into a const vector, in a const expr, we report the bounds-overrun late in compilation -- during translation. We should notice it earlier, in the const evaluation pass.

[jld]

Apparently the “error” this produces doesn't cause compilation to fail — code will be generated and rustc will return 0. LLVM will complain if the index is physically out of bounds, and this is currently always equivalent to an out-of-bounds index (I think), but that won't be the case for const slices.

This is especially unexpected during testing, which (except for check-fast, apparently?) doesn't show the output/error on success.

[catamorphism]

Not critical for 0.6; de-milestoning

[catamorphism]

Nominating for milestone 5, production-ready

[graydon]

accepted for production-ready milestone

[alexcrichton]

As an example of what I believe that this is talking about:

static a: &'static [int] = &[];
static b: int = a[1];

fn main() {}

yields

$ rustc foo.rs
foo.rs:2:16: 2:19 error: const index-expr is out of bounds
foo.rs:2 static b: int = a[1];
                         ^~~
Assertion failed: (ReqTy && "extractvalue indices invalid!"), function getExtractValue, file ../../../../src/llvm/lib/IR/Constants.cpp, line 1982.
zsh: abort      rustc foo.rs

Seems bad that we're hitting an LLVM assertion at all.

[pnkfelix]

Accepted for P-low.

[huonw]

Triage: @alexcrichton's (9 month old) example is still syntactically valid (yay!) and still fails with that assertion (boo!).

[vks]

I believe this is fixed.

[JustAPerson]

If you update @alexcrichton's example:

#![allow(dead_code)]
const A: &'static [usize] = &[];
const B: usize = A[1];

fn main() {}

It now compiles successfully without any complaints.

Only once you attempt to use the invalid value B will rustc encounter a problem.

#![allow(dead_code)]
const A: &'static [usize] = &[];
const B: usize = A[1];

fn main() {
    println!("B={}", B);
}

<anon>:3:18: 3:22 error: const index-expr is out of bounds
<anon>:3 const B: usize = A[1];
                          ^~~~
rustc: /home/rustbuild/src/rust-buildbot/slave/nightly-dist-rustc-linux/build/src/llvm/lib/IR/Constants.cpp:2174: static llvm::Constant* llvm::ConstantExpr::getExtractValue(llvm::Constant*, llvm::ArrayRef<unsigned int>, llvm::Type*): Assertion `ReqTy && "extractvalue indices invalid!"' failed.
Aborted (core dumped)
playpen: application terminated with error code 134

[steveklabnik]

error: cannot refer to other statics by value, use the address-of operator or a constant instead

Yup, seems good.

[pnkfelix]

wait, @JustAPerson 's example does not seem okay to me. Reopening.

[tamird]

I think this is now fixed for real:

$ cat foo.rs
#![allow(dead_code)]
const A: &'static [usize] = &[];
const B: usize = A[1];

fn main() {
    println!("B={}", B);
}
$ rustc foo.rs
foo.rs:3:18: 3:22 error: const index-expr is out of bounds
foo.rs:3 const B: usize = A[1];
                          ^~~~
error: aborting due to previous error
$ rustc --version
rustc 1.0.0-dev (a691f1eef 2015-04-15) (built 2015-04-15)

[steveklabnik]

So, sometime in the last 11 days:

hello.rs:3:18: 3:22 error: const index-expr is out of bounds
hello.rs:3 const B: usize = A[1];
                            ^~~~
rustc: /home/rustbuild/src/rust-buildbot/slave/nightly-dist-rustc-linux/build/src/llvm/include/llvm/Support/Casting.h:237: typename llvm::cast_retty<X, Y*>::ret_type llvm::cast(Y*) [with X = llvm::SequentialType; Y = llvm::Type; typename llvm::cast_retty<X, Y*>::ret_type = llvm::SequentialType*]: Assertion `isa<X>(Val) && "cast<Ty>() argument of incompatible type!"' failed.

soooo seems like it's already regressed?

[tamird]

Guh, this was my mistake. I made the above report using a toolchain compiled with LLVM assertions disabled (which I didn't know was the default).

[carols10cents]

(Hi, I'm trying to help make E-easy issues more accessible to newcomers 😸)

I made the above report using a toolchain compiled with LLVM assertions disabled (which I didn't know was the default).

It sounds like in order to reproduce this issue, you have to be using a toolchain compiled with a non-default setting? If so, how does one go about doing that?

It's kind of confusing what's supposed to happen here, with all the closing-reopening stuff going on. Could someone clarify what the expected behavior is and how it differs from the current behavior?

[jonas-schievink]

./configure --enable-llvm-assertions

However, you can just use a Rust nightly, since LLVM assertions are enabled there. http://is.gd/X2RztV still fails the assertion on nightly:

<anon>:2:17: 2:21 error: const index-expr is out of bounds
<anon>:2 static b: i32 = a[1];
                         ^~~~
rustc: /home/rustbuild/src/rust-buildbot/slave/nightly-dist-rustc-linux/build/src/llvm/include/llvm/Support/Casting.h:237: typename llvm::cast_retty<X, Y*>::ret_type llvm::cast(Y*) [with X = llvm::SequentialType; Y = llvm::Type; typename llvm::cast_retty<X, Y*>::ret_type = llvm::SequentialType*]: Assertion `isa<X>(Val) && "cast<Ty>() argument of incompatible type!"' failed.
Aborted (core dumped)
playpen: application terminated with error code 134

[jld]

It looks like the thing I was distressed by in 2013 is fixed: if rustc gives the const index-expr is out of bounds error, even if LLVM assertions are disabled, it exits with a failure status (and does not write an output file). I think translation used to not have access to the session/error stuff that checking does, because it wasn't supposed to be able to find errors that checking missed? But that looks like it's not the case now.

The other thing my past self mentioned… it sounds as if we used to just pass the index to LLVM even if we knew it was out-of-range, but now we construct an undef instead. But that part is taken care of in any case, because the error is handled properly now.

As for the LLVM assertion we're seeing today, I have a guess. After reporting the error, we're doing this:

C_undef(type_of::type_of(cx, bt).element_type())

and I think we want something like this (untested):

C_undef(val_ty(arr).element_type())

Because if the value being indexed is a slice or pointer, then bt (its type) won't be represented by an array type, so element_type will fail.

But, the thing @graydon opened this issue for, and what the comment in the code referencing this issue suggests doing, is to move that check to an earlier stage of compilation. And it looks like #25370/#25570 might wind up more or less accomplishing that?

[Manishearth]

I think this is fixed completely now. @oli-obk ?

[oli-obk]

nope, it needs to be solved in check_const and the current report location needs to be changed into a bug location

[Manishearth]

@oli-obk Is this an easy fix, and if so, do you want to mentor it and leave some hints so that newcomers can try a crack at it?

[oli-obk]

While it's an easy fix (you can basically steal https://github.com/rust-lang/rust/blob/master/src/librustc/middle/check_const.rs#L470-L490 and do it for ExprIndex and then turn https://github.com/rust-lang/rust/blob/master/src/librustc_trans/trans/consts.rs#L708-L712 into a bug/unimplemented!()).

But it'd also be a breaking change, because unused const ARR: [u32; 0] = []; const X: u32 = ARR[5]; currently does not cause a compilation error. And check_const also checks unused constants.

Also it would end up doing all const evaluation twice. Which could be remedied once it becomes a problem by caching constants.

Of course the breaking change can be remedied by simply emitting the const_err lint.

[fbstj]

this seems to have been 'solved' (possibly due to MIRI) as it now throws E0080 when used, rather than an LLVM assertion. though it still passes through if the access is unused.

[oli-obk]

The missing lint when the const is unused will be fixed by #50110

[oli-obk]

Yea I think we're done with this issue after 6 years 😆