Bug 206632

Created attachment 388493 [details]
Patch

Comment on attachment 388493 [details]
Patch

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

> Source/JavaScriptCore/dfg/DFGNode.h:1401
> +        switch (op()) {
> +        case IsCellWithType:
> +        case GetInternalField:
> +        case PutInternalField:
> +            return true;
> +        default:
> +            return false;
> +        }
>      }
>  
>      JSType queriedType()
>      {
>          static_assert(std::is_same<uint8_t, std::underlying_type<JSType>::type>::value, "Ensure that uint8_t is the underlying type for JSType.");
> -        return static_cast<JSType>(m_opInfo.as<uint32_t>());
> +        if (op() == IsCellWithType)
> +            return static_cast<JSType>(m_opInfo.as<uint32_t>());
> +        return static_cast<JSType>(m_opInfo2.as<uint32_t>());

Didn't mean to upload this. Will delete.

Comment on attachment 388493 [details]
Patch

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

> Source/JavaScriptCore/ChangeLog:9
> +        GetInternalField, etc. rely on a a proof that the cell passed to it is a subclass of InteralFieldObject

not necessarily a subclass of InternalFieltObject, but a subclass of a specific kind of InternalFieldObject, e.g, ArrayIterator

> Source/JavaScriptCore/ChangeLog:12
> +        Also, remove a bogus assertion that we will have proven the value passed to CheckNeutered is a TypedArray.

nit: maybe say why it's bogus.

> Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:1797
> +    MacroAssembler::JumpList speculationFailures;
> +    if (!speculationChecked(m_state.forNode(node->child1()).m_type, SpecTypedArrayView)) {
> +        GPRTemporary tmp(this);
> +        GPRReg tmpReg = tmp.gpr();
> +
> +        m_jit.load8(MacroAssembler::Address(baseReg, JSCell::typeInfoTypeOffset()), tmpReg);
> +        m_jit.sub32(TrustedImm32(FirstTypedArrayType), tmpReg);
> +        speculationFailures.append(m_jit.branch32(JITCompiler::AboveOrEqual, tmpReg, TrustedImm32(NumberOfTypedArrayTypesExcludingDataView)));
> +    }

we don't need this given your code inside SafeToExecute and how we emit this node only after CheckArray

Comment on attachment 388493 [details]
Patch

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

> JSTests/stress/for-of-bad-internal-field-hoist.js:1
> +//requireOptions("--maximumFunctionForCallInlineCandidateBytecodeCost=500")

//@ requireOptions(...

Created attachment 388499 [details]
Patch

Created attachment 388500 [details]
Patch

(In reply to Saam Barati from comment #3)
> Comment on attachment 388493 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=388493&action=review
> 
> > Source/JavaScriptCore/ChangeLog:9
> > +        GetInternalField, etc. rely on a a proof that the cell passed to it is a subclass of InteralFieldObject
> 
> not necessarily a subclass of InternalFieltObject, but a subclass of a
> specific kind of InternalFieldObject, e.g, ArrayIterator

Uhh, I guess I meant as a C++ subclass not a JS subclass. The Get/SetInternalField nodes don't know which one they are loading from at either the bytecode or DFG level.

Comment on attachment 388500 [details]
Patch

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

> Source/JavaScriptCore/ChangeLog:10
> +        GetInternalField, etc. rely on a a proof that the cell passed to it is a subclass of InteralFieldObject
> +        but we may hoist it past the check guarding it.

nit: sentence is a bit of a run on and can be made clearer

> Source/JavaScriptCore/ChangeLog:13
> +        It's not valid to require that AI will preserve any invariant since phases can make changes that AI doesn't

"preserve" isn't the right word here. AI won't break the program. But it might not be able to precisely model it.

> Source/JavaScriptCore/dfg/DFGSafeToExecute.h:38
> +// not prove it is valid. Thus, it is always 

you forgot to finish your comment here

Created attachment 388503 [details]
Patch for landing

Comment on attachment 388503 [details]
Patch for landing

Clearing flags on attachment: 388503

Committed r254959: <https://trac.webkit.org/changeset/254959>

All reviewed patches have been landed.  Closing bug.

<rdar://problem/58820894>

<rdar://problem/58450906>