REGRESSION (r203990): JSC Debug test stress/arity-check-ftl-throw.js failing
authorfpizlo@apple.com <fpizlo@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Tue, 2 Aug 2016 05:38:58 +0000 (05:38 +0000)
committerfpizlo@apple.com <fpizlo@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Tue, 2 Aug 2016 05:38:58 +0000 (05:38 +0000)
https://bugs.webkit.org/show_bug.cgi?id=160438

Reviewed by Mark Lam.

In r203990 I fixed a bug where CommonSlowPaths.h/arityCheckFor() was basically failing at
catching stack overflow due to large parameter count. It would only catch regular old stack
overflow, like if the frame pointer was already past the limit.

This had a secondary problem: unfortunately all of our tests for what happens when you overflow
the stack due to large parameter count were not going down that path at all, so we haven't had
test coverage for this in ages.  There were bugs in all tiers of the engine when handling this
case.

We need to be able to roll back the topCallFrame on paths that are meant to throw an exception
from the caller. Otherwise, we'd crash in StackVisitor because it would see a busted stack
frame. Rolling back like this "just works" except when the caller is the VM entry frame. I had
some choices here. I could have forced anyone who is rolling back to always skip VM entry
frames. They can't do it in a way that changes the value of VM::topVMEntryFrame, which is what
a stack frame roll back normally does, since exception unwinding needs to see the current value
of topVMEntryFrame. So, we have a choice to either try to magically avoid all of the paths that
look at topCallFrame, or give topCallFrame a state that unambiguously signals that we are
sitting right on top of a VM entry frame without having succeeded at making a JS call. The only
place that really needs to know is StackVisitor, which wants to start scanning at topCallFrame.
To signal this, I could have either made topCallFrame point to the real top JS call frame
without also rolling back topVMEntryFrame, or I could make topCallFrame == topVMEntryFrame. The
latter felt somehow cleaner. I filed a bug (https://bugs.webkit.org/show_bug.cgi?id=160441) for
converting topCallFrame to a void*, which would give us a chance to harden the rest of the
engine against this case.

* interpreter/StackVisitor.cpp:
(JSC::StackVisitor::StackVisitor):
We may do ShadowChicken processing, which invokes StackVisitor, when we have topCallFrame
pointing at topVMEntryFrame. This teaches StackVisitor how to handle this case. I believe that
StackVisitor is the only place that needs to be taught about this at this time, because it's
one of the few things that access topCallFrame along this special path.

* jit/JITOperations.cpp: Roll back the top call frame.
* runtime/CommonSlowPaths.cpp:
(JSC::SLOW_PATH_DECL): Roll back the top call frame.

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

Source/JavaScriptCore/ChangeLog
Source/JavaScriptCore/interpreter/StackVisitor.cpp
Source/JavaScriptCore/jit/JITOperations.cpp
Source/JavaScriptCore/runtime/CommonSlowPaths.cpp
Source/JavaScriptCore/runtime/VM.h

index 14b4dba..885c5c0 100644 (file)
@@ -1,3 +1,46 @@
+2016-08-01  Filip Pizlo  <fpizlo@apple.com>
+
+        REGRESSION (r203990): JSC Debug test stress/arity-check-ftl-throw.js failing
+        https://bugs.webkit.org/show_bug.cgi?id=160438
+
+        Reviewed by Mark Lam.
+        
+        In r203990 I fixed a bug where CommonSlowPaths.h/arityCheckFor() was basically failing at
+        catching stack overflow due to large parameter count. It would only catch regular old stack
+        overflow, like if the frame pointer was already past the limit.
+        
+        This had a secondary problem: unfortunately all of our tests for what happens when you overflow
+        the stack due to large parameter count were not going down that path at all, so we haven't had
+        test coverage for this in ages.  There were bugs in all tiers of the engine when handling this
+        case.
+
+        We need to be able to roll back the topCallFrame on paths that are meant to throw an exception
+        from the caller. Otherwise, we'd crash in StackVisitor because it would see a busted stack
+        frame. Rolling back like this "just works" except when the caller is the VM entry frame. I had
+        some choices here. I could have forced anyone who is rolling back to always skip VM entry
+        frames. They can't do it in a way that changes the value of VM::topVMEntryFrame, which is what
+        a stack frame roll back normally does, since exception unwinding needs to see the current value
+        of topVMEntryFrame. So, we have a choice to either try to magically avoid all of the paths that
+        look at topCallFrame, or give topCallFrame a state that unambiguously signals that we are
+        sitting right on top of a VM entry frame without having succeeded at making a JS call. The only
+        place that really needs to know is StackVisitor, which wants to start scanning at topCallFrame.
+        To signal this, I could have either made topCallFrame point to the real top JS call frame
+        without also rolling back topVMEntryFrame, or I could make topCallFrame == topVMEntryFrame. The
+        latter felt somehow cleaner. I filed a bug (https://bugs.webkit.org/show_bug.cgi?id=160441) for
+        converting topCallFrame to a void*, which would give us a chance to harden the rest of the
+        engine against this case.
+        
+        * interpreter/StackVisitor.cpp:
+        (JSC::StackVisitor::StackVisitor):
+        We may do ShadowChicken processing, which invokes StackVisitor, when we have topCallFrame
+        pointing at topVMEntryFrame. This teaches StackVisitor how to handle this case. I believe that
+        StackVisitor is the only place that needs to be taught about this at this time, because it's
+        one of the few things that access topCallFrame along this special path.
+        
+        * jit/JITOperations.cpp: Roll back the top call frame.
+        * runtime/CommonSlowPaths.cpp:
+        (JSC::SLOW_PATH_DECL): Roll back the top call frame.
+
 2016-08-01  Benjamin Poulain  <bpoulain@apple.com>
 
         [JSC][ARM64] Fix branchTest32/64 taking an immediate as mask
index f150445..b0e4361 100644 (file)
@@ -43,6 +43,11 @@ StackVisitor::StackVisitor(CallFrame* startFrame)
     if (startFrame) {
         m_frame.m_VMEntryFrame = startFrame->vm().topVMEntryFrame;
         topFrame = startFrame->vm().topCallFrame;
+        
+        if (topFrame && static_cast<void*>(m_frame.m_VMEntryFrame) == static_cast<void*>(topFrame)) {
+            topFrame = vmEntryRecord(m_frame.m_VMEntryFrame)->m_prevTopCallFrame;
+            m_frame.m_VMEntryFrame = vmEntryRecord(m_frame.m_VMEntryFrame)->m_prevTopVMEntryFrame;
+        }
     } else {
         m_frame.m_VMEntryFrame = 0;
         topFrame = 0;
index 4e785e9..42f6e47 100644 (file)
@@ -2182,7 +2182,7 @@ void JIT_OPERATION lookupExceptionHandler(VM* vm, ExecState* exec)
 
 void JIT_OPERATION lookupExceptionHandlerFromCallerFrame(VM* vm, ExecState* exec)
 {
-    NativeCallFrameTracer tracer(vm, exec);
+    vm->topCallFrame = exec->callerFrame();
     genericUnwind(vm, exec, UnwindFromCallerFrame);
     ASSERT(vm->targetMachinePCForThrow);
 }
index 4593b06..6b7b21c 100644 (file)
@@ -182,7 +182,8 @@ SLOW_PATH_DECL(slow_path_call_arityCheck)
     int slotsToAdd = CommonSlowPaths::arityCheckFor(exec, vm, CodeForCall);
     if (slotsToAdd < 0) {
         exec = exec->callerFrame();
-        ErrorHandlingScope errorScope(exec->vm());
+        vm.topCallFrame = exec;
+        ErrorHandlingScope errorScope(vm);
         CommonSlowPaths::interpreterThrowInCaller(exec, createStackOverflowError(exec));
         RETURN_TWO(bitwise_cast<void*>(static_cast<uintptr_t>(1)), exec);
     }
@@ -195,7 +196,8 @@ SLOW_PATH_DECL(slow_path_construct_arityCheck)
     int slotsToAdd = CommonSlowPaths::arityCheckFor(exec, vm, CodeForConstruct);
     if (slotsToAdd < 0) {
         exec = exec->callerFrame();
-        ErrorHandlingScope errorScope(exec->vm());
+        vm.topCallFrame = exec;
+        ErrorHandlingScope errorScope(vm);
         CommonSlowPaths::interpreterThrowInCaller(exec, createStackOverflowError(exec));
         RETURN_TWO(bitwise_cast<void*>(static_cast<uintptr_t>(1)), exec);
     }
index 069a009..5964e7f 100644 (file)
@@ -270,7 +270,11 @@ public:
     VMType vmType;
     ClientData* clientData;
     VMEntryFrame* topVMEntryFrame;
-    ExecState* topCallFrame;
+    // NOTE: When throwing an exception while rolling back the call frame, this may be equal to
+    // topVMEntryFrame.
+    // FIXME: This should be a void*, because it might not point to a CallFrame.
+    // https://bugs.webkit.org/show_bug.cgi?id=160441
+    ExecState* topCallFrame; 
     Strong<Structure> structureStructure;
     Strong<Structure> structureRareDataStructure;
     Strong<Structure> terminatedExecutionErrorStructure;