Closed Bug 1016523 Opened 5 years ago Closed 5 years ago

[jsdbg2] Consider invoking the interrupt handler a Debugger "step"

Categories

(Core :: JavaScript Engine, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla32

People

(Reporter: shu, Assigned: shu)

Details

Attachments

(4 files, 5 obsolete files)

The current implementation of slow script debugging sets an onStep on the youngest frame and expects that onStep to be called before we time out again. It is possible to time out and hit the interrupt handler in a long-running primitive, like Array.sort. If we time out in one of those, we never complete a "step" before we time out again.

Debugger should just consider invoking the interrupt handler itself a step, and call onStep directly after calling the handler.
Attached patch Part 2: JIT parts. (obsolete) — Splinter Review
Comment on attachment 8429538 [details] [diff] [review]
Part 1: Have Debugger treat invoking the interrupt handler as a step in the interpreter.

Review of attachment 8429538 [details] [diff] [review]:
-----------------------------------------------------------------

I'm unclear as to how we can be confident that the new magic value won't be mishandled.

Reasoning by analogy with JS_GENERATOR_CLOSING seems questionable: that is only established as an exception value while we force a generator to exit early. We know that the only continuation that will see JS_GENERATOR_CLOSING is the HandleError path in Interpreter.cpp. In the JS_DEBUGGER_FORCED_RETURN case, what do we know about the continuations that could see it?

Would it make sense to have, say JS_GetPendingException assert that it's never going to return a magic value?

Perhaps the way to test this would be a TestingFunctions.cpp primitive that simply requests an interrupt, calls CheckForInterrupt itself, and then checks that the interrupt request has been cleared. That could be used to exercise any kind of interrupt-handler behavior.

::: js/public/Value.h
@@ +225,5 @@
>                                    * to JS_EnumerateState, which really means the object can be
>                                    * enumerated like a native object. */
>      JS_NO_ITER_VALUE,            /* there is not a pending iterator value */
>      JS_GENERATOR_CLOSING,        /* exception value thrown when closing a generator */
> +    JS_DEBUGGER_FORCED_RETURN,   /* used by Debugger to propagate forced returns from interrupts */

You'll need to fix the comment above 'class Value' to mention that cx->exception can have values other than JS_GENERATOR_CLOSING.
(In reply to Jim Blandy :jimb from comment #3)
> Comment on attachment 8429538 [details] [diff] [review]
> Part 1: Have Debugger treat invoking the interrupt handler as a step in the
> interpreter.
> 
> Review of attachment 8429538 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I'm unclear as to how we can be confident that the new magic value won't be
> mishandled.
> 
> Reasoning by analogy with JS_GENERATOR_CLOSING seems questionable: that is
> only established as an exception value while we force a generator to exit
> early. We know that the only continuation that will see JS_GENERATOR_CLOSING
> is the HandleError path in Interpreter.cpp. In the JS_DEBUGGER_FORCED_RETURN
> case, what do we know about the continuations that could see it?

Well, in general, we don't. By auditing the code as it is now, I know that to be true. The only other continuation is the Baseline case, which is addressed in part 2.

The alternative would be to use a separate boolean flag on the JSContext. In that case, propagating a forced return will be like an uncatchable exception. Maybe that is even cleaner -- I'll write that up and post it for comparison.
Attached patch Part 2: JIT parts. (obsolete) — Splinter Review
No longer uses a magic exception value, but a separate flag on JSContext.
Attachment #8429538 - Attachment is obsolete: true
Attachment #8429539 - Attachment is obsolete: true
Attachment #8429538 - Flags: review?(jimb)
Attachment #8429737 - Flags: review?(jimb)
Attached patch Test. (obsolete) — Splinter Review
Attachment #8429739 - Flags: review?(jimb)
Comment on attachment 8429737 [details] [diff] [review]
Part 1: Have Debugger treat invoking the interrupt handler as a step in the interpreter.

Review of attachment 8429737 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/vm/Debugger.cpp
@@ +3684,5 @@
> +    // return: ... } resumption values straightforwardly from the interrupt
> +    // handler. Instead, we set the intended return value in the frame's rval
> +    // slot and set the pending exception as the magic value
> +    // JS_DEBUGGER_FORCED_RETURN. Exception handlers then check for this and
> +    // treat it as a forced return.

Oops, this comment is outdated from the previous version of the patch.
Comment on attachment 8429739 [details] [diff] [review]
Test.

Review of attachment 8429739 [details] [diff] [review]:
-----------------------------------------------------------------

Do we need to do anything specific to make sure this covers both of the spots that check isPropagatingForcedReturn, in the interpreter and in the baseline JIT code?

::: js/src/jit-test/tests/debug/Frame-onStep-resumption-05.js
@@ +2,5 @@
> +
> +function testResumptionVal(resumptionVal) {
> +  var g = newGlobal();
> +  var dbg = new Debugger;
> +  g.onStepHits = 0;

Instead of onStepHits, the tighter way to check behavior like this is to say:

at this point:                                          g.log = '';
in the function passed to invokeInterruptHandler:       log += 'f';
in the interrupt callback:                              g.log += 'i';
in the onStep handler:                                  g.log += 's';
and in a finally clause around the final g.eval:        assertEq(g.log, 'isf')

That way, you'll get a test failure even if invokeInterruptHandler never calls its function.

@@ +10,5 @@
> +      assertEq(onStepHits, 1);
> +      assertEq(interruptRv, resumptionVal == undefined);
> +    });
> +    return 42;
> +  });

This code were simply passed to g.eval after the setInterruptCallback call, instead of defining f above and then calling it below.

::: js/src/shell/js.cpp
@@ +3451,5 @@
> +            if (!cx->getPendingException(&exception))
> +                return false;
> +            restoreException = true;
> +            cx->clearPendingException();
> +        } else if (!cx->compartment()->debugMode() || !cx->isPropagatingForcedReturn()) {

Why do we check cx->compartment()->debugMode() before checking cx->isPropagatingForcedReturn()? When a handler takes a compartment out of debug mode, isn't that handler's resumption value still supposed to be respected?
Comment on attachment 8429739 [details] [diff] [review]
Test.

Review of attachment 8429739 [details] [diff] [review]:
-----------------------------------------------------------------

Do we need to do anything specific to make sure this covers both of the spots that check isPropagatingForcedReturn, in the interpreter and in the baseline JIT code?

::: js/src/jit-test/tests/debug/Frame-onStep-resumption-05.js
@@ +2,5 @@
> +
> +function testResumptionVal(resumptionVal) {
> +  var g = newGlobal();
> +  var dbg = new Debugger;
> +  g.onStepHits = 0;

Instead of onStepHits, the tighter way to check behavior like this is to say:

at this point:                                          g.log = '';
in the function passed to invokeInterruptHandler:       log += 'f';
in the interrupt callback:                              g.log += 'i';
in the onStep handler:                                  g.log += 's';
and in a finally clause around the final g.eval:        assertEq(g.log, 'isf')

That way, you'll get a test failure even if invokeInterruptHandler never calls its function.

@@ +10,5 @@
> +      assertEq(onStepHits, 1);
> +      assertEq(interruptRv, resumptionVal == undefined);
> +    });
> +    return 42;
> +  });

This code were simply passed to g.eval after the setInterruptCallback call, instead of defining f above and then calling it below.

::: js/src/shell/js.cpp
@@ +3451,5 @@
> +            if (!cx->getPendingException(&exception))
> +                return false;
> +            restoreException = true;
> +            cx->clearPendingException();
> +        } else if (!cx->compartment()->debugMode() || !cx->isPropagatingForcedReturn()) {

Why do we check cx->compartment()->debugMode() before checking cx->isPropagatingForcedReturn()? When a handler takes a compartment out of debug mode, isn't that handler's resumption value still supposed to be respected?
Attachment #8429739 - Flags: review?(jimb)
I don't know why Splinter double-posted my comments. I didn't make any changes, so you can skip comment 10.
Comment on attachment 8429737 [details] [diff] [review]
Part 1: Have Debugger treat invoking the interrupt handler as a step in the interpreter.

Review of attachment 8429737 [details] [diff] [review]:
-----------------------------------------------------------------

With the one comment addressed, this looks great.

::: js/src/jscntxt.cpp
@@ +1183,5 @@
>  
> +bool
> +JSContext::isPropagatingForcedReturn() const
> +{
> +    MOZ_ASSERT(compartment()->debugMode());

Forced returns should be respected even if the handler takes the compartment out of debug mode.
Attachment #8429737 - Flags: review?(jimb) → review+
(In reply to Jim Blandy :jimb from comment #9)
> Comment on attachment 8429739 [details] [diff] [review]
> Test.
> 
> Review of attachment 8429739 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Do we need to do anything specific to make sure this covers both of the
> spots that check isPropagatingForcedReturn, in the interpreter and in the
> baseline JIT code?

In the test? No -- tbpl will take care of running a copy with --baseline-eager.

> 
> ::: js/src/jit-test/tests/debug/Frame-onStep-resumption-05.js
> @@ +2,5 @@
> > +
> > +function testResumptionVal(resumptionVal) {
> > +  var g = newGlobal();
> > +  var dbg = new Debugger;
> > +  g.onStepHits = 0;
> 
> Instead of onStepHits, the tighter way to check behavior like this is to say:
> 
> at this point:                                          g.log = '';
> in the function passed to invokeInterruptHandler:       log += 'f';
> in the interrupt callback:                              g.log += 'i';
> in the onStep handler:                                  g.log += 's';
> and in a finally clause around the final g.eval:        assertEq(g.log,
> 'isf')
> 
> That way, you'll get a test failure even if invokeInterruptHandler never
> calls its function.
> 

Ah okay, good suggestion.

> @@ +10,5 @@
> > +      assertEq(onStepHits, 1);
> > +      assertEq(interruptRv, resumptionVal == undefined);
> > +    });
> > +    return 42;
> > +  });
> 
> This code were simply passed to g.eval after the setInterruptCallback call,
> instead of defining f above and then calling it below.
> 
> ::: js/src/shell/js.cpp
> @@ +3451,5 @@
> > +            if (!cx->getPendingException(&exception))
> > +                return false;
> > +            restoreException = true;
> > +            cx->clearPendingException();
> > +        } else if (!cx->compartment()->debugMode() || !cx->isPropagatingForcedReturn()) {
> 
> Why do we check cx->compartment()->debugMode() before checking
> cx->isPropagatingForcedReturn()? When a handler takes a compartment out of
> debug mode, isn't that handler's resumption value still supposed to be
> respected?

Good catch. Forgot that resumption values need to always be respected.
Assignee: nobody → shu
Status: NEW → ASSIGNED
Attached patch Part 2: JIT parts. (obsolete) — Splinter Review
Attachment #8429736 - Attachment is obsolete: true
Attachment #8431233 - Flags: review?(jdemooij)
Attached patch Test.Splinter Review
Attachment #8429739 - Attachment is obsolete: true
Attachment #8431234 - Flags: review?(jimb)
Some cleanup
Attachment #8431233 - Attachment is obsolete: true
Attachment #8431233 - Flags: review?(jdemooij)
Attachment #8433669 - Flags: review?(jdemooij)
Attached patch Test.Splinter Review
Fix AutoSaveExceptionState
Attachment #8431234 - Attachment is obsolete: true
Attachment #8431234 - Flags: review?(jimb)
Attachment #8433709 - Flags: review?(jimb)
Attachment #8433669 - Flags: review?(jdemooij) → review+
Comment on attachment 8431234 [details] [diff] [review]
Test.

Review of attachment 8431234 [details] [diff] [review]:
-----------------------------------------------------------------

The test looks great.

::: js/src/shell/js.cpp
@@ +3427,5 @@
>      return true;
>  }
>  
>  static bool
> +InvokeInterruptHandlerWrapper(JSContext *cx, unsigned argc, jsval *vp)

It would be more consistent with the rest of the file for this function to be named "InvokeInterruptHandler". Is there a reason not to follow that convention?

Am I correct that this needs to be in js/src/shell/js.cpp and not TestingFunctions.cpp, because it needs gServiceInterrupt? If so, that's fine.

@@ +3442,5 @@
> +
> +    // The interrupt handler could have set a pending exception. Since we call
> +    // back into JS, don't have it see the pending exception. If we have an
> +    // uncatchable exception that's not propagating a debug mode forced
> +    // return, return.

Can we use JS::AutoSaveExceptionState here? I guess we'd return early if !interruptRv and !cx->isExceptionPending() and !cx->isPropagatingForcedReturn(), but then construct an AutoSaveExceptionState to protect the rest of the body.

Oddly, because we check for forced return first, the bug in AutoSaveExceptionState I mentioned in IRC would be unobservable here.
Attachment #8431234 - Attachment is obsolete: false
Comment on attachment 8433709 [details] [diff] [review]
Test.

Review of attachment 8433709 [details] [diff] [review]:
-----------------------------------------------------------------

Yes, all looks good. Thanks very much!
Attachment #8433709 - Flags: review?(jimb) → review+
You need to log in before you can comment on or make changes to this bug.