Bug 1013219 - set line number of return instruction; r=efaust, r=fitzgen, r=ejpbruel
authorTom Tromey <tromey@mozilla.com>
Mon, 28 Mar 2016 12:20:00 +0200
changeset 290852 1b45c030f024b195ad68b3680306e873bc11deb1
parent 290851 5777c32d2b391e43d9e5c23fea0c57324e125d72
child 290853 55a506e5ae4ed3dc008be28a812ca4c0e45be7be
push id19656
push usergwagner@mozilla.com
push dateMon, 04 Apr 2016 13:43:23 +0000
treeherderb2g-inbound@e99061fde28a [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersefaust, fitzgen, ejpbruel
bugs1013219
milestone48.0a1
Bug 1013219 - set line number of return instruction; r=efaust, r=fitzgen, r=ejpbruel
devtools/client/debugger/test/mochitest/browser_dbg_split-console-keypress.js
devtools/client/debugger/test/mochitest/browser_dbg_step-out.js
devtools/server/tests/unit/setBreakpoint-on-column-with-no-offsets-at-end-of-script.js
devtools/server/tests/unit/setBreakpoint-on-line-with-no-offsets-at-end-of-script.js
devtools/server/tests/unit/test_setBreakpoint-on-line-with-no-offsets-at-end-of-script.js
devtools/server/tests/unit/test_stepping-06.js
devtools/server/tests/unit/test_stepping-07.js
devtools/server/tests/unit/xpcshell.ini
js/src/frontend/BytecodeCompiler.cpp
js/src/frontend/BytecodeEmitter.cpp
js/src/frontend/BytecodeEmitter.h
js/src/jit-test/tests/debug/Frame-onStep-16.js
js/src/jit-test/tests/debug/Frame-onStep-lines-01.js
--- a/devtools/client/debugger/test/mochitest/browser_dbg_split-console-keypress.js
+++ b/devtools/client/debugger/test/mochitest/browser_dbg_split-console-keypress.js
@@ -38,23 +38,23 @@ function test() {
     // The console is now open (if not make the test fail already)
     ok(gToolbox.splitConsole, "Split console is shown.");
 
     // Information for sub-tests. When 'key' is synthesized 'keyRepeat' times,
     // cursor should be at 'caretLine' of this test..
     let stepTests = [
       {key: 'VK_F11', keyRepeat: 1, caretLine: 16},
       {key: 'VK_F11', keyRepeat: 2, caretLine: 18},
-      {key: 'VK_F11', keyRepeat: 2, caretLine: 26},
-      {key: 'VK_F10', keyRepeat: 1, caretLine: 18},
-      {key: 'VK_F11', keyRepeat: 1, caretLine: 19},
-      {key: 'VK_F11', keyRepeat: 5, caretLine: 29},
-      {key: 'VK_F11', modifier:'Shift', keyRepeat: 1, caretLine: 32},
-      {key: 'VK_F11', modifier:'Shift', keyRepeat: 2, caretLine: 32},
-      {key: 'VK_F11', modifier:'Shift', keyRepeat: 2, caretLine: 20}
+      {key: 'VK_F11', keyRepeat: 2, caretLine: 27},
+      {key: 'VK_F10', keyRepeat: 1, caretLine: 27},
+      {key: 'VK_F11', keyRepeat: 1, caretLine: 18},
+      {key: 'VK_F11', keyRepeat: 5, caretLine: 32},
+      {key: 'VK_F11', modifier:'Shift', keyRepeat: 1, caretLine: 29},
+      {key: 'VK_F11', modifier:'Shift', keyRepeat: 2, caretLine: 34},
+      {key: 'VK_F11', modifier:'Shift', keyRepeat: 2, caretLine: 34}
     ];
     // Trigger script that stops at debugger statement
     executeSoon(() => generateMouseClickInTab(gTab,
       "content.document.getElementById('start')"));
     yield waitForPause(gThreadClient);
 
     // Focus the console and add event listener to track whether it loses focus
     // (Must happen after generateMouseClickInTab() call)
--- a/devtools/client/debugger/test/mochitest/browser_dbg_step-out.js
+++ b/devtools/client/debugger/test/mochitest/browser_dbg_step-out.js
@@ -20,17 +20,17 @@ function test() {
     gVars = gDebugger.DebuggerView.Variables;
 
     testNormalReturn();
   });
 }
 
 function testNormalReturn() {
   waitForSourceAndCaretAndScopes(gPanel, ".html", 17).then(() => {
-    waitForCaretAndScopes(gPanel, 19).then(() => {
+    waitForCaretAndScopes(gPanel, 20).then(() => {
       let innerScope = gVars.getScopeAtIndex(0);
       let returnVar = innerScope.get("<return>");
 
       is(returnVar.name, "<return>",
         "Should have the right property name for the returned value.");
       is(returnVar.value, 10,
         "Should have the right property value for the returned value.");
       ok(returnVar._internalItem, "Should be an internal item");
deleted file mode 100644
--- a/devtools/server/tests/unit/setBreakpoint-on-column-with-no-offsets-at-end-of-script.js
+++ /dev/null
@@ -1,5 +0,0 @@
-"use strict";
-
-function f() {
-  function g() { var a = 1; var b = 2; return; } g();
-}
deleted file mode 100644
--- a/devtools/server/tests/unit/setBreakpoint-on-line-with-no-offsets-at-end-of-script.js
+++ /dev/null
@@ -1,12 +0,0 @@
-"use strict";
-
-function f() {
-  function g() {
-    var a = 1;
-    var b = 2;
-    return;
-
-  }
-
-  g();
-}
deleted file mode 100644
--- a/devtools/server/tests/unit/test_setBreakpoint-on-line-with-no-offsets-at-end-of-script.js
+++ /dev/null
@@ -1,53 +0,0 @@
-"use strict";
-
-var SOURCE_URL = getFileUrl("setBreakpoint-on-line-with-no-offsets-at-end-of-script.js");
-
-function run_test() {
-  return Task.spawn(function* () {
-    do_test_pending();
-
-    DebuggerServer.registerModule("xpcshell-test/testactors");
-    DebuggerServer.init(() => true);
-
-    let global = createTestGlobal("test");
-    DebuggerServer.addTestGlobal(global);
-
-    let client = new DebuggerClient(DebuggerServer.connectPipe());
-    yield connect(client);
-
-    let { tabs } = yield listTabs(client);
-    let tab = findTab(tabs, "test");
-    let [, tabClient] = yield attachTab(client, tab);
-    let [, threadClient] = yield attachThread(tabClient);
-    yield resume(threadClient);
-
-    let promise = waitForNewSource(threadClient, SOURCE_URL);
-    loadSubScript(SOURCE_URL, global);
-    let { source } = yield promise;
-    let sourceClient = threadClient.source(source);
-
-    let location = { line: 8 };
-    let [packet, breakpointClient] = yield setBreakpoint(sourceClient, location);
-    do_check_false(packet.isPending); // NOTE: Change this when bug 1148356 lands
-    do_check_true("actualLocation" in packet);
-    let actualLocation = packet.actualLocation;
-    do_check_eq(actualLocation.line, 11);
-
-    packet = yield executeOnNextTickAndWaitForPause(function () {
-      Cu.evalInSandbox("f()", global);
-    }, client);
-    do_check_eq(packet.type, "paused");
-    let why = packet.why;
-    do_check_eq(why.type, "breakpoint");
-    do_check_eq(why.actors.length, 1);
-    do_check_eq(why.actors[0], breakpointClient.actor);
-    let where = packet.frame.where;
-    do_check_eq(where.source.actor, source.actor);
-    do_check_eq(where.line, actualLocation.line);
-
-    yield resume(threadClient);
-    yield close(client);
-
-    do_test_finished();
-  });
-}
--- a/devtools/server/tests/unit/test_stepping-06.js
+++ b/devtools/server/tests/unit/test_stepping-06.js
@@ -38,17 +38,17 @@ function run_test_with_server(aServer, a
 }
 
 function test_simple_stepping()
 {
   gThreadClient.addOneTimeListener("paused", function (aEvent, aPacket) {
     gThreadClient.addOneTimeListener("paused", function (aEvent, aPacket) {
       // Check that the return value is 10.
       do_check_eq(aPacket.type, "paused");
-      do_check_eq(aPacket.frame.where.line, gDebuggee.line0 + 4);
+      do_check_eq(aPacket.frame.where.line, gDebuggee.line0 + 5);
       do_check_eq(aPacket.why.type, "resumeLimit");
       do_check_eq(aPacket.why.frameFinished.return, 10);
 
       gThreadClient.addOneTimeListener("paused", function (aEvent, aPacket) {
         gThreadClient.addOneTimeListener("paused", function (aEvent, aPacket) {
           // Check that the return value is undefined.
           do_check_eq(aPacket.type, "paused");
           do_check_eq(aPacket.frame.where.line, gDebuggee.line0 + 8);
--- a/devtools/server/tests/unit/test_stepping-07.js
+++ b/devtools/server/tests/unit/test_stepping-07.js
@@ -49,18 +49,21 @@ const testSteppingAndReturns = Task.asyn
   const dbgStmt2 = yield resumeAndWaitForPause(gClient, threadClient);
   equal(dbgStmt2.frame.where.line, 12,
         "Should be at debugger statement on line 3")
 
   dumpn("Testing stepping with explicit return");
   const step3 = yield stepOver(gClient, threadClient);
   equal(step3.frame.where.line, 13, "Should step to line 13");
   const step4 = yield stepOver(gClient, threadClient);
-  equal(step4.frame.where.line, 13, "Should step out of the function from line 13");
-  ok(step4.why.frameFinished, "This should be the explicit function return");
+  equal(step4.frame.where.line, 15, "Should step out of the function from line 15");
+  // This step is a bit funny, see bug 1013219 for details.
+  const step5 = yield stepOver(gClient, threadClient);
+  equal(step5.frame.where.line, 15, "Should step out of the function from line 15");
+  ok(step5.why.frameFinished, "This should be the explicit function return");
 
   finishClient(gClient, gCallback);
 });
 
 function evaluateTestCode() {
   Cu.evalInSandbox(
     `                                   //  1
     function implicitReturn() {         //  2
--- a/devtools/server/tests/unit/xpcshell.ini
+++ b/devtools/server/tests/unit/xpcshell.ini
@@ -18,24 +18,22 @@ support-files =
   registertestactors-03.js
   sourcemapped.js
   testactors.js
   hello-actor.js
   setBreakpoint-on-column.js
   setBreakpoint-on-column-in-gcd-script.js
   setBreakpoint-on-column-with-no-offsets.js
   setBreakpoint-on-column-with-no-offsets-at-end-of-line.js
-  setBreakpoint-on-column-with-no-offsets-at-end-of-script.js
   setBreakpoint-on-column-with-no-offsets-in-gcd-script.js
   setBreakpoint-on-line.js
   setBreakpoint-on-line-in-gcd-script.js
   setBreakpoint-on-line-with-multiple-offsets.js
   setBreakpoint-on-line-with-multiple-statements.js
   setBreakpoint-on-line-with-no-offsets.js
-  setBreakpoint-on-line-with-no-offsets-at-end-of-script.js
   setBreakpoint-on-line-with-no-offsets-in-gcd-script.js
 
 [test_animation_name.js]
 [test_animation_type.js]
 [test_ScriptStore.js]
 [test_actor-registry-actor.js]
 [test_nesting-01.js]
 [test_nesting-02.js]
@@ -262,12 +260,11 @@ reason = bug 1014071
 [test_setBreakpoint-on-column.js]
 [test_setBreakpoint-on-column-in-gcd-script.js]
 [test_setBreakpoint-on-column-with-no-offsets-at-end-of-line.js]
 [test_setBreakpoint-on-line.js]
 [test_setBreakpoint-on-line-in-gcd-script.js]
 [test_setBreakpoint-on-line-with-multiple-offsets.js]
 [test_setBreakpoint-on-line-with-multiple-statements.js]
 [test_setBreakpoint-on-line-with-no-offsets.js]
-[test_setBreakpoint-on-line-with-no-offsets-at-end-of-script.js]
 [test_setBreakpoint-on-line-with-no-offsets-in-gcd-script.js]
 [test_safe-getter.js]
 [test_client_close.js]
--- a/js/src/frontend/BytecodeCompiler.cpp
+++ b/js/src/frontend/BytecodeCompiler.cpp
@@ -341,22 +341,26 @@ BytecodeCompiler::handleParseFailure(con
     return true;
 }
 
 bool
 BytecodeCompiler::prepareAndEmitTree(ParseNode** ppn)
 {
     if (!FoldConstants(cx, ppn, parser.ptr()) ||
         !NameFunctions(cx, *ppn) ||
-        !emitter->updateLocalsToFrameSlots() ||
-        !emitter->emitTree(*ppn))
+        !emitter->updateLocalsToFrameSlots())
     {
         return false;
     }
 
+    emitter->setFunctionBodyEndPos((*ppn)->pn_pos);
+
+    if (!emitter->emitTree(*ppn))
+        return false;
+
     return true;
 }
 
 bool
 BytecodeCompiler::maybeSetDisplayURL(TokenStream& tokenStream)
 {
     if (tokenStream.hasDisplayURL()) {
         if (!scriptSource->setDisplayURL(cx, tokenStream.displayURL()))
@@ -841,17 +845,17 @@ frontend::CompileLazyFunction(JSContext*
      * We just pass false for insideNonGlobalEval and insideEval, because we
      * don't actually know whether we are or not.  The only consumer of those
      * booleans is TryConvertFreeName, and it has special machinery to avoid
      * doing bad things when a lazy function is inside eval.
      */
     MOZ_ASSERT(!options.forEval);
     BytecodeEmitter bce(/* parent = */ nullptr, &parser, pn->pn_funbox, script, lazy,
                         /* insideEval = */ false, /* evalCaller = */ nullptr,
-                        /* insideNonGlobalEval = */ false, options.lineno,
+                        /* insideNonGlobalEval = */ false, pn->pn_pos,
                         BytecodeEmitter::LazyFunction);
     if (!bce.init())
         return false;
 
     return bce.emitFunctionScript(pn->pn_body);
 }
 
 // Compile a JS function body, which might appear as the value of an event
--- a/js/src/frontend/BytecodeEmitter.cpp
+++ b/js/src/frontend/BytecodeEmitter.cpp
@@ -138,22 +138,37 @@ BytecodeEmitter::BytecodeEmitter(Bytecod
     typesetCount(0),
     hasSingletons(false),
     hasTryFinally(false),
     emittingForInit(false),
     emittingRunOnceLambda(false),
     insideEval(insideEval),
     insideNonGlobalEval(insideNonGlobalEval),
     insideModule(false),
-    emitterMode(emitterMode)
+    emitterMode(emitterMode),
+    functionBodyEndPosSet(false)
 {
     MOZ_ASSERT_IF(evalCaller, insideEval);
     MOZ_ASSERT_IF(emitterMode == LazyFunction, lazyScript);
 }
 
+BytecodeEmitter::BytecodeEmitter(BytecodeEmitter* parent,
+                                 Parser<FullParseHandler>* parser, SharedContext* sc,
+                                 HandleScript script, Handle<LazyScript*> lazyScript,
+                                 bool insideEval, HandleScript evalCaller,
+                                 bool insideNonGlobalEval, TokenPos bodyPosition,
+                                 EmitterMode emitterMode)
+    : BytecodeEmitter(parent, parser, sc, script, lazyScript, insideEval,
+                      evalCaller, insideNonGlobalEval,
+                      parser->tokenStream.srcCoords.lineNum(bodyPosition.begin),
+                      emitterMode)
+{
+    setFunctionBodyEndPos(bodyPosition);
+}
+
 bool
 BytecodeEmitter::init()
 {
     return atomIndices.ensureMap(cx);
 }
 
 bool
 BytecodeEmitter::updateLocalsToFrameSlots()
@@ -3593,16 +3608,17 @@ BytecodeEmitter::emitFunctionScript(Pars
     bool runOnce = isRunOnceLambda();
     if (runOnce) {
         switchToPrologue();
         if (!emit1(JSOP_RUNONCE))
             return false;
         switchToMain();
     }
 
+    setFunctionBodyEndPos(body->pn_pos);
     if (!emitTree(body))
         return false;
 
     if (!updateSourceCoordNotes(body->pn_pos.end))
         return false;
 
     if (sc->isFunctionBox()) {
         if (sc->asFunctionBox()->isGenerator()) {
@@ -3698,16 +3714,17 @@ BytecodeEmitter::emitModuleScript(ParseN
 
     ModuleBox* modulebox = sc->asModuleBox();
     MOZ_ASSERT(modulebox);
 
     // Link the module and the script to each other, so that StaticScopeIter
     // may walk the scope chain of currently compiling scripts.
     JSScript::linkToModuleFromEmitter(cx, script, modulebox);
 
+    setFunctionBodyEndPos(body->pn_pos);
     if (!emitTree(body))
         return false;
 
     // Always end the script with a JSOP_RETRVAL. Some other parts of the codebase
     // depend on this opcode, e.g. InterpreterRegs::setToEndOfScript.
     if (!emit1(JSOP_RETRVAL))
         return false;
 
@@ -6424,20 +6441,19 @@ BytecodeEmitter::emitFunction(ParseNode*
             Rooted<JSScript*> script(cx, JSScript::Create(cx, enclosingScope, false, options,
                                                           sourceObject,
                                                           funbox->bufStart, funbox->bufEnd));
             if (!script)
                 return false;
 
             script->bindings = funbox->bindings;
 
-            uint32_t lineNum = parser->tokenStream.srcCoords.lineNum(pn->pn_pos.begin);
             BytecodeEmitter bce2(this, parser, funbox, script, /* lazyScript = */ nullptr,
                                  insideEval, evalCaller,
-                                 insideNonGlobalEval, lineNum, emitterMode);
+                                 insideNonGlobalEval, pn->pn_pos, emitterMode);
             if (!bce2.init())
                 return false;
 
             /* We measured the max scope depth when we parsed the function. */
             if (!bce2.emitFunctionScript(pn->pn_body))
                 return false;
 
             if (funbox->isLikelyConstructorWrapper())
@@ -6798,16 +6814,23 @@ BytecodeEmitter::emitReturn(ParseNode* p
             return false;
     }
 
     if (sc->isFunctionBox() && sc->asFunctionBox()->isStarGenerator()) {
         if (!emitFinishIteratorResult(true))
             return false;
     }
 
+    // We know functionBodyEndPos is set because "return" is only
+    // valid in a function, and so we've passed through
+    // emitFunctionScript.
+    MOZ_ASSERT(functionBodyEndPosSet);
+    if (!updateSourceCoordNotes(functionBodyEndPos))
+        return false;
+
     /*
      * EmitNonLocalJumpFixup may add fixup bytecode to close open try
      * blocks having finally clauses and to exit intermingled let blocks.
      * We can't simply transfer control flow to our caller in that case,
      * because we must gosub to those finally clauses from inner to outer,
      * with the correct stack pointer (i.e., after popping any with,
      * for/in, etc., slots nested inside the finally's try).
      *
--- a/js/src/frontend/BytecodeEmitter.h
+++ b/js/src/frontend/BytecodeEmitter.h
@@ -227,26 +227,39 @@ struct BytecodeEmitter
          * Check the static scope chain of the root function for resolving free
          * variable accesses in the script.
          */
         LazyFunction
     };
 
     const EmitterMode emitterMode;
 
+    // The end location of a function body that is being emitted.
+    uint32_t functionBodyEndPos;
+    // Whether functionBodyEndPos was set.
+    bool functionBodyEndPosSet;
+
     /*
      * Note that BytecodeEmitters are magic: they own the arena "top-of-stack"
      * space above their tempMark points. This means that you cannot alloc from
      * tempLifoAlloc and save the pointer beyond the next BytecodeEmitter
      * destruction.
      */
     BytecodeEmitter(BytecodeEmitter* parent, Parser<FullParseHandler>* parser, SharedContext* sc,
                     HandleScript script, Handle<LazyScript*> lazyScript,
                     bool insideEval, HandleScript evalCaller,
                     bool insideNonGlobalEval, uint32_t lineNum, EmitterMode emitterMode = Normal);
+
+    // An alternate constructor that uses a TokenPos for the starting
+    // line and that sets functionBodyEndPos as well.
+    BytecodeEmitter(BytecodeEmitter* parent, Parser<FullParseHandler>* parser, SharedContext* sc,
+                    HandleScript script, Handle<LazyScript*> lazyScript,
+                    bool insideEval, HandleScript evalCaller,
+                    bool insideNonGlobalEval, TokenPos bodyPosition, EmitterMode emitterMode = Normal);
+
     bool init();
     bool updateLocalsToFrameSlots();
 
     StmtInfoBCE* innermostStmt() const { return stmtStack.innermost(); }
     StmtInfoBCE* innermostScopeStmt() const { return stmtStack.innermostScopeStmt(); }
     JSObject* innermostStaticScope() const;
     JSObject* blockScopeOfDef(Definition* dn) const {
         return parser->blockScopes[dn->pn_blockid];
@@ -297,16 +310,21 @@ struct BytecodeEmitter
     void switchToPrologue() { current = &prologue; }
     bool inPrologue() const { return current == &prologue; }
 
     SrcNotesVector& notes() const { return current->notes; }
     ptrdiff_t lastNoteOffset() const { return current->lastNoteOffset; }
     unsigned currentLine() const { return current->currentLine; }
     unsigned lastColumn() const { return current->lastColumn; }
 
+    void setFunctionBodyEndPos(TokenPos pos) {
+        functionBodyEndPos = pos.end;
+        functionBodyEndPosSet = true;
+    }
+
     bool reportError(ParseNode* pn, unsigned errorNumber, ...);
     bool reportStrictWarning(ParseNode* pn, unsigned errorNumber, ...);
     bool reportStrictModeError(ParseNode* pn, unsigned errorNumber, ...);
 
     // If pn contains a useful expression, return true with *answer set to true.
     // If pn contains a useless expression, return true with *answer set to
     // false. Return false on error.
     //
new file mode 100644
--- /dev/null
+++ b/js/src/jit-test/tests/debug/Frame-onStep-16.js
@@ -0,0 +1,34 @@
+// Stepping through a function with a return statement should pause on
+// the closing brace of the function.
+
+var g = newGlobal();
+var dbg = Debugger(g);
+var log;
+
+function test(fnStr) {
+  log = '';
+  g.eval(fnStr);
+
+  dbg.onDebuggerStatement = function(frame) {
+        frame.onStep = function() {
+      let {lineNumber, isEntryPoint} = frame.script.getOffsetLocation(frame.offset);
+      if (isEntryPoint) {
+        log += lineNumber + ' ';
+      }
+    };
+  };
+
+  g.eval("f(23);");
+}
+
+test("function f(x) {\n" +    // 1
+     "    debugger;\n" +      // 2
+     "    return 23 + x;\n" + // 3
+     "}\n");                  // 4
+assertEq(log, '3 4 ');
+
+test("function f(x) {\n" +    // 1
+     "    debugger;\n" +      // 2
+     "    return;\n" +        // 3
+     "}\n");                  // 4
+assertEq(log, '3 4 ');
--- a/js/src/jit-test/tests/debug/Frame-onStep-lines-01.js
+++ b/js/src/jit-test/tests/debug/Frame-onStep-lines-01.js
@@ -43,17 +43,17 @@ assertEq(Object.keys(offsets).length, 1)
 g.eval('t(10,20,30)');
 assertEq(Object.keys(offsets).length, 2);
 
 // This shouldn't stop at all. It's the frame that's in single-step mode,
 // not the script, so the prior execution of t in single-step mode should
 // have no effect on this one.
 doSingleStep = false;
 g.eval('t(0, 0, 0)');
-assertEq(Object.keys(offsets).length, 6);
+assertEq(Object.keys(offsets).length, 7);
 doSingleStep = true;
 
 // Single-step in an eval frame. This should reach every line but the
 // first.
 g.eval(
        'debugger;                        \n' +
        'var a=1, b=2, c=3;               \n' +
        'var x = a;                       \n' +