468782 - TM: js_FastValueToIterator and js_FastCallIteratorNext can reenter

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect, P1)

Description

[Jason Orendorff [:jorendorff]]

Attachment: buggy hairy attempt

This must be fixed at the same time as bug 462027 to avoid regressing for-in loops in 462027 (a two-way dependency, which bugzilla prohibits).

The fix in bug 462027 allows certain native functions to bail off trace in midflight.  Calls to such functions are followed by a special guard that detects when the native function bailed off trace (and reconstituted the JS stack) but then successfully completed.

First I tried having record_JSOP_ITER and record_JSOP_NEXTITER just emit special guards after calls to js_FastValueToIterator and js_FastCallIteratorNext.  Using the new stuff in 462027 to fix this problem.  This is hairy.  See the attached (buggy and incomplete) patch.

I think I could use imacros instead, and make js_FastValueToIterator and js_FastCallIteratorNext traceable natives which the imacros simply CALL.  Which approach is better?

Comment 1

[Brendan Eich [:brendan]]

Try imacros, they are the new hawtness ;-). We use 'em already for JSOP_ITER and JSOP_NEXTITER in the hasIteratorMethod(v) case of JSOP_ITER, and in JSOP_NEXTITER when the iterator object is an instance of js_IteratorClass.

/be

Comment 2

[Brendan Eich [:brendan]]

Jason, I'm hoping you'll take this one too.

/be

Comment 3

[Robert Sayre]

This needs to be fixed for beta, so P1.

Comment 4

[Jason Orendorff [:jorendorff]]

Very well.  I'll try imacros Monday.

Comment 5

[Brendan Eich [:brendan]]

Ping me on IRC about anything, let me know if I can help.

/be

Comment 6

[Jason Orendorff [:jorendorff]]

Attachment: v1

This applies on top of the patches in bug 462027 and bug 462042.

The good news is, it fixes the bug like magic.  None of the regrtests hit my "don't reenter except through _FAIL traceable natives" assertion with this patch.

The bad news is I had to add two new opcodes to do it, JSOP_CALLBUILTIN and JSOP_CHECKHOLE.  They are used only by the new imacros.  It all feels fairly kludgey to me; advice welcome.

Comment 7

[Brendan Eich [:brendan]]

Comment on attachment 356855 [details] [diff] [review]
v1

>+          BEGIN_CASE(JSOP_CALLBUILTIN)
>+          {

Nit: no unnecessary braces in opcode interpreter cases.

>+              obj = js_GetBuiltinFunction(cx, GET_INDEX(regs.pc));
>+              if (!obj)
>+                  goto error;
>+              rval = FETCH_OPND(-1);
>+              STORE_OPND(-1, OBJECT_TO_JSVAL(obj));
>+              PUSH_OPND(rval);

This is a total nit, but it might be less stressful for anyone worried about rooting to see rval always referenced by live stack: push rval before store at -2.

>+          }
>+          END_CASE(JSOP_CALLBUILTIN)
>+
>+          BEGIN_CASE(JSOP_CHECKHOLE)
>+          {
>+              rval = FETCH_OPND(-1);
>+              PUSH_OPND(BOOLEAN_TO_JSVAL(rval != JSVAL_HOLE));
>+          }
>+          END_CASE(JSOP_CHECKHOLE)

You avoid adding this op by coding hole;stricteq in the imacro.

>+/* Builtin functions. */
>+#define BUILTIN_FUNCTION_FastObjectToIterator   0
>+#define BUILTIN_FUNCTION_FastCallIteratorNext   1

Couple of nits: BUILTIN_ seems enough, all builtins are functions; "Fast" seems unnecessary now too.

Would an enum be better? C++ might require casts to jsbytecode of imacro_asm.js.in, yay.

>+/* Functions for use with JSOP_CALLBUILTIN. */
>+
>+static JSBool
>+FastObjectToIterator(JSContext *cx, uintN argc, jsval *vp)
>+{
>+    jsval *argv = JS_ARGV(cx, vp);
>+    JS_ASSERT(JSVAL_IS_INT(argv[0]));
>+    return js_ValueToIterator(cx, JSVAL_TO_INT(argv[0]), &JS_RVAL(cx, vp));
>+}

Is this function necessary? I mean, yeah, it is for the tn stuff in full, but is there a lower level of macro to use that avoids the non-FASTCALL JSFastNative peer? It doesn't seem to be used. Separate and lower priority bug at most.

>+static JSBool
>+FastCallIteratorNext(JSContext *cx, uintN argc, jsval *vp)
>+{
>+    return js_CallIteratorNext(cx, JS_THIS_OBJECT(cx, vp), &JS_RVAL(cx, vp));
>+}

Ditto.

>+JSObject *
>+js_GetBuiltinFunction(JSContext *cx, uintN index)
>+{
>+    JSRuntime *rt = cx->runtime;
>+    JSObject *f = rt->builtinFunctions[index];
>+    if (!f) {

Nit: canonical name is obj or funobj not f.

+JS_REQUIRES_STACK bool
>+TraceRecorder::record_JSOP_CHECKHOLE()
>+{
>+    jsval& v = stackval(-1);
>+
>+    if (JSVAL_TAG(v) == JSVAL_BOOLEAN)
>+        stack(0, lir->ins2(LIR_eq, get(&v), lir->insImm(JSVAL_TO_BOOLEAN(JSVAL_HOLE))));
>+    else
>+        stack(0, INS_CONST(JS_TRUE));
>+    return true;
>+}

Yeah, trace type inducing a constant strongly suggests this should be recoded using existing ops JSVAL_HOLE and JSVAL_STRICTEQ.

Looks great otherwise -- I never liked the resumeAfter and record_IteratorNextComplete hacks, this is nice. I'd even hope for more use of JSOP_CALLBUILTIN, so it can stay. ;-)

/be

Comment 10

[Bob Clary [:bc] (inactive)]

Assertion failure: tm->reservedDoublePoolPtr > tm->reservedDoublePool also occurs in browser with js1_6/extensions/regress-455464-04.js on 1.9.1-tracemonkey, mozilla-central.

Comment 11

[Jason Orendorff [:jorendorff]]

Attachment: interdiff v1 to v2

Comment 12

[Jason Orendorff [:jorendorff]]

Attachment: v2

Thanks for the review.

The right sequence to replace JSOP_CHECKHOLE turns out to be: dup; hole; strictne.

Comment 13

[Brendan Eich [:brendan]]

Need a dup, right.

> >+/* Functions for use with JSOP_CALLBUILTIN. */
> >+
> >+static JSBool
> >+FastObjectToIterator(JSContext *cx, uintN argc, jsval *vp)
> >+{
> >+    jsval *argv = JS_ARGV(cx, vp);
> >+    JS_ASSERT(JSVAL_IS_INT(argv[0]));
> >+    return js_ValueToIterator(cx, JSVAL_TO_INT(argv[0]), &JS_RVAL(cx, vp));
> >+}
> 
> Is this function necessary? I mean, yeah, it is for the tn stuff in full, but
> is there a lower level of macro to use that avoids the non-FASTCALL
> JSFastNative peer? It doesn't seem to be used. Separate and lower priority bug
> at most.

Any thoughts? I didn't know if this was missed. I'll review the v2 patch in a moment.

> >+    JSObject *f = rt->builtinFunctions[index];
> >+    if (!f) {
> 
> Nit: canonical name is obj or funobj not f.

This too, but obviously a minor nit.

/be

Comment 14

[Brendan Eich [:brendan]]

Comment on attachment 357045 [details] [diff] [review]
v2

>  * JSOP_NEXTITER stores the next iterated value in the top of stack slot which
>  * was allocated by JSOP_ITER and pushes true, or stores JSVAL_HOLE and pushes
>- * false. It is followed immediately by JSOP_IFNE{,X}.
>+ * false. It is followed immediately by JSOP_IFNE{,X}. (JSOP_NEXTITER has
>+ * JOF_TMPSLOT2 because there is an imacro implementation of it that uses an
>+ * extra stack slot.)

Could you try writing imacros.jsasm source instead of adding TMPSLOT2 here and extending my first hand-crafted imacros in jstracer.cpp? Then there's no stack budget magic. I'll help if imacro_asm.js.in needs extending.

/be

Comment 15

[Jason Orendorff [:jorendorff]]

Attachment: interdiff v3 to v4

Comment 16

[Jason Orendorff [:jorendorff]]

Attachment: v4

Comment 17

[Jason Orendorff [:jorendorff]]

(In reply to comment #13)
> > >+/* Functions for use with JSOP_CALLBUILTIN. */
> > >+
> > >+static JSBool
> > >+FastObjectToIterator(JSContext *cx, uintN argc, jsval *vp)
> > >+{ [...]
> > 
> > Is this function necessary? [...]

Sorry I didn't respond to this earlier.  The boring JSFastNative version is called from the interpreter when recording.

> > Nit: canonical name is obj or funobj not f.
>
> This too, but obviously a minor nit.

An oversight, corrected in v4.

> Could you try writing imacros.jsasm source instead of adding TMPSLOT2 here and
> extending my first hand-crafted imacros in jstracer.cpp?

You're right, that was a lot more fun.  :)

Comment 18

[Jason Orendorff [:jorendorff]]

I think comment 0 is wrong about the mutual dependency.  I think I can land this before bug 462027.  If I can, I should; they're both significant enough changes to warrant separate Tinderbox spins.

Transposed patches to come, if it works out for me.

Comment 19

[Jason Orendorff [:jorendorff]]

Attachment: v5

Transposed.  This is the same as v4 except many lines of context changed.

Comment 20

[Brendan Eich [:brendan]]

Comment on attachment 357835 [details] [diff] [review]
v5

>+    if (info.flags.indexOf("JOF_UINT16") >= 0) {
>+        if (/^\(/.test(op.imm1))
>+ 	    return '(_ & 0xff00) >> 8, (_ & 0xff)'.replace(/_/g, op.imm1);

Nit: 4-space indentation needed.

>+        let m = /(?:(\w+):)?\s*(\.?\w+)(?:\s+(\w+|\([^)]*\)))?(?:\s+([\w-]+|\([^)]*\)))?(?:\s*(?:#.*))?$/.exec(a[i]);

I should have used this old chestnut in global code:

let line_regexp_parts = [
    "^(?:(\w+):)?",
    "\s*(\.?\w+)",
    "(?:\s+(\w+|\([^)]*\)))?",
    "(?:\s+([\w-]+|\([^)]*\)))?",
    "(?:\s*(?:#.*))?$"
];
let line_regexp = new RegExp(line_regexp_parts.join(""));

Also not the missing ^ in first part -- I think that's needed to avoid skipping arbitrary junk at the front (duh on me for forgetting).

>   error:
>     if (fp->imacpc && cx->throwing) {
>         // To keep things simple, we hard-code imacro exception handlers here.
>         if (*fp->imacpc == JSOP_NEXTITER) {
>-            JS_ASSERT(*regs.pc == JSOP_CALL);
>+            JS_ASSERT(*regs.pc == JSOP_CALL || *regs.pc == JSOP_DUP);

Asked over IRC, seems JSOP_DUP should not be allowed by this assertion.

r=me with nit and assert fixes. line_regexp at your discretion.

/be

Comment 21

[Brendan Eich [:brendan]]

s/Also not/Also note/

/be

Comment 22

[Jason Orendorff [:jorendorff]]

It turns out that pc can point to JSOP_DUP there due to bug 474854.  (It hadn't occurred to me that that behavior is a bug, but it is.)

js1_8/extensions/regress-455973.js triggers that.

Comment 23

[Jason Orendorff [:jorendorff]]

http://hg.mozilla.org/tracemonkey/rev/17663da1b840

Comment 24

[Jason Orendorff [:jorendorff]]

Backed out.  Suspected of causing leaks.

Possible mechanism: rt->builtinFunctions could be entraining the global object via __parent__.  The comment says null parent, but I think js_NewFunction is assigning a parent anyway.

Comment 25

[Brendan Eich [:brendan]]

Sorry, I should have seen that. Solution alternatives:

1. Call js_NewObjectWithGivenProto first and pass that funobj into js_NewFunction.

2. STOBJ_CLEAR_PROTO(funobj) after the js_NewFunction call returns.

Alternative 2 is strictly less code.

/be

Comment 26

[Andreas Gal :gal]

Confirmed to leak and backout out (again, after re-landing it).

http://hg.mozilla.org/tracemonkey/rev/89a99401da84

Comment 27

[Jason Orendorff [:jorendorff]]

Attachment: interdiff v5 to v6

Comment 28

[Jason Orendorff [:jorendorff]]

Attachment: v6

Fix probable leak and a possible thread safety issue.

Comment 29

[Jason Orendorff [:jorendorff]]

Sorry, that interdiff is a bit too hasty; the actual v6 patch adds another line:

             STOBJ_CLEAR_PROTO(funobj);
+            STOBJ_CLEAR_PARENT(funobj);

Debugger tells me that js_NewFunction chooses both a proto and a parent for you.

Comment 30

[Brendan Eich [:brendan]]

Comment on attachment 358927 [details] [diff] [review]
v6

>+JSObject *
>+js_GetBuiltinFunction(JSContext *cx, uintN index)
>+{
>+    JSRuntime *rt = cx->runtime;
>+    JS_LOCK_GC(rt);
>+    JSObject *funobj = rt->builtinFunctions[index];
>+    if (!funobj) {
>+        /* Use NULL parent and atom. Builtin functions never escape to scripts. */
>+        JSFunction *fun = js_NewFunction(cx,
>+                                         NULL,
>+                                         (JSNative) builtinFunctionInfo[index].tn,
>+                                         builtinFunctionInfo[index].nargs,
>+                                         JSFUN_FAST_NATIVE | JSFUN_TRACEABLE,
>+                                         NULL,
>+                                         NULL);
>+        if (fun) {
>+            rt->builtinFunctions[index] = funobj = FUN_OBJECT(fun);
>+            STOBJ_CLEAR_PROTO(funobj);
>+            STOBJ_CLEAR_PARENT(funobj);
>+        }
>+    }
>+    JS_UNLOCK_GC(rt);

I mentioned this on IRC, may have missed a reply saying why not: if rt->buildinFunctions[i] never goes null after becoming non-null, avoid taking the GC lock until you have loaded null once. You'll have to retest in case you lost a race, but then the fast path is lock free.

/be

Comment 31

[Jason Orendorff [:jorendorff]]

Attachment: interdiff v6 to v7

Comment 32

[Jason Orendorff [:jorendorff]]

Attachment: v7

That interdiff is -b ...the actual v7 patch shows correct indentation.

Comment 33

[Brendan Eich [:brendan]]

Comment on attachment 358937 [details] [diff] [review]
v7

Sorry, don't mean to drip these comments out like water torture, but this needs to avoid self-locking on the GC lock:

>+JSObject *
>+js_GetBuiltinFunction(JSContext *cx, uintN index)
>+{
>+    JSRuntime *rt = cx->runtime;
>+    JSObject *funobj = rt->builtinFunctions[index];
>+
>+    if (!funobj) {
>+        JS_LOCK_GC(rt);
>+        funobj = rt->builtinFunctions[index];  /* Retest now that the lock is held. */
>+        if (!funobj) {
>+            /* Use NULL parent and atom. Builtin functions never escape to scripts. */
>+            JSFunction *fun = js_NewFunction(cx,
>+                                             NULL,
>+                                             (JSNative) builtinFunctionInfo[index].tn,
>+                                             builtinFunctionInfo[index].nargs,
>+                                             JSFUN_FAST_NATIVE | JSFUN_TRACEABLE,
>+                                             NULL,
>+                                             NULL);
>+            if (fun) {
>+                rt->builtinFunctions[index] = funobj = FUN_OBJECT(fun);
>+                STOBJ_CLEAR_PROTO(funobj);
>+                STOBJ_CLEAR_PARENT(funobj);
>+            }
>+        }
>+        JS_UNLOCK_GC(rt);
>+    }
>+    return funobj;

Juggle things so on a null load, you make a new function, and only store in rt->builtinFunctions inside the lock and within a null retest. Race loser makes a bit of garbage, no big deal.

/be

Comment 34

[Jason Orendorff [:jorendorff]]

Attachment: v8

Good point.

Comment 35

[Brendan Eich [:brendan]]

Comment on attachment 358942 [details] [diff] [review]
v8

>+js_GetBuiltinFunction(JSContext *cx, uintN index)
>+{
>+    JSRuntime *rt = cx->runtime;
>+    JSObject *funobj = rt->builtinFunctions[index];
>+
>+    if (!funobj) {
>+        /* Use NULL parent and atom. Builtin functions never escape to scripts. */
>+        JSFunction *fun = js_NewFunction(cx,
>+                                         NULL,
>+                                         (JSNative) builtinFunctionInfo[index].tn,
>+                                         builtinFunctionInfo[index].nargs,
>+                                         JSFUN_FAST_NATIVE | JSFUN_TRACEABLE,
>+                                         NULL,
>+                                         NULL);
>+        if (fun) {
>+            funobj = FUN_OBJECT(fun);
>+            STOBJ_CLEAR_PROTO(funobj);
>+            STOBJ_CLEAR_PARENT(funobj);
>+
>+            JS_LOCK_GC(rt);
>+            if (!rt->builtinFunctions[index])  /* retest now that the lock is held */
>+                rt->builtinFunctions[index] = funobj;
>+            JS_UNLOCK_GC(rt);
>+        }
>+    }
>+    return funobj;
>+}

Although the builtin should never escape, it seems better to do the usual trick here of returning the one true funobj, and letting the dup created by the losing racer become garbage right away:

>+            JS_LOCK_GC(rt);
>+            if (!rt->builtinFunctions[index])  /* retest now that the lock is held */
>+                rt->builtinFunctions[index] = funobj;
              else
                  funobj = rt->builtinFunctions[index];
>+            JS_UNLOCK_GC(rt);

Could use another JSObject * temporary, but no need with any compiler worthy of the name.

/be

Comment 36

[Andreas Gal :gal]

Caused failures across all platforms. Backed out.

http://hg.mozilla.org/tracemonkey/rev/947262de4794

Comment 37

[Jason Orendorff [:jorendorff]]

Attachment: interdiff v8 to v9

Dumb bug in my changes to imacro_asm.js.in.

This passed our tests, which means we have no trace-tests or regrtests that check the values produced by a for-each loop nested in a (traceable) outer loop.  The patch adds a couple trace-tests.

Comment 38

[Jason Orendorff [:jorendorff]]

Attachment: v9

Comment 39

[Robert Sayre]

http://hg.mozilla.org/mozilla-central/rev/2773d74789f4

Comment 40

[Bob Clary [:bc] (inactive)]

http://hg.mozilla.org/tracemonkey/rev/ece63b96379b caused js1_5/Regress/regress-451322.js to change from a time out (> 480 seconds) to Assertion failed: "Unknown branch type in nPatchBranch": 0 (../nanojit/Nativei386.cpp:442)

Comment 41

[Mike Beltzner [:beltzner, not reading bugmail]]

Should this get re-opened? Dunno if we want to land it on 191 with a known regression :(

Comment 42

[Bob Clary [:bc] (inactive)]

I think Gal's opinion is that this patch was not the cause of the "Unknown branch type" assertion, but rather was just responsible for making the existing problem more visible.

Comment 43

[Brendan Eich [:brendan]]

This should go into 1.9.1 with the patch for bug 475916.

/be

Comment 44

[Andreas Gal :gal]

http://hg.mozilla.org/tracemonkey/rev/9d9412b90552

fixes the rare OOM condition this patch tickled. The issue is completely unrelated. It just changed the alignment of the moon and the stars under memory pressure.

Comment 45

[Robert Sayre]

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/00d1e8523f73

Comment 46

[Robert Sayre]

(In reply to comment #45)
> http://hg.mozilla.org/releases/mozilla-1.9.1/rev/00d1e8523f73

er, wrong rev. this hasn't landed yet

Comment 47

[Bob Clary [:bc] (inactive)]

http://hg.mozilla.org/mozilla-central/rev/e9226dd6c073

Comment 48

[Robert Sayre]

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/a14fc9eab0be

Comment 49

[Bob Clary [:bc] (inactive)]

js1_8_1/trace/trace-test.js
v 1.9.1, 1.9.2