Bug 212383 - for-of should check the iterable is a JSArray for FastArray in DFG iterator_open
Summary: for-of should check the iterable is a JSArray for FastArray in DFG iterator_open
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Keith Miller
URL:
Keywords: InRadar
: 212185 (view as bug list)
Depends on:
Blocks:
 
Reported: 2020-05-26 13:48 PDT by Keith Miller
Modified: 2020-06-03 00:53 PDT (History)
11 users (show)

See Also:


Attachments
Patch (31.43 KB, patch)
2020-05-26 14:00 PDT, Keith Miller
no flags Details | Formatted Diff | Diff
Patch (37.22 KB, patch)
2020-05-26 18:00 PDT, Keith Miller
no flags Details | Formatted Diff | Diff
Patch (43.56 KB, patch)
2020-05-26 20:15 PDT, Keith Miller
no flags Details | Formatted Diff | Diff
Patch (40.31 KB, patch)
2020-05-26 20:21 PDT, Keith Miller
no flags Details | Formatted Diff | Diff
Patch (46.30 KB, patch)
2020-05-27 14:21 PDT, Keith Miller
no flags Details | Formatted Diff | Diff
Patch (61.60 KB, patch)
2020-05-27 19:00 PDT, Keith Miller
no flags Details | Formatted Diff | Diff
Patch (53.19 KB, patch)
2020-05-27 19:58 PDT, Keith Miller
sbarati: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Keith Miller 2020-05-26 13:48:00 PDT
for-of should check the iterable is a JSArray for FastArray in DFG iterator_open
Comment 1 Keith Miller 2020-05-26 14:00:53 PDT
Created attachment 400273 [details]
Patch
Comment 2 Keith Miller 2020-05-26 18:00:08 PDT
Created attachment 400290 [details]
Patch
Comment 3 Keith Miller 2020-05-26 20:15:43 PDT
Created attachment 400304 [details]
Patch
Comment 4 Keith Miller 2020-05-26 20:19:03 PDT
rdar://problem/63269579
Comment 5 Keith Miller 2020-05-26 20:21:10 PDT
Created attachment 400305 [details]
Patch
Comment 6 Saam Barati 2020-05-27 11:38:41 PDT
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?
Comment 7 Keith Miller 2020-05-27 11:41:38 PDT
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.
Comment 8 Saam Barati 2020-05-27 12:12:17 PDT
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.
Comment 9 Saam Barati 2020-05-27 12:16:10 PDT
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
Comment 10 Keith Miller 2020-05-27 14:21:48 PDT
Created attachment 400381 [details]
Patch
Comment 11 Saam Barati 2020-05-27 16:40:11 PDT
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.
Comment 12 Keith Miller 2020-05-27 19:00:16 PDT
Created attachment 400419 [details]
Patch
Comment 13 Keith Miller 2020-05-27 19:58:44 PDT
Created attachment 400423 [details]
Patch
Comment 14 Saam Barati 2020-05-28 11:08:09 PDT
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?
Comment 15 Keith Miller 2020-05-28 11:33:51 PDT
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.
Comment 16 Keith Miller 2020-05-28 11:38:54 PDT
Committed r262252: <https://trac.webkit.org/changeset/262252>
Comment 17 Minh Tran 2020-06-03 00:53:35 PDT
*** Bug 212185 has been marked as a duplicate of this bug. ***