Stream: t-compiler/wg-mir-opt

Topic: Box doesn't respect self alignment #68304


Santiago Pastorino (Apr 14 2020 at 14:02, on Zulip):

@nikomatsakis :wave:

nikomatsakis (Apr 14 2020 at 14:02, on Zulip):

oh heh

nikomatsakis (Apr 14 2020 at 14:03, on Zulip):

let's just use this topic

nikomatsakis (Apr 14 2020 at 14:04, on Zulip):

I'm just re-skimming the comments

Santiago Pastorino (Apr 14 2020 at 14:04, on Zulip):

so I got that the issue is because ...

nikomatsakis (Apr 14 2020 at 14:04, on Zulip):

I realize I dont' think we've 100% written out the 'new plan'

nikomatsakis (Apr 14 2020 at 14:05, on Zulip):

Santiago Pastorino said:

so I got that the issue is because ...

yes, let's start with that

Santiago Pastorino (Apr 14 2020 at 14:05, on Zulip):

repr(align(256)) is not respected when Box<dyn FnOnce() ...> because that copies self to the stack using the default alignment of 16 instead of 256. This happens because in this case, self is an unsized local and alloca only support constant arguments, not dynamic ones.

Santiago Pastorino (Apr 14 2020 at 14:06, on Zulip):

that's the summary I got from the issue

nikomatsakis (Apr 14 2020 at 14:06, on Zulip):

that sounds right

nikomatsakis (Apr 14 2020 at 14:07, on Zulip):

but also, we would prefer to have a way to invoke Box<FnOnce> that doesn't copy the argument at all

Santiago Pastorino (Apr 14 2020 at 14:07, on Zulip):

what can be done to alleviate that is the question, I saw you were commenting about some stuff on mir building

Santiago Pastorino (Apr 14 2020 at 14:07, on Zulip):

nikomatsakis said:

but also, we would prefer to have a way to invoke Box<FnOnce> that doesn't copy the argument at all

yeah, then you won't have the problem

Santiago Pastorino (Apr 14 2020 at 14:08, on Zulip):

nikomatsakis said:

but also, we would prefer to have a way to invoke Box<FnOnce> that doesn't copy the argument at all

if it's not copying the argument is it reusing it from the previous context?

nikomatsakis (Apr 14 2020 at 14:09, on Zulip):

Initially, when we were contemplating 'unsized locals', the idea was only to support cases where no copying is required. Then we discussed extending it to support some interesting alloca use cases, but ideally with "guaranteed optimizations" that avoid copying if you stick to certain patterns. As far as I know, those guaranteed opts do not exist.

nikomatsakis (Apr 14 2020 at 14:09, on Zulip):

Yes, the idea would be that you are passing a pointer to the place where the unsized argument exists now

nikomatsakis (Apr 14 2020 at 14:09, on Zulip):

Which, side note, does raise a question that I hadn't considered that much -- I'm not sure about the "backend" part of this. ie., what I was describing were changes to MIR building

nikomatsakis (Apr 14 2020 at 14:10, on Zulip):

such that instead of generating this MIR:

nikomatsakis (Apr 14 2020 at 14:10, on Zulip):
tmp0 = *x // tmp0: dyn FnOnce()
FnOnce::call(tmp0, ())
nikomatsakis (Apr 14 2020 at 14:10, on Zulip):

you generate MIR like

nikomatsakis (Apr 14 2020 at 14:10, on Zulip):
tmp0 = x // tmp0: Box<dyn FnOnce()>
FnOnce::call(*tmp0, ())
nikomatsakis (Apr 14 2020 at 14:11, on Zulip):

I guess it's kind of "on the backend" to pass that first argument "by reference", in other words, we're passing a pointer into the Box and not to a temp on the stack

Santiago Pastorino (Apr 14 2020 at 14:11, on Zulip):

hmm unsure I got the difference, wouldn't you want to pass tmp0 and then when referring to the value always dereference insing the FnOnce?

nikomatsakis (Apr 14 2020 at 14:13, on Zulip):

This is in some sense the point -- and maybe @eddyb can weigh in a bit here --

nikomatsakis (Apr 14 2020 at 14:13, on Zulip):

the callee should be invoked with what is essentially a "fat pointer" to the dyn FnOnce

nikomatsakis (Apr 14 2020 at 14:14, on Zulip):

it doesn't have to know that whether this is found in a Box or what

eddyb (Apr 14 2020 at 14:14, on Zulip):

you can't change the signature of FnOnce::call

eddyb (Apr 14 2020 at 14:15, on Zulip):

but the ABI allows copy-less passing of unsized values (or any values, really)

nikomatsakis (Apr 14 2020 at 14:15, on Zulip):

in particular the fn in question is <dyn FnOnce(...) as FnOnce(...)>::call

Santiago Pastorino (Apr 14 2020 at 14:15, on Zulip):

ahh I see

nikomatsakis (Apr 14 2020 at 14:15, on Zulip):

so I guess that, in the case of parameters with unsize type, we expect a reference to them to be passed in, right?

nikomatsakis (Apr 14 2020 at 14:15, on Zulip):

(and perhaps other types)

nikomatsakis (Apr 14 2020 at 14:16, on Zulip):

(since the parameter here is self: dyn FnOnce(...))

Santiago Pastorino (Apr 14 2020 at 14:17, on Zulip):

there is some kind of magic going on that I don't see how happens here

eddyb (Apr 14 2020 at 14:18, on Zulip):

anything other than Scalar/Vector/ScalarPair is just passed by reference at the ABI level

Santiago Pastorino (Apr 14 2020 at 14:18, on Zulip):

so FnOnce expects a param that is self: dyn FnOnce(...) but with some sort of magic we pass a reference to that instead?

eddyb (Apr 14 2020 at 14:18, on Zulip):

so Operand::Move(_123) as a call argument will result in the argument local in the callee point to _123 in the caller's stack frame

Santiago Pastorino (Apr 14 2020 at 14:19, on Zulip):

eddyb said:

anything other than Scalar/Vector/ScalarPair is just passed by reference at the ABI level

ahh ok :), I guess that's the magic I wasn't seeing :)

eddyb (Apr 14 2020 at 14:19, on Zulip):

Operand::Move(*some_box) will result in the argument local in the callee point to the heap

Santiago Pastorino (Apr 14 2020 at 14:19, on Zulip):

ok, makes sense

eddyb (Apr 14 2020 at 14:19, on Zulip):

(again, assuming it's not a scalar/vector/scalar-pair - which can be passed "by value", in registers or on the stack)

Santiago Pastorino (Apr 14 2020 at 14:20, on Zulip):

yeah, so what's the code doing right now?, given what you're saying right now I guess is a scalar/vector/scalar-pair?

eddyb (Apr 14 2020 at 14:20, on Zulip):

dyn Trait is not scalar/vector/scalar-pair :P

eddyb (Apr 14 2020 at 14:20, on Zulip):

since we don't know what the type is

Santiago Pastorino (Apr 14 2020 at 14:20, on Zulip):

yeah but what I was asking is how is then behaving like that according to what you're saying?

eddyb (Apr 14 2020 at 14:21, on Zulip):

how is what behaving?

eddyb (Apr 14 2020 at 14:21, on Zulip):

and in what way?

Santiago Pastorino (Apr 14 2020 at 14:21, on Zulip):

so maybe my interpretation of what you're saying is wrong ...

eddyb (Apr 14 2020 at 14:21, on Zulip):

I'm saying this is what the Rust ABI is

eddyb (Apr 14 2020 at 14:21, on Zulip):

and has been for years

eddyb (Apr 14 2020 at 14:22, on Zulip):

it will not require any changes

Santiago Pastorino (Apr 14 2020 at 14:22, on Zulip):

yeah, I'm reading you wrong probably

Santiago Pastorino (Apr 14 2020 at 14:22, on Zulip):

so self is being copied into the stack

nikomatsakis (Apr 14 2020 at 14:22, on Zulip):

(PS, I was distracted writing up this comment, which tries to collect the proposal into a single write-up, versus diffs on older write-ups, but so far what @eddyb is saying matches my expectations, I think)

eddyb (Apr 14 2020 at 14:22, on Zulip):

@Santiago Pastorino by what and when?

Santiago Pastorino (Apr 14 2020 at 14:22, on Zulip):

in this case

eddyb (Apr 14 2020 at 14:23, on Zulip):

the status quo or what we want to change it to?

Santiago Pastorino (Apr 14 2020 at 14:23, on Zulip):

the status quo

eddyb (Apr 14 2020 at 14:23, on Zulip):

the status quo copies in the caller

Santiago Pastorino (Apr 14 2020 at 14:23, on Zulip):

yeah

eddyb (Apr 14 2020 at 14:23, on Zulip):

before passing by reference the copied local

nikomatsakis (Apr 14 2020 at 14:23, on Zulip):

in particular, in the

tmp = *x

line of the MIR

Santiago Pastorino (Apr 14 2020 at 14:24, on Zulip):

so I was interpreting that you were saying that only scalar/vector/scalar-pair are copied

Santiago Pastorino (Apr 14 2020 at 14:24, on Zulip):

that's what have confused me

eddyb (Apr 14 2020 at 14:24, on Zulip):

I was talking about calls

eddyb (Apr 14 2020 at 14:24, on Zulip):

= always copies

eddyb (Apr 14 2020 at 14:24, on Zulip):

fn_once(move tmp) doesn't do any more copies

eddyb (Apr 14 2020 at 14:24, on Zulip):

(at least I hope it doesn't, lol)

eddyb (Apr 14 2020 at 14:24, on Zulip):

it's the = we want to get rid of

Santiago Pastorino (Apr 14 2020 at 14:25, on Zulip):

:+1:

nikomatsakis (Apr 14 2020 at 14:26, on Zulip):

OK, @Santiago Pastorino, so the goal is "clear-ish" now?

Santiago Pastorino (Apr 14 2020 at 14:26, on Zulip):

was reading the comment

nikomatsakis (Apr 14 2020 at 14:26, on Zulip):

also, does what I wrote in this comment make sense to you?

Santiago Pastorino (Apr 14 2020 at 14:26, on Zulip):

I think so

Santiago Pastorino (Apr 14 2020 at 14:26, on Zulip):

let me finish reading it

Santiago Pastorino (Apr 14 2020 at 14:27, on Zulip):

yeah makes sense, in any case I will ask some questions

Santiago Pastorino (Apr 14 2020 at 14:27, on Zulip):

thanks for clarifying it

nikomatsakis (Apr 14 2020 at 14:27, on Zulip):

ok, so I was going to drop a few pointers into where I think we would modify the code

nikomatsakis (Apr 14 2020 at 14:28, on Zulip):

though you may be more familiar with it than me at this point!

Santiago Pastorino (Apr 14 2020 at 14:29, on Zulip):

feel free to add more information if you want, I guess I'd need to investigate a couple of things to be 100% clear about how to do certain checks

nikomatsakis (Apr 14 2020 at 14:29, on Zulip):

well so this is the main place that calls are lowered

nikomatsakis (Apr 14 2020 at 14:30, on Zulip):

in particular, this loop is what generates the temporaries for each argument

nikomatsakis (Apr 14 2020 at 14:31, on Zulip):

it's not going to be trivial to modify this and have the code stay "beautiful"

nikomatsakis (Apr 14 2020 at 14:32, on Zulip):

well I guess we need some operation simiar to as_local_operand, like as_call_temporary

nikomatsakis (Apr 14 2020 at 14:33, on Zulip):

as_local_operand is defined there

nikomatsakis (Apr 14 2020 at 14:34, on Zulip):

and you can see that, after some intermediate calls, it winds up checking the "category" of the expression

nikomatsakis (Apr 14 2020 at 14:34, on Zulip):

(in particular, it distinguishes places from other things)

nikomatsakis (Apr 14 2020 at 14:35, on Zulip):

well, I guess we would want to look for ExprKind::Deref

Santiago Pastorino (Apr 14 2020 at 21:45, on Zulip):

@nikomatsakis @eddyb I was wondering a couple of things about this

Santiago Pastorino (Apr 14 2020 at 21:45, on Zulip):

first how do I check if this is not Move

Santiago Pastorino (Apr 14 2020 at 21:46, on Zulip):

@eddyb mentioned that maybe this should be done in expr itself, checking that expr is Copy and Sized

Santiago Pastorino (Apr 14 2020 at 21:46, on Zulip):

and also if it's Deref

eddyb (Apr 14 2020 at 21:47, on Zulip):

that it doesn't implement Copy, so there's a move

eddyb (Apr 14 2020 at 21:47, on Zulip):

but I think Sized is enough since, for now, !Sized implies !Copy (kind of sad but w/e)

eddyb (Apr 14 2020 at 21:48, on Zulip):

as for Deref, it wouldn't be the trait, but rather a syntactical * operator in the Expr

Santiago Pastorino (Apr 14 2020 at 21:49, on Zulip):

yeah, Deref checking is just checking if expr.kind is ExprKind::Deref

Santiago Pastorino (Apr 14 2020 at 21:50, on Zulip):

saw also that @RalfJ commented https://github.com/rust-lang/rust/issues/68304#issuecomment-613691940

eddyb (Apr 14 2020 at 21:50, on Zulip):

in fact, if you look for how Operand::Move vs Operand::Copy is decided, you should find something like is_copy_modulo_regions

Santiago Pastorino (Apr 14 2020 at 21:50, on Zulip):

ahh nice, gonna check that out

Santiago Pastorino (Apr 14 2020 at 21:50, on Zulip):

will do this tomorrow, it's late here already, but thanks for the tips

RalfJ (Apr 15 2020 at 09:51, on Zulip):

Santiago Pastorino said:

saw also that @**RalfJ** commented

sorry, I was just confused

Santiago Pastorino (Apr 15 2020 at 15:46, on Zulip):

@eddyb @nikomatsakis #71170, although it doesn't work, just opened as a WIP

Santiago Pastorino (Apr 15 2020 at 15:47, on Zulip):

feedback is welcome though

Santiago Pastorino (Apr 15 2020 at 16:18, on Zulip):

figured that I forgot to add the *tmp part

Santiago Pastorino (Apr 15 2020 at 16:54, on Zulip):

@nikomatsakis @eddyb did this and didn't work

Santiago Pastorino (Apr 15 2020 at 16:54, on Zulip):

have pushed all the stuff I have, there's code duplication that I was planning to remove but didn't bother that much to do it because it's not working

Santiago Pastorino (Apr 15 2020 at 16:55, on Zulip):

basically expr_as_call_temporary is the same as expr_as_operand but it just adds the Deref bit

Santiago Pastorino (Apr 16 2020 at 19:19, on Zulip):
[santiago@galago mir_dump (dyn-fnonce-alignment)]$ cat rustc.f2-\{\{closure\}\}.-------.mir_map.0.mir
// MIR for `f2::{{closure}}#0` 0 mir_map

fn f2::{{closure}}#0(_1: [closure@src/test/ui/fn/dyn-fn-alignment.rs:18:14: 18:27 a:A]) -> *const A {
    debug a => (_1.0: A);                // in scope 0 at src/test/ui/fn/dyn-fn-alignment.rs:17:9: 17:10
    let mut _0: *const A;                // return place in scope 0 at src/test/ui/fn/dyn-fn-alignment.rs:18:22: 18:22
    let mut _2: &A;                      // in scope 0 at src/test/ui/fn/dyn-fn-alignment.rs:18:22: 18:23

    bb0: {
        StorageLive(_2);                 // bb0[0]: scope 0 at src/test/ui/fn/dyn-fn-alignment.rs:18:22: 18:23
        _2 = &(_1.0: A);                 // bb0[1]: scope 0 at src/test/ui/fn/dyn-fn-alignment.rs:18:22: 18:23
        _0 = const A::f(move _2) -> [return: bb2, unwind: bb1]; // bb0[2]: scope 0 at src/test/ui/fn/dyn-fn-alignment.rs:18:22: 18:27
                                         // ty::Const
                                         // + ty: for<'r> fn(&'r A) -> *const A {A::f}
                                         // + val: Value(Scalar(<ZST>))
                                         // mir::Constant
                                         // + span: src/test/ui/fn/dyn-fn-alignment.rs:18:24: 18:25
                                         // + literal: Const { ty: for<'r> fn(&'r A) -> *const A {A::f}, val: Value(Scalar(<ZST>)) }
    }

    bb1 (cleanup): {
        resume;                          // bb1[0]: scope 0 at src/test/ui/fn/dyn-fn-alignment.rs:18:14: 18:27
    }

    bb2: {
        StorageDead(_2);                 // bb2[0]: scope 0 at src/test/ui/fn/dyn-fn-alignment.rs:18:26: 18:27
        goto -> bb3;                     // bb2[1]: scope 0 at src/test/ui/fn/dyn-fn-alignment.rs:18:27: 18:27
    }

    bb3: {
        return;                          // bb3[0]: scope 0 at src/test/ui/fn/dyn-fn-alignment.rs:18:27: 18:27
    }
}
Santiago Pastorino (Apr 16 2020 at 19:19, on Zulip):

@nikomatsakis :point_up:

Santiago Pastorino (Apr 16 2020 at 19:33, on Zulip):

and ...

Santiago Pastorino (Apr 16 2020 at 19:34, on Zulip):
// MIR for `f2::{{closure}}#0` 0 mir_map

fn f2::{{closure}}#0(_1: [closure@src/test/ui/fn/dyn-fn-alignment.rs:18:14: 18:27 a:A]) -> *const A {
    debug a => (_1.0: A);                // in scope 0 at src/test/ui/fn/dyn-fn-alignment.rs:17:9: 17:10
    let mut _0: *const A;                // return place in scope 0 at src/test/ui/fn/dyn-fn-alignment.rs:18:22: 18:22
    let mut _2: &A;                      // in scope 0 at src/test/ui/fn/dyn-fn-alignment.rs:18:22: 18:23

    bb0: {
        StorageLive(_2);                 // bb0[0]: scope 0 at src/test/ui/fn/dyn-fn-alignment.rs:18:22: 18:23
        _2 = &(_1.0: A);                 // bb0[1]: scope 0 at src/test/ui/fn/dyn-fn-alignment.rs:18:22: 18:23
        _0 = const A::f(move _2) -> [return: bb2, unwind: bb1]; // bb0[2]: scope 0 at src/test/ui/fn/dyn-fn-alignment.rs:18:22: 18:27
                                         // ty::Const
                                         // + ty: for<'r> fn(&'r A) -> *const A {A::f}
                                         // + val: Value(Scalar(<ZST>))
                                         // mir::Constant
                                         // + span: src/test/ui/fn/dyn-fn-alignment.rs:18:24: 18:25
                                         // + literal: Const { ty: for<'r> fn(&'r A) -> *const A {A::f}, val: Value(Scalar(<ZST>)) }
    }

    bb1 (cleanup): {
        resume;                          // bb1[0]: scope 0 at src/test/ui/fn/dyn-fn-alignment.rs:18:14: 18:27
    }

    bb2: {
        StorageDead(_2);                 // bb2[0]: scope 0 at src/test/ui/fn/dyn-fn-alignment.rs:18:26: 18:27
        goto -> bb3;                     // bb2[1]: scope 0 at src/test/ui/fn/dyn-fn-alignment.rs:18:27: 18:27
    }

    bb3: {
        return;                          // bb3[0]: scope 0 at src/test/ui/fn/dyn-fn-alignment.rs:18:27: 18:27
    }
}
Santiago Pastorino (Apr 16 2020 at 19:34, on Zulip):

@nikomatsakis :point_up:

Santiago Pastorino (Apr 16 2020 at 19:48, on Zulip):

to be honest is not clear to me what's going on

Santiago Pastorino (Apr 16 2020 at 20:04, on Zulip):

but nothing is being dereferenced

Santiago Pastorino (Apr 16 2020 at 20:04, on Zulip):

so ...

Santiago Pastorino (Apr 16 2020 at 20:08, on Zulip):

expr.kind is always Scope in my case, and once is Tuple for the entire executing of the test case

Santiago Pastorino (Apr 16 2020 at 20:09, on Zulip):

is never Deref

eddyb (Apr 16 2020 at 20:24, on Zulip):

there only Deref should be in libcore

eddyb (Apr 16 2020 at 20:24, on Zulip):

err, liballoc

eddyb (Apr 16 2020 at 20:24, on Zulip):

the impl<A> FnOnce<A> for Box<dyn FnOnce<A>>

Santiago Pastorino (Apr 16 2020 at 20:29, on Zulip):

ohh

Santiago Pastorino (Apr 16 2020 at 20:29, on Zulip):

but still should be printed by debug

eddyb (Apr 16 2020 at 20:29, on Zulip):

when compiling liballoc

eddyb (Apr 16 2020 at 20:29, on Zulip):

not any testcases

Santiago Pastorino (Apr 16 2020 at 20:29, on Zulip):

right

Santiago Pastorino (Apr 16 2020 at 20:32, on Zulip):

so the important bit should be in main https://gist.github.com/spastorino/36af6c8d7a31fc325ad8fe591202f9c7

eddyb (Apr 16 2020 at 20:32, on Zulip):

wait, are you testing with the example from the issue?

eddyb (Apr 16 2020 at 20:33, on Zulip):

number 1 rule of staring at MIR/LLVM IR: do not use formatting :P

eddyb (Apr 16 2020 at 20:33, on Zulip):

too much noise

eddyb (Apr 16 2020 at 20:33, on Zulip):

but also, I don't think you'll find the deref in there

eddyb (Apr 16 2020 at 20:33, on Zulip):

it's in liballoc

eddyb (Apr 16 2020 at 20:33, on Zulip):

stable code can't do it, it's gated by #![feature(unsized_locals)]

Santiago Pastorino (Apr 16 2020 at 20:34, on Zulip):

eddyb said:

wait, are you testing with the example from the issue?

yes

Santiago Pastorino (Apr 16 2020 at 20:34, on Zulip):

eddyb said:

it's in liballoc

ahh got it got it, so how can I debug this?

eddyb (Apr 16 2020 at 20:34, on Zulip):

find the impl and copy it

Santiago Pastorino (Apr 16 2020 at 20:34, on Zulip):

:)

Santiago Pastorino (Apr 16 2020 at 20:34, on Zulip):

ok

eddyb (Apr 16 2020 at 20:35, on Zulip):

or rather, take the method out and make it a free fn

Santiago Pastorino (Apr 16 2020 at 20:35, on Zulip):

eddyb said:

number 1 rule of staring at MIR/LLVM IR: do not use formatting :P

what do you meant?

eddyb (Apr 16 2020 at 20:35, on Zulip):

don't use println!, dbg!, panic! etc.

Santiago Pastorino (Apr 16 2020 at 20:35, on Zulip):

ok ok :)

Santiago Pastorino (Apr 16 2020 at 20:39, on Zulip):
#[stable(feature = "boxed_closure_impls", since = "1.35.0")]
impl<A, F: FnOnce<A> + ?Sized> FnOnce<A> for Box<F> {
    type Output = <F as FnOnce<A>>::Output;

    extern "rust-call" fn call_once(self, args: A) -> Self::Output {
        <F as FnOnce<A>>::call_once(*self, args)
    }
}
Santiago Pastorino (Apr 16 2020 at 20:39, on Zulip):

@eddyb you meant to copy this to my test file and see the mir dump of it?

eddyb (Apr 16 2020 at 20:40, on Zulip):

but not like that, since it would be orphan

eddyb (Apr 16 2020 at 20:41, on Zulip):
extern "rust-call" fn dispatch_boxed<A, F: FnOnce<A> + ?Sized>(boxed: Box<F>, args: A) -> F::Output {
    F::call_once(*boxed, args)
}
eddyb (Apr 16 2020 at 20:41, on Zulip):

I expect this will work

Santiago Pastorino (Apr 16 2020 at 20:58, on Zulip):

I guess you want something like ...

Santiago Pastorino (Apr 16 2020 at 20:58, on Zulip):
#![feature(fn_traits)]
#![feature(unboxed_closures)]

extern "rust-call" fn dispatch_boxed<A, F: FnOnce<A> + ?Sized>(
    boxed: Box<F>,
    args: A,
) -> F::Output {
    F::call_once(*boxed, args)
}

#[derive(Clone, Copy)]
struct A {
    v: u8,
}

impl A {
    fn f(&self) -> *const A {
        assert_eq!(self as *const A as usize % 256, 0, "optimized out");
        self
    }
}

fn f2(v: u8) -> Box<dyn FnOnce() -> *const A> {
    let a = A { v };
    dispatch_boxed(Box::new(|| a.f()), a)
}

fn main() {}
Santiago Pastorino (Apr 16 2020 at 20:58, on Zulip):

@eddyb getting compilation errors but I'm not sure what was your intention exactly

Santiago Pastorino (Apr 16 2020 at 20:59, on Zulip):

like dispatch_boxed won't return a Box<dyn FnOnce> anymore

Santiago Pastorino (Apr 16 2020 at 21:00, on Zulip):

maybe we want -> Box<F::Output>?

eddyb (Apr 16 2020 at 21:01, on Zulip):

no, why would the return type be boxed?

Santiago Pastorino (Apr 16 2020 at 21:02, on Zulip):

yeah doesn't make sense :)

eddyb (Apr 16 2020 at 21:02, on Zulip):

let me fix the example

Wesley Wiser (Apr 16 2020 at 21:03, on Zulip):

Is this it?

#![feature(fn_traits)]
#![feature(unboxed_closures)]
#![feature(unsized_locals)]

fn dispatch_boxed<A, F: FnOnce<A> + ?Sized>(
    boxed: Box<F>,
    args: A,
) -> F::Output {
    F::call_once(*boxed, args)
}

#[derive(Clone, Copy)]
struct A {
    v: u8,
}

impl A {
    fn f(&self) -> *const A {
        assert_eq!(self as *const A as usize % 256, 0, "optimized out");
        self
    }
}

fn f2(v: u8) -> Box<dyn FnOnce() -> *const A> {
    let a = A { v };
    Box::new(move || a.f())
}

fn main() {
    dispatch_boxed(f2(42), ());
}
eddyb (Apr 16 2020 at 21:03, on Zulip):

that looks more plausible, yes

eddyb (Apr 16 2020 at 21:03, on Zulip):

basically you only want to replace the call with dispatch_boxed

eddyb (Apr 16 2020 at 21:04, on Zulip):

also, extern "rust-call" is unnecessary I've just realized

Santiago Pastorino (Apr 16 2020 at 21:05, on Zulip):

:+1:

Santiago Pastorino (Apr 16 2020 at 21:05, on Zulip):
#![feature(fn_traits)]
#![feature(unboxed_closures)]
#![feature(unsized_locals)]

fn dispatch_boxed<A, F: FnOnce<A> + ?Sized>(boxed: Box<F>, args: A) -> F::Output {
    F::call_once(*boxed, args)
}

#[derive(Clone, Copy)]
struct A {
    v: u8,
}

impl A {
    fn f(&self) -> *const A {
        assert_eq!(self as *const A as usize % 256, 0, "optimized out");
        self
    }
}

fn f2(v: u8) -> Box<dyn FnOnce() -> *const A> {
    let a = A { v };
    Box::new(move || a.f())
}

fn main() {
    dispatch_boxed(f2(42), ());
}
Santiago Pastorino (Apr 16 2020 at 21:06, on Zulip):

which is basically what @Wesley Wiser have said

Santiago Pastorino (Apr 16 2020 at 21:07, on Zulip):

gonna check mir_dump of this

Santiago Pastorino (Apr 16 2020 at 21:11, on Zulip):
// MIR for `dispatch_boxed` 0 mir_map

| User Type Annotations
| 0: Canonical { max_universe: U0, variables: [CanonicalVarInfo { kind: Ty(General(U0)) }], value: TypeOf(DefId(2:2105 ~ core[a8ff]::ops[0]::function[0]::FnOnce[0]::call_once[0]), UserSubsts { substs: [F, ^0], user_self_ty: None }) } at test.rs:6:5: 6:17
|
fn dispatch_boxed(_1: std::boxed::Box<F>, _2: A) -> <F as std::ops::FnOnce<A>>::Output {
    debug boxed => _1;                   // in scope 0 at test.rs:5:45: 5:50
    debug args => _2;                    // in scope 0 at test.rs:5:60: 5:64
    let mut _0: <F as std::ops::FnOnce<A>>::Output; // return place in scope 0 at test.rs:5:72: 5:81
    let mut _3: F;                       // in scope 0 at test.rs:6:18: 6:24
    let mut _4: A;                       // in scope 0 at test.rs:6:26: 6:30

    bb0: {
        StorageLive(_3);                 // bb0[0]: scope 0 at test.rs:6:18: 6:24
        _3 = move (*_1);                 // bb0[1]: scope 0 at test.rs:6:18: 6:24
        StorageLive(_4);                 // bb0[2]: scope 0 at test.rs:6:26: 6:30
        _4 = move _2;                    // bb0[3]: scope 0 at test.rs:6:26: 6:30
        _0 = const <F as std::ops::FnOnce<A>>::call_once(move _3, move _4) -> [return: bb2, unwind: bb6]; // bb0[4]: scope 0 at test.rs:6:5: 6:31
                                         // ty::Const
                                         // + ty: extern "rust-call" fn(F, A) -> <F as std::ops::FnOnce<A>>::Output {<F as std::ops::FnOnce<A>>::call_once}
                                         // + val: Value(Scalar(<ZST>))
                                         // mir::Constant
                                         // + span: test.rs:6:5: 6:17
                                         // + user_ty: UserType(0)
                                         // + literal: Const { ty: extern "rust-call" fn(F, A) -> <F as std::ops::FnOnce<A>>::Output {<F as std::ops::FnOnce<A>>::call_once}, val: Value(Scalar(<ZST>)) }
    }

    bb1 (cleanup): {
        resume;                          // bb1[0]: scope 0 at test.rs:5:1: 7:2
    }

    bb2: {
        StorageDead(_4);                 // bb2[0]: scope 0 at test.rs:6:30: 6:31
        StorageDead(_3);                 // bb2[1]: scope 0 at test.rs:6:30: 6:31
        drop(_2) -> [return: bb7, unwind: bb3]; // bb2[2]: scope 0 at test.rs:7:1: 7:2
    }

    bb3 (cleanup): {
        drop(_1) -> bb1;                 // bb3[0]: scope 0 at test.rs:7:1: 7:2
    }

    bb4 (cleanup): {
        drop(_2) -> bb3;                 // bb4[0]: scope 0 at test.rs:7:1: 7:2
    }

    bb5 (cleanup): {
        drop(_3) -> bb4;                 // bb5[0]: scope 0 at test.rs:6:30: 6:31
    }

    bb6 (cleanup): {
        drop(_4) -> bb5;                 // bb6[0]: scope 0 at test.rs:6:30: 6:31
    }

    bb7: {
        drop(_1) -> [return: bb8, unwind: bb1]; // bb7[0]: scope 0 at test.rs:7:1: 7:2
    }

    bb8: {
        goto -> bb9;                     // bb8[0]: scope 0 at test.rs:7:2: 7:2
    }

    bb9: {
        return;                          // bb9[0]: scope 0 at test.rs:7:2: 7:2
    }
}
eddyb (Apr 16 2020 at 21:14, on Zulip):

yeah it's all in bb0

eddyb (Apr 16 2020 at 21:14, on Zulip):

the move out of *_1 into _3 is what you want to be different

Santiago Pastorino (Apr 16 2020 at 21:16, on Zulip):

yeah I ran this with debug now and I never see a ExprKind::Deref

Santiago Pastorino (Apr 16 2020 at 21:17, on Zulip):

I wonder if I'm traversing Expr in the right way

Santiago Pastorino (Apr 16 2020 at 21:17, on Zulip):

there's always ExprKind::Scope

eddyb (Apr 16 2020 at 21:26, on Zulip):

oh you might need to unpack that

eddyb (Apr 16 2020 at 21:26, on Zulip):

Scope is everywhere

eddyb (Apr 16 2020 at 21:26, on Zulip):

(we may want a better mechanism for that, idk)

eddyb (Apr 16 2020 at 21:26, on Zulip):

look around for code that peeks inside an ExprKind::Scope

eddyb (Apr 16 2020 at 21:26, on Zulip):

there should be some already

Santiago Pastorino (Apr 16 2020 at 21:27, on Zulip):

yeah, as_operand does recursive

Santiago Pastorino (Apr 16 2020 at 21:28, on Zulip):

I was also trying that before with no success but now at least I can debug it :)

Santiago Pastorino (Apr 16 2020 at 21:28, on Zulip):
        if let ExprKind::Scope { region_scope, lint_level, value } = expr.kind {
            let source_info = this.source_info(expr.span);
            let region_scope = (region_scope, source_info);
            return this
                .in_scope(region_scope, lint_level, |this| this.as_operand(block, scope, value));
        }
eddyb (Apr 16 2020 at 21:31, on Zulip):

not as_operand, but something that peeks

eddyb (Apr 16 2020 at 21:31, on Zulip):

as_operand processes

eddyb (Apr 16 2020 at 21:31, on Zulip):

lemme look

Santiago Pastorino (Apr 16 2020 at 21:32, on Zulip):

so I meant to do recursive

Santiago Pastorino (Apr 16 2020 at 21:32, on Zulip):

this is what I've added ...

Santiago Pastorino (Apr 16 2020 at 21:32, on Zulip):
        if let ExprKind::Scope { region_scope, lint_level, value } = expr.kind {
            let source_info = self.source_info(expr.span);
            let region_scope = (region_scope, source_info);
            return self
                .in_scope(region_scope, lint_level, |this| this.as_call_temporary(block, value));
        }
eddyb (Apr 16 2020 at 21:32, on Zulip):

I'm not sure recursive is correct

eddyb (Apr 16 2020 at 21:33, on Zulip):

hmpf

Santiago Pastorino (Apr 16 2020 at 21:33, on Zulip):

I could totally match Scope and value of kind Deref in that case

eddyb (Apr 16 2020 at 21:33, on Zulip):

you might have to, I guess. I'll let @nikomatsakis decide what's correct

eddyb (Apr 16 2020 at 21:33, on Zulip):

I can't find the peeking I was thinking of

Santiago Pastorino (Apr 16 2020 at 21:33, on Zulip):

with recursion I get

Santiago Pastorino (Apr 16 2020 at 21:33, on Zulip):
// MIR for `dispatch_boxed` 0 mir_map

| User Type Annotations
| 0: Canonical { max_universe: U0, variables: [CanonicalVarInfo { kind: Ty(General(U0)) }], value: TypeOf(DefId(2:2105 ~ core[a8ff]::ops[0]::function[0]::FnOnce[0]::call_once[0]), UserSubsts { substs: [F, ^0], user_self_ty: None }) } at test.rs:6:5: 6:17
|
fn dispatch_boxed(_1: std::boxed::Box<F>, _2: A) -> <F as std::ops::FnOnce<A>>::Output {
    debug boxed => _1;                   // in scope 0 at test.rs:5:45: 5:50
    debug args => _2;                    // in scope 0 at test.rs:5:60: 5:64
    let mut _0: <F as std::ops::FnOnce<A>>::Output; // return place in scope 0 at test.rs:5:72: 5:81
    let mut _3: F;                       // in scope 0 at test.rs:6:18: 6:24
    let mut _4: A;                       // in scope 0 at test.rs:6:26: 6:30

    bb0: {
        StorageLive(_3);                 // bb0[0]: scope 0 at test.rs:6:18: 6:24
        _3 = move (*_1);                 // bb0[1]: scope 0 at test.rs:6:18: 6:24
        drop(_3) -> [return: bb5, unwind: bb3]; // bb0[2]: scope 0 at test.rs:6:23: 6:24
    }

    bb1 (cleanup): {
        resume;                          // bb1[0]: scope 0 at test.rs:5:1: 7:2
    }

    bb2 (cleanup): {
        drop(_1) -> bb1;                 // bb2[0]: scope 0 at test.rs:7:1: 7:2
    }

    bb3 (cleanup): {
        drop(_2) -> bb2;                 // bb3[0]: scope 0 at test.rs:7:1: 7:2
    }

    bb4 (cleanup): {
        drop(_3) -> bb3;                 // bb4[0]: scope 0 at test.rs:6:23: 6:24
    }

    bb5: {
        StorageDead(_3);                 // bb5[0]: scope 0 at test.rs:6:23: 6:24
        StorageLive(_4);                 // bb5[1]: scope 0 at test.rs:6:26: 6:30
        _4 = move _2;                    // bb5[2]: scope 0 at test.rs:6:26: 6:30
        drop(_4) -> [return: bb7, unwind: bb3]; // bb5[3]: scope 0 at test.rs:6:29: 6:30
    }

    bb6 (cleanup): {
        drop(_4) -> bb3;                 // bb6[0]: scope 0 at test.rs:6:29: 6:30
    }

    bb7: {
        StorageDead(_4);                 // bb7[0]: scope 0 at test.rs:6:29: 6:30
        _0 = const <F as std::ops::FnOnce<A>>::call_once(move _3, move _4) -> [return: bb8, unwind: bb3]; // bb7[1]: scope 0 at test.rs:6:5: 6:31
                                         // ty::Const
                                         // + ty: extern "rust-call" fn(F, A) -> <F as std::ops::FnOnce<A>>::Output {<F as std::ops::FnOnce<A>>::call_once}
                                         // + val: Value(Scalar(<ZST>))
                                         // mir::Constant
                                         // + span: test.rs:6:5: 6:17
                                         // + user_ty: UserType(0)
                                         // + literal: Const { ty: extern "rust-call" fn(F, A) -> <F as std::ops::FnOnce<A>>::Output {<F as std::ops::FnOnce<A>>::call_once}, val: Value(Scalar(<ZST>)) }
    }

    bb8: {
        drop(_2) -> [return: bb9, unwind: bb2]; // bb8[0]: scope 0 at test.rs:7:1: 7:2
    }

    bb9: {
        drop(_1) -> [return: bb10, unwind: bb1]; // bb9[0]: scope 0 at test.rs:7:1: 7:2
    }

    bb10: {
        goto -> bb11;                    // bb10[0]: scope 0 at test.rs:7:2: 7:2
    }

    bb11: {
        return;                          // bb11[0]: scope 0 at test.rs:7:2: 7:2
    }
}
Santiago Pastorino (Apr 16 2020 at 21:34, on Zulip):

still not ok

Santiago Pastorino (Apr 16 2020 at 22:34, on Zulip):

@eddyb @Wesley Wiser ...

Santiago Pastorino (Apr 16 2020 at 22:34, on Zulip):
// MIR for `dispatch_boxed` 0 mir_map

| User Type Annotations
| 0: Canonical { max_universe: U0, variables: [CanonicalVarInfo { kind: Ty(General(U0)) }], value: TypeOf(DefId(2:2105 ~ core[a8ff]::ops[0]::function[0]::FnOnce[0]::call_once[0]), UserSubsts { substs: [F, ^0], user_self_ty: None }) } at test.rs:6:5: 6:17
|
fn dispatch_boxed(_1: std::boxed::Box<F>, _2: A) -> <F as std::ops::FnOnce<A>>::Output {
    debug boxed => _1;                   // in scope 0 at test.rs:5:45: 5:50
    debug args => _2;                    // in scope 0 at test.rs:5:60: 5:64
    let mut _0: <F as std::ops::FnOnce<A>>::Output; // return place in scope 0 at test.rs:5:72: 5:81
    let mut _3: std::boxed::Box<F>;      // in scope 0 at test.rs:6:19: 6:24
    let mut _4: A;                       // in scope 0 at test.rs:6:26: 6:30

    bb0: {
        StorageLive(_3);                 // bb0[0]: scope 0 at test.rs:6:19: 6:24
        _3 = move _1;                    // bb0[1]: scope 0 at test.rs:6:19: 6:24
        StorageLive(_4);                 // bb0[2]: scope 0 at test.rs:6:26: 6:30
        _4 = move _2;                    // bb0[3]: scope 0 at test.rs:6:26: 6:30
        _0 = const <F as std::ops::FnOnce<A>>::call_once(move (*_3), move _4) -> [return: bb2, unwind: bb6]; // bb0[4]: scope 0 at test.rs:6:5: 6:31
                                         // ty::Const
                                         // + ty: extern "rust-call" fn(F, A) -> <F as std::ops::FnOnce<A>>::Output {<F as std::ops::FnOnce<A>>::call_once}
                                         // + val: Value(Scalar(<ZST>))
                                         // mir::Constant
                                         // + span: test.rs:6:5: 6:17
                                         // + user_ty: UserType(0)
                                         // + literal: Const { ty: extern "rust-call" fn(F, A) -> <F as std::ops::FnOnce<A>>::Output {<F as std::ops::FnOnce<A>>::call_once}, val: Value(Scalar(<ZST>)) }
    }

    bb1 (cleanup): {
        resume;                          // bb1[0]: scope 0 at test.rs:5:1: 7:2
    }

    bb2: {
        StorageDead(_4);                 // bb2[0]: scope 0 at test.rs:6:30: 6:31
        drop(_3) -> [return: bb7, unwind: bb4]; // bb2[1]: scope 0 at test.rs:6:30: 6:31
    }

    bb3 (cleanup): {
        drop(_1) -> bb1;                 // bb3[0]: scope 0 at test.rs:7:1: 7:2
    }

    bb4 (cleanup): {
        drop(_2) -> bb3;                 // bb4[0]: scope 0 at test.rs:7:1: 7:2
    }

    bb5 (cleanup): {
        drop(_3) -> bb4;                 // bb5[0]: scope 0 at test.rs:6:30: 6:31
    }

    bb6 (cleanup): {
        drop(_4) -> bb5;                 // bb6[0]: scope 0 at test.rs:6:30: 6:31
    }

    bb7: {
        StorageDead(_3);                 // bb7[0]: scope 0 at test.rs:6:30: 6:31
        drop(_2) -> [return: bb8, unwind: bb3]; // bb7[1]: scope 0 at test.rs:7:1: 7:2
    }

    bb8: {
        drop(_1) -> [return: bb9, unwind: bb1]; // bb8[0]: scope 0 at test.rs:7:1: 7:2
    }

    bb9: {
        goto -> bb10;                    // bb9[0]: scope 0 at test.rs:7:2: 7:2
    }

    bb10: {
        return;                          // bb10[0]: scope 0 at test.rs:7:2: 7:2
    }
}
Santiago Pastorino (Apr 16 2020 at 22:34, on Zulip):

mir looks good

Santiago Pastorino (Apr 16 2020 at 22:34, on Zulip):

test doesn't pass

eddyb (Apr 16 2020 at 22:35, on Zulip):

there might still be dynamic allocas

eddyb (Apr 16 2020 at 22:35, on Zulip):

from another source

Santiago Pastorino (Apr 16 2020 at 22:35, on Zulip):

hmmm

Santiago Pastorino (Apr 17 2020 at 00:19, on Zulip):

actually it works :)

Santiago Pastorino (Apr 17 2020 at 00:19, on Zulip):

I was testing too fast, come back and realized about --keep-stage 1

Santiago Pastorino (Apr 17 2020 at 00:20, on Zulip):

anyway, other tests are failing need to check, will do tomorrow

Santiago Pastorino (Apr 17 2020 at 00:21, on Zulip):
2020-04-16T23:56:26.6116980Z error[E0382]: use of moved value: `y`
2020-04-16T23:56:26.6117524Z   --> /checkout/src/test/ui/unsized-locals/double-move.rs:20:22
2020-04-16T23:56:26.6117775Z    |
2020-04-16T23:56:26.6117967Z LL |         let y = *x;
2020-04-16T23:56:26.6118550Z    |             - move occurs because `y` has type `str`, which does not implement the `Copy` trait
2020-04-16T23:56:26.6118897Z LL |         drop_unsized(y);
2020-04-16T23:56:26.6119577Z    |                      - value moved here
2020-04-16T23:56:26.6119881Z LL |         drop_unsized(y); //~ERROR use of moved value
2020-04-16T23:56:26.6120194Z    |                      ^ value used here after move
2020-04-16T23:56:26.6120614Z error[E0382]: use of moved value: `x`
Santiago Pastorino (Apr 17 2020 at 00:21, on Zulip):

:)

Santiago Pastorino (Apr 17 2020 at 00:21, on Zulip):

will see tomorrow

eddyb (Apr 17 2020 at 00:57, on Zulip):

hah

eddyb (Apr 17 2020 at 00:57, on Zulip):

--keep-stage is not your friend!!

eddyb (Apr 17 2020 at 00:58, on Zulip):

/me has never used it, nor felt the need to use it

eddyb (Apr 17 2020 at 00:58, on Zulip):

incremental is nice and libstd only takes like a minute to build at stage1

Santiago Pastorino (Apr 17 2020 at 01:07, on Zulip):

the information you're omitting is that you have 62356 cores :P

eddyb (Apr 17 2020 at 01:07, on Zulip):

libstd should take a minute to build with just -j8 IIRC

eddyb (Apr 17 2020 at 01:07, on Zulip):

cc @simulacrum

eddyb (Apr 17 2020 at 01:08, on Zulip):

or maybe I'm misremembering

simulacrum (Apr 17 2020 at 01:08, on Zulip):

Yeah just over on my 16 cores

simulacrum (Apr 17 2020 at 01:09, on Zulip):

And keep stage was never really intended for compiler development

simulacrum (Apr 17 2020 at 01:10, on Zulip):

Mostly for std, to make it easier to iterate on std in stage 1 without a compiler rebuild

eddyb (Apr 17 2020 at 01:11, on Zulip):

can't wait to have a proper workflow for that

eddyb (Apr 17 2020 at 01:11, on Zulip):

I just wish we thought through the implications of master builds earlier :P

eddyb (Apr 17 2020 at 01:11, on Zulip):

@simulacrum oh damn we can tag nightlies into git

eddyb (Apr 17 2020 at 01:11, on Zulip):

or keep a nightly branch

eddyb (Apr 17 2020 at 01:11, on Zulip):

tagging is nice because it allows you to use a nightly with a specific date explicitly

eddyb (Apr 17 2020 at 01:12, on Zulip):

oops this is getting offtopic fast :P

Santiago Pastorino (Apr 17 2020 at 13:02, on Zulip):

@nikomatsakis @eddyb #71170 is now ready for review, there are some little stderr files under unsized_locals feature flag that have changed slightly as I guess you were expecting

eddyb (Apr 17 2020 at 13:02, on Zulip):

we accidentally used the wrong topic lmao lemme fix that done

Santiago Pastorino (Apr 17 2020 at 23:08, on Zulip):

@eddyb we were discussing with @nikomatsakis about this issue after his reviews

Santiago Pastorino (Apr 17 2020 at 23:09, on Zulip):

Niko told me to use the recursion and you kind of advised against it

Santiago Pastorino (Apr 17 2020 at 23:09, on Zulip):

I've done with recursion and it doesn't work

Santiago Pastorino (Apr 17 2020 at 23:09, on Zulip):

all I can see is that ty is getting from an inner expr

Santiago Pastorino (Apr 17 2020 at 23:09, on Zulip):

but the errors I'm getting seems to suggest that the Scope is doing something wrong

Santiago Pastorino (Apr 17 2020 at 23:10, on Zulip):
error[E0382]: use of moved value
    --> src/libcore/iter/traits/iterator.rs:2534:25
     |
2534 |             move |x, y| cmp::max_by(x, y, &mut compare)
     |                         ^^^^^^^^^^^^-^^^^^^^^^^^^^^^^^^
     |                         |           |
     |                         |           value moved here
     |                         |           move occurs because value has type T, which does not implement the Copy trait
     |                         value used here after move
     |
help: consider restricting type parameter T
eddyb (Apr 18 2020 at 00:15, on Zulip):

uhhhhhh

eddyb (Apr 18 2020 at 00:15, on Zulip):

that doesn't pass any arguments by dereference

eddyb (Apr 18 2020 at 00:16, on Zulip):

which means no code should run different than today

eddyb (Apr 18 2020 at 00:16, on Zulip):

this is why I was suggesting you might peek behind the Scope to see if it is an unsized Deref

eddyb (Apr 18 2020 at 00:16, on Zulip):

so that you don't do anything if it's not

eddyb (Apr 18 2020 at 00:16, on Zulip):

basically you can do the recursion thing but only after you confirm that it needs the special case

RalfJ (Apr 19 2020 at 08:12, on Zulip):

@Santiago Pastorino so after your PR, there'll be no alloca for code that is still permitted, right? is that checked/ensured by codegen?

Santiago Pastorino (Apr 19 2020 at 13:36, on Zulip):

@RalfJ what did you mean exactly by code that is still permitted?

Santiago Pastorino (Apr 19 2020 at 13:36, on Zulip):

and by checked/ensured by codegen you meant if we had some kind of test case or what?

RalfJ (Apr 19 2020 at 13:45, on Zulip):

Santiago Pastorino said:

RalfJ what did you mean exactly by code that is still permitted?

I was under the impression that not all unsized-local code can fit the pattern that is used to fix the soundness bug?

RalfJ (Apr 19 2020 at 13:45, on Zulip):

Santiago Pastorino said:

and by checked/ensured by codegen you meant if we had some kind of test case or what?

more like, ICE in codegen at the place where we used to emit an alloca

Last update: Sep 28 2020 at 15:45UTC