Missed UB around assignments and function calls when using custom MIR

[RalfJung]

There are some open questions around assignments and function calls:

• What are the semantics of move operands? unsafe-code-guidelines#416
• What exactly is going on with return places and the LHS of assignments? unsafe-code-guidelines#417

When writing custom MIR, Miri currently definitely misses some UB here due to ignoring those questions. For function calls, we have examples of that. For assignments, I am not entirely sure what the current status is -- if I understood @bjorn3 correctly, we do use memcpy for some kinds of assignments, so it should be possible to write MIR that is accepted by Miri but UB in LLVM? I hope this requires custom MIR, or can something like that be written in surface Rust?

[bjorn3]

I hope this requires custom MIR, or can something like that be written in surface Rust?

MIR building currently defensively emits a copy basically everywhere. MIR optimizations may allow observing move operands though depending on which ones have been enabled right now. And I hope that in the future we can stop emitting copies as aggresively during MIR building as it reduces effectivity of MIR optimizations, hurts perf for unoptimized builds and even somewhat (and even a lot in some cases) for fully optimized builds.

memcpy is used for all assignments of non-primitive types. Function returns work either using registers or a return value pointer which allows the callee to directly write to the return place of the caller's frame. Copy function arguments are copied to a temporary if they are by-ref. Move function arguments directly pass a pointer to the place of the move operand.

[RalfJung]

MIR optimizations may allow observing move operands though depending on which ones have been enabled right now.

How does an operand being move affect codegen / MIR optimizations (outside of fn calls)?

memcpy is used for all assignments of non-primitive types.

What exactly is "primitive" here? Is this about having an Aggregate ABI?

[bjorn3]

How does an operand being move affect codegen / MIR optimizations (outside of fn calls)?

Codegen doesn't distinguish between copy and move in assignments. Only for function calls is it special cased.

What exactly is "primitive" here? Is this about having an Aggregate ABI?

Yes, I believe so.

[RalfJung]

Yes, I believe so.

I guess I could implement the reborrow-based approach outlined here and only use it for aggregate ABI assignments to make sure we check what codegen needs, but that feels very arbitrary.

Elsewhere you said once that this affects only some rvalue expressions -- is that based on the observation that e.g. BinOp always returns a Scalar/ScalarPair (i.e., a primitive)?

Do you know where in codegen this decision is made? I found this which looks promising. The decision whether to use OperandValue::Ref is also made here, but that seems to be about function calls so I guess it is different.

[bjorn3]

I found this which looks promising.

It is around https://github.com/rust-lang/rust/blob/4996b56ba9647d149265d03dcbd9ab837af3a1bb/compiler/rustc_codegen_ssa/src/mir/mod.rs#L226-L251 to determine if it could be stored as SSA value in the first place or needs to be on the stack, and then https://github.com/rust-lang/rust/blob/4996b56ba9647d149265d03dcbd9ab837af3a1bb/compiler/rustc_codegen_ssa/src/mir/statement.rs#L16-L39 handles the actual assignment depending on how it is stored.

For cg_clif the logic is simply https://github.com/bjorn3/rustc_codegen_cranelift/blob/45781e107c37524386478b02082700f09bd10d78/src/abi/mod.rs#L196-L204 for locals where is_ssa will be false for Abi::Aggregate locals. https://github.com/bjorn3/rustc_codegen_cranelift/blob/45781e107c37524386478b02082700f09bd10d78/src/abi/mod.rs#L295-L319 for arguments and finally https://github.com/bjorn3/rustc_codegen_cranelift/blob/45781e107c37524386478b02082700f09bd10d78/src/abi/returning.rs#L10 for the return value _0.

The decision whether to use OperandValue::Ref is also made here, but that seems to be about function calls so I guess it is different.

Yeah, that is for function calls.

[RalfJung]

rust-lang/rust#113441 should fix the situation with assignments.

[RalfJung]

@bjorn3 if you could take a look at the tests added in rust-lang/rust#113569 and whether they reflect all the UB we need for Move argument passing, that would be great. :)