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.
Created attachment 206885 [details] proposed fix
(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.
(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.
(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() ?(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() ?
(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.
(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.
<rdar://problem/14473897>
Created attachment 206920 [details] the patch
Created attachment 206922 [details] the patch
Created attachment 206925 [details] the patch
Created attachment 206926 [details] patch for landing Still running some more tests.
Landed in http://trac.webkit.org/changeset/152813
Fixed 32-bit after http://trac.webkit.org/changeset/152818.