Fix debugger stepping behavior with match expressions

[wesleywiser]

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:

Ubuntu.2021-08-06.10-33-55.mp4

After:

Ubuntu.2021-08-06.18-30-43.mp4

Fixes #87817

[rust-highfive]

r? @davidtwco

(rust-highfive has picked a reviewer for you, use r? to override)

[wesleywiser]

Some diagnostic changes occurred as a result of modifying the MIR spans. Most of these seem like improvements to me while others seem like regressions:

Improved:

• https://github.com/rust-lang/rust/pull/87832/files#diff-df9eba8a013e09a7304428ffe53fcc67a584bfdc8d52bfec6364d362925955b5
• https://github.com/rust-lang/rust/pull/87832/files#diff-e7728bdf681759850e0ae11b7ffc658a00de76d36f24e9a64ef046f3f694dd0b
• https://github.com/rust-lang/rust/pull/87832/files#diff-f6e6d5ff2bef898fe092ac42288fddd19e3a7fd4a1caee4b4c30454b53e22553
• https://github.com/rust-lang/rust/pull/87832/files#diff-b01c336e25604f441081a5f3ddf9dc29fcb2341b72368f5cd7934b3f3b2019a9
• https://github.com/rust-lang/rust/pull/87832/files#diff-a86619e7117393d98c9c78378f16188bbfedb0eb32897cea66b8c91ef6ba8650
• https://github.com/rust-lang/rust/pull/87832/files#diff-b9d4231fd8e034783fe0bd6612537b86ed066f73de1901f2e686d7e19f7edcd5
• https://github.com/rust-lang/rust/pull/87832/files#diff-a7385e9c1a294cabb12be88c459426b1dc1783d4de110d2b16c22f2346e2cb2d
• https://github.com/rust-lang/rust/pull/87832/files#diff-eed18751af43d53e366118e26342dbf51b73dd1e101c7cb37f7b0ac922c7e33a

Regressed:

• https://github.com/rust-lang/rust/pull/87832/files#diff-4cc12135a26ccfa8b6a5a23a11d34d36040a4779aee783484d6eaf3e7c3b8836 (could go either way IMO)
• https://github.com/rust-lang/rust/pull/87832/files#diff-0e25c9874eed9bd06c9ae2d0d10f199171fb86e861a2130c72eb276011d0336f (could go either way IMO)
• https://github.com/rust-lang/rust/pull/87832/files#diff-4442ad1d36b039a25eacdbe39daa0c18ddfc84ff6611c6304f278bc73f8520af

cc @estebank
This seems a net positive to change to me but please feel free to weigh in if you disagree or think we need to do some work to improve the diagnostics.

[Mark-Simulacrum]

Changing the tests to attribute to the match expression, rather than the branch, seems like it will hurt our ability to gauge in profilers (e.g. perf or valgrind) which branches are "hot" in some sense. Currently that can be quite useful in my experience.

It seems like the PR makes two separate changes in some sense - changing the branch out of the arm to attribute elsewhere seems rather unimportant to me from a profiling perspective, so seems good. Maybe we can keep the test instruction attribution to the arms though?

[wesleywiser]

It seems like the PR makes two separate changes in some sense - changing the branch out of the arm to attribute elsewhere seems rather unimportant to me from a profiling perspective, so seems good. Maybe we can keep the test instruction attribution to the arms though?

We could try that! My only concern is that it will change our code coverage since with either the status quo or these changes, the match expression will be covered while with that strategy, the match expression will not be covered.

[estebank]

This seems a net positive to change to me but please feel free to weigh in if you disagree or think we need to do some work to improve the diagnostics.

I'm overall ok with landing the PR with the current state of the diagnostics, but it'd be nice to improve them, even beyond what we have today.

[davidtwco]

LGTM, r=me if you don't intend to make any other changes.

[Mark-Simulacrum]

It seems like the PR makes two separate changes in some sense - changing the branch out of the arm to attribute elsewhere seems rather unimportant to me from a profiling perspective, so seems good. Maybe we can keep the test instruction attribution to the arms though?

We could try that! My only concern is that it will change our code coverage since with either the status quo or these changes, the match expression will be covered while with that strategy, the match expression will not be covered.

Hm, I would expect the match expression to be covered by its execution -- that is, something like the annotations below. I've annotated with (1) the order in which I expect debugger stepping to go, at least without optimizations, presuming Arm2 is taken.

match foo { // (1) coverage/debugger step on entry to match expression for executing `foo`
    Arm1 => { // (2) debugger steps here when this match arm's condition is evaluated
        body; // coverage/debugger step if body executes
    } // no coverage here (presuming nothing drops in this arm)
    Arm2 => { // (3) debugger steps here when this match arm's condition is evaluated. Note that this may be in a different order depending on optimizations compared to Arm1 (this is OK).
        body; // (4) coverage/debugger step if body executes
    }
}

[bors]

☔ The latest upstream changes (presumably #83417) made this pull request unmergeable. Please resolve the merge conflicts.

[wesleywiser]

Sorry for the delay, just now getting back to this.

Hm, I would expect the match expression to be covered by its execution -- that is, something like the annotations below.

I totally agree and the annotated source you left matches what I would prefer to see. The main challenge here is that when we lower the match to a SwitchInt terminator, we can only annotate that with one source location. As such, there isn't really a way to annotate the arms themselves with source info (locations 2 & 3 in your example). We could do something like generate the arm blocks with a leading Nop statement and annotate that to the arm but cg_ssa skips Nops anyway so we would still lose the source info at the codegen step.

In the current setup, only one of the arms (or parts of the arm) is actually annotated per required comparison so the results shown in a profiler are potentially misleading. For instance:

match opt { // covered from branch out of `match`
    Some(_) => // covered because of the `discriminant(_)` rvalue
        1, // covered because of the `_n = const 1` assignment
    None => // NOT COVERED
        0, // covered because of the `_n = const 0` assignment
}

(playground)

match opt { // covered from branch out of `match`
    Some(1) => // `Some(1)` is covered by the `discriminant(_)` rvalue test
               // `1` is covered by the test for `1` or `2` in the pattern match
        1, // covered because of the `_n = const 1` assignment
    Some(2) => // NOT COVERED
        2, // covered because of the `_n = const 2` assignment
    None => // NOT COVERED
        0, // covered because of the `_0 = const 0` assignment
}

(playground)

For match expressions written in the common style, I could see this causing issues in profiling or code coverage:

match None { // line hit 1 time
    Some(_) => 1, // line hit 1 time (testing discriminant)
    None => 0, // line hit 1 time (arm block actually hit)
}

In the case of match guards, those guards are properly annotated both before and after my changes.

Does that address your concerns @Mark-Simulacrum?

[Mark-Simulacrum]

Hm, I think so, though I'm not sure that I entirely followed your comment -- to some extent, it feels like part of the problem might be that we want sort of two different things for coverage and debugger stepping, but it sounds like both currently must use the same underlying information.

Namely, for debugger stepping, it is useful to map the 'test' instructions to the appropriate match arm, but for coverage purposes that is less helpful - you probably only want coverage to "cover" an arm if it's actually taken, not just that the condition was checked, right?

But, in any case, I don't want to block this on my concerns -- it sounds like we're more or less on the same page, and I don't actually use debuggers or coverage reports on a regular basis so likely not really an expert :)

[wesleywiser]

But, in any case, I don't want to block this on my concerns -- it sounds like we're more or less on the same page, and I don't actually use debuggers or coverage reports on a regular basis so likely not really an expert :)

That's totally fine! I don't want to merge if you have outstanding concerns.

to some extent, it feels like part of the problem might be that we want sort of two different things for coverage and debugger stepping

I might have confused things with my use of "coverage" in my last post. I just meant that a line or expression is "covered" if some MIR statement or terminator's SourceInfo includes the span of the source code. By "currently", I was referring to the state in master without my changes.

I think generally we want the same MIR -> source code mappings for both code coverage and debug info. Code coverage is a bit more lenient in that it doesn't matter what order the source lines are hit in, just how many times they are executed. We also have ways of marking source lines as executed even if they actually weren't at runtime (for example, if const evaluation kicks in and turns match None { None => 0, Some(_) => 1 } into 0) or including information about source lines that were optimized out (for example, if false { /*source code here*/ }).

For debugging, things are more challenging as we can only map each asm instruction to one source line (not even a range of lines like source based code coverage) and we also need to arrange the instructions so that the source lines execute in an order consistent with what the user might expect.

Namely, for debugger stepping, it is useful to map the 'test' instructions to the appropriate match arm, but for coverage purposes that is less helpful - you probably only want coverage to "cover" an arm if it's actually taken, not just that the condition was checked, right?

Ideally, I think your earlier post was correct. Something like this makes the most intuitive sense to me:

match Some(8) {
    Some(1) => 1,
    Some(2) => 2,
    Some(n) if n % 2 == 0 => 3,
    _ => 4,
}

Where the debugger would highlight Some(8) as the current expression to be evaluated, stepping would then jump to Some(1), then Some(2), then Some(n), then if n % 2 == 0, then 3 and then finally the closing }. (This is also nice as a teaching tool since it clearly shows the top-to-bottom evaluation order of the arms.)

However, debuggers (and some formats like pdb) only have line level resolution. With that restriction in mind, the above arrangement then looks like match Some(8) { is highlighted, stepping jumps to Some(1) => 1,, then Some(2) => 2,, then Some(n) if n % 2 == 0 => 3, and then finally }. An astute observer could deduce which arm was taken because _ => 4 was never evaluated but I think it's not a great UX because of how subtle the behavior is.

With my proposed changes, we instead see match Some(8) { is highlighted, stepping jumps to Some(n) if n % 2 == 0 => 3, and then finally stepping again takes us to the next line. While definitely not as nice as the ideal version, it at least makes it very clear which arm matches and has the correct ordering between the scrutinee line and the arm line.

For uses beyond the debugger like code coverage or profiling tools, I think this behavior also makes sense. Source based code coverage in particular doesn't see any difference in output with my change because it is able to track the arm block/expression independently from the arm.

[Mark-Simulacrum]

Source based code coverage in particular doesn't see any difference in output with my change because it is able to track the arm block/expression independently from the arm.

Ah, OK, I was not aware of this -- seems good then. I was thinking we were constrained to essentially line-level resolution on source-based code coverage as well, in which case seeing match arms as "hit" if they're not actually run (just failed tests for their patterns) would be quite unfortunate.

I think I am happy to proceed with this as-is, it looks like it is a net improvement though may not be "perfect" in some ways. Ultimately we can iterate further in the future, and from what you've said in your latest comment it sounds like it may not be quite possible to achieve the ideal state without considerable work anyway (given current debugger limitations).

[wesleywiser]

Rebased and updated various 32-bit test outputs I'd missed.

@bors r=davidtwco

[bors]

📌 Commit 64123f1889a3164faf6ce50682834a436f5ef65a has been approved by davidtwco

[bors]

⌛ Testing commit 64123f1889a3164faf6ce50682834a436f5ef65a with merge 052a830111e08785b6759e5f3132acadf77add87...

[bors]

💔 Test failed - checks-actions

[wesleywiser]

GDB doesn't seem to work the same on android ("target does not support the run command"). I'm going to ignore that test on android (many of the other debuginfo tests are also ignored on that platform for the same reason).

@bors r=davidtwco

[bors]

📌 Commit 84a2661 has been approved by davidtwco