1432682 - (boring-bytecode) Breakpoint on first expression in block, before lexical declaration, excludes block from environment hierarchy

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect, P1)

Description

[Logan Smyth [:loganfsmyth]]

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_12_5) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/63.0.3239.132 Safari/537.36

Steps to reproduce:

Run the following snippet in jsshell:

var js = `(function () {
  debugger;
  for (let x in { a: 4 }) {
    "any";
    let y = 4;
  }
})();
`;
 
function printLine(script, offset) {
  var loc = script.getOffsetLocation(offset);
  var line = js.split('\n')[loc.lineNumber - 1];
  return line.slice(0, loc.columnNumber) + '$' + line.slice(loc.columnNumber);
}
 
function printState(frame) {
  print('---- ' + frame.offset + ': ' +
        printLine(frame.script, frame.offset));
  var env = frame.environment;
  while (env) {
    print('==== ' + env.type);
    if (!env.parent) {
      print('... global stuff');
      break;
    }
    env.names().forEach(function (n) {
      var value = env.getVariable(n);
      print(n + ': ' + value);
    });
    env = env.parent;
  }
}
 
var g = newGlobal();
var dbg = new Debugger(g);
dbg.onDebuggerStatement = function (frame) {
  printState(frame);
  frame.onStep = function () {
    printState(frame);
  };
  frame.onPop = function () {
    frame.onStep = undefined;
  };
};
g.eval(js);

to execute

(function () {
  debugger;
  for (let x in { a: 4 }) {
    "any";
    let y = 4;
  }
})();

and dump all scoping information.

Snippet bytecode:

00000:  arguments                       # arguments
00001:  setaliasedvar "arguments" (hops = 0, slot = 2) # arguments
00006:  pop                             #
00007:  functionthis                    # THIS
00008:  setaliasedvar ".this" (hops = 0, slot = 3) # THIS
00013:  pop                             #
main:
00014:  debugger                        #
00015:  pushlexicalenv lexical {x: env slot 2} #
00020:  newobject ({a:(void 0)})        # OBJ
00025:  int8 4                          # OBJ 4
00027:  initprop "a"                    # OBJ
00032:  iter                            # ITER
00033:  undefined                       # ITER undefined
00034:  goto 61 (+27)                   # ITER undefined
 
# from ifeq @ 00066
00039:  loophead                        # ITER MOREITER
00040:  recreatelexicalenv              # ITER MOREITER
00041:  iternext                        # ITER MOREITER
00042:  initaliasedlexical "x" (hops = 0, slot = 2) # ITER MOREITER
00047:  pushlexicalenv lexical {y: env slot 2} # ITER MOREITER
00052:  int8 4                          # ITER MOREITER 4
00054:  initaliasedlexical "y" (hops = 0, slot = 2) # ITER MOREITER 4
00059:  pop                             # ITER MOREITER
00060:  poplexicalenv                   # ITER MOREITER
 
# from goto @ 00034
00061:  loopentry 129                   # ITER undefined
00063:  pop                             # ITER
00064:  moreiter                        # ITER MOREITER
00065:  isnoiter                        # ITER MOREITER ISNOITER
00066:  ifeq 39 (-27)                   # ITER MOREITER
00071:  jumptarget                      # ITER MOREITER
00072:  pop                             # ITER
00073:  enditer                         #
00074:  poplexicalenv                   #
00075:  retrval                         #


Actual results:

The important section of output is:

---- 47: $    "any";
==== declarative
x: a
==== declarative
arguments: [object Object]
==== declarative
==== object
... global stuff
---- 52:     let $y = 4;
==== declarative
y: [object Object]
==== declarative
x: a
==== declarative
arguments: [object Object]
==== declarative
==== object
... global stuff

Note that when stepping over the "any" line, the scopes are

==== declarative
x: a

which does not include an uninitialized `y` binding, whereas the next step actually creates the binding:

==== declarative
y: [object Object]
==== declarative
x: a
==== declarative
arguments: [object Object]



Expected results:

The `y` binding and its environment should show up when the debugger is paused at the "any" line.

Comment 1

[Logan Smyth [:loganfsmyth]]

This may be related to the similar issue for function bodies rather than blocks: https://bugzilla.mozilla.org/show_bug.cgi?id=1432885

Comment 2

[Nicolas B. Pierron [:nbp]]

Jim, when would you be able to look at this issue?

Comment 3

[Jim Blandy :jimb]

Yes, I'll take this.

Comment 4

[Jason Orendorff [:jorendorff]]

The ineffectual statement `"any";` is optimized away before bytecode is emitted. So the debugger isn't stopping just before the bytecode for `"any";`, because there is no bytecode for `"any";`.

The troublesome pause is at the JSOP_PUSHLEXICALENV instruction that enters the scope of `y`. We pause before executing the instruction and therefore before `y` exists.

The location information for that instruction is bogus: entering a lexical scope is not a Step.

Comment 5

[Jason Orendorff [:jorendorff]]

We never want to stop immediately before JSOP_PUSHLEXICALENV. There isn't a reasonable answer to the question "where are we paused?" from a user's perspective.

Currently we have a hack to try to prevent us from ever stopping there, but it isn't working for us in this case:
  https://searchfox.org/mozilla-central/source/js/src/frontend/BytecodeEmitter.cpp#6808-6827

I don't think any such hack can succeed in all cases. Consider:

    1   if (f()) {
    2       g();
    3   }
    4   {  // JSOP_PUSHLEXICALENV
    5       let x = h();
    6       i(x);
    7   }

Suppose we're stepping through the above code. The JSOP_PUSHLEXICALENV instruction will have some location. Suppose `f()` returns true, so we execute `g();`. If we want to avoid pausing on JSOP_PUSHLEXICALENV in this case, it must have the same location as `g()`. But that would be very bad in the case where `f()` returns false: after stepping over `if (f())`, we would step to `g();` which does not actually execute!

So I propose a different hack: never stop on this opcode.

Comment 6

[Jason Orendorff [:jorendorff]]

Or JSOP_UNINITIALIZED or JSOP_INITLEXICAL. What could go wrong?

Comment 7

[Jason Orendorff [:jorendorff]]

I don't think

Spoke with Jim. Here's a possible plan.

1. Start by changing the onStep handler to never fire on JSOP_PUSHLEXICALENV. (And possibly other opcodes; I need to figure out what sort of code results in UNINITIALIZED.)

I'm not 100% sure this is OK; it sounded good while we were chatting, but there are a lot of things about this that are jut a little bit odd, like the fact that you might want to stop at these opcode when stepping out but not when stepping in/over...

2. Remove the existing hack. I am pretty sure this is OK. With #1 it's great. Without #1, it'll cause us to pause at some curly braces where we do not exactly want to pause, but it wouldn't be confusing for users, just a bit of a nuisance.

3. This is a case where we have a "boring bytecode" that we don't want to stop on, in this case because end users consider the step too small to bother with. As it stands, our source notes don't support this. Long term, if we accumulate other "boring bytecode" cases, we'll want to change that.

Comment 8

[Jason Orendorff [:jorendorff]]

Attachment: Part 0: Factor out some test code into a new lib file, jit-test/lib/stepping.js

This also fixes a bug in that code: it skipped the first stopping point in the
function (before the first instruction). It turns out that we already almost
always stop at the opening brace of a function, and that's fine.

Comment 9

[Jason Orendorff [:jorendorff]]

Attachment: Part 0: Factor out some test code into a new lib file, jit-test/lib/stepping.js

This also fixes a bug in that code: it skipped the first stopping point in the
function (before the first instruction).

Comment 10

[Jason Orendorff [:jorendorff]]

Attachment: Part 1: Remove the hack that causes the bad behavior

The hack caused bytecode for block declaration instantiation to be assigned the
location of the first statement inside the block. Unfortunately it made the
source view of the debugger client seem out of sync with the Scopes panel: when
paused after hitting a breakpoint on that line or stepping there, the source
panel showed our location as being inside the block, but the Scopes panel did
not show a block scope.

Comment 11

[Jason Orendorff [:jorendorff]]

Attachment: Part 1: Remove the hack that causes the bad behavior

The hack caused bytecode for block declaration instantiation to be assigned the
location of the first statement inside the block. Unfortunately it made the
source view of the debugger client seem out of sync with the Scopes panel: when
paused after hitting a breakpoint on that line or stepping there, the source
panel showed our location as being inside the block, but the Scopes panel did
not show a block scope.

Comment 12

[Jason Orendorff [:jorendorff]]

Attachment: Part 2: Add new hack to avoid stopping in declaration instantiation

Comment 13

[Jim Blandy :jimb]

Comment on attachment 8950778 [details] [diff] [review]
Part 2: Add new hack to avoid stopping in declaration instantiation

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

::: js/src/vm/Debugger.cpp
@@ +2126,4 @@
>      JS::AutoSaveExceptionState saveExc(cx);
>  
> +    // If the next instruction is boring, don't report a step.
> +    if (iter.abstractFramePtr().hasScript()) {

Does it matter that this code falls within the AutoSaveExceptionState's scope? I think it doesn't matter, so would it be clearer to move it above?

Comment 14

[Jason Orendorff [:jorendorff]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=63a41b803a5777611d2279bb11aa2c1f954e9e18

Comment 15

[Jason Orendorff [:jorendorff]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=9300a0f19c402710058ebe3cace3a88c1671cb06

Comment 16

[Jason Orendorff [:jorendorff]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=2f4e7be1d5cd1558996cf4ed32e0a24492e08ef4

Comment 17

[Jason Orendorff [:jorendorff]]

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

Comment 18

[Jason Orendorff [:jorendorff]]

Attachment: Part 1b: Adjust an existing test for new stepping behavior

This existing test assumes that stepping into a function stops at the first
statement in the function. This is usually true. However, now we are removing a
hack, such that our actual behavior for this *particular* function is to stop
at the opening curly brace. This causes the test to fail, without anything
really being broken.

The test is intended to test the interaction of stepping and breakpoints, so
the fix that stays truest to the purpose of the test is to change the debuggee
here to a function with no prologue instructions, so that we don't stop at the
opening brace.

Comment 19

[Jason Orendorff [:jorendorff]]

Update: this had a bug which took a long time to track down! It turned out to be related to some filtering going on when enabling stepping in baseline jitcode (bug 1440481).

Since that was so confusing for me, I decided not to land part 2 here, which to be honest is a bad hack and also involves filtering onStep calls. It could be just as confusing for someone else (or me again) down the line.

The right answer is to mark this as a boring *step*. Separate bug.

Comment 20

[Jason Orendorff [:jorendorff]]

Attachment: Part 1b: Adjust an existing test for new stepping behavior

This existing test assumes that stepping into a function stops at the first
statement in the function. This is usually true. However, now we are removing a
hack, such that our actual behavior for this *particular* function is to stop
at the opening curly brace. This causes the test to fail, without anything
really being broken.

The test is intended to test the interaction of stepping and breakpoints, so
the fix that stays truest to the purpose of the test is to change the debuggee
here to a function with no prologue instructions, so that we don't stop at the
opening brace.

Comment 21

[Jason Orendorff [:jorendorff]]

Attachment: Part 1b: Adjust an existing test for new stepping behavior

test_stepping-08.js assumes that stepping into a function stops at the first
statement in the function. This is usually true. However, now we are removing a
hack, such that our actual behavior for this *particular* function is to stop
at the opening curly brace. This causes the test to fail, without anything
really being broken.

The test is intended to test the interaction of stepping and breakpoints, so
the fix that stays truest to the purpose of the test is to change the debuggee
here to a function with no prologue instructions, so that we don't stop at the
opening brace.

test_blackboxing-01.js is a similar story.

Comment 22

[Jason Orendorff [:jorendorff]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/77878d66cb1f61c508131b0e5b29aaa3883a3a02
Bug 1432682 - Part 0: Factor out some test code into a new lib file, jit-test/lib/stepping.js. r=jimb.

https://hg.mozilla.org/integration/mozilla-inbound/rev/9114315e792d29e3fc41a0748e683ded96f689d5
Bug 1432682 - Part 1: Remove the hack that causes the bad behavior. r=jimb.

Comment 23

[Arthur Iakab [arthur_iakab]]

https://hg.mozilla.org/mozilla-central/rev/77878d66cb1f
https://hg.mozilla.org/mozilla-central/rev/9114315e792d