Inlining of a function that ends in op_unreachable in a non-tail position triggers...
authorrmorisset@apple.com <rmorisset@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Fri, 30 Mar 2018 14:03:10 +0000 (14:03 +0000)
committerrmorisset@apple.com <rmorisset@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Fri, 30 Mar 2018 14:03:10 +0000 (14:03 +0000)
https://bugs.webkit.org/show_bug.cgi?id=183812

Reviewed by Keith Miller.

JSTests:

* stress/inlining-unreachable-non-tail.js: Added.
(foo.):
(foo):

Source/JavaScriptCore:

The fix I landed for https://bugs.webkit.org/show_bug.cgi?id=181027 was flawed: I tried setting the bytecodeIndex for the new block on line 1679 (at the end of inlineCall), but it is going to be reset on line 6612 (in parseCodeBlock).
The fix is simply to make the block untargetable by default, and let parseCodeBlock make it targetable afterwards if it is a jump target.

* dfg/DFGByteCodeParser.cpp:
(JSC::DFG::ByteCodeParser::allocateTargetableBlock):
(JSC::DFG::ByteCodeParser::inlineCall):

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

JSTests/ChangeLog
JSTests/stress/inlining-unreachable-non-tail.js [new file with mode: 0644]
Source/JavaScriptCore/ChangeLog
Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp

index 0954167..be61bef 100644 (file)
@@ -1,5 +1,16 @@
 2018-03-30  Robin Morisset  <rmorisset@apple.com>
 
+        Inlining of a function that ends in op_unreachable in a non-tail position triggers an ASSERT
+        https://bugs.webkit.org/show_bug.cgi?id=183812
+
+        Reviewed by Keith Miller.
+
+        * stress/inlining-unreachable-non-tail.js: Added.
+        (foo.):
+        (foo):
+
+2018-03-30  Robin Morisset  <rmorisset@apple.com>
+
         A stack overflow in the parsing of a builtin (called by createExecutable) cause a crash instead of a catchable js exception
         https://bugs.webkit.org/show_bug.cgi?id=184074
         <rdar://problem/37165897>
diff --git a/JSTests/stress/inlining-unreachable-non-tail.js b/JSTests/stress/inlining-unreachable-non-tail.js
new file mode 100644 (file)
index 0000000..7ceb136
--- /dev/null
@@ -0,0 +1,13 @@
+"use strict";
+
+var foo = class {
+  constructor() {
+    foo();
+    try {} catch {}
+  }
+};
+for (var i = 0; i < 1000; i++) {
+  try {
+    new foo();
+  } catch {}
+}
index bfb2088..ffb17e1 100644 (file)
@@ -1,5 +1,19 @@
 2018-03-30  Robin Morisset  <rmorisset@apple.com>
 
+        Inlining of a function that ends in op_unreachable in a non-tail position triggers an ASSERT
+        https://bugs.webkit.org/show_bug.cgi?id=183812
+
+        Reviewed by Keith Miller.
+
+        The fix I landed for https://bugs.webkit.org/show_bug.cgi?id=181027 was flawed: I tried setting the bytecodeIndex for the new block on line 1679 (at the end of inlineCall), but it is going to be reset on line 6612 (in parseCodeBlock).
+        The fix is simply to make the block untargetable by default, and let parseCodeBlock make it targetable afterwards if it is a jump target.
+
+        * dfg/DFGByteCodeParser.cpp:
+        (JSC::DFG::ByteCodeParser::allocateTargetableBlock):
+        (JSC::DFG::ByteCodeParser::inlineCall):
+
+2018-03-30  Robin Morisset  <rmorisset@apple.com>
+
         A stack overflow in the parsing of a builtin (called by createExecutable) cause a crash instead of a catchable js exception
         https://bugs.webkit.org/show_bug.cgi?id=184074
         <rdar://problem/37165897>
index 9b45b1b..87fa088 100644 (file)
@@ -139,7 +139,6 @@ private:
     // It is also used when doing an early return from an inlined callee: it is easier to fix the bytecode index later on if needed
     // than to move the right index all the way to the treatment of op_ret.
     BasicBlock* allocateTargetableBlock(unsigned bytecodeIndex);
-    BasicBlock* allocateTargetableBlock(InlineStackEntry*, unsigned bytecodeIndex);
     BasicBlock* allocateUntargetableBlock();
     // An untargetable block can be given a bytecodeIndex to be later managed by linkBlock, but only once, and it can never go in the other direction
     void makeBlockTargetable(BasicBlock*, unsigned bytecodeIndex);
@@ -1158,18 +1157,13 @@ private:
 
 BasicBlock* ByteCodeParser::allocateTargetableBlock(unsigned bytecodeIndex)
 {
-    return allocateTargetableBlock(m_inlineStackTop, bytecodeIndex);
-}
-
-BasicBlock* ByteCodeParser::allocateTargetableBlock(InlineStackEntry* stackEntry, unsigned bytecodeIndex)
-{
     ASSERT(bytecodeIndex != UINT_MAX);
     Ref<BasicBlock> block = adoptRef(*new BasicBlock(bytecodeIndex, m_numArguments, m_numLocals, 1));
     BasicBlock* blockPtr = block.ptr();
     // m_blockLinkingTargets must always be sorted in increasing order of bytecodeBegin
-    if (stackEntry->m_blockLinkingTargets.size())
-        ASSERT(stackEntry->m_blockLinkingTargets.last()->bytecodeBegin < bytecodeIndex);
-    stackEntry->m_blockLinkingTargets.append(blockPtr);
+    if (m_inlineStackTop->m_blockLinkingTargets.size())
+        ASSERT(m_inlineStackTop->m_blockLinkingTargets.last()->bytecodeBegin < bytecodeIndex);
+    m_inlineStackTop->m_blockLinkingTargets.append(blockPtr);
     m_graph.appendBlock(WTFMove(block));
     return blockPtr;
 }
@@ -1672,11 +1666,10 @@ void ByteCodeParser::inlineCall(Node* callTargetNode, int resultOperand, CallVar
     
     // Most functions have at least one op_ret and thus set up the continuation block.
     // In some rare cases, a function ends in op_unreachable, forcing us to allocate a new continuationBlock here.
-    // We must be careful to allocate it in the caller and not the top of the inline stack, since the callee is still on the stack at this point.
     if (inlineStackEntry.m_continuationBlock)
         m_currentBlock = inlineStackEntry.m_continuationBlock;
     else
-        m_currentBlock = allocateTargetableBlock(inlineStackEntry.m_caller, m_currentIndex);
+        m_currentBlock = allocateUntargetableBlock();
     ASSERT(!m_currentBlock->terminal());
 
     prepareToParseBlock();