Miscompilation(s) due to MIR inlining

[saethlin]

I just figured I'd try running some tests and benchmarks with -Zmir-opt-level=3 and it is not going well. I realize I'm painting a rather grim picture here, but to be clear some code does in fact seem to compile correctly.

On commit e1d819583f0bf13b016b119c1c2c43e6d3979450 I ran:

$ RUSTFLAGS_NOT_BOOTSTRAP=-Zmir-opt-level=3 ./x.py test --stage 1 library/core

And I see 6 test failures in core_simd all like this:

---- src/../../portable-simd/crates/core_simd/src/swizzle.rs - core_simd::swizzle::Simd<T,LANES>::deinterleave (line 339) stdout ----
Invalid bitcast
  %29 = bitcast <4 x i32> %24 to [4 x i32]
Invalid bitcast
  %38 = bitcast <4 x i32> %28 to [4 x i32]
in function _ZN8rust_out4main75_doctest_main_src_______portable_simd_crates_core_simd_src_swizzle_rs_339_017h788d0453a929bc62E
LLVM ERROR: Broken function found, compilation aborted!
Couldn't compile the test.

---

I also tried running the tinyvec benchmark suite, commit cd8b94964ffd857119c2095ad3d232a4e16a2ad0 of https://github.com/lokathor/tinyvec

$ RUSTFLAGS="-Zmir-opt-level=3" cargo +nightly bench --features=alloc
   Compiling tinyvec v1.6.0 (/home/ben/tinyvec)
    Finished bench [optimized + debuginfo] target(s) in 2.22s
     Running unittests src/lib.rs (target/release/deps/tinyvec-031d885bae3cce6f)

running 0 tests

test result: ok. 0 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out; finished in 0.00s

     Running benches/macros.rs (target/release/deps/macros-c4a72fc6cba5f49d)
WARNING: HTML report generation will become a non-default optional feature in Criterion.rs 0.4.0.
This feature is being moved to cargo-criterion (https://github.com/bheisler/cargo-criterion) and will be optional in a future version of Criterion.rs. To silence this warning, either switch to cargo-criterion or enable the 'html_reports' feature in your Cargo.toml.

Gnuplot not found, using plotters backend
thread 'main' panicked at 'Unable to parse report_link template.: ParseError { msg: "Expected a closing '}' but found end-of-line instead.", line: 1, column: 14 }', /home/ben/.cargo/registry/src/github.com-1ecc6299db9ec823/criterion-0.3.6/src/html/mod.rs:280:14
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
error: bench failed, to rerun pass `--bench macros`

(this is not supposed to panic)

---

I also tried running the test suite for https://github.com/rust-lang/regex on commit ac2d0e1b33b4674ad9b26266ef4b828d7200ec0f

$ RUSTFLAGS="-Zmir-opt-level=3" cargo +nightly test --release
   Compiling memchr v2.5.0
   Compiling aho-corasick v0.7.20
   Compiling regex v1.7.0 (/home/ben/regex)
    Finished release [optimized + debuginfo] target(s) in 20.59s
     Running unittests src/lib.rs (target/release/deps/regex-0d0deeb6b9675203)

running 36 tests
error: test failed, to rerun pass `--lib`

Caused by:
  process didn't exit successfully: `/home/ben/regex/target/release/deps/regex-0d0deeb6b9675203` (signal: 4, SIGILL: illegal instruction)

rustc 1.67.0-nightly (53e4b9dd7 2022-12-04)
binary: rustc
commit-hash: 53e4b9dd74c29cc9308b8d0f10facac70bb101a7
commit-date: 2022-12-04
host: x86_64-unknown-linux-gnu
release: 1.67.0-nightly
LLVM version: 15.0.4

[saethlin]

Looks like the regex and tinyvec/criterion problems reproduce if I just remove the restriction that at at mir_opt_level() <= 2 we only consider #[inline] functions for inlining. That means that those crates could start miscompiling if someone just alters the source code somewhere, no nightly flags needed.

@rustbot label +A-mir-opt +I-unsound

[saethlin]

The same patch exposes the miscompile in core_simd:

diff --git a/compiler/rustc_mir_transform/src/inline.rs b/compiler/rustc_mir_transform/src/inline.rs
index bf670c5c26a..02a401e7c63 100644
--- a/compiler/rustc_mir_transform/src/inline.rs
+++ b/compiler/rustc_mir_transform/src/inline.rs
@@ -343,8 +343,8 @@ fn check_codegen_attributes(
             InlineAttr::Never => return Err("never inline hint"),
             InlineAttr::Always | InlineAttr::Hint => {}
             InlineAttr::None => {
-                if self.tcx.sess.mir_opt_level() <= 2 {
-                    return Err("at mir-opt-level=2, only #[inline] is inlined");
+                if self.tcx.sess.mir_opt_level() <= 1 {
+                    return Err("at mir-opt-level=1, only #[inline] is inlined");
                 }
             }
         }

Then this produces failing tests:

RUSTFLAGS_NOT_BOOTSTRAP="-Zmir-opt-level=2 -Zinline-mir" x test library/core

[matthiaskrgr]

Does -Zvalidate-mir show anything interesting in that context?

[saethlin]

Nope. Nothing at all.

[Noratrieb]

I minimized the regex case:

fn main() {
    TranslatorI.visit_pre();
}

impl TranslatorI {
    fn visit_pre(self) {
        Some(())
            .map(|_| self.flags())
            .unwrap_or_else(|| self.flags());
    }
}

struct TranslatorI;

impl TranslatorI {
    fn flags(&self) {}
}

Debug info is required to trigger the SIGILL.

[profile.release]
debug = true

or just rustc --edition=2021 file.rs -C opt-level=3 -C debuginfo=2 -Zmir-opt-level=3

Interestingly, adding #[inline] to fn flags stops it from miscompiling.

I used dustmite for a bunch of the work and it worked really well (apart of being quite annoying to set up), I can recommend it. Special thanks to @Byter09 for helping me with that :D

[Noratrieb]

searched nightlies: from nightly-2018-11-01 to nightly-2022-12-06
regressed nightly: nightly-2022-07-14
searched commit range: 1c7b36d...87588a2
regressed commit: 42bd138

bisected with cargo-bisect-rustc v0.6.4

Host triple: x86_64-unknown-linux-gnu
Reproduce with:

cargo bisect-rustc --access github --script ./script.sh --preserve

[saethlin]

I also found an ICE in encoding_rs https://github.com/hsivonen/encoding_rs 5931e21bc20bad0335964f441da0c4309401107a, definitely seems related to the inliner because removing -Zinline-mir=yes or -Zmir-opt-level=3 removes the ICE.

$ RUSTFLAGS="-Zmir-opt-level=3 -Zvalidate-mir -Zinline-mir=yes" cargo +nightly test

error: internal compiler error: broken MIR in Item(WithOptConstParam { did: DefId(0:1315 ~ encoding_rs[5e1f]::utf_16::{impl#0}::decode_to_utf16_raw), const_param_did: None }) (before pass SimplifyCfg-final) at bb218[0]:
                                encountered overlapping memory in `Call` terminator: _454 = bswap::<u16>(move _454) -> bb163
  --> src/utf_16.rs:85:21
   |
85 |                     dest.copy_utf16_from::<LittleEndian>(&mut source)
   |                     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
   |
   = note: delayed at compiler/rustc_const_eval/src/transform/validate.rs:784:26

[saethlin]

searched nightlies: from nightly-2022-01-01 to nightly-2022-12-05
regressed nightly: nightly-2022-11-28
searched commit range: 80a9646...1eb62b1
regressed commit: 0e9eee6

bisected with cargo-bisect-rustc v0.6.5

Host triple: x86_64-unknown-linux-gnu
Reproduce with:

cargo bisect-rustc --script script --start 2022-01-01 --end 2022-12-05 

[JakobDegen]

These two issues are unrelated, can we split them up?

@Nilstrieb 's is caused by a bug somewhere in codegen, Godbolt. The LLVM IR has UB on line 66, where a load is marked !noundef but it's loading from a freshly allocated stack slot. The MIR has no such load and no UB.

@rustbot label +I-wrong -I-unsound +A-codegen +A-llvm

[JakobDegen]

I don't know what sets that bug off, but it's worth pointing out that I have no reason to believe that it requires -Zmir-opt-level=3. It could be related to debuginfo generation for inlined scopes, but since inlining has been turned on that is possible on stable.

[matthiaskrgr]

@Nilstrieb could you share somehow how you did get dustmite to work?
I think having a way to automatically reduce code would be super useful for all kinds of tickets/regessions, I played around with dustmite a bit yesterday but the setup seems to be kinda tricky and I couldn't get it past "look I removed your crash.rs and the ICE seems to be gone!!" :/

[Byter09]

@matthiaskrgr I was the one that got it working for @Nilstrieb. DustMite wasn't really developed for Rust, so some syntax is problematic for it, but in 99% of cases it works just fine. I have it running right now reducing my library to a ICE at the moment, because I can't figure it out myself.

The basic idea is to first get it reliably running without DustMite. Once you have a valid test case that returns 0 if the problem occurs, you can use that to let DustMite have a go.

Also, --split '*.rs:d' is very important as a parameter, or else it won't even look inside the Rust files, as it doesn't consider them D source files. This parameter forces it to treat the file as if it was D code, and then it works just fine except for a few edge cases. In the case of @Nilstrieb's code the solution was to simply remove the unit test from the offending file.

Back then, I wrote the minimizer for Saltwater: https://github.com/jyn514/saltwater/tree/master/minimizer

[saethlin]

@JakobDegen I split the issue that bisects to DestProp out into #105428

[JakobDegen]

With those two issues separated out, is there still anything in this one that hasn't been investigated? The core simd ones I suppose?

[saethlin]

I bisected this:

#![feature(portable_simd)]
fn main() {
    use core::simd::Simd;
    let a = Simd::from_array([0, 4, 1, 5]);
    let b = Simd::from_array([2, 6, 3, 7]);
    let (x, y) = a.deinterleave(b);
    assert_eq!(x.to_array(), [0, 1, 2, 3]);
    assert_eq!(y.to_array(), [4, 5, 6, 7]);
}

back to nightly-2021-11-14 (which is too long ago to have CI toolchains), best guess is that this has reproduced since #89167. Which as far as I can tell is the PR that introduced core::simd. Is it possible to bisect further using different code?

[saethlin]

Looks like everything has been split out! Thanks everyone!