From 21bdfa14cbd99aba173751a2c723fb1e65de401b Mon Sep 17 00:00:00 2001 From: Aurelia Molzer <5550310+HeroicKatora@users.noreply.github.com> Date: Sat, 9 Aug 2025 17:54:43 +0200 Subject: [PATCH 1/5] Ensure consistent drop for hint::select_unpredictable There are a few alternatives to the implementation. The principal problem is that the selected value must be owned (in the sense of having any drop flag of sorts) when the unselected value is dropped, such that panic unwind goes through the drop of both. This ownership must then be passed on in return when the drop went smoothly. The basic way of achieving this is by extracting the selected value first, at the cost of relying on the optimizer a little more for detecting the copy as constructing the return value despite having a place in the body. --- library/core/src/hint.rs | 6 +++++- library/coretests/tests/hint.rs | 37 +++++++++++++++++++++++++++++++++ 2 files changed, 42 insertions(+), 1 deletion(-) diff --git a/library/core/src/hint.rs b/library/core/src/hint.rs index c72eeb9a9c976..53c17d6c887c3 100644 --- a/library/core/src/hint.rs +++ b/library/core/src/hint.rs @@ -790,8 +790,12 @@ pub fn select_unpredictable(condition: bool, true_val: T, false_val: T) -> T // is returned. This is necessary because the intrinsic doesn't drop the // value that is not selected. unsafe { + // Extract the selected value first, ensure it is dropped as well if dropping the unselected + // value panics. + let ret = crate::intrinsics::select_unpredictable(condition, &true_val, &false_val) + .assume_init_read(); crate::intrinsics::select_unpredictable(!condition, &mut true_val, &mut false_val) .assume_init_drop(); - crate::intrinsics::select_unpredictable(condition, true_val, false_val).assume_init() + ret } } diff --git a/library/coretests/tests/hint.rs b/library/coretests/tests/hint.rs index 032bbc1dcc80f..9ef5567681e93 100644 --- a/library/coretests/tests/hint.rs +++ b/library/coretests/tests/hint.rs @@ -21,3 +21,40 @@ fn select_unpredictable_drop() { assert!(a_dropped.get()); assert!(b_dropped.get()); } + +#[test] +#[should_panic] +fn select_unpredictable_drop_on_panic() { + use core::cell::Cell; + + struct X<'a> { + cell: &'a Cell, + expect: u16, + write: u16, + } + + impl Drop for X<'_> { + fn drop(&mut self) { + let value = self.cell.get(); + self.cell.set(self.write); + assert_eq!(value, self.expect); + } + } + + let cell = Cell::new(0); + + // Trigger a double-panic if the selected cell was not dropped during panic. + let _armed = X { cell: &cell, expect: 0xdead, write: 0 }; + let selected = X { cell: &cell, write: 0xdead, expect: 1 }; + let unselected = X { cell: &cell, write: 1, expect: 0xff }; + + // The correct drop order is: + // + // 1. `unselected` drops, writes 1, and panics as 0 != 0xff + // 2. `selected` drops during unwind, writes 0xdead and does not panic as 1 == 1 + // 3. `armed` drops during unwind, writes 0 and does not panic as 0xdead == 0xdead + // + // If `selected` is not dropped, `armed` panics as 1 != 0xdead + let _unreachable = + core::hint::select_unpredictable(core::hint::black_box(true), selected, unselected); +} From 7d7fe3ff05c7c14834252e3345389e4dbf842cc5 Mon Sep 17 00:00:00 2001 From: Aurelia Molzer <5550310+HeroicKatora@users.noreply.github.com> Date: Sat, 9 Aug 2025 21:40:12 +0200 Subject: [PATCH 2/5] Adjust for codegen of unpredictable return value It seems important for LLVM that we select on the values by-value instead of reading and have no intermediate store. So make sure the guards selects both potential drops but defers the return value to the second selection. Since the two options alias we use raw mutable pointers instead of mutable references as before. --- library/core/src/hint.rs | 36 +++++++++++++++++++++++++++++++----- 1 file changed, 31 insertions(+), 5 deletions(-) diff --git a/library/core/src/hint.rs b/library/core/src/hint.rs index 53c17d6c887c3..1bb1c56950879 100644 --- a/library/core/src/hint.rs +++ b/library/core/src/hint.rs @@ -786,16 +786,42 @@ pub fn select_unpredictable(condition: bool, true_val: T, false_val: T) -> T // Change this to use ManuallyDrop instead. let mut true_val = MaybeUninit::new(true_val); let mut false_val = MaybeUninit::new(false_val); + + struct DropOnPanic { + inner: *mut MaybeUninit, + } + + impl Drop for DropOnPanic { + fn drop(&mut self) { + // SAFETY: Must be guaranteed on construction of local type `DropOnPanic`. + unsafe { (*self.inner).assume_init_drop() } + } + } + + let true_ptr = (&mut true_val) as *mut _; + let false_ptr = (&mut false_val) as *mut _; + // SAFETY: The value that is not selected is dropped, and the selected one // is returned. This is necessary because the intrinsic doesn't drop the // value that is not selected. unsafe { // Extract the selected value first, ensure it is dropped as well if dropping the unselected // value panics. - let ret = crate::intrinsics::select_unpredictable(condition, &true_val, &false_val) - .assume_init_read(); - crate::intrinsics::select_unpredictable(!condition, &mut true_val, &mut false_val) - .assume_init_drop(); - ret + let (guard, drop) = crate::intrinsics::select_unpredictable( + condition, + (true_ptr, false_ptr), + (false_ptr, true_ptr), + ); + + // SAFETY: both pointers are to valid `MaybeUninit`, in both variants they do not alias but + // the two arguments we have selected from did alias each other. + let guard = DropOnPanic { inner: guard }; + (*drop).assume_init_drop(); + crate::mem::forget(guard); + + // Note that it is important to use the values here. Reading from the pointer we got makes + // LLVM forget the !unpredictable annotation sometimes (in tests, integer sized values in + // particular seemed to confuse it, also observed in llvm/llvm-project #82340). + crate::intrinsics::select_unpredictable(condition, true_val, false_val).assume_init() } } From 539f8400e769ad6092b5fe1cac0ebe885e5543a8 Mon Sep 17 00:00:00 2001 From: Aurelia Molzer <5550310+HeroicKatora@users.noreply.github.com> Date: Sat, 30 Aug 2025 11:50:28 +0200 Subject: [PATCH 3/5] Clarify panic-drop test for select_unpredictable --- library/coretests/tests/hint.rs | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/library/coretests/tests/hint.rs b/library/coretests/tests/hint.rs index 9ef5567681e93..24de27b24b802 100644 --- a/library/coretests/tests/hint.rs +++ b/library/coretests/tests/hint.rs @@ -23,7 +23,7 @@ fn select_unpredictable_drop() { } #[test] -#[should_panic] +#[should_panic = "message canary"] fn select_unpredictable_drop_on_panic() { use core::cell::Cell; @@ -37,7 +37,7 @@ fn select_unpredictable_drop_on_panic() { fn drop(&mut self) { let value = self.cell.get(); self.cell.set(self.write); - assert_eq!(value, self.expect); + assert_eq!(value, self.expect, "message canary"); } } @@ -55,6 +55,5 @@ fn select_unpredictable_drop_on_panic() { // 3. `armed` drops during unwind, writes 0 and does not panic as 0xdead == 0xdead // // If `selected` is not dropped, `armed` panics as 1 != 0xdead - let _unreachable = - core::hint::select_unpredictable(core::hint::black_box(true), selected, unselected); + let _unreachable = core::hint::select_unpredictable(true, selected, unselected); } From 354787b5ba8210070929e7454312c51da8e6f988 Mon Sep 17 00:00:00 2001 From: Aurelia Molzer <5550310+HeroicKatora@users.noreply.github.com> Date: Sat, 30 Aug 2025 11:58:48 +0200 Subject: [PATCH 4/5] Simplify select_unpredictable guard selection Instead of a tuple, select the dropped value and its guard with two separate calls to the intrinsic which makes both calls have a pointer-valued argument that should be simpler in codegen. Use the same condition on all (not an inverted condition) to clarify the intent of parallel selection. This should also be a simpler value-dependency chain if the guard is deduced unused (i.e. drop_in_place a noop for the type). --- library/core/src/hint.rs | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/library/core/src/hint.rs b/library/core/src/hint.rs index 1bb1c56950879..852882aa7f4c7 100644 --- a/library/core/src/hint.rs +++ b/library/core/src/hint.rs @@ -788,6 +788,7 @@ pub fn select_unpredictable(condition: bool, true_val: T, false_val: T) -> T let mut false_val = MaybeUninit::new(false_val); struct DropOnPanic { + // Invariant: valid pointer and points to an initialized `MaybeUninit`. inner: *mut MaybeUninit, } @@ -806,12 +807,11 @@ pub fn select_unpredictable(condition: bool, true_val: T, false_val: T) -> T // value that is not selected. unsafe { // Extract the selected value first, ensure it is dropped as well if dropping the unselected - // value panics. - let (guard, drop) = crate::intrinsics::select_unpredictable( - condition, - (true_ptr, false_ptr), - (false_ptr, true_ptr), - ); + // value panics. We construct a temporary by-pointer guard around the selected value while + // dropping the unselected value. Arguments overlap here, so we can not use mutable + // reference for these arguments. + let guard = crate::intrinsics::select_unpredictable(condition, true_ptr, false_ptr); + let drop = crate::intrinsics::select_unpredictable(condition, false_ptr, true_ptr); // SAFETY: both pointers are to valid `MaybeUninit`, in both variants they do not alias but // the two arguments we have selected from did alias each other. From ee9803a244d65a830a999f8066da2a70e1972d5e Mon Sep 17 00:00:00 2001 From: Aurelia Molzer <5550310+HeroicKatora@users.noreply.github.com> Date: Sat, 30 Aug 2025 13:00:02 +0200 Subject: [PATCH 5/5] Switch select_unpredictable guard to raw pointer --- library/core/src/hint.rs | 19 +++++++++++-------- 1 file changed, 11 insertions(+), 8 deletions(-) diff --git a/library/core/src/hint.rs b/library/core/src/hint.rs index 852882aa7f4c7..89c64f6d28fef 100644 --- a/library/core/src/hint.rs +++ b/library/core/src/hint.rs @@ -788,19 +788,20 @@ pub fn select_unpredictable(condition: bool, true_val: T, false_val: T) -> T let mut false_val = MaybeUninit::new(false_val); struct DropOnPanic { - // Invariant: valid pointer and points to an initialized `MaybeUninit`. - inner: *mut MaybeUninit, + // Invariant: valid pointer and points to an initialized value that is not further used, + // i.e. it can be dropped by this guard. + inner: *mut T, } impl Drop for DropOnPanic { fn drop(&mut self) { // SAFETY: Must be guaranteed on construction of local type `DropOnPanic`. - unsafe { (*self.inner).assume_init_drop() } + unsafe { self.inner.drop_in_place() } } } - let true_ptr = (&mut true_val) as *mut _; - let false_ptr = (&mut false_val) as *mut _; + let true_ptr = true_val.as_mut_ptr(); + let false_ptr = false_val.as_mut_ptr(); // SAFETY: The value that is not selected is dropped, and the selected one // is returned. This is necessary because the intrinsic doesn't drop the @@ -813,10 +814,12 @@ pub fn select_unpredictable(condition: bool, true_val: T, false_val: T) -> T let guard = crate::intrinsics::select_unpredictable(condition, true_ptr, false_ptr); let drop = crate::intrinsics::select_unpredictable(condition, false_ptr, true_ptr); - // SAFETY: both pointers are to valid `MaybeUninit`, in both variants they do not alias but - // the two arguments we have selected from did alias each other. + // SAFETY: both pointers are well-aligned and point to initialized values inside a + // `MaybeUninit` each. In both possible values for `condition` the pointer `guard` and + // `drop` do not alias (even though the two argument pairs we have selected from did alias + // each other). let guard = DropOnPanic { inner: guard }; - (*drop).assume_init_drop(); + drop.drop_in_place(); crate::mem::forget(guard); // Note that it is important to use the values here. Reading from the pointer we got makes