Bug 161492 – ObjectAllocationSinkingPhase::insertOSRHintsForUpdate() fails to emit updated hints in some cases

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

[Filip Pizlo]

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.

[Filip Pizlo]

It looks like this is handled by m_materializationSiteToRecoveries.  I wonder why the scope isn't in there.

[Filip Pizlo]

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);

[Filip Pizlo]

Wow this is an epic one-line fix!

[Filip Pizlo]

Created attachment 287648 [details]
the patch

[Mark Lam]

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.

[Filip Pizlo]

(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.

[Filip Pizlo]

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

[Radar WebKit Bug Importer]

<rdar://problem/28120011>