TM: js_FastValueToIterator and js_FastCallIteratorNext can reenter

VERIFIED FIXED

Status

()

Core
JavaScript Engine
P1
normal
VERIFIED FIXED
9 years ago
9 years ago

People

(Reporter: jorendorff, Assigned: jorendorff)

Tracking

({verified1.9.1})

Other Branch
verified1.9.1
Points:
---
Dependency tree / graph
Bug Flags:
blocking1.9.1 +
in-testsuite +
in-litmus -

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 12 obsolete attachments)

(Assignee)

Description

9 years ago
Created attachment 352280 [details] [diff] [review]
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?
(Assignee)

Updated

9 years ago
No longer blocks: 462027
(Assignee)

Updated

9 years ago
Blocks: 462027

Updated

9 years ago
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

Comment 3

9 years ago
This needs to be fixed for beta, so P1.
Status: NEW → ASSIGNED
Priority: -- → P1
(Assignee)

Comment 4

9 years ago
Very well.  I'll try imacros Monday.
Ping me on IRC about anything, let me know if I can help.

/be
(Assignee)

Comment 6

9 years ago
Created attachment 356855 [details] [diff] [review]
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.
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

Updated

9 years ago
Duplicate of this bug: 464096

Updated

9 years ago
Duplicate of this bug: 473040

Comment 10

9 years ago
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.
(Assignee)

Comment 11

9 years ago
Created attachment 357037 [details] [diff] [review]
interdiff v1 to v2
(Assignee)

Comment 12

9 years ago
Created attachment 357045 [details] [diff] [review]
v2

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
(Assignee)

Comment 15

9 years ago
Created attachment 357501 [details] [diff] [review]
interdiff v3 to v4
Attachment #357037 - Attachment is obsolete: true
(Assignee)

Comment 16

9 years ago
Created attachment 357502 [details] [diff] [review]
v4
Attachment #357045 - Attachment is obsolete: true
Attachment #357502 - Flags: review?(brendan)
Attachment #357045 - Flags: review?(brendan)
(Assignee)

Comment 17

9 years ago
(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.  :)
(Assignee)

Comment 18

9 years ago
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.
(Assignee)

Comment 19

9 years ago
Created attachment 357835 [details] [diff] [review]
v5

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
(Assignee)

Comment 22

9 years ago
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.
(Assignee)

Comment 23

9 years ago
http://hg.mozilla.org/tracemonkey/rev/17663da1b840
Status: ASSIGNED → RESOLVED
Last Resolved: 9 years ago
Resolution: --- → FIXED
(Assignee)

Comment 24

9 years ago
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

Comment 26

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

http://hg.mozilla.org/tracemonkey/rev/89a99401da84
(Assignee)

Comment 27

9 years ago
Created attachment 358921 [details] [diff] [review]
interdiff v5 to v6
Attachment #357501 - Attachment is obsolete: true
(Assignee)

Comment 28

9 years ago
Created attachment 358927 [details] [diff] [review]
v6

Fix probable leak and a possible thread safety issue.
Attachment #357835 - Attachment is obsolete: true
Attachment #358927 - Flags: review?(brendan)
(Assignee)

Comment 29

9 years ago
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
(Assignee)

Comment 31

9 years ago
Created attachment 358935 [details] [diff] [review]
interdiff v6 to v7
Attachment #358921 - Attachment is obsolete: true
(Assignee)

Comment 32

9 years ago
Created attachment 358937 [details] [diff] [review]
v7

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
(Assignee)

Comment 34

9 years ago
Created attachment 358942 [details] [diff] [review]
v8

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+

Comment 36

9 years ago
Caused failures across all platforms. Backed out.

http://hg.mozilla.org/tracemonkey/rev/947262de4794
(Assignee)

Comment 37

9 years ago
Created attachment 359286 [details] [diff] [review]
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.
(Assignee)

Comment 38

9 years ago
Created attachment 359287 [details] [diff] [review]
v9
Attachment #358942 - Attachment is obsolete: true
Attachment #359287 - Flags: review?(brendan)
Attachment #359287 - Flags: review?(brendan) → review+

Comment 39

9 years ago
http://hg.mozilla.org/mozilla-central/rev/2773d74789f4
Status: REOPENED → RESOLVED
Last Resolved: 9 years ago9 years ago
Resolution: --- → FIXED

Comment 40

9 years ago
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 :(

Comment 42

9 years ago
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

Comment 44

9 years ago
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

9 years ago
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/00d1e8523f73
Keywords: fixed1.9.1

Comment 46

9 years ago
(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

Comment 47

9 years ago
http://hg.mozilla.org/mozilla-central/rev/e9226dd6c073
Flags: in-testsuite+
Flags: in-litmus-

Comment 48

9 years ago
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/a14fc9eab0be
Keywords: fixed1.9.1

Comment 49

9 years ago
js1_8_1/trace/trace-test.js
v 1.9.1, 1.9.2
Status: RESOLVED → VERIFIED
Keywords: fixed1.9.1 → verified1.9.1
You need to log in before you can comment on or make changes to this bug.