Skip to content

Commit

Permalink
Rollup merge of rust-lang#87832 - wesleywiser:fix_match_step, r=david…
Browse files Browse the repository at this point in the history 
…twco

Fix debugger stepping behavior with `match` expressions

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.

Before:

https://user-images.githubusercontent.com/831192/128577421-ee0c9c03-da28-4d16-997a-d57988a7bb7f.mp4

After:

https://user-images.githubusercontent.com/831192/128577433-2ceab04d-953e-4e31-9387-93f049c71ff3.mp4

Fixes rust-lang#87817
  • Loading branch information
@Dylan-DPC
Dylan-DPC authored Aug 26, 2021
2 parents 20997f6 + 84a2661 commit 6768a2d
Show file tree
Hide file tree
Showing 93 changed files with 916 additions and 482 deletions.
63 changes: 54 additions & 9 deletions compiler/rustc_mir_build/src/build/matches/mod.rs
View file
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
View file
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
Loading

0 comments on commit 6768a2d

Please sign in to comment.