DFG OSR exit shouldn't assume that the frame count for exit is greater than the frame...
authorfpizlo@apple.com <fpizlo@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Sun, 22 Mar 2015 19:35:26 +0000 (19:35 +0000)
committerfpizlo@apple.com <fpizlo@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Sun, 22 Mar 2015 19:35:26 +0000 (19:35 +0000)
https://bugs.webkit.org/show_bug.cgi?id=142948

Reviewed by Sam Weinig.

It's necessary to ensure that the stack pointer accounts for the extent of our stack usage
since a signal may clobber the area below the stack pointer. When the DFG is executing,
the stack pointer accounts for the DFG's worst-case stack usage. When we OSR exit back to
baseline, we will use a different amount of stack. This is because baseline is a different
compiler. It will make different decisions. So it will use a different amount of stack.

This gets tricky when we are in the process of doing an OSR exit, because we are sort of
incrementally transforming the stack from how it looked in the DFG to how it will look in
baseline. The most conservative approach would be to set the stack pointer to the max of
DFG and baseline.

When this code was written, a reckless assumption was made: that the stack usage in
baseline is always at least as large as the stack usage in DFG. Based on this incorrect
assumption, the code first adjusts the stack pointer to account for the baseline stack
usage. This sort of usually works, because usually baseline does happen to use more stack.
But that's not an invariant. Nobody guarantees this. We will never make any changes that
would make this be guaranteed, because that would be antithetical to how optimizing
compilers work. The DFG should be allowed to use however much stack it decides that it
should use in order to get good performance, and it shouldn't try to guarantee that it
always uses less stack than baseline.

As such, we must always assume that the frame size for DFG execution (i.e.
frameRegisterCount) and the frame size in baseline once we exit (i.e.
requiredRegisterCountForExit) are two independent quantities and they have no
relationship.

Fortunately, though, this code can be made correct by just moving the stack adjustment to
just before we do conversions. This is because we have since changed the OSR exit
algorithm to first lift up all state from the DFG state into a scratch buffer, and then to
drop it out of the scratch buffer and into the stack according to the baseline layout. The
point just before conversions is the point where we have finished reading the DFG frame
and will not read it anymore, and we haven't started writing the baseline frame. So, at
this point it is safe to set the stack pointer to account for the frame size at exit.

This is benign because baseline happens to create larger frames than DFG.

* dfg/DFGOSRExitCompiler32_64.cpp:
(JSC::DFG::OSRExitCompiler::compileExit):
* dfg/DFGOSRExitCompiler64.cpp:
(JSC::DFG::OSRExitCompiler::compileExit):
* dfg/DFGOSRExitCompilerCommon.cpp:
(JSC::DFG::adjustAndJumpToTarget):

git-svn-id: https://svn.webkit.org/repository/webkit/trunk@181835 268f45cc-cd09-0410-ab3c-d52691b4dbfc

Source/JavaScriptCore/ChangeLog
Source/JavaScriptCore/dfg/DFGOSRExitCompiler32_64.cpp
Source/JavaScriptCore/dfg/DFGOSRExitCompiler64.cpp
Source/JavaScriptCore/dfg/DFGOSRExitCompilerCommon.cpp

index 04a2911..bd874ea 100644 (file)
@@ -1,5 +1,55 @@
 2015-03-22  Filip Pizlo  <fpizlo@apple.com>
 
+        DFG OSR exit shouldn't assume that the frame count for exit is greater than the frame count in DFG
+        https://bugs.webkit.org/show_bug.cgi?id=142948
+
+        Reviewed by Sam Weinig.
+        
+        It's necessary to ensure that the stack pointer accounts for the extent of our stack usage
+        since a signal may clobber the area below the stack pointer. When the DFG is executing,
+        the stack pointer accounts for the DFG's worst-case stack usage. When we OSR exit back to
+        baseline, we will use a different amount of stack. This is because baseline is a different
+        compiler. It will make different decisions. So it will use a different amount of stack.
+        
+        This gets tricky when we are in the process of doing an OSR exit, because we are sort of
+        incrementally transforming the stack from how it looked in the DFG to how it will look in
+        baseline. The most conservative approach would be to set the stack pointer to the max of
+        DFG and baseline.
+        
+        When this code was written, a reckless assumption was made: that the stack usage in
+        baseline is always at least as large as the stack usage in DFG. Based on this incorrect
+        assumption, the code first adjusts the stack pointer to account for the baseline stack
+        usage. This sort of usually works, because usually baseline does happen to use more stack.
+        But that's not an invariant. Nobody guarantees this. We will never make any changes that
+        would make this be guaranteed, because that would be antithetical to how optimizing
+        compilers work. The DFG should be allowed to use however much stack it decides that it
+        should use in order to get good performance, and it shouldn't try to guarantee that it
+        always uses less stack than baseline.
+        
+        As such, we must always assume that the frame size for DFG execution (i.e.
+        frameRegisterCount) and the frame size in baseline once we exit (i.e.
+        requiredRegisterCountForExit) are two independent quantities and they have no
+        relationship.
+        
+        Fortunately, though, this code can be made correct by just moving the stack adjustment to
+        just before we do conversions. This is because we have since changed the OSR exit
+        algorithm to first lift up all state from the DFG state into a scratch buffer, and then to
+        drop it out of the scratch buffer and into the stack according to the baseline layout. The
+        point just before conversions is the point where we have finished reading the DFG frame
+        and will not read it anymore, and we haven't started writing the baseline frame. So, at
+        this point it is safe to set the stack pointer to account for the frame size at exit.
+        
+        This is benign because baseline happens to create larger frames than DFG.
+
+        * dfg/DFGOSRExitCompiler32_64.cpp:
+        (JSC::DFG::OSRExitCompiler::compileExit):
+        * dfg/DFGOSRExitCompiler64.cpp:
+        (JSC::DFG::OSRExitCompiler::compileExit):
+        * dfg/DFGOSRExitCompilerCommon.cpp:
+        (JSC::DFG::adjustAndJumpToTarget):
+
+2015-03-22  Filip Pizlo  <fpizlo@apple.com>
+
         Shorten the number of iterations to 10,000 since that's enough to test all tiers.
 
         Rubber stamped by Sam Weinig.
index 346eeb5..0e177e8 100644 (file)
@@ -48,12 +48,6 @@ void OSRExitCompiler::compileExit(const OSRExit& exit, const Operands<ValueRecov
         m_jit.debugCall(debugOperationPrintSpeculationFailure, debugInfo);
     }
     
-    // Need to ensure that the stack pointer accounts for the worst-case stack usage at exit.
-    m_jit.addPtr(
-        CCallHelpers::TrustedImm32(
-            -m_jit.codeBlock()->jitCode()->dfgCommon()->requiredRegisterCountForExit * sizeof(Register)),
-        CCallHelpers::framePointerRegister, CCallHelpers::stackPointerRegister);
-    
     // 2) Perform speculation recovery. This only comes into play when an operation
     //    starts mutating state before verifying the speculation it has already made.
     
@@ -259,6 +253,14 @@ void OSRExitCompiler::compileExit(const OSRExit& exit, const Operands<ValueRecov
         }
     }
     
+    // Need to ensure that the stack pointer accounts for the worst-case stack usage at exit. This
+    // could toast some stack that the DFG used. We need to do it before storing to stack offsets
+    // used by baseline.
+    m_jit.addPtr(
+        CCallHelpers::TrustedImm32(
+            -m_jit.codeBlock()->jitCode()->dfgCommon()->requiredRegisterCountForExit * sizeof(Register)),
+        CCallHelpers::framePointerRegister, CCallHelpers::stackPointerRegister);
+    
     // 7) Do all data format conversions and store the results into the stack.
     
     bool haveArguments = false;
index 3e2c4a6..f8c4fad 100644 (file)
@@ -1,5 +1,5 @@
 /*
- * Copyright (C) 2011, 2013, 2014 Apple Inc. All rights reserved.
+ * Copyright (C) 2011, 2013-2015 Apple Inc. All rights reserved.
  *
  * Redistribution and use in source and binary forms, with or without
  * modification, are permitted provided that the following conditions
@@ -52,12 +52,6 @@ void OSRExitCompiler::compileExit(const OSRExit& exit, const Operands<ValueRecov
         m_jit.debugCall(debugOperationPrintSpeculationFailure, debugInfo);
     }
     
-    // Need to ensure that the stack pointer accounts for the worst-case stack usage at exit.
-    m_jit.addPtr(
-        CCallHelpers::TrustedImm32(
-            -m_jit.codeBlock()->jitCode()->dfgCommon()->requiredRegisterCountForExit * sizeof(Register)),
-        CCallHelpers::framePointerRegister, CCallHelpers::stackPointerRegister);
-    
     // 2) Perform speculation recovery. This only comes into play when an operation
     //    starts mutating state before verifying the speculation it has already made.
     
@@ -251,6 +245,14 @@ void OSRExitCompiler::compileExit(const OSRExit& exit, const Operands<ValueRecov
         }
     }
     
+    // Need to ensure that the stack pointer accounts for the worst-case stack usage at exit. This
+    // could toast some stack that the DFG used. We need to do it before storing to stack offsets
+    // used by baseline.
+    m_jit.addPtr(
+        CCallHelpers::TrustedImm32(
+            -m_jit.codeBlock()->jitCode()->dfgCommon()->requiredRegisterCountForExit * sizeof(Register)),
+        CCallHelpers::framePointerRegister, CCallHelpers::stackPointerRegister);
+    
     // 7) Do all data format conversions and store the results into the stack.
     
     bool haveArguments = false;
index 2d690bf..4caa0ee 100644 (file)
@@ -266,7 +266,6 @@ static void osrWriteBarrier(CCallHelpers& jit, GPRReg owner, GPRReg scratch)
 void adjustAndJumpToTarget(CCallHelpers& jit, const OSRExitBase& exit)
 {
 #if ENABLE(GGC) 
-    // 11) Write barrier the owner executables because we're jumping into a different block.
     jit.move(AssemblyHelpers::TrustedImmPtr(jit.codeBlock()->ownerExecutable()), GPRInfo::nonArgGPR0);
     osrWriteBarrier(jit, GPRInfo::nonArgGPR0, GPRInfo::nonArgGPR1);
     InlineCallFrameSet* inlineCallFrames = jit.codeBlock()->jitCode()->dfgCommon()->inlineCallFrames.get();