Bug 215487 – Have an OOB+SaneChain Array::Speculation

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Saam Barati]

So we can return undefined when we're OOB.

[Saam Barati]

Created attachment 406565 [details]
WIP

[Saam Barati]

This at least helps crypto-md5-SP

[Saam Barati]

Created attachment 406645 [details]
patch

[Saam Barati]

Created attachment 406646 [details]
patch

[Saam Barati]

Created attachment 406650 [details]
patch

[Saam Barati]

Comment on attachment 406650 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=406650&action=review

> Source/JavaScriptCore/dfg/DFGValidate.cpp:386
> +                        // We rely on being an original array structure so we can return undefined for negative indices.
> +                        // We could do better in the future, and have and have an feedback based profiling mechanism
> +                        // where we speculate against the index being >= 0.

Will update this comment to reflect new reason we need to be original

[Saam Barati]

Created attachment 406651 [details]
patch

[Yusuke Suzuki]

Comment on attachment 406651 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=406651&action=review

r=me with one comment.

> Source/JavaScriptCore/dfg/DFGClobberize.h:945
> +                LocationKind kind = mode.isInBoundsSaneChain() ? IndexedPropertyDoubleSaneChainLoc : IndexedPropertyDoubleLoc;

This means that we will use IndexedPropertyDoubleLoc for OutOfBoundsSaneChain, is it correct?

[Saam Barati]

(In reply to Yusuke Suzuki from comment #8)
> Comment on attachment 406651 [details]
> patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=406651&action=review
> 
> r=me with one comment.
> 
> > Source/JavaScriptCore/dfg/DFGClobberize.h:945
> > +                LocationKind kind = mode.isInBoundsSaneChain() ? IndexedPropertyDoubleSaneChainLoc : IndexedPropertyDoubleLoc;
> 
> This means that we will use IndexedPropertyDoubleLoc for
> OutOfBoundsSaneChain, is it correct?

Yeah. Maybe we should introduce a new heap just out of paranoia that’s for OOBSaneChain

[Saam Barati]

(In reply to Saam Barati from comment #9)
> (In reply to Yusuke Suzuki from comment #8)
> > Comment on attachment 406651 [details]
> > patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=406651&action=review
> > 
> > r=me with one comment.
> > 
> > > Source/JavaScriptCore/dfg/DFGClobberize.h:945
> > > +                LocationKind kind = mode.isInBoundsSaneChain() ? IndexedPropertyDoubleSaneChainLoc : IndexedPropertyDoubleLoc;
> > 
> > This means that we will use IndexedPropertyDoubleLoc for
> > OutOfBoundsSaneChain, is it correct?
> 
> Yeah. Maybe we should introduce a new heap just out of paranoia that’s for
> OOBSaneChain

I did some more reading. What's going on here is SaneChain may return NaN, where InBounds-non-sane-chain won't ever return NaN. So that's why we have two heap locations.

The change I'm making that I discussed with Yusuke is:
- OOB+SaneChain+NoUsesAsOther, we'll now return unboxed double value, to behave like in bounds SaneChain. Then, we can use the same heap location
- OOB+SaneChain+UsesAsOther => return JSValue, and new heap location

[Saam Barati]

Created attachment 406730 [details]
patch for landing

[Saam Barati]

Comment on attachment 406730 [details]
patch for landing

View in context: https://bugs.webkit.org/attachment.cgi?id=406730&action=review

> Source/JavaScriptCore/dfg/DFGClobberize.h:932
> +            if (mode.isInBounds() || mode.isOutOfBoundsSaneChain()) {

I need a new heap location for this.

[Saam Barati]

Comment on attachment 406730 [details]
patch for landing

View in context: https://bugs.webkit.org/attachment.cgi?id=406730&action=review

>> Source/JavaScriptCore/dfg/DFGClobberize.h:932
>> +            if (mode.isInBounds() || mode.isOutOfBoundsSaneChain()) {
> 
> I need a new heap location for this.

The issue is, AI might say we only return Int32 for a node. If we  CSE it with something that says "Int32 | Other", we've  broken an AI invariant by growing the type.

[Saam Barati]

Created attachment 406733 [details]
patch for landing

[Saam Barati]

Created attachment 406741 [details]
patch for landing

Turn off OOBSaneChain for 32-bit

[EWS]

Committed r265775: <https://trac.webkit.org/changeset/265775>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 406741 [details].

[Radar WebKit Bug Importer]

<rdar://problem/67274231>