Attachment 348728 Details for Bug 189227 – proposed patch.

[patch] proposed patch.

bug-189227.patch (text/plain), 7.87 KB, created by Mark Lam on 2018-09-01 23:38:19 PDT

(hide)

Description:

Filename:

MIME Type:

Creator: Mark Lam

Created: 2018-09-01 23:38:19 PDT

Size: 7.87 KB

patch

obsolete

>Index: JSTests/ChangeLog
>===================================================================
>--- JSTests/ChangeLog	(revision 235588)
>+++ JSTests/ChangeLog	(working copy)
>@@ -1,3 +1,13 @@
>+2018-09-01  Mark Lam  <mark.lam@apple.com>
>+
>+        The watchdog sometimes fails to terminate a script.
>+        https://bugs.webkit.org/show_bug.cgi?id=189227
>+        <rdar://problem/39932857>
>+
>+        Reviewed by NOBODY (OOPS!).
>+
>+        * stress/regress-189227-watchdog-on-infinite-loop.js: Added.
>+
> 2018-08-24  Yusuke Suzuki  <yusukesuzuki@slowstart.org>
> 
>         Function object should convert params to string before throw a parsing error
>Index: JSTests/stress/regress-189227-watchdog-on-infinite-loop.js
>===================================================================
>--- JSTests/stress/regress-189227-watchdog-on-infinite-loop.js	(nonexistent)
>+++ JSTests/stress/regress-189227-watchdog-on-infinite-loop.js	(working copy)
>@@ -0,0 +1,7 @@
>+//@ requireOptions("--watchdog=20", "--jitPolicyScale=0", "--watchdog-exception-ok")
>+var x = 2000000;
>+while(x) {
>+    x--;
>+}
>+
>+throw "FAIL";
>Index: Source/JavaScriptCore/ChangeLog
>===================================================================
>--- Source/JavaScriptCore/ChangeLog	(revision 235588)
>+++ Source/JavaScriptCore/ChangeLog	(working copy)
>@@ -1,3 +1,73 @@
>+2018-09-01  Mark Lam  <mark.lam@apple.com>
>+
>+        The watchdog sometimes fails to terminate a script.
>+        https://bugs.webkit.org/show_bug.cgi?id=189227
>+        <rdar://problem/39932857>
>+
>+        Reviewed by NOBODY (OOPS!).
>+
>+        Consider the following scenario:
>+
>+        1. We have an infinite loop bytecode sequence as follows:
>+
>+            [  13] loop_hint
>+            [  14] check_traps
>+            [  15] jmp               -2(->13)
>+
>+        2. The VM tiers up from LLInt -> BaselineJIT -> DFG -> FTL.
>+
>+           Note that op_check_traps is represented as a CheckTraps node in the DFG and FTL.
>+           When we're not using pollingTraps (JSC_usePollingTraps is false by default),
>+           we emit no code for CheckTraps, but only record an InvalidationPoint there.
>+
>+        3. The watchdog fires, and invalidates all InvalidationPoints in the FTL CodeBlock.
>+
>+           InvalidationPoints OSR exits to the next instruction by design.  In this case,
>+           that means the VM will resumes executing at the op_jmp, which jumps to the
>+           op_loop_hint opcode.  At the loop_hint, the VM discovers that the function is
>+           already hot, and attempts to tier up.  It immediately discovers that a replacement
>+           CodeBlock is available because we still haven't jettisoned the DFG CodeBlock
>+           nor the FTL CodeBlock that was previously compiled for this function.
>+
>+           Note that jettisoning a CodeBlock necessarily means the VM will invalidate
>+           its InvalidationPoints (if the CodeBlock is DFG/FTL).  However, the reverse
>+           is not true: merely invalidating the InvalidationPoints does not necessarily
>+           mean that the CodeBlock is jettisoned.
>+
>+           VMTraps::tryInstallTrapBreakpoints() runs from a separate thread.  Hence,
>+           it is only safe for it to invalidate a CodeBlock's InvalidationPoints.  It
>+           is not safe for the CodeBlock to be jettisoned from another thread.  Instead,
>+           the VMTraps mechanism relies on the script thread running to an op_check_traps
>+           in the baseline JIT code where it will do the necessary jettisoning of optimized
>+           CodeBlocks.
>+
>+        Since the op_check_traps never get executed, the VM will perpetually tier up in
>+        the op_loop_hint, OSR exit to the op_jmp, jump to the op_loop_hint, and repeat.
>+        Consequently, the watchdog fails to terminate this script.
>+
>+        In this patch, we fix this by adding vm.needTrapHandling() checks in the baseline
>+        JIT's operationOptimize() and the DFG's triggerReoptimizationNow().  If
>+        vm.needTrapHandling() is true, they we choose not to tier up.
>+
>+        The check is not strictly needed in triggerReoptimizationNow().  However, if the
>+        VM needs trap handling, any optimized code will probably have to be invalidated
>+        anyway in order to handle the trap.  So, there's no point in allowing the tier
>+        up until we're done handling the trap.  Note that we don't need to back off the
>+        optimization attempt for a while.  This is because we expect the trap to be
>+        checked imminently.  Thereafter, we should be allowed to optimize and tier up
>+        immediately if execution is permitted to continue.
>+
>+        In this patch, we also change VMTraps::tryInstallTrapBreakpoints() to use
>+        CallFrame::unsafeCodeBlock() instead of CallFrame::codeBlock().  This is because
>+        we don't really know if the frame is properly set up.  We're just conservatively
>+        probing the stack.  ASAN does not like this probing.  Using unsafeCodeBlock() here
>+        will suppress the false positive ASAN complaint.
>+
>+        * dfg/DFGOperations.cpp:
>+        * jit/JITOperations.cpp:
>+        * runtime/VMTraps.cpp:
>+        (JSC::VMTraps::tryInstallTrapBreakpoints):
>+
> 2018-08-24  Yusuke Suzuki  <yusukesuzuki@slowstart.org>
> 
>         Function object should convert params to string before throw a parsing error
>Index: Source/JavaScriptCore/dfg/DFGOperations.cpp
>===================================================================
>--- Source/JavaScriptCore/dfg/DFGOperations.cpp	(revision 235560)
>+++ Source/JavaScriptCore/dfg/DFGOperations.cpp	(working copy)
>@@ -2849,9 +2849,13 @@ extern "C" void JIT_OPERATION triggerReo
> {
>     // It's sort of preferable that we don't GC while in here. Anyways, doing so wouldn't
>     // really be profitable.
>-    DeferGCForAWhile deferGC(codeBlock->vm()->heap);
>-    
>-    sanitizeStackForVM(codeBlock->vm());
>+    VM& vm = *codeBlock->vm();
>+    if (UNLIKELY(vm.needTrapHandling()))
>+        return; // Choose not to optimize.
>+
>+    DeferGCForAWhile deferGC(vm.heap);
>+
>+    sanitizeStackForVM(&vm);
> 
>     if (Options::verboseOSR())
>         dataLog(*codeBlock, ": Entered reoptimize\n");
>Index: Source/JavaScriptCore/jit/JITOperations.cpp
>===================================================================
>--- Source/JavaScriptCore/jit/JITOperations.cpp	(revision 235560)
>+++ Source/JavaScriptCore/jit/JITOperations.cpp	(working copy)
>@@ -1380,6 +1380,9 @@ SlowPathReturnType JIT_OPERATION operati
>     VM& vm = exec->vm();
>     NativeCallFrameTracer tracer(&vm, exec);
> 
>+    if (UNLIKELY(vm.needTrapHandling()))
>+        return encodeResult(0, 0); // Choose not to optimize.
>+
>     // Defer GC for a while so that it doesn't run between when we enter into this
>     // slow path and when we figure out the state of our code block. This prevents
>     // a number of awkward reentrancy scenarios, including:
>@@ -1395,7 +1398,7 @@ SlowPathReturnType JIT_OPERATION operati
>     // that case we would have already planted the optimized code block into the JS
>     // stack.
>     DeferGCForAWhile deferGC(vm.heap);
>-    
>+
>     CodeBlock* codeBlock = exec->codeBlock();
>     if (UNLIKELY(codeBlock->jitType() != JITCode::BaselineJIT)) {
>         dataLog("Unexpected code block in Baseline->DFG tier-up: ", *codeBlock, "\n");
>Index: Source/JavaScriptCore/runtime/VMTraps.cpp
>===================================================================
>--- Source/JavaScriptCore/runtime/VMTraps.cpp	(revision 235560)
>+++ Source/JavaScriptCore/runtime/VMTraps.cpp	(working copy)
>@@ -126,7 +126,7 @@ void VMTraps::tryInstallTrapBreakpoints(
>         if (!isSaneFrame(callFrame, calleeFrame, entryFrame, stackBounds))
>             return; // Let the SignalSender try again later.
> 
>-        CodeBlock* candidateCodeBlock = callFrame->codeBlock();
>+        CodeBlock* candidateCodeBlock = callFrame->unsafeCodeBlock();
>         if (candidateCodeBlock && vm.heap.codeBlockSet().contains(codeBlockSetLocker, candidateCodeBlock)) {
>             foundCodeBlock = candidateCodeBlock;
>             break;

Flags:

ews-watchlist: commit-queue-

Actions: View | Formatted Diff | Diff

Attachments on bug 189227: 348728 | 348733 | 348785 | 348786 | 348787 | 348794