Fix Miri failure with -Zmiri-tag-raw-pointers

[jturner314]

Before this PR, running MIRIFLAGS="-Zmiri-tag-raw-pointers" cargo miri test test_map_axis caused Miri to report undefined behavior in the test_map_axis test. This PR fixes the underlying issue. Basically, Miri doesn't like us using a reference to an element to access other elements. Here's some sample code to illustrate the issue:

let a: Vec<i32> = vec![5, 6];
let first_elt: &i32 = &a[0];
let ptr: *const i32 = first_elt as *const i32;
// Okay: the data is contained in the data referenced by `first_elt`.
let a0 = unsafe { &*ptr.offset(0) };
assert_eq!(*a0, 5);
// Not okay: the data is not contained in the data referenced by `first_elt`.
let a1 = unsafe { &*ptr.offset(1) };
assert_eq!(*a1, 6);

Before this commit, we were using self.index_axis(axis, 0).map(|first_elt| ...) to create views of the lanes, from references to the first elements in the lanes. Accessing elements within those views (other than the first element) led to the Miri error, since the view's pointer was derived from a reference to a single element.

Thanks to @5225225 for reporting the issue. (This PR fixes #1137.)

Should we add Miri to CI?

[adamreichold]

Should we add Miri to CI?

👍

For me, it found useful results in heavy-on-unsafe projects and doing it should be as simple as

- uses: actions-rs/toolchain@v1
    with:
    profile: minimal
    toolchain: nightly
    override: true
    components: miri, rust-src
- uses: actions-rs/cargo@v1
    with:
    command: miri
    args: setup
- uses: actions-rs/cargo@v1
    with:
    command: miri
    args: test

[5225225]

though I tend to run with more strict flags than miri's default, so you'll need to set some enviroment variables.

https://github.com/rust-lang/miri

I use tag-raw-pointers, check-number-validity.

and for rustc there is rust-lang/rust#87868 -Zrandomize_layout

I'd suggest running tests with all 3 of those set. Stacked borrows and uninit ints being UB are up in the air, the latter seems likely and the former, not sure.

[bluss]

I've looked at adding MIRI to ci before and would like to have it. Don't remember what the holdups have been. For one thing, MIRI is too slow to finish on time so it needs to be a reduced set of tests?

Thanks for the report and thanks for the fix! The error and fix make a lot of sense.

[5225225]

Miri is rather slow, so you might need to either skip or reduce the size of some tests when running in miri. I wouldn't do that until specific tests are shown to take far too long though.

There's also a fair few API's being unsupported, but I'm assuming ndarray doesn't use much of the stdlib (as opposed to alloc/core), so that shouldn't be an issue.

[bluss]

We can merge to the 0.15.x branch and make another release on there, if you think it's merited? 0.16 is likely to be in development for a bit

[jturner314]

We can merge to the 0.15.x branch and make another release on there, if you think it's merited?

I doubt there's much risk of this leading to broken code with the current compiler, but it would be nice to fix it now just in case. Also, it seems like people are slow to update to newer versions of ndarray, and I wouldn't be surprised for future compiler versions to have issues with this. I can manage the release if you'd like. (I'd merge this into master, cherry-pick it onto the 0.15.x branch, update the release notes, and release 0.15.5.)

[5225225]

By the way, there's still miri failures on this PR, not sure if you wanted to fix them in PRs one by one or go for all of them at a time.

Next error is test_mat_mul, there's possibly more

[bluss]

sounds good, I could also plan to do it if you wouldn't have time. we can plan the release to whenever we have multiple fixes batched up then.

[jturner314]

I took a look at some of the other errors. The error in test_mat_mul is an issue in matrixmultiply (fix in bluss/matrixmultiply#67). There are errors for the Windows and ExactChunks iterators, since they use an inner iterator which produces references to elements, and they then use those references to create views which span more elements. I think that's all of the remaining errors, although I'll need to run Miri again after fixing those errors to make sure.