Bug 207075 – Audit safe to execute

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Saam Barati]

For things like:
https://bugs.webkit.org/show_bug.cgi?id=206805

For example, GetButterfly should probably be checking that its base is an object

[Radar WebKit Bug Importer]

<rdar://problem/59085094>

[Filip Pizlo]

(In reply to Saam Barati from comment #0)
> For things like:
> https://bugs.webkit.org/show_bug.cgi?id=206805
> 
> For example, GetButterfly should probably be checking that its base is an
> object

Isn’t that covered by safeToExecute’s edge processing?

[Saam Barati]

(In reply to Filip Pizlo from comment #2)
> (In reply to Saam Barati from comment #0)
> > For things like:
> > https://bugs.webkit.org/show_bug.cgi?id=206805
> > 
> > For example, GetButterfly should probably be checking that its base is an
> > object
> 
> Isn’t that covered by safeToExecute’s edge processing?

GetButterfly only speculates Cell on its base.

[Saam Barati]

(In reply to Saam Barati from comment #3)
> (In reply to Filip Pizlo from comment #2)
> > (In reply to Saam Barati from comment #0)
> > > For things like:
> > > https://bugs.webkit.org/show_bug.cgi?id=206805
> > > 
> > > For example, GetButterfly should probably be checking that its base is an
> > > object
> > 
> > Isn’t that covered by safeToExecute’s edge processing?
> 
> GetButterfly only speculates Cell on its base.

Probably it should speculate Object.

[Saam Barati]

Created attachment 400984 [details]
patch

[Saam Barati]

Comment on attachment 400984 [details]
patch

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

> Source/JavaScriptCore/tools/JSDollarVM.cpp:1220
> +static const DOMJIT::Signature DOMJITFunctionObjectSignature(DOMJITFunctionObject::functionWithoutTypeCheck, DOMJITFunctionObject::info(), DOMJIT::Effect::forRead(DOMJIT::HeapRange(DOMJIT::HeapRange::ConstExpr, 0, 1)), SpecInt32Only);

we only had one test using this functionality, and it didn't rely on the old behavior, so I just changed it to this so I didn't have to create a new class.

[Yusuke Suzuki]

Comment on attachment 400984 [details]
patch

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

r=me

> Source/JavaScriptCore/dfg/DFGSafeToExecute.h:487
> +        bool isSafe = true;
> +        const ClassInfo* classInfo = node->requiredDOMJITClassInfo();
> +        structures.forEach([&] (RegisteredStructure structure) {
> +            isSafe &= structure->classInfo()->isSubClassOf(classInfo);
> +        });
> +        return isSafe;
> +    }

What happens if structure set is empty?

[Saam Barati]

(In reply to Yusuke Suzuki from comment #7)
> Comment on attachment 400984 [details]
> patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=400984&action=review
> 
> r=me
> 
> > Source/JavaScriptCore/dfg/DFGSafeToExecute.h:487
> > +        bool isSafe = true;
> > +        const ClassInfo* classInfo = node->requiredDOMJITClassInfo();
> > +        structures.forEach([&] (RegisteredStructure structure) {
> > +            isSafe &= structure->classInfo()->isSubClassOf(classInfo);
> > +        });
> > +        return isSafe;
> > +    }
> 
> What happens if structure set is empty?

We should never reach such a point in the program, so we can return true or false.

[Filip Pizlo]

(In reply to Saam Barati from comment #8)
> (In reply to Yusuke Suzuki from comment #7)
> > Comment on attachment 400984 [details]
> > patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=400984&action=review
> > 
> > r=me
> > 
> > > Source/JavaScriptCore/dfg/DFGSafeToExecute.h:487
> > > +        bool isSafe = true;
> > > +        const ClassInfo* classInfo = node->requiredDOMJITClassInfo();
> > > +        structures.forEach([&] (RegisteredStructure structure) {
> > > +            isSafe &= structure->classInfo()->isSubClassOf(classInfo);
> > > +        });
> > > +        return isSafe;
> > > +    }
> > 
> > What happens if structure set is empty?
> 
> We should never reach such a point in the program, so we can return true or
> false.

I think empty set means you have to return false.

[Filip Pizlo]

(In reply to Saam Barati from comment #4)
> (In reply to Saam Barati from comment #3)
> > (In reply to Filip Pizlo from comment #2)
> > > (In reply to Saam Barati from comment #0)
> > > > For things like:
> > > > https://bugs.webkit.org/show_bug.cgi?id=206805
> > > > 
> > > > For example, GetButterfly should probably be checking that its base is an
> > > > object
> > > 
> > > Isn’t that covered by safeToExecute’s edge processing?
> > 
> > GetButterfly only speculates Cell on its base.
> 
> Probably it should speculate Object.

Oh interesting. If we said it speculates object then we would really want it to be like KnownObject - so safe to execute if known object, but never actually checks it.

[Saam Barati]

(In reply to Filip Pizlo from comment #10)
> (In reply to Saam Barati from comment #4)
> > (In reply to Saam Barati from comment #3)
> > > (In reply to Filip Pizlo from comment #2)
> > > > (In reply to Saam Barati from comment #0)
> > > > > For things like:
> > > > > https://bugs.webkit.org/show_bug.cgi?id=206805
> > > > > 
> > > > > For example, GetButterfly should probably be checking that its base is an
> > > > > object
> > > > 
> > > > Isn’t that covered by safeToExecute’s edge processing?
> > > 
> > > GetButterfly only speculates Cell on its base.
> > 
> > Probably it should speculate Object.
> 
> Oh interesting. If we said it speculates object then we would really want it
> to be like KnownObject - so safe to execute if known object, but never
> actually checks it.

Yeah exactly. I fixed this issue a while ago where we only hoist GetButterfly if we prove the base is an object.

KnownObject probably sounds like the better long-term thing since it's what we really mean anyways

[Saam Barati]

(In reply to Filip Pizlo from comment #9)
> (In reply to Saam Barati from comment #8)
> > (In reply to Yusuke Suzuki from comment #7)
> > > Comment on attachment 400984 [details]
> > > patch
> > > 
> > > View in context:
> > > https://bugs.webkit.org/attachment.cgi?id=400984&action=review
> > > 
> > > r=me
> > > 
> > > > Source/JavaScriptCore/dfg/DFGSafeToExecute.h:487
> > > > +        bool isSafe = true;
> > > > +        const ClassInfo* classInfo = node->requiredDOMJITClassInfo();
> > > > +        structures.forEach([&] (RegisteredStructure structure) {
> > > > +            isSafe &= structure->classInfo()->isSubClassOf(classInfo);
> > > > +        });
> > > > +        return isSafe;
> > > > +    }
> > > 
> > > What happens if structure set is empty?
> > 
> > We should never reach such a point in the program, so we can return true or
> > false.
> 
> I think empty set means you have to return false.

Doesn't the empty set prove we would have exitted?

[EWS]

Committed r262642: <https://trac.webkit.org/changeset/262642>

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