Created attachment 449360 [details] [diff] [review] Implement JSOP_SETELEM, JSOP_GETELEM. The attached patch implements JSOP_SETELEM and JSOP_GETELEM on the moo branch. Applying the patch currently trips an assert in trace-tests:sunspider/check-mont.js, a smaller test case of which will be attached in the next post. The problem appears to be related to setlocal, a bug for which is being created imminently. All other tests are passing.
Attachment #449360 - Flags: review?(dvander)
A recent change by dvander causes sunspider/check-mont.js to pass with the above patch applied. A separate bug is being created for setlocal, which is unrelated to this patch.
Comment on attachment 449360 [details] [diff] [review] Implement JSOP_SETELEM, JSOP_GETELEM. >+ obj = &lval.asObject(); >+ if (!obj) >+ THROW(); This check be js_ValueToNonNullObject, as the VALUE_TO_OBJECT macro calls. Otherwise no exception is set and the error will be uncatchable. >+ if (!objval.isObject()) >+ THROW(); Ditto. >+ mid_setelem: >+ if (!obj->setProperty(cx, id, ®s.sp[-1])) >+ THROW(); >+ >+ end_setelem: >+ /* :FIXME: Moving the assigned object into the lowest stack slot >+ * is a temporary hack. What we actually want is an implementation >+ * of popAfterSet() that allows popping more than one value; >+ * this logic can then be handled in Compiler.cpp. */ >+ regs.sp[-3] = regs.sp[-1]; >+} The value of |regs.sp[-1]| must be cached locally because mid_setelem can change the value - that is, we must propagate the RHS, not LHS. r=me with those fixed.
Attachment #449360 - Flags: review?(dvander) → review+
Patch pushed. The temporary fix that caused sunspider/check-mont.js was backed out from the repo, so this patch knowingly fails on that test.
Status: NEW → RESOLVED
Last Resolved: 8 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.