Skip to content

Extend Polonius fact generation for (some) move tracking #62800

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 8 commits into from
Sep 5, 2019
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
60 changes: 58 additions & 2 deletions src/librustc_mir/borrow_check/nll/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -4,14 +4,14 @@ use crate::borrow_check::nll::facts::AllFactsExt;
use crate::borrow_check::nll::type_check::{MirTypeckResults, MirTypeckRegionConstraints};
use crate::borrow_check::nll::region_infer::values::RegionValueElements;
use crate::dataflow::indexes::BorrowIndex;
use crate::dataflow::move_paths::{MoveData, MovePathIndex};
use crate::dataflow::move_paths::{InitLocation, MoveData, MovePathIndex, InitKind};
use crate::dataflow::FlowAtLocation;
use crate::dataflow::MaybeInitializedPlaces;
use crate::transform::MirSource;
use crate::borrow_check::Upvar;
use rustc::hir::def_id::DefId;
use rustc::infer::InferCtxt;
use rustc::mir::{ClosureOutlivesSubject, ClosureRegionRequirements, Local, Body, Promoted};
use rustc::mir::{ClosureOutlivesSubject, ClosureRegionRequirements, Local, Location, Body, LocalKind, BasicBlock, Promoted};
use rustc::ty::{self, RegionKind, RegionVid};
use rustc_data_structures::indexed_vec::IndexVec;
use rustc_errors::Diagnostic;
Expand Down Expand Up @@ -69,6 +69,61 @@ pub(in crate::borrow_check) fn replace_regions_in_mir<'cx, 'tcx>(
universal_regions
}


// This function populates an AllFacts instance with base facts related to
// MovePaths and needed for the move analysis.
fn populate_polonius_move_facts(all_facts: &mut AllFacts, move_data: &MoveData<'_>, location_table: &LocationTable, body: &Body<'_>) {
all_facts.var_starts_path.extend(move_data.rev_lookup.iter_locals_enumerated().map(|(v, &m)| (v, m)));

for (idx, move_path) in move_data.move_paths.iter_enumerated() {
all_facts.parent.extend(move_path.parents(&move_data.move_paths).iter().map(|&parent| (parent, idx)));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

as noted in the polonius PR, ultimately I think we should leave the transitive computation here to polonius. But I guess I don't wnat to do that in this PR.

}

// initialized_at
for init in move_data.inits.iter() {

match init.location {
InitLocation::Statement(location) => {
let block_data = &body[location.block];
let is_terminator = location.statement_index == block_data.statements.len();

if is_terminator && init.kind == InitKind::NonPanicPathOnly {
// We are at the terminator of an init that has a panic path,
// and where the init should not happen on panic

for &successor in block_data.terminator().successors() {
if body[successor].is_cleanup {
continue;
}

// The initialization happened in (or rather, when arriving at)
// the successors, but not in the unwind block.
let first_statement = Location { block: successor, statement_index: 0};
all_facts.initialized_at.push((init.path, location_table.start_index(first_statement)));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Interesting quirk. I guess this makes sense.

Copy link
Contributor Author

@amandasystems amandasystems Sep 4, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's just say I scratched my head for quite some time before I figured out why this happened. The results were completely wrong otherwise. If you are curious about the details, there's a ton of logs on Zulip where I figure this out in the initialization topic.

}

} else {
// In all other cases, the initialization just happens at the
// midpoint, like any other effect.
all_facts.initialized_at.push((init.path, location_table.mid_index(location)));
}
},
// Arguments are initialized on function entry
InitLocation::Argument(local) => {
assert!(body.local_kind(local) == LocalKind::Arg);
let fn_entry = Location {block: BasicBlock::from_u32(0u32), statement_index: 0 };
all_facts.initialized_at.push((init.path, location_table.start_index(fn_entry)));

}
}
}


// moved_out_at
// deinitialisation is assumed to always happen!
all_facts.moved_out_at.extend(move_data.moves.iter().map(|mo| (mo.path, location_table.mid_index(mo.source))));
}

/// Computes the (non-lexical) regions from the input MIR.
///
/// This may result in errors being reported.
Expand Down Expand Up @@ -123,6 +178,7 @@ pub(in crate::borrow_check) fn compute_regions<'cx, 'tcx>(
all_facts
.universal_region
.extend(universal_regions.universal_regions());
populate_polonius_move_facts(all_facts, move_data, location_table, body);
}

// Create the region inference context, taking ownership of the
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ struct LivenessPointFactsExtractor<'me> {
var_defined: &'me mut VarPointRelations,
var_used: &'me mut VarPointRelations,
location_table: &'me LocationTable,
var_drop_used: &'me mut VarPointRelations,
}

// A Visitor to walk through the MIR and extract point-wise facts
Expand All @@ -30,15 +31,20 @@ impl LivenessPointFactsExtractor<'_> {
debug!("LivenessFactsExtractor::insert_use()");
self.var_used.push((local, self.location_to_index(location)));
}

fn insert_drop_use(&mut self, local: Local, location: Location) {
debug!("LivenessFactsExtractor::insert_drop_use()");
self.var_drop_used.push((local, self.location_to_index(location)));
}
}

impl Visitor<'tcx> for LivenessPointFactsExtractor<'_> {
fn visit_local(&mut self, &local: &Local, context: PlaceContext, location: Location) {
match categorize(context) {
Some(DefUse::Def) => self.insert_def(local, location),
Some(DefUse::Use) => self.insert_use(local, location),
Some(DefUse::Drop) => self.insert_drop_use(local, location),
_ => (),
// NOTE: Drop handling is now done in trace()
}
}
}
Expand All @@ -65,6 +71,7 @@ pub(super) fn populate_var_liveness_facts(
LivenessPointFactsExtractor {
var_defined: &mut facts.var_defined,
var_used: &mut facts.var_used,
var_drop_used: &mut facts.var_drop_used,
location_table,
}
.visit_body(mir);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -273,11 +273,6 @@ impl LivenessResults<'me, 'typeck, 'flow, 'tcx> {
debug_assert_eq!(self.cx.body.terminator_loc(location.block), location,);

if self.cx.initialized_at_terminator(location.block, mpi) {
// FIXME: this analysis (the initialization tracking) should be
// done in Polonius, but isn't yet.
if let Some(facts) = self.cx.typeck.borrowck_context.all_facts {
facts.var_drop_used.push((local, self.cx.location_table.mid_index(location)));
}
if self.drop_live_at.insert(drop_point) {
self.drop_locations.push(location);
self.stack.push(drop_point);
Expand Down
9 changes: 8 additions & 1 deletion src/librustc_mir/dataflow/move_paths/mod.rs
Original file line number Diff line number Diff line change
@@ -1,9 +1,10 @@
use rustc::ty::{Ty, TyCtxt};
use rustc::mir::*;
use rustc::util::nodemap::FxHashMap;
use rustc_data_structures::indexed_vec::{Idx, IndexVec};
use rustc_data_structures::indexed_vec::{Idx, IndexVec, Enumerated};
use smallvec::SmallVec;
use syntax_pos::{Span};
use core::slice::Iter;

use std::fmt;
use std::ops::{Index, IndexMut};
Expand Down Expand Up @@ -262,6 +263,12 @@ impl MovePathLookup {
pub fn find_local(&self, local: Local) -> MovePathIndex {
self.locals[local]
}

/// An enumerated iterator of `local`s and their associated
/// `MovePathIndex`es.
pub fn iter_locals_enumerated(&self) -> Enumerated<Local, Iter<'_, MovePathIndex>> {
self.locals.iter_enumerated()
}
}

#[derive(Debug)]
Expand Down