Closed Bug 1432682 (boring-bytecode) Opened 2 years ago Closed 2 years ago

Breakpoint on first expression in block, before lexical declaration, excludes block from environment hierarchy

Categories

(Core :: JavaScript Engine, defect, P1)

defect

Tracking

()

RESOLVED FIXED
mozilla60
Tracking Status
firefox60 --- fixed

People

(Reporter: loganfsmyth, Assigned: jorendorff)

References

Details

Attachments

(4 files, 4 obsolete files)

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.
This may be related to the similar issue for function bodies rather than blocks: https://bugzilla.mozilla.org/show_bug.cgi?id=1432885
Jim, when would you be able to look at this issue?
Flags: needinfo?(jimb)
Yes, I'll take this.
Assignee: nobody → jimb
Flags: needinfo?(jimb)
Priority: -- → P1
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.
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.
Or JSOP_UNINITIALIZED or JSOP_INITLEXICAL. What could go wrong?
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.
Alias: boring-bytecode
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.
Assignee: jimb → jorendorff
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Attachment #8950773 - Flags: review?(jimb)
This also fixes a bug in that code: it skipped the first stopping point in the
function (before the first instruction).
Attachment #8950774 - Flags: review?(jimb)
Attachment #8950773 - Attachment is obsolete: true
Attachment #8950773 - Flags: review?(jimb)
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.
Attachment #8950776 - Flags: review?(jimb)
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.
Attachment #8950777 - Flags: review?(jimb)
Attachment #8950776 - Attachment is obsolete: true
Attachment #8950776 - Flags: review?(jimb)
Attachment #8950774 - Flags: review?(jimb) → review+
Attachment #8950777 - Flags: review?(jimb) → review+
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?
Attachment #8950778 - Flags: review?(jimb) → review+
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.
Attachment #8954138 - Flags: review?(jimb)
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.
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.
Attachment #8954174 - Flags: review?(jimb)
Attachment #8954138 - Attachment is obsolete: true
Attachment #8954138 - Flags: review?(jimb)
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.
Attachment #8954221 - Flags: review?(jimb)
Attachment #8954174 - Attachment is obsolete: true
Attachment #8954174 - Flags: review?(jimb)
Attachment #8954221 - Flags: review?(jimb) → review+
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.
https://hg.mozilla.org/mozilla-central/rev/77878d66cb1f
https://hg.mozilla.org/mozilla-central/rev/9114315e792d
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla60
You need to log in before you can comment on or make changes to this bug.