Skip to content

Commit

Permalink
analyze: move Option<&mut T> on last use instead of reborrowing (#1179
Browse files Browse the repository at this point in the history
)

Currently, we reborrow `Option<&mut T>` at most use sites by calling
`p.as_deref_mut()`. This creates a new reference that's tied to the
lifetime of the `Option`, rather than the lifetime of the original
reference. This is fine for temporaries and local variables, but creates
a problem when returning the result from the current function:

```Rust
fn f(x: Option<&mut Foo>) -> Option<&mut i32> {
    x.as_deref_mut().map(|x| &mut x.field)
}
```

This produces a borrowck error because `as_deref_mut` has the signature
`(&'b mut Option<&'a mut T>) -> Option<&'b mut T>`, meaning the result
must not outlive `x`.

The fix here is to consume `x`:

```Rust
fn f(x: Option<&mut Foo>) -> Option<&mut i32> {
    // No `.as_deref_mut()` - `x` is moved into `map()`
    x.map(|x| &mut x.field)
}
```

In this case, `map` has the signature `(Option<&'a mut T>, [closure]) ->
Option<&'a mut i32>`, preserving the lifetime of the input reference.

This branch adds an analysis to identify the "last use" of each local
variable and modifies the rewriting rules around `Option<T>` to omit
`.as_deref_mut()` at the last use.
  • Loading branch information
spernsteiner authored Dec 9, 2024
2 parents 78c32c5 + 8f40e87 commit 9fcadbe
Show file tree
Hide file tree
Showing 7 changed files with 778 additions and 60 deletions.
127 changes: 106 additions & 21 deletions c2rust-analyze/src/analyze.rs
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ use crate::dataflow::DataflowConstraints;
use crate::equiv::GlobalEquivSet;
use crate::equiv::LocalEquivSet;
use crate::labeled_ty::LabeledTyCtxt;
use crate::last_use::{self, LastUse};
use crate::panic_detail;
use crate::panic_detail::PanicDetail;
use crate::pointee_type;
Expand Down Expand Up @@ -536,6 +537,31 @@ fn get_skip_pointee_defs() -> io::Result<HashSet<DefId>> {
Ok(skip_pointee)
}

fn get_rewrite_mode(tcx: TyCtxt, pointwise_fn_ldid: Option<LocalDefId>) -> rewrite::UpdateFiles {
let mut update_files = rewrite::UpdateFiles::No;
if let Ok(val) = env::var("C2RUST_ANALYZE_REWRITE_MODE") {
match val.as_str() {
"none" => {}
"inplace" => {
update_files = rewrite::UpdateFiles::InPlace;
}
"alongside" => {
update_files = rewrite::UpdateFiles::Alongside;
}
"pointwise" => {
let pointwise_fn_ldid = pointwise_fn_ldid.expect(
"C2RUST_ANALYZE_REWRITE_MODE=pointwise, \
but pointwise_fn_ldid is unset?",
);
let pointwise_fn_name = tcx.item_name(pointwise_fn_ldid.to_def_id());
update_files = rewrite::UpdateFiles::AlongsidePointwise(pointwise_fn_name);
}
_ => panic!("bad value {:?} for C2RUST_ANALYZE_REWRITE_MODE", val),
}
}
update_files
}

/// Local information, specific to a single function. Many of the data structures we use for
/// the pointer analysis have a "global" part that's shared between all functions and a "local"
/// part that's specific to the function being analyzed; this struct contains only the local
Expand All @@ -558,6 +584,9 @@ struct FuncInfo<'tcx> {
local_pointee_types: MaybeUnset<LocalPointerTable<PointeeTypes<'tcx>>>,
/// Table for looking up the most recent write to a given local.
recent_writes: MaybeUnset<RecentWrites>,
/// Analysis result indicating which uses of each local are actually the last use of that
/// local.
last_use: MaybeUnset<LastUse>,
}

fn run(tcx: TyCtxt) {
Expand Down Expand Up @@ -616,6 +645,7 @@ fn run(tcx: TyCtxt) {
// ----------------------------------

do_recent_writes(&gacx, &mut func_info, &all_fn_ldids);
do_last_use(&gacx, &mut func_info, &all_fn_ldids);

// ----------------------------------
// Remap `PointerId`s by equivalence class
Expand Down Expand Up @@ -1096,6 +1126,17 @@ fn run(tcx: TyCtxt) {
return;
}

if env::var("C2RUST_ANALYZE_DEBUG_LAST_USE").is_ok() {
let mut ann = AnnotationBuffer::new(tcx);
debug_annotate_last_use(&gacx, &func_info, &all_fn_ldids, &mut ann);
let annotations = ann.finish();
let update_files = get_rewrite_mode(tcx, None);
eprintln!("update mode = {:?}", update_files);
rewrite::apply_rewrites(tcx, Vec::new(), annotations, update_files);
eprintln!("finished writing last_use annotations - exiting");
return;
}

if !rewrite_pointwise {
run2(
None,
Expand Down Expand Up @@ -1239,6 +1280,7 @@ fn run2<'tcx>(
&mut acx,
&asn,
pointee_types,
&info.last_use,
ldid.to_def_id(),
&mir,
hir_body_id,
Expand Down Expand Up @@ -1547,27 +1589,7 @@ fn run2<'tcx>(
let annotations = ann.finish();

// Apply rewrite to all functions at once.
let mut update_files = rewrite::UpdateFiles::No;
if let Ok(val) = env::var("C2RUST_ANALYZE_REWRITE_MODE") {
match val.as_str() {
"none" => {}
"inplace" => {
update_files = rewrite::UpdateFiles::InPlace;
}
"alongside" => {
update_files = rewrite::UpdateFiles::Alongside;
}
"pointwise" => {
let pointwise_fn_ldid = pointwise_fn_ldid.expect(
"C2RUST_ANALYZE_REWRITE_MODE=pointwise, \
but pointwise_fn_ldid is unset?",
);
let pointwise_fn_name = tcx.item_name(pointwise_fn_ldid.to_def_id());
update_files = rewrite::UpdateFiles::AlongsidePointwise(pointwise_fn_name);
}
_ => panic!("bad value {:?} for C2RUST_ANALYZE_REWRITE_MODE", val),
}
}
let update_files = get_rewrite_mode(tcx, pointwise_fn_ldid);
rewrite::apply_rewrites(tcx, all_rewrites, annotations, update_files);

// ----------------------------------
Expand Down Expand Up @@ -1856,6 +1878,69 @@ fn do_recent_writes<'tcx>(
}
}

fn do_last_use<'tcx>(
gacx: &GlobalAnalysisCtxt<'tcx>,
func_info: &mut HashMap<LocalDefId, FuncInfo<'tcx>>,
all_fn_ldids: &[LocalDefId],
) {
let tcx = gacx.tcx;
for &ldid in all_fn_ldids {
if gacx.fn_analysis_invalid(ldid.to_def_id()) {
continue;
}

let ldid_const = WithOptConstParam::unknown(ldid);
let info = func_info.get_mut(&ldid).unwrap();
let mir = tcx.mir_built(ldid_const);
let mir = mir.borrow();

// This is very straightforward because it doesn't need an `AnalysisCtxt` and never fails.
info.last_use.set(last_use::calc_last_use(&mir));
}
}

fn debug_annotate_last_use<'tcx>(
gacx: &GlobalAnalysisCtxt<'tcx>,
func_info: &HashMap<LocalDefId, FuncInfo<'tcx>>,
all_fn_ldids: &[LocalDefId],
ann: &mut AnnotationBuffer,
) {
let tcx = gacx.tcx;
for &ldid in all_fn_ldids {
let ldid_const = WithOptConstParam::unknown(ldid);
let info = match func_info.get(&ldid) {
Some(x) => x,
None => continue,
};
let mir = tcx.mir_built(ldid_const);
let mir = mir.borrow();

if !info.last_use.is_set() {
continue;
}
let last_use = info.last_use.get();
let mut last_use = last_use.iter().collect::<Vec<_>>();
last_use.sort();
for (loc, which, local) in last_use {
let span = mir
.stmt_at(loc)
.either(|stmt| stmt.source_info.span, |term| term.source_info.span);
ann.emit(
span,
format!(
"{which:?}: last use of {} {local:?} ({})",
if mir.local_kind(local) == LocalKind::Temp {
"temporary"
} else {
"local"
},
describe_local(tcx, &mir.local_decls[local]),
),
);
}
}
}

/// Run the `pointee_type` analysis, which tries to determine the actual type of data that each
/// pointer can point to. This is particularly important for `void*` pointers, which are typically
/// cast to a different type before use.
Expand Down
Loading

0 comments on commit 9fcadbe

Please sign in to comment.