1027846 - Differential Testing: Different output message involving length

Status: RESOLVED INVALID

(Core :: JavaScript Engine: JIT, defect, P3)

Description

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

x = new Uint32Array;
x.__proto__ = this
print(evalcx("for(w in [0,0,0]){x.length}", this))

$ ./js-opt-64-dm-ts-darwin-ea703db56bcf --fuzzing-safe --ion-offthread-compile=off 21815.js
undefined

$ ./js-opt-64-dm-ts-darwin-ea703db56bcf --fuzzing-safe --ion-offthread-compile=off --ion-eager 21815.js
0

(Tested this on 64-bit Mac js opt threadsafe deterministic shell off m-c rev ea703db56bcf)

My configure flags are:

CC="clang -Qunused-arguments" CXX="clang++ -Qunused-arguments" AR=ar sh /Users/skywalker/trees/mozilla-central/js/src/configure --target=x86_64-apple-darwin12.5.0 --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:   https://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.

Brian, is bug 980013 a likely regressor?

Comment 1

[Jeff Walden [:Waldo]]

Attachment: Remove all the bogus typed array length-computing code in IonCaches

So I guess we missed a spot of caching when fixing bug 998059.  Blah, we have too much code, too scattered, too disorganized, too poorly-documented to know we've touched everything.

Comment 2

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

Thanks Waldo! Clearing needinfo? for Brian...

Comment 3

[Shu-yu Guo [:shu]]

Comment on attachment 8443120 [details] [diff] [review]
Remove all the bogus typed array length-computing code in IonCaches

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

I'm not really happy with this patch. While in the non-PIC case (e.g., when we can determine at JIT time the base object is always a TypedArray), IonBuilder::inlineNativeGetter inlines the TypedArray length getter, the ICs don't have such a path and should get one. This patch will probably regress TA performance.

For sequential GetProp ICs, it looks like we will at least generate a call getter in IsCacheableGetPropCallNative. I'd like to see an IC analogue of inlineNativeGetter that checks if the getter is the original TA getter.

For parallel GetProp ICs, once we have the IC analogue of inlineNativeGetter we should use that here as well. N.B. that there is another cutout for TypedArray .length in js::GetPropertyPure that is also wrong. GetPropertyPure currently can't handle any getter shapes, only hasSlot() shapes. So to fix this in the short term, I imagine the TypedArrayObject cutout should also check if the getter is the original length getter. To fix this in a more principled fashion, we would either need a way to have parallel versions of getters, or a way of marking getters are threadsafe.

Comment 4

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

Waldo, just wondering whether this may have fallen off the radar?

Comment 5

[Jeff Walden [:Waldo]]

It hasn't, I have the current patch here sitting in my queue waiting for me to figure out the fixes for it, and I see it every time I rebase.  But this nonsense about preserving the optimization in Ion is making it more work than I wish it were, or so, to just quickly finish off, versus other things that need doing.  I'll try to get to it next week after I get back from Boston.

Comment 6

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

$ ./js-dbg-64-dm-clang-darwin-d5d53a3b4e50 --fuzzing-safe --ion-offthread-compile=off 1027846.js
undefined

$ ./js-dbg-64-dm-clang-darwin-d5d53a3b4e50 --fuzzing-safe --ion-offthread-compile=off --ion-eager 1027846.js
undefined

This no longer seems to occur as of m-c rev d5d53a3b4e50 (end-March 2016), nor rev dc4b163f7db7 (early Nov 2014). Getting this off the compareJIT ignore list.

Comment 7

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

So I had tested with debug builds in comment 6, whereas the original issue was with opt builds.

For opt builds, it no longer seems to occur in m-c rev d5d53a3b4e50, but it was still occurring in rev dc4b163f7db7 (early Nov 2014). Bisecting to see the possible fix.

Comment 8

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

autoBisect shows this is probably related to the following changeset:

The first good revision is:
changeset:   https://hg.mozilla.org/mozilla-central/rev/8ed771ecfecd
user:        Jan de Mooij
date:        Thu Apr 23 15:51:28 2015 +0200
summary:     Bug 1155946 part 1 - Add a mayResolve class hook to optimize objects with resolve hooks better. r=bhackett

Jan, is bug 1155946 a likely fix?

Comment 9

[Jan de Mooij [:jandem]]

(In reply to Gary Kwong [:gkw] [:nth10sd] from comment #8)
> summary:     Bug 1155946 part 1 - Add a mayResolve class hook to optimize
> objects with resolve hooks better. r=bhackett
> 
> Jan, is bug 1155946 a likely fix?

That was just an optimization. This likely still repros if we make ClassMayResolveId return false.

Comment 10

[Jan de Mooij [:jandem]]

(In reply to Jan de Mooij [:jandem] from comment #9)
> This likely still repros if we make ClassMayResolveId return false.

Er, |true|, of course.

Comment 12

[André Bargull [:anba]]

Old bug related to Ion caches. No longer valid with CacheIR.