Title: [204010] trunk/Source/_javascript_Core
- Revision
- 204010
- Author
- fpi...@apple.com
- Date
- 2016-08-01 22:38:58 -0700 (Mon, 01 Aug 2016)
Log Message
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.
Modified Paths
Diff
Modified: trunk/Source/_javascript_Core/ChangeLog (204009 => 204010)
--- trunk/Source/_javascript_Core/ChangeLog 2016-08-02 05:02:27 UTC (rev 204009)
+++ trunk/Source/_javascript_Core/ChangeLog 2016-08-02 05:38:58 UTC (rev 204010)
@@ -1,3 +1,46 @@
+2016-08-01 Filip Pizlo <fpi...@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 <bpoul...@apple.com>
[JSC][ARM64] Fix branchTest32/64 taking an immediate as mask
Modified: trunk/Source/_javascript_Core/interpreter/StackVisitor.cpp (204009 => 204010)
--- trunk/Source/_javascript_Core/interpreter/StackVisitor.cpp 2016-08-02 05:02:27 UTC (rev 204009)
+++ trunk/Source/_javascript_Core/interpreter/StackVisitor.cpp 2016-08-02 05:38:58 UTC (rev 204010)
@@ -43,6 +43,11 @@
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;
Modified: trunk/Source/_javascript_Core/jit/JITOperations.cpp (204009 => 204010)
--- trunk/Source/_javascript_Core/jit/JITOperations.cpp 2016-08-02 05:02:27 UTC (rev 204009)
+++ trunk/Source/_javascript_Core/jit/JITOperations.cpp 2016-08-02 05:38:58 UTC (rev 204010)
@@ -2182,7 +2182,7 @@
void JIT_OPERATION lookupExceptionHandlerFromCallerFrame(VM* vm, ExecState* exec)
{
- NativeCallFrameTracer tracer(vm, exec);
+ vm->topCallFrame = exec->callerFrame();
genericUnwind(vm, exec, UnwindFromCallerFrame);
ASSERT(vm->targetMachinePCForThrow);
}
Modified: trunk/Source/_javascript_Core/runtime/CommonSlowPaths.cpp (204009 => 204010)
--- trunk/Source/_javascript_Core/runtime/CommonSlowPaths.cpp 2016-08-02 05:02:27 UTC (rev 204009)
+++ trunk/Source/_javascript_Core/runtime/CommonSlowPaths.cpp 2016-08-02 05:38:58 UTC (rev 204010)
@@ -182,7 +182,8 @@
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 @@
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);
}
Modified: trunk/Source/_javascript_Core/runtime/VM.h (204009 => 204010)
--- trunk/Source/_javascript_Core/runtime/VM.h 2016-08-02 05:02:27 UTC (rev 204009)
+++ trunk/Source/_javascript_Core/runtime/VM.h 2016-08-02 05:38:58 UTC (rev 204010)
@@ -270,7 +270,11 @@
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;
_______________________________________________
webkit-changes mailing list
webkit-changes@lists.webkit.org
https://lists.webkit.org/mailman/listinfo/webkit-changes
