Closed
Bug 1098696
Opened 10 years ago
Closed 10 years ago
[jsdbg2] Don't consider onDebuggerStatement an all-observing hook
Categories
(Core :: JavaScript Engine, defect)
Core
JavaScript Engine
Tracking
()
RESOLVED
FIXED
mozilla36
People
(Reporter: shu, Unassigned)
References
(Blocks 1 open bug)
Details
Attachments
(1 file)
25.70 KB,
patch
|
jandem
:
review+
jimb
:
review+
|
Details | Diff | Splinter Review |
No description provided.
Reporter | ||
Comment 1•10 years ago
|
||
Jan for the JIT parts, Jim for the Debugger parts.
Attachment #8522667 -
Flags: review?(jimb)
Attachment #8522667 -
Flags: review?(jdemooij)
Reporter | ||
Updated•10 years ago
|
Comment 2•10 years ago
|
||
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.
Attachment #8522667 -
Flags: review?(jdemooij) → review+
Reporter | ||
Comment 3•10 years ago
|
||
(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().
Reporter | ||
Comment 4•10 years ago
|
||
While testing I ran into some test failures in getting offsets and maintaining Debugger.Environment identity. Those are filed as bugs 1099444 and 1099446.
Reporter | ||
Updated•10 years ago
|
Comment 5•10 years ago
|
||
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.
Updated•10 years ago
|
Attachment #8522667 -
Flags: review?(jimb) → review+
Reporter | ||
Comment 6•10 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/4dad70c4554d
Comment 7•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/4dad70c4554d
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla36
You need to log in
before you can comment on or make changes to this bug.
Description
•