Skip to content

Commit

Permalink
Auto merge of #26173 - pnkfelix:fsk-trans-nzmove-take3, r=nikomatsakis
Browse files Browse the repository at this point in the history
Add dropflag hints (stack-local booleans) for unfragmented paths in trans.  Part of #5016.

Added code to maintain these hints at runtime, and to conditionalize drop-filling and calls to destructors.

In this early stage of my incremental implementation strategy, we are using hints, so we are always free to leave out a flag for a path -- then we just pass `None` as the dropflag hint in the corresponding schedule cleanup call. But, once a path has a hint, we must at least maintain it: i.e. if the hint exists, we must ensure it is never set to "moved" if the data in question might actually have been initialized. It remains sound to conservatively set the hint to "initialized" as long as the true drop-flag embedded in the value itself is up-to-date.

I hope the commit series has been broken up to be readable; most of the commits in the series should build (though I did not always check this).

----

Oh, I think this technically qualifies as a:
[breaking-change]
because it removes drop-filling in some cases where one could previously observe it. That should only affect `unsafe` code; no safe code should be able to inspect whether the drop-fill was present or not. For an example of code that needed to change to account for this, see commit a81c24ae0216ab47df59acd724f8a33124fb6d97 (a unit test of the `move_val_init` intrinsic).  I have not encountered actual code that needed to be updated to account for the change, since this should only be skipping the drop-filling on *moved* values, not on dropped one, and so even types that use `unsafe_no_drop_flag` should be unchanged by this particular PR. (Their time will come later.)
  • Loading branch information
bors committed Jul 28, 2015
2 parents ec49d01 + b4dd765 commit 661a5ad
Show file tree
Hide file tree
Showing 19 changed files with 832 additions and 164 deletions.
39 changes: 39 additions & 0 deletions src/librustc/middle/ty.rs
Original file line number Diff line number Diff line change
Expand Up @@ -957,6 +957,44 @@ pub struct ctxt<'tcx> {
/// Maps a cast expression to its kind. This is keyed on the
/// *from* expression of the cast, not the cast itself.
pub cast_kinds: RefCell<NodeMap<cast::CastKind>>,

/// Maps Fn items to a collection of fragment infos.
///
/// The main goal is to identify data (each of which may be moved
/// or assigned) whose subparts are not moved nor assigned
/// (i.e. their state is *unfragmented*) and corresponding ast
/// nodes where the path to that data is moved or assigned.
///
/// In the long term, unfragmented values will have their
/// destructor entirely driven by a single stack-local drop-flag,
/// and their parents, the collections of the unfragmented values
/// (or more simply, "fragmented values"), are mapped to the
/// corresponding collections of stack-local drop-flags.
///
/// (However, in the short term that is not the case; e.g. some
/// unfragmented paths still need to be zeroed, namely when they
/// reference parent data from an outer scope that was not
/// entirely moved, and therefore that needs to be zeroed so that
/// we do not get double-drop when we hit the end of the parent
/// scope.)
///
/// Also: currently the table solely holds keys for node-ids of
/// unfragmented values (see `FragmentInfo` enum definition), but
/// longer-term we will need to also store mappings from
/// fragmented data to the set of unfragmented pieces that
/// constitute it.
pub fragment_infos: RefCell<DefIdMap<Vec<FragmentInfo>>>,
}

/// Describes the fragment-state associated with a NodeId.
///
/// Currently only unfragmented paths have entries in the table,
/// but longer-term this enum is expected to expand to also
/// include data for fragmented paths.
#[derive(Copy, Clone, Debug)]
pub enum FragmentInfo {
Moved { var: NodeId, move_expr: NodeId },
Assigned { var: NodeId, assign_expr: NodeId, assignee_id: NodeId },
}

impl<'tcx> ctxt<'tcx> {
Expand Down Expand Up @@ -3498,6 +3536,7 @@ impl<'tcx> ctxt<'tcx> {
const_qualif_map: RefCell::new(NodeMap()),
custom_coerce_unsized_kinds: RefCell::new(DefIdMap()),
cast_kinds: RefCell::new(NodeMap()),
fragment_infos: RefCell::new(DefIdMap()),
}, f)
}

Expand Down
2 changes: 2 additions & 0 deletions src/librustc/session/config.rs
Original file line number Diff line number Diff line change
Expand Up @@ -594,6 +594,8 @@ options! {DebuggingOptions, DebuggingSetter, basic_debugging_options,
"Force drop flag checks on or off"),
trace_macros: bool = (false, parse_bool,
"For every macro invocation, print its name and arguments"),
disable_nonzeroing_move_hints: bool = (false, parse_bool,
"Force nonzeroing move optimization off"),
}

pub fn default_lib_output() -> CrateType {
Expand Down
3 changes: 3 additions & 0 deletions src/librustc/session/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -272,6 +272,9 @@ impl Session {
pub fn print_enum_sizes(&self) -> bool {
self.opts.debugging_opts.print_enum_sizes
}
pub fn nonzeroing_move_hints(&self) -> bool {
!self.opts.debugging_opts.disable_nonzeroing_move_hints
}
pub fn sysroot<'a>(&'a self) -> &'a Path {
match self.opts.maybe_sysroot {
Some (ref sysroot) => sysroot,
Expand Down
4 changes: 2 additions & 2 deletions src/librustc_borrowck/borrowck/check_loans.rs
Original file line number Diff line number Diff line change
Expand Up @@ -86,7 +86,7 @@ fn owned_ptr_base_path_rc<'tcx>(loan_path: &Rc<LoanPath<'tcx>>) -> Rc<LoanPath<'
struct CheckLoanCtxt<'a, 'tcx: 'a> {
bccx: &'a BorrowckCtxt<'a, 'tcx>,
dfcx_loans: &'a LoanDataFlow<'a, 'tcx>,
move_data: move_data::FlowedMoveData<'a, 'tcx>,
move_data: &'a move_data::FlowedMoveData<'a, 'tcx>,
all_loans: &'a [Loan<'tcx>],
param_env: &'a ty::ParameterEnvironment<'a, 'tcx>,
}
Expand Down Expand Up @@ -191,7 +191,7 @@ impl<'a, 'tcx> euv::Delegate<'tcx> for CheckLoanCtxt<'a, 'tcx> {

pub fn check_loans<'a, 'b, 'c, 'tcx>(bccx: &BorrowckCtxt<'a, 'tcx>,
dfcx_loans: &LoanDataFlow<'b, 'tcx>,
move_data: move_data::FlowedMoveData<'c, 'tcx>,
move_data: &move_data::FlowedMoveData<'c, 'tcx>,
all_loans: &[Loan<'tcx>],
fn_id: ast::NodeId,
decl: &ast::FnDecl,
Expand Down
88 changes: 87 additions & 1 deletion src/librustc_borrowck/borrowck/fragments.rs
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@
use self::Fragment::*;

use borrowck::InteriorKind::{InteriorField, InteriorElement};
use borrowck::LoanPath;
use borrowck::{self, LoanPath};
use borrowck::LoanPathKind::{LpVar, LpUpvar, LpDowncast, LpExtend};
use borrowck::LoanPathElem::{LpDeref, LpInterior};
use borrowck::move_data::InvalidMovePathIndex;
Expand Down Expand Up @@ -59,6 +59,84 @@ impl Fragment {
}
}

pub fn build_unfragmented_map(this: &mut borrowck::BorrowckCtxt,
move_data: &MoveData,
id: ast::NodeId) {
let fr = &move_data.fragments.borrow();

// For now, don't care about other kinds of fragments; the precise
// classfication of all paths for non-zeroing *drop* needs them,
// but the loose approximation used by non-zeroing moves does not.
let moved_leaf_paths = fr.moved_leaf_paths();
let assigned_leaf_paths = fr.assigned_leaf_paths();

let mut fragment_infos = Vec::with_capacity(moved_leaf_paths.len());

let find_var_id = |move_path_index: MovePathIndex| -> Option<ast::NodeId> {
let lp = move_data.path_loan_path(move_path_index);
match lp.kind {
LpVar(var_id) => Some(var_id),
LpUpvar(ty::UpvarId { var_id, closure_expr_id }) => {
// The `var_id` is unique *relative to* the current function.
// (Check that we are indeed talking about the same function.)
assert_eq!(id, closure_expr_id);
Some(var_id)
}
LpDowncast(..) | LpExtend(..) => {
// This simple implementation of non-zeroing move does
// not attempt to deal with tracking substructure
// accurately in the general case.
None
}
}
};

let moves = move_data.moves.borrow();
for &move_path_index in moved_leaf_paths {
let var_id = match find_var_id(move_path_index) {
None => continue,
Some(var_id) => var_id,
};

move_data.each_applicable_move(move_path_index, |move_index| {
let info = ty::FragmentInfo::Moved {
var: var_id,
move_expr: moves[move_index.get()].id,
};
debug!("fragment_infos push({:?} \
due to move_path_index: {} move_index: {}",
info, move_path_index.get(), move_index.get());
fragment_infos.push(info);
true
});
}

for &move_path_index in assigned_leaf_paths {
let var_id = match find_var_id(move_path_index) {
None => continue,
Some(var_id) => var_id,
};

let var_assigns = move_data.var_assignments.borrow();
for var_assign in var_assigns.iter()
.filter(|&assign| assign.path == move_path_index)
{
let info = ty::FragmentInfo::Assigned {
var: var_id,
assign_expr: var_assign.id,
assignee_id: var_assign.assignee_id,
};
debug!("fragment_infos push({:?} due to var_assignment", info);
fragment_infos.push(info);
}
}

let mut fraginfo_map = this.tcx.fragment_infos.borrow_mut();
let fn_did = ast::DefId { krate: ast::LOCAL_CRATE, node: id };
let prev = fraginfo_map.insert(fn_did, fragment_infos);
assert!(prev.is_none());
}

pub struct FragmentSets {
/// During move_data construction, `moved_leaf_paths` tracks paths
/// that have been used directly by being moved out of. When
Expand Down Expand Up @@ -103,6 +181,14 @@ impl FragmentSets {
}
}

pub fn moved_leaf_paths(&self) -> &[MovePathIndex] {
&self.moved_leaf_paths
}

pub fn assigned_leaf_paths(&self) -> &[MovePathIndex] {
&self.assigned_leaf_paths
}

pub fn add_move(&mut self, path_index: MovePathIndex) {
self.moved_leaf_paths.push(path_index);
}
Expand Down
5 changes: 4 additions & 1 deletion src/librustc_borrowck/borrowck/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -166,10 +166,13 @@ fn borrowck_fn(this: &mut BorrowckCtxt,
this.tcx,
sp,
id);
move_data::fragments::build_unfragmented_map(this,
&flowed_moves.move_data,
id);

check_loans::check_loans(this,
&loan_dfcx,
flowed_moves,
&flowed_moves,
&all_loans[..],
id,
decl,
Expand Down
4 changes: 4 additions & 0 deletions src/librustc_borrowck/borrowck/move_data.rs
Original file line number Diff line number Diff line change
Expand Up @@ -159,6 +159,9 @@ pub struct Assignment {

/// span of node where assignment occurs
pub span: Span,

/// id for l-value expression on lhs of assignment
pub assignee_id: ast::NodeId,
}

#[derive(Copy, Clone)]
Expand Down Expand Up @@ -412,6 +415,7 @@ impl<'tcx> MoveData<'tcx> {
path: path_index,
id: assign_id,
span: span,
assignee_id: assignee_id,
};

if self.is_var_path(path_index) {
Expand Down
Loading

0 comments on commit 661a5ad

Please sign in to comment.