Bug 106074 – DFG should not elide CheckStructure if it's needed to perform a cell check

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Filip Pizlo]

CheckStructure checks two things: if the value is a cell, and if so, that it has a particular structure (or one of a set of particular structures).  We shouldn't elide the CheckStructure just because we know that if the value was a cell then it would have a particular structure; we need to also prove that it is a cell.  (Example: we might have proved that a value is either a boolean or a cell with the primordial int32 array structure.)

Of course, it would be even better in those cases to replace CheckStructure with CheckCell.  But for now it's best to just fix the bug.

[Filip Pizlo]

Created attachment 181262 [details]
the patch

[Ryosuke Niwa]

Comment on attachment 181262 [details]
the patch

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

r=me. Despite the fact I’m not an expert of JSC, I think Filip explained me enough details to know what’s happening here.

> Source/JavaScriptCore/ChangeLog:3
> +        

Please add the bug title here.

> Source/JavaScriptCore/ChangeLog:9
> +        * dfg/DFGConstantFoldingPhase.cpp:
> +        (JSC::DFG::ConstantFoldingPhase::foldConstants):

It’ll be great if you could explain what you’ve described me in person (probably in some brief sentence).

> Source/JavaScriptCore/dfg/DFGConstantFoldingPhase.cpp:107
> +                if (!isCellSpeculation(value.m_type))
> +                    break;
>                  if (value.m_currentKnownStructure.isSubsetOf(set)) {

I wonder if you can add isSubsetOf that takes value and ASSERT that isCellSpeculation or whatever condition needed is true.

[Filip Pizlo]

(In reply to comment #2)
> (From update of attachment 181262 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=181262&action=review
> 
> r=me. Despite the fact I’m not an expert of JSC, I think Filip explained me enough details to know what’s happening here.
> 
> > Source/JavaScriptCore/ChangeLog:3
> > +        
> 
> Please add the bug title here.
> 
> > Source/JavaScriptCore/ChangeLog:9
> > +        * dfg/DFGConstantFoldingPhase.cpp:
> > +        (JSC::DFG::ConstantFoldingPhase::foldConstants):
> 
> It’ll be great if you could explain what you’ve described me in person (probably in some brief sentence).
> 
> > Source/JavaScriptCore/dfg/DFGConstantFoldingPhase.cpp:107
> > +                if (!isCellSpeculation(value.m_type))
> > +                    break;
> >                  if (value.m_currentKnownStructure.isSubsetOf(set)) {
> 
> I wonder if you can add isSubsetOf that takes value and ASSERT that isCellSpeculation or whatever condition needed is true.

Or even better, just returns false is isCellSpeculation(m_type) is false.  So we don't have to repeat the idiom of checking both things in the future.

[Filip Pizlo]

(In reply to comment #3)
> (In reply to comment #2)
> > (From update of attachment 181262 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=181262&action=review
> > 
> > r=me. Despite the fact I’m not an expert of JSC, I think Filip explained me enough details to know what’s happening here.
> > 
> > > Source/JavaScriptCore/ChangeLog:3
> > > +        
> > 
> > Please add the bug title here.
> > 
> > > Source/JavaScriptCore/ChangeLog:9
> > > +        * dfg/DFGConstantFoldingPhase.cpp:
> > > +        (JSC::DFG::ConstantFoldingPhase::foldConstants):
> > 
> > It’ll be great if you could explain what you’ve described me in person (probably in some brief sentence).
> > 
> > > Source/JavaScriptCore/dfg/DFGConstantFoldingPhase.cpp:107
> > > +                if (!isCellSpeculation(value.m_type))
> > > +                    break;
> > >                  if (value.m_currentKnownStructure.isSubsetOf(set)) {
> > 
> > I wonder if you can add isSubsetOf that takes value and ASSERT that isCellSpeculation or whatever condition needed is true.
> 
> Or even better, just returns false is isCellSpeculation(m_type) is false.  So we don't have to repeat the idiom of checking both things in the future.

I think I'll do that refactoring separately.  I'm now starting to think about how to more broadly rationalize the AbstractValue API, so I'll hold off until I get a better idea in my head.

[Filip Pizlo]

Landed in http://trac.webkit.org/changeset/138862