Last Comment Bug 707454 - Remove JSOP_TRAP
: Remove JSOP_TRAP
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: unspecified
: x86 Mac OS X
: -- normal (vote)
: mozilla11
Assigned To: Brian Hackett (:bhackett)
:
Mentors:
: 619830 (view as bug list)
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2011-12-03 10:25 PST by Brian Hackett (:bhackett)
Modified: 2015-10-07 09:57 PDT (History)
7 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
patch (138.62 KB, patch)
2011-12-03 15:13 PST, Brian Hackett (:bhackett)
jorendorff: review+
Details | Diff | Review

Description Brian Hackett (:bhackett) 2011-12-03 10:25:24 PST
The debugger trap design in the VM is incredibly error prone for doing any sort of analysis or compilation work.  Any opcode in the script can potentially be trapped (well, not any, but there is no simple or enforced definition for which opcodes can be trapped), which physically changes the bytecode to contain a JSOP_TRAP and requires every single place in the VM which operates on the bytecode to use js_GetOpcode or one of the at least three different auto-untrapper structures to get the real opcode which the trap replaced, otherwise things will botch asserts or crash.

It would be much better if installing a trap somewhere in a script did not actually change the script's bytecode, which would allow removing JSOP_TRAP, js_GetOpcode, the various untrappers and a whole bunch of other complexity.  Instead, scripts with traps on them would have an associated array of BreakpointSite* (compartment->breakpointSites would be removed) indexed by the opcode offset.  This could be done without bloating sizeof(JSScript), by using the bits storing script->stepMode (a uint32) and moving all the debug-related state into a DebugScript structure or somesuch field of JSScript, non-NULL only for scripts that are in single step mode or have installed traps.
Comment 1 Brian Hackett (:bhackett) 2011-12-03 15:13:46 PST
Created attachment 578862 [details] [diff] [review]
patch
Comment 2 Brian Hackett (:bhackett) 2011-12-03 15:15:04 PST
Also:

33 files changed, 399 insertions(+), 601 deletions(-)
Comment 3 Luke Wagner [:luke] 2011-12-03 21:14:17 PST
\o/ \o/
Comment 4 Jason Orendorff [:jorendorff] 2011-12-06 09:03:16 PST
Comment on attachment 578862 [details] [diff] [review]
patch

Even having implemented all that nonsense to set and clear JSOP_TRAP at
the right times, I was surprised how much this lightened the load.

It's really amazing the number of different workarounds we had in the
codebase for JSOP_TRAP. Phew.

In jsgcmark.cpp, MarkChildren(JSTracer *, JSScript *):
>     if (script->types)
>         script->types->trace(trc);
>+
>+    if (script->hasAnyBreakpointsOrStepMode())
>+        script->markTrapClosures(trc);
> }

The trap closure might be in a different compartment.

Unless I'm mistaken about how this works, Firebug will likely crash due
to this. But we probably currently have no way of testing this in the
shell.

We can fix that by changing ValueToScript (in shell/js.cpp) to see
through wrappers. Then a test could use trap() to set a breakpoint on a
wrapper of a function in a different compartment, and then do
single-compartment gc in that compartment.

In jsinterp.cpp:
>               case JSTRY_ITER: {
>                 /* This is similar to JSOP_ENDITER in the interpreter loop. */
>-                JS_ASSERT(js_GetOpcode(cx, regs.fp()->script(), regs.pc) == JSOP_ENDITER);
>+                  JS_ASSERT(JSOp(*regs.pc) == JSOP_ENDITER);
>                 Value v = cx->getPendingException();
>                 cx->clearPendingException();

Nit: Fix indentation.

In jsscript.cpp, JSScript::ensureHasDebug:
>+    if (!debug)
>+        return false;
>+
>+    /* Step mode has been enabled. Alert the interpreter. */
>+    InterpreterFrames *frames;
>+    for (frames = JS_THREAD_DATA(cx)->interpreterFrames; frames; frames = frames->older)
>+        frames->enableInterruptsIfRunning(this);
>+
>+    return !!debug;

This should just return true.

In jsscript.cpp, JSScript::tryNewStepMode:
>-        if (newValue) {
>-            /* Step mode has been enabled. Alert the interpreter. */
>-            InterpreterFrames *frames;
>-            for (frames = JS_THREAD_DATA(cx)->interpreterFrames; frames; frames = frames->older)
>-                frames->enableInterruptsIfRunning(this);

Why is it OK to remove this code?

> bool
> JSScript::setStepModeFlag(JSContext *cx, bool step)
> {
>-    return tryNewStepMode(cx, (stepMode & stepCountMask) | (step ? stepFlagMask : 0));
>+    return tryNewStepMode(cx, (debug->stepMode & stepCountMask) | (step ? stepFlagMask : 0));
> }

This needs to call ensureHasDebug() before trying to get debug->stepMode.

(Then I think tryNewStepMode probably doesn't need to call it anymore; but I didn't check.)

In JSScript::getOrCreateBreakpointSite:
>-    JS_ASSERT(script->code <= pc);
>-    JS_ASSERT(pc < script->code + script->length);
>+    JS_ASSERT(unsigned(pc - code) < length);

Micro-nit: cast to size_t instead of unsigned. Casting from ptrdiff_t to
unsigned truncates from 64 to 32 bits on some platforms, and could (I
mean, theoretically) make the assertion pass when it should fail, even
though JSScript::length is just 32 bits.

Same deal in JSScript::destroyBreakpointSite.

In jsscript.h, JSScript::getBreakpointSite:
>+        JS_ASSERT(unsigned(pc - code) < length);

Same comment about size_t vs. unsigned here.

In methodjit/Compiler.cpp, mjit::Compiler::finishThisUp:
>     size_t nNmapLive = loopEntries.length();
>     for (size_t i = 0; i < script->length; i++) {
>         Bytecode *opinfo = analysis->maybeCode(i);
>-        if (opinfo && opinfo->safePoint) {
>-            /* loopEntries cover any safe points which are at loop heads. */
>-            JSOp op = js_GetOpcode(cx, script, script->code + i);
>-            if (!cx->typeInferenceEnabled() || op != JSOP_LOOPHEAD)
>-                nNmapLive++;
>-        }
>+        if (opinfo && opinfo->safePoint)
>+            nNmapLive++;
>     }

I don't understand this change. I mean, I understand removing
js_GetOpcode, but I don't understand removing the rest.

Later in the same method:
>     if (jit->nNmapPairs > 0) {
>         for (size_t i = 0; i < script->length; i++) {
>             Bytecode *opinfo = analysis->maybeCode(i);
>             if (opinfo && opinfo->safePoint) {
>-                JSOp op = js_GetOpcode(cx, script, script->code + i);
>-                if (cx->typeInferenceEnabled() && op == JSOP_LOOPHEAD)
>-                    continue;

Same here.

In methodjit/InvokeHelpers.cpp, in js_InternalInterpret:
>       case REJOIN_TRAP:
>         /*
>          * Make sure when resuming in the interpreter we do not execute the
>          * trap again. Watch out for the case where the TRAP removed itself.
>          */
>-        if (untrap.trap)
>+        if (script->hasBreakpointsAt(pc))
>             skipTrap = true;
>         break;

Nit: I don't think the if-statement is necessary anymore. You can just
set skipTrap = true.

Super-nit: "TRAP" should be lowercased in the comment.

In vm/Debugger.cpp:
> Debugger::onLeaveFrame(JSContext *cx)
> {
>-    if (!cx->compartment->getDebuggees().empty() || !cx->compartment->breakpointSites.empty())
>+    /* Traps must be cleared from eval frames, see slowPathOnLeaveFrame. */
>+    bool evalTraps = cx->fp()->isEvalFrame()
>+                     ? cx->fp()->script()->hasAnyBreakpointsOrStepMode()
>+                     : false;

Style nit: cx->fp()->isEvalFrame() && cx->fp()->script()->hasAnyBreakpointsOrStepMode()
Comment 5 Jason Orendorff [:jorendorff] 2011-12-06 09:07:33 PST
(In reply to Jason Orendorff [:jorendorff] from comment #4)
> In jsgcmark.cpp, MarkChildren(JSTracer *, JSScript *):
> >     if (script->types)
> >         script->types->trace(trc);
> >+
> >+    if (script->hasAnyBreakpointsOrStepMode())
> >+        script->markTrapClosures(trc);
> > }
> 
> The trap closure might be in a different compartment.
> 
> Unless I'm mistaken about how this works, Firebug will likely crash due
> to this. But we probably currently have no way of testing this in the
> shell.
> 
> We can fix that by changing ValueToScript (in shell/js.cpp) to see
> through wrappers. Then a test could use trap() to set a breakpoint on a
> wrapper of a function in a different compartment, and then do
> single-compartment gc in that compartment.

billm pointed out that we already don't mark trap closures in compartment A for traps on scripts in compartment B when compartment A is GC'd.

So this patch is apparently no change in trap-marking behavior. Hmm. Maybe JSD1 is already using wrappers on trap closures. Then we could just add the appropriate assertion to JS_SetTrap. Looking now.
Comment 6 Jason Orendorff [:jorendorff] 2011-12-06 09:12:27 PST
Oh, OK. The only use of JS_SetTrap in JSD looks like this:

>    rv = JS_SetTrap(jsdc->dumbContext, jsdscript->script, 
>                    (jsbytecode*)pc, jsd_TrapHandler,
>                    PRIVATE_TO_JSVAL(jsdhook));

The trap closure is always an int.

And JSD is the only JS_SetTrap client we care about. So just add an assertion in JS_SetTrap, like below. Sorry for the confusion.

diff --git a/js/src/jsdbgapi.cpp b/js/src/jsdbgapi.cpp
--- a/js/src/jsdbgapi.cpp
+++ b/js/src/jsdbgapi.cpp
@@ -180,6 +180,7 @@ JS_SetSingleStepMode(JSContext *cx, JSSc
 JS_PUBLIC_API(JSBool)
 JS_SetTrap(JSContext *cx, JSScript *script, jsbytecode *pc, JSTrapHandler handler, jsval closure)
 {
+    assertSameCompartment(cx, script, closure);
     if (!CheckDebugMode(cx))
         return false;
Comment 7 Brian Hackett (:bhackett) 2011-12-06 15:35:04 PST
(In reply to Jason Orendorff [:jorendorff] from comment #4)
> In jsscript.cpp, JSScript::tryNewStepMode:
> >-        if (newValue) {
> >-            /* Step mode has been enabled. Alert the interpreter. */
> >-            InterpreterFrames *frames;
> >-            for (frames = JS_THREAD_DATA(cx)->interpreterFrames; frames; frames = frames->older)
> >-                frames->enableInterruptsIfRunning(this);
> 
> Why is it OK to remove this code?

This logic was moved into ensureHasDebug --- whenever a script has non-NULL debug info, interpreter frames executing it must have interrupts enabled.

> In methodjit/Compiler.cpp, mjit::Compiler::finishThisUp:
> >     size_t nNmapLive = loopEntries.length();
> >     for (size_t i = 0; i < script->length; i++) {
> >         Bytecode *opinfo = analysis->maybeCode(i);
> >-        if (opinfo && opinfo->safePoint) {
> >-            /* loopEntries cover any safe points which are at loop heads. */
> >-            JSOp op = js_GetOpcode(cx, script, script->code + i);
> >-            if (!cx->typeInferenceEnabled() || op != JSOP_LOOPHEAD)
> >-                nNmapLive++;
> >-        }
> >+        if (opinfo && opinfo->safePoint)
> >+            nNmapLive++;
> >     }
> 
> I don't understand this change. I mean, I understand removing
> js_GetOpcode, but I don't understand removing the rest.

Oh, this is fixing some messiness that was exposed when working on this patch.  The method JIT computes special trampolines for entering jitcode at loop heads, except in cases where a trap was placed exactly at the loop head (if that is even possible).  This makes the loopEntries comment wrong and no special casing for safe points at loop heads needs to be performed.
Comment 8 Brian Hackett (:bhackett) 2011-12-07 13:16:34 PST
https://hg.mozilla.org/integration/mozilla-inbound/rev/dfd8e10f7155
Comment 9 Ed Morley [:emorley] 2011-12-08 08:46:29 PST
https://hg.mozilla.org/mozilla-central/rev/dfd8e10f7155
Comment 10 André Bargull 2015-10-07 09:57:41 PDT
*** Bug 619830 has been marked as a duplicate of this bug. ***

Note You need to log in before you can comment on or make changes to this bug.