diff --git a/c2rust-analyze/src/borrowck/mod.rs b/c2rust-analyze/src/borrowck/mod.rs index 2330a62d14..a1f2eb72d6 100644 --- a/c2rust-analyze/src/borrowck/mod.rs +++ b/c2rust-analyze/src/borrowck/mod.rs @@ -326,6 +326,26 @@ fn run_polonius<'tcx>( local_ltys.push(lty); } + // Assign origins to `rvalue_tys`. We sort the keys first to ensure that we assign origins in + // a deterministic order. + let mut keys = acx.rvalue_tys.keys().collect::>(); + keys.sort(); + let rvalue_ltys = keys + .into_iter() + .map(|&loc| { + let lty = acx.rvalue_tys[&loc]; + let new_lty = assign_origins( + ltcx, + hypothesis, + &mut facts, + &mut maps, + &acx.gacx.adt_metadata, + lty, + ); + (loc, new_lty) + }) + .collect::>(); + // Gather field permissions let field_permissions = field_ltys .iter() @@ -348,6 +368,7 @@ fn run_polonius<'tcx>( &mut maps, &mut loans, &local_ltys, + &rvalue_ltys, &field_permissions, hypothesis, mir, diff --git a/c2rust-analyze/src/borrowck/type_check.rs b/c2rust-analyze/src/borrowck/type_check.rs index fc098acde2..c0f323b9fc 100644 --- a/c2rust-analyze/src/borrowck/type_check.rs +++ b/c2rust-analyze/src/borrowck/type_check.rs @@ -26,6 +26,7 @@ struct TypeChecker<'tcx, 'a> { maps: &'a mut AtomMaps<'tcx>, loans: &'a mut HashMap>, local_ltys: &'a [LTy<'tcx>], + rvalue_ltys: &'a HashMap>, field_permissions: &'a HashMap, hypothesis: &'a PointerTableMut<'a, PermissionSet>, local_decls: &'a IndexVec>, @@ -389,7 +390,7 @@ impl<'tcx> TypeChecker<'tcx, '_> { self.do_assign(result_lty, op_lty); result_lty } - Rvalue::Cast(_, _, ty) => self.ltcx.label(ty, &mut |_ty| { + Rvalue::Cast(_, _, _ty) => { // TODO: handle Unsize casts at minimum /* assert!( @@ -397,8 +398,9 @@ impl<'tcx> TypeChecker<'tcx, '_> { "pointer Cast NYI" ); */ - Label::default() - }), + // TODO: connect this type to the type of the operand + self.rvalue_ltys[&self.current_location] + } Rvalue::Aggregate(ref kind, ref ops) => match **kind { AggregateKind::Array(..) => { let ty = rv.ty(self.local_decls, *self.ltcx); @@ -623,6 +625,7 @@ pub fn visit_body<'tcx>( maps: &mut AtomMaps<'tcx>, loans: &mut HashMap>, local_ltys: &[LTy<'tcx>], + rvalue_ltys: &HashMap>, field_permissions: &HashMap, hypothesis: &PointerTableMut, mir: &Body<'tcx>, @@ -635,6 +638,7 @@ pub fn visit_body<'tcx>( maps, loans, local_ltys, + rvalue_ltys, field_permissions, hypothesis, local_decls: &mir.local_decls, diff --git a/c2rust-analyze/src/rewrite/apply.rs b/c2rust-analyze/src/rewrite/apply.rs index 2f9370a28b..d9e6b53e6b 100644 --- a/c2rust-analyze/src/rewrite/apply.rs +++ b/c2rust-analyze/src/rewrite/apply.rs @@ -361,7 +361,7 @@ impl Emitter<'_, S> { }) } Rewrite::MethodCall(ref method, ref receiver_rw, ref arg_rws) => { - self.emit(receiver_rw, 0)?; + self.emit(receiver_rw, 3)?; self.emit_str(".")?; self.emit_str(method)?; self.emit_parenthesized(true, |slf| { diff --git a/c2rust-analyze/src/rewrite/expr/convert.rs b/c2rust-analyze/src/rewrite/expr/convert.rs index 3815eb4cf9..6ee0b0bde2 100644 --- a/c2rust-analyze/src/rewrite/expr/convert.rs +++ b/c2rust-analyze/src/rewrite/expr/convert.rs @@ -345,6 +345,7 @@ impl<'tcx> ConvertVisitor<'tcx> { elem_size, src_single, dest_single, + option, } => { // `realloc(p, n)` -> `Box::new(...)` assert!(matches!(hir_rw, Rewrite::Identity)); @@ -364,9 +365,14 @@ impl<'tcx> ConvertVisitor<'tcx> { } let expr = match (src_single, dest_single) { (false, false) => { + let src = if option { + "src_ptr.unwrap_or(Box::new([]))" + } else { + "src_ptr" + }; stmts.push(Rewrite::Let1( "mut dest_ptr".into(), - Box::new(Rewrite::Text("Vec::from(src_ptr)".into())), + Box::new(format_rewrite!("Vec::from({src})")), )); stmts.push(format_rewrite!( "dest_ptr.resize_with(dest_n, || {})", @@ -375,8 +381,9 @@ impl<'tcx> ConvertVisitor<'tcx> { Rewrite::Text("dest_ptr.into_boxed_slice()".into()) } (false, true) => { + let opt_flatten = if option { ".flatten()" } else { "" }; format_rewrite!( - "src_ptr.into_iter().next().unwrap_or_else(|| {})", + "src_ptr.into_iter(){opt_flatten}.next().unwrap_or_else(|| {})", zeroize_expr ) } @@ -385,16 +392,28 @@ impl<'tcx> ConvertVisitor<'tcx> { "mut dest_ptr".into(), Box::new(Rewrite::Text("Vec::with_capacity(dest_n)".into())), )); - stmts.push(Rewrite::Text( - "if dest_n >= 1 { dest_ptr.push(*src_ptr); }".into(), - )); + if option { + stmts.push(Rewrite::Text( + "if dest_n >= 1 { if let Some(src) = src_ptr { dest_ptr.push(*src); } }".into(), + )); + } else { + stmts.push(Rewrite::Text( + "if dest_n >= 1 { dest_ptr.push(*src_ptr); }".into(), + )); + } stmts.push(format_rewrite!( "dest_ptr.resize_with(dest_n, || {})", zeroize_expr, )); Rewrite::Text("dest_ptr.into_boxed_slice()".into()) } - (true, true) => Rewrite::Text("src_ptr".into()), + (true, true) => { + if option { + format_rewrite!("src_ptr.unwrap_or_else(|| Box::new({}))", zeroize_expr) + } else { + Rewrite::Text("src_ptr".into()) + } + } }; Rewrite::Block(stmts, Some(Box::new(expr))) } @@ -679,6 +698,7 @@ fn generate_zeroize_code(zero_ty: &ZeroizeType, lv: &str) -> String { match *zero_ty { ZeroizeType::Int => format!("{lv} = 0"), ZeroizeType::Bool => format!("{lv} = false"), + ZeroizeType::Option => format!("{lv} = None"), ZeroizeType::Array(ref elem_zero_ty) => format!( " {{ @@ -712,6 +732,7 @@ fn generate_zeroize_expr(zero_ty: &ZeroizeType) -> String { match *zero_ty { ZeroizeType::Int => format!("0"), ZeroizeType::Bool => format!("false"), + ZeroizeType::Option => format!("None"), ZeroizeType::Array(ref elem_zero_ty) => format!( "std::array::from_fn(|| {})", generate_zeroize_expr(elem_zero_ty) @@ -758,6 +779,11 @@ pub fn convert_cast_rewrite(kind: &mir_op::RewriteKind, hir_rw: Rewrite) -> Rewr Rewrite::Ref(Box::new(place), mutbl_from_bool(mutbl)) } + mir_op::RewriteKind::Deref => { + // `p` -> `*p` + Rewrite::Deref(Box::new(hir_rw)) + } + mir_op::RewriteKind::OptionUnwrap => { // `p` -> `p.unwrap()` Rewrite::MethodCall("unwrap".to_string(), Box::new(hir_rw), vec![]) diff --git a/c2rust-analyze/src/rewrite/expr/mir_op.rs b/c2rust-analyze/src/rewrite/expr/mir_op.rs index 968e9528f9..6db229a594 100644 --- a/c2rust-analyze/src/rewrite/expr/mir_op.rs +++ b/c2rust-analyze/src/rewrite/expr/mir_op.rs @@ -66,6 +66,9 @@ pub enum RewriteKind { /// Replace &raw with & or &raw mut with &mut RawToRef { mutbl: bool }, + /// Replace `ptr` with `*ptr`. + Deref, + /// Replace `ptr.is_null()` with `ptr.is_none()`. IsNullToIsNone, /// Replace `ptr.is_null()` with the constant `false`. We use this in cases where the rewritten @@ -108,6 +111,7 @@ pub enum RewriteKind { elem_size: u64, src_single: bool, dest_single: bool, + option: bool, }, CallocSafe { zero_ty: ZeroizeType, @@ -135,7 +139,7 @@ pub enum RewriteKind { /// Extract the `T` from `DynOwned`. DynOwnedUnwrap, - /// Move out of a `DynOwned` and set the original location to empty / non-owned. + /// Move out of `&mut DynOwned` and set the original location to empty / non-owned. DynOwnedTake, /// Wrap `T` in `Ok` to produce `DynOwned`. DynOwnedWrap, @@ -172,6 +176,8 @@ pub enum ZeroizeType { Int, /// Zeroize by storing the literal `false`. Bool, + /// Zeroize by storing `None`. + Option, /// Iterate over `x.iter_mut()` and zeroize each element. Array(Box), /// Zeroize each named field. @@ -445,10 +451,25 @@ impl<'a, 'tcx> ExprRewriteVisitor<'a, 'tcx> { // `visit_place` directly with the desired access. 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); + eprintln!("END visit_place for ownership transfer"); }); }); + // Obtain a reference to the place containing the `DynOwned` pointer. + if v.is_nullable(rv_lty.label) { + v.emit(RewriteKind::OptionDowngrade { + mutbl: true, + deref: false, + }); + v.emit(RewriteKind::OptionMapBegin); + v.emit(RewriteKind::Deref); + } + // Take the pointer out of that place. v.emit(RewriteKind::DynOwnedTake); + if v.is_nullable(rv_lty.label) { + v.emit(RewriteKind::OptionMapEnd); + } v.emit_cast_lty_lty(rv_lty, pl_lty); return; } @@ -653,7 +674,10 @@ impl<'a, 'tcx> ExprRewriteVisitor<'a, 'tcx> { let pointee_lty = match dest_pointee { Some(x) => x, // TODO: emit void* cast before bailing out - None => return, + None => { + trace!("{callee:?}: no pointee type for dest"); + return; + } }; let orig_pointee_ty = pointee_lty.ty; @@ -664,12 +688,18 @@ impl<'a, 'tcx> ExprRewriteVisitor<'a, 'tcx> { let single = !v.perms[dest_lty.label] .intersects(PermissionSet::OFFSET_ADD | PermissionSet::OFFSET_SUB); - // TODO: use rewritten types here, so that the `ZeroizeType` will - // reflect the actual types and fields after rewriting. - let zero_ty = match ZeroizeType::from_ty(tcx, orig_pointee_ty) { + let opt_zero_ty = + ZeroizeType::from_lty(&v.acx, &v.perms, &v.flags, pointee_lty); + let zero_ty = match opt_zero_ty { Some(x) => x, // TODO: emit void* cast before bailing out - None => return, + None => { + trace!( + "{callee:?}: failed to compute ZeroizeType \ + for {pointee_lty:?}" + ); + return; + } }; let rw = match *callee { @@ -739,7 +769,7 @@ impl<'a, 'tcx> ExprRewriteVisitor<'a, 'tcx> { }); } - Callee::Realloc => { + ref callee @ Callee::Realloc => { self.enter_rvalue(|v| { let src_lty = v.acx.type_of(&args[0]); let src_pointee = v.pointee_lty(src_lty); @@ -749,7 +779,13 @@ impl<'a, 'tcx> ExprRewriteVisitor<'a, 'tcx> { let pointee_lty = match common_pointee { Some(x) => x, // TODO: emit void* cast before bailing out - None => return, + None => { + trace!( + "{callee:?}: no common pointee type \ + between {src_pointee:?} and {dest_pointee:?}" + ); + return; + } }; let orig_pointee_ty = pointee_lty.ty; @@ -767,21 +803,35 @@ impl<'a, 'tcx> ExprRewriteVisitor<'a, 'tcx> { let zero_ty = match ZeroizeType::from_ty(tcx, orig_pointee_ty) { Some(x) => x, // TODO: emit void* cast before bailing out - None => return, + None => { + trace!( + "{callee:?}: failed to compute ZeroizeType \ + for {orig_pointee_ty:?}" + ); + return; + } }; // Cast input to either `Box` or `Box<[T]>`, as in `free`. + let mut option = false; v.enter_call_arg(0, |v| { - v.emit_cast_lty_adjust(src_lty, |desc| TypeDesc { - own: Ownership::Box, - qty: if src_single { - Quantity::Single - } else { - Quantity::Slice - }, - dyn_owned: false, - option: desc.option, - pointee_ty: desc.pointee_ty, + v.emit_cast_lty_adjust(src_lty, |desc| { + // `realloc(NULL, ...)` is explicitly allowed by the spec, so + // we can't force an unwrap here by returning `option: false`. + // Instead, we record the `option` flag as part of the rewrite + // so the nullable case can be handled appropriately. + option = desc.option; + TypeDesc { + own: Ownership::Box, + qty: if src_single { + Quantity::Single + } else { + Quantity::Slice + }, + dyn_owned: false, + option: desc.option, + pointee_ty: desc.pointee_ty, + } }); }); @@ -790,21 +840,25 @@ impl<'a, 'tcx> ExprRewriteVisitor<'a, 'tcx> { elem_size, src_single, dest_single, + option, }); // Cast output from `Box`/`Box<[T]>` to the target type, as in // `malloc`. v.emit_cast_adjust_lty( - |desc| TypeDesc { - own: Ownership::Box, - qty: if dest_single { - Quantity::Single - } else { - Quantity::Slice - }, - dyn_owned: false, - option: false, - pointee_ty: desc.pointee_ty, + |desc| { + TypeDesc { + own: Ownership::Box, + qty: if dest_single { + Quantity::Single + } else { + Quantity::Slice + }, + dyn_owned: false, + // We always return non-null from `realloc`. + option: false, + pointee_ty: desc.pointee_ty, + } }, dest_lty, ); @@ -1025,6 +1079,7 @@ 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()`. @@ -1036,12 +1091,15 @@ impl<'a, 'tcx> ExprRewriteVisitor<'a, 'tcx> { if !desc.own.is_copy() { v.emit(RewriteKind::OptionDowngrade { mutbl: access == PlaceAccess::Mut, - deref: true, + deref: !dyn_owned, }); } v.emit(RewriteKind::OptionUnwrap); + if dyn_owned { + v.emit(RewriteKind::Deref); + } } - if v.is_dyn_owned(base_lty) { + if dyn_owned { v.emit(RewriteKind::DynOwnedDowngrade { mutbl: access == PlaceAccess::Mut, }); @@ -1222,6 +1280,64 @@ impl ZeroizeType { _ => return None, }) } + + fn from_lty<'tcx>( + acx: &AnalysisCtxt<'_, 'tcx>, + perms: &PointerTable<'_, PermissionSet>, + flags: &PointerTable<'_, FlagSet>, + lty: LTy<'tcx>, + ) -> Option { + Some(match *lty.kind() { + TyKind::Int(_) | TyKind::Uint(_) => ZeroizeType::Int, + TyKind::Bool => ZeroizeType::Bool, + TyKind::Adt(adt_def, substs) => { + if !adt_def.is_struct() { + eprintln!("ZeroizeType::from_lty({lty:?}): not a struct"); + return None; + } + let variant = adt_def.non_enum_variant(); + let mut fields = Vec::with_capacity(variant.fields.len()); + for (field_idx, field) in variant.fields.iter().enumerate() { + let name = field.name.to_string(); + let ty = field.ty(acx.tcx(), substs); + let lty = acx.projection_lty(lty, &PlaceElem::Field(field_idx.into(), ty)); + let zero = ZeroizeType::from_lty(acx, perms, flags, lty)?; + fields.push((name, zero)); + } + + let name_printer = FmtPrinter::new(acx.tcx(), Namespace::ValueNS); + let name = name_printer + .print_value_path(adt_def.did(), &[]) + .unwrap() + .into_buffer(); + + ZeroizeType::Struct(name, fields) + } + TyKind::Array(_, _) => { + let elem_zero = ZeroizeType::from_lty(acx, perms, flags, lty.args[0])?; + ZeroizeType::Array(Box::new(elem_zero)) + } + TyKind::Ref(..) | TyKind::RawPtr(..) => { + if lty.label.is_none() { + eprintln!("ZeroizeType::from_lty({lty:?}): ptr has no label"); + return None; + } + let ptr = lty.label; + if flags[ptr].contains(FlagSet::FIXED) { + eprintln!("ZeroizeType::from_lty({lty:?}): ptr is FIXED"); + return None; + } + let nullable = !perms[ptr].contains(PermissionSet::NON_NULL); + if nullable { + ZeroizeType::Option + } else { + eprintln!("ZeroizeType::from_lty({lty:?}): ptr is NON_NULL"); + return None; + } + } + _ => return None, + }) + } } pub struct CastBuilder<'a, 'tcx, PT1, PT2, F> { @@ -1285,6 +1401,12 @@ where return Ok(()); } + // To downgrade and unwrap a non-`dyn_owned` pointer, we call `p.as_deref().unwrap()`, + // which goes from `Option>` to `&T`. To downgrade a `dyn_owned` pointer, we + // instead do `*p.as_ref().unwrap()`, which goes from `Option>` to + // `DynOwned`, with the latter being a read-only `Place` that can be further downgraded + // to eventually reach `&T`. + let mut deref_after_unwrap = false; if from.option && from.own != to.own { // Downgrade ownership before unwrapping the `Option` when possible. This can avoid // moving/consuming the input. For example, if the `from` type is `Option>` and @@ -1297,15 +1419,17 @@ where Ownership::Raw | Ownership::Imm => { (self.emit)(RewriteKind::OptionDowngrade { mutbl: false, - deref: true, + deref: !from.dyn_owned, }); + deref_after_unwrap = from.dyn_owned; from.own = Ownership::Imm; } Ownership::RawMut | Ownership::Cell | Ownership::Mut => { (self.emit)(RewriteKind::OptionDowngrade { mutbl: true, - deref: true, + deref: !from.dyn_owned, }); + deref_after_unwrap = from.dyn_owned; from.own = Ownership::Mut; } Ownership::Rc if from.own == Ownership::Rc => { @@ -1328,6 +1452,9 @@ where if from.option && !to.option { // Unwrap first, then perform remaining casts. (self.emit)(RewriteKind::OptionUnwrap); + if deref_after_unwrap { + (self.emit)(RewriteKind::Deref); + } from.option = false; } else if from.option && to.option { trace!("try_build_cast_desc_desc: emit OptionMapBegin"); @@ -1345,6 +1472,9 @@ where ); } (self.emit)(RewriteKind::OptionMapBegin); + if deref_after_unwrap { + (self.emit)(RewriteKind::Deref); + } from.option = false; in_option_map = true; } diff --git a/c2rust-analyze/tests/filecheck.rs b/c2rust-analyze/tests/filecheck.rs index 94672a3865..bba56350b2 100644 --- a/c2rust-analyze/tests/filecheck.rs +++ b/c2rust-analyze/tests/filecheck.rs @@ -63,6 +63,7 @@ define_tests! { pointee, ptrptr1, regions_fixed, + rewrite_nullable_box, rewrite_paths, rewrite_paths_manual_shim, statics, diff --git a/c2rust-analyze/tests/filecheck/rewrite_nullable_box.rs b/c2rust-analyze/tests/filecheck/rewrite_nullable_box.rs new file mode 100644 index 0000000000..8c8ec09d81 --- /dev/null +++ b/c2rust-analyze/tests/filecheck/rewrite_nullable_box.rs @@ -0,0 +1,35 @@ +//! Test cases for rewriting of nullable owned pointers. +#![feature(rustc_private)] +#![feature(register_tool)] +#![register_tool(c2rust_analyze_test)] + +use std::mem; +extern crate libc; + +extern "C" { + fn malloc(_: libc::c_ulong) -> *mut libc::c_void; + fn free(__ptr: *mut libc::c_void); +} + +#[c2rust_analyze_test::skip_borrowck] +// CHECK-LABEL: fn test_is_null( +pub unsafe fn test_is_null(cond: bool) { + // CHECK: let mut ptr: core::option::Option,()>> = + let mut ptr: *mut i32 = malloc(4) as *mut i32; + // This assignment exercises the option + dyn_owned case in `visit_place_ref`. + // CHECK: *(*(ptr).as_mut().unwrap()).as_deref_mut().unwrap() = 1; + *ptr = 1; + if cond { + ptr = 0 as *mut i32; + } + // This condition exercises the option + dyn_owned case in `try_build_cast_desc_desc`. + // CHECK: if !(ptr).as_ref().map(|__ptr| (*__ptr).as_deref().unwrap()).is_none() { + if !ptr.is_null() { + // CHECK: *(*(ptr).as_mut().unwrap()).as_deref_mut().unwrap() = 2; + *ptr = 2; + } + // This condition exercises the option + dyn_owned case in the `assignment_transfers_ownership` + // case of `visit_statement`. + // CHECK: std::mem::drop(((ptr).as_mut().map(|__ptr| std::mem::replace(&mut *__ptr,Err(()))).map(|__ptr| __ptr.unwrap()))); + free(ptr as *mut libc::c_void); +}