From 30da2a600cec4231ece4ae78b23010495c0f3dd0 Mon Sep 17 00:00:00 2001 From: Khyber Sen Date: Tue, 2 Aug 2022 00:05:52 -0700 Subject: [PATCH 1/5] Refactored `mod deref` so that most of the functions are implemented in terms of `try_remove_outer_deref_as_ref` so we know they're in sync. --- .../src/mir_utils/deref.rs | 40 +++++++++++-------- 1 file changed, 24 insertions(+), 16 deletions(-) diff --git a/dynamic_instrumentation/src/mir_utils/deref.rs b/dynamic_instrumentation/src/mir_utils/deref.rs index 75418e7076..fd9b67d5e8 100644 --- a/dynamic_instrumentation/src/mir_utils/deref.rs +++ b/dynamic_instrumentation/src/mir_utils/deref.rs @@ -3,13 +3,6 @@ use rustc_middle::{ ty::TyCtxt, }; -pub fn has_outer_deref(p: &Place) -> bool { - matches!( - p.iter_projections().last(), - Some((_, ProjectionElem::Deref)) - ) -} - /// Get the inner-most dereferenced [`Place`]. pub fn strip_all_deref<'tcx>(p: &Place<'tcx>, tcx: TyCtxt<'tcx>) -> Place<'tcx> { let mut base_dest = p.as_ref(); @@ -27,18 +20,33 @@ pub fn strip_all_deref<'tcx>(p: &Place<'tcx>, tcx: TyCtxt<'tcx>) -> Place<'tcx> } } -/// Strip the initital [`Deref`](ProjectionElem::Deref) -/// from a [`projection`](PlaceRef::projection) sequence. -pub fn remove_outer_deref<'tcx>(p: Place<'tcx>, tcx: TyCtxt<'tcx>) -> Place<'tcx> { +fn try_remove_outer_deref_as_ref<'tcx>(p: &Place<'tcx>) -> Option> { // Remove outer deref if present match p.as_ref() { PlaceRef { local, - projection: &[ref base @ .., ProjectionElem::Deref], - } => Place { - local, - projection: tcx.intern_place_elems(base), - }, - _ => p, + projection: &[ref projection @ .., ProjectionElem::Deref], + } => Some(PlaceRef { local, projection }), + _ => None, } } + +pub fn has_outer_deref(p: &Place) -> bool { + try_remove_outer_deref_as_ref(p).is_some() +} + +/// Try to strip the initital [`Deref`](ProjectionElem::Deref) +/// from a [`projection`](PlaceRef::projection) sequence. +pub fn try_remove_outer_deref<'tcx>(p: Place<'tcx>, tcx: TyCtxt<'tcx>) -> Option> { + try_remove_outer_deref_as_ref(&p).map(|PlaceRef { local, projection }| Place { + local, + projection: tcx.intern_place_elems(projection), + }) +} + +/// Strip the initital [`Deref`](ProjectionElem::Deref) +/// from a [`projection`](PlaceRef::projection) sequence +/// if there is one. +pub fn remove_outer_deref<'tcx>(p: Place<'tcx>, tcx: TyCtxt<'tcx>) -> Place<'tcx> { + try_remove_outer_deref(p, tcx).unwrap_or(p) +} From 682595eecb9643464b8b944cbc30d9cf757ed0d0 Mon Sep 17 00:00:00 2001 From: Khyber Sen Date: Tue, 2 Aug 2022 00:07:13 -0700 Subject: [PATCH 2/5] Combined the two `Rvalue::AddressOf` `match` arms as they're similar and this lets us eliminate duplicate calls. --- dynamic_instrumentation/src/instrument.rs | 34 +++++++++++------------ 1 file changed, 16 insertions(+), 18 deletions(-) diff --git a/dynamic_instrumentation/src/instrument.rs b/dynamic_instrumentation/src/instrument.rs index 793a480c4f..d52bf81e25 100644 --- a/dynamic_instrumentation/src/instrument.rs +++ b/dynamic_instrumentation/src/instrument.rs @@ -20,7 +20,9 @@ use std::sync::Mutex; use crate::arg::{ArgKind, InstrumentationArg}; use crate::hooks::Hooks; -use crate::mir_utils::{has_outer_deref, remove_outer_deref, strip_all_deref}; +use crate::mir_utils::{ + has_outer_deref, remove_outer_deref, strip_all_deref, try_remove_outer_deref, +}; use crate::point::cast_ptr_to_usize; use crate::point::InstrumentationAdder; use crate::point::InstrumentationApplier; @@ -229,25 +231,21 @@ impl<'tcx> Visitor<'tcx> for InstrumentationAdder<'_, 'tcx> { } } _ if !is_region_or_unsafe_ptr(value_ty) => {} - Rvalue::AddressOf(_, p) - if has_outer_deref(p) - && place_ty(&remove_outer_deref(*p, self.tcx())).is_region_ptr() => - { - let source = remove_outer_deref(*p, self.tcx()); - // Instrument which local's address is taken - self.loc(location.successor_within_block(), copy_fn) - .arg_var(dest) - .source(&source) - .dest(&dest) - .debug_mir(location) - .add_to(self); - } Rvalue::AddressOf(_, p) => { // Instrument which local's address is taken - self.loc(location.successor_within_block(), addr_local_fn) - .arg_var(dest) - .arg_index_of(p.local) - .source(p) + let source = try_remove_outer_deref(*p, self.tcx()) + .filter(|source| place_ty(source).is_region_ptr()); + let func = match source { + Some(_) => copy_fn, + None => addr_local_fn, + }; + let mut b = self + .loc(location.successor_within_block(), func) + .arg_var(dest); + if source.is_none() { + b = b.arg_index_of(p.local) + } + b.source(source.as_ref().unwrap_or(p)) .dest(&dest) .debug_mir(location) .add_to(self); From 2b7a1b6a93edfc942244d6e0cbf710263d4f3ca0 Mon Sep 17 00:00:00 2001 From: Khyber Sen Date: Tue, 2 Aug 2022 00:08:24 -0700 Subject: [PATCH 3/5] Added `apply` to simplify the conditional builder chain. --- Cargo.lock | 7 +++++++ dynamic_instrumentation/Cargo.toml | 1 + dynamic_instrumentation/src/instrument.rs | 15 ++++++++------- dynamic_instrumentation/src/point/mod.rs | 2 +- 4 files changed, 17 insertions(+), 8 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index e2e4cb5906..947e5df4a1 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -41,6 +41,12 @@ version = "1.0.57" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "08f9b8508dccb7687a1d6c4ce66b2b0ecef467c94667de27d8d7fe1f8d2a9cdc" +[[package]] +name = "apply" +version = "0.3.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "f47b57fc4521e3cae26a4d45b5227f8fadee4c345be0fefd8d5d1711afb8aeb9" + [[package]] name = "arc-swap" version = "1.5.0" @@ -284,6 +290,7 @@ name = "c2rust-dynamic-instrumentation" version = "0.1.0" dependencies = [ "anyhow", + "apply", "bincode", "c2rust-analysis-rt", "cargo", diff --git a/dynamic_instrumentation/Cargo.toml b/dynamic_instrumentation/Cargo.toml index 323f93ad79..d63e270db9 100644 --- a/dynamic_instrumentation/Cargo.toml +++ b/dynamic_instrumentation/Cargo.toml @@ -5,6 +5,7 @@ edition = "2021" [dependencies] anyhow = "1.0" +apply = "0.3" bincode = "1.0.1" c2rust-analysis-rt = { path = "../analysis/runtime"} cargo = "0.62" diff --git a/dynamic_instrumentation/src/instrument.rs b/dynamic_instrumentation/src/instrument.rs index d52bf81e25..70f695a4cf 100644 --- a/dynamic_instrumentation/src/instrument.rs +++ b/dynamic_instrumentation/src/instrument.rs @@ -1,4 +1,5 @@ use anyhow::Context; +use apply::Apply; use c2rust_analysis_rt::metadata::Metadata; use c2rust_analysis_rt::mir_loc::{self, EventMetadata, Func, MirLoc, MirLocId, TransferKind}; use c2rust_analysis_rt::HOOK_FUNCTIONS; @@ -239,13 +240,13 @@ impl<'tcx> Visitor<'tcx> for InstrumentationAdder<'_, 'tcx> { Some(_) => copy_fn, None => addr_local_fn, }; - let mut b = self - .loc(location.successor_within_block(), func) - .arg_var(dest); - if source.is_none() { - b = b.arg_index_of(p.local) - } - b.source(source.as_ref().unwrap_or(p)) + self.loc(location.successor_within_block(), func) + .arg_var(dest) + .apply(|b| match source { + Some(_) => b, + None => b.arg_index_of(p.local), + }) + .source(source.as_ref().unwrap_or(p)) .dest(&dest) .debug_mir(location) .add_to(self); diff --git a/dynamic_instrumentation/src/point/mod.rs b/dynamic_instrumentation/src/point/mod.rs index 136b29d2fb..c6b0a15a00 100644 --- a/dynamic_instrumentation/src/point/mod.rs +++ b/dynamic_instrumentation/src/point/mod.rs @@ -12,7 +12,7 @@ use rustc_span::def_id::DefId; use crate::{arg::InstrumentationArg, hooks::Hooks, util::Convert}; -pub use apply::InstrumentationApplier; +pub use self::apply::InstrumentationApplier; pub use cast::cast_ptr_to_usize; pub struct InstrumentationPoint<'tcx> { From f009781a4a260ebac5b84f02660dc2a6707ccc9c Mon Sep 17 00:00:00 2001 From: Khyber Sen Date: Tue, 2 Aug 2022 00:34:50 -0700 Subject: [PATCH 4/5] Applied a very similar refactor to the `Rvalue::Ref` `match` arms using `try_remove_outer_deref`. --- dynamic_instrumentation/src/instrument.rs | 59 ++++++++--------------- 1 file changed, 19 insertions(+), 40 deletions(-) diff --git a/dynamic_instrumentation/src/instrument.rs b/dynamic_instrumentation/src/instrument.rs index 70f695a4cf..478a491175 100644 --- a/dynamic_instrumentation/src/instrument.rs +++ b/dynamic_instrumentation/src/instrument.rs @@ -21,9 +21,7 @@ use std::sync::Mutex; use crate::arg::{ArgKind, InstrumentationArg}; use crate::hooks::Hooks; -use crate::mir_utils::{ - has_outer_deref, remove_outer_deref, strip_all_deref, try_remove_outer_deref, -}; +use crate::mir_utils::{remove_outer_deref, strip_all_deref, try_remove_outer_deref}; use crate::point::cast_ptr_to_usize; use crate::point::InstrumentationAdder; use crate::point::InstrumentationApplier; @@ -280,49 +278,30 @@ impl<'tcx> Visitor<'tcx> for InstrumentationAdder<'_, 'tcx> { .debug_mir(location) .add_to(self); } - Rvalue::Ref(_, bkind, p) if has_outer_deref(p) => { + Rvalue::Ref(_, bkind, p) => { // this is a reborrow or field reference, i.e. _2 = &(*_1) - let source = remove_outer_deref(*p, self.tcx()); - if let BorrowKind::Mut { .. } = bkind { + let source = try_remove_outer_deref(*p, self.tcx()); + let loc = if let BorrowKind::Mut { .. } = bkind { // Instrument which local's address is taken - self.loc(location, copy_fn) - .arg_addr_of(*p) - .source(&source) - .dest(&dest) - .debug_mir(location) - .add_to(self); + location } else { // Instrument immutable borrows by tracing the reference itself - self.loc(location.successor_within_block(), copy_fn) - .arg_var(dest) - .source(&source) - .dest(&dest) - .debug_mir(location) - .add_to(self); + location.successor_within_block() }; - } - Rvalue::Ref(_, bkind, p) if !p.is_indirect() => { - // this is a reborrow or field reference, i.e. _2 = &(*_1) - let source = remove_outer_deref(*p, self.tcx()); - if let BorrowKind::Mut { .. } = bkind { - // Instrument which local's address is taken - self.loc(location, addr_local_fn) - .arg_addr_of(*p) - .arg_index_of(p.local) - .source(&source) - .dest(&dest) - .debug_mir(location) - .add_to(self); - } else { - // Instrument immutable borrows by tracing the reference itself - self.loc(location.successor_within_block(), addr_local_fn) - .arg_var(dest) - .arg_index_of(p.local) - .source(&source) - .dest(&dest) - .debug_mir(location) - .add_to(self); + let func = match source { + Some(_) => copy_fn, + None => addr_local_fn, }; + self.loc(loc, func) + .arg_addr_of(*p) + .apply(|b| match source { + Some(_) => b, + None => b.arg_index_of(p.local), + }) + .source(source.as_ref().unwrap_or(p)) + .dest(&dest) + .debug_mir(location) + .add_to(self); } _ => (), } From 319656faff6d6ae880aee79d436cf309d67681ae Mon Sep 17 00:00:00 2001 From: Khyber Sen Date: Tue, 2 Aug 2022 00:35:43 -0700 Subject: [PATCH 5/5] `has_outer_deref` is dead code now, so remove it. It encouraged an anti-pattern anyways, using `has_outer_deref` and then `remove_outer_deref` instead of `try_remove_outer_deref`. --- dynamic_instrumentation/src/mir_utils/deref.rs | 4 ---- 1 file changed, 4 deletions(-) diff --git a/dynamic_instrumentation/src/mir_utils/deref.rs b/dynamic_instrumentation/src/mir_utils/deref.rs index fd9b67d5e8..de5bcccc6a 100644 --- a/dynamic_instrumentation/src/mir_utils/deref.rs +++ b/dynamic_instrumentation/src/mir_utils/deref.rs @@ -31,10 +31,6 @@ fn try_remove_outer_deref_as_ref<'tcx>(p: &Place<'tcx>) -> Option } } -pub fn has_outer_deref(p: &Place) -> bool { - try_remove_outer_deref_as_ref(p).is_some() -} - /// Try to strip the initital [`Deref`](ProjectionElem::Deref) /// from a [`projection`](PlaceRef::projection) sequence. pub fn try_remove_outer_deref<'tcx>(p: Place<'tcx>, tcx: TyCtxt<'tcx>) -> Option> {