Bug 1491331: Check overridePc in collectJitStackScripts even if frame is not throwing r=tcampbell
authorIain Ireland <iireland@mozilla.com>
Fri, 19 Oct 2018 01:04:31 +0000
changeset 490787 e58c42a7b469627f21c75c25d39364fb7a4dc0a4
parent 490786 09a38fc0ac95612377bc130dd1241c8faa7edf91
child 490788 f092493f896a931f1eba48bb341797bd8885b249
push id247
push userfmarier@mozilla.com
push dateSat, 27 Oct 2018 01:06:44 +0000
reviewerstcampbell
bugs1491331
milestone64.0a1
Bug 1491331: Check overridePc in collectJitStackScripts even if frame is not throwing r=tcampbell Differential Revision: https://phabricator.services.mozilla.com/D9151
js/src/jit-test/tests/generators/bug1491331.js
js/src/jit/BaselineCompiler.cpp
js/src/jit/BaselineDebugModeOSR.cpp
js/src/jit/BaselineFrame.h
new file mode 100644
--- /dev/null
+++ b/js/src/jit-test/tests/generators/bug1491331.js
@@ -0,0 +1,11 @@
+let g = newGlobal();
+g.eval('function* f() { yield 123; }');
+
+let dbg = Debugger(g);
+dbg.onEnterFrame = frame => {
+    dbg.removeDebuggee(g);
+    dbg.addDebuggee(g);
+};
+
+let genObj = g.f();
+genObj.return();
--- a/js/src/jit/BaselineCompiler.cpp
+++ b/js/src/jit/BaselineCompiler.cpp
@@ -5172,26 +5172,33 @@ BaselineCompiler::emit_JSOP_RESUME()
         prepareVMCall();
         pushArg(Imm32(resumeKind));
         pushArg(retVal);
         pushArg(genObj);
         pushArg(scratch2);
 
         TrampolinePtr code = cx->runtime()->jitRuntime()->getVMWrapper(GeneratorThrowOrReturnInfo);
 
-        // Create the frame descriptor.
+        // Create and push the frame descriptor.
         masm.subStackPtrFrom(scratch1);
         masm.makeFrameDescriptor(scratch1, FrameType::BaselineJS, ExitFrameLayout::Size());
-
-        // Push the frame descriptor and a dummy return address (it doesn't
-        // matter what we push here, frame iterators will use the frame pc
-        // set in jit::GeneratorThrowOrReturn).
         masm.push(scratch1);
 
-        // On ARM64, the callee will push the return address.
+        // We have created a baseline frame as if we were the
+        // callee. However, if we just did a regular call at this
+        // point, our return address would be bogus: it would point at
+        // self-hosted code, instead of the generator code that we are
+        // pretending we are already executing. Instead, we push a
+        // dummy return address. In jit::GeneratorThrowOrReturn,
+        // we will set the baseline frame's overridePc. Frame iterators
+        // will use the override pc instead of relying on the return
+        // address.
+
+        // On ARM64, the callee will push a bogus return address. On
+        // other architectures, we push a null return address.
 #ifndef JS_CODEGEN_ARM64
         masm.push(ImmWord(0));
 #endif
         masm.jump(code);
     }
 
     // If the generator script has no JIT code, call into the VM.
     masm.bind(&interpret);
--- a/js/src/jit/BaselineDebugModeOSR.cpp
+++ b/js/src/jit/BaselineDebugModeOSR.cpp
@@ -201,23 +201,23 @@ CollectJitStackScripts(JSContext* cx, co
 
             if (BaselineDebugModeOSRInfo* info = baselineFrame->getDebugModeOSRInfo()) {
                 // If patching a previously patched yet unpopped frame, we can
                 // use the BaselineDebugModeOSRInfo on the frame directly to
                 // patch. Indeed, we cannot use frame.returnAddressToFp(), as
                 // it points into the debug mode OSR handler and cannot be
                 // used to look up a corresponding ICEntry.
                 //
-                // See cases F and G in PatchBaselineFramesForDebugMode.
+                // See case F in PatchBaselineFramesForDebugMode.
                 if (!entries.append(DebugModeOSREntry(script, info))) {
                     return false;
                 }
-            } else if (baselineFrame->isHandlingException()) {
-                // We are in the middle of handling an exception and the frame
-                // must have an override pc.
+            } else if (baselineFrame->hasOverridePc()) {
+                // If the frame is not settled on a pc with an ICEntry, overridePc
+                // will contain an explicit bytecode offset. We can (and must) use that.
                 uint32_t offset = script->pcToOffset(baselineFrame->overridePc());
                 if (!entries.append(DebugModeOSREntry(script, offset))) {
                     return false;
                 }
             } else {
                 // The frame must be settled on a pc with an ICEntry.
                 uint8_t* retAddr = frame.returnAddressToFp();
                 ICEntry& icEntry = script->baselineScript()->icEntryFromReturnAddress(retAddr);
@@ -363,26 +363,27 @@ PatchBaselineFramesForDebugMode(JSContex
     //
     // When toggling debug mode with live baseline scripts on the stack, we
     // could have entered the VM via the following ways from the baseline
     // script.
     //
     // Off to On:
     //  A. From a "can call" stub.
     //  B. From a VM call.
-    //  H. From inside HandleExceptionBaseline.
+    //  H. From inside HandleExceptionBaseline
     //  I. From inside the interrupt handler via the prologue stack check.
     //  J. From the warmup counter in the prologue.
     //
     // On to Off:
     //  - All the ways above.
     //  C. From the debug trap handler.
     //  D. From the debug prologue.
+    //  E. From the debug epilogue.
+    //  G. From GeneratorThrowOrReturn
     //  K. From a JSOP_DEBUGAFTERYIELD instruction.
-    //  E. From the debug epilogue.
     //
     // Cycles (On to Off to On)+ or (Off to On to Off)+:
     //  F. Undo cases B, C, D, E, I or J above on previously patched yet unpopped
     //     frames.
     //
     // In general, we patch the return address from the VM call to return to a
     // "continuation fixer" to fix up machine state (registers and stack
     // state). Specifics on what needs to be done are documented below.
@@ -433,29 +434,28 @@ PatchBaselineFramesForDebugMode(JSContex
                 DebugModeOSRVolatileJitFrameIter::forwardLiveIterators(
                     cx, prev->returnAddress(), retAddr);
                 prev->setReturnAddress(retAddr);
                 entryIndex++;
                 break;
             }
 
             if (kind == ICEntry::Kind_Invalid) {
-                // Case H above.
+                // Cases G and H above.
                 //
-                // We are recompiling on-stack scripts from inside the
-                // exception handler, by way of an onExceptionUnwind
-                // invocation, on a pc without an ICEntry. This means the
-                // frame must have an override pc.
+                // We are recompiling a frame with an override pc.
+                // This may occur from inside the exception handler,
+                // by way of an onExceptionUnwind invocation, on a pc
+                // without an ICEntry. It may also happen if we call
+                // GeneratorThrowOrReturn and trigger onEnterFrame.
                 //
                 // If profiling is off, patch the resume address to nullptr,
                 // to ensure the old address is not used anywhere.
-                //
                 // If profiling is on, JSJitProfilingFrameIterator requires a
                 // valid return address.
-                MOZ_ASSERT(frame.baselineFrame()->isHandlingException());
                 MOZ_ASSERT(frame.baselineFrame()->overridePc() == pc);
                 uint8_t* retAddr;
                 if (cx->runtime()->geckoProfiler().enabled()) {
                     retAddr = bl->nativeCodeForPC(script, pc);
                 } else {
                     retAddr = nullptr;
                 }
                 SpewPatchBaselineFrameFromExceptionHandler(prev->returnAddress(), retAddr,
@@ -690,17 +690,17 @@ RecompileBaselineScriptForDebugMode(JSCo
 
     // If a script is on the stack multiple times, it may have already
     // been recompiled.
     if (oldBaselineScript->hasDebugInstrumentation() == observing) {
         return true;
     }
 
     JitSpew(JitSpew_BaselineDebugModeOSR, "Recompiling (%s:%u:%u) for %s",
-            script->filename(), script->lineno(), script->column(), 
+            script->filename(), script->lineno(), script->column(),
             observing ? "DEBUGGING" : "NORMAL EXECUTION");
 
     AutoKeepTypeScripts keepTypes(cx);
     script->setBaselineScript(cx->runtime(), nullptr);
 
     MethodStatus status = BaselineCompile(cx, script, /* forceDebugMode = */ observing);
     if (status != Method_Compiled) {
         // We will only fail to recompile for debug mode due to OOM. Restore
--- a/js/src/jit/BaselineFrame.h
+++ b/js/src/jit/BaselineFrame.h
@@ -60,17 +60,19 @@ class BaselineFrame
         // This flag is intended for use whenever the frame is settled on a
         // native code address without a corresponding ICEntry. In this case,
         // the frame contains an explicit bytecode offset for frame iterators.
         //
         // There can also be an override pc if the frame has had its
         // environment chain unwound to a pc during exception handling that is
         // different from its current pc.
         //
-        // This flag should never be set when we're executing JIT code.
+        // This flag should never be set on the top frame while we're
+        // executing JIT code. In debug mode, it is checked before and
+        // after VM calls.
         HAS_OVERRIDE_PC = 1 << 11,
 
         // If set, we're handling an exception for this frame. This is set for
         // debug mode OSR sanity checking when it handles corner cases which
         // only arise during exception handling.
         HANDLING_EXCEPTION = 1 << 12,
     };