Skip to content

Commit

Permalink
Fix debugger stepping behavior around match expressions
Browse files Browse the repository at this point in the history
Previously, we would set up the source lines for `match` expressions so
that the code generated to perform the test of the scrutinee was matched
to the line of the arm that required the test and then jump from the arm
block to the "next" block was matched to all of the lines in the `match`
expression.

While that makes sense, it has the side effect of causing strange
stepping behavior in debuggers.

I've changed the source information so that all of the generated tests
are sourced to `match {scrutinee}` and the jumps are sourced to the last
line of the block they are inside. This resolves the weird stepping
behavior in all debuggers and resolves some instances of "ambiguous
symbol" errors in WinDbg preventing the user from setting breakpoints at
`match` expressions.
  • Loading branch information
wesleywiser committed Aug 25, 2021
1 parent a992a11 commit 0a42dfc
Show file tree
Hide file tree
Showing 92 changed files with 533 additions and 482 deletions.
63 changes: 54 additions & 9 deletions compiler/rustc_mir_build/src/build/matches/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@ use rustc_middle::mir::*;
use rustc_middle::thir::{self, *};
use rustc_middle::ty::{self, CanonicalUserTypeAnnotation, Ty};
use rustc_span::symbol::Symbol;
use rustc_span::Span;
use rustc_span::{BytePos, Pos, Span};
use rustc_target::abi::VariantIdx;
use smallvec::{smallvec, SmallVec};

Expand Down Expand Up @@ -143,8 +143,15 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {
let mut candidates =
arm_candidates.iter_mut().map(|(_, candidate)| candidate).collect::<Vec<_>>();

let fake_borrow_temps =
self.lower_match_tree(block, scrutinee_span, match_has_guard, &mut candidates);
let match_start_span = span.shrink_to_lo().to(scrutinee.span);

let fake_borrow_temps = self.lower_match_tree(
block,
scrutinee_span,
match_start_span,
match_has_guard,
&mut candidates,
);

self.lower_match_arms(
destination,
Expand Down Expand Up @@ -224,6 +231,7 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {
&mut self,
block: BasicBlock,
scrutinee_span: Span,
match_start_span: Span,
match_has_guard: bool,
candidates: &mut [&mut Candidate<'pat, 'tcx>],
) -> Vec<(Place<'tcx>, Local)> {
Expand All @@ -236,7 +244,14 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {

// This will generate code to test scrutinee_place and
// branch to the appropriate arm block
self.match_candidates(scrutinee_span, block, &mut otherwise, candidates, &mut fake_borrows);
self.match_candidates(
match_start_span,
scrutinee_span,
block,
&mut otherwise,
candidates,
&mut fake_borrows,
);

if let Some(otherwise_block) = otherwise {
// See the doc comment on `match_candidates` for why we may have an
Expand Down Expand Up @@ -339,8 +354,14 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {
// all the arm blocks will rejoin here
let end_block = self.cfg.start_new_block();

let end_brace = self.source_info(
outer_source_info.span.with_lo(outer_source_info.span.hi() - BytePos::from_usize(1)),
);
for arm_block in arm_end_blocks {
self.cfg.goto(unpack!(arm_block), outer_source_info, end_block);
let block = &self.cfg.basic_blocks[arm_block.0];
let last_location = block.statements.last().map(|s| s.source_info);

self.cfg.goto(unpack!(arm_block), last_location.unwrap_or(end_brace), end_block);
}

self.source_scope = outer_source_info.scope;
Expand Down Expand Up @@ -533,8 +554,13 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {
set_match_place: bool,
) -> BlockAnd<()> {
let mut candidate = Candidate::new(initializer.clone(), &irrefutable_pat, false);
let fake_borrow_temps =
self.lower_match_tree(block, irrefutable_pat.span, false, &mut [&mut candidate]);
let fake_borrow_temps = self.lower_match_tree(
block,
irrefutable_pat.span,
irrefutable_pat.span,
false,
&mut [&mut candidate],
);
// For matches and function arguments, the place that is being matched
// can be set when creating the variables. But the place for
// let PATTERN = ... might not even exist until we do the assignment.
Expand Down Expand Up @@ -993,6 +1019,7 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {
fn match_candidates<'pat>(
&mut self,
span: Span,
scrutinee_span: Span,
start_block: BasicBlock,
otherwise_block: &mut Option<BasicBlock>,
candidates: &mut [&mut Candidate<'pat, 'tcx>],
Expand Down Expand Up @@ -1022,6 +1049,7 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {
}
self.match_simplified_candidates(
span,
scrutinee_span,
start_block,
otherwise_block,
&mut *new_candidates,
Expand All @@ -1030,6 +1058,7 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {
} else {
self.match_simplified_candidates(
span,
scrutinee_span,
start_block,
otherwise_block,
candidates,
Expand All @@ -1042,6 +1071,7 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {
fn match_simplified_candidates(
&mut self,
span: Span,
scrutinee_span: Span,
start_block: BasicBlock,
otherwise_block: &mut Option<BasicBlock>,
candidates: &mut [&mut Candidate<'_, 'tcx>],
Expand Down Expand Up @@ -1087,6 +1117,7 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {
// Test for the remaining candidates.
self.test_candidates_with_or(
span,
scrutinee_span,
unmatched_candidates,
block,
otherwise_block,
Expand Down Expand Up @@ -1257,6 +1288,7 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {
fn test_candidates_with_or(
&mut self,
span: Span,
scrutinee_span: Span,
candidates: &mut [&mut Candidate<'_, 'tcx>],
block: BasicBlock,
otherwise_block: &mut Option<BasicBlock>,
Expand All @@ -1269,7 +1301,14 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {
match *first_candidate.match_pairs[0].pattern.kind {
PatKind::Or { .. } => (),
_ => {
self.test_candidates(span, candidates, block, otherwise_block, fake_borrows);
self.test_candidates(
span,
scrutinee_span,
candidates,
block,
otherwise_block,
fake_borrows,
);
return;
}
}
Expand Down Expand Up @@ -1302,6 +1341,7 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {

self.match_candidates(
span,
scrutinee_span,
remainder_start,
otherwise_block,
remaining_candidates,
Expand Down Expand Up @@ -1330,6 +1370,7 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {
otherwise
};
self.match_candidates(
or_span,
or_span,
candidate.pre_binding_block.unwrap(),
otherwise,
Expand Down Expand Up @@ -1497,6 +1538,7 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {
fn test_candidates<'pat, 'b, 'c>(
&mut self,
span: Span,
scrutinee_span: Span,
mut candidates: &'b mut [&'c mut Candidate<'pat, 'tcx>],
block: BasicBlock,
otherwise_block: &mut Option<BasicBlock>,
Expand Down Expand Up @@ -1591,6 +1633,7 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {
let candidate_start = this.cfg.start_new_block();
this.match_candidates(
span,
scrutinee_span,
candidate_start,
remainder_start,
&mut *candidates,
Expand All @@ -1607,6 +1650,7 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {
let remainder_start = remainder_start.unwrap_or_else(|| this.cfg.start_new_block());
this.match_candidates(
span,
scrutinee_span,
remainder_start,
otherwise_block,
candidates,
Expand All @@ -1617,7 +1661,7 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {
target_blocks
};

self.perform_test(block, match_place, &test, make_target_blocks);
self.perform_test(span, scrutinee_span, block, match_place, &test, make_target_blocks);
}

/// Determine the fake borrows that are needed from a set of places that
Expand Down Expand Up @@ -1713,6 +1757,7 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {
let fake_borrow_temps = self.lower_match_tree(
block,
pat.span,
pat.span,
false,
&mut [&mut guard_candidate, &mut otherwise_candidate],
);
Expand Down
14 changes: 11 additions & 3 deletions compiler/rustc_mir_build/src/build/matches/test.rs
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ use rustc_middle::ty::util::IntTypeExt;
use rustc_middle::ty::{self, adjustment::PointerCast, Ty, TyCtxt};
use rustc_span::def_id::DefId;
use rustc_span::symbol::{sym, Symbol};
use rustc_span::Span;
use rustc_target::abi::VariantIdx;

use std::cmp::Ordering;
Expand Down Expand Up @@ -151,6 +152,8 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {

pub(super) fn perform_test(
&mut self,
match_start_span: Span,
scrutinee_span: Span,
block: BasicBlock,
place_builder: PlaceBuilder<'tcx>,
test: &Test<'tcx>,
Expand Down Expand Up @@ -206,10 +209,15 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {
debug!("num_enum_variants: {}, variants: {:?}", num_enum_variants, variants);
let discr_ty = adt_def.repr.discr_type().to_ty(tcx);
let discr = self.temp(discr_ty, test.span);
self.cfg.push_assign(block, source_info, discr, Rvalue::Discriminant(place));
self.cfg.push_assign(
block,
self.source_info(scrutinee_span),
discr,
Rvalue::Discriminant(place),
);
self.cfg.terminate(
block,
source_info,
self.source_info(match_start_span),
TerminatorKind::SwitchInt {
discr: Operand::Move(discr),
switch_ty: discr_ty,
Expand Down Expand Up @@ -246,7 +254,7 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {
targets: switch_targets,
}
};
self.cfg.terminate(block, source_info, terminator);
self.cfg.terminate(block, self.source_info(match_start_span), terminator);
}

TestKind::Eq { value, ty } => {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -7,18 +7,18 @@
let mut _2: isize; // in scope 0 at $DIR/76803_regression.rs:12:9: 12:16

bb0: {
_2 = discriminant(_1); // scope 0 at $DIR/76803_regression.rs:12:9: 12:16
switchInt(move _2) -> [0_isize: bb2, otherwise: bb1]; // scope 0 at $DIR/76803_regression.rs:12:9: 12:16
_2 = discriminant(_1); // scope 0 at $DIR/76803_regression.rs:11:11: 11:12
switchInt(move _2) -> [0_isize: bb2, otherwise: bb1]; // scope 0 at $DIR/76803_regression.rs:11:5: 11:12
}

bb1: {
_0 = move _1; // scope 0 at $DIR/76803_regression.rs:13:14: 13:15
goto -> bb3; // scope 0 at $DIR/76803_regression.rs:11:5: 14:6
goto -> bb3; // scope 0 at $DIR/76803_regression.rs:13:14: 13:15
}

bb2: {
discriminant(_0) = 1; // scope 0 at $DIR/76803_regression.rs:12:20: 12:27
goto -> bb3; // scope 0 at $DIR/76803_regression.rs:11:5: 14:6
goto -> bb3; // scope 0 at $DIR/76803_regression.rs:12:20: 12:27
}

bb3: {
Expand Down
8 changes: 4 additions & 4 deletions src/test/mir-opt/const_goto.issue_77355_opt.ConstGoto.diff
Original file line number Diff line number Diff line change
Expand Up @@ -10,10 +10,10 @@

bb0: {
- StorageLive(_2); // scope 0 at $SRC_DIR/core/src/macros/mod.rs:LL:COL
- _3 = discriminant(_1); // scope 0 at $DIR/const_goto.rs:12:22: 12:28
- switchInt(move _3) -> [1_isize: bb2, 2_isize: bb2, otherwise: bb1]; // scope 0 at $DIR/const_goto.rs:12:22: 12:28
+ _2 = discriminant(_1); // scope 0 at $DIR/const_goto.rs:12:22: 12:28
+ switchInt(move _2) -> [1_isize: bb2, 2_isize: bb2, otherwise: bb1]; // scope 0 at $DIR/const_goto.rs:12:22: 12:28
- _3 = discriminant(_1); // scope 0 at $DIR/const_goto.rs:12:17: 12:20
- switchInt(move _3) -> [1_isize: bb2, 2_isize: bb2, otherwise: bb1]; // scope 0 at $SRC_DIR/core/src/macros/mod.rs:LL:COL
+ _2 = discriminant(_1); // scope 0 at $DIR/const_goto.rs:12:17: 12:20
+ switchInt(move _2) -> [1_isize: bb2, 2_isize: bb2, otherwise: bb1]; // scope 0 at $SRC_DIR/core/src/macros/mod.rs:LL:COL
}

bb1: {
Expand Down
18 changes: 9 additions & 9 deletions src/test/mir-opt/const_goto_const_eval_fail.f.ConstGoto.diff
Original file line number Diff line number Diff line change
Expand Up @@ -10,35 +10,35 @@
StorageLive(_1); // scope 0 at $DIR/const_goto_const_eval_fail.rs:7:11: 12:6
StorageLive(_2); // scope 0 at $DIR/const_goto_const_eval_fail.rs:8:15: 8:16
_2 = const A; // scope 0 at $DIR/const_goto_const_eval_fail.rs:8:15: 8:16
switchInt(_2) -> [1_i32: bb2, 2_i32: bb2, 3_i32: bb2, otherwise: bb1]; // scope 0 at $DIR/const_goto_const_eval_fail.rs:9:13: 9:14
switchInt(_2) -> [1_i32: bb2, 2_i32: bb2, 3_i32: bb2, otherwise: bb1]; // scope 0 at $DIR/const_goto_const_eval_fail.rs:8:9: 8:16
}

bb1: {
_1 = const true; // scope 0 at $DIR/const_goto_const_eval_fail.rs:10:18: 10:22
goto -> bb3; // scope 0 at $DIR/const_goto_const_eval_fail.rs:8:9: 11:10
goto -> bb3; // scope 0 at $DIR/const_goto_const_eval_fail.rs:10:18: 10:22
}

bb2: {
_1 = const B; // scope 0 at $DIR/const_goto_const_eval_fail.rs:9:26: 9:27
- goto -> bb3; // scope 0 at $DIR/const_goto_const_eval_fail.rs:8:9: 11:10
+ switchInt(_1) -> [false: bb4, otherwise: bb3]; // scope 0 at $DIR/const_goto_const_eval_fail.rs:13:9: 13:14
- goto -> bb3; // scope 0 at $DIR/const_goto_const_eval_fail.rs:9:26: 9:27
+ switchInt(_1) -> [false: bb4, otherwise: bb3]; // scope 0 at $DIR/const_goto_const_eval_fail.rs:7:5: 12:6
}

bb3: {
- switchInt(_1) -> [false: bb5, otherwise: bb4]; // scope 0 at $DIR/const_goto_const_eval_fail.rs:13:9: 13:14
- switchInt(_1) -> [false: bb5, otherwise: bb4]; // scope 0 at $DIR/const_goto_const_eval_fail.rs:7:5: 12:6
- }
-
- bb4: {
_0 = const 2_u64; // scope 0 at $DIR/const_goto_const_eval_fail.rs:14:17: 14:18
- goto -> bb6; // scope 0 at $DIR/const_goto_const_eval_fail.rs:7:5: 15:6
+ goto -> bb5; // scope 0 at $DIR/const_goto_const_eval_fail.rs:7:5: 15:6
- goto -> bb6; // scope 0 at $DIR/const_goto_const_eval_fail.rs:14:17: 14:18
+ goto -> bb5; // scope 0 at $DIR/const_goto_const_eval_fail.rs:14:17: 14:18
}

- bb5: {
+ bb4: {
_0 = const 1_u64; // scope 0 at $DIR/const_goto_const_eval_fail.rs:13:18: 13:19
- goto -> bb6; // scope 0 at $DIR/const_goto_const_eval_fail.rs:7:5: 15:6
+ goto -> bb5; // scope 0 at $DIR/const_goto_const_eval_fail.rs:7:5: 15:6
- goto -> bb6; // scope 0 at $DIR/const_goto_const_eval_fail.rs:13:18: 13:19
+ goto -> bb5; // scope 0 at $DIR/const_goto_const_eval_fail.rs:13:18: 13:19
}

- bb6: {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@
}

bb2: {
switchInt(((_3 as Some).0: bool)) -> [false: bb1, otherwise: bb3]; // scope 0 at $DIR/discriminant.rs:11:26: 11:30
switchInt(((_3 as Some).0: bool)) -> [false: bb1, otherwise: bb3]; // scope 0 at $DIR/discriminant.rs:11:21: 11:31
}

bb3: {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@
}

bb2: {
switchInt(((_3 as Some).0: bool)) -> [false: bb1, otherwise: bb3]; // scope 0 at $DIR/discriminant.rs:11:26: 11:30
switchInt(((_3 as Some).0: bool)) -> [false: bb1, otherwise: bb3]; // scope 0 at $DIR/discriminant.rs:11:21: 11:31
}

bb3: {
Expand Down
4 changes: 2 additions & 2 deletions src/test/mir-opt/const_prop/switch_int.main.ConstProp.diff
Original file line number Diff line number Diff line change
Expand Up @@ -8,8 +8,8 @@
bb0: {
StorageLive(_1); // scope 0 at $DIR/switch_int.rs:7:11: 7:12
_1 = const 1_i32; // scope 0 at $DIR/switch_int.rs:7:11: 7:12
- switchInt(_1) -> [1_i32: bb2, otherwise: bb1]; // scope 0 at $DIR/switch_int.rs:8:9: 8:10
+ switchInt(const 1_i32) -> [1_i32: bb2, otherwise: bb1]; // scope 0 at $DIR/switch_int.rs:8:9: 8:10
- switchInt(_1) -> [1_i32: bb2, otherwise: bb1]; // scope 0 at $DIR/switch_int.rs:7:5: 7:12
+ switchInt(const 1_i32) -> [1_i32: bb2, otherwise: bb1]; // scope 0 at $DIR/switch_int.rs:7:5: 7:12
}

bb1: {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -8,8 +8,8 @@
bb0: {
StorageLive(_1); // scope 0 at $DIR/switch_int.rs:7:11: 7:12
_1 = const 1_i32; // scope 0 at $DIR/switch_int.rs:7:11: 7:12
- switchInt(const 1_i32) -> [1_i32: bb2, otherwise: bb1]; // scope 0 at $DIR/switch_int.rs:8:9: 8:10
+ goto -> bb2; // scope 0 at $DIR/switch_int.rs:8:9: 8:10
- switchInt(const 1_i32) -> [1_i32: bb2, otherwise: bb1]; // scope 0 at $DIR/switch_int.rs:7:5: 7:12
+ goto -> bb2; // scope 0 at $DIR/switch_int.rs:7:5: 7:12
}

bb1: {
Expand Down
Loading

0 comments on commit 0a42dfc

Please sign in to comment.