1139235 - Single-stepping through a switch statement should always directly jump to the right case

Status: VERIFIED FIXED

(DevTools :: Debugger, defect)

Description

[Till Schneidereit [:till]]

Currently, whether we directly switch to the right `case` or not depends on how we're treating the `switch` internally. If it's sparse, single-stepping stops at every `case` until finally the right one is reached. If it's dense, we do the right thing.

This makes debugging Shumway's main interpreter loop close to impossible, causing me to frequently switch to Chrome :(

Comment 1

[Tom Tromey :tromey]

This seems fine to me, except for the situation where the case expression is not a literal.
In that situation it seems like one might reasonably want to step through the case.
I didn't look at Shumway to see what is done there.

Bug 1003554 is related.

There, the current plan is to restrict entry points to offsets that have an explicit
entry in the line table.

Given that bug, one approach to fixing this one would be to change BytecodeEmitter.cpp:EmitSwitch
to avoid emitting a line note for a "case" if the case's expression is a literal.

Comment 2

[Till Schneidereit [:till]]

(In reply to Tom Tromey :tromey from comment #1)
> This seems fine to me, except for the situation where the case expression is
> not a literal.
> In that situation it seems like one might reasonably want to step through
> the case.

Agreed.

> I didn't look at Shumway to see what is done there.

We only use literals as case expressions, so this doesn't apply to Shumway.

Comment 3

[Tom Tromey :tromey]

Attachment: don't set line for literal case expressions

Here's a patch.  It requires the patches from the other bugs,
specifically 1003554.

Comment 4

[Tom Tromey :tromey]

Attachment: don't set line for literal case expressions

Rebased and updated for bytecode emitter changes.

Comment 5

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Comment on attachment 8588047 [details] [diff] [review]
don't set line for literal case expressions

Review of attachment 8588047 [details] [diff] [review]:
-----------------------------------------------------------------

Jim agreed on IRC to pass this review on to me.

Comment 6

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Comment on attachment 8588047 [details] [diff] [review]
don't set line for literal case expressions

Review of attachment 8588047 [details] [diff] [review]:
-----------------------------------------------------------------

r=me with comments addressed

::: js/src/frontend/BytecodeEmitter.cpp
@@ +6860,5 @@
>      return true;
>  }
>  
>  bool
> +BytecodeEmitter::emitTree(ParseNode* pn, bool suppressLine)

Magical bool parameters at the end of arguments lists kinda suck. It is annoying to tell what's going on at call sites, unless you add /* supressLine = */ comments, which also suck to author. I would prefer an enum class, but if you think it's overkill I'm not going to cry foul.

::: js/src/jit-test/tests/debug/Frame-onStep-14.js
@@ +5,5 @@
> +g.eval('function bob() { return "bob"; }');
> +
> +// Stepping into a sparse switch should not stop on literal cases.
> +g.eval(`function lit(x) {
> +  debugger;                     // +0

Rather than checking line deltas, you can use the `evaluate` function to give the source a sane starting line number (such as 1). You can run the js shell and do help(evaluate) for docs on the function.

@@ +7,5 @@
> +// Stepping into a sparse switch should not stop on literal cases.
> +g.eval(`function lit(x) {
> +  debugger;                     // +0
> +  switch(x) {                   // +1
> +  case "bob":                   // +2

Can you add other cases, so we can verify that they get skipped past without requiring multiple steps?

Comment 7

[Tom Tromey :tromey]

Attachment: don't set line for literal case expressions

Updated according to review.

Comment 8

[Tom Tromey :tromey]

Attachment: don't set line for literal case expressions

I overlooked changing a parameter name in BytecodeEmitter.h.

Comment 9

[Tom Tromey :tromey]

Attachment: don't set line for literal case expressions

Rebased.

Comment 10

[Tom Tromey :tromey]

Attachment: don't set line for literal case expressions

Operator at end of line.

Comment 11

[Tom Tromey :tromey]

Attachment: don't set line for literal case expressions

Rebased.

Comment 12

[Tom Tromey :tromey]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=8ead55e5a5ba

Comment 13

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/52e817396ca4

Comment 14

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/52e817396ca4

Comment 15

[Sebastian Zartner [:sebo]]

I can confirm that it's working in today's Nightly.

Sebastian