From 8a1c1ec8b2d806c135e19c748f0bc28188a79d00 Mon Sep 17 00:00:00 2001 From: pierwill Date: Thu, 7 Jul 2022 12:34:33 -0500 Subject: [PATCH 1/3] MIR dataflow: Rename function to `always_storage_live_locals` Related to #99021. --- compiler/rustc_const_eval/src/interpret/eval_context.rs | 4 ++-- compiler/rustc_const_eval/src/transform/validate.rs | 4 ++-- compiler/rustc_mir_dataflow/src/storage.rs | 2 +- compiler/rustc_mir_transform/src/generator.rs | 4 ++-- 4 files changed, 7 insertions(+), 7 deletions(-) diff --git a/compiler/rustc_const_eval/src/interpret/eval_context.rs b/compiler/rustc_const_eval/src/interpret/eval_context.rs index 3892d1920ce..2e47cf89210 100644 --- a/compiler/rustc_const_eval/src/interpret/eval_context.rs +++ b/compiler/rustc_const_eval/src/interpret/eval_context.rs @@ -15,7 +15,7 @@ use rustc_middle::ty::layout::{ use rustc_middle::ty::{ self, query::TyCtxtAt, subst::SubstsRef, ParamEnv, Ty, TyCtxt, TypeFoldable, }; -use rustc_mir_dataflow::storage::always_live_locals; +use rustc_mir_dataflow::storage::always_storage_live_locals; use rustc_query_system::ich::StableHashingContext; use rustc_session::Limit; use rustc_span::{Pos, Span}; @@ -707,7 +707,7 @@ impl<'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> { let mut locals = IndexVec::from_elem(dummy, &body.local_decls); // Now mark those locals as live that have no `Storage*` annotations. - let always_live = always_live_locals(self.body()); + let always_live = always_storage_live_locals(self.body()); for local in locals.indices() { if always_live.contains(local) { locals[local].value = LocalValue::Live(Operand::Immediate(Immediate::Uninit)); diff --git a/compiler/rustc_const_eval/src/transform/validate.rs b/compiler/rustc_const_eval/src/transform/validate.rs index 7b9c6329d32..a02d17283f5 100644 --- a/compiler/rustc_const_eval/src/transform/validate.rs +++ b/compiler/rustc_const_eval/src/transform/validate.rs @@ -14,7 +14,7 @@ use rustc_middle::mir::{ use rustc_middle::ty::fold::BottomUpFolder; use rustc_middle::ty::{self, InstanceDef, ParamEnv, Ty, TyCtxt, TypeFoldable, TypeVisitable}; use rustc_mir_dataflow::impls::MaybeStorageLive; -use rustc_mir_dataflow::storage::always_live_locals; +use rustc_mir_dataflow::storage::always_storage_live_locals; use rustc_mir_dataflow::{Analysis, ResultsCursor}; use rustc_target::abi::{Size, VariantIdx}; @@ -48,7 +48,7 @@ impl<'tcx> MirPass<'tcx> for Validator { let param_env = tcx.param_env(def_id); let mir_phase = self.mir_phase; - let always_live_locals = always_live_locals(body); + let always_live_locals = always_storage_live_locals(body); let storage_liveness = MaybeStorageLive::new(always_live_locals) .into_engine(tcx, body) .iterate_to_fixpoint() diff --git a/compiler/rustc_mir_dataflow/src/storage.rs b/compiler/rustc_mir_dataflow/src/storage.rs index 4a354c4c65b..566c9d2d505 100644 --- a/compiler/rustc_mir_dataflow/src/storage.rs +++ b/compiler/rustc_mir_dataflow/src/storage.rs @@ -7,7 +7,7 @@ use rustc_middle::mir::{self, Local}; // // FIXME: Currently, we need to traverse the entire MIR to compute this. We should instead store it // as a field in the `LocalDecl` for each `Local`. -pub fn always_live_locals(body: &mir::Body<'_>) -> BitSet { +pub fn always_storage_live_locals(body: &mir::Body<'_>) -> BitSet { let mut always_live_locals = BitSet::new_filled(body.local_decls.len()); for block in body.basic_blocks() { diff --git a/compiler/rustc_mir_transform/src/generator.rs b/compiler/rustc_mir_transform/src/generator.rs index 9078b8a1cb7..9fdea835967 100644 --- a/compiler/rustc_mir_transform/src/generator.rs +++ b/compiler/rustc_mir_transform/src/generator.rs @@ -67,7 +67,7 @@ use rustc_middle::ty::{self, AdtDef, Ty, TyCtxt}; use rustc_mir_dataflow::impls::{ MaybeBorrowedLocals, MaybeLiveLocals, MaybeRequiresStorage, MaybeStorageLive, }; -use rustc_mir_dataflow::storage; +use rustc_mir_dataflow::storage::always_storage_live_locals; use rustc_mir_dataflow::{self, Analysis}; use rustc_target::abi::VariantIdx; use rustc_target::spec::PanicStrategy; @@ -1379,7 +1379,7 @@ impl<'tcx> MirPass<'tcx> for StateTransform { }, ); - let always_live_locals = storage::always_live_locals(&body); + let always_live_locals = always_storage_live_locals(&body); let liveness_info = locals_live_across_suspend_points(tcx, body, &always_live_locals, movable); From bdf1d22515ba3a657e2a608e84ded6e5f0c76d1a Mon Sep 17 00:00:00 2001 From: est31 Date: Fri, 8 Jul 2022 22:41:41 +0200 Subject: [PATCH 2/3] Intra-doc-link-ify reference to Clone::clone_from --- library/alloc/src/borrow.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/library/alloc/src/borrow.rs b/library/alloc/src/borrow.rs index 7a79fb77dea..904a53bb4ac 100644 --- a/library/alloc/src/borrow.rs +++ b/library/alloc/src/borrow.rs @@ -60,7 +60,7 @@ pub trait ToOwned { /// Uses borrowed data to replace owned data, usually by cloning. /// - /// This is borrow-generalized version of `Clone::clone_from`. + /// This is borrow-generalized version of [`Clone::clone_from`]. /// /// # Examples /// From 4939f6c64b12c0392f824f812b9bcad9bf3d1019 Mon Sep 17 00:00:00 2001 From: Jakob Degen Date: Fri, 8 Jul 2022 00:58:48 -0700 Subject: [PATCH 3/3] Clarify MIR semantics of storage statements --- .../src/transform/validate.rs | 8 ++++++- compiler/rustc_middle/src/mir/syntax.rs | 22 +++++++++---------- 2 files changed, 18 insertions(+), 12 deletions(-) diff --git a/compiler/rustc_const_eval/src/transform/validate.rs b/compiler/rustc_const_eval/src/transform/validate.rs index 7b9c6329d32..10416f081b8 100644 --- a/compiler/rustc_const_eval/src/transform/validate.rs +++ b/compiler/rustc_const_eval/src/transform/validate.rs @@ -205,7 +205,13 @@ impl<'a, 'tcx> Visitor<'tcx> for TypeChecker<'a, 'tcx> { } if self.reachable_blocks.contains(location.block) && context.is_use() { - // Uses of locals must occur while the local's storage is allocated. + // We check that the local is live whenever it is used. Technically, violating this + // restriction is only UB and not actually indicative of not well-formed MIR. This means + // that an optimization which turns MIR that already has UB into MIR that fails this + // check is not necessarily wrong. However, we have no such optimizations at the moment, + // and so we include this check anyway to help us catch bugs. If you happen to write an + // optimization that might cause this to incorrectly fire, feel free to remove this + // check. self.storage_liveness.seek_after_primary_effect(location); let locals_with_storage = self.storage_liveness.get(); if !locals_with_storage.contains(local) { diff --git a/compiler/rustc_middle/src/mir/syntax.rs b/compiler/rustc_middle/src/mir/syntax.rs index dcc37c565c9..45fc5f24a60 100644 --- a/compiler/rustc_middle/src/mir/syntax.rs +++ b/compiler/rustc_middle/src/mir/syntax.rs @@ -237,19 +237,19 @@ pub enum StatementKind<'tcx> { /// `StorageLive` and `StorageDead` statements mark the live range of a local. /// - /// Using a local before a `StorageLive` or after a `StorageDead` is not well-formed. These - /// statements are not required. If the entire MIR body contains no `StorageLive`/`StorageDead` - /// statements for a particular local, the local is always considered live. + /// At any point during the execution of a function, each local is either allocated or + /// unallocated. Except as noted below, all locals except function parameters are initially + /// unallocated. `StorageLive` statements cause memory to be allocated for the local while + /// `StorageDead` statements cause the memory to be freed. Using a local in any way (not only + /// reading/writing from it) while it is unallocated is UB. /// - /// More precisely, the MIR validator currently does a `MaybeStorageLiveLocals` analysis to - /// check validity of each use of a local. I believe this is equivalent to requiring for every - /// use of a local, there exist at least one path from the root to that use that contains a - /// `StorageLive` more recently than a `StorageDead`. + /// Some locals have no `StorageLive` or `StorageDead` statements within the entire MIR body. + /// These locals are implicitly allocated for the full duration of the function. There is a + /// convenience method at `rustc_mir_dataflow::storage::always_storage_live_locals` for + /// computing these locals. /// - /// **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? + /// If the local is already allocated, calling `StorageLive` again is UB. However, for an + /// unallocated local an additional `StorageDead` all is simply a nop. StorageLive(Local), /// See `StorageLive` above.