Closed Bug 469625 Opened 16 years ago Closed 15 years ago

TM: Crash [@ js_String_getelem]

Categories

(Core :: JavaScript Engine, defect, P1)

defect

Tracking

()

VERIFIED FIXED
mozilla1.9.1b3

People

(Reporter: gkw, Assigned: graydon)

References

Details

(Keywords: crash, testcase, verified1.9.1)

Crash Data

Attachments

(4 files, 8 obsolete files)

[].__proto__[0] = 'a';
for (var j = 0; j < 3; ++j) [[, ]] = [];

crashes both opt and debug js shells at js_String_getelem.

Thanks Jesse for helping to reduce this testcase.
Flags: blocking1.9.1?
Confirmed. Excellent test case.

#0  0x001020dc in js_String_getelem (cx=0x30b4e0, str=0x2, i=0) at ../jsstr.cpp:2487
#1  0x0028cfba in ?? ()

That doesn't look right.
recording starting from s.js:2@23
globalObj=0x24e000, shape=142
    start
    state = param 0 ecx
    0
    sp = ld state[0]
    4
    rp = ld state[4]
    12
    cx = ld state[12]
    8
    gp = ld state[8]
    16
    eos = ld state[16]
    20
    eor = ld state[20]
    obj
    js_FastNewArray1 = js_FastNewArray ( cx obj )

    eq1 = eq js_FastNewArray1, 0
    xt1: xt eq1 -> 0:23 sp+0 rp+0

    sti sp[0] = js_FastNewArray1
    sti sp[8] = js_FastNewArray1
    #0:0 /* 0 */
    sti sp[16] = 0
    ld1 = ld js_FastNewArray1[4]
    -4
    and1 = and ld1, -4
    clasp
    guard(class is Array) = eq and1, clasp
    xf1: xf guard(class is Array) -> 0:28 sp+24 rp+0

    28
    ld2 = ld js_FastNewArray1[28]
    ld3 = ld js_FastNewArray1[16]
    ugt1 = ugt ld3, 0
    jf ugt1 -> unpatched

    eq2 = eq ld2, 0
    jt eq2 -> unpatched

    ld4 = ld ld2[-4]
    ugt2 = ugt ld4, 0
    jf ugt2 -> unpatched

    1
    x1: x  -> 0:28 sp+24 rp+0

    label1:
    116
    ld5 = ld cx[116]
    #0x10898
    ld6 = ld ld5[#0x10898]
    0
    eq3 = eq ld6, 0
    xf2: xf eq3 -> 0:28 sp+24 rp+0

    2
    sti sp[8] = 2
    sti sp[16] = 2
    #0:0 /* 0 */
    sti sp[24] = 0
    js_String_getelem1 = js_String_getelem ( cx 2 0 )

    eq4 = eq js_String_getelem1, 0
    xt2: xt eq4 -> 0:31 sp+32 rp+0

import vp=0x811334 name=$global0 type=int flags=0
    sti sp[16] = js_String_getelem1
    648
    ld7 = ld gp[648]
    $global0 = i2f ld7
    #3FF00000:0 /* 1 */
    1
    add1 = add ld7, 1
    ov1 = ov add1
    xt3: xt ov1 -> 0:35 sp+0 rp+0

    i2f1 = i2f add1
    sti sp[0] = add1
    st gp[648] = add1
    sti sp[0] = add1
    #40080000:0 /* 3 */
    3
    sti sp[8] = 3
    lt1 = lt add1, 3
    xf3: xf lt1 -> 0:44 sp+16 rp+0

Checking type stability against self=0x30d390
global0 checkType(tag=1, t=1, isnum=1, i2f=1) stage_count=0
    sti sp[0] = lt1
    st gp[648] = add1
    loop
As you can see in the LIR trace we actually emit a constant "2" for the getelem.
It seems to me that we read an undefined from the newly created array (newinit) and getelem thinks its a string.

00023:  newinit 3
00025:  endinit
00026:  dup
00027:  zero
00028:  getelem
00029:  dup
00030:  zero
00031:  getelem
This is the case where we're supposed to have JSRuntime::anyArrayProtoHasElement set and we're supposed to bail off trace here.  The "2" you're seeing in the LIR is JSVAL_TO_BOOLEAN(JSVAL_VOID).
Well, there are some bugs here.

First of all, note that [].__proto__ === Object.prototype.  It should be Array.prototype, but this has been broken at least since 2 Nov 2008.

anyArrayProtoHasElement isn't being set when we do [].__proto__[0]='a' (which is to say, Object.prototype[0]='a').  The current write barriers do not detect this.  There are two write barriers: one in array_defineProperty and one that is called when __proto__ is set.  Both are inadequate in that they don't cover non-arrays, like Object.prototype; and they don't follow the prototype chain to look for elements.
Anyone patching this? It's my fault, I'll take it unless someone is already hacking on the fix.

/be
Assignee: general → brendan
Status: NEW → ASSIGNED
Priority: -- → P1
Target Milestone: --- → mozilla1.9.1b3
Blocks: 450274
[].__proto__ === Object.prototype is bug 450274.
Attached patch proposed fix (obsolete) — Splinter Review
This adds JSOP_PROTO (fixing bug 450274).

This patch also conservatively but correctly (I hope!) maintains the anyArrayProtoHasElements flag. This means fuzz-generated tests will change what is JITed.

I still have to test that it's not too conservative -- if that flag is set, it stays set and the tracer can't see through holes in dense arrays.

/be
Attachment #353117 - Flags: review?(jorendorff)
Comment on attachment 353117 [details] [diff] [review]
proposed fix

>diff --git a/js/src/jsinterp.cpp b/js/src/jsinterp.cpp
>+            rval = obj->fslots[JSSLOT_PROTO];

This *really* wants to go through js_CheckAccess.

>+            STORE_OPND(-1, rval);
>+          END_CASE(JSOP_PROTO)
> 
>           BEGIN_CASE(JSOP_CALLPROP)
>           {
>             JSObject *aobj;
>             JSPropCacheEntry *entry;
> 
>             lval = FETCH_OPND(-1);
>             if (!JSVAL_IS_PRIMITIVE(lval)) {
>diff --git a/js/src/jsobj.cpp b/js/src/jsobj.cpp
>--- a/js/src/jsobj.cpp
>+++ b/js/src/jsobj.cpp
>@@ -321,22 +321,26 @@ js_SetProtoOrParent(JSContext *cx, JSObj
>                                  (slot == JSSLOT_PROTO) ? js_proto_str
>                                                         : js_parent_str
> #endif
>                                  );
>         }
>         return JS_FALSE;
>     }
> 
>-    // Maintain the "any Array prototype has indexed properties hazard" flag.
>-    if (slot == JSSLOT_PROTO &&
>-        OBJ_IS_ARRAY(cx, pobj) &&
>-        pobj->fslots[JSSLOT_ARRAY_LENGTH] != 0) {
>+    /*
>+     * Maintain the "any Array prototype has indexed properties hazard" flag by
>+     * conservatively setting it. We simply don't know what pobj has in the way
>+     * of indexed properties, either directly or along its prototype chain, and
>+     * we won't expend effort here to find out. This pessimistic approach could
>+     * be improved, but setting __proto__ is quite rare and arguably deserving
>+     * of deoptimization.
>+     */
>+    if (slot == JSSLOT_PROTO)
>         rt->anyArrayProtoHasElement = JS_TRUE;
>-    }
>     return JS_TRUE;
> }
> 
> static JSHashNumber
> js_hash_object(const void *key)
> {
>     return (JSHashNumber)JS_PTR_TO_UINT32(key) >> JSVAL_TAGBITS;
> }
>@@ -3156,29 +3160,34 @@ js_DefineProperty(JSContext *cx, JSObjec
>                                    0, 0, propp);
> }
> 
> /*
>  * Backward compatibility requires allowing addProperty hooks to mutate the
>  * nominal initial value of a slot-full property, while GC safety wants that
>  * value to be stored before the call-out through the hook.  Optimize to do
>  * both while saving cycles for classes that stub their addProperty hook.
>+ *
>+ * As in js_SetProtoOrParent (see above), we maintain the "any Array prototype
>+ * has indexed properties hazard" flag by conservatively setting it.
>  */
> #define ADD_PROPERTY_HELPER(cx,clasp,obj,scope,sprop,vp,cleanup)              \
>     JS_BEGIN_MACRO                                                            \
>         if ((clasp)->addProperty != JS_PropertyStub) {                        \
>             jsval nominal_ = *(vp);                                           \
>             if (!(clasp)->addProperty(cx, obj, SPROP_USERID(sprop), vp)) {    \
>                 cleanup;                                                      \
>             }                                                                 \
>             if (*(vp) != nominal_) {                                          \
>                 if (SPROP_HAS_VALID_SLOT(sprop, scope))                       \
>                     LOCKED_OBJ_WRITE_BARRIER(cx, obj, (sprop)->slot, *(vp));  \
>             }                                                                 \
>         }                                                                     \
>+        if (STOBJ_IS_DELEGATE(obj) && JSID_IS_INT(sprop->id))                 \
>+            cx->runtime->anyArrayProtoHasElement = JS_TRUE;                   \
>     JS_END_MACRO
> 
> JSBool
> js_DefineNativeProperty(JSContext *cx, JSObject *obj, jsid id, jsval value,
>                         JSPropertyOp getter, JSPropertyOp setter, uintN attrs,
>                         uintN flags, intN shortid, JSProperty **propp)
> {
>     JSClass *clasp;
>diff --git a/js/src/jsopcode.cpp b/js/src/jsopcode.cpp
>--- a/js/src/jsopcode.cpp
>+++ b/js/src/jsopcode.cpp
>@@ -880,17 +880,17 @@ GetOff(SprintStack *ss, uintN i)
>     char *bytes;
> 
>     off = ss->offsets[i];
>     if (off >= 0)
>         return off;
> 
>     JS_ASSERT(off <= -2);
>     JS_ASSERT(ss->printer->pcstack);
>-    if (off < -2 && ss->printer->pcstack) {
>+    if (off <= -2 && ss->printer->pcstack) {
>         pc = ss->printer->pcstack[-2 - off];
>         bytes = DecompileExpression(ss->sprinter.context, ss->printer->script,
>                                     ss->printer->fun, pc);
>         if (!bytes)
>             return 0;
>         if (bytes != FAILED_EXPRESSION_DECOMPILER) {
>             off = SprintCString(&ss->sprinter, bytes);
>             if (off < 0)
>@@ -1245,22 +1245,29 @@ GetLocal(SprintStack *ss, jsint i)
>     off = ss->offsets[i];
>     if (off >= 0)
>         return OFF2STR(&ss->sprinter, off);
> 
>     /*
>      * We must be called from js_DecompileValueGenerator (via Decompile) when
>      * dereferencing a local that's undefined or null. Search script->objects
>      * for the block containing this local by its stack index, i.
>+     *
>+     * In case of destructuring's use of JSOP_GETLOCAL, however, there may be
>+     * no such local. This could mean no blocks (no script objects at all, or
>+     * none of the script's object literals are blocks), or the stack slot i is
>+     * not in a block. In either case, return GetStr(ss, i).
>      */
>     cx = ss->sprinter.context;
>     script = ss->printer->script;
>-    LOCAL_ASSERT(script->objectsOffset != 0);
>+    if (script->objectsOffset == 0)
>+        return GetStr(ss, i);
>     for (j = 0, n = JS_SCRIPT_OBJECTS(script)->length; ; j++) {
>-        LOCAL_ASSERT(j < n);
>+        if (j == n)
>+            return GetStr(ss, i);
>         JS_GET_SCRIPT_OBJECT(script, j, obj);
>         if (OBJ_GET_CLASS(cx, obj) == &js_BlockClass) {
>             depth = OBJ_BLOCK_DEPTH(cx, obj);
>             count = OBJ_BLOCK_COUNT(cx, obj);
>             if ((jsuint)(i - depth) < (jsuint)count)
>                 break;
>         }
>     }
>@@ -1522,16 +1529,20 @@ DecompileDestructuring(SprintStack *ss, 
>                     if (SprintPut(&ss->sprinter, ", ", 2) < 0)
>                         return NULL;
>                 }
>             }
>             break;
> 
>           case JSOP_LENGTH:
>             atom = cx->runtime->atomState.lengthAtom;
>+            goto do_destructure_atom;
>+
>+          case JSOP_PROTO:
>+            atom = cx->runtime->atomState.protoAtom;
>             goto do_destructure_atom;
> 
>           case JSOP_CALLPROP:
>           case JSOP_GETPROP:
>             GET_ATOM_FROM_BYTECODE(jp->script, pc, 0, atom);
>           do_destructure_atom:
>             *OFF2STR(&ss->sprinter, head) = '{';
>             str = ATOM_TO_STRING(atom);
>@@ -2788,17 +2799,17 @@ Decompile(SprintStack *ss, jsbytecode *p
>                     atom = GetArgOrVarAtom(jp, i);
>                     LOCAL_ASSERT(atom);
>                     goto do_name;
>                 }
>                 LOCAL_ASSERT((uintN)i < ss->top);
>                 sn = js_GetSrcNote(jp->script, pc);
> 
> #if JS_HAS_DESTRUCTURING
>-                if (sn && SN_TYPE(sn) == SRC_GROUPASSIGN) {
>+                if (sn && SN_TYPE(sn) == SRC_GROUPASSIGN && len > JSOP_GETLOCAL_LENGTH) {
>                     pc = DecompileGroupAssignment(ss, pc, endpc, sn, &todo);
>                     if (!pc)
>                         return NULL;
>                     LOCAL_ASSERT(*pc == JSOP_POPN);
>                     len = oplen = JSOP_POPN_LENGTH;
>                     goto end_groupassignment;
>                 }
> #endif
>@@ -3689,16 +3700,21 @@ Decompile(SprintStack *ss, jsbytecode *p
>                 }
>                 break;
> 
>               case JSOP_LENGTH:
>                 fmt = dot_format;
>                 rval = js_length_str;
>                 goto do_getprop_lval;
> 
>+              case JSOP_PROTO:
>+                fmt = dot_format;
>+                rval = js_proto_str;
>+                goto do_getprop_lval;
>+
>               case JSOP_GETPROP2:
>                 op = JSOP_GETPROP;
>                 (void) PopOff(ss, lastop);
>                 /* FALL THROUGH */
> 
>               case JSOP_CALLPROP:
>               case JSOP_GETPROP:
>               case JSOP_GETXPROP:
>@@ -5069,16 +5085,22 @@ DecompileExpression(JSContext *cx, JSScr
>     }
> 
>     op = (JSOp) *pc;
> 
>     /* None of these stack-writing ops generates novel values. */
>     JS_ASSERT(op != JSOP_CASE && op != JSOP_CASEX &&
>               op != JSOP_DUP && op != JSOP_DUP2);
> 
>+    /* JSOP_PUSH is used to generate undefined for group assignment holes. */
>+    if (op == JSOP_PUSH) {
>+        name = JS_strdup(cx, js_undefined_str);
>+        goto out;
>+    }
>+
>     /*
>      * |this| could convert to a very long object initialiser, so cite it by
>      * its keyword name instead.
>      */
>     if (op == JSOP_THIS) {
>         name = JS_strdup(cx, js_this_str);
>         goto out;
>     }
>diff --git a/js/src/jsopcode.tbl b/js/src/jsopcode.tbl
>--- a/js/src/jsopcode.tbl
>+++ b/js/src/jsopcode.tbl
>@@ -531,17 +531,18 @@ OPDEF(JSOP_UNUSED219,     219,"unused219
>  */
> OPDEF(JSOP_INDEXBASE1,    220,"atombase1",     NULL,  1,  0,  0,  0,  JOF_BYTE |JOF_INDEXBASE)
> OPDEF(JSOP_INDEXBASE2,    221,"atombase2",     NULL,  1,  0,  0,  0,  JOF_BYTE |JOF_INDEXBASE)
> OPDEF(JSOP_INDEXBASE3,    222,"atombase3",     NULL,  1,  0,  0,  0,  JOF_BYTE |JOF_INDEXBASE)
> 
> OPDEF(JSOP_CALLGVAR,      223, "callgvar",     NULL,  3,  0,  2, 19,  JOF_ATOM|JOF_NAME|JOF_CALLOP)
> OPDEF(JSOP_CALLLOCAL,     224, "calllocal",    NULL,  3,  0,  2, 19,  JOF_LOCAL|JOF_NAME|JOF_CALLOP)
> OPDEF(JSOP_CALLARG,       225, "callarg",      NULL,  3,  0,  2, 19,  JOF_QARG |JOF_NAME|JOF_CALLOP)
>-OPDEF(JSOP_UNUSED226,     226, "unused226",    NULL,  1,  0,  1,  1,  JOF_BYTE)
>+
>+OPDEF(JSOP_PROTO,         226, "proto",        NULL,  1,  1,  1, 18,  JOF_BYTE|JOF_PROP)
> 
> /*
>  * Opcodes to hold 8-bit and 32-bit immediate integer operands.
>  */
> OPDEF(JSOP_INT8,          227, "int8",         NULL,  2,  0,  1, 16,  JOF_INT8)
> OPDEF(JSOP_INT32,         228, "int32",        NULL,  5,  0,  1, 16,  JOF_INT32)
> 
> /*
>diff --git a/js/src/jstracer.cpp b/js/src/jstracer.cpp
>--- a/js/src/jstracer.cpp
>+++ b/js/src/jstracer.cpp
>@@ -8453,17 +8453,17 @@ TraceRecorder::record_JSOP_INT32()
> }
> 
> JS_REQUIRES_STACK bool
> TraceRecorder::record_JSOP_LENGTH()
> {
>     jsval& l = stackval(-1);
>     if (JSVAL_IS_PRIMITIVE(l)) {
>         if (!JSVAL_IS_STRING(l))
>-            ABORT_TRACE("non-string primitives unsupported");
>+            ABORT_TRACE("non-string primitive JSOP_LENGTH unsupported");
>         LIns* str_ins = get(&l);
>         LIns* len_ins = lir->insLoad(LIR_ldp, str_ins, (int)offsetof(JSString, length));
> 
>         LIns* masked_len_ins = lir->ins2(LIR_piand,
>                                          len_ins,
>                                          INS_CONSTPTR(JSSTRING_LENGTH_MASK));
> 
>         LIns *choose_len_ins =
>@@ -8484,16 +8484,28 @@ TraceRecorder::record_JSOP_LENGTH()
>     }
> 
>     JSObject* obj = JSVAL_TO_OBJECT(l);
>     if (!OBJ_IS_DENSE_ARRAY(cx, obj))
>         ABORT_TRACE("only dense arrays supported");
>     if (!guardDenseArray(obj, get(&l)))
>         ABORT_TRACE("OBJ_IS_DENSE_ARRAY but not?!?");
>     LIns* v_ins = lir->ins1(LIR_i2f, stobj_get_fslot(get(&l), JSSLOT_ARRAY_LENGTH));
>+    set(&l, v_ins);
>+    return true;
>+}
>+
>+JS_REQUIRES_STACK bool
>+TraceRecorder::record_JSOP_PROTO()
>+{
>+    jsval& l = stackval(-1);
>+    if (JSVAL_IS_PRIMITIVE(l))
>+        ABORT_TRACE("primitive JSOP_PROTO unsupported");
>+
>+    LIns* v_ins = stobj_get_fslot(get(&l), JSSLOT_PROTO);
>     set(&l, v_ins);
>     return true;
> }
> 
> JS_REQUIRES_STACK bool
> TraceRecorder::record_JSOP_NEWARRAY()
> {
>     return false;
>@@ -8577,9 +8589,8 @@ UNUSED(203)
> UNUSED(203)
> UNUSED(204)
> UNUSED(205)
> UNUSED(206)
> UNUSED(207)
> UNUSED(208)
> UNUSED(209)
> UNUSED(219)
>-UNUSED(226)
Flyby: The generated code for JSOP_PROTO needs a null check, except with your patch (which splits Null from Object).
mrbkap: buzzkill!

gal: I will mark the dependency. I'm debugging the patch for bug 465460 now, will stop by to consult with you shortly.

/be
Depends on: 465460
Attached patch proposed fix, v2 (obsolete) — Splinter Review
mrbkap gets r? for his drive-by ;-).

/be
Attachment #353117 - Attachment is obsolete: true
Attachment #353323 - Flags: review?(mrbkap)
Attachment #353117 - Flags: review?(jorendorff)
Attached patch proposed fix, v3 (obsolete) — Splinter Review
Attachment #353323 - Attachment is obsolete: true
Attachment #353351 - Flags: review?(mrbkap)
Attachment #353323 - Flags: review?(mrbkap)
Attached patch proposed fix, v3a (obsolete) — Splinter Review
We really should get rid of unnecessary stack fiddling for temporary roots. Igor had a plan. Then each interpreter case could use an add-signed-immediate to adjust regs.sp, which could be come a register-allocated local again if the GC could compute stack depth quickly from pc, or count on zero-padded stack (with some entrainment due to uncleared, previously-popped values).

/be
Attachment #353351 - Attachment is obsolete: true
Attachment #353353 - Flags: review?(mrbkap)
Attachment #353351 - Flags: review?(mrbkap)
Igor, see comment 15 and feel free to review too.

/be
js_SetProtoOrParent could be less conservative.  If (pobj == NULL), we're definitely safe.  I think we're also safe if (!OBJ_IS_ARRAY(obj) && !OBJ_IS_DELEGATE(obj)).
The check in array_defineProperty can be removed, right?  That path goes through ADD_PROPERTY_HELPER a bit later.  (That check doesn't make much sense to me anyway.)
(In reply to comment #17)
> js_SetProtoOrParent could be less conservative.  If (pobj == NULL), we're
> definitely safe.  I think we're also safe if (!OBJ_IS_ARRAY(obj) &&
> !OBJ_IS_DELEGATE(obj)).

Good points -- I will update the patch.

(In reply to comment #18)
> The check in array_defineProperty can be removed, right?  That path goes
> through ADD_PROPERTY_HELPER a bit later.

No, because array_defineProperty is an object-op -- if you're in it, you are not in jsobj.cpp's js_DefineProperty peer implementation.

> (That check doesn't make much sense to me anyway.)

How not?

/be
> No, because array_defineProperty is an object-op -- if you're in it, you are
> not in jsobj.cpp's js_DefineProperty peer implementation.

Er, I'm blind. Never mind, new patch soon.

/be
Attached patch proposed fix, v4 (obsolete) — Splinter Review
Attachment #353353 - Attachment is obsolete: true
Attachment #353509 - Flags: review?(jorendorff)
Attachment #353353 - Flags: review?(mrbkap)
OS: Mac OS X → All
Hardware: PC → All
Attachment #353509 - Flags: review?(mrbkap)
As Jason points out, now slow array property creation funnels through the native JSObjectOp.defineProperty, js_DefineProperty, which of course goes through js_DefineNativeProperty and into the ADD_PROPERTY_HELPER macro -- and that macro now has the rt->anyArrayProtoHasElement setting logic.

/be
Flags: blocking1.9.1? → blocking1.9.1+
Comment on attachment 353509 [details] [diff] [review]
proposed fix, v4

The anyArrayProtoHasElement stuff looks good.

The JSOP_PROTO part leaves a bug:

js> Array.prototype.__proto__ = function () 3;            
function () 3
js> [].__proto__()
3

It should be a TypeError.  Perhaps this can be easily fixed with a special case in the emitter: emit
  string "__proto__"
  callelem
instead of
  callprop "__proto__"

I guess getprop "__proto__" could be implemented in the same way.  Then we wouldn't need JSOP_PROTO.  Alternatively we could have JSOP_CALLPROTO.  :-)
Attachment #353509 - Flags: review?(jorendorff) → review+
Heh, I didn't think of using getelem/callelem -- that's smart!

/be
Attached patch proposed fix, v5 (obsolete) — Splinter Review
Much better -- thanks for the great suggestion, Jason. I hope I didn't leave any uncovered cases. Everything tests well, even decompiles nicely:

js> function f()o.__proto__;    
js> f 
function f() o.__proto__;

/be
Attachment #353509 - Attachment is obsolete: true
Attachment #353604 - Flags: review?(jorendorff)
Attachment #353509 - Flags: review?(mrbkap)
Comment on attachment 353604 [details] [diff] [review]
proposed fix, v5

Can't entirely r+ this yet, but I'll finish the review today.  Long story, but I'm without a development environment for the moment. So the observations below are thanks to bugzilla+hgweb+mxr.

>+    /*
>+     * Special case for obj.__proto__ to deoptimize away from fast paths in the
>+     * interpreter and trace recorder, which skip dense array instances by going
>+     * up to Array.prototype before looking up the property name.
>+     */

This comment could also say "See bug 450274."

It would be nice if instead of having special cases in two places, the += code could be made to route through EmitPropOp. Maybe that's inconvenient, in which case never mind, but I couldn't tell offhand.

I'll also look at the other places where GETPROP is emitted.

Anyway, this looks great. I like it a lot better than a new opcode!

That leaves the changes in jsopcode.cpp and jstracer.cpp, which I think are unrelated fixes. They seem OK but I don't really grok. I'll look more closely at them later today and most likely r+ this in the afternoon.

Btw, if you want to split e.g. the decompiler fixes into a separate MQ patch, you can:
  hg qrefresh --exclude jsopcode.cpp
  hg qnew -f jsopcode-fixes
OK, the disassembler changes are for this bug:

  function f(x) {
    var [a, b, [c0, c1]] = [x, x, x];
  }
  f(null)
Assertion failure: script->objectsOffset != 0, at ../jsopcode.cpp:1256

There is a "group assignment" emitter optimization that avoids creating the array and uses JSOP_GETLOCAL to access operand stack slots!  Exciting!  Btw, this optimization has a buggy corner case when the rhs has holes:

  js> Array.prototype[1] = 'y';
  y
  js> var [x, y, z] = ['x', , 'z'];
  js> print(y)
  undefined

I think it would be fair to leave this case unoptimized.  Separate bug.
Attachment #353604 - Flags: review?(jorendorff) → review+
Comment on attachment 353604 [details] [diff] [review]
proposed fix, v5

I think I was wrong--there aren't any other places GETPROP can be emitted.  r=me.
Depends on: 470374
(In reply to comment #26)
> It would be nice if instead of having special cases in two places, the += code
> could be made to route through EmitPropOp. Maybe that's inconvenient, in which
> case never mind, but I couldn't tell offhand.

The QNAMEPART / GETELEM sequence is special, would require factoring those four lines out of EmitElemOp (not calling EmitPropOp) -- not worth it.

> Btw, if you want to split e.g. the decompiler fixes into a separate MQ patch,
> you can:
>   hg qrefresh --exclude jsopcode.cpp
>   hg qnew -f jsopcode-fixes

Thanks, I've filed bug 470374.

(In reply to comment #27)
> There is a "group assignment" emitter optimization that avoids creating the
> array and uses JSOP_GETLOCAL to access operand stack slots!  Exciting!  Btw,
> this optimization has a buggy corner case when the rhs has holes:
> 
>   js> Array.prototype[1] = 'y';
>   y
>   js> var [x, y, z] = ['x', , 'z'];
>   js> print(y)
>   undefined
> 
> I think it would be fair to leave this case unoptimized.  Separate bug.

How is this a bug? Destructuring desugars to assignment, and ['x',,'z'][1] reads as undefined.

/be
Fixed in m-c:

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

/be
Status: ASSIGNED → RESOLVED
Closed: 16 years ago
Resolution: --- → FIXED
Whiteboard: fixed-in-tracemonkey
Something's very wrong.

/be
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Whiteboard: fixed-in-tracemonkey
Status: REOPENED → ASSIGNED
Backed out of tm and m-c.

/be
(In reply to comment #29)
> (In reply to comment #27)
> > There is a "group assignment" emitter optimization that avoids creating the
> > array and uses JSOP_GETLOCAL to access operand stack slots!  Exciting!  Btw,
> > this optimization has a buggy corner case when the rhs has holes:
> > 
> >   js> Array.prototype[1] = 'y';
> >   y
> >   js> var [x, y, z] = ['x', , 'z'];
> >   js> print(y)
> >   undefined
> > 
> > I think it would be fair to leave this case unoptimized.  Separate bug.
> 
> How is this a bug? Destructuring desugars to assignment, and ['x',,'z'][1]
> reads as undefined.

Not once you've assigned Array.prototype[1] = 'y'.  There's a hole in the array.
Oh, of course -- yes, we should just de-optimize that group assignment from holey array case.

/be
Do we know what was so very wrong here?  Want me to take?
No idea -- just bad test results => back-out. Please take, I'm still officially on vacation, back on the 2nd. Thanks,

/be
(In reply to comment #27)
> I think it would be fair to leave this case unoptimized.  Separate bug.

Bug 471703.

/be
Comment on attachment 353604 [details] [diff] [review]
proposed fix, v5

This is first of three patches in three bugs (see blocking chain), the second of which is 1.9.1+, where all three want to get into 1.9.1.

/be
Attachment #353604 - Flags: approval1.9.1?
Attachment #353604 - Flags: approval1.9.1? → approval1.9.1+
(In reply to comment #27)

Jason, is this expected or covered by an existing bug?

 function f(x) { var [a, b, [c0, c1]] = [x, x, x]; }
 f(null)

TypeError on line 2: x is null
I thought graydon was taking this one.

Bob, that result is correct. You can't destructure null into a two-element array (the [c0, c1] nested in the outer array pattern on the left, after var). The diag is citing the formal parameter whose value the function is trying to destructure, by name, which seems good too. What's wrong?

/be
(partly caffeinated so I may be missing something, don't mean to sound snarky ;-)
no snarkiness perceived.
Checking in js1_8/decompilation/regress-469625-01.js
http://hg.mozilla.org/mozilla-central/rev/eaad4041922a

test for bug 470374
Flags: in-testsuite?
Flags: in-litmus-
Attempting to land it on TM:

http://hg.mozilla.org/tracemonkey/rev/7246c4dcf997
Assignee: brendan → gal
Seems to stick.
Whiteboard: fixed-in-tracemonkey
Broke trace-test.js, even with the tracer turned off.

...
testCaseTypeMismatchBadness : passed
testDoubleComparison : passed
testLirBufOOM : passed
trace-test.js:4028: TypeError: invalid 'in' operand d
Yeah, as well as the preceding failures in testTypeUnstableForIn and testForEach.  Gonna try to remember how to back things out in hg now!
Done.
Whiteboard: fixed-in-tracemonkey
Mea culpa. I didn't try trace-tests before pushing it in.
This is the bug that already carried in parts of the patch:

# HG changeset patch
# User Brendan Eich <brendan@mozilla.org>
# Date 1230621743 28800
# Node ID d0e8862aa513f3509a176006ef2bf43f6d3120a4
# Parent  a2d17feae1836b45d31816e31fd079e1fbdad1d9
Bug 470374 - Decompiler fixes from bug 469625 (r=jorendorff).
Attached file test case
With the patch even with JIT off the following test case produces 17. Without we produce 17. I think the bug is actually in the existing code. The new improved detection of array __proto__ fiddling just exposes it.
... without we produce 16. Need more caffeine.
Pushed the trackCfgMerges fix separately: (cosmetic issue, unrelated to this bug)

http://hg.mozilla.org/tracemonkey/rev/aca68a29101d
So what seems to happen here is that u doesn't get properly delete and still gets enumerated with this patch applied.
crowder is gonna split the jsemit.cpp and jstracer.cpp changes out as the patch for bug 450274 (and cover __parent__ and __count__ as well as __proto__, bonus).

That will leave this bug's patch pretty small. Should be easy to fix, right? :-P

/be
Yeah. Particularly given this simpler testcase:

[].__proto__.u = 10;
delete [].__proto__.u;
print([].u);

prints "10" with the patch, "undefined" without. Should be tractable at this point? I'll continue sniffing around.
Tractable per IRC. More emitter hacking fun. Maybe this should all be combined with the stuff crowder was gonna split out.

/be
The split is done, fwiw, see bug 450274
Attached patch Proposed fix (obsolete) — Splinter Review
The deoptimization was incomplete, and didn't handle cases in which a .__proto__ property access occurred as part of a chain of TOK_DOT nodes. 

As far as I know, this variant completes the deoptimization. But this is extremely vague territory for me, and I have little confidence it's correct; happy to have more eyes on it.
Attachment #359860 - Flags: review?(brendan)
Comment on attachment 359860 [details] [diff] [review]
Proposed fix

> static JSBool
>+EmitDotProtoOp(JSContext *cx, JSParseNode *pn, JSOp op, JSCodeGenerator *cg)

Suggest EmitDotMagicOp or EmitMagicOp and make it work for protoAtom, parentAtom, and countAtom, as per crowder's patch in bug 450274. One of you collides and hand-merges, btw.

>+{
>+    /*
>+     * Special case for obj.__proto__ to deoptimize away from fast paths in the
>+     * interpreter and trace recorder, which skip dense array instances by going
>+     * up to Array.prototype before looking up the property name.
>+     */
>+    JS_ASSERT(pn->pn_atom == cx->runtime->atomState.protoAtom);
>+    if (pn->pn_expr)
>+        if (!js_EmitTree(cx, cg, pn->pn_expr))
>+            return JS_FALSE;

Nit: prevailing style would use && or, for if-if based on some overriding win, brace the outer then clause.

More substantively, doesn't this recur potentially quite deeply? I guess we have to, instead of pointer-reversing linked list traversal? If we have no choice, comment the inner if and of course brace. That's one overriding win motivating if-if instead of if (&&).

>+    JSAtomListElement *ale = js_IndexAtom(cx, pn->pn_atom, &cg->atomList);
>+    if (!ale)
>+        return JS_FALSE;
>+    jsatomid atomIndex = ALE_INDEX(ale);
>+    if (!EmitIndexOp(cx, JSOP_QNAMEPART, atomIndex, cg))
>+        return JS_FALSE;

We usually avoid single-use vars esp. if the initializer is simple enough, so no need for atomIndex.

>+    /* Special case deoptimization on .__proto__. */
>+    if (pn->pn_arity == PN_NAME && 
>+        pn->pn_atom == cx->runtime->atomState.protoAtom)
>+        return EmitDotProtoOp(cx, pn, callContext ? JSOP_CALLELEM : JSOP_GETELEM, cg);

Any of if (condition), then, and else taking more than one line means bracing all control structures in house style. Here, you could avoid by using >80 columns for the condition all on one line (tw=99 new world order). Your call.

>+            /* Special case deoptimization on .__proto__, as above. */
>+            if (pndot->pn_arity == PN_NAME && 
>+                pndot->pn_atom == cx->runtime->atomState.protoAtom) {
>+                if (!EmitDotProtoOp(cx, pndot, JSOP_GETELEM, cg))
>+                    return JS_FALSE;
>+            } else if (!EmitAtomOp(cx, pndot, PN_OP(pndot), cg))
>                 return JS_FALSE;

Another case where all controlled statements get braces if any do -- and even better: make the else brace its if-return for parallel structure.

/be
(In reply to comment #63)
> (From update of attachment 359860 [details] [diff] [review])
> > static JSBool
> >+EmitDotProtoOp(JSContext *cx, JSParseNode *pn, JSOp op, JSCodeGenerator *cg)
> 
> Suggest EmitDotMagicOp or EmitMagicOp and make it work for protoAtom,
> parentAtom, and countAtom, as per crowder's patch in bug 450274. One of you
> collides and hand-merges, btw.

Or really: just subsume crowder's patch here, make his bug block on this one, and we can all go to the beach sooner ;-).

/be
Assignee: gal → graydon
Attached patch Revised patchSplinter Review
This variant corrects (most of) the style points above and afaict subsumes crowder's patch for bug 450274. Mochitesting presently.

Note that the cited deep-recursion is eliminated when walking lists of DOT exprs by moving the emitTree call to the sole early-return case and letting the existing list-walk case handle the list-of-DOTs. Worryingly, both variants of the patch seem to work, although I think the former variant was incorrect, as it would both emitTree *and* carry on walking the expr list; but I'm not *sure*. So I think I do not have a testcase that properly flexes that corner of the patch. Suggestions welcome.
Attachment #359860 - Attachment is obsolete: true
Attachment #360354 - Flags: review?(brendan)
Attachment #359860 - Flags: review?(brendan)
(In reply to comment #65)
> This variant corrects (most of) the style points above and afaict subsumes
> crowder's patch for bug 450274. Mochitesting presently.

Great, thanks -- I'll pick uber-nits on my own time (the various orderings of proto/parent/count is destroying me ;-).

> Note that the cited deep-recursion is eliminated when walking lists of DOT
> exprs by moving the emitTree call to the sole early-return case and letting the
> existing list-walk case handle the list-of-DOTs. Worryingly, both variants of
> the patch seem to work, although I think the former variant was incorrect, as
> it would both emitTree *and* carry on walking the expr list; but I'm not
> *sure*.

Which patch in this bug is the former variant? The currently committed code never recurs from EmitPropOp to js_EmitTree for TOK_DOT nodes along the chain, only when the bottom (pndown->pn_type != TOK_DOT) node is reached.

> So I think I do not have a testcase that properly flexes that corner of
> the patch. Suggestions welcome.

Patch looks good apart from nit whinage. Any new test results? I'll stamp r+ in a minute.

/be
Nevermind, I found myself a testcase that helps me believe my own lies. All the tests I know how to run seem fine.
Attachment #360354 - Flags: review?(brendan) → review+
Landed http://hg.mozilla.org/tracemonkey/rev/569acf636d50 , will see if it sticks.
http://hg.mozilla.org/mozilla-central/rev/569acf636d50
Status: ASSIGNED → RESOLVED
Closed: 16 years ago15 years ago
Resolution: --- → FIXED
http://hg.mozilla.org/tracemonkey/rev/d46c0b0ff9fa
/cvsroot/mozilla/js/tests/js1_5/extensions/regress-469625.js,v  <--  regress-469625.js
initial revision: 1.1
/cvsroot/mozilla/js/tests/js1_8/extensions/regress-469625.js,v  <--  regress-469625.js
initial revision: 1.1
Flags: in-testsuite? → in-testsuite+
everything except js1_8/regress/regress-469625-02.js passes (see bug 471703)
verified 1.9.1, 1.9.2
Status: RESOLVED → VERIFIED
Flags: wanted1.9.0.x?
Crash Signature: [@ js_String_getelem]
Flags: wanted1.9.0.x?
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: