The backend should be happy to compile Unreachable even if AI didn't prove it to...
authorfpizlo@apple.com <fpizlo@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Fri, 10 Jun 2016 19:56:18 +0000 (19:56 +0000)
committerfpizlo@apple.com <fpizlo@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Fri, 10 Jun 2016 19:56:18 +0000 (19:56 +0000)
https://bugs.webkit.org/show_bug.cgi?id=158631

Reviewed by Keith Miller.

We've been slowly making the DFG Unreachable opcode behave like a grown-up. When we first
added it, it was a hack for Throw, and we could always rely on AI proving that Unreachable
was not reachable. But then we started using Unreachable as a proper Unreachable opcode,
like Oops in B3 for example, which has a more nuanced meaning: you use it whenever you
emit code that *you* know will not return, and you need some way of terminating the basic
block. The DFG is not a proof-carrying compiler, and it never will be. So, when you have
proved that something is not reachable, you should be able to use Unreachable even if
there is no guarantee that the compiler will later be able to replicate your proof. This
means that the backend may find itself compiling Unreachable because AI did not prove that
it was unreachable.

Prior to this change, we would crash compiling Unreachable because we would rely on AI
preventing us from reaching Unreachable in the backend. But that's silly! We don't want
users of Unreachable to have to also convince AI that their Unreachable is really
Unreachable.

This fixes crashes on real websites. I couldn't work out how to turn them into a reduced
test.

* assembler/AbortReason.h:
* dfg/DFGSpeculativeJIT.cpp:
(JSC::DFG::SpeculativeJIT::emitInvalidationPoint):
(JSC::DFG::SpeculativeJIT::unreachable):
(JSC::DFG::SpeculativeJIT::terminateSpeculativeExecution):
* dfg/DFGSpeculativeJIT.h:
* dfg/DFGSpeculativeJIT32_64.cpp:
(JSC::DFG::SpeculativeJIT::compile):
* dfg/DFGSpeculativeJIT64.cpp:
(JSC::DFG::SpeculativeJIT::compile):
* ftl/FTLLowerDFGToB3.cpp:
(JSC::FTL::DFG::LowerDFGToB3::compileNode):
(JSC::FTL::DFG::LowerDFGToB3::compilePutDynamicVar):
(JSC::FTL::DFG::LowerDFGToB3::compileUnreachable):
(JSC::FTL::DFG::LowerDFGToB3::compareEqObjectOrOtherToObject):

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

Source/JavaScriptCore/ChangeLog
Source/JavaScriptCore/assembler/AbortReason.h
Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp
Source/JavaScriptCore/dfg/DFGSpeculativeJIT.h
Source/JavaScriptCore/dfg/DFGSpeculativeJIT32_64.cpp
Source/JavaScriptCore/dfg/DFGSpeculativeJIT64.cpp
Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp

index f33368b..a9f7ad4 100644 (file)
@@ -1,3 +1,45 @@
+2016-06-10  Filip Pizlo  <fpizlo@apple.com>
+
+        The backend should be happy to compile Unreachable even if AI didn't prove it to be unreachable
+        https://bugs.webkit.org/show_bug.cgi?id=158631
+
+        Reviewed by Keith Miller.
+        
+        We've been slowly making the DFG Unreachable opcode behave like a grown-up. When we first
+        added it, it was a hack for Throw, and we could always rely on AI proving that Unreachable
+        was not reachable. But then we started using Unreachable as a proper Unreachable opcode,
+        like Oops in B3 for example, which has a more nuanced meaning: you use it whenever you
+        emit code that *you* know will not return, and you need some way of terminating the basic
+        block. The DFG is not a proof-carrying compiler, and it never will be. So, when you have
+        proved that something is not reachable, you should be able to use Unreachable even if
+        there is no guarantee that the compiler will later be able to replicate your proof. This
+        means that the backend may find itself compiling Unreachable because AI did not prove that
+        it was unreachable.
+        
+        Prior to this change, we would crash compiling Unreachable because we would rely on AI
+        preventing us from reaching Unreachable in the backend. But that's silly! We don't want
+        users of Unreachable to have to also convince AI that their Unreachable is really
+        Unreachable.
+        
+        This fixes crashes on real websites. I couldn't work out how to turn them into a reduced
+        test.
+
+        * assembler/AbortReason.h:
+        * dfg/DFGSpeculativeJIT.cpp:
+        (JSC::DFG::SpeculativeJIT::emitInvalidationPoint):
+        (JSC::DFG::SpeculativeJIT::unreachable):
+        (JSC::DFG::SpeculativeJIT::terminateSpeculativeExecution):
+        * dfg/DFGSpeculativeJIT.h:
+        * dfg/DFGSpeculativeJIT32_64.cpp:
+        (JSC::DFG::SpeculativeJIT::compile):
+        * dfg/DFGSpeculativeJIT64.cpp:
+        (JSC::DFG::SpeculativeJIT::compile):
+        * ftl/FTLLowerDFGToB3.cpp:
+        (JSC::FTL::DFG::LowerDFGToB3::compileNode):
+        (JSC::FTL::DFG::LowerDFGToB3::compilePutDynamicVar):
+        (JSC::FTL::DFG::LowerDFGToB3::compileUnreachable):
+        (JSC::FTL::DFG::LowerDFGToB3::compareEqObjectOrOtherToObject):
+
 2016-06-09  Alex Christensen  <achristensen@webkit.org>
 
         Clean up JavaScriptCore.vcxproj directory after switching to CMake.
index 0e82dad..57c990f 100644 (file)
@@ -1,5 +1,5 @@
 /*
- * Copyright (C) 2014, 2015 Apple Inc. All rights reserved.
+ * Copyright (C) 2014-2016 Apple Inc. All rights reserved.
  *
  * Redistribution and use in source and binary forms, with or without
  * modification, are permitted provided that the following conditions
@@ -57,6 +57,7 @@ enum AbortReason {
     DFGNegativeStringLength                           = 200,
     DFGSlowPathGeneratorFellThrough                   = 210,
     DFGUnreachableBasicBlock                          = 220,
+    DFGUnreachableNode                                = 225,
     DFGUnreasonableOSREntryJumpDestination            = 230,
     DFGVarargsThrowingPathDidNotThrow                 = 235,
     JITDidReturnFromTailCall                          = 237,
index 0581acd..73126b6 100644 (file)
@@ -307,6 +307,12 @@ void SpeculativeJIT::emitInvalidationPoint(Node* node)
     noResult(node);
 }
 
+void SpeculativeJIT::unreachable(Node* node)
+{
+    m_compileOkay = false;
+    m_jit.abortWithReason(DFGUnreachableNode, node->op());
+}
+
 void SpeculativeJIT::terminateSpeculativeExecution(ExitKind kind, JSValueRegs jsValueRegs, Node* node)
 {
     if (!m_compileOkay)
index 431db5a..5926f5c 100644 (file)
@@ -2661,6 +2661,8 @@ public:
     
     void emitInvalidationPoint(Node*);
     
+    void unreachable(Node*);
+    
     // Called when we statically determine that a speculation will fail.
     void terminateSpeculativeExecution(ExitKind, JSValueRegs, Node*);
     void terminateSpeculativeExecution(ExitKind, JSValueRegs, Edge);
index 6b3862a..8498bf0 100644 (file)
@@ -5273,7 +5273,7 @@ void SpeculativeJIT::compile(Node* node)
     }
 
     case Unreachable:
-        RELEASE_ASSERT_NOT_REACHED();
+        unreachable(node);
         break;
 
     case LastNodeType:
index 744139b..1d4fa76 100644 (file)
@@ -4801,7 +4801,7 @@ void SpeculativeJIT::compile(Node* node)
         break;
 
     case Unreachable:
-        DFG_CRASH(m_jit.graph(), node, "Unexpected Unreachable node");
+        unreachable(node);
         break;
 
     case StoreBarrier: {
index eacf24a..dde7c77 100644 (file)
@@ -1000,6 +1000,9 @@ private:
         case PutDynamicVar:
             compilePutDynamicVar();
             break;
+        case Unreachable:
+            compileUnreachable();
+            break;
 
         case PhantomLocal:
         case LoopHint:
@@ -7684,6 +7687,18 @@ private:
             m_callFrame, lowCell(m_node->child1()), lowJSValue(m_node->child2()), m_out.constIntPtr(uid), m_out.constInt32(m_node->getPutInfo())));
     }
     
+    void compileUnreachable()
+    {
+        // It's so tempting to assert that AI has proved that this is unreachable. But that's
+        // simply not a requirement of the Unreachable opcode at all. If you emit an opcode that
+        // *you* know will not return, then it's fine to end the basic block with Unreachable
+        // after that opcode. You don't have to also prove to AI that your opcode does not return.
+        // Hence, there is nothing to do here but emit code that will crash, so that we catch
+        // cases where you said Unreachable but you lied.
+        
+        crash();
+    }
+    
     void compareEqObjectOrOtherToObject(Edge leftChild, Edge rightChild)
     {
         LValue rightCell = lowCell(rightChild);