Jan for the JIT parts, Jim for the Debugger parts.

Comment on attachment 8522667 [details] [diff] [review] Make onDebuggerStatement able to trigger on non-debuggee frames. Review of attachment 8522667 [details] [diff] [review]: ----------------------------------------------------------------- Looks good. I assume we have jit-tests that fail if you make JSOP_DEBUGGER a no-op in Ion? ::: js/src/jit/Lowering.cpp @@ +4101,5 @@ > +bool > +LIRGenerator::visitDebugger(MDebugger *ins) > +{ > + LDebugger *lir = new(alloc()) LDebugger(tempFixed(CallTempReg0), tempFixed(CallTempReg1)); > + return assignSnapshot(lir, Bailout_Debugger) && add(lir, ins) && assignSafepoint(lir, ins); There's no callVM so you can remove the assignSafepoint. Also tempFixed(...) could be just temp() I think. ::: js/src/jit/MIR.h @@ +11841,5 @@ > +class MDebugger : public MNullaryInstruction > +{ > + MDebugger() { > + setGuard(); > + } The instruction does not override getAliasSet so it's considered effectful and won't be eliminated, even without the setGuard(). ::: js/src/vm/Debugger.cpp @@ +1910,1 @@ > Debugger::HookCount }; Just onEnterFrame? Nice.

(In reply to Jan de Mooij [:jandem] from comment #2) > Comment on attachment 8522667 [details] [diff] [review] > Make onDebuggerStatement able to trigger on non-debuggee frames. > > Review of attachment 8522667 [details] [diff] [review]: > ----------------------------------------------------------------- > > Looks good. I assume we have jit-tests that fail if you make JSOP_DEBUGGER a > no-op in Ion? > > ::: js/src/jit/Lowering.cpp > @@ +4101,5 @@ > > +bool > > +LIRGenerator::visitDebugger(MDebugger *ins) > > +{ > > + LDebugger *lir = new(alloc()) LDebugger(tempFixed(CallTempReg0), tempFixed(CallTempReg1)); > > + return assignSnapshot(lir, Bailout_Debugger) && add(lir, ins) && assignSafepoint(lir, ins); > > There's no callVM so you can remove the assignSafepoint. Also tempFixed(...) > could be just temp() I think. > I get regalloc asserts if I change those to just temp().

While testing I ran into some test failures in getting offsets and maintaining Debugger.Environment identity. Those are filed as bugs 1099444 and 1099446.

Comment on attachment 8522667 [details] [diff] [review] Make onDebuggerStatement able to trigger on non-debuggee frames. Review of attachment 8522667 [details] [diff] [review]: ----------------------------------------------------------------- Looks great - r=me, with comments addressed. I had no idea so much plumbing would be necessary for this. New MIR and LIR instructions?? ::: js/src/vm/Debugger.cpp @@ +1119,3 @@ > ScriptFrameIter iter(cx); > + if (iter.isIon() && !iter.ensureHasRematerializedFrame(cx)) > + return JSTRAP_ERROR; I think this ought to be 'return handleUncaughtException(ac, false)', for consistency. (If we only ever raise OOM, the effect should be the same.) @@ +1905,5 @@ > comp->setDebugObservesAllExecution(); > return updateExecutionObservability(cx, obs, Observing); > } > > +static const Debugger::Hook ObserveAllExecutionHooks[] = { Debugger::OnEnterFrame, Let's just remove this array entirely; it's silly now. ::: js/src/vm/Debugger.h @@ +530,5 @@ > + * encountered on the youngest JS frame on |cx|. Call whatever hooks have > + * been registered to observe this. > + * > + * Note that this method is called for all |debugger;| statements, > + * regardless of the frame's debuggee-ness. Please specifically mention Ion frames here. Thanks very much for consolidating the comments --- it's a definite improvement! However, you need to mention |vp| here, since the general comments above don't quite fit this hook's signature.