Closed Bug 468782 Opened 11 years ago Closed 11 years ago

TM: js_FastValueToIterator and js_FastCallIteratorNext can reenter

Categories

(Core :: JavaScript Engine, defect, P1)

Other Branch
defect

Tracking

()

VERIFIED FIXED

People

(Reporter: jorendorff, Assigned: jorendorff)

References

Details

(Keywords: verified1.9.1)

Attachments

(2 files, 12 obsolete files)

Attached patch buggy hairy attempt (obsolete) — Splinter Review
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?
No longer blocks: deepbail
Blocks: deepbail
Flags: blocking1.9.1+
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
Jason, I'm hoping you'll take this one too.

/be
Assignee: general → jorendorff
This needs to be fixed for beta, so P1.
Status: NEW → ASSIGNED
Priority: -- → P1
Very well.  I'll try imacros Monday.
Ping me on IRC about anything, let me know if I can help.

/be
Attached patch v1 (obsolete) — Splinter Review
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.
Attachment #352280 - Attachment is obsolete: true
Attachment #356855 - Flags: review?(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
Duplicate of this bug: 464096
Duplicate of this bug: 473040
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.
Attached patch interdiff v1 to v2 (obsolete) — Splinter Review
Attached patch v2 (obsolete) — Splinter Review
Thanks for the review.

The right sequence to replace JSOP_CHECKHOLE turns out to be: dup; hole; strictne.
Attachment #356855 - Attachment is obsolete: true
Attachment #357045 - Flags: review?(brendan)
Attachment #356855 - Flags: review?(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 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
Attached patch interdiff v3 to v4 (obsolete) — Splinter Review
Attachment #357037 - Attachment is obsolete: true
Attached patch v4 (obsolete) — Splinter Review
Attachment #357045 - Attachment is obsolete: true
Attachment #357502 - Flags: review?(brendan)
Attachment #357045 - Flags: review?(brendan)
(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.  :)
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.
Attached patch v5 (obsolete) — Splinter Review
Transposed.  This is the same as v4 except many lines of context changed.
Attachment #357502 - Attachment is obsolete: true
Attachment #357835 - Flags: review?(brendan)
Attachment #357502 - Flags: review?(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
Attachment #357835 - Flags: review?(brendan) → review+
s/Also not/Also note/

/be
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.
http://hg.mozilla.org/tracemonkey/rev/17663da1b840
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
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.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
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
Confirmed to leak and backout out (again, after re-landing it).

http://hg.mozilla.org/tracemonkey/rev/89a99401da84
Attached patch interdiff v5 to v6 (obsolete) — Splinter Review
Attachment #357501 - Attachment is obsolete: true
Attached patch v6 (obsolete) — Splinter Review
Fix probable leak and a possible thread safety issue.
Attachment #357835 - Attachment is obsolete: true
Attachment #358927 - Flags: review?(brendan)
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 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
Attached patch interdiff v6 to v7 (obsolete) — Splinter Review
Attachment #358921 - Attachment is obsolete: true
Attached patch v7 (obsolete) — Splinter Review
That interdiff is -b ...the actual v7 patch shows correct indentation.
Attachment #358927 - Attachment is obsolete: true
Attachment #358937 - Flags: review?(brendan)
Attachment #358927 - Flags: review?(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
Attached patch v8 (obsolete) — Splinter Review
Good point.
Attachment #358935 - Attachment is obsolete: true
Attachment #358937 - Attachment is obsolete: true
Attachment #358942 - Flags: review?(brendan)
Attachment #358937 - Flags: review?(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
Attachment #358942 - Flags: review?(brendan) → review+
Caused failures across all platforms. Backed out.

http://hg.mozilla.org/tracemonkey/rev/947262de4794
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.
Attached patch v9Splinter Review
Attachment #358942 - Attachment is obsolete: true
Attachment #359287 - Flags: review?(brendan)
Attachment #359287 - Flags: review?(brendan) → review+
http://hg.mozilla.org/mozilla-central/rev/2773d74789f4
Status: REOPENED → RESOLVED
Closed: 11 years ago11 years ago
Resolution: --- → FIXED
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)
Should this get re-opened? Dunno if we want to land it on 191 with a known regression :(
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.
This should go into 1.9.1 with the patch for bug 475916.

/be
Depends on: 475916
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.
(In reply to comment #45)
> http://hg.mozilla.org/releases/mozilla-1.9.1/rev/00d1e8523f73

er, wrong rev. this hasn't landed yet
Keywords: fixed1.9.1
http://hg.mozilla.org/mozilla-central/rev/e9226dd6c073
Flags: in-testsuite+
Flags: in-litmus-
js1_8_1/trace/trace-test.js
v 1.9.1, 1.9.2
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.