Bug 206632 – InternalField and CheckNeutered DFG nodes are not always safe to execute

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

[Keith Miller]

InternalField and CheckNeutered DFG nodes are not always safe to execute

[Keith Miller]

Created attachment 388493 [details]
Patch

[Keith Miller]

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.

[Saam Barati]

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

[Mark Lam]

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(...

[Keith Miller]

Created attachment 388499 [details]
Patch

[Keith Miller]

Created attachment 388500 [details]
Patch

[Keith Miller]

(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.

[Saam Barati]

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

[Keith Miller]

Created attachment 388503 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 388503 [details]
Patch for landing

Clearing flags on attachment: 388503

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

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/58820894>

[Mark Lam]

<rdar://problem/58450906>