Skip to content
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

add TerminatorKind::FalseEdges and use it in matches #45384

Merged
Merged
Show file tree
Hide file tree
Changes from 5 commits
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
9 changes: 8 additions & 1 deletion src/librustc/ich/impls_mir.rs
Original file line number Diff line number Diff line change
Expand Up @@ -62,7 +62,8 @@ for mir::Terminator<'gcx> {
mir::TerminatorKind::Drop { .. } |
mir::TerminatorKind::DropAndReplace { .. } |
mir::TerminatorKind::Yield { .. } |
mir::TerminatorKind::Call { .. } => false,
mir::TerminatorKind::Call { .. } |
mir::TerminatorKind::FalseEdges { .. } => false,
};

if hash_spans_unconditionally {
Expand Down Expand Up @@ -210,6 +211,12 @@ for mir::TerminatorKind<'gcx> {
target.hash_stable(hcx, hasher);
cleanup.hash_stable(hcx, hasher);
}
mir::TerminatorKind::FalseEdges { ref real_target, ref imaginary_targets } => {
real_target.hash_stable(hcx, hasher);
for target in imaginary_targets {
target.hash_stable(hcx, hasher);
}
}
}
}
}
Expand Down
32 changes: 28 additions & 4 deletions src/librustc/mir/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -682,6 +682,11 @@ pub enum TerminatorKind<'tcx> {

/// Indicates the end of the dropping of a generator
GeneratorDrop,

FalseEdges {
real_target: BasicBlock,
imaginary_targets: Vec<BasicBlock>
},
}

impl<'tcx> Terminator<'tcx> {
Expand Down Expand Up @@ -731,6 +736,11 @@ impl<'tcx> TerminatorKind<'tcx> {
}
Assert { target, cleanup: Some(unwind), .. } => vec![target, unwind].into_cow(),
Assert { ref target, .. } => slice::ref_slice(target).into_cow(),
FalseEdges { ref real_target, ref imaginary_targets } => {
let mut s = vec![*real_target];
s.extend_from_slice(imaginary_targets);
s.into_cow()
}
}
}

Expand All @@ -757,7 +767,12 @@ impl<'tcx> TerminatorKind<'tcx> {
vec![target]
}
Assert { ref mut target, cleanup: Some(ref mut unwind), .. } => vec![target, unwind],
Assert { ref mut target, .. } => vec![target]
Assert { ref mut target, .. } => vec![target],
FalseEdges { ref mut real_target, ref mut imaginary_targets } => {
let mut s = vec![real_target];
s.extend(imaginary_targets.iter_mut());
s
}
}
}
}
Expand Down Expand Up @@ -874,7 +889,8 @@ impl<'tcx> TerminatorKind<'tcx> {
}

write!(fmt, ")")
}
},
FalseEdges { .. } => write!(fmt, "falseEdges")
}
}

Expand Down Expand Up @@ -910,7 +926,12 @@ impl<'tcx> TerminatorKind<'tcx> {
}
Assert { cleanup: None, .. } => vec!["".into()],
Assert { .. } =>
vec!["success".into_cow(), "unwind".into_cow()]
vec!["success".into_cow(), "unwind".into_cow()],
FalseEdges { ref imaginary_targets, .. } => {
let mut l = vec!["real".into()];
l.resize(imaginary_targets.len() + 1, "imaginary".into());
l
}
}
}
}
Expand Down Expand Up @@ -1878,6 +1899,8 @@ impl<'tcx> TypeFoldable<'tcx> for Terminator<'tcx> {
Resume => Resume,
Return => Return,
Unreachable => Unreachable,
FalseEdges { real_target, ref imaginary_targets } =>
FalseEdges { real_target, imaginary_targets: imaginary_targets.clone() }
};
Terminator {
source_info: self.source_info,
Expand Down Expand Up @@ -1917,7 +1940,8 @@ impl<'tcx> TypeFoldable<'tcx> for Terminator<'tcx> {
Resume |
Return |
GeneratorDrop |
Unreachable => false
Unreachable |
FalseEdges { .. } => false
}
}
}
Expand Down
7 changes: 7 additions & 0 deletions src/librustc/mir/visit.rs
Original file line number Diff line number Diff line change
Expand Up @@ -486,8 +486,15 @@ macro_rules! make_mir_visitor {
self.visit_operand(value, source_location);
self.visit_branch(block, resume);
drop.map(|t| self.visit_branch(block, t));

}

TerminatorKind::FalseEdges { real_target, ref imaginary_targets } => {
self.visit_branch(block, real_target);
for target in imaginary_targets {
self.visit_branch(block, *target);
}
}
}
}

Expand Down
3 changes: 2 additions & 1 deletion src/librustc_mir/borrow_check.rs
Original file line number Diff line number Diff line change
Expand Up @@ -364,7 +364,8 @@ impl<'c, 'b, 'a: 'b+'c, 'gcx, 'tcx: 'a> DataflowResultsConsumer<'b, 'tcx>
TerminatorKind::Resume |
TerminatorKind::Return |
TerminatorKind::GeneratorDrop |
TerminatorKind::Unreachable => {
TerminatorKind::Unreachable |
TerminatorKind::FalseEdges { .. } => {
// no data used, thus irrelevant to borrowck
}
}
Expand Down
118 changes: 83 additions & 35 deletions src/librustc_mir/build/matches/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@ use rustc::mir::*;
use rustc::hir;
use hair::*;
use syntax::ast::{Name, NodeId};
use syntax_pos::{DUMMY_SP, Span};
use syntax_pos::Span;

// helper functions, broken out by category:
mod simplify;
Expand Down Expand Up @@ -54,29 +54,43 @@ impl<'a, 'gcx, 'tcx> Builder<'a, 'gcx, 'tcx> {
(body, scope.unwrap_or(self.visibility_scope))
}).collect();

// create binding start block for link them by false edges
let candidate_count = arms.iter().fold(0, |ac, c| ac + c.patterns.len());
let pre_binding_blocks: Vec<_> = (0..candidate_count + 1)
.map(|_| self.cfg.start_new_block()).collect();

// assemble a list of candidates: there is one candidate per
// pattern, which means there may be more than one candidate
// *per arm*. These candidates are kept sorted such that the
// highest priority candidate comes first in the list.
// (i.e. same order as in source)

let candidates: Vec<_> =
arms.iter()
.enumerate()
.flat_map(|(arm_index, arm)| {
arm.patterns.iter()
.map(move |pat| (arm_index, pat, arm.guard.clone()))
})
.map(|(arm_index, pattern, guard)| {
.zip(pre_binding_blocks.iter().zip(pre_binding_blocks.iter().skip(1)))
.map(|((arm_index, pattern, guard),
(pre_binding_block, next_candidate_pre_binding_block))| {
Candidate {
span: pattern.span,
match_pairs: vec![MatchPair::new(discriminant_lvalue.clone(), pattern)],
bindings: vec![],
guard,
arm_index,
pre_binding_block: *pre_binding_block,
next_candidate_pre_binding_block: *next_candidate_pre_binding_block,
}
})
.collect();

let outer_source_info = self.source_info(span);
self.cfg.terminate(*pre_binding_blocks.last().unwrap(),
outer_source_info, TerminatorKind::Unreachable);

// this will generate code to test discriminant_lvalue and
// branch to the appropriate arm block
let otherwise = self.match_candidates(span, &mut arm_blocks, candidates, block);
Expand Down Expand Up @@ -148,7 +162,11 @@ impl<'a, 'gcx, 'tcx> Builder<'a, 'gcx, 'tcx> {
match_pairs: vec![MatchPair::new(initializer.clone(), &irrefutable_pat)],
bindings: vec![],
guard: None,
arm_index: 0, // since we don't call `match_candidates`, this field is unused

// since we don't call `match_candidates`, next fields is unused
arm_index: 0,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't like it, but add Option and unwraps it's worse, may be create sub struct for simplify

pre_binding_block: block,
next_candidate_pre_binding_block: block
};

// Simplify the candidate. Since the pattern is irrefutable, this should
Expand Down Expand Up @@ -278,6 +296,10 @@ pub struct Candidate<'pat, 'tcx:'pat> {

// ...and then we branch to arm with this index.
arm_index: usize,

// ...and the blocks for add false edges between candidates
pre_binding_block: BasicBlock,
next_candidate_pre_binding_block: BasicBlock,
}

#[derive(Clone, Debug)]
Expand Down Expand Up @@ -398,17 +420,43 @@ impl<'a, 'gcx, 'tcx> Builder<'a, 'gcx, 'tcx> {
candidates.iter().take_while(|c| c.match_pairs.is_empty()).count();
debug!("match_candidates: {:?} candidates fully matched", fully_matched);
let mut unmatched_candidates = candidates.split_off(fully_matched);
for (index, candidate) in candidates.into_iter().enumerate() {

let fully_matched_with_guard =
candidates.iter().take_while(|c| c.guard.is_some()).count();

let unreachable_candidates = if fully_matched_with_guard + 1 < candidates.len() {
candidates.split_off(fully_matched_with_guard + 1)
} else {
vec![]
};

for candidate in candidates {
// If so, apply any bindings, test the guard (if any), and
// branch to the arm.
let is_last = index == fully_matched - 1;
if let Some(b) = self.bind_and_guard_matched_candidate(block, arm_blocks,
candidate, is_last) {
if let Some(b) = self.bind_and_guard_matched_candidate(block, arm_blocks, candidate) {
block = b;
} else {
// if None is returned, then any remaining candidates
// are unreachable (at least not through this path).
return vec![];
// Link them with false edges.
debug!("match_candidates: add false edges for unreachable {:?} and unmatched {:?}",
unreachable_candidates, unmatched_candidates);
for candidate in unreachable_candidates {
Copy link
Contributor

@arielb1 arielb1 Oct 24, 2017

Choose a reason for hiding this comment

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

can't you iterate over unreachable_candidates.zip(unmatched_candidates) in the for-loop here? These are both unreachable candidates, and should be treated similarly. I think the current way will cause an infinite loop if you have a partial candidate after a full candidate, e.g.:

match Some(42) {
    x => {},
    Some(y) if maybe => {},
    z => {}
}

In any case, you should add a mir opt test for that (to src/test/mir-opt) to check that the generated MIR is sane

let source_info = self.source_info(candidate.span);
let target = self.cfg.start_new_block();
if let Some(otherwise) = self.bind_and_guard_matched_candidate(target,
arm_blocks,
candidate) {
self.cfg.terminate(otherwise, source_info, TerminatorKind::Unreachable);
}
}

if unmatched_candidates.is_empty() {
return vec![]
} else {
let target = self.cfg.start_new_block();
return self.match_candidates(span, arm_blocks, unmatched_candidates, target);
}
}
}

Expand All @@ -423,9 +471,7 @@ impl<'a, 'gcx, 'tcx> Builder<'a, 'gcx, 'tcx> {
self.test_candidates(span, arm_blocks, &unmatched_candidates, block);

// If the target candidates were exhaustive, then we are done.
if otherwise.is_empty() {
return vec![];
}
// But for borrowck continue build decision tree.

// If all candidates were sorted into `target_candidates` somewhere, then
// the initial set was inexhaustive.
Expand Down Expand Up @@ -666,48 +712,50 @@ impl<'a, 'gcx, 'tcx> Builder<'a, 'gcx, 'tcx> {
fn bind_and_guard_matched_candidate<'pat>(&mut self,
mut block: BasicBlock,
arm_blocks: &mut ArmBlocks,
candidate: Candidate<'pat, 'tcx>,
is_last_arm: bool)
candidate: Candidate<'pat, 'tcx>)
-> Option<BasicBlock> {
debug!("bind_and_guard_matched_candidate(block={:?}, candidate={:?})",
block, candidate);

debug_assert!(candidate.match_pairs.is_empty());

self.bind_matched_candidate(block, candidate.bindings);

let arm_block = arm_blocks.blocks[candidate.arm_index];
let candidate_source_info = self.source_info(candidate.span);

self.cfg.terminate(block, candidate_source_info,
TerminatorKind::Goto { target: candidate.pre_binding_block });

block = self.cfg.start_new_block();
self.cfg.terminate(candidate.pre_binding_block, candidate_source_info,
TerminatorKind::FalseEdges {
real_target: block,
imaginary_targets:
vec![candidate.next_candidate_pre_binding_block]});

self.bind_matched_candidate(block, candidate.bindings);

if let Some(guard) = candidate.guard {
// the block to branch to if the guard fails; if there is no
// guard, this block is simply unreachable
let guard = self.hir.mirror(guard);
let source_info = self.source_info(guard.span);
let cond = unpack!(block = self.as_local_operand(block, guard));
let otherwise = self.cfg.start_new_block();

let false_edge_block = self.cfg.start_new_block();
self.cfg.terminate(block, source_info,
TerminatorKind::if_(self.hir.tcx(), cond, arm_block, otherwise));
Some(otherwise)
} else if !is_last_arm {
// Add always true guard in case of more than one arm
// it creates false edges and allow MIR borrowck detects errors
// FIXME(#45184) -- permit "false edges"
let source_info = self.source_info(candidate.span);
let true_expr = Expr {
temp_lifetime: None,
ty: self.hir.tcx().types.bool,
span: DUMMY_SP,
kind: ExprKind::Literal{literal: self.hir.true_literal()},
};
let cond = unpack!(block = self.as_local_operand(block, true_expr));
TerminatorKind::if_(self.hir.tcx(), cond, arm_block,
false_edge_block));

let otherwise = self.cfg.start_new_block();
self.cfg.terminate(block, source_info,
TerminatorKind::if_(self.hir.tcx(), cond, arm_block, otherwise));
self.cfg.terminate(false_edge_block, source_info,
TerminatorKind::FalseEdges {
real_target: otherwise,
imaginary_targets:
vec![candidate.next_candidate_pre_binding_block] });
Some(otherwise)
Copy link
Contributor

@arielb1 arielb1 Oct 24, 2017

Choose a reason for hiding this comment

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

You also want to add a false edge (pointing to the imaginary target block) right after the guard - that would fix #31287 in the future-proof case (you can also add a test for both the issue and the future-proof to see that MIR borrowck correctly emits an error):

let my_str = "hello".to_owned();
match Some(42) {
    Some(_) if { drop(my_str); false } => {} 
    Some(_) => {}
    None => {
        println!("{}", my_str); //~ ERROR [mir] use of moved value
    }
}

I mean, technically you could just have an edge after the guard, but that would be a hazard if we allow guard to initialize variables:

let my_str;
match Some(42) {
    _ if {
        my_str = String::new(); //~ ERROR cannot assign in a pattern guard
        // ^ this is an error today, but should be removed in the future
        false
    } => {}
    _ => {
        println!("{}", my_str); //~ ERROR [mir] use of possibly-uninitialized value
    }
}

} else {
let source_info = self.source_info(candidate.span);
self.cfg.terminate(block, source_info,
TerminatorKind::Goto { target: arm_block });
self.cfg.terminate(block, candidate_source_info,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

may be we don't need it

Copy link
Contributor

Choose a reason for hiding this comment

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

We don't - we only need 1 false edge.

TerminatorKind::Goto { target: arm_block });
None
}
}
Expand Down
4 changes: 4 additions & 0 deletions src/librustc_mir/build/matches/test.rs
Original file line number Diff line number Diff line change
Expand Up @@ -598,6 +598,8 @@ impl<'a, 'gcx, 'tcx> Builder<'a, 'gcx, 'tcx> {
bindings: candidate.bindings.clone(),
guard: candidate.guard.clone(),
arm_index: candidate.arm_index,
pre_binding_block: candidate.pre_binding_block,
next_candidate_pre_binding_block: candidate.next_candidate_pre_binding_block,
}
}

Expand Down Expand Up @@ -659,6 +661,8 @@ impl<'a, 'gcx, 'tcx> Builder<'a, 'gcx, 'tcx> {
bindings: candidate.bindings.clone(),
guard: candidate.guard.clone(),
arm_index: candidate.arm_index,
pre_binding_block: candidate.pre_binding_block,
next_candidate_pre_binding_block: candidate.next_candidate_pre_binding_block,
}
}

Expand Down
6 changes: 6 additions & 0 deletions src/librustc_mir/dataflow/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -721,6 +721,12 @@ impl<'a, 'tcx: 'a, D> DataflowAnalysis<'a, 'tcx, D> where D: BitDenotation
self.propagate_bits_into_entry_set_for(in_out, changed, dest_bb);
}
}
mir::TerminatorKind::FalseEdges { ref real_target, ref imaginary_targets } => {
self.propagate_bits_into_entry_set_for(in_out, changed, real_target);
for target in imaginary_targets {
self.propagate_bits_into_entry_set_for(in_out, changed, target);
}
}
}
}

Expand Down
1 change: 1 addition & 0 deletions src/librustc_mir/dataflow/move_paths/builder.rs
Original file line number Diff line number Diff line change
Expand Up @@ -305,6 +305,7 @@ impl<'b, 'a, 'gcx, 'tcx> Gatherer<'b, 'a, 'gcx, 'tcx> {
TerminatorKind::Goto { target: _ } |
TerminatorKind::Resume |
TerminatorKind::GeneratorDrop |
TerminatorKind::FalseEdges { .. } |
TerminatorKind::Unreachable => { }

TerminatorKind::Return => {
Expand Down
3 changes: 2 additions & 1 deletion src/librustc_mir/transform/check_unsafety.rs
Original file line number Diff line number Diff line change
Expand Up @@ -73,7 +73,8 @@ impl<'a, 'tcx> Visitor<'tcx> for UnsafetyChecker<'a, 'tcx> {
TerminatorKind::GeneratorDrop |
TerminatorKind::Resume |
TerminatorKind::Return |
TerminatorKind::Unreachable => {
TerminatorKind::Unreachable |
TerminatorKind::FalseEdges { .. } => {
// safe (at least as emitted during MIR construction)
}

Expand Down
Loading