998059 - Differential Testing: IonMonkey gives incorrect result for typedArray.length

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, defect)

Description

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

x = Uint16Array()
x.__proto__ = [0]
Object.defineProperty(this, "y", {
    get: function() {
        return x.length
    }
})
y
a = y
x = Int32Array
print(uneval(a))


$   ./js-opt-64-dm-ts-linux-9f9e83390b46 --fuzzing-safe --ion-parallel-compile=off w3462-cj-in.js
1

$   ./js-opt-64-dm-ts-linux-9f9e83390b46 --fuzzing-safe --ion-parallel-compile=off --ion-eager w3462-cj-in.js
0

(Tested this on 64-bit Linux js opt threadsafe deterministic shell off m-c rev 9f9e83390b46)

My configure flags (Linux) are:

AR=ar sh /home/fuzz2lin/trees/mozilla-inbound/js/src/configure --enable-optimize --disable-debug --enable-profiling --enable-gczeal --enable-debug-symbols --disable-tests --enable-more-deterministic --with-ccache --enable-threadsafe <other NSPR options>

autoBisect shows this is probably related to the following changeset:

The first bad revision is:
changeset:   http://hg.mozilla.org/mozilla-central/rev/6f8ea87eb8d1
user:        Brian Hackett
date:        Thu Mar 06 14:03:03 2014 -0700
summary:     Bug 980013 - Watch for length accesses on typed arrays with overridden prototypes, r=luke.

Since Brian is away for awhile, switching to Hannes since he's on a roll. :) Hannes, is this related to bug 980013?

(Please feel free to pass it back, in case you feel it isn't your area)

Comment 1

[Hannes Verschore [:h4writer]]

Attachment: Test prototype

So since the length property is on the __prototype__ for typedarrays. Adjusting the prototype also changes the value of length. I.e. setting the Array prototype, will result in returning the Array length instead of the typedArrays length. So I think we  also need to test the prototype to be a typedArray before doing the optimization.

Comment 2

[Jeff Walden [:Waldo]]

Comment on attachment 8408900 [details] [diff] [review]
Test prototype

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

::: js/src/jit/IonBuilder.cpp
@@ +8083,5 @@
>          }
>  
> +        if (objTypes && objTypes->getCommonPrototype() &&
> +            objTypes->getTypedArrayType() != ScalarTypeDescr::TYPE_MAX &&
> +            objTypes->getCommonPrototype()->is<TypedArrayObject>())

The length property on %TypedArray% (Int8Array.prototype and so on) is a configurable accessor property.  If we're going to inline accesses and not have them go through the actual function, you need to check everything that would be checked in that case -- the absence of a property on the typed array directly, the presence of it on the prototype, and the identity of that accessor's getter function (note that the Int8Array.prototype.length will throw on a Uint8Array, and so on).

We can do the fast path for arrays because "length" on an array is a non-configurable own property.  That's not the case for "length" on typed arrays.  It seems like it'd be much better, to me, if this optimization were performed using the MCallOptimize.cpp paths, so you pick up all the property checking for free.

Comment 3

[Brian Hackett [Laid off!]]

Comment on attachment 8408900 [details] [diff] [review]
Test prototype

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

Yeah, see Jeff's review comment.  The common mechanism to use though is getPropTryCommonGetter() though.  The main problem with that is that it requires a shape guard even in the best case because we don't track type information for getter/setter properties.  This could be fixed I think (for typed arrays and for all other prototype getter/setter properties) by using the object state change machinery to detect invalidation when a getter/setter property on an object is redefined.

Comment 4

[Eric Faust [:efaust]]

(In reply to Brian Hackett (:bhackett) from comment #3)
> Comment on attachment 8408900 [details] [diff] [review]
> Test prototype
> 
> Review of attachment 8408900 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This could be fixed I think (for
> typed arrays and for all other prototype getter/setter properties) by using
> the object state change machinery to detect invalidation when a
> getter/setter property on an object is redefined.

Brian, would this buy us anything concrete? One of the benefits I thought we were getting is the extra guarding when we had a single object own property. Can the object change mechanism handle this case?

Comment 5

[Hannes Verschore [:h4writer]]

Attachment: Remove bogus optimizations

So if I understand correctly, the fastpaths are just bogus. So we should just delete them. In the IonMonkey we will fall back to the described method if I understand this correctly.

Feel free to steal this bug to re-implement the fastpaths...

Comment 7

[Brian Hackett [Laid off!]]

Comment on attachment 8414381 [details] [diff] [review]
Remove bogus optimizations

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

What does this patch do for typed array length accessing performance?  The fallback method will I think call the prototype's getter so it will do the right thing but without specialization for that getter we will end up doing some super slow non-loop-hoistable thing.

Comment 8

[Hannes Verschore [:h4writer]]

(In reply to Brian Hackett (:bhackett) from comment #7)
> Comment on attachment 8414381 [details] [diff] [review]
> Remove bogus optimizations
> 
> Review of attachment 8414381 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> What does this patch do for typed array length accessing performance?  The
> fallback method will I think call the prototype's getter so it will do the
> right thing but without specialization for that getter we will end up doing
> some super slow non-loop-hoistable thing.

Sorry about the delay. Yes it will probably. Can I forward that part of the patch to you, Brain? I think we might want this to get fixed sooner than later and I don't think I understand comment 3 where you explain how this could get solved?

Comment 9

[Brian Hackett [Laid off!]]

OK, I'll write a patch within the next week which fixes this and preserves performance.

Comment 10

[Brian Hackett [Laid off!]]

Attachment: v2

Actually, the Ion optimization for this is already pretty close to correct.  The |length| property on typed array prototypes can't be redefined since it is non-configurable, so IonBuilder just needs to make sure the instances don't have their own |length| property or a mutated prototype.  This patch fixes that and also completely removes the typed array length stub in baseline, which doesn't have the same assurances.  Typed array length accesses in baseline do get caught by a native getter stub though, which seems sufficient.

Comment 11

[Jeff Walden [:Waldo]]

(In reply to Brian Hackett (:bhackett) from comment #10)
> The |length| property on typed array prototypes can't be redefined since it
> is non-configurable

This is actually a bug in our implementation.  The ES6 draft I have says that accessors in the spec have { [[Enumerable]]: false, [[Configurable]]: true } as attributes unless otherwise specified (section 17), and I don't see anything in 22.2.3.17 to specify other attributes.  So you should probably just remove JSPROP_PERMANENT in defineGetter/DefineGetter and do the extra work to handle the property being reconfigured, if you're changing this.

Comment 12

[Brian Hackett [Laid off!]]

Comment on attachment 8420583 [details] [diff] [review]
v2

ok

Comment 13

[Brian Hackett [Laid off!]]

Attachment: v3

OK, this patch updates the property attributes to match what v8 seems to be doing, and also fixes the JITs.  r? waldo for the attribute changes, jandem for everything else.

This does require a shape check on the length, but that is loop hoistable and so is the length computation itself.

Comment 14

[Jan de Mooij [:jandem]]

Comment on attachment 8420655 [details] [diff] [review]
v3

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

Looks good, r=me with comment below addressed. Nice to have the inlineNativeGetter infrastructure; should make it easier to inline .byteLength etc.

::: js/src/jit/BaselineIC.h
@@ -410,5 @@
>      _(GetIntrinsic_Constant)    \
>                                  \
>      _(GetProp_Fallback)         \
>      _(GetProp_ArrayLength)      \
> -    _(GetProp_TypedArrayLength) \

If this regresses perf somewhere we should do something else for Baseline.

::: js/src/jit/MCallOptimize.cpp
@@ +218,5 @@
> +    // typed array prototype, and make sure we are accessing the right one
> +    // for the type of the instance object.
> +    if (thisTypes) {
> +        ScalarTypeDescr::Type type = (ScalarTypeDescr::Type) thisTypes->getTypedArrayType();
> +        if (type != ScalarTypeDescr::TYPE_MAX &&

Hm, TYPE_MAX is not a member of ScalarTypeDescr::Type, so this cast/comparison is a bit confusing. We should probably move TYPE_MAX to the enum, and change the return type of getTypedArrayType to ScalarTypeDescr::Type so that we don't need this cast?

Comment 15

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

Brian, is this almost ready for landing? (hoping to get it in before the next train leaves next Monday)

Comment 16

[Brian Hackett [Laid off!]]

(In reply to Gary Kwong [:gkw] [:nth10sd] from comment #15)
> Brian, is this almost ready for landing? (hoping to get it in before the
> next train leaves next Monday)

This is still waiting for review from waldo.

Comment 17

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

Ah, ok. Sorry, I only saw jandem's r+.

Comment 18

[Jeff Walden [:Waldo]]

Comment on attachment 8420655 [details] [diff] [review]
v3

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

Sorry for delay, security work ate my life for a very long time recently.

::: js/src/jit-test/tests/ion/bug998059.js
@@ +1,4 @@
> +// Test various ways of changing the behavior of |typedArray.length|.
> +
> +function addLengthProperty() {
> +  var x = Uint16Array();

Put a new in front of this.  I keep hearing claims we're going to make these throw if not called with new, as part of subclassing, and there was a tree-wide change to fix this once.  New uses should use new, to not keep requiring tree-wide changes to fix.

@@ +8,5 @@
> +}
> +addLengthProperty();
> +
> +function changePrototype() {
> +  var x = Uint16Array();

new

@@ +16,5 @@
> +}
> +changePrototype();
> +
> +function redefineLengthProperty() {
> +  var x = Uint16Array();

new

::: js/src/jit/CodeGenerator.cpp
@@ +3418,5 @@
>              if (!iter->accept(this))
>                  return false;
>  
>  #ifdef DEBUG
> +            if (!counts && !emitDebugResultChecks(*iter))

This is unrelated, right?  Please put this in another patch/rev, if it was deliberate and not necessary for this change.

::: js/src/jit/MCallOptimize.cpp
@@ +218,5 @@
> +    // typed array prototype, and make sure we are accessing the right one
> +    // for the type of the instance object.
> +    if (thisTypes) {
> +        ScalarTypeDescr::Type type = (ScalarTypeDescr::Type) thisTypes->getTypedArrayType();
> +        if (type != ScalarTypeDescr::TYPE_MAX &&

Yeah, agreed about the cast/change here.  Needing a cast at all (where the old IonBuilder code didn't) looked pretty strange in the context of the patch.

Comment 19

[Brian Hackett [Laid off!]]

I needed to leave the cast in because jsinfer.h can't refer to ScalarTypeDescr due to include cycles.  We already do these ugly casts in MIR.cpp anyways.  The whole thing related to typed array enums is a mess as there are two almost-but-not-quite-identical enums hanging around (ViewType and ScalarTypeDescr::Type), I've wanted to fix this and will write a patch to do so.  I also left in the change to CodeGenerator.cpp as I needed this fix to see that codegen on typed array lengths was back where it ought to be.

https://hg.mozilla.org/integration/mozilla-inbound/rev/b1d854aec0e3

Comment 20

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/b1d854aec0e3

Comment 21

[Mayank Bansal]

this appears to have caused a 4.2% (48682 -->  46620) regression on Octane-gameboy on machine11. 

Range : http://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=2a2526aa56f4&tochange=b1d854aec0e3