Bug 161492

Summary: ObjectAllocationSinkingPhase::insertOSRHintsForUpdate() fails to emit updated hints in some cases
Product: WebKit Reporter: Filip Pizlo <fpizlo>
Component: JavaScriptCoreAssignee: Filip Pizlo <fpizlo>
Status: RESOLVED FIXED    
Severity: Normal CC: commit-queue, keith_miller, mark.lam, msaboff, saam, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: All   
OS: All   
Bug Depends on: 143073    
Bug Blocks: 160125    
Attachments:
Description Flags
the patch mark.lam: review+

Description Filip Pizlo 2016-09-01 10:39:37 PDT
This seems to cause trouble:

a: CreateActivation()
MovHint(@a, loc1)
b: NewFunction(@a)
MovHint(@b, loc2)
c: NewFunction(@b)
MovHint(@c, loc3)
Escape(@b)

I'm seeing code like this:

a: PhantomCreateActivation()
MovHint(@a, loc1)
b: PhantomNewFunction(@a)
PutHint(@b, @a, scope)
MovHint(@b, loc2)
c: PhantomNewFunction(@b)
PutHint(@c, @a, scope)
MovHint(@c, loc3)
d: MaterializeCreateActivation()
e: NewFunction(@d)
MovHint(@d, loc1)
MovHint(@e, loc2)
Escape(@e)

But this is wrong, because if we exit, then we will recreate @c pointing at a newly materialized activation, rather than at @d.  In other words, the phase should have produced this code instead:

a: PhantomCreateActivation()
MovHint(@a, loc1)
b: PhantomNewFunction(@a)
PutHint(@b, @a, scope)
MovHint(@b, loc2)
c: PhantomNewFunction(@b)
PutHint(@c, @a, scope)
MovHint(@c, loc3)
d: MaterializeCreateActivation()
e: NewFunction(@d)
PutHint(@c, @e, scope)   <---- THIS
MovHint(@d, loc1)
MovHint(@e, loc2)
Escape(@e)

The phase seems to have code to do this.  I'm still investigating what goes wrong.
Comment 1 Filip Pizlo 2016-09-01 11:12:33 PDT
It looks like this is handled by m_materializationSiteToRecoveries.  I wonder why the scope isn't in there.
Comment 2 Filip Pizlo 2016-09-01 11:18:10 PDT
100% repro:


function bar()
{
}

noInline(bar);

function foo(p, x)
{
    var value = 1;
    function inc()
    {
        return value + 1;
    }
    function dec()
    {
        return value - 1;
    }
    
    if (!p)
        return 0;
    
    bar(inc);
    
    x += 2000000000;
    
    value = 42;
    return dec();
}

noInline(foo);

function test(x)
{
    var result = foo(true, x);
    if (result != 42 - 1)
        throw "Error: bad result: " + result;
}

for (var i = 0; i < 100000; ++i)
    test(0);

test(2000000000);
Comment 3 Filip Pizlo 2016-09-01 11:42:51 PDT
Wow this is an epic one-line fix!
Comment 4 Filip Pizlo 2016-09-01 11:48:31 PDT
Created attachment 287648 [details]
the patch
Comment 5 Mark Lam 2016-09-01 11:55:13 PDT
Comment on attachment 287648 [details]
the patch

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

r=me

> Source/JavaScriptCore/ChangeLog:16
> +        it's a special meta-data field initialed on construction. But just because it's immutable

/initialed/initialized/ ?

> Source/JavaScriptCore/dfg/DFGObjectAllocationSinkingPhase.cpp:1942
> -
> +        

Please remove.
Comment 6 Filip Pizlo 2016-09-01 11:56:17 PDT
(In reply to comment #5)
> Comment on attachment 287648 [details]
> the patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=287648&action=review
> 
> r=me
> 
> > Source/JavaScriptCore/ChangeLog:16
> > +        it's a special meta-data field initialed on construction. But just because it's immutable
> 
> /initialed/initialized/ ?

Fixed.

> 
> > Source/JavaScriptCore/dfg/DFGObjectAllocationSinkingPhase.cpp:1942
> > -
> > +        
> 
> Please remove.

Fixed.
Comment 7 Filip Pizlo 2016-09-01 11:58:01 PDT
Landed in http://trac.webkit.org/changeset/205304
Comment 8 Radar WebKit Bug Importer 2016-09-01 11:58:54 PDT
<rdar://problem/28120011>