Bug 214721 – [JSC] DFG::AbstractValue::filterByValue should re-filter configured m_value via m_type

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

[Yusuke Suzuki]

[JSC] DFG::AbstractValue::filterByValue should re-filter configured m_value via m_type

[Yusuke Suzuki]

Created attachment 405124 [details]
Patch

[Yusuke Suzuki]

<rdar://problem/65882837>

[Yusuke Suzuki]

Created attachment 405126 [details]
Patch

[Yusuke Suzuki]

Created attachment 405127 [details]
Patch

[Yusuke Suzuki]

Fixed the typo.

[Yusuke Suzuki]

Created attachment 405128 [details]
Patch

[Mark Lam]

Comment on attachment 405126 [details]
Patch

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

> Source/JavaScriptCore/ChangeLog:12
> +            2. We have CheckIsConstant with StringPrototype (which is StringObjectOther | SpecStringObject in the current SpeculatedType).

Please fix typo: StringObjectOther => SpecObjectOther.

> Source/JavaScriptCore/bytecode/SpeculatedType.cpp:547
> +    // We do not want to accept String.prototype in StringObjectUse, so that we do not include it as SpecStringObject.

I suggest /so that we/so we/ because I think you mean to say "because of A, we do B" (A leads to B), not "we want A because we do B" (B leads to A).

> Source/JavaScriptCore/dfg/DFGAbstractValue.cpp:327
> +        // It is possible that SpeculatedType from value is broader than original m_type. The filter operation keeps m_type as is,
> +        // and as a result, SpeculatedType from m_value becomes broader than m_type. This breaks invariant.

I suggest the this change: "The filter operation keeps m_type as is, and as a result, SpeculatedType from m_value becomes broader than m_type. This breaks invariant" => "The filter operation can only keep m_type as is or make it narrower. As a result, the SpeculatedType from m_value can become broader than m_type. This breaks an invariant."

Can you also apply this comment below.

[Mark Lam]

Comment on attachment 405128 [details]
Patch

r=me

[Yusuke Suzuki]

Comment on attachment 405126 [details]
Patch

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

Thanks!!

>> Source/JavaScriptCore/ChangeLog:12
>> +            2. We have CheckIsConstant with StringPrototype (which is StringObjectOther | SpecStringObject in the current SpeculatedType).
> 
> Please fix typo: StringObjectOther => SpecObjectOther.

Fixed.

>> Source/JavaScriptCore/bytecode/SpeculatedType.cpp:547
>> +    // We do not want to accept String.prototype in StringObjectUse, so that we do not include it as SpecStringObject.
> 
> I suggest /so that we/so we/ because I think you mean to say "because of A, we do B" (A leads to B), not "we want A because we do B" (B leads to A).

Fixed.

>> Source/JavaScriptCore/dfg/DFGAbstractValue.cpp:327
>> +        // and as a result, SpeculatedType from m_value becomes broader than m_type. This breaks invariant.
> 
> I suggest the this change: "The filter operation keeps m_type as is, and as a result, SpeculatedType from m_value becomes broader than m_type. This breaks invariant" => "The filter operation can only keep m_type as is or make it narrower. As a result, the SpeculatedType from m_value can become broader than m_type. This breaks an invariant."
> 
> Can you also apply this comment below.

Fixed.

[Yusuke Suzuki]

Looks like JSTests/stress/get-array-length-node-should-be-blessed-in-fixup.js failure is real. Looking.

[Yusuke Suzuki]

iterator protocol is enabled for ArrayPrototype too. I don't think this is required.

[Yusuke Suzuki]

Comment on attachment 405128 [details]
Patch

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

> Source/JavaScriptCore/bytecode/SpeculatedType.cpp:507
>  
>      if (classInfo->isSubClassOf(JSFunction::info())) {
>          if (classInfo == JSBoundFunction::info())

I'll remove these changes in speculationFromClassInfoInheritance to make it conservative. This is required by AbstractValue::filterByClassInfo

> Source/JavaScriptCore/bytecode/SpeculatedType.cpp:550
> +    default:

On the other hand, this is OK.

[Yusuke Suzuki]

Created attachment 405143 [details]
Patch for landing

[EWS]

Committed r264857: <https://trac.webkit.org/changeset/264857>

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