Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Semantics of MIR function calls #71117

Open
RalfJung opened this issue Apr 14, 2020 · 46 comments
Open

Semantics of MIR function calls #71117

RalfJung opened this issue Apr 14, 2020 · 46 comments
Labels
A-MIR Area: Mid-level IR (MIR) - https://blog.rust-lang.org/2016/04/19/MIR.html A-miri Area: The miri tool T-lang Relevant to the language team, which will review and decide on the PR/issue.

Comments

@RalfJung
Copy link
Member

Some discussion at #71005 (comment) revealed that we are not entirely sure what exactly the semantics of passing arguments and return values for function calls should be.

The simplest possible semantics is to say that when a stack frame is created, we allocate fresh memory for all arguments and return values (according to the known layout, determined by the callee). We copy the function arguments into the argument slots. Then we evaluate the function, and when it returns, we copy the return value back.

However, such a model is hard to compile down to destination-passing style, where the callee actually writes its return value directly into caller-provided memory. If that aliases with other things the function can access, behavior could differ with and without destination-passing style. This is complicated by the fact that in MIR right now a Call does not provide a return place, but even with destination-passing style diverging functions (without a return place) may access their return local _0 . Moreover @eddyb says that also for some function arguments, we might want to elide the copy during codegen; it is unclear whether that is behaviorally equivalent to the above copying semantics or not.

This is something of a sibling to #68364. We should have a good way to collect all these "MIR semantics" issues...

@RalfJung
Copy link
Member Author

I think a first step we should take is to make the Call terminator always provide a return place. Right now, every backend has to replicate the same hack where some scratch memory still needs to be allocated for the return place in case the caller did not provide some (except for Miri which makes it illegal to access the return place in this situation, but that is likely just wrong).

Beyond that, I am not sure. Miri right now implements copying as described above for arguments. For return values, it directly uses the caller-provided place, which means RETURN_PLACE needs to be special-cased in a bunch of places. We can probably get rid of this special treatment if we are okay with losing the "immediate value" optimization for return places; then we could force_allocate the caller-provided return place and make the callee _0 an Indirect local. (This would entirely remove return_place from Frame, which is good I think.)

To ensure that the return place does not alias with anything, we could try using Stacked Borrows: rust-lang/miri#1330. However, hat is quite the hack -- usually retags are explicitly in the code; this would make the return place the only implicit retag in our semantics. Also we should at least go over a bunch of tricky examples to ensure that this indeed makes all bad cases UB. Unfortunately, without a solution to rust-lang/miri#196, it is hard to test these things.

For passing arguments without a copy, I don't know if what Miri does is a problem and I don't know what a solution could look like.

@eddyb
Copy link
Member

eddyb commented Apr 14, 2020

cc @rust-lang/wg-mir-opt @nikomatsakis

@jonas-schievink jonas-schievink added A-MIR Area: Mid-level IR (MIR) - https://blog.rust-lang.org/2016/04/19/MIR.html A-miri Area: The miri tool T-lang Relevant to the language team, which will review and decide on the PR/issue. labels Apr 14, 2020
@nikomatsakis
Copy link
Contributor

@RalfJung is the optional place only employed for functions that return uninhabited values? The type ! has size 0, but I suppose in some cases the type might be non-zero in size..? I'm a bit confused about that part of what you wrote.

@bjorn3
Copy link
Member

bjorn3 commented Apr 14, 2020

(!, u8) has a size of 1.

@eddyb
Copy link
Member

eddyb commented Apr 14, 2020

Because of partial initialization, you could have fields of e.g. (A, B, C, !) written to.

@nikomatsakis
Copy link
Contributor

Right. I just wanted to be sure that this is the kind of "dummy place" that @RalfJung was referring to, or if this was also a problem for the ! type.

@nikomatsakis
Copy link
Contributor

I think that in general when we are assigning to a place, that place should not alias any of the values being read during the instruction. In other words, I think we should avoid the need for backends to introduce "temporaries".

I'm not 100% sure what this implies for arguments. I forget if we permit the arguments in MIR to be mutable, or do we have a function with a mut parameter copy those values into a local copy?

This is all related to #68304, since in there we are talking about cases where the size of the parameter is not known at runtime, and we would like to be able to pass it as argument by reference without having to create a temporary (which would require an alloca). Presumably this is at least partly what @eddyb was referring to.

@eddyb
Copy link
Member

eddyb commented Apr 14, 2020

@nikomatsakis For optimization reasons we want calls to not do copies of anything we pass by reference in the ABI. Otherwise MIR optimizations could never remove those copies, even when it would be correct to do so.

@nikomatsakis
Copy link
Contributor

Right, that'd be the other case. Still, it looks if I compile

fn foo(mut x: u32) {
    x += 1;
}

I get this:

fn  foo(_1: u32) -> () {
    debug x => _1;                       // in scope 0 at src/main.rs:1:8: 1:13
    let mut _0: ();                      // return place in scope 0 at src/main.rs:1:20: 1:20
    let mut _2: (u32, bool);             // in scope 0 at src/main.rs:2:5: 2:11

    bb0: {
        _2 = CheckedAdd(_1, const 1u32); // bb0[0]: scope 0 at src/main.rs:2:5: 2:11
        assert(!move (_2.1: bool), "attempt to add with overflow") -> bb1; // bb0[1]: scope 0 at src/main.rs:2:5: 2:11
    }

    bb1: {
        _1 = move (_2.0: u32);           // bb1[0]: scope 0 at src/main.rs:2:5: 2:11
        return;                          // bb1[1]: scope 0 at src/main.rs:3:2: 3:2
    }
}

Note in particular the _1 = move _2 at the end I think that if parameters were (at least sometimes) references into the caller's stack frame, that could be problematic, right? (In other words, we don't want the callee to be mutating the caller's variables.)

@eddyb
Copy link
Member

eddyb commented Apr 14, 2020

(In other words, we don't want the callee to be mutating the caller's variables.)

We do, again, for optimizations reasons. This only happens with Operand::Move arguments, Operand::Copy arguments will do a copy in the caller before the call IIRC.

@hanna-kruppe
Copy link
Contributor

hanna-kruppe commented Apr 14, 2020

To clarify, do you mean that e.g. in

fn foo(s: String) {
    bar(s);
}

the call to bar should pass on the same address foo received as argument? This is not currently the case, but IIUC it falls out of the aspiration / codegen strategy that you describe.

Edit: to be clear, the reason it currently copies the String in foo is an explicit temporary in the MIR, whose use as Operand::Move in the call to bar the indeed happens without a further temporary that would have been implicit in the MIR. But presumably you'd want that temporary to be removed too?

@RalfJung
Copy link
Member Author

RalfJung commented Apr 14, 2020

I think that in general when we are assigning to a place, that place should not alias any of the values being read during the instruction. In other words, I think we should avoid the need for backends to introduce "temporaries".

This issue is not about assignments though but about function calls. Or do you view _1 = foo(...) as an assignment? (I don't, it's not StatementKind::Assign.)

is the optional place only employed for functions that return uninhabited values? The type ! has size 0, but I suppose in some cases the type might be non-zero in size..? I'm a bit confused about that part of what you wrote.

I don't know if you are asking about current behavior or proposed change.
Right now, it seems like indeed diverging functions do get a return place and a return destination (basic block) if their return type has non-zero size (demo). That's really strange, why would whether or not the function gets a return destination have anything to do with the size of its return type? I think this is basically an accident because in the MIR we have an Option<(Place, BasicBlock)> so we can only have both or neither. (Note that the "if Some, the call is converging" comment there is wrong as my example demonstrates.)

Worse, the callee cannot even rely on this because the callee might be generic and hence, potentially, have a non-ZST uninhabited return type, but the caller knows its ZST and then fails to provide a return place. This has convinced me that diverging functions should always have a return place. IOW, the MIR Call terminator should have Option<BasicBlock>, that's fine (and then returning from a function is just UB if no return destination block was provided, easy), but it should have a mandatory Place.

@nikomatsakis
Copy link
Contributor

nikomatsakis commented Apr 14, 2020

@RalfJung I was including calls under the category of assignment, yes, but I am happy to limit the term "assignment" to StmtAssign, though I think it'd be useful to have a blanker term for "something that mutates a destination place".

Regarding the question of whether diverging calls should always have a "destination place", I think that's fine, simpler and more uniform MIR seems better.


@eddyb

We do, again, for optimizations reasons. This only happens with Operand::Move arguments, Operand::Copy arguments will do a copy in the caller before the call IIRC.

Interesting. I guess that limiting this to Move arguments makes sense, in that the caller must regard that memory as deinitialized after the call (so it does no harm for the callee to have mutated it).

@RalfJung
Copy link
Member Author

Interesting. I guess that limiting this to Move arguments makes sense, in that the caller must regard that memory as deinitialized after the call (so it does no harm for the callee to have mutated it).

There's still something that makes no sense, and that is that an Operand would be used "by reference". It was my understanding that the entire point of Operand is to be a value, i.e., something you can copy out of, but not something that has an address in memory.

@eddyb

For optimization reasons we want calls to not do copies of anything we pass by reference in the ABI. Otherwise MIR optimizations could never remove those copies, even when it would be correct to do so.

For reasons of basic sanity we want MIR semantics to be as simple as we can make it. So the question is, with MIR semantics as implemented by Miri (function arguments always copy), what is the concrete MIR optimization or lowering that does not observably preserve this behavior?

@eddyb
Copy link
Member

eddyb commented Apr 14, 2020

Regarding the question of whether diverging calls should always have a "destination place", I think that's fine, simpler and more uniform MIR seems better.

I think we all agree on this now, who's gonna do it? Do we want an issue just for that?

There's still something that makes no sense, and that is that an Operand would be used "by reference". It was my understanding that the entire point of Operand is to be a value, i.e., something you can copy out of, but not something that has an address in memory.

I keep bringing this up, but: I don't like it either. I have previously called it an abuse of Operand, including for the RHS of an assignment where (non-overlapping) memcpy may be used (a "by-reference" use with aliasing implications).

So the question is, with MIR semantics as implemented by Miri (function arguments always copy), what is the concrete MIR optimization or lowering that does not observably preserve this behavior?

It's fine, you're just not detecting the UB necessary to observe the distinction (i.e. you get a difference between codegen and miri without miri detecting it as UB).

This is just like with the discussion around assignments and aliasing between destination and source. Even if miri wouldn't detect that, codegen could still result in something that doesn't crash but corrupts the values silently.

@nikomatsakis
Copy link
Contributor

@RalfJung

There's still something that makes no sense, and that is that an Operand would be used "by reference". It was my understanding that the entire point of Operand is to be a value, i.e., something you can copy out of, but not something that has an address in memory.

Yes, that was bugging me too! It seems like what we're saying is that calls should take places, not operands.

@RalfJung
Copy link
Member Author

I think we all agree on this now, who's gonna do it? Do we want an issue just for that?

Sure, why not?
This seems like MIR building might need some non-trivial adjustments.

I keep bringing this up, but: I don't like it either. I have previously called it an abuse of Operand, including for the RHS of an assignment where (non-overlapping) memcpy may be used (a "by-reference" use with aliasing implications).

Hm, I am somehow not seeing this as abuse for assignment... but this goes back to the discussion we had in the assignment issue.

It's fine, you're just not detecting the UB necessary to observe the distinction (i.e. you get a difference between codegen and miri without miri detecting it as UB).

This is just like with the discussion around assignments and aliasing between destination and source. Even if miri wouldn't detect that, codegen could still result in something that doesn't crash but corrupts the values silently.

That doesn't sound fine at all! When Miri and codegen differ like that, that is a critical soundness bug in either Miri or codegen.


The latest proposal by @jonas-schievink in #71005 is also interesting. Together with rust-lang/miri#1330, this is a mixture of the two alternatives I described above: we always allocate backing store for the _0 local and use that during execution of the function. Moreover, when the function returns, if a return place is given, we copy from _0 to there (and at that point perform validation of the return value).

What I like about this is that it avoids having to manually call validation on stack frame pop like we do so far, and like we would have to even if we make _0 directly point to the caller-provided return place. At the same time, retagging the return place (while still something of a hack) should ensure that eliding the copy during codegen is correct. This almost seems like the best of both worlds to me (and at least for Miri it makes making the return place mandatory much less pressing, as we allocate for it manually anyway).

@nikomatsakis
Copy link
Contributor

@RalfJung maybe I'm confused -- it seems like if the official semantics are that we have allocated backing storage for _0, we would have problems with unsized return values... oh, I see, of course _0 must always be sized, so it's always possible in principle to allocate that temporary up front.

I guess the key point has to do with retagging the return place -- can you elaborate a bit on why that ensures eliding the copy is correct? (bit out of cache for me I suppose)

@RalfJung
Copy link
Member Author

it seems like if the official semantics are that we have allocated backing storage for _0, we would have problems with unsized return values... oh, I see, of course _0 must always be sized, so it's always possible in principle to allocate that temporary up front.

Honestly I did not give much thought to unsized return values. How are they supposed to work anyway with a caller-allocated return place?

I think the existing handling of lazily allocating unsized locals (i.e., allocating them on the first write to them, when the size is known) should in principle also work for return values. It's something of a hack, but it works.

I guess the key point has to do with retagging the return place -- can you elaborate a bit on why that ensures eliding the copy is correct? (bit out of cache for me I suppose)

It's about retagging with a protector. The reasoning is that, after the retag-with-protector, we have a fresh tag that is not known to anyone except this return place, and that tag is at the top of the borrow stack protected by the current function call. This means that if any other tag is used to access this memory while the function is ongoing (i.e., until it gets popped), there is immediate UB as a protected item would get popped off the stack.
(If the callee does &mut _0, that reference may indeed be used to access the return place as it is derived from the right tag. But that is expected, I presume.)

This is basically the same reasoning that permits the following optimization (assuming no unwinding happens):

fn foo(x: &mut i32) {
    // We are allowed to move the write down below the call, because if `bar`
    // reads or writes `x`, it causes immediate UB.
    // Without protectors this transformation would be illegal as `bar` could just pop `x`'s
    // tag off the stack without any bad consequences, since `x` does not get used again.
    *x = 13;
    bar();
}

@nikomatsakis
Copy link
Contributor

@RalfJung Thanks for the explanation, that makes sense.

Regarding unsized return values, it's very much not clear how they should work (who allocates the memory?), so I wouldn't worry about that. I think that if we do ever try to implement such a scheme, we can wrestle with it then.

@bjorn3
Copy link
Member

bjorn3 commented May 14, 2021

fn foo(a: Vec<u8>) {}
fn bar(a: Vec<u8>) {
    foo(a);
}

currently results in

// WARNING: This output format is intended for human consumers only
// and is subject to change without notice. Knock yourself out.
fn bar(_1: Vec<u8>) -> () {
    debug a => _1;                       // in scope 0 at <anon>:1:30: 1:31
    let mut _0: ();                      // return place in scope 0 at <anon>:1:42: 1:42
    let _2: ();                          // in scope 0 at <anon>:1:44: 1:50
    let mut _3: std::vec::Vec<u8>;       // in scope 0 at <anon>:1:48: 1:49

    bb0: {
        _3 = move _1;                    // scope 0 at <anon>:1:48: 1:49
        _2 = foo(move _3) -> bb1;        // scope 0 at <anon>:1:44: 1:50
                                         // mir::Constant
                                         // + span: <anon>:1:44: 1:47
                                         // + literal: Const { ty: fn(std::vec::Vec<u8>) {foo}, val: Value(Scalar(<ZST>)) }
    }

    bb1: {
        _0 = const ();                   // scope 0 at <anon>:1:42: 1:53
        return;                          // scope 0 at <anon>:1:53: 1:53
    }
}

fn foo(_1: Vec<u8>) -> () {
    debug a => _1;                       // in scope 0 at <anon>:1:8: 1:9
    let mut _0: ();                      // return place in scope 0 at <anon>:1:20: 1:20

    bb0: {
        _0 = const ();                   // scope 0 at <anon>:1:20: 1:22
        drop(_1) -> bb1;                 // scope 0 at <anon>:1:21: 1:22
    }

    bb1: {
        return;                          // scope 0 at <anon>:1:22: 1:22
    }
}

Even when conservatively considering any aliasing between any call arguments or a call argument and the return place UB at the caller side, it would be fine to omit the _3 = move _1; and instead pass move _1 as argument. This is true even when there is a reference to _1 as said reference is invalidated by the _3 = move _1 cq _2 = foo(move _1) -> bb1. Because it is a move and not a copy, it is fine for the callee to directly mutate the stack slot of the caller's copy of the local like codegen can do right now. I think it would be fine to omit this extra copy to a temp var before borrowck while building mir, where it would be very cheap. This should improve both compilation time and debug mode runtime.

@RalfJung
Copy link
Member Author

Even when conservatively considering any aliasing between any call arguments or a call argument and the return place UB at the caller side,

More UB means more compiler freedom, so from a compiler perspective the conservative approach would be to do more copies... right?

Because it is a move and not a copy

So far, we don't have a clear idea for how moves and copies would be any different operationally (and, thus, in terms of UB).

@bjorn3
Copy link
Member

bjorn3 commented May 15, 2021

More UB means more compiler freedom, so from a compiler perspective the conservative approach would be to do more copies... right?

The conservative choice here is assuming that the callee modifies the locals used to store arguments and thus that accessing the locals used for arguments after the call is UB. Even when under this conservative choice, it should be fine to omit the copy as the local isn't used anymore before being reinitialized.

So far, we don't have a clear idea for how moves and copies would be any different operationally (and, thus, in terms of UB).

Why would it be fine to read a local after it has been moved out of? The surface rust language doesn't even allow this and I think borrowck enforces this at the MIR level. Writing (aka reinitializing) is fine though.

@RalfJung
Copy link
Member Author

RalfJung commented May 8, 2022

Yeah but I don't see how that helps for this optimization. We cannot declare _1 dead before the call since it is used as an argument to the call.

@JakobDegen
Copy link
Contributor

We can declare the order to be:

  1. Argument values are computed (via a load in the case of move arguments)
  2. Control flow enters the callee
  3. The callee allocates memory for the parameters
  4. The parameter values are stored to the new allocations

In that case the optimization is allowed because the parameter allocations are not live until after the last step, and the argument allocations are dead after step 1. This is... admittedly a bit subtle, but it should totally work. Also, given that we'll probably additionally have somewhat surprising ordering around the return place computation (to disallow overlaps), I don't think this makes things that much worse.

@RalfJung
Copy link
Member Author

RalfJung commented May 8, 2022

We should be able to directly represent this in the MIR though, by placing a StorageDead in a suitable location -- which seems hard with this proposal?

@JakobDegen
Copy link
Contributor

Oh hmm, I see what you mean. My assumption had been that with this proposal we can basically get rid of Storage* statements and just replace them with liveness analysis, but that's not quite true; it's only the case at MIR building time.

bors added a commit to rust-lang-ci/rust that referenced this issue May 24, 2022
…-obk

Refactor call terminator to always include destination place

In rust-lang#71117 people seemed to agree that call terminators should always have a destination place, even if the call was guaranteed to diverge. This implements that. Unsurprisingly, the diff touches a lot of code, but thankfully I had to do almost nothing interesting. The only interesting thing came up in const prop, where the stack frame having no return place was also used to indicate that the layout could not be computed (or similar). I replaced this with a ZST allocation, which should continue to do the right things.

cc `@RalfJung` `@eddyb` who were involved in the original conversation

r? rust-lang/mir-opt
flip1995 pushed a commit to flip1995/rust that referenced this issue Jun 4, 2022
…-obk

Refactor call terminator to always include destination place

In rust-lang#71117 people seemed to agree that call terminators should always have a destination place, even if the call was guaranteed to diverge. This implements that. Unsurprisingly, the diff touches a lot of code, but thankfully I had to do almost nothing interesting. The only interesting thing came up in const prop, where the stack frame having no return place was also used to indicate that the layout could not be computed (or similar). I replaced this with a ZST allocation, which should continue to do the right things.

cc `@RalfJung` `@eddyb` who were involved in the original conversation

r? rust-lang/mir-opt
bors added a commit to rust-lang-ci/rust that referenced this issue Nov 27, 2022
Fix Dest Prop

Closes rust-lang#82678, rust-lang#79191 .

This was not originally a total re-write of the pass but is has gradually turned into one. Notable changes:

 1. Significant improvements to documentation all around. The top of the file has been extended with a more precise argument for soundness. The code should be fairly readable, and I've done my best to add useful comments wherever possible. I would very much like for the bus factor to not be one on this code.
 3. Improved handling of conflicts that are not visible in normal dataflow.  This was the cause of rust-lang#79191. Handling this correctly requires us to make decision about the semantics and specifically evaluation order of basically all MIR constructs (see specifically rust-lang#68364 rust-lang#71117.  The way this is implemented is based on my preferred resolution to these questions around the semantics of assignment statements.
 4. Some re-architecting to improve performance. More details below.
 5. Possible future improvements to this optimization are documented, and the code is written with the needs of those improvements in mind. The hope is that adding support for more precise analyses will not require a full re-write of this opt, but just localized changes.

### Regarding Performance

The previous approach had some performance issues; letting `l` be the number of locals and `s` be the number of statements/terminators, the runtime of the pass was `O(l^2 * s)`, both in theory and in practice. This version is smarter about not calculating unnecessary things and doing more caching. Our runtime is now dominated by one invocation of `MaybeLiveLocals` for each "round," and the number of rounds is less than 5 in over 90% of cases. This means it's linear-ish in practice.

r? `@oli-obk` who reviewed the last version of this, but review from anyone else would be more than welcome
@RalfJung
Copy link
Member Author

RalfJung commented May 8, 2023

One aspect that hasn't come up yet is the evaluation order wrt the return place: in *ptr = foo(...);, does it use the pointer value before or after the call? Miri says before and I hope that's also what codegen does. It should be fairly uncontroversial IMO except that in surface Rust we evaluate the RHS twice (but MIR building introduces temporaries so that MIR itself can have a different eval order).

@cbeuw
Copy link
Contributor

cbeuw commented May 12, 2023

In custom MIR one could observe whether the codegen has used the same memory location for caller destination and callee return slot through pointer comparison. fn1 is supplied with a pointer pointing to the destination x, and inside fn1 this is compared with a pointer to RET. Miri prints false for this and LLVM prints true.

This is not reproducible in normal Rust for two reasons: one is that a temporary is always generated as the destination before a Call terminator, so there's no way to get a pointer to it. Another is that inside the callee you can no longer get &raw _0 now that NVRO is disabled in #111007 (though the latter is still possible on current stable and if it's reenabled in the future)

#![feature(custom_mir, core_intrinsics)]
extern crate core;
use core::intrinsics::mir::*;
#[custom_mir(dialect = "runtime", phase = "initial")]
pub fn fn0() -> bool {
    mir! {
    let x: (u64, bool, u8, bool);
    let ptr: *const (u64, bool, u8, bool);
    {
    x = (1, true, 2, true);
    ptr = core::ptr::addr_of!(x);
    Call(x, bb1, fn2(ptr))
    }
    bb1 = {
    RET = (*ptr).1;
    Return()
    }

    }
}

#[custom_mir(dialect = "runtime", phase = "initial")]
pub fn fn2(dst_ptr: *const (u64, bool, u8, bool)) -> (u64, bool, u8, bool) {
    mir! {
    type RET = (u64, bool, u8, bool);
    let ret_ptr: *const (u64, bool, u8, bool);
    {
    RET = (1, true, 2, true);

    ret_ptr = core::ptr::addr_of!(RET);
    RET.1 = dst_ptr == ret_ptr;
    Return()
    }

    }
}

pub fn main() {
    println!("{}", fn0());
}

@bjorn3
Copy link
Member

bjorn3 commented May 12, 2023

This depends on the return type and calling convention. If the calling convention says the return value has to be returned using an out pointer, you get the observed behavior of the address being identical. If the calling convention says it has to be returned in a register, you get two different addresses. Same applies to function arguments.

@RalfJung
Copy link
Member Author

I think on the Rust level we probably want to treat this as non-deterministic. The question is how to best do that, given that these are two distinct allocations in a straight-forward definition of what happens at a function call.

Maybe we need more of that "two distinct allocations at the same address" trickery...

@JakobDegen
Copy link
Contributor

I'm losing track of this conversation a bit, it seems like we're discussing at least three different things here:

The first is surface Rust evaluation order, which Ralf shared an example for here. I don't think we'd be able to change this even if we wanted to, so I don't think it's worth much discussion. In any case, I also think that what Rust does today is probably the right thing anyway.

The second is the semantics of a surface Rust version of the program Andy wrote above. It's hard to write such a program because Rust has no native concept of a return place, but it might look something like this:

static ADDR: usize = 0;

fn get_return_address() -> usize {
    let x = 0;
    ADDR = addr_of!(x) as usize;
    return x;
}

fn main() {
    let mut x = 0;
    let a = addr_of!(x) as usize;
    x = get_return_address();
    dbg!(a == ADDR);
}

I don't know how to write an opsem in which this program is allowed to print true. It would maybe be nice if we could. We should discuss this question on UCG too though.

The third thing that is being discussed here (and the one which is actually on topic for this issue) is the one about Mir semantics. On this I have three thoughts:

a. The behavior of Miri today is certainly not wrong for surface Rust programs as a result of Mir building introducing lots of temporaries. It is possible that Miri doesn't report UB on some programs on which it maybe should (ie a Mir version of Ralf's zulip example). That might be worth fixing but it's not a huge deal.
b. What codegen does today should eventually be fixed. Even if the address equality issue wasn't there, there are still other ways to observe that the callee can write to the destination place before the call returns. At the end of the day what it is is a crappy attempt to implement something like destprop in codegen. It also means that destprop ends up having to do extra work to turn itself off so that it doesn't miscompile cases like the one I wrote above. I can't justify disabling this behavior right now, but I'm very much looking to do so in the future. At the very least, Lir will only get these semantics over my dead body (this is also true because even if we had a more reasonable representation, these aren't even the semantics an optimizer wants).
c. Once we fix this behavior in codegen and have a reasonable way to get those optimizations which we agree are actually sound, we should just change Mir semantics to match surface Rust. That sounds like the most straightforward solution to making most of these problems go away.

@RalfJung
Copy link
Member Author

Yeah I've been using this as a grab-bag issue for all things related to fn calls. Some of these should probably get their own UCG threads.

One main thing we discussed here in the past is the interaction of move operands and function calls. Function calls seem to be one of the main motivation for move operands being special, so if we want to replace move operands by something else (which I think we should), then function calls will show us some key design constraints.

@RalfJung
Copy link
Member Author

Thanks to @cbeuw for some examples using custom MIR that demonstrate the current behavior of LLVM here.

I particularly worry about this one: in a function like

pub unsafe fn write(mut ptr_to_non_copy: *mut (usize, i32, f64), mut non_copy: Inner) {

we see that ptr_to_non_copy and addr_of_mut!(non_copy) can be the same pointer! This is hard to reconcile with the idea that each function argument is allocated as a fresh place when the function starts.

If we view move as a boolean flag associated with each function argument, the best attempt I can come up with at explaining this is something like this:

  1. First we evaluate all function arguments to their value (as in, MiniRust Value)
  2. Then we deallocate the backing store of all move arguments (only makes sense if those are locals, we should reject any other place expression)
  3. Then we allocate backing store for the callee and write these values in there

What about other places to move from? @bjorn3 said in the past we should allow at least *local (for passing unsized arguments, IIRC). I guess if we specifically say that local must be a Box in that case we can still say that we will deallocate the box in step (2).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-MIR Area: Mid-level IR (MIR) - https://blog.rust-lang.org/2016/04/19/MIR.html A-miri Area: The miri tool T-lang Relevant to the language team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests

10 participants