We'll handle support for the debugger the same way JM handles it. When a new breakpoint is added to a given bytecode PC, we should recompile the script and insert a jump out. If the script is currently on the call stack, then we deoptimize back to the interpreter, and then recompile the script.

Recompilation is pretty complicated, even for a baseline compiler. It would be good to avoid it entirely. Turning debug mode on for a compartment requires that no JS is on the stack, so when that happens, we can sweep all code objects attached to scripts. Depending on how flexible the debugger API is, we could insert trap points at every breakable spot in the script. These would just be nops and they'd be patchable to calls. Breakable spots would be new lines or statements or something, and we might have to find the nearest one when an arbitrary pc is requested.

(In reply to David Anderson [:dvander] from comment #1) > Depending on how flexible the debugger API is, we could insert trap points > at every breakable spot in the script. These would just be nops and they'd > be patchable to calls. Breakable spots would be new lines or statements or > something, and we might have to find the nearest one when an arbitrary pc is > requested. Debugger could certainly work with that. We may want to be able to break on a statement boundary, rather than a line boundary (potentially more fine-grained, if many statements are packed on a line), but in any case there can be understood conventions for where breaks are possible and where they aren't. Debugger defines "single-stepping" very loosely: it just says that some amount of forward progress must happen, and that it won't go further than a statement. So even if we can only stop on statement boundaries, it should be fine.

This has been said elsewhere, but: we'd really like to be able to turn debug mode on and off with frames on the stack, because it has positive consequences visible all the way up to the developer. The Debugger API would become a lot more useful in general.

I'd like to take a first stab at this the coming week. Plan of attack: * If debug mode is turned on, all JIT code is destroyed. No JS frames are on the stack so this does not require any on-stack invalidation. As Jim mentioned in comment 3, maybe later on we will want the ability to turn on debug mode with JS frames on the stack, but that's not trivial and for now I think it's more important to get things working. We can certainly experiment with that when the compiler is (more) stable though. * When compiling in debug mode, for all "breakable" ops we will emit a series of nops. When a breakpoint is added (or single-step mode is enabled for the script) these nops are patched to a "call <debug_thunk>" instruction. * The debug_thunk calls a VM function with signature: bool Trap(JSContext *cx, MutableHandleValue rval); Trap can either: (a) Continue execution. Trap returns |true| and rval is a MagicValue. (b) Cause the JS function to return. Trap returns |true|, rval is set to a non-Magic value and the debug thunk returns rval to the caller. (c) Throw a value. Trap sets cx->pendingException and returns |false|. Handled by the VM wrapper infrastructure). (d) Fail (OOM, etc): Trap clears cx->pendingException and returns |false|. Handled by the VM wrapper infrastructure.

For the debugger we will have to add a pretty large number of methods to BaselineFrame, so this patch moves BaselineFrame to BaselineFrame.h and simplifies GC marking a bit. This conflicts with your OSR patch, but it also fixes some bugs (the offsetOf methods were incorrect, it becomes a problem when C++ code uses BaselineFrame *).

(In reply to Jan de Mooij [:jandem] from comment #4) > * When compiling in debug mode, for all "breakable" ops we will emit a > series of nops. When a breakpoint is added (or single-step mode is enabled > for the script) these nops are patched to a "call <debug_thunk>" instruction. When I read this, I think: "I sure hope that patching code asserts that it's only overwriting no-ops." I'd hate to have bugs due to the call instruction (or instructions) overwriting something meaningful.

Part 0: https://hg.mozilla.org/projects/ionmonkey/rev/826a73f55120

(In reply to Jim Blandy :jimb from comment #6) > When I read this, I think: "I sure hope that patching code asserts that it's > only overwriting no-ops." I'd hate to have bugs due to the call instruction > (or instructions) overwriting something meaningful. Sure, asserting that should be straight-forward.

Adds script prologue and epilogue hooks + various other fixes for problems exposed by jit-tests. For now we still abort compilation in debug mode, but if I remove that the patch fixes 21 (out of 57) debug test failures.

Mostly as described in comment 4. Main difference I think is that the stub does not return a magic value but just a boolean. If it's |true|, we return the value in the frame's return value slot. If it's |false|, we continue execution. This brings the number of debug test failures down to 16. The patch does not apply cleanly, it depends on earlier patches in this bug and the patches in bug 831754.

Comment on attachment 702402 [details] [diff] [review] Part 1: script prologue and epilogue hooks Review of attachment 702402 [details] [diff] [review]: ----------------------------------------------------------------- Looks good. I think it'd be good to run this by somebody with a more intimate knowledge of the debugging code. ::: js/src/vm/Debugger.cpp @@ +1993,1 @@ > continue; In this and in several places like this in the code, something confuses me. Shouldn't we just be JS_ASSERTING(!i.isIonOptimizedJS()) here? If we're in this code it means that debugging is on, which means that Ion optimized frames simply won't be on the stack. ::: js/src/vm/Stack.cpp @@ +1791,5 @@ > return; > case ION: > +#ifdef JS_ION > + if (data_.ionFrames_.isBaselineJS()) { > + RootedScript script(data_.maybecx_); Should data_.maybecx_ be JS_ASSERTED in this method? It's used implicitly but the naming implies that it may be null.

Just to improve test coverage. And indeed, there are 4 new failures now related to eval-in-frame, which we don't handle yet.

Comment on attachment 703333 [details] [diff] [review] Part 2: Breakpoints and step mode Review of attachment 703333 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/ion/BaselineCompiler.cpp @@ +346,5 @@ > + return false; > + > + // Emit patchable call to debug trap handler. > + IonCode *handler = cx->compartment->ionCompartment()->debugTrapHandler(cx); > + mozilla::DebugOnly<CodeOffsetLabel> offset = masm.toggledCall(handler, enabled); I can't find a definition for masm.toggledCall. Is this in a later patch? ::: js/src/ion/IonCompartment.h @@ +161,4 @@ > IonCode *valuePreBarrier() { > return rt->valuePreBarrier_; > } > Nit: pre-existing trailing whitespace here. would be nice if you could fix :) ::: js/src/ion/arm/MacroAssembler-arm.cpp @@ +7,5 @@ > > #include "mozilla/DebugOnly.h" > > #include "ion/arm/MacroAssembler-arm.h" > +#include "ion/BaselineFrame.h" Is this include necessary? ::: js/src/ion/x86/BaselineHelpers-x86.h @@ +139,5 @@ > // in that case we use the frame pointer. > if (calledIntoIon) { > + masm.pop(ebx); > + masm.shrl(Imm32(FRAMESIZE_SHIFT), ebx); > + masm.addl(ebx, BaselineStackReg); Given the way we're changing these, I think the register used here should be an argument to the function, given a default value of ebx. The EmitLeaveStubFrame variants for the other archs can have ScratchReg as the default register argument. It's not perfect, but makes it a little bit more obvious that EmitLeaveStubFrame clobbers some register.

Part 1-3: https://hg.mozilla.org/projects/ionmonkey/rev/d17c631afadb https://hg.mozilla.org/projects/ionmonkey/rev/7c42080211ad https://hg.mozilla.org/projects/ionmonkey/rev/648791c1fd99 (In reply to Kannan Vijayan [:djvj] from comment #11) > Shouldn't we just be JS_ASSERTING(!i.isIonOptimizedJS()) here? If we're in > this code it means that debugging is on, which means that Ion optimized > frames simply won't be on the stack. There may be frames from other compartments on the stack. Debug mode is enabled per compartment, so we may still see Ion frames (but we can skip them since they must be from another compartment). > Should data_.maybecx_ be JS_ASSERTED in this method? It was renamed to cx_ on trunk in the meantime :) (In reply to Kannan Vijayan [:djvj] from comment #13) > I can't find a definition for masm.toggledCall. Is this in a later patch? Woops, I added it in bug 831754, should have CC'ed you or mentioned it here. > Is this include necessary? Yeah, a previous patch required it but I forgot to add it there. > Given the way we're changing these, I think the register used here should be > an argument to the function, given a default value of ebx. The > EmitLeaveStubFrame variants for the other archs can have ScratchReg as the > default register argument. Good idea, done.

Handle OnExceptionUnwind hooks + some other minor changes. With the eval-in-frame patches in bug 832373, this brings the number of jsdbg2 jit-test failures down to 8. With this patch we support all debugger hooks though.

Fixes two test failures.

This patch adds a block chain to the frame and makes ENTERBLOCK/LEAVEBLOCK push/prop from it. Gets rid of some TODOs and makes us pass Environment-identity-02.js and Environment-variables.js, especially the latter is nice because it tests a lot of complicated stuff.

When debug mode is enabled we perform a debug-mode GC to release JIT code. This patch destroys all baseline JIT scripts that are not on the stack during GC (and there are no scripts on the stack when we enter debug mode). 1 jit-test/tests/debug/* failure remaining (though there are still a bunch of non-jsdbg2 debugger failures in other directories)

We have some old debugger tests that trap instructions for which SrcNoteLineScanner::isLineHeader() is |false|. jorendorff says the Debugger API supports this too and for now the easiest thing to do is allow traps/breakpoints at every pc.

Comment on attachment 705017 [details] [diff] [review] Part 4: Handle onExceptionUnwind Review of attachment 705017 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/ion/IonFrames.cpp @@ +445,5 @@ > if (iter.checkInvalidation(&ionScript)) > ionScript->decref(cx->runtime->defaultFreeOp()); > } > > if (iter.isBaselineJS()) { The structure of the two "if" blocks here seems to suggest that both iter.isOptimizedJS() and iter.isBaselineJS() can be true at the same time. It would be clearer to use an "else if" here.

Comment on attachment 705292 [details] [diff] [review] Part 6: Add block chain Review of attachment 705292 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/ion/BaselineCompiler.cpp @@ +1434,5 @@ > bool > BaselineCompiler::emit_JSOP_LEAVEBLOCK() > { > + // Call a stub to pop the block from the block chain. > + prepareVMCall(); Do we need a stack sync before this? I suspect not since the |popn| later is basically discarding any unsynced info anyway, but just bringing it up for thought. ::: js/src/ion/BaselineFrame-inl.h @@ +60,5 @@ > + JS_ASSERT(scopeChain_->asClonedBlock().staticBlock() == *blockChain_); > + popOffScopeChain(); > + } > + > + blockChain_ = blockChain_->enclosingBlock(); nit: setBlockChain(*blockChain_->enclosingBlock());

Part 4-8 with nits addressed: https://hg.mozilla.org/projects/ionmonkey/rev/e48f83e2aee5 https://hg.mozilla.org/projects/ionmonkey/rev/c833d2173bdc https://hg.mozilla.org/projects/ionmonkey/rev/d55a0c48bf10 https://hg.mozilla.org/projects/ionmonkey/rev/20819f78fc42 https://hg.mozilla.org/projects/ionmonkey/rev/cd4699153db0 Getting close :)

(In reply to Jim Blandy :jimb from comment #6) > (In reply to Jan de Mooij [:jandem] from comment #4) > > * When compiling in debug mode, for all "breakable" ops we will emit a > > series of nops. When a breakpoint is added (or single-step mode is enabled > > for the script) these nops are patched to a "call <debug_thunk>" instruction. > > When I read this, I think: "I sure hope that patching code asserts that it's > only overwriting no-ops." I'd hate to have bugs due to the call instruction > (or instructions) overwriting something meaningful. See bug 1208674 - ARM64: BaselineScript::toggleDebugTraps() is clobbering unrelated instructions ;-)