Calling a C function with a wrong type from Miri results in panic

[kpp]

Code

extern "C" {
    fn malloc(size: u32) -> *mut std::ffi::c_void;
}

fn main() {
    unsafe { malloc(42) };
}

https://play.rust-lang.org/?version=nightly&mode=debug&edition=2018&gist=c161d6b819c07b126414739cf6e32274

Meta

rustc 1.44.0-nightly (3712e11a8 2020-04-12) running on x86_64-unknown-linux-gnu

Error output

thread 'rustc' panicked at 'assertion failed: `(left == right)`
  left: `8`,
 right: `4`', /rustc/3712e11a828af2eea273a3e7300115e65833fbc5/src/libstd/macros.rs:16:9
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

error: internal compiler error: unexpected panic

note: the compiler unexpectedly panicked. this is a bug.

note: we would appreciate a bug report: https://github.com/rust-lang/rust/blob/master/CONTRIBUTING.md#bug-reports

note: rustc 1.44.0-nightly (3712e11a8 2020-04-12) running on x86_64-unknown-linux-gnu

note: compiler flags: -Z always-encode-mir -Z mir-emit-retag -Z mir-opt-level=0 -C debug-assertions=on -C codegen-units=1 -C debuginfo=2 --crate-type bin

note: some of the compiler flags provided by cargo are hidden

error: could not compile `playground`.

To learn more, run the command again with --verbose.

[RalfJung]

Thanks for the report! I moved this to the Miri tracker.

This is conceptually close to #1272, but much harder to fix than that. We'd need variants of our to_usize etc methods that raise UB instead of ICEing when the size does not match. Then we have to use them everywhere. And we have to do that without making all the shim code even less readable.

The good news is that I don't think this issue can hide UB, it just leads to bad error messages.

[RalfJung]

Well I guess an alternative might be to adjust our existing Scalar::to_* methods to replace the assertion that the size matches, with raising UB when it does not match. In some cases that could be false UB and actually indicate a bug in Miri, but I am not sure if it's worth spending the effort to tease those cases apart. @oli-obk what do you think?

[oli-obk]

So basically rustc will always be fine with the panicking variants, but miri needs non-panicking variants? I guess we could play with extension traits to get such behaviour differences between miri and ctfe/engine but that would be very confusing.

I'm fine with just making all the methods return results, even if most/all of the errors reported on these mean bugs in ctfe/engine, but it may make debugging harder if we actually have bugs again there. Similar to the size field on Scalar::Bits

[RalfJung]

It may actually also make debugging easier because errors, unlike assertions, point to the MIR span that was evaluated when they got triggered. ;)

[samrat]

I'd like to work on this issue.

[RalfJung]

@samrat great. :)

This issue will mostly (maybe entirely) require work on rustc, where the assertions that we are hitting lie.

The first step (and maybe the only step) is to turn this assertion into raising an appropriate interpreter error instead:

https://github.com/rust-lang/rust/blob/40008dcb494a00571123b7d59115068a200ebe34/src/librustc_middle/mir/interpret/value.rs#L396

Probably a new variant should be added to this enum for that purpose:

https://github.com/rust-lang/rust/blob/40008dcb494a00571123b7d59115068a200ebe34/src/librustc_middle/mir/interpret/error.rs#L309

Once that fix lands on rustc, a testcase should be added to Miri.