Bug 1003554 - make entry points correspond to entries in the line table; r=jimb,fitzgen
authorTom Tromey <tromey@mozilla.com>
Thu, 22 Oct 2015 09:49:00 +0200
changeset 269931 093802a6d8aeff51c68c92323b3b799b5fc7cd4c
parent 269841 c4aebb4abcc52d6a4e096f6d30c799e82f2717d8
child 269932 b494ad467ced34e4dff5177377644b088da4fc8d
push id29595
push userkwierso@gmail.com
push dateWed, 28 Oct 2015 23:41:46 +0000
treeherdermozilla-central@769f29c92bb2 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersjimb, fitzgen
bugs1003554
milestone44.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 1003554 - make entry points correspond to entries in the line table; r=jimb,fitzgen
devtools/server/actors/script.js
js/src/doc/Debugger/Debugger.Script.md
js/src/jit-test/tests/debug/Frame-onStep-11.js
js/src/jit-test/tests/debug/Frame-onStep-12.js
js/src/jit-test/tests/debug/Script-getAllColumnOffsets-06.js
js/src/jit-test/tests/debug/Script-getOffsetLocation.js
js/src/vm/CommonPropertyNames.h
js/src/vm/Debugger.cpp
--- a/devtools/server/actors/script.js
+++ b/devtools/server/actors/script.js
@@ -809,16 +809,27 @@ ThreadActor.prototype = {
         return pauseAndRespond(this);
       };
     }
 
     // Otherwise take what a "step" means into consideration.
     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));
 
       // Cases when we should pause because we have executed enough to consider
       // a "step" to have occured:
       //
       // 1.1. We change frames.
--- a/js/src/doc/Debugger/Debugger.Script.md
+++ b/js/src/doc/Debugger/Debugger.Script.md
@@ -213,16 +213,19 @@ methods of other kinds of objects.
 :   Return an object describing the source code location responsible for the
     bytecode at <i>offset</i> in this script.  The object has the
     following properties:
 
     * lineNumber: the line number for which offset is an entry point
 
     * 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
     a single opcode, and contains the following properties:
 
     * lineNumber: the line number of the current opcode.
 
     * columnNumber: the column number of the current opcode.
new file mode 100644
--- /dev/null
+++ b/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");
new file mode 100644
--- /dev/null
+++ b/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");
--- a/js/src/jit-test/tests/debug/Script-getAllColumnOffsets-06.js
+++ b/js/src/jit-test/tests/debug/Script-getAllColumnOffsets-06.js
@@ -9,19 +9,20 @@ Debugger(global).onDebuggerStatement = f
                 assertEq(offset.lineNumber, 1);
                 global.log += offset.columnNumber + " ";
             }
         });
     });
 };
 
 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
 // 25 - Inner print()
 // 19 - Outer print()
 // 37 - new Error()
 // 48 - Exit the function body
 assertEq(global.log, "14 25 19 37 48 ");
--- a/js/src/jit-test/tests/debug/Script-getOffsetLocation.js
+++ b/js/src/jit-test/tests/debug/Script-getOffsetLocation.js
@@ -1,24 +1,34 @@
 // getOffsetLocation agrees with getAllColumnOffsets
 
 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;
+        byOffset[offset] = {lineNumber, columnNumber};
+    });
+
+    frame.onStep = function() {
+        var offset = frame.offset;
         var location = script.getOffsetLocation(offset);
-        assertEq(location.lineNumber, lineNumber);
-        assertEq(location.columnNumber, columnNumber);
-    });
+        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);
 }
 
 test("for (var i = 0; i < n; ++i) ;");
 test("var w0,x1=3,y2=4,z3=9");
 test("print(n),print(n),print(n)");
 test("var o={a:1,b:2,c:3}");
 test("var a=[1,2,n]");
--- a/js/src/vm/CommonPropertyNames.h
+++ b/js/src/vm/CommonPropertyNames.h
@@ -119,16 +119,17 @@
     macro(isNaN, isNaN, "isNaN") \
     macro(isPrototypeOf, isPrototypeOf, "isPrototypeOf") \
     macro(iterate, iterate, "iterate") \
     macro(Infinity, Infinity, "Infinity") \
     macro(InterpretGeneratorResume, InterpretGeneratorResume, "InterpretGeneratorResume") \
     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") \
     macro(keys, keys, "keys") \
     macro(label, label, "label") \
     macro(lastIndex, lastIndex, "lastIndex") \
     macro(LegacyGeneratorCloseInternal, LegacyGeneratorCloseInternal, "LegacyGeneratorCloseInternal") \
     macro(length, length, "length") \
--- a/js/src/vm/Debugger.cpp
+++ b/js/src/vm/Debugger.cpp
@@ -4851,63 +4851,81 @@ class BytecodeRangeWithPosition : privat
   public:
     using BytecodeRange::empty;
     using BytecodeRange::frontPC;
     using BytecodeRange::frontOpcode;
     using BytecodeRange::frontOffset;
 
     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;
 };
 
 /*
  * FlowGraphSummary::populate(cx, script) computes a summary of script's
  * control flow graph used by DebuggerScript_{getAllOffsets,getLineOffsets}.
  *
  * An instruction on a given line is an entry point for that line if it can be
  * reached from (an instruction on) a different line. We distinguish between the
@@ -5078,16 +5096,20 @@ DebuggerScript_getOffsetLocation(JSConte
 {
     THIS_DEBUGSCRIPT_SCRIPT(cx, argc, vp, "getOffsetLocation", args, obj, script);
     if (!args.requireAtLeast(cx, "Debugger.Script.getOffsetLocation", 1))
         return false;
     size_t offset;
     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;
 
     BytecodeRangeWithPosition r(cx, script);
     while (!r.empty() && r.frontOffset() < offset)
         r.popFront();
 
@@ -5095,16 +5117,25 @@ DebuggerScript_getOffsetLocation(JSConte
     RootedValue value(cx, NumberValue(r.frontLineNumber()));
     if (!DefineProperty(cx, result, id, value))
         return false;
 
     value = NumberValue(r.frontColumnNumber());
     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;
 }
 
 static bool
 DebuggerScript_getAllOffsets(JSContext* cx, unsigned argc, Value* vp)
 {
     THIS_DEBUGSCRIPT_SCRIPT(cx, argc, vp, "getAllOffsets", args, obj, script);
@@ -5117,16 +5148,19 @@ DebuggerScript_getAllOffsets(JSContext* 
     if (!flowData.populate(cx, script))
         return false;
 
     /* Second pass: build the result array. */
     RootedObject result(cx, NewDenseEmptyArray(cx));
     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();
 
         /* Make a note, if the current instruction is an entry point for the current line. */
         if (!flowData[offset].hasNoEdges() && flowData[offset].lineno() != lineno) {
             /* Get the offsets array for this line. */
             RootedObject offsets(cx);
             RootedValue offsetsv(cx);
@@ -5190,17 +5224,18 @@ DebuggerScript_getAllColumnOffsets(JSCon
     if (!result)
         return false;
     for (BytecodeRangeWithPosition r(cx, script); !r.empty(); r.popFront()) {
         size_t lineno = r.frontLineNumber();
         size_t column = r.frontColumnNumber();
         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));
             if (!entry)
                 return false;
 
             RootedId id(cx, NameToId(cx->names().lineNumber));
             RootedValue value(cx, NumberValue(lineno));
@@ -5254,16 +5289,19 @@ DebuggerScript_getLineOffsets(JSContext*
     if (!flowData.populate(cx, script))
         return false;
 
     /* Second pass: build the result array. */
     RootedObject result(cx, NewDenseEmptyArray(cx));
     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. */
         if (r.frontLineNumber() == lineno &&
             !flowData[offset].hasNoEdges() &&
             flowData[offset].lineno() != lineno)
         {
             if (!NewbornArrayPush(cx, result, NumberValue(offset)))