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 :(

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.

(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.

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

Rebased and updated for bytecode emitter changes.

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 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?

Updated according to review.

I overlooked changing a parameter name in BytecodeEmitter.h.

Rebased.

Operator at end of line.

Rebased.

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