Stream: t-compiler

Topic: REPL: dyn Debug intrinsic


oli (Nov 29 2019 at 15:58, on Zulip):

What does the intrinsic do exactly? create a dummy vtable printing nothing or a default value if there's no Debug impl?

oli (Nov 29 2019 at 16:01, on Zulip):

context: https://hackmd.io/GJokfI0wQ0i4SIgRbFTmfw

Alexander Regueiro (Nov 29 2019 at 16:06, on Zulip):

right, let me paste my code from the REPL repo

Alexander Regueiro (Nov 29 2019 at 16:08, on Zulip):

This is the handling of the new intrinsic, in the match stmt in call_intrinsic:

            "as_debug" => {
                let existential_trait_ref = match dest.layout.ty.sty {
                    ty::RawPtr(tm) => {
                        match tm.ty.sty {
                            ty::Dynamic(ref data, _) =>
                                data.principal()
                                    .expect("'debug trait' has no principal trait"),
                            _ => panic!("return type pointee is not dynamic type"),
                        }
                    }
                    _ => panic!("return type is not raw pointer"),
                };

                let pointee_ty = substs.type_at(0);
                let trait_predicate = existential_trait_ref.with_self_ty(*tcx, pointee_ty).to_predicate();
                let val = if evaluate_predicate(tcx, instance, trait_predicate) {
                    let ptr = this.read_immediate(args[0])?.to_scalar_ptr()?;
                    let vtable = this.get_vtable(pointee_ty, Some(existential_trait_ref))?;
                    Immediate::new_dyn_trait(ptr, vtable)
                } else {
                    let vtable = this.get_vtable(pointee_ty, None)?;
                    Immediate::new_dyn_trait(Scalar::ptr_null(this), vtable)
                };
                this.write_immediate(val, dest)?;
            }
Alexander Regueiro (Nov 29 2019 at 16:08, on Zulip):

and a helper fn:

fn evaluate_predicate<'tcx>(
    tcx: &TyCtxt<'tcx>,
    instance: Instance<'tcx>,
    predicate: Predicate<'tcx>,
) -> bool {
    // FIXME: is this really necessary? Maybe we can just use `DUMMY_HIR_ID` instead.
    // Or use `Obligation::new` and `ObligationCause::dummy`?
    let body_id = tcx.hir().as_local_hir_id(instance.def_id())
        .expect("`instance` is not in local crate");
    let obligation = Obligation::misc(DUMMY_SP, body_id, ParamEnv::reveal_all(), predicate);
    tcx.infer_ctxt().enter(|infer_ctxt| {
        match infer_ctxt.evaluate_obligation(&obligation) {
            Ok(EvaluationResult::EvaluatedToOk) |
            Ok(EvaluationResult::EvaluatedToOkModuloRegions) => true,
            Ok(_) => false,
            Err(_) => false,
        }
    })
}
Alexander Regueiro (Nov 29 2019 at 16:10, on Zulip):

alternatively (as I believe I mentioned in the design doc), the intrinsic could save the debug representation to a field in the Evaluator, and let the REPL do the printing of its own accord.

Alexander Regueiro (Nov 29 2019 at 16:10, on Zulip):

this might make (e.g.) formatted/coloured printing, depending on settings of the REPL, a bit easier.

Alexander Regueiro (Nov 29 2019 at 16:10, on Zulip):

obviously a minor point though.

oli (Nov 29 2019 at 16:12, on Zulip):

so... what debug impl is being used in case there is no debug impl?

oli (Nov 29 2019 at 16:12, on Zulip):

the pointer to the data is printed?

oli (Nov 29 2019 at 16:12, on Zulip):

no, it looks like a null ponter, but I don't get what the vtable is

oli (Nov 29 2019 at 16:13, on Zulip):

sadly specialization doesn't work for this case it seems, otherwise we could use https://play.rust-lang.org/?version=nightly&mode=debug&edition=2018&gist=faaa84ebb20225679401d0c7cc108149

Alexander Regueiro (Nov 29 2019 at 16:14, on Zulip):

yes, indeed. I experimented with that before trying this.

Alexander Regueiro (Nov 29 2019 at 16:15, on Zulip):

so, right now, the main fn (template) looks like this:

    fn main() {
        let user_body = #[rustc_interp_user_fn] || { user_body_placeholder };

        #[allow(unused)]
        let ret = user_body();
        let debug = as_debug(&ret);
        if let Some(debug) = debug {
            println!("{:?}", debug);
        } else {
            println!("[{}]", type_name(&ret));
        }
    }
oli (Nov 29 2019 at 16:16, on Zulip):

oh, the null turns into None :upside_down: ok

Alexander Regueiro (Nov 29 2019 at 16:16, on Zulip):

yep heh

oli (Nov 29 2019 at 16:16, on Zulip):

seems good to me, but needs a lot of comments so future readers get all that

Alexander Regueiro (Nov 29 2019 at 16:17, on Zulip):

I will change user_body_placeholder to be a macro call however, and do the pre-parsing of the whole template in the REPL itself (with an even more minor extension to the Parser interface than present), which I think people prefer.

Alexander Regueiro (Nov 29 2019 at 16:17, on Zulip):

yeah, no problem. more comments is understandable.

Alexander Regueiro (Nov 29 2019 at 16:28, on Zulip):

anyway, thanks for giving the nod to a lot of this REPL design stuff and helping to move it along, in spite of our differences on some matters. I will r? you (or RalfJung, I guess) on the mir/mir-interpreter-related rustc PR when the time arises, if that's alright.

oli (Nov 29 2019 at 16:30, on Zulip):

yea, as always, please only add functional changes to the PRs and make one PR per "topic" (e.g. one for Machine changes, one for command line flag + new attributes). We'll be able to move along much faster with individual PRs

Alexander Regueiro (Nov 29 2019 at 16:30, on Zulip):

will do

oli (Nov 29 2019 at 16:30, on Zulip):

Thanks!

Alexander Regueiro (Nov 29 2019 at 16:32, on Zulip):

if there are any formatting changes or changes to comments, I'll make sure they're only clear improvements that are directly next to functional changes. and they shan't be many. I will have a pass over my existing PRs to remove extraneous "cosmetic" stuff, but if I forget, just let me know matter-of-factly, because it will probably be that I've simply overlooked them. :-)

oli (Nov 29 2019 at 16:32, on Zulip):

sgtm

Alexander Regueiro (Nov 29 2019 at 16:33, on Zulip):

;-)

Alexander Regueiro (Nov 29 2019 at 16:34, on Zulip):

don't worry: I've just gotten this design doc essentially approved. the last thing I want to do is annoy reviewers or make getting PRs approved a pain for all of us!

Nadrieril (Nov 30 2019 at 11:36, on Zulip):

sadly specialization doesn't work for this case it seems, otherwise we could use https://play.rust-lang.org/?version=nightly&mode=debug&edition=2018&gist=faaa84ebb20225679401d0c7cc108149

Does it not ? I think you just omitted a default https://play.rust-lang.org/?version=nightly&mode=debug&edition=2018&gist=bc90cd6d61e11d07cd9b3517bdf38940

oli (Nov 30 2019 at 12:04, on Zulip):

Awesome, you don't even need the DebugEverything trait since it's implemented for all types. https://play.rust-lang.org/?version=nightly&mode=debug&edition=2018&gist=83de0447fe2d0bc2eda7fc27c5437dcc

oli (Nov 30 2019 at 12:06, on Zulip):

@Alexander Regueiro you can get rid of the intrinsic by putting the DebugEverything trait in your generated file (or injecting a dependency on a crate that implements it, not sure whether the current REPL can import crates)

Alexander Regueiro (Nov 30 2019 at 15:23, on Zulip):

@oli yeah, that's not bad, but the thing is a) I don't want to depend on nightly, b) I still want a way to "hand back" string values to the REPL itself, so some sort of intrinsic would be neded for that.

oli (Nov 30 2019 at 15:29, on Zulip):

Not sure why b) would be affected?

oli (Nov 30 2019 at 15:31, on Zulip):

Right now you get a *const dyn Debug, with this you'd get a &dyn DebugEverything. In both cases you have to call the fmt method

oli (Nov 30 2019 at 15:33, on Zulip):

Wrt a) are you worried the specialization feature gate will break/disappear?

Alexander Regueiro (Nov 30 2019 at 15:34, on Zulip):

no, I just want people to be able to use the REPL without nightly. so they can specify the version/toolchain, ideally. :-)

Alexander Regueiro (Nov 30 2019 at 15:35, on Zulip):

b) because I don't want to just print from within the evaluated code, but also hand back strings to the REPL, so it can use them as it pleases (maybe just format and then print them according to settings, maybe something fancY)

oli (Nov 30 2019 at 16:41, on Zulip):

I understand what you're saying wrt b), but the intrinsic doesn't give any string to the REPL

oli (Nov 30 2019 at 16:42, on Zulip):

since you're calling the intrinsic, you're forever stuck on nightly anyway

oli (Nov 30 2019 at 16:44, on Zulip):

activating a feature gate for an intrinsic or for a feature makes no difference as far as i can tell

oli (Nov 30 2019 at 16:47, on Zulip):

wrt b) I think I'm missing some information, thus don't get what the difference between as_debug(&value) and value as *DebugEverything is (except for the None result, but that is resolved by the default impl)

oli (Nov 30 2019 at 16:47, on Zulip):

Or maybe you're thinking about future changes?

bjorn3 (Nov 30 2019 at 18:03, on Zulip):

Making it work on stable should be possible by using hygiene to pretend that the respective code came from a macro which allowed the necessary feature gates, like:

https://github.com/rust-lang/rust/blob/d8bdb3fdcbd88eb16e1a6669236122c41ed2aed3/src/libstd/thread/local.rs#L127

oli (Nov 30 2019 at 18:15, on Zulip):

the use of that attribute also requires feature gates, so you don't gain anything (unless we put it in libcore and stabilize it, which we won't do for an internal thing)

bjorn3 (Nov 30 2019 at 18:35, on Zulip):

I meant that the REPL changes the parsed syntax so that it looks like it came from a macro with that attribute.

oli (Nov 30 2019 at 18:36, on Zulip):

right, but that applies to either solution :slight_smile:

Alexander Regueiro (Nov 30 2019 at 20:53, on Zulip):

@oli yeah, I forgot about that, oops. I still very a little uneasy about the specialisation-based approach... I remember discussing this with @centril (he implemented something similar to DebugEverything trait?) and deciding it couldn't work properly, but I forget why exactly... perhaps I was mistaken.

Alexander Regueiro (Nov 30 2019 at 20:54, on Zulip):

as for b), the point is that it's nice to have an intrinsic for being able to pass some string value to the Miri evaluator.

Alexander Regueiro (Nov 30 2019 at 20:54, on Zulip):

which would be different from as_debug, sure.

Alexander Regueiro (Nov 30 2019 at 20:54, on Zulip):

(it could be part of the same intrinsic, or a separate one, rather)

Matthew Jasper (Nov 30 2019 at 21:02, on Zulip):

I'm not sure if this is what Centril found, but specialization on traits (as implemented by rustc) is unsound and this can even be exploited by downstream crates without the feature.

Matthew Jasper (Nov 30 2019 at 21:05, on Zulip):

I don't think that your intrinsic avoids this problem though.

Alexander Regueiro (Nov 30 2019 at 21:07, on Zulip):

aha

Alexander Regueiro (Nov 30 2019 at 21:07, on Zulip):

probably not, but that's a fair point too

Alexander Regueiro (Nov 30 2019 at 21:08, on Zulip):

@Matthew Jasper what's the problem with the intrinsic, as you see it?

Matthew Jasper (Nov 30 2019 at 21:10, on Zulip):

It would probably need to do something similar to generator witnesses, where all of the regions are replaced with regions that are bound by the binder around the predicate.

Matthew Jasper (Nov 30 2019 at 21:10, on Zulip):

The issue is that it's only checking that there are some lifetimes where the predicate holds.

Matthew Jasper (Nov 30 2019 at 21:11, on Zulip):

Maybe the unusual nature of the repl works around this somehow.

Alexander Regueiro (Nov 30 2019 at 21:52, on Zulip):

hmm

Alexander Regueiro (Nov 30 2019 at 21:53, on Zulip):

@Matthew Jasper yes possibly.

Alexander Regueiro (Nov 30 2019 at 21:53, on Zulip):

how would this look in code?

Matthew Jasper (Nov 30 2019 at 21:54, on Zulip):

impl Debug for A<'static> { /* impl that requires 'static */ } and then debug printing an A<'short>

Matthew Jasper (Nov 30 2019 at 21:55, on Zulip):

It's fairly fiddly to create a use after free from this.

bjorn3 (Nov 30 2019 at 21:57, on Zulip):

A could contain a stack reference and the Debug impl could store it in a static.

bjorn3 (Nov 30 2019 at 21:57, on Zulip):

That way a dangling reference is made

oli (Nov 30 2019 at 22:26, on Zulip):

The main reason I like the specialization approach over the intrinsic is that it's much easier to grok the Rust code than the rustc code emulating it

oli (Nov 30 2019 at 22:26, on Zulip):

the unsoundness is irrelevant for this use case, miri won't allow you to do anything stupid

oli (Nov 30 2019 at 22:27, on Zulip):

so even if you build an impl that leaks a local value, once it's deallocated and you try to use it you'll get an error

oli (Nov 30 2019 at 22:27, on Zulip):

even without stacked borrows

oli (Nov 30 2019 at 22:29, on Zulip):

there's also no need for an intrinsic for the "give a value back to the repl", because you can just define an arbitrary extern "C" function with a name noone will use and hook that

oli (Nov 30 2019 at 22:29, on Zulip):

no need for rustc to know about it

Alexander Regueiro (Nov 30 2019 at 22:39, on Zulip):

I suppose so.

Alexander Regueiro (Nov 30 2019 at 22:39, on Zulip):

An intrinsic seems somehow "neater" though...

Alexander Regueiro (Nov 30 2019 at 22:39, on Zulip):

arguably less hacky?

oli (Nov 30 2019 at 22:42, on Zulip):

not hackier than miri supporting puts

Alexander Regueiro (Nov 30 2019 at 23:20, on Zulip):

heh

Alexander Regueiro (Nov 30 2019 at 23:21, on Zulip):

I suppose. It just feels semantically like it should be an intrinsic... hmm.

Alexander Regueiro (Nov 30 2019 at 23:21, on Zulip):

Let me ponder it a bit.

Alexander Regueiro (Dec 03 2019 at 03:17, on Zulip):

@oli Can't say I've changed my mind about it being hacky... but I do see why you might be drawn to this, since it requires no rustc changes (not that the rustc changes would be anything but tiny). As for puts, I consider that a bit different, since it's meant to be a C stdlib fn that is normally called via the C ABI. But miri wouldn' t be emulating a C fn / ABI call here. It's very much something that only makes sense to use within the REPL. :-)

oli (Dec 03 2019 at 07:13, on Zulip):

There's another advntage of it not being in rustc: modifying its API does not require you to touch rustc, giving you more freedom

Alexander Regueiro (Dec 03 2019 at 16:26, on Zulip):

@oli That's true actually... I mean, making a PR to rustc is pretty easy, but you have a fair point.

Alexander Regueiro (Dec 03 2019 at 16:26, on Zulip):

@oli can the fn just be "extern" and not "extern C", in fact?

bjorn3 (Dec 03 2019 at 16:32, on Zulip):

impl Debug for A<'static> { /* impl that requires 'static */ } and then debug printing an A<'short>

Maybe replace all ReErased with fresh lifetimes? I think that will require the Debug impl to be valid for all lifetimes, thus fixing the unsoundness.

bjorn3 (Dec 03 2019 at 16:37, on Zulip):

I prefer as_debug to be an intrinsic, as all functions handled specially by both cg_llvm and cg_clif are either lang items or intrinsics. Miri does handle some extern "C" functions specially, but those don't perform magic, but are only shims. If the code wasn't running in an interpreter, those functions could be implemented without the help of the compiler. as_debug however is magic, as it inspects if a type implements Debug.

oli (Dec 03 2019 at 17:16, on Zulip):

oli can the fn just be "extern" and not "extern C", in fact?

yes, it does not have to be "C", but that makes it easier to use from miri

Alexander Regueiro (Dec 03 2019 at 17:24, on Zulip):

hmm, okay. makes it easier because the ABI is simpler? Miri can surely use the Rust ABI though?

oli (Dec 03 2019 at 17:25, on Zulip):

sure, but you need to shim it. Most of the time that is totally not a problem though.

oli (Dec 03 2019 at 17:26, on Zulip):

and for your REPL you are right, you are in control anyway, so you can just us the Rust ABI

Alexander Regueiro (Dec 03 2019 at 17:31, on Zulip):

cool

Alexander Regueiro (Dec 03 2019 at 17:31, on Zulip):

okay, you've won me over

Alexander Regueiro (Dec 03 2019 at 17:31, on Zulip):

let's do that.

Alexander Regueiro (Dec 03 2019 at 17:33, on Zulip):

(unless anyone else has objections...)

Last update: Dec 12 2019 at 01:35UTC