Bug 100461 – Forward OSR calculation is wrong in the presence of multiple SetLocals, or a mix of SetLocals and Phantoms

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

[Filip Pizlo]

This was broken by the attempt to have the forward OSR exit calculator collect all SetLocals and create recovery overrides for them.  The fix is to not do that, and instead have bytecode ops that decompose into multiple SetLocals explicitly help the forward OSR exit calculator with SetLocal hints.

<rdar://problem/12551946>

[Filip Pizlo]

Created attachment 170822 [details]
the patch

[Filip Pizlo]

Comment on attachment 170822 [details]
the patch

Clearing cq? because I will probably want to land this with a test.  I would appreciate a review even though I haven't had a chance to try to write the test, yet.  Doing so will be tricky.

[Oliver Hunt]

Comment on attachment 170822 [details]
the patch

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

> Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:3118
> +                // First create OSR hints only.
> +                set(baseDst, base);
> +                set(valueDst, value);
> +                
> +                // If we try to hoist structure checks into here, then we're guaranteed that they will occur
> +                // *after* we have already set up the values for OSR.
> +                
> +                // Then do the real SetLocals.

Don't we need this for resolve_with_this as well?

[Filip Pizlo]

(In reply to comment #3)
> (From update of attachment 170822 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=170822&action=review
> 
> > Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:3118
> > +                // First create OSR hints only.
> > +                set(baseDst, base);
> > +                set(valueDst, value);
> > +                
> > +                // If we try to hoist structure checks into here, then we're guaranteed that they will occur
> > +                // *after* we have already set up the values for OSR.
> > +                
> > +                // Then do the real SetLocals.
> 
> Don't we need this for resolve_with_this as well?

Ooops.

[Filip Pizlo]

Created attachment 171016 [details]
the patch

[Filip Pizlo]

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