570237 - JM: Implement JSOP_SETELEM, JSOP_GETELEM.

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Sean Stangl [:sstangl]]

Attachment: 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.

Comment 1

[Sean Stangl [:sstangl]]

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 2

[David Anderson [:dvander] - inactive, e-mail if emergency]

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, &regs.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.

Comment 3

[Sean Stangl [:sstangl]]

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.