921571 - IonMonkey: SetElementIC dense element stub ignores double arrays

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, defect)

Description

[Jan de Mooij [:jandem]]

Attachment: Testcase

The attached testcase crashes on OS X 32-bit, inbound rev 961256d21b8e:

Assertion failure: JSVAL_IS_DOUBLE_IMPL(l), at ../dist/include/js/Value.h:404

With an opt build there's a correctness problem:

test.js:14:1 Error: Assertion failed: got 44, expected 26350

Comment 1

[Jan de Mooij [:jandem]]

In case it's not clear from the title, the problem is that the SetElementIC stub writes an int32 value to an array with the ObjectElements::CONVERT_DOUBLE_ELEMENTS flag. Ion code then expects a double and finds an int32 value.

Comment 2

[Daniel Veditz [:dveditz]]

What's the failure case here? We just interpret the number wrong? Or do we scoop up an extra byte to make the double and then misinterpret everything from that point on?

Comment 3

[Andrew McCreight [:mccr8]]

int-float confusion doesn't sound too bad

Comment 4

[Jan de Mooij [:jandem]]

Attachment: Patch

This fell off my radar but, finally, here's a fix.

It's a bit unfortunate that we have to handle all these ConstantOrRegister cases, but this is the most efficient way to do it I think and pretty straight-forward.

Comment 5

[Shu-yu Guo [:shu]]

Comment on attachment 8359296 [details] [diff] [review]
Patch

Review of attachment 8359296 [details] [diff] [review]:
-----------------------------------------------------------------

I'm cancelling review until I'm convinced of the reg.type() assertion. You're probably right, but I'd like to understand why.

::: js/src/jit/IonCaches.cpp
@@ +3543,5 @@
> +        if (v.isInt32()) {
> +            Label dontConvert;
> +            masm.branchTest32(Assembler::Zero, elementsFlags,
> +                              Imm32(ObjectElements::CONVERT_DOUBLE_ELEMENTS),
> +                              &dontConvert);

I'm not clear on the mutual exclusion of the members of ObjectElements::Flags. CONVERT_DOUBLE_ELEMENTS doesn't seem like it can coexist with ASMJS_ARRAY_BUFFER or NEUTERED_BUFFER, but what about NONWRITABLE_ARRAY_LENGTH?

If so, this needs to be and'd here.

@@ +3562,5 @@
> +
> +    Label convert, storeValue, done;
> +    masm.branchTest32(Assembler::NonZero, elementsFlags,
> +                      Imm32(ObjectElements::CONVERT_DOUBLE_ELEMENTS),
> +                      &convert);

Ditto.

@@ +3573,5 @@
> +        masm.branchTestInt32(Assembler::NotEqual, reg.valueReg(), &storeValue);
> +        masm.int32ValueToDouble(reg.valueReg(), ScratchFloatReg);
> +        masm.storeDouble(ScratchFloatReg, target);
> +    } else {
> +        JS_ASSERT(reg.type() == MIRType_Int32);

Why should this assert hold? What if we lowered into a LSetElementCacheT on a non-int32 type and tried to emit this dense stub?

Comment 6

[Jan de Mooij [:jandem]]

(In reply to Shu-yu Guo [:shu] from comment #5)
> I'm not clear on the mutual exclusion of the members of
> ObjectElements::Flags. CONVERT_DOUBLE_ELEMENTS doesn't seem like it can
> coexist with ASMJS_ARRAY_BUFFER or NEUTERED_BUFFER, but what about
> NONWRITABLE_ARRAY_LENGTH?
> 
> If so, this needs to be and'd here.

branchTest32 *does* a bitwise-and. We're testing if the flag is set or not, it will work if there are other flags.

> Why should this assert hold? What if we lowered into a LSetElementCacheT on
> a non-int32 type and tried to emit this dense stub?

There's a "if (reg.hasTyped() && reg.type() != MIRType_Int32)" check before this. In that case we just do the store and return early.

Comment 7

[Shu-yu Guo [:shu]]

(In reply to Jan de Mooij [:jandem] from comment #6)
> (In reply to Shu-yu Guo [:shu] from comment #5)
> > I'm not clear on the mutual exclusion of the members of
> > ObjectElements::Flags. CONVERT_DOUBLE_ELEMENTS doesn't seem like it can
> > coexist with ASMJS_ARRAY_BUFFER or NEUTERED_BUFFER, but what about
> > NONWRITABLE_ARRAY_LENGTH?
> > 
> > If so, this needs to be and'd here.
> 
> branchTest32 *does* a bitwise-and. We're testing if the flag is set or not,
> it will work if there are other flags.

Ah silly me, it's a test, not a cmp.

> 
> > Why should this assert hold? What if we lowered into a LSetElementCacheT on
> > a non-int32 type and tried to emit this dense stub?
> 
> There's a "if (reg.hasTyped() && reg.type() != MIRType_Int32)" check before
> this. In that case we just do the store and return early.

I missed that part also. Sloppy read through.

Comment 8

[Carsten Book [:Tomcat]]

landed on https://hg.mozilla.org/mozilla-central/rev/d3f32887012e

i guess jan landed that before on inbound but that was checkin comment was missed

Comment 9

[Jan de Mooij [:jandem]]

Comment on attachment 8359296 [details] [diff] [review]
Patch

Apparently this fixes some problems with pythonfiddle.com, see bug 959655 and bug 961124, so I think we should backport this.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Bug 774006.
User impact if declined: Broken websites.
Testing completed: On m-c.
Risk to taking this patch (and alternatives if risky): Low.
String or UUID changes made by this patch: None.

[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration:
User impact if declined: Fixes some problems with real-world websites.
Fix Landed on Version: 29, will likely be backported.
Risk to taking this patch (and alternatives if risky): Low.
String or UUID changes made by this patch: None.

Comment 10

[Jan de Mooij [:jandem]]

Pushed to beta as requested on IRC:

https://hg.mozilla.org/releases/mozilla-beta/rev/90107551b8f5

Comment 11

[Jan de Mooij [:jandem]]

Setting checkin-needed for aurora.

Comment 12

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/bd0f87902b62

Comment 13

[Preeti Raghunath(:Preeti)]

Comment on attachment 8359296 [details] [diff] [review]
Patch

Minus on approval‑mozilla‑b2g26. This is a low sec issue.

Comment 14

[Lukas Blakk [:lsblakk] use ?needinfo]

Comment on attachment 8359296 [details] [diff] [review]
Patch

minus for esr 24, this doesn't meet criteria and can ride the trains.