remove scalar size mismatch interpreter error, make it an ICE instead - #161238
Conversation
|
Some changes occurred to the CTFE / Miri interpreter cc @rust-lang/miri
cc @rust-lang/miri Some changes occurred to the CTFE machinery Some changes occurred to the CTFE / Miri interpreter cc @rust-lang/miri, @oli-obk, @lcnr Some changes occurred to MIR optimizations cc @rust-lang/wg-mir-opt |
|
rustbot has assigned @petrochenkov. Use Why was this reviewer chosen?The reviewer was selected based on:
|
|
@bors r+ |
…uwer Rollup of 11 pull requests Successful merges: - #161024 (resolving cyclic glob vis-max) - #161231 (passes: `rustc_scalable_vector` fields are not dead) - #161238 (remove scalar size mismatch interpreter error, make it an ICE instead) - #160345 (Resolver: add `checked` methods for `Cm(Ref)Cell`) - #161129 (Avoid ICE when recovering parenthesized type parameters) - #161235 (Compute job time in post-merge-report from the actual GitHub duration) - #161239 (Fix `#[repr(transparent)]` wrapper types not working with `Complex<T>`) - #161244 (Add regression test for indexing an unsized static without a body) - #161257 (Ignore target feature test when LLVM fails to compile minicore) - #161258 (perf: return early from in_external_macro for root contexts) - #161278 (Add regression test for normalization failure on erased closure in async block)
Rollup merge of #161238 - RalfJung:remove-scalar-size-mismatch, r=petrochenkov remove scalar size mismatch interpreter error, make it an ICE instead This error was added in #71569 to deal with ICEs on incorrect Miri shim signatures. However, that was a partial fix: if the signature is "even more incorrect", using a non-scalar type where the type should be scalar, we still ICE. The proper fix is tracked in rust-lang/miri#3842, and that is already used for a good chunk of our shims. I don't think it's worth keeping around the old, incomplete check here, so let's get rid of it. Sadly, the `to_$int` methods on `Scalar` still return a `Result` as they must still error if the scalar is a pointer and we are in const-eval where we cannot turn that pointer into an integer. We should probably use `ScalarInt` in a lot more places where we currently use `Scalar` to statically exclude this case... but that's a change for another time.
…uwer Rollup of 11 pull requests Successful merges: - rust-lang/rust#161024 (resolving cyclic glob vis-max) - rust-lang/rust#161231 (passes: `rustc_scalable_vector` fields are not dead) - rust-lang/rust#161238 (remove scalar size mismatch interpreter error, make it an ICE instead) - rust-lang/rust#160345 (Resolver: add `checked` methods for `Cm(Ref)Cell`) - rust-lang/rust#161129 (Avoid ICE when recovering parenthesized type parameters) - rust-lang/rust#161235 (Compute job time in post-merge-report from the actual GitHub duration) - rust-lang/rust#161239 (Fix `#[repr(transparent)]` wrapper types not working with `Complex<T>`) - rust-lang/rust#161244 (Add regression test for indexing an unsized static without a body) - rust-lang/rust#161257 (Ignore target feature test when LLVM fails to compile minicore) - rust-lang/rust#161258 (perf: return early from in_external_macro for root contexts) - rust-lang/rust#161278 (Add regression test for normalization failure on erased closure in async block)
|
for #161289 @rust-timer build 02877d8 |
This comment has been minimized.
This comment has been minimized.
|
Finished benchmarking commit (02877d8): comparison URL. Overall result: ❌ regressions - please read:Benchmarking means the PR may be perf-sensitive. It's automatically marked not fit for rolling up. Overriding is possible but disadvised: it risks changing compiler perf. Next, please: If you can, justify the regressions found in this try perf run in writing along with @bors rollup=never rustc-perf Instruction countOur most reliable metric. Used to determine the overall result above. However, even this metric can be noisy.
Max RSS (memory usage)Results (primary -2.5%)A less reliable metric. May be of interest, but not used to determine the overall result above.
CyclesResults (primary 2.1%, secondary 5.5%)A less reliable metric. May be of interest, but not used to determine the overall result above.
Binary sizeResults (secondary 0.0%)A less reliable metric. May be of interest, but not used to determine the overall result above.
Bootstrap: 455.347s -> 456.26s (0.20%) |
|
Looks like this caused a regression in #161289 This intuitively seems like it should be an improvement instead, so it looks unexpected. Do we know where this regression is coming from? Can we do something about it? |
|
Indeed this is very strange, I can only guess the the inliner is making worse decisions in some places now? |
This error was added in #71569 to deal with ICEs on incorrect Miri shim signatures. However, that was a partial fix: if the signature is "even more incorrect", using a non-scalar type where the type should be scalar, we still ICE. The proper fix is tracked in rust-lang/miri#3842, and that is already used for a good chunk of our shims.
I don't think it's worth keeping around the old, incomplete check here, so let's get rid of it.
Sadly, the
to_$intmethods onScalarstill return aResultas they must still error if the scalar is a pointer and we are in const-eval where we cannot turn that pointer into an integer. We should probably useScalarIntin a lot more places where we currently useScalarto statically exclude this case... but that's a change for another time.