From 6b4f96d492e768a86e776310386f384a9b651951 Mon Sep 17 00:00:00 2001 From: Ben Kimock Date: Thu, 9 Oct 2025 19:00:21 -0400 Subject: [PATCH 1/4] Change int-to-ptr transmute lowering back to inttoptr (cherry picked from commit 029579d1770fb23c693b2ecbab724fe3af66349b) --- compiler/rustc_codegen_ssa/src/mir/rvalue.rs | 4 ++-- tests/codegen-llvm/common_prim_int_ptr.rs | 2 +- tests/codegen-llvm/enum/enum-aggregate.rs | 2 +- .../int-ptr-int-enum-miscompile.rs | 22 +++++++++++++++++++ .../intrinsics/transmute-niched.rs | 2 +- tests/codegen-llvm/intrinsics/transmute.rs | 4 ++-- tests/codegen-llvm/transmute-scalar.rs | 2 +- 7 files changed, 30 insertions(+), 8 deletions(-) create mode 100644 tests/codegen-llvm/int-ptr-int-enum-miscompile.rs diff --git a/compiler/rustc_codegen_ssa/src/mir/rvalue.rs b/compiler/rustc_codegen_ssa/src/mir/rvalue.rs index f6f2e3f2a3a3c..addd41dbd2b36 100644 --- a/compiler/rustc_codegen_ssa/src/mir/rvalue.rs +++ b/compiler/rustc_codegen_ssa/src/mir/rvalue.rs @@ -1048,14 +1048,14 @@ pub(super) fn transmute_scalar<'a, 'tcx, Bx: BuilderMethods<'a, 'tcx>>( imm = match (from_scalar.primitive(), to_scalar.primitive()) { (Int(..) | Float(_), Int(..) | Float(_)) => bx.bitcast(imm, to_backend_ty), (Pointer(..), Pointer(..)) => bx.pointercast(imm, to_backend_ty), - (Int(..), Pointer(..)) => bx.ptradd(bx.const_null(bx.type_ptr()), imm), + (Int(..), Pointer(..)) => bx.inttoptr(imm, to_backend_ty), (Pointer(..), Int(..)) => { // FIXME: this exposes the provenance, which shouldn't be necessary. bx.ptrtoint(imm, to_backend_ty) } (Float(_), Pointer(..)) => { let int_imm = bx.bitcast(imm, bx.cx().type_isize()); - bx.ptradd(bx.const_null(bx.type_ptr()), int_imm) + bx.inttoptr(int_imm, to_backend_ty) } (Pointer(..), Float(_)) => { // FIXME: this exposes the provenance, which shouldn't be necessary. diff --git a/tests/codegen-llvm/common_prim_int_ptr.rs b/tests/codegen-llvm/common_prim_int_ptr.rs index 53716adccbf21..2c906e86bd736 100644 --- a/tests/codegen-llvm/common_prim_int_ptr.rs +++ b/tests/codegen-llvm/common_prim_int_ptr.rs @@ -11,7 +11,7 @@ #[no_mangle] pub fn insert_int(x: usize) -> Result> { // CHECK: start: - // CHECK-NEXT: %[[WO_PROV:.+]] = getelementptr i8, ptr null, [[USIZE:i[0-9]+]] %x + // CHECK-NEXT: %[[WO_PROV:.+]] = inttoptr [[USIZE:i[0-9]+]] %x to ptr // CHECK-NEXT: %[[R:.+]] = insertvalue { [[USIZE]], ptr } { [[USIZE]] 0, ptr poison }, ptr %[[WO_PROV]], 1 // CHECK-NEXT: ret { [[USIZE]], ptr } %[[R]] Ok(x) diff --git a/tests/codegen-llvm/enum/enum-aggregate.rs b/tests/codegen-llvm/enum/enum-aggregate.rs index f58d7ef12b661..e87f43b2e27b9 100644 --- a/tests/codegen-llvm/enum/enum-aggregate.rs +++ b/tests/codegen-llvm/enum/enum-aggregate.rs @@ -71,7 +71,7 @@ fn make_ok_ptr(x: NonNull) -> Result, usize> { fn make_ok_int(x: usize) -> Result> { // CHECK-LABEL: { i64, ptr } @make_ok_int(i64 %x) // CHECK-NEXT: start: - // CHECK-NEXT: %[[NOPROV:.+]] = getelementptr i8, ptr null, i64 %x + // CHECK-NEXT: %[[NOPROV:.+]] = inttoptr i64 %x to ptr // CHECK-NEXT: %[[R:.+]] = insertvalue { i64, ptr } { i64 0, ptr poison }, ptr %[[NOPROV]], 1 // CHECK-NEXT: ret { i64, ptr } %[[R]] Ok(x) diff --git a/tests/codegen-llvm/int-ptr-int-enum-miscompile.rs b/tests/codegen-llvm/int-ptr-int-enum-miscompile.rs new file mode 100644 index 0000000000000..299c3bf8847b2 --- /dev/null +++ b/tests/codegen-llvm/int-ptr-int-enum-miscompile.rs @@ -0,0 +1,22 @@ +// This is a regression test for https://github.com/rust-lang/rust/issues/147265. + +//@ compile-flags: -Copt-level=3 + +#![crate_type = "lib"] + +#[no_mangle] +pub fn mk_result(a: usize) -> Result { + // CHECK-LABEL: @mk_result + // CHECK-NOT: unreachable + // CHECK: load i8, + // CHECK-NOT: unreachable + match g(a) { + Ok(b) => Ok(unsafe { *(b as *const u8) }), + Err(c) => Err(c), + } +} + +#[cold] +fn g(a: usize) -> Result { + Ok(a) +} diff --git a/tests/codegen-llvm/intrinsics/transmute-niched.rs b/tests/codegen-llvm/intrinsics/transmute-niched.rs index a886d9eee5909..8c647655f65bb 100644 --- a/tests/codegen-llvm/intrinsics/transmute-niched.rs +++ b/tests/codegen-llvm/intrinsics/transmute-niched.rs @@ -249,7 +249,7 @@ pub unsafe fn check_four_or_eight_to_nonnull(x: FourOrEight) -> NonNull { // OPT: call void @llvm.assume(i1 %1) // CHECK-NOT: icmp // CHECK-NOT: assume - // CHECK: %[[RET:.+]] = getelementptr i8, ptr null, i64 %x + // CHECK: %[[RET:.+]] = inttoptr i64 %x to ptr // CHECK-NEXT: ret ptr %[[RET]] transmute(x) diff --git a/tests/codegen-llvm/intrinsics/transmute.rs b/tests/codegen-llvm/intrinsics/transmute.rs index e327c100d7ddd..b86c6df3267e4 100644 --- a/tests/codegen-llvm/intrinsics/transmute.rs +++ b/tests/codegen-llvm/intrinsics/transmute.rs @@ -303,7 +303,7 @@ pub unsafe fn check_pair_with_bool(x: (u8, bool)) -> (bool, i8) { pub unsafe fn check_float_to_pointer(x: f64) -> *const () { // CHECK-NOT: alloca // CHECK: %0 = bitcast double %x to i64 - // CHECK: %_0 = getelementptr i8, ptr null, i64 %0 + // CHECK: %_0 = inttoptr i64 %0 to ptr // CHECK: ret ptr %_0 transmute(x) } @@ -378,7 +378,7 @@ pub unsafe fn check_issue_110005(x: (usize, bool)) -> Option> { // CHECK-LABEL: @check_pair_to_dst_ref( #[no_mangle] pub unsafe fn check_pair_to_dst_ref<'a>(x: (usize, usize)) -> &'a [u8] { - // CHECK: %_0.0 = getelementptr i8, ptr null, i64 %x.0 + // CHECK: %_0.0 = inttoptr i64 %x.0 to ptr // CHECK: %0 = icmp ne ptr %_0.0, null // CHECK: call void @llvm.assume(i1 %0) // CHECK: %1 = insertvalue { ptr, i64 } poison, ptr %_0.0, 0 diff --git a/tests/codegen-llvm/transmute-scalar.rs b/tests/codegen-llvm/transmute-scalar.rs index 21f7287047cf0..e1ce8e506066a 100644 --- a/tests/codegen-llvm/transmute-scalar.rs +++ b/tests/codegen-llvm/transmute-scalar.rs @@ -56,7 +56,7 @@ pub fn ptr_to_int(p: *mut u16) -> usize { } // CHECK: define{{.*}}ptr @int_to_ptr([[USIZE]] %i) -// CHECK: %_0 = getelementptr i8, ptr null, [[USIZE]] %i +// CHECK: %_0 = inttoptr [[USIZE]] %i to ptr // CHECK-NEXT: ret ptr %_0 #[no_mangle] pub fn int_to_ptr(i: usize) -> *mut u16 { From 00c06488ada8ae1e88b0273b695863ad72805ed1 Mon Sep 17 00:00:00 2001 From: Boxy Uwu Date: Fri, 10 Oct 2025 19:56:57 +0100 Subject: [PATCH 2/4] in opaque type handling lift region vars to static if they outlive placeholders (cherry picked from commit 30bedc74d4812d8618fb31dbf002f44346979fd6) --- .../rustc_borrowck/src/handle_placeholders.rs | 2 +- .../opaque_types/member_constraints.rs | 3 +- .../src/region_infer/opaque_types/mod.rs | 7 ++++ .../region_infer/opaque_types/region_ctxt.rs | 36 +++++++++++++++---- .../placeholders_lift_to_static.rs | 33 +++++++++++++++++ tests/ui/impl-trait/nested-rpit-hrtb.rs | 1 - tests/ui/impl-trait/nested-rpit-hrtb.stderr | 14 +++----- tests/ui/nll/ice-106874.stderr | 26 +++++++------- 8 files changed, 90 insertions(+), 32 deletions(-) create mode 100644 tests/ui/impl-trait/member-constraints/placeholders_lift_to_static.rs diff --git a/compiler/rustc_borrowck/src/handle_placeholders.rs b/compiler/rustc_borrowck/src/handle_placeholders.rs index 94379cdebf730..90d5e5e4f23f9 100644 --- a/compiler/rustc_borrowck/src/handle_placeholders.rs +++ b/compiler/rustc_borrowck/src/handle_placeholders.rs @@ -348,7 +348,7 @@ pub(crate) fn compute_sccs_applying_placeholder_outlives_constraints<'tcx>( } } -fn rewrite_placeholder_outlives<'tcx>( +pub(crate) fn rewrite_placeholder_outlives<'tcx>( sccs: &Sccs, annotations: &SccAnnotations<'_, '_, RegionTracker>, fr_static: RegionVid, diff --git a/compiler/rustc_borrowck/src/region_infer/opaque_types/member_constraints.rs b/compiler/rustc_borrowck/src/region_infer/opaque_types/member_constraints.rs index 667fc440ac002..a2e2b61ae2d35 100644 --- a/compiler/rustc_borrowck/src/region_infer/opaque_types/member_constraints.rs +++ b/compiler/rustc_borrowck/src/region_infer/opaque_types/member_constraints.rs @@ -39,7 +39,7 @@ pub(super) fn apply_member_constraints<'tcx>( debug!(?member_constraints); for scc_a in rcx.constraint_sccs.all_sccs() { debug!(?scc_a); - // Start by adding the region values required by outlives constraints. This + // Start by adding the region values required by outlives constraints. This // matches how we compute the final region values in `fn compute_regions`. // // We need to do this here to get a lower bound when applying member constraints. @@ -64,6 +64,7 @@ fn apply_member_constraint<'tcx>( // If the member region lives in a higher universe, we currently choose // the most conservative option by leaving it unchanged. if !rcx.max_placeholder_universe_reached(member).is_root() { + debug!("member region reached non root universe, bailing"); return; } diff --git a/compiler/rustc_borrowck/src/region_infer/opaque_types/mod.rs b/compiler/rustc_borrowck/src/region_infer/opaque_types/mod.rs index 0af636aa734ca..0c35bec316694 100644 --- a/compiler/rustc_borrowck/src/region_infer/opaque_types/mod.rs +++ b/compiler/rustc_borrowck/src/region_infer/opaque_types/mod.rs @@ -254,6 +254,10 @@ fn collect_defining_uses<'tcx>( } } else { errors.push(DeferredOpaqueTypeError::InvalidOpaqueTypeArgs(err)); + debug!( + "collect_defining_uses: InvalidOpaqueTypeArgs for {:?} := {:?}", + non_nll_opaque_type_key, hidden_type + ); } continue; } @@ -277,6 +281,7 @@ fn collect_defining_uses<'tcx>( defining_uses } +#[instrument(level = "debug", skip(rcx, concrete_opaque_types, defining_uses, errors))] fn compute_concrete_types_from_defining_uses<'tcx>( rcx: &RegionCtxt<'_, 'tcx>, concrete_opaque_types: &mut ConcreteOpaqueTypes<'tcx>, @@ -288,6 +293,7 @@ fn compute_concrete_types_from_defining_uses<'tcx>( let mut decls_modulo_regions: FxIndexMap, (OpaqueTypeKey<'tcx>, Span)> = FxIndexMap::default(); for &DefiningUse { opaque_type_key, ref arg_regions, hidden_type } in defining_uses { + debug!(?opaque_type_key, ?arg_regions, ?hidden_type); // After applying member constraints, we now map all regions in the hidden type // to the `arg_regions` of this defining use. In case a region in the hidden type // ended up not being equal to any such region, we error. @@ -295,6 +301,7 @@ fn compute_concrete_types_from_defining_uses<'tcx>( match hidden_type.try_fold_with(&mut ToArgRegionsFolder::new(rcx, arg_regions)) { Ok(hidden_type) => hidden_type, Err(r) => { + debug!("UnexpectedHiddenRegion: {:?}", r); errors.push(DeferredOpaqueTypeError::UnexpectedHiddenRegion { hidden_type, opaque_type_key, diff --git a/compiler/rustc_borrowck/src/region_infer/opaque_types/region_ctxt.rs b/compiler/rustc_borrowck/src/region_infer/opaque_types/region_ctxt.rs index 88326e4eebfc2..90b15cbdd2cc8 100644 --- a/compiler/rustc_borrowck/src/region_infer/opaque_types/region_ctxt.rs +++ b/compiler/rustc_borrowck/src/region_infer/opaque_types/region_ctxt.rs @@ -11,7 +11,9 @@ use crate::constraints::ConstraintSccIndex; use crate::handle_placeholders::{SccAnnotations, region_definitions}; use crate::region_infer::reverse_sccs::ReverseSccGraph; use crate::region_infer::values::RegionValues; -use crate::region_infer::{ConstraintSccs, RegionDefinition, RegionTracker, Representative}; +use crate::region_infer::{ + ConstraintSccs, OutlivesConstraintSet, RegionDefinition, RegionTracker, Representative, +}; use crate::type_check::MirTypeckRegionConstraints; use crate::type_check::free_region_relations::UniversalRegionRelations; use crate::universal_regions::UniversalRegions; @@ -39,16 +41,36 @@ impl<'a, 'tcx> RegionCtxt<'a, 'tcx> { location_map: Rc, constraints: &MirTypeckRegionConstraints<'tcx>, ) -> RegionCtxt<'a, 'tcx> { + let mut outlives_constraints = constraints.outlives_constraints.clone(); let universal_regions = &universal_region_relations.universal_regions; let (definitions, _has_placeholders) = region_definitions(infcx, universal_regions); + + let compute_sccs = + |outlives_constraints: &OutlivesConstraintSet<'tcx>, + annotations: &mut SccAnnotations<'_, 'tcx, RegionTracker>| { + ConstraintSccs::new_with_annotation( + &outlives_constraints + .graph(definitions.len()) + .region_graph(outlives_constraints, universal_regions.fr_static), + annotations, + ) + }; + let mut scc_annotations = SccAnnotations::init(&definitions); - let constraint_sccs = ConstraintSccs::new_with_annotation( - &constraints - .outlives_constraints - .graph(definitions.len()) - .region_graph(&constraints.outlives_constraints, universal_regions.fr_static), - &mut scc_annotations, + let mut constraint_sccs = compute_sccs(&outlives_constraints, &mut scc_annotations); + + let added_constraints = crate::handle_placeholders::rewrite_placeholder_outlives( + &constraint_sccs, + &scc_annotations, + universal_regions.fr_static, + &mut outlives_constraints, ); + + if added_constraints { + scc_annotations = SccAnnotations::init(&definitions); + constraint_sccs = compute_sccs(&outlives_constraints, &mut scc_annotations); + } + let scc_annotations = scc_annotations.scc_to_annotation; // Unlike the `RegionInferenceContext`, we only care about free regions diff --git a/tests/ui/impl-trait/member-constraints/placeholders_lift_to_static.rs b/tests/ui/impl-trait/member-constraints/placeholders_lift_to_static.rs new file mode 100644 index 0000000000000..ff52b70c2528d --- /dev/null +++ b/tests/ui/impl-trait/member-constraints/placeholders_lift_to_static.rs @@ -0,0 +1,33 @@ +//@ check-pass + +// We have some `RPIT` with an item bound of `for<'a> Outlives<'a>`. We +// infer a hidden type of `&'?x i32` where `'?x` is required to outlive +// some placeholder `'!a` due to the `for<'a> Outlives<'a>` item bound. +// +// We previously did not write constraints of the form `'?x: '!a` into +// `'?x: 'static`. This caused member constraints to bail and not consider +// `'?x` to be constrained to an arg region. + +pub trait Outlives<'a> {} +impl<'a, T: 'a> Outlives<'a> for T {} + +pub fn foo() -> impl for<'a> Outlives<'a> { + let x: &'static i32 = &1; + x +} + +// This *didn't* regress but feels like it's "the same thing" so +// test it anyway +pub fn bar() -> impl Sized { + let x: &'static i32 = &1; + hr_outlives(x) +} + +fn hr_outlives(v: T) -> T +where + for<'a> T: 'a +{ + v +} + +fn main() {} diff --git a/tests/ui/impl-trait/nested-rpit-hrtb.rs b/tests/ui/impl-trait/nested-rpit-hrtb.rs index f4ff13d6c20f9..11d79bcff7371 100644 --- a/tests/ui/impl-trait/nested-rpit-hrtb.rs +++ b/tests/ui/impl-trait/nested-rpit-hrtb.rs @@ -48,7 +48,6 @@ fn one_hrtb_mention_fn_trait_param_uses<'b>() -> impl for<'a> Bar<'a, Assoc = im // This should resolve. fn one_hrtb_mention_fn_outlives_uses<'b>() -> impl for<'a> Bar<'a, Assoc = impl Sized + 'b> {} //~^ ERROR implementation of `Bar` is not general enough -//~| ERROR lifetime may not live long enough // This should resolve. fn two_htrb_trait_param() -> impl for<'a> Foo<'a, Assoc = impl for<'b> Qux<'b>> {} diff --git a/tests/ui/impl-trait/nested-rpit-hrtb.stderr b/tests/ui/impl-trait/nested-rpit-hrtb.stderr index 93cd7bd788f45..2e95ef370c7fd 100644 --- a/tests/ui/impl-trait/nested-rpit-hrtb.stderr +++ b/tests/ui/impl-trait/nested-rpit-hrtb.stderr @@ -1,5 +1,5 @@ error[E0261]: use of undeclared lifetime name `'b` - --> $DIR/nested-rpit-hrtb.rs:57:77 + --> $DIR/nested-rpit-hrtb.rs:56:77 | LL | fn two_htrb_outlives() -> impl for<'a> Foo<'a, Assoc = impl for<'b> Sized + 'b> {} | ^^ undeclared lifetime @@ -15,7 +15,7 @@ LL | fn two_htrb_outlives<'b>() -> impl for<'a> Foo<'a, Assoc = impl for<'b> Siz | ++++ error[E0261]: use of undeclared lifetime name `'b` - --> $DIR/nested-rpit-hrtb.rs:65:82 + --> $DIR/nested-rpit-hrtb.rs:64:82 | LL | fn two_htrb_outlives_uses() -> impl for<'a> Bar<'a, Assoc = impl for<'b> Sized + 'b> {} | ^^ undeclared lifetime @@ -87,12 +87,6 @@ LL | fn one_hrtb_mention_fn_trait_param_uses<'b>() -> impl for<'a> Bar<'a, Assoc but trait `Qux<'_>` is implemented for `()` = help: for that trait implementation, expected `()`, found `&'a ()` -error: lifetime may not live long enough - --> $DIR/nested-rpit-hrtb.rs:49:93 - | -LL | fn one_hrtb_mention_fn_outlives_uses<'b>() -> impl for<'a> Bar<'a, Assoc = impl Sized + 'b> {} - | -- lifetime `'b` defined here ^^ opaque type requires that `'b` must outlive `'static` - error: implementation of `Bar` is not general enough --> $DIR/nested-rpit-hrtb.rs:49:93 | @@ -103,7 +97,7 @@ LL | fn one_hrtb_mention_fn_outlives_uses<'b>() -> impl for<'a> Bar<'a, Assoc = = note: ...but it actually implements `Bar<'0>`, for some specific lifetime `'0` error[E0277]: the trait bound `for<'a, 'b> &'a (): Qux<'b>` is not satisfied - --> $DIR/nested-rpit-hrtb.rs:61:64 + --> $DIR/nested-rpit-hrtb.rs:60:64 | LL | fn two_htrb_trait_param_uses() -> impl for<'a> Bar<'a, Assoc = impl for<'b> Qux<'b>> {} | ^^^^^^^^^^^^^^^^^^^^ the trait `for<'a, 'b> Qux<'b>` is not implemented for `&'a ()` @@ -112,7 +106,7 @@ LL | fn two_htrb_trait_param_uses() -> impl for<'a> Bar<'a, Assoc = impl for<'b> but trait `Qux<'_>` is implemented for `()` = help: for that trait implementation, expected `()`, found `&'a ()` -error: aborting due to 10 previous errors +error: aborting due to 9 previous errors Some errors have detailed explanations: E0261, E0277, E0657. For more information about an error, try `rustc --explain E0261`. diff --git a/tests/ui/nll/ice-106874.stderr b/tests/ui/nll/ice-106874.stderr index 0edbd7b44ef1c..629570b602ed6 100644 --- a/tests/ui/nll/ice-106874.stderr +++ b/tests/ui/nll/ice-106874.stderr @@ -17,6 +17,20 @@ LL | A(B(C::new(D::new(move |st| f(st))))) = note: ...but it actually implements `FnOnce<(&'1 mut V,)>`, for some specific lifetime `'1` = note: duplicate diagnostic emitted due to `-Z deduplicate-diagnostics=no` +error: higher-ranked subtype error + --> $DIR/ice-106874.rs:8:5 + | +LL | A(B(C::new(D::new(move |st| f(st))))) + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + +error: higher-ranked subtype error + --> $DIR/ice-106874.rs:8:5 + | +LL | A(B(C::new(D::new(move |st| f(st))))) + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + | + = note: duplicate diagnostic emitted due to `-Z deduplicate-diagnostics=no` + error: implementation of `FnOnce` is not general enough --> $DIR/ice-106874.rs:8:7 | @@ -75,17 +89,5 @@ LL | A(B(C::new(D::new(move |st| f(st))))) = note: ...but it actually implements `Fn<(&'2 mut V,)>`, for some specific lifetime `'2` = note: duplicate diagnostic emitted due to `-Z deduplicate-diagnostics=no` -error: higher-ranked subtype error - --> $DIR/ice-106874.rs:8:7 - | -LL | A(B(C::new(D::new(move |st| f(st))))) - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ - -error: higher-ranked subtype error - --> $DIR/ice-106874.rs:8:41 - | -LL | A(B(C::new(D::new(move |st| f(st))))) - | ^ - error: aborting due to 10 previous errors From 5b36a0a7b2cd49711464c24fa5d937d956869e95 Mon Sep 17 00:00:00 2001 From: dianqk Date: Sun, 12 Oct 2025 19:32:57 +0800 Subject: [PATCH 3/4] Add miscompiled test cases (cherry picked from commit 64c023bad80b8de5f2f711c30424cfbab4dda171) --- .../mir-opt/gvn_loop.loop_deref_mut.GVN.diff | 115 ++++++++++++++++++ tests/mir-opt/gvn_loop.rs | 31 +++++ tests/ui/mir/gvn-loop-miscompile.rs | 34 ++++++ 3 files changed, 180 insertions(+) create mode 100644 tests/mir-opt/gvn_loop.loop_deref_mut.GVN.diff create mode 100644 tests/mir-opt/gvn_loop.rs create mode 100644 tests/ui/mir/gvn-loop-miscompile.rs diff --git a/tests/mir-opt/gvn_loop.loop_deref_mut.GVN.diff b/tests/mir-opt/gvn_loop.loop_deref_mut.GVN.diff new file mode 100644 index 0000000000000..1a53667624aac --- /dev/null +++ b/tests/mir-opt/gvn_loop.loop_deref_mut.GVN.diff @@ -0,0 +1,115 @@ +- // MIR for `loop_deref_mut` before GVN ++ // MIR for `loop_deref_mut` after GVN + + fn loop_deref_mut(_1: &mut Value) -> Value { + debug val => _1; + let mut _0: Value; + let _2: &Value; + let _3: &Value; + let mut _4: &Value; + let mut _6: !; + let mut _8: isize; + let mut _9: !; + let mut _10: (); + let mut _12: i32; + let _13: (); + let mut _14: bool; + let mut _15: !; + let mut _16: Value; + scope 1 { + debug val_alias => _2; + let mut _5: bool; + scope 2 { + debug stop => _5; + let _7: i32; + scope 3 { + debug v => _7; + let _11: Value; + scope 4 { + debug v => _11; + } + } + } + } + + bb0: { + StorageLive(_2); +- StorageLive(_3); ++ nop; + StorageLive(_4); + _4 = &(*_1); + _3 = get::(move _4) -> [return: bb1, unwind unreachable]; + } + + bb1: { + _2 = &(*_3); + StorageDead(_4); +- StorageDead(_3); ++ nop; + StorageLive(_5); + _5 = const false; +- _8 = discriminant((*_2)); ++ _8 = discriminant((*_3)); + switchInt(move _8) -> [0: bb3, otherwise: bb2]; + } + + bb2: { + unreachable; + } + + bb3: { +- StorageLive(_7); +- _7 = copy (((*_2) as V0).0: i32); ++ nop; ++ _7 = copy (((*_3) as V0).0: i32); + StorageLive(_9); + goto -> bb4; + } + + bb4: { + StorageLive(_11); + StorageLive(_12); + _12 = copy _7; +- _11 = Value::V0(move _12); ++ _11 = copy (*_3); + StorageDead(_12); + StorageLive(_13); + StorageLive(_14); + _14 = copy _5; + switchInt(move _14) -> [0: bb6, otherwise: bb5]; + } + + bb5: { + _0 = move _11; + StorageDead(_14); + StorageDead(_13); + StorageDead(_11); + StorageDead(_9); +- StorageDead(_7); ++ nop; + StorageDead(_5); + StorageDead(_2); + return; + } + + bb6: { + _13 = const (); + StorageDead(_14); + StorageDead(_13); + _5 = const true; + StorageLive(_16); +- _16 = Value::V1; +- (*_1) = move _16; ++ _16 = const Value::V1; ++ (*_1) = const Value::V1; + StorageDead(_16); + _10 = const (); + StorageDead(_11); + goto -> bb4; + } ++ } ++ ++ ALLOC0 (size: 8, align: 4) { ++ 01 00 00 00 __ __ __ __ │ ....░░░░ + } + diff --git a/tests/mir-opt/gvn_loop.rs b/tests/mir-opt/gvn_loop.rs new file mode 100644 index 0000000000000..a993f9b53f839 --- /dev/null +++ b/tests/mir-opt/gvn_loop.rs @@ -0,0 +1,31 @@ +// skip-filecheck +//@ test-mir-pass: GVN + +#![crate_type = "lib"] +#![feature(core_intrinsics, rustc_attrs)] + +pub enum Value { + V0(i32), + V1, +} + +// EMIT_MIR gvn_loop.loop_deref_mut.GVN.diff +fn loop_deref_mut(val: &mut Value) -> Value { + let val_alias: &Value = get(val); + let mut stop = false; + let Value::V0(v) = *val_alias else { unsafe { core::intrinsics::unreachable() } }; + loop { + let v = Value::V0(v); + if stop { + return v; + } + stop = true; + *val = Value::V1; + } +} + +#[inline(never)] +#[rustc_nounwind] +fn get(v: &T) -> &T { + v +} diff --git a/tests/ui/mir/gvn-loop-miscompile.rs b/tests/ui/mir/gvn-loop-miscompile.rs new file mode 100644 index 0000000000000..f24858918b16c --- /dev/null +++ b/tests/ui/mir/gvn-loop-miscompile.rs @@ -0,0 +1,34 @@ +//@ compile-flags: -O +//@ run-fail + +pub enum Value { + V0(i32), + V1, +} + +fn set_discriminant(val: &mut Value) -> Value { + let val_alias: &Value = get(val); + let mut stop = false; + let Value::V0(v) = *val_alias else { + unreachable!(); + }; + loop { + let v = Value::V0(v); + if stop { + return v; + } + stop = true; + *val = Value::V1; + } +} + +fn main() { + let mut v = Value::V0(1); + let v = set_discriminant(&mut v); + assert!(matches!(v, Value::V0(1))); +} + +#[inline(never)] +fn get(v: &T) -> &T { + v +} From 408e09e77c9feaa6872671b3505416b357ddff02 Mon Sep 17 00:00:00 2001 From: dianqk Date: Sun, 12 Oct 2025 19:53:12 +0800 Subject: [PATCH 4/4] GVN: Invalidate derefs at loop headers (cherry picked from commit 2048b9c027c8c55d1a83be5d769bb65a855397d3) --- compiler/rustc_middle/src/mir/loops.rs | 29 +++++++++++++++++++ compiler/rustc_middle/src/mir/mod.rs | 1 + compiler/rustc_mir_transform/src/gvn.rs | 6 ++++ .../rustc_mir_transform/src/jump_threading.rs | 20 +------------ .../mir-opt/gvn_loop.loop_deref_mut.GVN.diff | 2 +- tests/mir-opt/gvn_loop.rs | 10 ++++++- tests/ui/mir/gvn-loop-miscompile.rs | 2 +- 7 files changed, 48 insertions(+), 22 deletions(-) create mode 100644 compiler/rustc_middle/src/mir/loops.rs diff --git a/compiler/rustc_middle/src/mir/loops.rs b/compiler/rustc_middle/src/mir/loops.rs new file mode 100644 index 0000000000000..ae332913c6421 --- /dev/null +++ b/compiler/rustc_middle/src/mir/loops.rs @@ -0,0 +1,29 @@ +use rustc_index::bit_set::DenseBitSet; + +use super::*; + +/// Compute the set of loop headers in the given body. A loop header is usually defined as a block +/// which dominates one of its predecessors. This definition is only correct for reducible CFGs. +/// However, computing dominators is expensive, so we approximate according to the post-order +/// traversal order. A loop header for us is a block which is visited after its predecessor in +/// post-order. This is ok as we mostly need a heuristic. +pub fn maybe_loop_headers(body: &Body<'_>) -> DenseBitSet { + let mut maybe_loop_headers = DenseBitSet::new_empty(body.basic_blocks.len()); + let mut visited = DenseBitSet::new_empty(body.basic_blocks.len()); + for (bb, bbdata) in traversal::postorder(body) { + // Post-order means we visit successors before the block for acyclic CFGs. + // If the successor is not visited yet, consider it a loop header. + for succ in bbdata.terminator().successors() { + if !visited.contains(succ) { + maybe_loop_headers.insert(succ); + } + } + + // Only mark `bb` as visited after we checked the successors, in case we have a self-loop. + // bb1: goto -> bb1; + let _new = visited.insert(bb); + debug_assert!(_new); + } + + maybe_loop_headers +} diff --git a/compiler/rustc_middle/src/mir/mod.rs b/compiler/rustc_middle/src/mir/mod.rs index da2245b12d2c2..cfc2576756d22 100644 --- a/compiler/rustc_middle/src/mir/mod.rs +++ b/compiler/rustc_middle/src/mir/mod.rs @@ -51,6 +51,7 @@ mod statement; mod syntax; mod terminator; +pub mod loops; pub mod traversal; pub mod visit; diff --git a/compiler/rustc_mir_transform/src/gvn.rs b/compiler/rustc_mir_transform/src/gvn.rs index bf6aa800d20f4..7795fac1941a4 100644 --- a/compiler/rustc_mir_transform/src/gvn.rs +++ b/compiler/rustc_mir_transform/src/gvn.rs @@ -126,6 +126,7 @@ impl<'tcx> crate::MirPass<'tcx> for GVN { let ssa = SsaLocals::new(tcx, body, typing_env); // Clone dominators because we need them while mutating the body. let dominators = body.basic_blocks.dominators().clone(); + let maybe_loop_headers = loops::maybe_loop_headers(body); let mut state = VnState::new(tcx, body, typing_env, &ssa, dominators, &body.local_decls); @@ -136,6 +137,11 @@ impl<'tcx> crate::MirPass<'tcx> for GVN { let reverse_postorder = body.basic_blocks.reverse_postorder().to_vec(); for bb in reverse_postorder { + // N.B. With loops, reverse postorder cannot produce a valid topological order. + // A statement or terminator from inside the loop, that is not processed yet, may have performed an indirect write. + if maybe_loop_headers.contains(bb) { + state.invalidate_derefs(); + } let data = &mut body.basic_blocks.as_mut_preserves_cfg()[bb]; state.visit_basic_block_data(bb, data); } diff --git a/compiler/rustc_mir_transform/src/jump_threading.rs b/compiler/rustc_mir_transform/src/jump_threading.rs index f9e642e28ebd7..fb2f82fcaf707 100644 --- a/compiler/rustc_mir_transform/src/jump_threading.rs +++ b/compiler/rustc_mir_transform/src/jump_threading.rs @@ -84,7 +84,7 @@ impl<'tcx> crate::MirPass<'tcx> for JumpThreading { body, arena, map: Map::new(tcx, body, Some(MAX_PLACES)), - loop_headers: loop_headers(body), + loop_headers: loops::maybe_loop_headers(body), opportunities: Vec::new(), }; @@ -832,21 +832,3 @@ enum Update { Incr, Decr, } - -/// Compute the set of loop headers in the given body. We define a loop header as a block which has -/// at least a predecessor which it dominates. This definition is only correct for reducible CFGs. -/// But if the CFG is already irreducible, there is no point in trying much harder. -/// is already irreducible. -fn loop_headers(body: &Body<'_>) -> DenseBitSet { - let mut loop_headers = DenseBitSet::new_empty(body.basic_blocks.len()); - let dominators = body.basic_blocks.dominators(); - // Only visit reachable blocks. - for (bb, bbdata) in traversal::preorder(body) { - for succ in bbdata.terminator().successors() { - if dominators.dominates(succ, bb) { - loop_headers.insert(succ); - } - } - } - loop_headers -} diff --git a/tests/mir-opt/gvn_loop.loop_deref_mut.GVN.diff b/tests/mir-opt/gvn_loop.loop_deref_mut.GVN.diff index 1a53667624aac..92e5ccabedf9e 100644 --- a/tests/mir-opt/gvn_loop.loop_deref_mut.GVN.diff +++ b/tests/mir-opt/gvn_loop.loop_deref_mut.GVN.diff @@ -71,7 +71,7 @@ StorageLive(_12); _12 = copy _7; - _11 = Value::V0(move _12); -+ _11 = copy (*_3); ++ _11 = Value::V0(copy _7); StorageDead(_12); StorageLive(_13); StorageLive(_14); diff --git a/tests/mir-opt/gvn_loop.rs b/tests/mir-opt/gvn_loop.rs index a993f9b53f839..6e9df55a968dc 100644 --- a/tests/mir-opt/gvn_loop.rs +++ b/tests/mir-opt/gvn_loop.rs @@ -1,4 +1,3 @@ -// skip-filecheck //@ test-mir-pass: GVN #![crate_type = "lib"] @@ -9,8 +8,17 @@ pub enum Value { V1, } +// Check that we do not use the dereferenced value of `val_alias` when returning. + // EMIT_MIR gvn_loop.loop_deref_mut.GVN.diff fn loop_deref_mut(val: &mut Value) -> Value { + // CHECK-LABEL: fn loop_deref_mut( + // CHECK: [[VAL_REF:_.*]] = get::( + // CHECK: [[V:_.*]] = copy (((*[[VAL_REF]]) as V0).0: i32); + // CEHCK-NOT: copy (*[[VAL_REF]]); + // CHECK: [[RET:_*]] = Value::V0(copy [[V]]); + // CEHCK-NOT: copy (*[[VAL_REF]]); + // CHECK: _0 = move [[RET]] let val_alias: &Value = get(val); let mut stop = false; let Value::V0(v) = *val_alias else { unsafe { core::intrinsics::unreachable() } }; diff --git a/tests/ui/mir/gvn-loop-miscompile.rs b/tests/ui/mir/gvn-loop-miscompile.rs index f24858918b16c..8e987c5c94662 100644 --- a/tests/ui/mir/gvn-loop-miscompile.rs +++ b/tests/ui/mir/gvn-loop-miscompile.rs @@ -1,5 +1,5 @@ //@ compile-flags: -O -//@ run-fail +//@ run-pass pub enum Value { V0(i32),