Closed
Bug 469625
Opened 16 years ago
Closed 16 years ago
TM: Crash [@ js_String_getelem]
Categories
(Core :: JavaScript Engine, defect, P1)
Core
JavaScript Engine
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)
(deleted),
application/x-javascript
|
Details | |
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
patch
|
brendan
:
review+
|
Details | Diff | Splinter Review |
[].__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?
Comment 1•16 years ago
|
||
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.
Comment 2•16 years ago
|
||
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
Comment 3•16 years ago
|
||
As you can see in the LIR trace we actually emit a constant "2" for the getelem.
Comment 4•16 years ago
|
||
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
Comment 5•16 years ago
|
||
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).
Comment 6•16 years ago
|
||
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.
Comment 7•16 years ago
|
||
Anyone patching this? It's my fault, I'll take it unless someone is already hacking on the fix.
/be
Updated•16 years ago
|
Assignee: general → brendan
Status: NEW → ASSIGNED
Priority: -- → P1
Target Milestone: --- → mozilla1.9.1b3
Comment 8•16 years ago
|
||
[].__proto__ === Object.prototype is bug 450274.
Comment 9•16 years ago
|
||
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 10•16 years ago
|
||
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)
Comment 11•16 years ago
|
||
Flyby: The generated code for JSOP_PROTO needs a null check, except with your patch (which splits Null from Object).
Comment 12•16 years ago
|
||
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
Comment 13•16 years ago
|
||
mrbkap gets r? for his drive-by ;-).
/be
Attachment #353117 -
Attachment is obsolete: true
Attachment #353323 -
Flags: review?(mrbkap)
Attachment #353117 -
Flags: review?(jorendorff)
Comment 14•16 years ago
|
||
Attachment #353323 -
Attachment is obsolete: true
Attachment #353351 -
Flags: review?(mrbkap)
Attachment #353323 -
Flags: review?(mrbkap)
Comment 15•16 years ago
|
||
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)
Comment 16•16 years ago
|
||
Igor, see comment 15 and feel free to review too.
/be
Comment 17•16 years ago
|
||
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)).
Comment 18•16 years ago
|
||
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.)
Comment 19•16 years ago
|
||
(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
Comment 20•16 years ago
|
||
> 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
Comment 21•16 years ago
|
||
Attachment #353353 -
Attachment is obsolete: true
Attachment #353509 -
Flags: review?(jorendorff)
Attachment #353353 -
Flags: review?(mrbkap)
Updated•16 years ago
|
OS: Mac OS X → All
Hardware: PC → All
Updated•16 years ago
|
Attachment #353509 -
Flags: review?(mrbkap)
Comment 22•16 years ago
|
||
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
Updated•16 years ago
|
Flags: blocking1.9.1? → blocking1.9.1+
Comment 23•16 years ago
|
||
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+
Comment 24•16 years ago
|
||
Heh, I didn't think of using getelem/callelem -- that's smart!
/be
Comment 25•16 years ago
|
||
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 26•16 years ago
|
||
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
Comment 27•16 years ago
|
||
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.
Updated•16 years ago
|
Attachment #353604 -
Flags: review?(jorendorff) → review+
Comment 28•16 years ago
|
||
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.
Comment 29•16 years ago
|
||
(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
Comment 30•16 years ago
|
||
Fixed in m-c:
http://hg.mozilla.org/mozilla-central/rev/e98754e147eb
/be
Status: ASSIGNED → RESOLVED
Closed: 16 years ago
Resolution: --- → FIXED
Updated•16 years ago
|
Whiteboard: fixed-in-tracemonkey
Comment 31•16 years ago
|
||
Something's very wrong.
/be
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Whiteboard: fixed-in-tracemonkey
Updated•16 years ago
|
Status: REOPENED → ASSIGNED
Comment 32•16 years ago
|
||
Backed out of tm and m-c.
/be
Comment 33•16 years ago
|
||
(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.
Comment 34•16 years ago
|
||
Oh, of course -- yes, we should just de-optimize that group assignment from holey array case.
/be
Comment 35•16 years ago
|
||
Do we know what was so very wrong here? Want me to take?
Comment 36•16 years ago
|
||
No idea -- just bad test results => back-out. Please take, I'm still officially on vacation, back on the 2nd. Thanks,
/be
Comment 37•16 years ago
|
||
(In reply to comment #27)
> I think it would be fair to leave this case unoptimized. Separate bug.
Bug 471703.
/be
Comment 38•16 years ago
|
||
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?
Updated•16 years ago
|
Attachment #353604 -
Flags: approval1.9.1? → approval1.9.1+
Comment 39•16 years ago
|
||
(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
Comment 40•16 years ago
|
||
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
Comment 41•16 years ago
|
||
(partly caffeinated so I may be missing something, don't mean to sound snarky ;-)
Comment 42•16 years ago
|
||
no snarkiness perceived.
Comment 43•16 years ago
|
||
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-
Comment 44•16 years ago
|
||
Attempting to land it on TM:
http://hg.mozilla.org/tracemonkey/rev/7246c4dcf997
Assignee: brendan → gal
Assignee | ||
Comment 46•16 years ago
|
||
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
Comment 49•16 years ago
|
||
Mea culpa. I didn't try trace-tests before pushing it in.
Comment 50•16 years ago
|
||
Attachment #353604 -
Attachment is obsolete: true
Comment 51•16 years ago
|
||
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).
Comment 52•16 years ago
|
||
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.
Comment 53•16 years ago
|
||
... without we produce 16. Need more caffeine.
Comment 54•16 years ago
|
||
Attachment #359700 -
Attachment is obsolete: true
Comment 55•16 years ago
|
||
Pushed the trackCfgMerges fix separately: (cosmetic issue, unrelated to this bug)
http://hg.mozilla.org/tracemonkey/rev/aca68a29101d
Comment 56•16 years ago
|
||
Comment 57•16 years ago
|
||
So what seems to happen here is that u doesn't get properly delete and still gets enumerated with this patch applied.
Comment 58•16 years ago
|
||
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
Assignee | ||
Comment 59•16 years ago
|
||
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.
Comment 60•16 years ago
|
||
Tractable per IRC. More emitter hacking fun. Maybe this should all be combined with the stuff crowder was gonna split out.
/be
Comment 61•16 years ago
|
||
The split is done, fwiw, see bug 450274
Assignee | ||
Comment 62•16 years ago
|
||
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.
Assignee | ||
Updated•16 years ago
|
Attachment #359860 -
Flags: review?(brendan)
Comment 63•16 years ago
|
||
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
Comment 64•16 years ago
|
||
(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
Updated•16 years ago
|
Assignee: gal → graydon
Assignee | ||
Comment 65•16 years ago
|
||
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)
Comment 66•16 years ago
|
||
(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
Assignee | ||
Comment 67•16 years ago
|
||
Nevermind, I found myself a testcase that helps me believe my own lies. All the tests I know how to run seem fine.
Updated•16 years ago
|
Attachment #360354 -
Flags: review?(brendan) → review+
Assignee | ||
Comment 68•16 years ago
|
||
Landed http://hg.mozilla.org/tracemonkey/rev/569acf636d50 , will see if it sticks.
Comment 69•16 years ago
|
||
Status: ASSIGNED → RESOLVED
Closed: 16 years ago → 16 years ago
Resolution: --- → FIXED
Comment 70•16 years ago
|
||
Keywords: fixed1.9.1
Comment 71•16 years ago
|
||
Comment 72•16 years ago
|
||
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+
Comment 73•16 years ago
|
||
everything except js1_8/regress/regress-469625-02.js passes (see bug 471703)
verified 1.9.1, 1.9.2
Status: RESOLVED → VERIFIED
Keywords: fixed1.9.1 → verified1.9.1
Updated•16 years ago
|
Flags: wanted1.9.0.x?
Updated•14 years ago
|
Crash Signature: [@ js_String_getelem]
Updated•9 years ago
|
Flags: wanted1.9.0.x?
You need to log in
before you can comment on or make changes to this bug.
Description
•