[JSC] DFG::AbstractValue::filterByValue should re-filter configured m_value via m_type
Created attachment 405124 [details] Patch
<rdar://problem/65882837>
Created attachment 405126 [details] Patch
Created attachment 405127 [details] Patch
Fixed the typo.
Created attachment 405128 [details] Patch
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.
Comment on attachment 405128 [details] Patch r=me
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.
Looks like JSTests/stress/get-array-length-node-should-be-blessed-in-fixup.js failure is real. Looking.
iterator protocol is enabled for ArrayPrototype too. I don't think this is required.
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.
Created attachment 405143 [details] Patch for landing
Committed r264857: <https://trac.webkit.org/changeset/264857> All reviewed patches have been landed. Closing bug and clearing flags on attachment 405143 [details].