for-of should check the iterable is a JSArray for FastArray in DFG iterator_open
Created attachment 400273 [details] Patch
Created attachment 400290 [details] Patch
Created attachment 400304 [details] Patch
rdar://problem/63269579
Created attachment 400305 [details] Patch
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 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 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 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
Created attachment 400381 [details] Patch
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.
Created attachment 400419 [details] Patch
Created attachment 400423 [details] Patch
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 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.
Committed r262252: <https://trac.webkit.org/changeset/262252>
*** Bug 212185 has been marked as a duplicate of this bug. ***