Bug 212383 – for-of should check the iterable is a JSArray for FastArray in DFG iterator_open

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Keith Miller]

for-of should check the iterable is a JSArray for FastArray in DFG iterator_open

[Keith Miller]

Created attachment 400273 [details]
Patch

[Keith Miller]

Created attachment 400290 [details]
Patch

[Keith Miller]

Created attachment 400304 [details]
Patch

[Keith Miller]

rdar://problem/63269579

[Keith Miller]

Created attachment 400305 [details]
Patch

[Saam Barati]

Comment on attachment 400305 [details]
Patch

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

> Source/JavaScriptCore/bytecode/SpeculatedType.cpp:445
> +    if (classInfo->isSubClassOf(JSArray::info()))
> +        return SpecArray | SpecDerivedArray;

this is a behavior change. Why?

Don't we do this specifically for the Array constructor function?

This also seems like a pessimization, since. we no longer return SpecArray when we're JSArray::info

> Source/JavaScriptCore/runtime/JSCast.h:138
> +    bool canCast = range.first <= from->type() && from->type() <= range.last;

nit: this inequality exists in a few different places. Now that you're using a struct to abstract this range, why not add a method "bool contains(JSType)" on JSTypeRange and invoke that here and elsewhere?

[Keith Miller]

Comment on attachment 400305 [details]
Patch

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

>> Source/JavaScriptCore/bytecode/SpeculatedType.cpp:445
>> +        return SpecArray | SpecDerivedArray;
> 
> this is a behavior change. Why?
> 
> Don't we do this specifically for the Array constructor function?
> 
> This also seems like a pessimization, since. we no longer return SpecArray when we're JSArray::info

It's just incorrect. Array.prototype is a subclass of JSArray::info. If you want to check for only JSArray then you'd need a different operation. 

The existing code will happily let Array.prototype through CheckSubClass (now CheckJSCast).

>> Source/JavaScriptCore/runtime/JSCast.h:138
>> +    bool canCast = range.first <= from->type() && from->type() <= range.last;
> 
> nit: this inequality exists in a few different places. Now that you're using a struct to abstract this range, why not add a method "bool contains(JSType)" on JSTypeRange and invoke that here and elsewhere?

Sure.

[Saam Barati]

Comment on attachment 400305 [details]
Patch

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

>>> Source/JavaScriptCore/bytecode/SpeculatedType.cpp:445
>>> +        return SpecArray | SpecDerivedArray;
>> 
>> this is a behavior change. Why?
>> 
>> Don't we do this specifically for the Array constructor function?
>> 
>> This also seems like a pessimization, since. we no longer return SpecArray when we're JSArray::info
> 
> It's just incorrect. Array.prototype is a subclass of JSArray::info. If you want to check for only JSArray then you'd need a different operation. 
> 
> The existing code will happily let Array.prototype through CheckSubClass (now CheckJSCast).

This isn't right. This function's job wasn't to represent the type system. Maybe we should make a version that does represent the type system.

But this is saying what we should speculate, now what might be allowed to flow through.

[Saam Barati]

Comment on attachment 400305 [details]
Patch

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

> Source/JavaScriptCore/bytecode/SpeculatedType.cpp:438
> +    if (classInfo->isSubClassOf(JSString::info()))
>          return SpecString;

I think you can have == here still,
and in the else, add an assert that isSubclassOf is false.

> Source/JavaScriptCore/bytecode/SpeculatedType.cpp:463
> +    if (classInfo->isSubClassOf(StringObject::info()))

my guess is now we're introducing OSR exit loops here for StringPrototype and its use with shouldSpeculateStringObject

[Keith Miller]

Created attachment 400381 [details]
Patch

[Saam Barati]

Comment on attachment 400381 [details]
Patch

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

Mostly LGTM, but I found a bug

> Source/JavaScriptCore/bytecode/SpeculatedType.cpp:488
> +

nit: remove extra new line

> Source/JavaScriptCore/bytecode/SpeculatedType.cpp:530
> +    if (structure->typeInfo().type() == ArrayType)
> +        return SpecArray;
> +    if (structure->typeInfo().type() == StringObjectType)
> +        return SpecStringObject;

can you turn all these ifs into a helper lambda, and capture the result JSType, and then add a debug assert that speculationFromClassInfoInheritance is a supertype of whatever we return here?

> Source/JavaScriptCore/bytecode/SpeculatedType.h:527
> +// ASSERT(!c->inherits(classInfo) || speculationChecked(speculationFromCell(c), speculationFromClassInfoInheritance(classInfo)));

can you add a form of this assert as I propose above

> Source/JavaScriptCore/dfg/DFGAbstractInterpreterInlines.h:3520
> +                    ASSERT(classInfo->inheritsJSTypeRange->first <= constant.asCell()->type() && constant.asCell()->type() <= classInfo->inheritsJSTypeRange->last);

I still think we should add a helper for this as we're now dealing with a struct.

Each call site shouldn't have to worry about if the bounds are inclusive or not, let's abstract it into the helper

> Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:13941
> +            LValue hasType = isCellWithType(cell, classInfo->inheritsJSTypeRange->first, classInfo->inheritsJSTypeRange->last);

You're misunderstanding what this function is doing.

[Keith Miller]

Created attachment 400419 [details]
Patch

[Keith Miller]

Created attachment 400423 [details]
Patch

[Saam Barati]

Comment on attachment 400423 [details]
Patch

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

r=me

> Source/JavaScriptCore/bytecode/SpeculatedType.cpp:488
> +

extra space

> Source/JavaScriptCore/bytecode/SpeculatedType.cpp:491
> +    if (classInfo->isSubClassOf(JSString::info()))
> +        return SpecString;

this can still be an == with an assert that if == is false then isSubClassOf is also false

> Source/JavaScriptCore/bytecode/SpeculatedType.cpp:536
> +    SpeculatedType classInfoTypes = speculationFromClassInfoInheritance(structure->classInfo());
> +    ASSERT(!filteredResult || isSubtypeSpeculation(filteredResult, classInfoTypes));

I wouldn't call this unconditionally. Just branch on filtered result, and if non zero, debug assert.

I suspect the compiler isn't smart enough to not do this function invocation when !!filteredResult

> Source/JavaScriptCore/bytecode/SpeculatedType.cpp:538
> +    // Fallback to what ClassInfo tells us if we don't have something more refined.

remove this comment, it's obvious from reading the code

> Source/JavaScriptCore/jit/AssemblyHelpers.h:998
> +        return branchIfType(cellGPR, JSTypeRange { type, type });

why not just make JSTypeRange have a constructor from JSType,  and then you wouldn't need this variant anymore?

[Keith Miller]

Comment on attachment 400423 [details]
Patch

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

>> Source/JavaScriptCore/bytecode/SpeculatedType.cpp:488
>> +
> 
> extra space

Removed.

>> Source/JavaScriptCore/bytecode/SpeculatedType.cpp:491
>> +        return SpecString;
> 
> this can still be an == with an assert that if == is false then isSubClassOf is also false

Done.

>> Source/JavaScriptCore/bytecode/SpeculatedType.cpp:536
>> +    ASSERT(!filteredResult || isSubtypeSpeculation(filteredResult, classInfoTypes));
> 
> I wouldn't call this unconditionally. Just branch on filtered result, and if non zero, debug assert.
> 
> I suspect the compiler isn't smart enough to not do this function invocation when !!filteredResult

Did some refactoring now this function is:

SpeculatedType speculationFromStructure(Structure* structure)
{
    SpeculatedType filteredResult = SpecNone;
    if (structure->typeInfo().type() == StringType)
        filteredResult = SpecString;
    else if (structure->typeInfo().type() == SymbolType)
        filteredResult = SpecSymbol;
    else if (structure->typeInfo().type() == HeapBigIntType)
        filteredResult = SpecHeapBigInt;
    else if (structure->typeInfo().type() == DerivedArrayType)
        filteredResult = SpecDerivedArray;
    else if (structure->typeInfo().type() == ArrayType)
        filteredResult = SpecArray;
    else if (structure->typeInfo().type() == StringObjectType)
        filteredResult = SpecStringObject;
    else
        return speculationFromClassInfoInheritance(structure->classInfo());

    ASSERT(filteredResult);
    ASSERT(isSubtypeSpeculation(filteredResult, speculationFromClassInfoInheritance(structure->classInfo())));
    return filteredResult;
}

>> Source/JavaScriptCore/bytecode/SpeculatedType.cpp:538
>> +    // Fallback to what ClassInfo tells us if we don't have something more refined.
> 
> remove this comment, it's obvious from reading the code

Done.

>> Source/JavaScriptCore/jit/AssemblyHelpers.h:998
>> +        return branchIfType(cellGPR, JSTypeRange { type, type });
> 
> why not just make JSTypeRange have a constructor from JSType,  and then you wouldn't need this variant anymore?

I think you can't aggregate initialize JSTypeRange if you have any user-defined constructors.

[Keith Miller]

Committed r262252: <https://trac.webkit.org/changeset/262252>

[Minh Tran]

*** Bug 212185 has been marked as a duplicate of this bug. ***