Bug 157358 – [JSC] In DFG, an OSR Exit on SetLocal can trash its child node

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

[Benjamin Poulain]

[JSC] In DFG, an OSR Exit on SetLocal can trash its child node

[Benjamin Poulain]

Created attachment 278130 [details]
Patch

[Filip Pizlo]

Comment on attachment 278130 [details]
Patch

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

> Source/JavaScriptCore/ChangeLog:22
> +            @1 = SomethingProducingDouble()
> +            @2 = MovHint(@1)
> +            @3 = ValueRep(@1)
> +            @4 = SetLocal(@3, FlushedInt32)
> +
> +        When we lower SetLocal(), we start by speculating that @3 is an Int32.
> +        Now this can fail if @1 was really a double.
> +        When that happens, we go over the VariableEventStream to find where values
> +        are, and @1 died at @3. Since the speculation failure happens before
> +        the SetLocal event, we don't do anything with @3.

The bug is that the PhantomInsertionPhase didn't put a Phantom for @1 immediately after @4.  Do you know why it did that?

> Source/JavaScriptCore/dfg/DFGSpeculativeJIT64.cpp:2109
> +        Node* child = node->child1().node();
> +        noticeOSRBirth(child);
> +        m_stream->appendAndLog(VariableEvent::movHint(MinifiedID(child), node->local()));

I think that this is just wrong.  I see what you're trying to do, but SetLocal isn't supposed to have this kind of power.  The DFG should be getting the value that was MovHinted.

[Benjamin Poulain]

Created attachment 278149 [details]
Patch

[Benjamin Poulain]

I took a bit of a shortcut here: instead of querying if the Local is really killed after the current bytecode index, I assumed the local is always killed from SetLocal. I did that because it is cheaper than iterating until the next origin and getting the live locals there.

[Filip Pizlo]

Comment on attachment 278149 [details]
Patch

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

A bit ugly, but it looks right.  Damn that's a tricky corner case!

> Source/JavaScriptCore/dfg/DFGPhantomInsertionPhase.cpp:142
> +            auto processKilledOperand = [&] (VirtualRegister reg)
> +            {

I think that our style is to put the { on the same line as the lambda variable.  The style bot complains about this because it does not understand lambdas.  But, this is the style that I have followed, because that's what I saw elsewhere in WK when I first started using them.  So, let's go with that style.

[Benjamin Poulain]

Created attachment 278192 [details]
Patch for landing

[WebKit Commit Bot]

The commit-queue encountered the following flaky tests while processing attachment 278192 [details]:

The commit-queue is continuing to process your patch.

[WebKit Commit Bot]

The commit-queue encountered the following flaky tests while processing attachment 278192 [details]:

transitions/default-timing-function.html bug 138901 (author: [e-mail])
The commit-queue is continuing to process your patch.

[WebKit Commit Bot]

Comment on attachment 278192 [details]
Patch for landing

Clearing flags on attachment: 278192

Committed r200498: <http://trac.webkit.org/changeset/200498>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Csaba Osztrogonác]

(In reply to comment #9)
> Comment on attachment 278192 [details]
> Patch for landing
> 
> Clearing flags on attachment: 278192
> 
> Committed r200498: <http://trac.webkit.org/changeset/200498>

It made stress/tagged-templates-template-object.js assert
on Apple Mac x86 32-bit debug bots:
- https://build.webkit.org/builders/Apple%20Yosemite%2032-bit%20JSC%20%28BuildAndTest%29/builds/9046
- https://build.webkit.org/builders/Apple%20El%20Capitan%2032-bit%20JSC%20%28BuildAndTest%29/builds/2305

[Benjamin Poulain]

I'll have a look.

[Benjamin Poulain]

(In reply to comment #12)
> I'll have a look.

The fix is here: https://bugs.webkit.org/show_bug.cgi?id=157436