Document the current MIR semantics that are clear from existing code #95320

JakobDegen · 2022-03-26T00:33:18Z

This PR adds documentation to places, operands, rvalues, statementkinds, and terminatorkinds that describes their existing semantics and requirements. In many places the semantics depend on the Rust memory model or other T-Lang decisions - when this is the case, it is just noted as such with links to UCG issues where possible. I'm hopeful that none of the documentation added here can be used to justify optimizations that depend on the memory model. The documentation for places and operands probably comes closest to running afoul of this - if people think that it cannot be merged as is, it can definitely also be taken out.

The goal here is to only document parts of MIR that seem to be decided already, or are at least depended on by existing code. That leaves quite a number of open questions - those are marked as "needs clarification." I'm not sure what to do with those in this PR - we obviously can't decide all these questions here. Should I just leave them in as is? Take them out? Keep them in but as // instead of /// comments?

If this is too big to review at once, I can split this up.

r? rust-lang/mir-opt

JakobDegen · 2022-03-26T00:34:41Z

I've left a bunch of FIXME's in here. I may try and get to some of them before merging, but they should all be non-critical and can just be added later.

The test change has a relevant Zulip conversation

Also, I didn't add anything for SetDiscriminant pending #95125 . If we decide against such a change, I'll put up a PR documenting the current semantics

compiler/rustc_middle/src/mir/mod.rs

bjorn3 · 2022-03-26T06:20:45Z

compiler/rustc_middle/src/mir/mod.rs

+///    pointed to type.
+///
+/// **Needs clarification**: What about metadata resulting from dereferencing wide pointers (and
+/// possibly from accessing unsized locals - not sure how those work)? That probably deserves to go


A place stores an address and an optional meta value.

Yeah, this is probably the right thing to have. That being said, I'm going to leave this open until I get a chance to more thoroughly go through all of the documentation and deal with DSTs (or probably someone else should do that because I know very little about it)

bjorn3 · 2022-03-26T06:24:24Z

compiler/rustc_middle/src/mir/mod.rs

+/// Not all of these are allowed at every [`MirPhase`] - when this is the case, it's stated below.
+///
+/// Computing any rvalue begins by evaluating the places and operands in the rvalue in the order in
+/// which they appear. These are then used to produce a "value" - the same kind of value that an


I think it should be left undefined in which order theybare evaluated.

I've changed the ordering question to a "needs clarification" section in an effort to only document things which are already decided.

That being said, I don't think this is a good option. First of all, I'm not sure "order is left indeterminate" implies "any attempt on depending on the order is UB" - it might be the case that we instead get two distinct well-defined possibilities. More importantly though, this doesn't feel like a thing that MIRI can implement, which is concerning.

I also can't produce an example, but I would not be surprised if this is actually required for current Rust, since the evaluation order in Rust is well-defined. I can't get MIR building to produce such MIR, but it would at least imply that the temporaries produced by this are required:

pub fn foo(a: &mut i32) -> i32 { let derived = &mut *a; *derived + *a }

compiler/rustc_middle/src/mir/mod.rs

bjorn3 · 2022-03-26T06:36:47Z

compiler/rustc_const_eval/src/transform/validate.rs

+        // terribly important that they pass the validator. However, I think other passes might
+        // still see them, in which case they might be surprised. It would probably be better if we
+        // didn't put this through the MIR pipeline at all.
+        if matches!(body.source.instance, InstanceDef::Intrinsic(..) | InstanceDef::Virtual(..)) {


intrinsics may be codegened in the future.

Indeed. I think at that point this check should be refined to verify those bodies that do get codegened only. Even better would be if code elsewhere got changed to stop passing intrinsics that don't get codegened through the MIR validator. cc @oli-obk

compiler/rustc_middle/src/mir/mod.rs

bjorn3 · 2022-03-26T06:42:39Z

compiler/rustc_middle/src/mir/mod.rs

+    /// executed `StorageDead`? How about two `StorageLive`s without an intervening `StorageDead`?
+    /// Two `StorageDead`s without an intervening `StorageLive`? LLVM says yes, poison, yes. If the
+    /// answer to any of these is "no," is breaking that rule UB or is it an error to have a path in
+    /// the CFG that might do this?


Miri should be made to match whatever is decided. It is currently very relaxed.

bjorn3 · 2022-03-26T06:44:39Z

compiler/rustc_middle/src/mir/mod.rs

+    ///
+    /// **Needs clarification**: We currently require that the LHS place not overlap with any place
+    /// read as part of computation of the RHS. This requirement is under discussion in [#68364]. As
+    /// a part of this discussion, it is also unclear in what order the components are evaluated.


I think order should be left indetermimate so any attempt on depending on the order is UB.

(see the comment above - basically the same concerns apply here)

I think leaving evaluation order indeterminate is a bad idea. We should specify some order eventually, and probably it should be left-to-right. (That's what Miri does.)

The disjointness thing can be enforced without leaving the order indeterminate, using the aliasing model instead. That's better because it can be checked by Miri.

bjorn3 · 2022-03-26T06:49:39Z

compiler/rustc_middle/src/mir/terminator.rs

@@ -187,7 +248,7 @@ pub enum TerminatorKind<'tcx> {
        /// This allows the memory occupied by "by-value" arguments to be
        /// reused across function calls without duplicating the contents.
        args: Vec<Operand<'tcx>>,
-        /// Destination for the return value. If some, the call is converging.
+        /// Destination for the return value. If none, the call necessarily diverges.
        destination: Option<(Place<'tcx>, BasicBlock)>,


Must not alias the place given to any Operand::Move passed as argument. All codegen backends depend on this property to allow passing Operand::Move by reference in case of a type not fitting inside registers without having to male a copy on the stack or having to restrict the callee to not mutate the value.

I've added in a mention of that in the "needs clarification" section above. Based on the status of the linked issue, it seems that it is still unclear how we are justifying that optimization

compiler/rustc_const_eval/src/transform/validate.rs

compiler/rustc_middle/src/mir/mod.rs

JakobDegen · 2022-03-27T01:12:43Z

cc @RalfJung - want to check that everything written here (of specific concern is the place and operand stuff) is at least compatible with your model of MIR

RalfJung · 2022-04-07T01:58:09Z

compiler/rustc_middle/src/mir/mod.rs

+    /// Assign statements roughly correspond to an assignment in Rust proper (`x = ...`) except
+    /// without the possibility of dropping the previous value (that must be done separately, if at
+    /// all). The *exact* way this works is undecided. It probably does something like evaluating
+    /// the LHS and RHS, and then doing the inverse of a place to value conversion to write the


FWIW I think "place to value conversion" is a terrible term, though I know it is standard in C. This operation accesses memory! It shouldn't be described as if it was a simple cast.

~~So I'd say something more along the lines of: it evaluates the LHS and RHS to places, loads a value from the place denoted by the RHS, and then stores that value into the place denoted by the LHS.~~

But also, this doesn't even do place-to-value conversion? The RHS is evaluated to a value, and that is stored to the place denoted by the LHS.

RalfJung · 2022-04-07T02:05:58Z

compiler/rustc_middle/src/mir/mod.rs

+    /// **Needs clarification**: Is it permitted to have two `StorageLive`s without an intervening
+    /// `StorageDead`? Two `StorageDead`s without an intervening `StorageLive`? LLVM says poison,
+    /// yes. If the answer to any of these is "no," is breaking that rule UB or is it an error to
+    /// have a path in the CFG that might do this?


I don't think CFG paths are relevant. What's relevant is the actual sequence of statements that are executed when the function runs. There's basically a boolean flag dead-or-alive for each local that starts out 'dead' for locals with annotations, and that is set appropriately by these annotations. Accessing (evaluating the place expression) of a local is UB if it is currently 'dead'.

Actually it's more than that, each StorageLive allows the local to be put at a different location in memory.

I don't think CFG paths are relevant

Well, because of the MaybeStorageLiveLocals in the validator, this is already the case. Certainly CFG paths should not be relevant in the spec/miri, but the docs here have to also mention what is required by the validator, which might have a stronger opinion

I think the storage check currently present in the validator should be considered to be merely a debugging aid. It is possible to write unsafe code that uses local when it does not have storage. With additional optimizations present such code would fail the check. When we get to that point the check should be disabled.

Ah, this is a good point. With the current setup, we can't turn *r into x even if we know that r = &mut x;, because that might turn UB into an ICE. I'll mention this in the documentation

Come to think of it, we should probably mention that we often don't want to "remove" UB in MIR optimizations in general, since keeping it visible to the codegen backend is important.

For example, we don't want to lower switchInt [0 => bb3] otherwise: bbUnreachable to goto bb3, since that keeps LLVM from knowing about the impossible condition.

Yeah, these kinds of things should definitely be documented, but imo not here. I do want to start an organized way of defining "canonicalized MIR" at some point, because I think that will be increasingly important, and this seems like the kind of thing to include there

compiler/rustc_middle/src/mir/mod.rs

RalfJung · 2022-04-07T02:10:14Z

compiler/rustc_middle/src/mir/mod.rs

+///    complicated. They are only legal on parent places that are references, pointers, or `Box`. A
+///    `Deref` projection begins by creating a value from the parent place, as if by
+///    [`Operand::Copy`]. It then dereferences the resulting pointer, creating a place of the
+///    pointed to type.


Note that this operation can cause UB: the crated place must be dereferencable and suitably aligned, as determined by its type.

RalfJung · 2022-04-07T02:11:13Z

compiler/rustc_middle/src/mir/mod.rs

+/// One possible model that I believe makes sense is that "part of memory" is actually just the
+/// address of the beginning of the referred to range of bytes. For sized types, the size of the
+/// range is then stored in the type, and for unsized types it's stored (possibly indirectly,
+/// through a vtable) in the metadata.


That's what Miri does, and I think it is the right model.

RalfJung · 2022-04-07T02:13:06Z

compiler/rustc_middle/src/mir/mod.rs

+///
+/// [value-def]: https://github.com/rust-lang/unsafe-code-guidelines/blob/master/wip/value-domain.md
+///
+/// The most common way to create values is via a place to value conversion. A place to value


Ah now this is the right place for the rant that I put earlier. ;)

I don't think we should all this a "conversion". That sounds like an as cast, like just putting something into a different representation. That's not at all what happens though. This is a load from memory, and should be described as that.

compiler/rustc_middle/src/mir/mod.rs

…e conversions

oli-obk · 2022-04-11T19:57:41Z

@bors r+

bors · 2022-04-11T19:57:42Z

📌 Commit 8732bf5 has been approved by oli-obk

Document the current MIR semantics that are clear from existing code This PR adds documentation to places, operands, rvalues, statementkinds, and terminatorkinds that describes their existing semantics and requirements. In many places the semantics depend on the Rust memory model or other T-Lang decisions - when this is the case, it is just noted as such with links to UCG issues where possible. I'm hopeful that none of the documentation added here can be used to justify optimizations that depend on the memory model. The documentation for places and operands probably comes closest to running afoul of this - if people think that it cannot be merged as is, it can definitely also be taken out. The goal here is to only document parts of MIR that seem to be decided already, or are at least depended on by existing code. That leaves quite a number of open questions - those are marked as "needs clarification." I'm not sure what to do with those in this PR - we obviously can't decide all these questions here. Should I just leave them in as is? Take them out? Keep them in but as `//` instead of `///` comments? If this is too big to review at once, I can split this up. r? rust-lang/mir-opt

…askrgr Rollup of 7 pull requests Successful merges: - rust-lang#95320 (Document the current MIR semantics that are clear from existing code) - rust-lang#95722 (pre-push.sh: Use python3 if python is not found) - rust-lang#95881 (Use `to_string` instead of `format!`) - rust-lang#95909 (rustdoc: Reduce allocations in a `theme` function) - rust-lang#95910 (Fix crate_type attribute to not warn on duplicates) - rust-lang#95920 (use `Span::find_ancestor_inside` to get right span in CastCheck) - rust-lang#95936 (Fix a bad error message for `relative paths are not supported in visibilities` error) Failed merges: r? `@ghost` `@rustbot` modify labels: rollup

rustbot added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Mar 26, 2022

rust-highfive assigned oli-obk Mar 26, 2022

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Mar 26, 2022

scottmcm reviewed Mar 26, 2022

View reviewed changes

compiler/rustc_middle/src/mir/mod.rs Outdated Show resolved Hide resolved

bjorn3 reviewed Mar 26, 2022

View reviewed changes

compiler/rustc_middle/src/mir/mod.rs Outdated Show resolved Hide resolved

bjorn3 reviewed Mar 26, 2022

View reviewed changes

compiler/rustc_middle/src/mir/mod.rs Outdated Show resolved Hide resolved

bjorn3 reviewed Mar 26, 2022

View reviewed changes

compiler/rustc_middle/src/mir/mod.rs Outdated Show resolved Hide resolved

bjorn3 reviewed Mar 26, 2022

View reviewed changes

compiler/rustc_middle/src/mir/mod.rs Outdated Show resolved Hide resolved

bjorn3 reviewed Mar 26, 2022

View reviewed changes

compiler/rustc_middle/src/mir/mod.rs Outdated Show resolved Hide resolved

bjorn3 reviewed Mar 26, 2022

View reviewed changes

compiler/rustc_middle/src/mir/mod.rs Outdated Show resolved Hide resolved

bjorn3 reviewed Mar 26, 2022

View reviewed changes

compiler/rustc_middle/src/mir/mod.rs Outdated Show resolved Hide resolved

bjorn3 reviewed Mar 26, 2022

View reviewed changes

compiler/rustc_const_eval/src/transform/validate.rs Outdated Show resolved Hide resolved

SkiFire13 reviewed Mar 26, 2022

View reviewed changes

compiler/rustc_middle/src/mir/mod.rs Outdated Show resolved Hide resolved

RalfJung reviewed Apr 7, 2022

View reviewed changes

compiler/rustc_middle/src/mir/mod.rs Outdated Show resolved Hide resolved

RalfJung reviewed Apr 7, 2022

View reviewed changes

compiler/rustc_middle/src/mir/mod.rs Outdated Show resolved Hide resolved

RalfJung reviewed Apr 7, 2022

View reviewed changes

compiler/rustc_middle/src/mir/mod.rs Outdated Show resolved Hide resolved

RalfJung reviewed Apr 7, 2022

View reviewed changes

compiler/rustc_middle/src/mir/mod.rs Outdated Show resolved Hide resolved

JakobDegen added 8 commits April 11, 2022 15:18

Adjust computation of place types to detect more invalid places

2f4a7a0

Add documentation for the semantics of MIR rvalues

6343691

Extend the MIR validator to check many more things around rvalues.

9ac5e98

Improve documentation for MIR statement kinds.

8e01cd6

Improve documentation for MIR terminators

f1f25c0

Adjust MIR validator to check a few more things for terminators

f2d7908

Improve MIR phases documentation with summaries of changes

1d318e4

Address various comments and change some details around place to valu…

411ae6f

…e conversions

JakobDegen force-pushed the mir-docs branch from 9745a17 to a4df623 Compare April 11, 2022 19:23

JakobDegen added 2 commits April 11, 2022 15:56

Add more clarifications in response to Ralf's comments

4bce639

Remove rule that place loads may not happen with variant index set

8732bf5

JakobDegen force-pushed the mir-docs branch from a4df623 to 8732bf5 Compare April 11, 2022 19:56

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Apr 11, 2022

Dylan-DPC mentioned this pull request Apr 12, 2022

Rollup of 8 pull requests #95959

Closed

matthiaskrgr mentioned this pull request Apr 12, 2022

Rollup of 7 pull requests #95966

Merged

bors merged commit 1d35179 into rust-lang:master Apr 12, 2022

rustbot added this to the 1.62.0 milestone Apr 12, 2022

matthiaskrgr mentioned this pull request Apr 13, 2022

ICE: broken mir: Cannot compare unequal types fn(&str) and for<'r> fn(&'r str) with -Zvalidate-mir #95978

Closed

JakobDegen deleted the mir-docs branch April 17, 2022 19:20

beepster4096 mentioned this pull request May 9, 2022

Fix ProjectionElem validation #96856

Merged

compiler-errors mentioned this pull request May 29, 2022

rustc should not suggest #![feature(core_intrinsics)] #97501

Open

JakobDegen mentioned this pull request Jun 27, 2022

Revise how MIR variants are distinguished rust-lang/compiler-team#522

Closed

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Document the current MIR semantics that are clear from existing code #95320

Document the current MIR semantics that are clear from existing code #95320

JakobDegen commented Mar 26, 2022

JakobDegen commented Mar 26, 2022 •

edited

Loading

bjorn3 Mar 26, 2022

JakobDegen Mar 27, 2022

bjorn3 Mar 26, 2022

JakobDegen Mar 27, 2022 •

edited

Loading

bjorn3 Mar 26, 2022

JakobDegen Mar 27, 2022

bjorn3 Mar 26, 2022

bjorn3 Mar 26, 2022

JakobDegen Mar 27, 2022

RalfJung Apr 7, 2022

bjorn3 Mar 26, 2022 •

edited

Loading

JakobDegen Mar 27, 2022

JakobDegen commented Mar 27, 2022

RalfJung Apr 7, 2022 •

edited

Loading

RalfJung Apr 7, 2022

JakobDegen Apr 8, 2022

tmiasko Apr 8, 2022

JakobDegen Apr 8, 2022

scottmcm Apr 8, 2022

JakobDegen Apr 8, 2022

RalfJung Apr 7, 2022

RalfJung Apr 7, 2022

RalfJung Apr 7, 2022

oli-obk commented Apr 11, 2022

bors commented Apr 11, 2022

Document the current MIR semantics that are clear from existing code #95320

Document the current MIR semantics that are clear from existing code #95320

Conversation

JakobDegen commented Mar 26, 2022

JakobDegen commented Mar 26, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JakobDegen Mar 27, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bjorn3 Mar 26, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JakobDegen commented Mar 27, 2022

RalfJung Apr 7, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oli-obk commented Apr 11, 2022

bors commented Apr 11, 2022

JakobDegen commented Mar 26, 2022 •

edited

Loading

JakobDegen Mar 27, 2022 •

edited

Loading

bjorn3 Mar 26, 2022 •

edited

Loading

RalfJung Apr 7, 2022 •

edited

Loading