Fix MIR pretty printer for non-local DefIds

[osa1]

Tries to fix #81200 -- the reproducer in the issue is not fixed yet.
Submitting PR to get feedback.

r? oli-obk

[rust-highfive]

r? @estebank

(rust-highfive has picked a reviewer for you, use r? to override)

[osa1]

r? @oli-obk

This doesn't quite fix the reproducer yet, but at least some of the tests don't panic anymore. @oli-obk could you comment on whether this is what you had in mind in your comment?

[oli-obk]

Hmm this would work mostly (we won't see any more mir for ctfe for const fn though).

What my comment in #81200 (comment) was referring to was to not make any difference between local MIR and MIR from other crates. So you can invoke tcx.is_const_fn_raw(def_id). If it returns true, then you run the code in the Some(rustc_hir::ConstContext::ConstFn) branch below, if it is false, you run the code you have now added (using instance_mir.

The one thing I don't know is whether we should attempt to use try_lookup instead of unknown if the DefId is local. That could potentially resolve the other failures you're seeing.

[osa1]

@oli-obk thanks for the review. I tried this

        // For `const fn` we want to render the optimized MIR. If you want the mir used in
        // ctfe, you can dump the MIR after the `Deaggregator` optimization pass.
        if tcx.is_const_fn_raw(def_id) {
            render_body(w, tcx.optimized_mir(def_id))?;
            writeln!(w)?;
            writeln!(w, "// MIR FOR CTFE")?;
            // Do not use `render_body`, as that would render the promoteds again, but these
            // are shared between mir_for_ctfe and optimized_mir
            write_mir_fn(tcx, tcx.mir_for_ctfe(def_id), &mut |_, _| Ok(()), w)?;
        } else {
            match def_id.as_local() {
                None => {
                    let instance_mir = tcx.instance_mir(ty::InstanceDef::Item(
                        ty::WithOptConstParam::unknown(def_id),
                    ));
                    render_body(w, instance_mir)?;
                }
                Some(local_def_id) => match tcx.hir().body_const_context(local_def_id) {
                    None => render_body(w, tcx.optimized_mir(def_id))?,
                    Some(_) => render_body(w, tcx.mir_for_ctfe(def_id))?,
                }
            }
        }

but it still doesn't fix MIRAI tests (though they no longer panic in rustc code).

I'm trying to understand what the strategy should be here. Just to make sure my understanding of the problem is accurate: previously the MIR printer would panic if the requested definition is not local. Normally I'm guessing write_mir_pretty is used for the crate being compiled (e.g. with --emit=mir) so that's fine. But the function is publicly available so projects like MIRAI use it, and pass non-local DefIds, causing it to panic.

Is this correct so far?

Now, I'm curious how did this work before. We have two cases:

• For local definition, I'm guessing we have MIRs for all functions, right? However we still have two options: (1) print optimized MIR (2) print CTFE MIR. I'm guessing (1) is always available but (2) is not. If both are available, which one should we print, and why?

• For non-local definitions, MIR is available for (1) inlinable functions (2) const functions (3) all functions with -Z always_encode_mir. In some of these cases we will again have one of both of (1) optimzied MIR (2) CTFE MIR. Which one should we print, and why?

I'm also curious what "instance MIR" (the instance_mir query) is. Could you say a few words about this?

[oli-obk]

* If both are available, which one should we print, and why?

The answer is both, for local and non-local definitions. A const fn should always print both MIR bodies.

I'm also curious what "instance MIR" (the instance_mir query) is. Could you say a few words about this?

There are more MIR bodies than definitions. Some things like shims (automatically generated bodies) require some way to identify them. Instances can either be InstanceDef::Item which is what you already know (just a DefId), or one of the many variants of that enum referring to specific shims (e.g. drop glue).

but it still doesn't fix MIRAI tests (though they no longer panic in rustc code).

Maybe the tests are wrong? Idk how they are failing, so I'd just merge this PR and then MIRAI can figure things out on their end with a nightly compiler that doesn't ICE

[hermanventer]

Some of the MIRAI test failures would be the result of recent breaking changes in the Rust compiler. Debugging these failures is a nightmare without the ability to see human readable MIR for everything MIRAI is executing. As long as you are not seeing thread 'rustc' panicked at 'DefId::expect_local I should be able to proceed on my own once this lands and makes it into the nightly channel.

[hermanventer]

I've updated the PR with the reproducer. If you make a one character source change, all tests should pass (it does on 2021-02-12). See https://github.com/facebookexperimental/MIRAI/pull/754/files#r575573754 for what to change.

[osa1]

Thanks @oli-obk. One more question, in the current version of the code, how can the Some branch of match tcx.hir().body_const_context(local_def_id) { ... } ever be taken? I already have a is_const_fn_raw check before that match, so I was guessing I could simplify the code as

        // For `const fn` we want to render the optimized MIR. If you want the mir used in
        // ctfe, you can dump the MIR after the `Deaggregator` optimization pass.
        if tcx.is_const_fn_raw(def_id) {
            render_body(w, tcx.optimized_mir(def_id))?;
            writeln!(w)?;
            writeln!(w, "// MIR FOR CTFE")?;
            // Do not use `render_body`, as that would render the promoteds again, but these
            // are shared between mir_for_ctfe and optimized_mir
            write_mir_fn(tcx, tcx.mir_for_ctfe(def_id), &mut |_, _| Ok(()), w)?;
        } else {
            match def_id.as_local() {
                None => {
                    let instance_mir = tcx.instance_mir(ty::InstanceDef::Item(
                        ty::WithOptConstParam::unknown(def_id),
                    ));
                    render_body(w, instance_mir)?;
                }
                Some(_local_def_id) =>
                    render_body(w, tcx.optimized_mir(def_id))?,
            }
        }

because we already check if the function is a const fn before thsi branch, but this change causes MIRAI tests to panic in rustc code again.

(Note to self: in the None case, instance_mir function returns the optimized MIR when available)

In general, I'm still a bit confused about (1) what different kinds of MIRs we have (optimized, CTFE, shims, ...) (2) which ones are available when (3) which ones do we want to show here (I'm guessing all available ones?).

[bjorn3]

is_const_fn_raw only checks for const fn. body_const_context can also be Some for statics and consts.

[oli-obk]

@bors r+

Great! Thanks!

[bors]

📌 Commit fe82365 has been approved by oli-obk