add TerminatorKind::FalseEdges and use it in matches #45384
Conversation
|
r? @arielb1 (rust_highfive has picked a reviewer for you, use r? to override) |
| //FIXME need to debug MIR borrowck | ||
| Foo::A(x) => x //[ast]~ ERROR [E0503] | ||
| //[mir]~^ ERROR (Ast) [E0503] | ||
| //[mir]~| ERROR (Mir) [E0381] |
| @@ -246,6 +246,7 @@ fn main() { | |||
| //[mir]~| ERROR cannot borrow `e.0` as immutable because it is also borrowed as mutable (Mir) | |||
| //[mir]~| ERROR cannot use `e` because it was mutably borrowed (Mir) | |||
| println!("e.ax: {:?}", ax), | |||
| //[mir]~^ ERROR borrow of possibly uninitialized variable: `ax` (Mir) | |||
kennytm
added
the
S-waiting-on-author
|
About first comment, there is no appropriate bind block generated for unreachable arms, so MIR borrowck detects uninitialized variable and doesn't detect reborrow. |
|
the second note is a bug with nested match. |
src/librustc/ich/impls_mir.rs
Outdated
| @@ -210,6 +211,9 @@ for mir::TerminatorKind<'gcx> { | |||
| target.hash_stable(hcx, hasher); | |||
| cleanup.hash_stable(hcx, hasher); | |||
| } | |||
| mir::TerminatorKind::FalseEdges { ref real_target, .. } => { | |||
There was a problem hiding this comment.
please hash the imaginary targets too
kennytm
added
S-waiting-on-review
|
☔ The latest upstream changes (presumably #45381) made this pull request unmergeable. Please resolve the merge conflicts. |
mikhail-m1
force-pushed
the
mir_add_false_edges_terminator_kind
branch
from
3f0be75
to
29c15d2
Compare
src/librustc/mir/visit.rs
Outdated
| } | ||
|
|
||
| TerminatorKind::FalseEdges { real_target, .. } => { |
There was a problem hiding this comment.
I think you should visit the imaginary edges too, so that code that uses this to compute the CFG gets the "correct" approximation.
I don't think anyone actually listens on visit_branch, but it's still the right thing.
kennytm
added
S-waiting-on-author
src/librustc_mir/transform/inline.rs
Outdated
| @@ -720,6 +720,9 @@ impl<'a, 'tcx> MutVisitor<'tcx> for Integrator<'a, 'tcx> { | |||
| } | |||
| } | |||
| TerminatorKind::Unreachable => { } | |||
| TerminatorKind::FalseEdges { ref mut real_target, .. } => { | |||
| *real_target = self.update_target(*real_target); | |||
There was a problem hiding this comment.
I think you either need to have a bug! here or to also update the imaginary targets.
| @@ -303,7 +303,8 @@ impl<'a, 'tcx> Qualifier<'a, 'tcx, 'tcx> { | |||
| TerminatorKind::Goto { target } | | |||
| TerminatorKind::Drop { target, .. } | | |||
| TerminatorKind::Assert { target, .. } | | |||
| TerminatorKind::Call { destination: Some((_, target)), .. } => { | |||
| TerminatorKind::Call { destination: Some((_, target)), .. } | | |||
| TerminatorKind::FalseEdges { real_target: target, .. } => { | |||
There was a problem hiding this comment.
This should be handled like an if false aka a SwitchInt. I believe we'll get rid of this soon for MIRI, but try to be consistent until then.
| @@ -685,6 +686,8 @@ impl<'a, 'gcx, 'tcx> TypeChecker<'a, 'gcx, 'tcx> { | |||
| self.assert_iscleanup(mir, block, cleanup, true); | |||
| } | |||
| } | |||
| TerminatorKind::FalseEdges { real_target, .. } => | |||
There was a problem hiding this comment.
Also assert_iscleanup for the imaginary targets?
| @@ -678,6 +715,23 @@ impl<'a, 'gcx, 'tcx> Builder<'a, 'gcx, 'tcx> { | |||
|
|
|||
| let arm_block = arm_blocks.blocks[candidate.arm_index]; | |||
|
|
|||
| // link previously generated false edge to the enter block | |||
| let dummy_source_info = self.source_info(DUMMY_SP); | |||
There was a problem hiding this comment.
please do not use DUMMY_SP but instead use a real span.
| // Link them with false edges. | ||
| debug!("match_candidates: add false edges for unreachable {:?} and unmatched {:?}", | ||
| unreachable_candidates, unmatched_candidates); | ||
| let dummy_source_info = self.source_info(DUMMY_SP); |
There was a problem hiding this comment.
please use a real source info
| @@ -377,7 +385,8 @@ impl<'a, 'gcx, 'tcx> Builder<'a, 'gcx, 'tcx> { | |||
| span: Span, | |||
| arm_blocks: &mut ArmBlocks, | |||
| mut candidates: Vec<Candidate<'pat, 'tcx>>, | |||
| mut block: BasicBlock) | |||
| mut block: BasicBlock, | |||
| opt_imaginary_target_block: &mut Option<BasicBlock>) | |||
There was a problem hiding this comment.
You should modify one of the comments somewhere explaining the purpose of the opt_imaginary_target_block to chain things together
| debug!("match_candidates: add false edges for unreachable {:?} and unmatched {:?}", | ||
| unreachable_candidates, unmatched_candidates); | ||
| let dummy_source_info = self.source_info(DUMMY_SP); | ||
| for candidate in unreachable_candidates { |
There was a problem hiding this comment.
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
| @@ -688,26 +742,10 @@ impl<'a, 'gcx, 'tcx> Builder<'a, 'gcx, 'tcx> { | |||
| self.cfg.terminate(block, source_info, | |||
| TerminatorKind::if_(self.hir.tcx(), cond, arm_block, otherwise)); | |||
| Some(otherwise) | |||
There was a problem hiding this comment.
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
}
}
| @@ -112,6 +115,11 @@ impl<'a, 'gcx, 'tcx> Builder<'a, 'gcx, 'tcx> { | |||
| } | |||
| self.visibility_scope = outer_source_info.scope; | |||
|
|
|||
| if let Some(imaginary_target_block) = opt_imaginary_target_block.take() { | |||
There was a problem hiding this comment.
No take needed here - you are taking it by value.
| @@ -678,6 +715,23 @@ impl<'a, 'gcx, 'tcx> Builder<'a, 'gcx, 'tcx> { | |||
|
|
|||
| let arm_block = arm_blocks.blocks[candidate.arm_index]; | |||
|
|
|||
| // link previously generated false edge to the enter block | |||
| let dummy_source_info = self.source_info(DUMMY_SP); | |||
| if let Some(imaginary_target) = opt_imaginary_target_block.take() { | |||
There was a problem hiding this comment.
I think it would be cleaner to use mem::replace(opt_imaginary_target_block, Some(imaginary_target_block)) here
|
So I like the approach here, but you need to generate the correct double chain to cover all the cases, and there are a few other nits. |
|
two notes remain unfixed, I'd like discuss it by gitter |
mikhail-m1
force-pushed
the
mir_add_false_edges_terminator_kind
branch
from
b895fcf
to
d4373e2
Compare
|
updated, now implementation is close to to #45184 (comment) except that false edges is lead to binding. Connection to start of guard produces uninitialized variable error in next sample |
kennytm
added
the
S-waiting-on-review
|
💔 Test failed - status-travis |
|
in arm: |
|
⌛ Testing commit 7d87054 with merge 07db96a7c8ef870e92f902930e3731cc5b895704... |
|
💔 Test failed - status-travis |
|
Job Cancelled |
|
wut. @bors retry |
|
Err looks like mirror.centos.org is down or something. |
|
⌛ Testing commit 7d87054 with merge 22af4957aa66dd218b5e56fb883c196bd97426d9... |
|
💔 Test failed - status-travis |
|
@bors retry |
|
⌛ Testing commit 7d87054 with merge 65cf0be341f0f94b6aa49d4d6ec6c6ab293743a7... |
|
💔 Test failed - status-appveyor |
|
now |
|
@bors retry We've seen the same spurious failure before... @nikomatsakis |
|
☀️ Test successful - status-appveyor, status-travis |
…crichton Miscellaneous changes for CI, Docker and compiletest. This PR contains 7 independent commits that improves interaction with CI, Docker and compiletest. 1. a4e5c91 — Forces a newline every 100 dots when testing in quiet mode. Prevents spurious timeouts when abusing the CI to test Android jobs. 2. 1b5aaf2 — Use vault.centos.org for dist-powerpc64le-linux, see rust-lang#45744. 3. 33400fb — Modify `src/ci/docker/run.sh` so that the docker images can be run from Docker Toolbox for Windows on Windows 7. I haven't checked the behavior of the newer Docker for Windows on Windows 10. Also, "can run" does not mean all the test can pass successfully (the UDP tests failed last time I checked) 4. d517668 — Don't emit a real warning the linker segfault, which affects UI tests like rust-lang#45489 (comment). Log it instead. 5. 51e2247 — During run-pass, trim the output if stdout/stderr exceeds 416 KB (top 160 KB + bottom 256 KB). This is an attempt to avoid spurious failures like rust-lang#45384 (comment) 6. 9cfdaba — Force `gem update --system` before deploy. This is an attempt to prevent spurious error rust-lang#44159. 7. eee10cc — Tries to print the crash log on macOS on failure. This is an attempt to debug rust-lang#45230.
Miscellaneous changes for CI, Docker and compiletest. This PR contains 7 independent commits that improves interaction with CI, Docker and compiletest. 1. a4e5c91cb8 — Forces a newline every 100 dots when testing in quiet mode. Prevents spurious timeouts when abusing the CI to test Android jobs. 2. 1b5aaf22e8 — Use vault.centos.org for dist-powerpc64le-linux, see #45744. 3. 33400fbbcd — Modify `src/ci/docker/run.sh` so that the docker images can be run from Docker Toolbox for Windows on Windows 7. I haven't checked the behavior of the newer Docker for Windows on Windows 10. Also, "can run" does not mean all the test can pass successfully (the UDP tests failed last time I checked) 4. d517668a08 — Don't emit a real warning the linker segfault, which affects UI tests like rust-lang/rust#45489 (comment). Log it instead. 5. 51e2247948 — During run-pass, trim the output if stdout/stderr exceeds 416 KB (top 160 KB + bottom 256 KB). This is an attempt to avoid spurious failures like rust-lang/rust#45384 (comment) 6. 9cfdabaf3c — Force `gem update --system` before deploy. This is an attempt to prevent spurious error #44159. 7. eee10cc482 — Tries to print the crash log on macOS on failure. This is an attempt to debug #45230.
impl #45184 and fixes #45043 right way.
False edges unexpectedly affects uninitialized variables analysis in MIR borrowck.