Make sure interpreter errors are never discarded

[RalfJung]

One important invariant of Miri is that when an interpreter error is raised (in particular a UB error), those must not be discarded: it's not okay to just check foo().is_err() and then continue executing.

This seems to catch new contributors by surprise fairly regularly. Would be good to make sure this can never happen. Ideally we'd have some sort of static analysis for this, but I can't think of an easy way to do that (could be an interesting clippy lint). The next best thing is to enforce this dynamically. The problem is that InterpError creation does not have access to the InterpCx. So instead we'd need some thread-local state to indicate "this interpreter is busted, don't continue executing in it... and we'd have to hope that nobody creates two interpreter instances on the same thread... but I can't think of a better way right now.

[workingjubilee]

drop bombs to discourage misuse?

[RalfJung]

Yeah, maybe that works.

[tiif]

I actually encountered this a few times so I'd really appreciate this enhancement, here is how I usually encounter this, hope it would help to produce a clearer diagnostic/ a test if anyone would like to work on this:

This usually happen when I forgot to put a ? for function that returns InterpResult, for example:

        match offset {
            None => fd.read(&fd, communicate, buf, count, dest, this), // the `?` is missing here
            Some(offset) => {
                let Ok(offset) = u64::try_from(offset) else {
                    let einval = this.eval_libc("EINVAL");
                    this.set_last_error(einval)?;
                    this.write_int(-1, dest)?;
                    return Ok(());
                };
                fd.pread(communicate, offset, buf, count, dest, this) // the `?` is missing here
            }
        };

(fd.read and fd.pread return InterpResult)

At this point, the compiler will emit an error and suggestion:

warning: unused `Result` that must be used
   --> src/shims/unix/fd.rs:587:9
    |
587 | /         match offset {
588 | |             None => fd.read(&fd, communicate, buf, count, dest, this),
589 | |             Some(offset) => {
590 | |                 let Ok(offset) = u64::try_from(offset) else {
...   |
597 | |             }
598 | |         };
    | |_________^
    |
    = note: this `Result` may be an `Err` variant, which should be handled
    = note: `#[warn(unused_must_use)]` on by default
help: use `let _ = ...` to ignore the resulting value
    |
587 |         let _ = match offset {
    |         +++++++

So if I didn't examine the suggestion carefully, I would just directly apply the suggestion and leads to:

        let _ = match offset { // the change is the `let _ = here`
            None => fd.read(&fd, communicate, buf, count, dest, this),
            Some(offset) => {
                let Ok(offset) = u64::try_from(offset) else {
                    let einval = this.eval_libc("EINVAL");
                    this.set_last_error(einval)?;
                    this.write_int(-1, dest)?;
                    return Ok(());
                };
                fd.pread(communicate, offset, buf, count, dest, this)
            }
        };

After the change, it will compile successfully. IMO this error is pretty dangerous as it can only be caught manually or by certain test, and the test error is not helpful for debugging as it points to different direction ( related discussion in zulip )

The correct fix for the initial code is actually:

        match offset {
            None => fd.read(&fd, communicate, buf, count, dest, this)?, // Added a `? `here
            Some(offset) => {
                let Ok(offset) = u64::try_from(offset) else {
                    let einval = this.eval_libc("EINVAL");
                    this.set_last_error(einval)?;
                    this.write_int(-1, dest)?;
                    return Ok(());
                };
                fd.pread(communicate, offset, buf, count, dest, this)? // Added a `?` here
            }
        };

[RalfJung]

Yeah IMO that's a dangerous suggestion to make.

[CAD97]

FWIW diagnostics and compilation errors in rustc work in a similar fashion, and dropping one without emitting it will ICE ultimately via panic_any. Since this is a programmer error and not user error, panicking is a reasonable out.

Just make sure to check if you're already panicking to avoid a double panic abort.

[RalfJung]

Unfortunately, panic-on-drop doesn't work. We have users like GVN that call interpreter methods like this

                let lhs = self.evaluated[lhs].as_ref()?;
                let lhs = self.ecx.read_immediate(lhs).ok()?;
                let rhs = self.evaluated[rhs].as_ref()?;
                let rhs = self.ecx.read_immediate(rhs).ok()?;
                let val = self.ecx.binary_op(bin_op, &lhs, &rhs).ok()?;

So any error is immediately dropped and turned into an Option.

However, it also seems entirely possible that one interpreter triggers a query that triggers const-eval that leads to another interpreter instance being created, possibly on the same thread. So TLS won't work either.

So... the next best thing I can think of is to use an extension trait to add a discard_interp_error method to InterpResult that returns Option, and use that everywhere in GVN (and everywhere else) instead of ok().

[oli-obk]

However, it also seems entirely possible that one interpreter triggers a query that triggers const-eval that leads to another interpreter instance being created, possibly on the same thread. So TLS won't work either.

This works, you take out the old value, set your own and at the end when you check the value, you set the old value again.

[RalfJung]

I went with the extension trait method: rust-lang/rust#130885.

[tiif]

I hit this error today, and I really like this change!

Preparing a sysroot for Miri (target: x86_64-unknown-linux-gnu)... done
thread 'rustc' panicked at compiler/rustc_middle/src/mir/interpret/error.rs:745:13:
an interpreter error got improperly discarded; use `discard_err()` if this is intentional