Miri exception improvements

[RalfJung]

The Miri engine uses "exceptions" (InterpResult) to report misbehaving executions and propagate this information outwards to the machine (const-eval/const-prop/miri-the-tool). When an error is raised, we optionally capture a backtrace to ease debugging.

There are two problems with this:

• Currently, checking whether we want to capture a backtrace is rather expensive (we check an env var). @wesleywiser proposed a scheme to use a session variable instead.
• Sometimes, the engine catches these "exceptions". This means we caught the backtrace in vein. Check RUSTC_CTFE_BACKTRACE much less by generating fewer errors #69290 brought some nice speed-up by removing just one case of catching an interpreter error! I think we should never catch these errors. Is there something we can do to prevent accidental catching?

Cc @oli-obk

[oli-obk]

The obvious solution is to go through the engine (we could still use the ? operator and basically not require any changes except to type signatures if we made it an associated constant or at least not an &self function), but that requires infecting all error types.

Didn't we also discuss checking the current query name somewhere?

[wesleywiser]

Didn't we also discuss checking the current query name somewhere?

Yes, it was proposed here but at least for ctfe-stress-4, that didn't pay off because most of the errors were created during const_eval-raw (see here).

[RalfJung]

@oli-obk I take you you are talking about the first point? As I wrote above, I think @wesleywiser's proposed scheme of using a session var is a good next step there -- certainly better than what we do now.

[RalfJung]

I found some more cases of exceptions being caught:

• rust/src/librustc_mir/interpret/validity.rs

  Line 758 in 97b3d81

  let op = self.force_op_ptr(op).unwrap_or(op);

• rust/src/librustc/mir/interpret/allocation.rs

  Line 384 in 97b3d81

  if self.check_defined(ptr, size).is_err() {

  (fixed by avoid catching InterpError #70375)

The first one occurs quite a bit in Miri because we validate all the time (CTFE validates less often so this should be less of an issue, but still). I am not sure how best to fix it, short of implementing rust-lang/miri#841.

[RalfJung]

@wesleywiser do you still plan to submit this approach as a PR or would you like someone else to take over?

[wesleywiser]

@RalfJung Thanks for the reminder! I just posted a slightly more cleaned up version as #69888.

[RalfJung]

Status: last time I looked into this, I found some more cases where exceptions are being caught, that are related to handling of pointer provenance and ptr-int casts. I consider this problem blocked on rust-lang/miri#841.

[RalfJung]

I am also seeing many exceptions being raised and caught in ConstProp -- so, RUSTC_CTFE_BACKTRACE=1 is a serious slowdown for constprop. I am not sure if there is anything we can do about that... @oli-obk any idea?

Sadly this also masks any exceptions being raised elsewhere, making it really hard to figure out if there are non-constprop instances of this problem left.

[oli-obk]

I'm not sure const prop can do better other than duplicating all the checks that would lead to the exceptions being raised. We can do some half measures to eliminate obvious cases.

We could add a Drop impl to the error type and require all destructions of the errors to be made explicit. Not sure how annoying that would be in practice, but it could work well.

[RalfJung]

We could also do something awful and have some thread-local storage so this code

rust/compiler/rustc_middle/src/mir/interpret/error.rs

Line 116 in b919797

let backtrace = match capture_backtrace {

knows it is inside ConstProp and always treats this as Disabled.

[oli-obk]

We could also do something awful and have some thread-local storage so this code

rust/compiler/rustc_middle/src/mir/interpret/error.rs

Line 116 in b919797

let backtrace = match capture_backtrace {

knows it is inside ConstProp and always treats this as Disabled.

and then re-enable (in a scoped manner) when inside the CTFE query. Seems hacky and annyoing and actually manageable. I think it's fine, it's just for the backtrace printing, nothing that actually affects anyone that is not already debugging CTFE

[RalfJung]

I think I'll just use a locally patched rustc that does not do const-prop.^^ But we should maybe make a note that RUST_CTFE_BACKTRACE=1 currently is rather expensive since it captures a lot of backtraces during const-prop. I'll open an issue for that.
EDIT: actually... not sure if a perf issue that only arises during debugging is really worth an issue. 🤷

Regarding the issue at hand, the following are the only Miri test cases that have any exceptions caught and discarded. :)

    [ui] run-pass/exit.rs
    [ui] run-pass/issue-miri-1075.rs

[RalfJung]

Ah, and those are expected, since they are using throw_machine_stop!.

So I think this issue is solved. :-) We don't have a strategy in place to detect when it arises again in the future, but still I think we can close this one now.