From 9b05e154f39f1a413ebc09c8f1dd751f991b3bba Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Sat, 8 Jun 2024 12:04:38 +0200 Subject: [PATCH 1/2] StorageLive: refresh storage (instead of UB) when local is already live --- compiler/rustc_const_eval/messages.ftl | 2 -- .../rustc_const_eval/src/interpret/eval_context.rs | 8 +++----- compiler/rustc_middle/src/mir/syntax.rs | 11 +++++++---- 3 files changed, 10 insertions(+), 11 deletions(-) diff --git a/compiler/rustc_const_eval/messages.ftl b/compiler/rustc_const_eval/messages.ftl index 2dbeb7d5e0ca4..14c036b8df9b5 100644 --- a/compiler/rustc_const_eval/messages.ftl +++ b/compiler/rustc_const_eval/messages.ftl @@ -73,8 +73,6 @@ const_eval_division_by_zero = dividing by zero const_eval_division_overflow = overflow in signed division (dividing MIN by -1) -const_eval_double_storage_live = - StorageLive on a local that was already live const_eval_dyn_call_not_a_method = `dyn` call trying to call something that is not a method diff --git a/compiler/rustc_const_eval/src/interpret/eval_context.rs b/compiler/rustc_const_eval/src/interpret/eval_context.rs index 7c2100fcbe38a..c1daa2fac338e 100644 --- a/compiler/rustc_const_eval/src/interpret/eval_context.rs +++ b/compiler/rustc_const_eval/src/interpret/eval_context.rs @@ -1103,11 +1103,9 @@ impl<'tcx, M: Machine<'tcx>> InterpCx<'tcx, M> { Operand::Immediate(Immediate::Uninit) }); - // StorageLive expects the local to be dead, and marks it live. + // If the local is already live, deallocate its old memory. let old = mem::replace(&mut self.frame_mut().locals[local].value, local_val); - if !matches!(old, LocalValue::Dead) { - throw_ub_custom!(fluent::const_eval_double_storage_live); - } + self.deallocate_local(old)?; Ok(()) } @@ -1121,7 +1119,7 @@ impl<'tcx, M: Machine<'tcx>> InterpCx<'tcx, M> { assert!(local != mir::RETURN_PLACE, "Cannot make return place dead"); trace!("{:?} is now dead", local); - // It is entirely okay for this local to be already dead (at least that's how we currently generate MIR) + // If the local is already dead, this is a NOP. let old = mem::replace(&mut self.frame_mut().locals[local].value, LocalValue::Dead); self.deallocate_local(old)?; Ok(()) diff --git a/compiler/rustc_middle/src/mir/syntax.rs b/compiler/rustc_middle/src/mir/syntax.rs index ebe77a1abfd8b..9855c72197edc 100644 --- a/compiler/rustc_middle/src/mir/syntax.rs +++ b/compiler/rustc_middle/src/mir/syntax.rs @@ -361,16 +361,19 @@ pub enum StatementKind<'tcx> { /// 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. + /// `StorageDead` statements cause the memory to be freed. In other words, + /// `StorageLive`/`StorageDead` act like the heap operations `allocate`/`deallocate`, but for + /// stack-allocated local variables. Using a local in any way (not only reading/writing from it) + /// while it is unallocated is UB. /// /// 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. /// - /// If the local is already allocated, calling `StorageLive` again is UB. However, for an - /// unallocated local an additional `StorageDead` all is simply a nop. + /// If the local is already allocated, calling `StorageLive` again will implicitly free the + /// local and then allocate fresh uninitilized memory. If a local is already deallocated, + /// calling `StorageDead` again is a NOP. StorageLive(Local), /// See `StorageLive` above. From 408c8eb983b2c460e56e1e71db26d6b7e76aa695 Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Fri, 14 Jun 2024 12:14:32 +0200 Subject: [PATCH 2/2] add Miri tests --- .../miri/tests/fail/storage-live-dead-var.rs | 14 ++++++++++++++ .../tests/fail/storage-live-dead-var.stderr | 15 +++++++++++++++ .../miri/tests/fail/storage-live-resets-var.rs | 17 +++++++++++++++++ .../tests/fail/storage-live-resets-var.stderr | 15 +++++++++++++++ 4 files changed, 61 insertions(+) create mode 100644 src/tools/miri/tests/fail/storage-live-dead-var.rs create mode 100644 src/tools/miri/tests/fail/storage-live-dead-var.stderr create mode 100644 src/tools/miri/tests/fail/storage-live-resets-var.rs create mode 100644 src/tools/miri/tests/fail/storage-live-resets-var.stderr diff --git a/src/tools/miri/tests/fail/storage-live-dead-var.rs b/src/tools/miri/tests/fail/storage-live-dead-var.rs new file mode 100644 index 0000000000000..83ab98d79d1a1 --- /dev/null +++ b/src/tools/miri/tests/fail/storage-live-dead-var.rs @@ -0,0 +1,14 @@ +#![feature(core_intrinsics, custom_mir)] +use std::intrinsics::mir::*; + +#[custom_mir(dialect = "runtime")] +fn main() { + mir! { + let val: i32; + { + val = 42; //~ERROR: accessing a dead local variable + StorageLive(val); // too late... (but needs to be here to make `val` not implicitly live) + Return() + } + } +} diff --git a/src/tools/miri/tests/fail/storage-live-dead-var.stderr b/src/tools/miri/tests/fail/storage-live-dead-var.stderr new file mode 100644 index 0000000000000..ccc77b1c978dc --- /dev/null +++ b/src/tools/miri/tests/fail/storage-live-dead-var.stderr @@ -0,0 +1,15 @@ +error: Undefined Behavior: accessing a dead local variable + --> $DIR/storage-live-dead-var.rs:LL:CC + | +LL | val = 42; + | ^^^^^^^^ accessing a dead local variable + | + = help: this indicates a bug in the program: it performed an invalid operation, and caused Undefined Behavior + = help: see https://doc.rust-lang.org/nightly/reference/behavior-considered-undefined.html for further information + = note: BACKTRACE: + = note: inside `main` at $DIR/storage-live-dead-var.rs:LL:CC + +note: some details are omitted, run with `MIRIFLAGS=-Zmiri-backtrace=full` for a verbose backtrace + +error: aborting due to 1 previous error + diff --git a/src/tools/miri/tests/fail/storage-live-resets-var.rs b/src/tools/miri/tests/fail/storage-live-resets-var.rs new file mode 100644 index 0000000000000..bfdd9e78943e9 --- /dev/null +++ b/src/tools/miri/tests/fail/storage-live-resets-var.rs @@ -0,0 +1,17 @@ +#![feature(core_intrinsics, custom_mir)] +use std::intrinsics::mir::*; + +#[custom_mir(dialect = "runtime")] +fn main() { + mir! { + let val: i32; + let _val2: i32; + { + StorageLive(val); + val = 42; + StorageLive(val); // reset val to `uninit` + _val2 = val; //~ERROR: uninitialized + Return() + } + } +} diff --git a/src/tools/miri/tests/fail/storage-live-resets-var.stderr b/src/tools/miri/tests/fail/storage-live-resets-var.stderr new file mode 100644 index 0000000000000..07d39cc9d6b3f --- /dev/null +++ b/src/tools/miri/tests/fail/storage-live-resets-var.stderr @@ -0,0 +1,15 @@ +error: Undefined Behavior: constructing invalid value: encountered uninitialized memory, but expected an integer + --> $DIR/storage-live-resets-var.rs:LL:CC + | +LL | _val2 = val; + | ^^^^^^^^^^^ constructing invalid value: encountered uninitialized memory, but expected an integer + | + = help: this indicates a bug in the program: it performed an invalid operation, and caused Undefined Behavior + = help: see https://doc.rust-lang.org/nightly/reference/behavior-considered-undefined.html for further information + = note: BACKTRACE: + = note: inside `main` at $DIR/storage-live-resets-var.rs:LL:CC + +note: some details are omitted, run with `MIRIFLAGS=-Zmiri-backtrace=full` for a verbose backtrace + +error: aborting due to 1 previous error +