From 5ceeeaa707d552672cc8f18ab9e8ece246e575a8 Mon Sep 17 00:00:00 2001 From: Stuart Pernsteiner Date: Mon, 23 Sep 2024 16:50:16 -0700 Subject: [PATCH 1/5] analyze: mir_op: respect FIXED when handling Deref projections --- c2rust-analyze/src/rewrite/expr/mir_op.rs | 33 ++++++++++++----------- 1 file changed, 17 insertions(+), 16 deletions(-) diff --git a/c2rust-analyze/src/rewrite/expr/mir_op.rs b/c2rust-analyze/src/rewrite/expr/mir_op.rs index d4a1a915b..726b28a3c 100644 --- a/c2rust-analyze/src/rewrite/expr/mir_op.rs +++ b/c2rust-analyze/src/rewrite/expr/mir_op.rs @@ -1091,30 +1091,31 @@ impl<'a, 'tcx> ExprRewriteVisitor<'a, 'tcx> { PlaceElem::Deref => { self.enter_place_deref_pointer(|v| { v.visit_place_ref(base_pl, proj_ltys, access); - let dyn_owned = v.is_dyn_owned(base_lty); - if v.is_nullable(base_lty.label) { - // If the pointer type is non-copy, downgrade (borrow) before calling - // `unwrap()`. + if !v.flags[base_lty.label].contains(FlagSet::FIXED) { let desc = type_desc::perms_to_desc( base_lty.ty, v.perms[base_lty.label], v.flags[base_lty.label], ); - if !desc.own.is_copy() { - v.emit(RewriteKind::OptionDowngrade { + if v.is_nullable(base_lty.label) { + // If the pointer type is non-copy, downgrade (borrow) before calling + // `unwrap()`. + if !desc.own.is_copy() { + v.emit(RewriteKind::OptionDowngrade { + mutbl: access == PlaceAccess::Mut, + deref: !desc.dyn_owned, + }); + } + v.emit(RewriteKind::OptionUnwrap); + if desc.dyn_owned { + v.emit(RewriteKind::Deref); + } + } + if desc.dyn_owned { + v.emit(RewriteKind::DynOwnedDowngrade { mutbl: access == PlaceAccess::Mut, - deref: !dyn_owned, }); } - v.emit(RewriteKind::OptionUnwrap); - if dyn_owned { - v.emit(RewriteKind::Deref); - } - } - if dyn_owned { - v.emit(RewriteKind::DynOwnedDowngrade { - mutbl: access == PlaceAccess::Mut, - }); } }); } From f2780c596b1bd56f6db8c5fcdaaa8137d59a63d2 Mon Sep 17 00:00:00 2001 From: Stuart Pernsteiner Date: Mon, 5 Aug 2024 14:36:47 -0700 Subject: [PATCH 2/5] analyze: don't invalidate loans on StorageDead --- c2rust-analyze/src/borrowck/def_use.rs | 23 ++++++++++++-- c2rust-analyze/tests/filecheck.rs | 1 + c2rust-analyze/tests/filecheck/alloc.rs | 18 +++++++++++ c2rust-analyze/tests/filecheck/move_mut.rs | 30 +++++++++++++++++++ .../tests/filecheck/non_null_force.rs | 13 ++++---- 5 files changed, 75 insertions(+), 10 deletions(-) create mode 100644 c2rust-analyze/tests/filecheck/move_mut.rs diff --git a/c2rust-analyze/src/borrowck/def_use.rs b/c2rust-analyze/src/borrowck/def_use.rs index 395fd33f8..63b8761cb 100644 --- a/c2rust-analyze/src/borrowck/def_use.rs +++ b/c2rust-analyze/src/borrowck/def_use.rs @@ -34,12 +34,31 @@ pub fn categorize(context: PlaceContext) -> Option { // path and not the unwind path. -nmatsakis PlaceContext::MutatingUse(MutatingUseContext::Call) | PlaceContext::MutatingUse(MutatingUseContext::AsmOutput) | - PlaceContext::MutatingUse(MutatingUseContext::Yield) | + PlaceContext::MutatingUse(MutatingUseContext::Yield) => Some(DefUse::Def), // Storage live and storage dead aren't proper defines, but we can ignore // values that come before them. + // + // C2Rust: For borrowchecking purposes, we ignore `StorageLive` and `StorageDead`. In the + // original version of this function, they're considered to be `DefUse::Def`s, but this + // approach creates a problem for code like this: + // + // ``` + // let q = { + // let p = &mut ...; + // p + // }; + // *q = 1; + // ``` + // + // The MIR for this has an assignment like `q = p`, followed by `StorageDead(p)`. We + // interpret the assignment as a reborrow of `p`, and if `StorageDead(p)` was considered a + // `Def`, we would invalidate the loan at the end of the block when `StorageDead` "writes" + // to `p`. However, this code is perfectly valid, and omitting `loan_invalidated_at` for + // `StorageLive` and `StorageDead` appears to be consistent with `rustc -Z nll-facts` + // output (tested on `tests/filecheck/move_mut.rs`). PlaceContext::NonUse(NonUseContext::StorageLive) | - PlaceContext::NonUse(NonUseContext::StorageDead) => Some(DefUse::Def), + PlaceContext::NonUse(NonUseContext::StorageDead) => None, /////////////////////////////////////////////////////////////////////////// // REGULAR USES diff --git a/c2rust-analyze/tests/filecheck.rs b/c2rust-analyze/tests/filecheck.rs index bba56350b..10ba60c2a 100644 --- a/c2rust-analyze/tests/filecheck.rs +++ b/c2rust-analyze/tests/filecheck.rs @@ -55,6 +55,7 @@ define_tests! { insertion_sort_driver, insertion_sort_rewrites, known_fn, + move_mut, non_null, non_null_force, non_null_rewrites, diff --git a/c2rust-analyze/tests/filecheck/alloc.rs b/c2rust-analyze/tests/filecheck/alloc.rs index fa03fde55..282d7ce8d 100644 --- a/c2rust-analyze/tests/filecheck/alloc.rs +++ b/c2rust-analyze/tests/filecheck/alloc.rs @@ -96,6 +96,24 @@ unsafe extern "C" fn realloc1(n: libc::c_ulong) { free(buf as *mut libc::c_void); } + +// CHECK-LABEL: final labeling for "malloc_return" +pub unsafe extern "C" fn malloc_return(mut cnt: libc::c_int) -> *mut i32 { + // CHECK-DAG: ([[@LINE+1]]: mut i): addr_of = UNIQUE | NON_NULL | STACK, type = READ | WRITE | UNIQUE | FREE | NON_NULL | HEAP# + let mut i: *mut i32 = malloc(::std::mem::size_of::() as libc::c_ulong) as *mut i32; + i +} + +// CHECK-LABEL: final labeling for "malloc_return_free1" +pub unsafe extern "C" fn malloc_return_free1(mut cnt: libc::c_int) { + // CHECK-DAG: ([[@LINE+1]]: mut i): addr_of = UNIQUE | NON_NULL | STACK, type = READ | WRITE | UNIQUE | FREE | NON_NULL | HEAP# + let mut i: *mut i32 = malloc_return(cnt); + *i = 2; + // CHECK-DAG: ([[@LINE+1]]: i{{.*}}): {{.*}}type = UNIQUE | FREE | NON_NULL | HEAP# + free(i as *mut libc::c_void); +} + + // Rewrites of malloc/calloc/realloc/memset should use `mem::size_of` to convert byte counts to // element counts. // CHECK: let n = byte_len as usize / std::mem::size_of::(); diff --git a/c2rust-analyze/tests/filecheck/move_mut.rs b/c2rust-analyze/tests/filecheck/move_mut.rs new file mode 100644 index 000000000..5ec0eb32c --- /dev/null +++ b/c2rust-analyze/tests/filecheck/move_mut.rs @@ -0,0 +1,30 @@ +use std::ptr; + +// CHECK-LABEL: final labeling for "move_mut1" +pub unsafe fn move_mut1(mut x: i32) -> i32 { + // CHECK-DAG: ([[@LINE+1]]: q): addr_of = UNIQUE | NON_NULL | STACK, type = READ | WRITE | UNIQUE | NON_NULL | STACK# + let q: *mut i32 = { + // CHECK-DAG: ([[@LINE+1]]: p): addr_of = UNIQUE | NON_NULL | STACK, type = READ | WRITE | UNIQUE | NON_NULL | STACK# + let p: *mut i32 = ptr::addr_of_mut!(x); + *p = 1; + // This produces a move from `p` into `q`. c2rust-analyze interprets this as a reborrow, + // which gets invalidated at the end of `p`'s lifetime. Currently we work around this by + // ignoring `StorageLive` and `StorageDead`, so they don't invalidate any borrows. + p + }; + *q = 2; + *q +} + +// Safe version of `move_mut1`. This can be run through `rustc -Z nll-facts` to produce Polonius +// facts from the reference implementation for comparison. +// CHECK-LABEL: final labeling for "move_mut1_safe" +pub fn move_mut1_safe(mut x: i32) -> i32 { + let q: &mut i32 = { + let p: &mut i32 = &mut x; + *p = 1; + p + }; + *q = 2; + *q +} diff --git a/c2rust-analyze/tests/filecheck/non_null_force.rs b/c2rust-analyze/tests/filecheck/non_null_force.rs index 0d25f9f95..171d5283b 100644 --- a/c2rust-analyze/tests/filecheck/non_null_force.rs +++ b/c2rust-analyze/tests/filecheck/non_null_force.rs @@ -1,40 +1,37 @@ #![feature(register_tool)] #![register_tool(c2rust_analyze_test)] -// TODO: All the pointers here are currently inferred to be non-`UNIQUE`, even though the access -// patterns look fine. - use std::ptr; // CHECK-LABEL: final labeling for "f" fn f(cond: bool) { let x = 1_i32; - // CHECK: ([[@LINE+1]]: mut y): {{.*}}, type = STACK# + // CHECK: ([[@LINE+1]]: mut y): {{.*}}, type = UNIQUE | STACK# let mut y = ptr::addr_of!(x); if cond { y = 0 as *const _; } // The expression `y` is considered nullable even though it's passed for argument `p` of `g`, // which is forced to be `NON_NULL`. - // CHECK: ([[@LINE+1]]: y): {{.*}}, type = STACK# + // CHECK: ([[@LINE+1]]: y): {{.*}}, type = UNIQUE | STACK# g(cond, y); } // CHECK-LABEL: final labeling for "g" // `p` should be non-null, as it's forced to be by the attribute. This emulates the "unsound" PDG // case, where a variable is forced to stay `NON_NULL` even though a null possibly flows into it. -// CHECK: ([[@LINE+2]]: p): {{.*}}, type = NON_NULL | STACK# +// CHECK: ([[@LINE+2]]: p): {{.*}}, type = UNIQUE | NON_NULL | STACK# #[c2rust_analyze_test::force_non_null_args] fn g(cond: bool, p: *const i32) { // `q` is not forced to be `NON_NULL`, so it should be inferred nullable due to the null // assignment below. - // CHECK: ([[@LINE+1]]: mut q): {{.*}}, type = STACK# + // CHECK: ([[@LINE+1]]: mut q): {{.*}}, type = UNIQUE | STACK# let mut q = p; if cond { q = 0 as *const _; } // `r` is derived from `q` (and is not forced), so it should also be nullable. - // CHECK: ([[@LINE+1]]: r): {{.*}}, type = STACK# + // CHECK: ([[@LINE+1]]: r): {{.*}}, type = UNIQUE | STACK# let r = q; } From d2a1f5ab738998c72d56e2b979b933c70f2bb686 Mon Sep 17 00:00:00 2001 From: Stuart Pernsteiner Date: Thu, 15 Aug 2024 16:21:55 -0700 Subject: [PATCH 3/5] analyze: rewrite::ty: add missing walk_stmt call in HirTyVisitor --- c2rust-analyze/src/rewrite/ty.rs | 1 + 1 file changed, 1 insertion(+) diff --git a/c2rust-analyze/src/rewrite/ty.rs b/c2rust-analyze/src/rewrite/ty.rs index 3f29e7ed4..b8ef2957e 100644 --- a/c2rust-analyze/src/rewrite/ty.rs +++ b/c2rust-analyze/src/rewrite/ty.rs @@ -681,6 +681,7 @@ impl<'tcx, 'a> intravisit::Visitor<'tcx> for HirTyVisitor<'a, 'tcx> { } _ => (), } + intravisit::walk_stmt(self, s); } } From 8657670b6f341823e65cde527290501ca151f04c Mon Sep 17 00:00:00 2001 From: Stuart Pernsteiner Date: Thu, 22 Aug 2024 11:26:07 -0700 Subject: [PATCH 4/5] analyze: mir_op: convert &[T] to &T for certain deref projections --- c2rust-analyze/src/rewrite/expr/mir_op.rs | 74 +++++++++++++++---- c2rust-analyze/tests/filecheck.rs | 1 + .../tests/filecheck/offset_rewrites.rs | 12 +++ 3 files changed, 73 insertions(+), 14 deletions(-) create mode 100644 c2rust-analyze/tests/filecheck/offset_rewrites.rs diff --git a/c2rust-analyze/src/rewrite/expr/mir_op.rs b/c2rust-analyze/src/rewrite/expr/mir_op.rs index 726b28a3c..d579f698b 100644 --- a/c2rust-analyze/src/rewrite/expr/mir_op.rs +++ b/c2rust-analyze/src/rewrite/expr/mir_op.rs @@ -220,6 +220,21 @@ impl PlaceAccess { } } +/// Named boolean for use with `visit_place`. `RequireSinglePointer::Yes` means that if the +/// `Place` ends with a `Deref` projection, the pointer being dereferenced must have +/// `Quantity::Single`. +#[derive(Clone, Copy, PartialEq, Eq, Debug, Hash)] +enum RequireSinglePointer { + No, + Yes, +} + +impl RequireSinglePointer { + pub fn as_bool(self) -> bool { + self == RequireSinglePointer::Yes + } +} + struct ExprRewriteVisitor<'a, 'tcx> { acx: &'a AnalysisCtxt<'a, 'tcx>, perms: &'a GlobalPointerTable, @@ -475,7 +490,7 @@ impl<'a, 'tcx> ExprRewriteVisitor<'a, 'tcx> { v.enter_rvalue_operand(0, |v| { v.enter_operand_place(|v| { eprintln!("BEGIN visit_place for ownership transfer"); - v.visit_place(rv_pl, PlaceAccess::Mut); + v.visit_place(rv_pl, PlaceAccess::Mut, RequireSinglePointer::No); eprintln!("END visit_place for ownership transfer"); }); }); @@ -501,7 +516,7 @@ impl<'a, 'tcx> ExprRewriteVisitor<'a, 'tcx> { v.visit_rvalue(rv, Some(rv_lty)); v.emit_cast_lty_lty(rv_lty, pl_lty) }); - self.enter_dest(|v| v.visit_place(pl, PlaceAccess::Mut)); + self.enter_dest(|v| v.visit_place(pl, PlaceAccess::Mut, RequireSinglePointer::Yes)); } StatementKind::FakeRead(..) => {} StatementKind::SetDiscriminant { .. } => todo!("statement {:?}", stmt), @@ -905,7 +920,9 @@ impl<'a, 'tcx> ExprRewriteVisitor<'a, 'tcx> { BorrowKind::Mut { .. } => true, BorrowKind::Shared | BorrowKind::Shallow | BorrowKind::Unique => false, }; - self.enter_rvalue_place(0, |v| v.visit_place(pl, PlaceAccess::from_bool(mutbl))); + self.enter_rvalue_place(0, |v| { + v.visit_place(pl, PlaceAccess::from_bool(mutbl), RequireSinglePointer::No) + }); if let Some(expect_ty) = expect_ty { if self.is_nullable(expect_ty.label) { @@ -919,7 +936,9 @@ impl<'a, 'tcx> ExprRewriteVisitor<'a, 'tcx> { // TODO } Rvalue::AddressOf(mutbl, pl) => { - self.enter_rvalue_place(0, |v| v.visit_place(pl, PlaceAccess::from_mutbl(mutbl))); + self.enter_rvalue_place(0, |v| { + v.visit_place(pl, PlaceAccess::from_mutbl(mutbl), RequireSinglePointer::No) + }); if let Some(expect_ty) = expect_ty { let desc = type_desc::perms_to_desc_with_pointee( self.acx.tcx(), @@ -941,7 +960,9 @@ impl<'a, 'tcx> ExprRewriteVisitor<'a, 'tcx> { } } Rvalue::Len(pl) => { - self.enter_rvalue_place(0, |v| v.visit_place(pl, PlaceAccess::Imm)); + self.enter_rvalue_place(0, |v| { + v.visit_place(pl, PlaceAccess::Imm, RequireSinglePointer::No) + }); } Rvalue::Cast(_kind, ref op, ty) => { if util::is_null_const_operand(op) && ty.is_unsafe_ptr() { @@ -1000,7 +1021,9 @@ impl<'a, 'tcx> ExprRewriteVisitor<'a, 'tcx> { self.enter_rvalue_operand(0, |v| v.visit_operand(op, None)); } Rvalue::Discriminant(pl) => { - self.enter_rvalue_place(0, |v| v.visit_place(pl, PlaceAccess::Imm)); + self.enter_rvalue_place(0, |v| { + v.visit_place(pl, PlaceAccess::Imm, RequireSinglePointer::No) + }); } Rvalue::Aggregate(ref _kind, ref ops) => { for (i, op) in ops.iter().enumerate() { @@ -1011,7 +1034,9 @@ impl<'a, 'tcx> ExprRewriteVisitor<'a, 'tcx> { self.enter_rvalue_operand(0, |v| v.visit_operand(op, None)); } Rvalue::CopyForDeref(pl) => { - self.enter_rvalue_place(0, |v| v.visit_place(pl, PlaceAccess::Imm)); + self.enter_rvalue_place(0, |v| { + v.visit_place(pl, PlaceAccess::Imm, RequireSinglePointer::No) + }); } } } @@ -1022,7 +1047,9 @@ impl<'a, 'tcx> ExprRewriteVisitor<'a, 'tcx> { match *op { Operand::Copy(pl) | Operand::Move(pl) => { // TODO: should this be Move, Imm, or dependent on the type? - self.enter_operand_place(|v| v.visit_place(pl, PlaceAccess::Move)); + self.enter_operand_place(|v| { + v.visit_place(pl, PlaceAccess::Move, RequireSinglePointer::Yes) + }); if let Some(expect_ty) = expect_ty { let ptr_lty = self.acx.type_of(pl); @@ -1040,7 +1067,7 @@ impl<'a, 'tcx> ExprRewriteVisitor<'a, 'tcx> { match *op { Operand::Copy(pl) | Operand::Move(pl) => { // TODO: should this be Move, Imm, or dependent on the type? - self.visit_place(pl, PlaceAccess::Move); + self.visit_place(pl, PlaceAccess::Move, RequireSinglePointer::Yes); let ptr_lty = self.acx.type_of(pl); if !ptr_lty.label.is_none() { @@ -1051,14 +1078,19 @@ impl<'a, 'tcx> ExprRewriteVisitor<'a, 'tcx> { } } - fn visit_place(&mut self, pl: Place<'tcx>, access: PlaceAccess) { + fn visit_place( + &mut self, + pl: Place<'tcx>, + access: PlaceAccess, + require_single_ptr: RequireSinglePointer, + ) { let mut ltys = Vec::with_capacity(1 + pl.projection.len()); ltys.push(self.acx.type_of(pl.local)); for proj in pl.projection { let prev_lty = ltys.last().copied().unwrap(); ltys.push(self.acx.projection_lty(prev_lty, &proj)); } - self.visit_place_ref(pl.as_ref(), <ys, access); + self.visit_place_ref(pl.as_ref(), <ys, access, require_single_ptr); } /// Generate rewrites for a `Place` represented as a `PlaceRef`. `proj_ltys` gives the `LTy` @@ -1069,6 +1101,7 @@ impl<'a, 'tcx> ExprRewriteVisitor<'a, 'tcx> { pl: PlaceRef<'tcx>, proj_ltys: &[LTy<'tcx>], access: PlaceAccess, + require_single_ptr: RequireSinglePointer, ) { let (&last_proj, rest) = match pl.projection.split_last() { Some(x) => x, @@ -1090,13 +1123,17 @@ impl<'a, 'tcx> ExprRewriteVisitor<'a, 'tcx> { match last_proj { PlaceElem::Deref => { self.enter_place_deref_pointer(|v| { - v.visit_place_ref(base_pl, proj_ltys, access); + v.visit_place_ref(base_pl, proj_ltys, access, RequireSinglePointer::Yes); if !v.flags[base_lty.label].contains(FlagSet::FIXED) { let desc = type_desc::perms_to_desc( base_lty.ty, v.perms[base_lty.label], v.flags[base_lty.label], ); + // TODO: This logic is quite similar to the cast builder code and could + // probably be replaced with a call to `emit_cast_lty_adjust`. But + // currently this tries to introduce casts on the borrow projection in + // `array.as_mut_ptr()`, which causes rewriting to fail. if v.is_nullable(base_lty.label) { // If the pointer type is non-copy, downgrade (borrow) before calling // `unwrap()`. @@ -1116,14 +1153,23 @@ impl<'a, 'tcx> ExprRewriteVisitor<'a, 'tcx> { mutbl: access == PlaceAccess::Mut, }); } + if require_single_ptr.as_bool() && desc.qty != Quantity::Single { + v.emit(RewriteKind::SliceFirst { + mutbl: access == PlaceAccess::Mut, + }); + } } }); } PlaceElem::Field(_idx, _ty) => { - self.enter_place_field_base(|v| v.visit_place_ref(base_pl, proj_ltys, access)); + self.enter_place_field_base(|v| { + v.visit_place_ref(base_pl, proj_ltys, access, RequireSinglePointer::Yes) + }); } PlaceElem::Index(_) | PlaceElem::ConstantIndex { .. } | PlaceElem::Subslice { .. } => { - self.enter_place_index_array(|v| v.visit_place_ref(base_pl, proj_ltys, access)); + self.enter_place_index_array(|v| { + v.visit_place_ref(base_pl, proj_ltys, access, RequireSinglePointer::Yes) + }); } PlaceElem::Downcast(_, _) => {} } diff --git a/c2rust-analyze/tests/filecheck.rs b/c2rust-analyze/tests/filecheck.rs index 10ba60c2a..12e0028cc 100644 --- a/c2rust-analyze/tests/filecheck.rs +++ b/c2rust-analyze/tests/filecheck.rs @@ -61,6 +61,7 @@ define_tests! { non_null_rewrites, offset1, offset2, + offset_rewrites, pointee, ptrptr1, regions_fixed, diff --git a/c2rust-analyze/tests/filecheck/offset_rewrites.rs b/c2rust-analyze/tests/filecheck/offset_rewrites.rs new file mode 100644 index 000000000..f7172fcea --- /dev/null +++ b/c2rust-analyze/tests/filecheck/offset_rewrites.rs @@ -0,0 +1,12 @@ + +// Test dereferencing a pointer that has OFFSET permissions. This requires inserting a cast from +// `&mut [T]` to `&mut T`. +// CHECK-LABEL: fn offset_deref +pub unsafe fn offset_deref(x: *mut i32, off: isize) -> *mut i32 { + // CHECK: *&mut (x)[0] = 0; + *x = 0; + // CHECK: *&mut (&mut (x)[((1) as usize) ..])[0] = 1; + *x.offset(1) = 1; + // CHECK: {{.*}}x{{.*}} + x +} From b8773cc618786f0f19d0f5077091cdbe734d5176 Mon Sep 17 00:00:00 2001 From: Stuart Pernsteiner Date: Fri, 30 Aug 2024 10:16:16 -0700 Subject: [PATCH 5/5] analyze: don't consider REWRITE_INVALID in fn_analysis_invalid --- c2rust-analyze/src/context.rs | 1 - 1 file changed, 1 deletion(-) diff --git a/c2rust-analyze/src/context.rs b/c2rust-analyze/src/context.rs index fc4d42a3a..14e906773 100644 --- a/c2rust-analyze/src/context.rs +++ b/c2rust-analyze/src/context.rs @@ -245,7 +245,6 @@ bitflags! { | Self::DATAFLOW_INVALID.bits | Self::BORROWCK_INVALID.bits | Self::MISC_ANALYSIS_INVALID.bits - | Self::REWRITE_INVALID.bits | Self::FAKE_INVALID_FOR_TESTING.bits; } }