Bug 1003554 - make entry points correspond to entries in the line table; r=jimb,fitzgen

devtools/server/actors/script.js

@@ -814,6 +814,17 @@ ThreadActor.prototype = {
     return function () {
       // onStep is called with 'this' set to the current frame.
 
+      // Only allow stepping stops at entry points for the line, when
+      // the stepping occurs in a single frame.  The "same frame"
+      // check makes it so a sequence of steps can step out of a frame
+      // and into subsequent calls in the outer frame.  E.g., if there
+      // is a call "a(b())" and the user steps into b, then this
+      // condition makes it possible to step out of b and into a.
+      if (this === startFrame &&
+          !this.script.getOffsetLocation(this.offset).isEntryPoint) {
+        return undefined;
+      }
+
       const generatedLocation = thread.sources.getFrameLocation(this);
       const newLocation = thread.unsafeSynchronize(thread.sources.getOriginalLocation(
         generatedLocation));

js/src/doc/Debugger/Debugger.Script.md

@@ -218,6 +218,9 @@ methods of other kinds of objects.
 
     * columnNumber: the column number for which offset is an entry point
 
+    * isEntryPoint: true if the offset is a column entry point, as
+      would be reported by getAllColumnOffsets(); otherwise false.
+
 `getOffsetsCoverage()`:
 :   Return `null` or an array which contains informations about the coverage of
     all opcodes. The elements of the array are objects, each of which describes

js/src/jit-test/tests/debug/Frame-onStep-11.js

@@ -0,0 +1,36 @@
+// Stepping out of a finally should not appear to
+// step backward to some earlier statement.
+
+var g = newGlobal();
+g.eval(`function f() {
+         debugger;              // +0
+         var x = 0;             // +1
+         try {                  // +2
+           x = 1;               // +3
+           throw 'something';   // +4
+         } catch (e) {          // +5
+           x = 2;               // +6
+         } finally {            // +7
+           x = 3;               // +8
+         }                      // +9
+         x = 4;                 // +10
+       }`);                     // +11
+
+var dbg = Debugger(g);
+
+let foundLines = '';
+
+dbg.onDebuggerStatement = function(frame) {
+  let debugLine = frame.script.getOffsetLocation(frame.offset).lineNumber;
+  frame.onStep = function() {
+    // Only record a stop when the offset is an entry point.
+    let foundLine = this.script.getOffsetLocation(this.offset).lineNumber;
+    if (foundLine != debugLine && this.script.getLineOffsets(foundLine).indexOf(this.offset) >= 0) {
+      foundLines += "," + (foundLine - debugLine);
+    }
+  };
+};
+
+g.f();
+
+assertEq(foundLines, ",1,2,3,4,6,7,8,10");

js/src/jit-test/tests/debug/Frame-onStep-12.js

@@ -0,0 +1,129 @@
+// Because our script source notes record only those bytecode offsets
+// at which source positions change, the default behavior in the
+// absence of a source note is to attribute a bytecode instruction to
+// the same source location as the preceding instruction. When control
+// flows from the preceding bytecode to the one we're emitting, that's
+// usually plausible. But successors in the bytecode stream are not
+// necessarily successors in the control flow graph. If the preceding
+// bytecode was a back edge of a loop, or the jump at the end of a
+// 'then' clause, its source position can be completely unrelated to
+// that of its successor.
+
+// We try to avoid showing such nonsense offsets to the user by
+// requiring breakpoints and single-stepping to stop only at a line's
+// entry points, as reported by Debugger.Script.prototype.getLineOffsets;
+// and then ensuring that those entry points are all offsets mentioned
+// explicitly in the source notes, and hence deliberately attributed
+// to the given bytecode.
+
+// This bit of JavaScript compiles to bytecode ending in a branch
+// instruction whose source position is the body of an unreachable
+// loop. The first instruction of the bytecode we emit following it
+// will inherit this nonsense position, if we have not explicitly
+// emitted a source note for said instruction.
+
+// This test steps across such code and verifies that control never
+// appears to enter the unreachable loop.
+
+var bitOfCode = `debugger;                    // +0
+                 if(false) {                  // +1
+                   for(var b=0; b<0; b++) {   // +2
+                      c = 2                   // +3
+                    }                         // +4
+                 }`;                          // +5
+
+var g = newGlobal();
+var dbg = Debugger(g);
+
+g.eval("function nothing() { }\n");
+
+var log = '';
+dbg.onDebuggerStatement = function(frame) {
+  let debugLine = frame.script.getOffsetLocation(frame.offset).lineNumber;
+  frame.onStep = function() {
+    let foundLine = this.script.getOffsetLocation(this.offset).lineNumber;
+    if (this.script.getLineOffsets(foundLine).indexOf(this.offset) >= 0) {
+      log += (foundLine - debugLine).toString(16);
+    }
+  };
+};
+
+function testOne(name, body, expected) {
+  print(name);
+  log = '';
+  g.eval(`function ${name} () { ${body} }`);
+  g.eval(`${name}();`);
+  assertEq(log, expected);
+}
+
+
+
+// Test the instructions at the end of a "try".
+testOne("testTryFinally",
+        `try {
+           ${bitOfCode}
+         } finally {            // +6
+         }                      // +7
+         nothing();             // +8
+        `, "168");
+
+// The same but without a finally clause.
+testOne("testTryCatch",
+        `try {
+           ${bitOfCode}
+         } catch (e) {          // +6
+         }                      // +7
+         nothing();             // +8
+        `, "18");
+
+// Test the instructions at the end of a "catch".
+testOne("testCatchFinally",
+        `try {
+           throw new TypeError();
+         } catch (e) {
+           ${bitOfCode}
+         } finally {            // +6
+         }                      // +7
+         nothing();             // +8
+        `, "168");
+
+// The same but without a finally clause.  This relies on a
+// SpiderMonkey extension, because otherwise there's no way to see
+// extra instructions at the end of a catch.
+testOne("testCatch",
+        `try {
+           throw new TypeError();
+         } catch (e if e instanceof TypeError) {
+           ${bitOfCode}
+         } catch (e) {          // +6
+         }                      // +7
+         nothing();             // +8
+        `, "18");
+
+// Test the instruction at the end of a "finally" clause.
+testOne("testFinally",
+        `try {
+         } finally {
+           ${bitOfCode}
+         }                      // +6
+         nothing();             // +7
+        `, "17");
+
+// Test the instruction at the end of a "then" clause.
+testOne("testThen",
+        `if (1 === 1) {
+           ${bitOfCode}
+         } else {               // +6
+         }                      // +7
+         nothing();             // +8
+        `, "18");
+
+// Test the instructions leaving a switch block.
+testOne("testSwitch",
+        `var x = 5;
+         switch (x) {
+           case 5:
+             ${bitOfCode}
+         }                      // +6
+         nothing();             // +7
+        `, "17");

js/src/jit-test/tests/debug/Script-getAllColumnOffsets-06.js

@@ -14,9 +14,10 @@ Debugger(global).onDebuggerStatement = function (frame) {
 };
 
 global.log = "";
+global.eval("function ppppp() { return 1; }");
 //                     1         2         3         4
 //           0123456789012345678901234567890123456789012345678
-global.eval("function f(){ 1 && print(print()) && new Error() } debugger;");
+global.eval("function f(){ 1 && ppppp(ppppp()) && new Error() } debugger;");
 global.f();
 
 // 14 - Enter the function body

js/src/jit-test/tests/debug/Script-getOffsetLocation.js

@@ -2,18 +2,28 @@
 
 var global = newGlobal();
 Debugger(global).onDebuggerStatement = function (frame) {
-    var script = frame.eval("f").return.script;
+    var script = frame.script;
+    var byOffset = [];
     script.getAllColumnOffsets().forEach(function (entry) {
         var {lineNumber, columnNumber, offset} = entry;
-        var location = script.getOffsetLocation(offset);
-        assertEq(location.lineNumber, lineNumber);
-        assertEq(location.columnNumber, columnNumber);
+        byOffset[offset] = {lineNumber, columnNumber};
     });
+
+    frame.onStep = function() {
+        var offset = frame.offset;
+        var location = script.getOffsetLocation(offset);
+        if (location.isEntryPoint) {
+            assertEq(location.lineNumber, byOffset[offset].lineNumber);
+            assertEq(location.columnNumber, byOffset[offset].columnNumber);
+        } else {
+            assertEq(byOffset[offset], undefined);
+        }
+    };
 };
 
 function test(body) {
   print("Test: " + body);
-  global.eval(`function f(n) { ${body} } debugger;`);
+  global.eval(`function f(n) { debugger; ${body} }`);
   global.f(3);
 }
 

js/src/vm/CommonPropertyNames.h

@@ -124,6 +124,7 @@
     macro(int8, int8, "int8") \
     macro(int16, int16, "int16") \
     macro(int32, int32, "int32") \
+    macro(isEntryPoint, isEntryPoint, "isEntryPoint") \
     macro(isExtensible, isExtensible, "isExtensible") \
     macro(iteratorIntrinsic, iteratorIntrinsic, "__iterator__") \
     macro(join, join, "join") \

js/src/vm/Debugger.cpp

@@ -4856,53 +4856,71 @@ class BytecodeRangeWithPosition : private BytecodeRange
 
     BytecodeRangeWithPosition(JSContext* cx, JSScript* script)
       : BytecodeRange(cx, script), lineno(script->lineno()), column(0),
-        sn(script->notes()), snpc(script->code())
+        sn(script->notes()), snpc(script->code()), isEntryPoint(false)
     {
         if (!SN_IS_TERMINATOR(sn))
             snpc += SN_DELTA(sn);
         updatePosition();
         while (frontPC() != script->main())
             popFront();
+        isEntryPoint = true;
     }
 
     void popFront() {
         BytecodeRange::popFront();
-        if (!empty())
+        if (empty())
+            isEntryPoint = false;
+        else
             updatePosition();
     }
 
     size_t frontLineNumber() const { return lineno; }
     size_t frontColumnNumber() const { return column; }
 
+    // Entry points are restricted to bytecode offsets that have an
+    // explicit mention in the line table.  This restriction avoids a
+    // number of failing cases caused by some instructions not having
+    // sensible (to the user) line numbers, and it is one way to
+    // implement the idea that the bytecode emitter should tell the
+    // debugger exactly which offsets represent "interesting" (to the
+    // user) places to stop.
+    bool frontIsEntryPoint() const { return isEntryPoint; }
+
   private:
     void updatePosition() {
         /*
          * Determine the current line number by reading all source notes up to
          * and including the current offset.
          */
+        jsbytecode *lastLinePC = nullptr;
         while (!SN_IS_TERMINATOR(sn) && snpc <= frontPC()) {
             SrcNoteType type = (SrcNoteType) SN_TYPE(sn);
             if (type == SRC_COLSPAN) {
                 ptrdiff_t colspan = SN_OFFSET_TO_COLSPAN(GetSrcNoteOffset(sn, 0));
                 MOZ_ASSERT(ptrdiff_t(column) + colspan >= 0);
                 column += colspan;
+                lastLinePC = snpc;
             } else if (type == SRC_SETLINE) {
                 lineno = size_t(GetSrcNoteOffset(sn, 0));
                 column = 0;
+                lastLinePC = snpc;
             } else if (type == SRC_NEWLINE) {
                 lineno++;
                 column = 0;
+                lastLinePC = snpc;
             }
 
             sn = SN_NEXT(sn);
             snpc += SN_DELTA(sn);
         }
+        isEntryPoint = lastLinePC == frontPC();
     }
 
     size_t lineno;
     size_t column;
     jssrcnote* sn;
     jsbytecode* snpc;
+    bool isEntryPoint;
 };
 
 /*
@@ -5083,6 +5101,10 @@ DebuggerScript_getOffsetLocation(JSContext* cx, unsigned argc, Value* vp)
     if (!ScriptOffset(cx, script, args[0], &offset))
         return false;
 
+    FlowGraphSummary flowData(cx);
+    if (!flowData.populate(cx, script))
+        return false;
+
     RootedPlainObject result(cx, NewBuiltinClassInstance<PlainObject>(cx));
     if (!result)
         return false;
@@ -5100,6 +5122,15 @@ DebuggerScript_getOffsetLocation(JSContext* cx, unsigned argc, Value* vp)
     if (!DefineProperty(cx, result, cx->names().columnNumber, value))
         return false;
 
+    // The same entry point test that is used by getAllColumnOffsets.
+    bool isEntryPoint = (r.frontIsEntryPoint() &&
+                         !flowData[offset].hasNoEdges() &&
+                         (flowData[offset].lineno() != r.frontLineNumber() ||
+                          flowData[offset].column() != r.frontColumnNumber()));
+    value.setBoolean(isEntryPoint);
+    if (!DefineProperty(cx, result, cx->names().isEntryPoint, value))
+        return false;
+
     args.rval().setObject(*result);
     return true;
 }
@@ -5122,6 +5153,9 @@ DebuggerScript_getAllOffsets(JSContext* cx, unsigned argc, Value* vp)
     if (!result)
         return false;
     for (BytecodeRangeWithPosition r(cx, script); !r.empty(); r.popFront()) {
+        if (!r.frontIsEntryPoint())
+            continue;
+
         size_t offset = r.frontOffset();
         size_t lineno = r.frontLineNumber();
 
@@ -5195,7 +5229,8 @@ DebuggerScript_getAllColumnOffsets(JSContext* cx, unsigned argc, Value* vp)
         size_t offset = r.frontOffset();
 
         /* Make a note, if the current instruction is an entry point for the current position. */
-        if (!flowData[offset].hasNoEdges() &&
+        if (r.frontIsEntryPoint() &&
+            !flowData[offset].hasNoEdges() &&
             (flowData[offset].lineno() != lineno ||
              flowData[offset].column() != column)) {
             RootedPlainObject entry(cx, NewBuiltinClassInstance<PlainObject>(cx));
@@ -5259,6 +5294,9 @@ DebuggerScript_getLineOffsets(JSContext* cx, unsigned argc, Value* vp)
     if (!result)
         return false;
     for (BytecodeRangeWithPosition r(cx, script); !r.empty(); r.popFront()) {
+        if (!r.frontIsEntryPoint())
+            continue;
+
         size_t offset = r.frontOffset();
 
         /* If the op at offset is an entry point, append offset to result. */