Bug 118798 – DFG assumes that NewFunction will never pass its input through

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

[Gabor Rapcsanyi]

32 bit systems are segfaulting with the following test:

function run_tests() {
    function x() {
      return 3;
    }  

    var x = 13;
    return x + x;
}

for(var i=0; i<100; ++i)
    run_tests();


The problem is that SpeculativeJIT::fillJSValue() set the jsvalue tag to cell after a NewFunction node.

[Gabor Rapcsanyi]

Created attachment 206885 [details]
proposed fix

[Gabor Rapcsanyi]

(In reply to comment #0)
> 32 bit systems are segfaulting with the following test:
> 
> function run_tests() {
>     function x() {
>       return 3;
>     }  
> 
>     var x = 13;
>     return x + x;
> }
> 
> for(var i=0; i<100; ++i)
>     run_tests();
> 
> 
> The problem is that SpeculativeJIT::fillJSValue() set the jsvalue tag to cell after a NewFunction node.

On 64 bit systems fillJSValue() doesn't set the jsvalue tag but we should fix that as well.

[Filip Pizlo]

(In reply to comment #2)
> (In reply to comment #0)
> > 32 bit systems are segfaulting with the following test:
> > 
> > function run_tests() {
> >     function x() {
> >       return 3;
> >     }  
> > 
> >     var x = 13;
> >     return x + x;
> > }
> > 
> > for(var i=0; i<100; ++i)
> >     run_tests();
> > 
> > 
> > The problem is that SpeculativeJIT::fillJSValue() set the jsvalue tag to cell after a NewFunction node.
> 
> On 64 bit systems fillJSValue() doesn't set the jsvalue tag but we should fix that as well.

It seems like your fix is all wrong.

- NewFunction should set the data format to cell, and that's not the big. 

- On 64-bit systems there is no such thing as a cell tag.

[Filip Pizlo]

(In reply to comment #3)
> (In reply to comment #2)
> > (In reply to comment #0)
> > > 32 bit systems are segfaulting with the following test:
> > > 
> > > function run_tests() {
> > >     function x() {
> > >       return 3;
> > >     }  
> > > 
> > >     var x = 13;
> > >     return x + x;
> > > }
> > > 
> > > for(var i=0; i<100; ++i)
> > >     run_tests();
> > > 
> > > 
> > > The problem is that SpeculativeJIT::fillJSValue() set the jsvalue tag to cell after a NewFunction node.
> > 
> > On 64 bit systems fillJSValue() doesn't set the jsvalue tag but we should fix that as well.
> 
> It seems like your fix is all wrong.
> 
> - NewFunction should set the data format to cell, and that's not the big. 

*bug. 

> 
> - On 64-bit systems there is no such thing as a cell tag.

[Gabor Rapcsanyi]

So what do you suggest then because the DFG graph has this NewFunction node which will always set the registerFormat to cell in cellResult() ?(In reply to comment #4)
> (In reply to comment #3)
> > (In reply to comment #2)
> > > (In reply to comment #0)
> > > > 32 bit systems are segfaulting with the following test:
> > > > 
> > > > function run_tests() {
> > > >     function x() {
> > > >       return 3;
> > > >     }  
> > > > 
> > > >     var x = 13;
> > > >     return x + x;
> > > > }
> > > > 
> > > > for(var i=0; i<100; ++i)
> > > >     run_tests();
> > > > 
> > > > 
> > > > The problem is that SpeculativeJIT::fillJSValue() set the jsvalue tag to cell after a NewFunction node.
> > > 
> > > On 64 bit systems fillJSValue() doesn't set the jsvalue tag but we should fix that as well.
> > 
> > It seems like your fix is all wrong.
> > 
> > - NewFunction should set the data format to cell, and that's not the big. 
> 
> *bug. 
> 
> > 
> > - On 64-bit systems there is no such thing as a cell tag.

So what do you suggest then because the DFG graph has this NewFunction node which will always set the registerFormat to cell in cellResult() ?

[Filip Pizlo]

(In reply to comment #5)
> So what do you suggest then because the DFG graph has this NewFunction node which will always set the registerFormat to cell in cellResult() ?(In reply to comment #4)
> > (In reply to comment #3)
> > > (In reply to comment #2)
> > > > (In reply to comment #0)
> > > > > 32 bit systems are segfaulting with the following test:
> > > > > 
> > > > > function run_tests() {
> > > > >     function x() {
> > > > >       return 3;
> > > > >     }  
> > > > > 
> > > > >     var x = 13;
> > > > >     return x + x;
> > > > > }
> > > > > 
> > > > > for(var i=0; i<100; ++i)
> > > > >     run_tests();
> > > > > 
> > > > > 
> > > > > The problem is that SpeculativeJIT::fillJSValue() set the jsvalue tag to cell after a NewFunction node.
> > > > 
> > > > On 64 bit systems fillJSValue() doesn't set the jsvalue tag but we should fix that as well.
> > > 
> > > It seems like your fix is all wrong.
> > > 
> > > - NewFunction should set the data format to cell, and that's not the big. 
> > 
> > *bug. 
> > 
> > > 
> > > - On 64-bit systems there is no such thing as a cell tag.
> 
> So what do you suggest then because the DFG graph has this NewFunction node which will always set the registerFormat to cell in cellResult() ?

It's is correct for NewFunction to set the registerFormat to cell in cellResult().  NewFunction returns a cell.  cellResult() is what you call when you want to return a cell.  The type of a function is a cell.

That's not the bug here.

I will investigate what the bug is.

[Filip Pizlo]

(In reply to comment #6)
> (In reply to comment #5)
> > So what do you suggest then because the DFG graph has this NewFunction node which will always set the registerFormat to cell in cellResult() ?(In reply to comment #4)
> > > (In reply to comment #3)
> > > > (In reply to comment #2)
> > > > > (In reply to comment #0)
> > > > > > 32 bit systems are segfaulting with the following test:
> > > > > > 
> > > > > > function run_tests() {
> > > > > >     function x() {
> > > > > >       return 3;
> > > > > >     }  
> > > > > > 
> > > > > >     var x = 13;
> > > > > >     return x + x;
> > > > > > }
> > > > > > 
> > > > > > for(var i=0; i<100; ++i)
> > > > > >     run_tests();
> > > > > > 
> > > > > > 
> > > > > > The problem is that SpeculativeJIT::fillJSValue() set the jsvalue tag to cell after a NewFunction node.
> > > > > 
> > > > > On 64 bit systems fillJSValue() doesn't set the jsvalue tag but we should fix that as well.
> > > > 
> > > > It seems like your fix is all wrong.
> > > > 
> > > > - NewFunction should set the data format to cell, and that's not the big. 
> > > 
> > > *bug. 
> > > 
> > > > 
> > > > - On 64-bit systems there is no such thing as a cell tag.
> > 
> > So what do you suggest then because the DFG graph has this NewFunction node which will always set the registerFormat to cell in cellResult() ?
> 
> It's is correct for NewFunction to set the registerFormat to cell in cellResult().  NewFunction returns a cell.  cellResult() is what you call when you want to return a cell.  The type of a function is a cell.
> 
> That's not the bug here.
> 
> I will investigate what the bug is.

Argh.  You're half right, and I'm full wrong.  The NewFunction node flows its input through like this:

if (input & SpecEmpty)
    result = (input & ~SpecEmpty) | SpecFunction
else
    result = input

The problem can either be fixed by making NewFunction explicitly behave this way in the prediction propagator, abstract state, and backend; or by changing how we parse op_new_function.  Not sure, yet, which is the lesser evil.

[Filip Pizlo]

<rdar://problem/14473897>

[Filip Pizlo]

Created attachment 206920 [details]
the patch

[Filip Pizlo]

Created attachment 206922 [details]
the patch

[Filip Pizlo]

Created attachment 206925 [details]
the patch

[Filip Pizlo]

Created attachment 206926 [details]
patch for landing

Still running some more tests.

[Filip Pizlo]

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

[Filip Pizlo]

Fixed 32-bit after http://trac.webkit.org/changeset/152818.