JM: Implement JSOP_SETELEM, JSOP_GETELEM.

RESOLVED FIXED

Status

()

RESOLVED FIXED
8 years ago
8 years ago

People

(Reporter: sstangl, Unassigned)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

(Reporter)

Description

8 years ago
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)
(Reporter)

Comment 1

8 years ago
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, &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.
Attachment #449360 - Flags: review?(dvander) → review+
(Reporter)

Comment 3

8 years ago
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.