Attachment 348787 Details for Bug 189227 – proposed patch.

[patch] proposed patch.

bug-189227.patch (text/plain), 10.23 KB, created by Mark Lam on 2018-09-03 13:08:11 PDT

(hide)

Description:

Filename:

MIME Type:

Creator: Mark Lam

Created: 2018-09-03 13:08:11 PDT

Size: 10.23 KB

patch

obsolete

>Index: JSTests/ChangeLog
>===================================================================
>--- JSTests/ChangeLog	(revision 235600)
>+++ JSTests/ChangeLog	(working copy)
>@@ -1,3 +1,13 @@
>+2018-09-03  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-09-02  Yusuke Suzuki  <yusukesuzuki@slowstart.org>
> 
>         Implement Object.fromEntries
>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,4 @@
>+//@ requireOptions("--watchdog=20", "--jitPolicyScale=0", "--watchdog-exception-ok")
>+
>+// This test should not time out.
>+while (1) { }
>Index: Source/JavaScriptCore/ChangeLog
>===================================================================
>--- Source/JavaScriptCore/ChangeLog	(revision 235588)
>+++ Source/JavaScriptCore/ChangeLog	(working copy)
>@@ -1,3 +1,77 @@
>+2018-09-03  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 making the DFG BytecodeParser emit an InvalidationPoint
>+        node directly (when the VM is not configured to use polling traps).  This ensures
>+        that the check traps invalidation point will OSR exit to the op_check_traps opcode
>+        in the baseline JIT.
>+
>+        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/DFGByteCodeParser.cpp:
>+        (JSC::DFG::ByteCodeParser::parseBlock):
>+        * dfg/DFGClobberize.h:
>+        (JSC::DFG::clobberize):
>+        * dfg/DFGFixupPhase.cpp:
>+        (JSC::DFG::FixupPhase::fixupNode):
>+        * dfg/DFGPredictionPropagationPhase.cpp:
>+        * dfg/DFGSpeculativeJIT32_64.cpp:
>+        (JSC::DFG::SpeculativeJIT::compile):
>+        * dfg/DFGSpeculativeJIT64.cpp:
>+        (JSC::DFG::SpeculativeJIT::compile):
>+        * ftl/FTLLowerDFGToB3.cpp:
>+        (JSC::FTL::DFG::LowerDFGToB3::compileNode):
>+        * 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/DFGByteCodeParser.cpp
>===================================================================
>--- Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp	(revision 235560)
>+++ Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp	(working copy)
>@@ -6405,7 +6405,7 @@ void ByteCodeParser::parseBlock(unsigned
>         }
>         
>         case op_check_traps: {
>-            addToGraph(CheckTraps);
>+            addToGraph(Options::usePollingTraps() ? CheckTraps : InvalidationPoint);
>             NEXT_OPCODE(op_check_traps);
>         }
> 
>Index: Source/JavaScriptCore/dfg/DFGClobberize.h
>===================================================================
>--- Source/JavaScriptCore/dfg/DFGClobberize.h	(revision 235560)
>+++ Source/JavaScriptCore/dfg/DFGClobberize.h	(working copy)
>@@ -471,11 +471,8 @@ void clobberize(Graph& graph, Node* node
>         return;
> 
>     case CheckTraps:
>-        if (Options::usePollingTraps()) {
>-            read(InternalState);
>-            write(InternalState);
>-        } else
>-            write(Watchpoint_fire);
>+        read(InternalState);
>+        write(InternalState);
>         return;
> 
>     case InvalidationPoint:
>Index: Source/JavaScriptCore/dfg/DFGFixupPhase.cpp
>===================================================================
>--- Source/JavaScriptCore/dfg/DFGFixupPhase.cpp	(revision 235560)
>+++ Source/JavaScriptCore/dfg/DFGFixupPhase.cpp	(working copy)
>@@ -1612,7 +1612,6 @@ private:
>         case CheckTierUpInLoop:
>         case CheckTierUpAtReturn:
>         case CheckTierUpAndOSREnter:
>-        case InvalidationPoint:
>         case CheckArray:
>         case CheckInBounds:
>         case ConstantStoragePointer:
>@@ -2241,6 +2240,7 @@ private:
>         case FilterGetByIdStatus:
>         case FilterPutByIdStatus:
>         case FilterInByIdStatus:
>+        case InvalidationPoint:
>             break;
> #else
>         default:
>Index: Source/JavaScriptCore/dfg/DFGPredictionPropagationPhase.cpp
>===================================================================
>--- Source/JavaScriptCore/dfg/DFGPredictionPropagationPhase.cpp	(revision 235560)
>+++ Source/JavaScriptCore/dfg/DFGPredictionPropagationPhase.cpp	(working copy)
>@@ -1105,7 +1105,6 @@ private:
>         case CheckTierUpInLoop:
>         case CheckTierUpAtReturn:
>         case CheckTierUpAndOSREnter:
>-        case InvalidationPoint:
>         case CheckInBounds:
>         case ValueToInt32:
>         case DoubleRep:
>@@ -1230,6 +1229,7 @@ private:
>         case FilterInByIdStatus:
>         case ClearCatchLocals:
>         case DataViewSet:
>+        case InvalidationPoint:
>             break;
>             
>         // This gets ignored because it only pretends to produce a value.
>Index: Source/JavaScriptCore/dfg/DFGSpeculativeJIT32_64.cpp
>===================================================================
>--- Source/JavaScriptCore/dfg/DFGSpeculativeJIT32_64.cpp	(revision 235560)
>+++ Source/JavaScriptCore/dfg/DFGSpeculativeJIT32_64.cpp	(working copy)
>@@ -3964,10 +3964,7 @@ void SpeculativeJIT::compile(Node* node)
>         break;
> 
>     case CheckTraps:
>-        if (Options::usePollingTraps())
>-            compileCheckTraps(node);
>-        else
>-            noResult(node); // This is a no-op.
>+        compileCheckTraps(node);
>         break;
> 
>     case CountExecution:
>Index: Source/JavaScriptCore/dfg/DFGSpeculativeJIT64.cpp
>===================================================================
>--- Source/JavaScriptCore/dfg/DFGSpeculativeJIT64.cpp	(revision 235560)
>+++ Source/JavaScriptCore/dfg/DFGSpeculativeJIT64.cpp	(working copy)
>@@ -4445,10 +4445,7 @@ void SpeculativeJIT::compile(Node* node)
>         break;
> 
>     case CheckTraps:
>-        if (Options::usePollingTraps())
>-            compileCheckTraps(node);
>-        else
>-            noResult(node); // This is a no-op.
>+        compileCheckTraps(node);
>         break;
> 
>     case Phantom:
>Index: Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp
>===================================================================
>--- Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp	(revision 235560)
>+++ Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp	(working copy)
>@@ -1216,8 +1216,7 @@ private:
>             compileMaterializeCreateActivation();
>             break;
>         case CheckTraps:
>-            if (Options::usePollingTraps())
>-                compileCheckTraps();
>+            compileCheckTraps();
>             break;
>         case CreateRest:
>             compileCreateRest();
>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:

saam: review+
mark.lam: commit-queue-

Actions: View | Formatted Diff | Diff

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