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)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla36

People

(Reporter: shu, Unassigned)

References

(Blocks 1 open bug)

Details

Attachments

(1 file)

      No description provided.
Blocks: 1000532
Jan for the JIT parts, Jim for the Debugger parts.
Attachment #8522667 - Flags: review?(jimb)
Attachment #8522667 - Flags: review?(jdemooij)
Blocks: 1032869
No longer blocks: 1032869
Depends on: 1032869
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+
(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.
Depends on: 1099444, 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.
Attachment #8522667 - Flags: review?(jimb) → review+
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.