DFG shouldn't treat the 'this' argument as being captured if a code block uses arguments
authorfpizlo@apple.com <fpizlo@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Wed, 9 Jan 2013 01:11:32 +0000 (01:11 +0000)
committerfpizlo@apple.com <fpizlo@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Wed, 9 Jan 2013 01:11:32 +0000 (01:11 +0000)
https://bugs.webkit.org/show_bug.cgi?id=106398
<rdar://problem/12439776>

Source/JavaScriptCore:

Reviewed by Mark Hahnenberg.

This is a possible optimization for inlined calls, and fixes crashes for inlined constructors, in the case
that the inlined code used arguments. The problem was that assuming that 'this' was captured implies the
assumption that it was initialized by the caller, which is wrong for constructors and this.

Also added a pretty essential DFG IR validation rule: we shouldn't have any live locals at the top of the
root block. This helps to catch this bug: our assumption that 'this' was captured in an inlined constructor
that used arguments led to liveness for the temporary that would have held 'this' in the caller being
propagated all the way up to the entrypoint of the function.

* bytecode/CodeBlock.h:
(JSC::CodeBlock::isCaptured):
* dfg/DFGValidate.cpp:
(JSC::DFG::Validate::validate):
(JSC::DFG::Validate::reportValidationContext):
(Validate):
(JSC::DFG::Validate::dumpGraphIfAppropriate):

LayoutTests:

Reviewed by Mark Hahnenberg.

* fast/js/dfg-inline-constructor-that-uses-arguments-expected.txt: Added.
* fast/js/dfg-inline-constructor-that-uses-arguments.html: Added.
* fast/js/jsc-test-list:
* fast/js/script-tests/dfg-inline-constructor-that-uses-arguments.js: Added.
(Foo):
(bar):

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

LayoutTests/ChangeLog
LayoutTests/fast/js/dfg-inline-constructor-that-uses-arguments-expected.txt [new file with mode: 0644]
LayoutTests/fast/js/dfg-inline-constructor-that-uses-arguments.html [new file with mode: 0644]
LayoutTests/fast/js/jsc-test-list
LayoutTests/fast/js/script-tests/dfg-inline-constructor-that-uses-arguments.js [new file with mode: 0644]
Source/JavaScriptCore/ChangeLog
Source/JavaScriptCore/bytecode/CodeBlock.h
Source/JavaScriptCore/dfg/DFGValidate.cpp

index 540d69c..606d402 100644 (file)
@@ -1,3 +1,18 @@
+2013-01-08  Filip Pizlo  <fpizlo@apple.com>
+
+        DFG shouldn't treat the 'this' argument as being captured if a code block uses arguments
+        https://bugs.webkit.org/show_bug.cgi?id=106398
+        <rdar://problem/12439776>
+
+        Reviewed by Mark Hahnenberg.
+
+        * fast/js/dfg-inline-constructor-that-uses-arguments-expected.txt: Added.
+        * fast/js/dfg-inline-constructor-that-uses-arguments.html: Added.
+        * fast/js/jsc-test-list:
+        * fast/js/script-tests/dfg-inline-constructor-that-uses-arguments.js: Added.
+        (Foo):
+        (bar):
+
 2013-01-08  Viatcheslav Ostapenko  <sl.ostapenko@samsung.com>
 
         [EFL][WK2] WebGL test cases are sometimes crashing
diff --git a/LayoutTests/fast/js/dfg-inline-constructor-that-uses-arguments-expected.txt b/LayoutTests/fast/js/dfg-inline-constructor-that-uses-arguments-expected.txt
new file mode 100644 (file)
index 0000000..f77e95a
--- /dev/null
@@ -0,0 +1,209 @@
+Tests that we can inline a constructor that uses arguments without failing DFG validation.
+
+On success, you will see a series of "PASS" messages, followed by "TEST COMPLETE".
+
+
+PASS bar().x is 42
+PASS bar().x is 42
+PASS bar().x is 42
+PASS bar().x is 42
+PASS bar().x is 42
+PASS bar().x is 42
+PASS bar().x is 42
+PASS bar().x is 42
+PASS bar().x is 42
+PASS bar().x is 42
+PASS bar().x is 42
+PASS bar().x is 42
+PASS bar().x is 42
+PASS bar().x is 42
+PASS bar().x is 42
+PASS bar().x is 42
+PASS bar().x is 42
+PASS bar().x is 42
+PASS bar().x is 42
+PASS bar().x is 42
+PASS bar().x is 42
+PASS bar().x is 42
+PASS bar().x is 42
+PASS bar().x is 42
+PASS bar().x is 42
+PASS bar().x is 42
+PASS bar().x is 42
+PASS bar().x is 42
+PASS bar().x is 42
+PASS bar().x is 42
+PASS bar().x is 42
+PASS bar().x is 42
+PASS bar().x is 42
+PASS bar().x is 42
+PASS bar().x is 42
+PASS bar().x is 42
+PASS bar().x is 42
+PASS bar().x is 42
+PASS bar().x is 42
+PASS bar().x is 42
+PASS bar().x is 42
+PASS bar().x is 42
+PASS bar().x is 42
+PASS bar().x is 42
+PASS bar().x is 42
+PASS bar().x is 42
+PASS bar().x is 42
+PASS bar().x is 42
+PASS bar().x is 42
+PASS bar().x is 42
+PASS bar().x is 42
+PASS bar().x is 42
+PASS bar().x is 42
+PASS bar().x is 42
+PASS bar().x is 42
+PASS bar().x is 42
+PASS bar().x is 42
+PASS bar().x is 42
+PASS bar().x is 42
+PASS bar().x is 42
+PASS bar().x is 42
+PASS bar().x is 42
+PASS bar().x is 42
+PASS bar().x is 42
+PASS bar().x is 42
+PASS bar().x is 42
+PASS bar().x is 42
+PASS bar().x is 42
+PASS bar().x is 42
+PASS bar().x is 42
+PASS bar().x is 42
+PASS bar().x is 42
+PASS bar().x is 42
+PASS bar().x is 42
+PASS bar().x is 42
+PASS bar().x is 42
+PASS bar().x is 42
+PASS bar().x is 42
+PASS bar().x is 42
+PASS bar().x is 42
+PASS bar().x is 42
+PASS bar().x is 42
+PASS bar().x is 42
+PASS bar().x is 42
+PASS bar().x is 42
+PASS bar().x is 42
+PASS bar().x is 42
+PASS bar().x is 42
+PASS bar().x is 42
+PASS bar().x is 42
+PASS bar().x is 42
+PASS bar().x is 42
+PASS bar().x is 42
+PASS bar().x is 42
+PASS bar().x is 42
+PASS bar().x is 42
+PASS bar().x is 42
+PASS bar().x is 42
+PASS bar().x is 42
+PASS bar().x is 42
+PASS bar().x is 42
+PASS bar().x is 42
+PASS bar().x is 42
+PASS bar().x is 42
+PASS bar().x is 42
+PASS bar().x is 42
+PASS bar().x is 42
+PASS bar().x is 42
+PASS bar().x is 42
+PASS bar().x is 42
+PASS bar().x is 42
+PASS bar().x is 42
+PASS bar().x is 42
+PASS bar().x is 42
+PASS bar().x is 42
+PASS bar().x is 42
+PASS bar().x is 42
+PASS bar().x is 42
+PASS bar().x is 42
+PASS bar().x is 42
+PASS bar().x is 42
+PASS bar().x is 42
+PASS bar().x is 42
+PASS bar().x is 42
+PASS bar().x is 42
+PASS bar().x is 42
+PASS bar().x is 42
+PASS bar().x is 42
+PASS bar().x is 42
+PASS bar().x is 42
+PASS bar().x is 42
+PASS bar().x is 42
+PASS bar().x is 42
+PASS bar().x is 42
+PASS bar().x is 42
+PASS bar().x is 42
+PASS bar().x is 42
+PASS bar().x is 42
+PASS bar().x is 42
+PASS bar().x is 42
+PASS bar().x is 42
+PASS bar().x is 42
+PASS bar().x is 42
+PASS bar().x is 42
+PASS bar().x is 42
+PASS bar().x is 42
+PASS bar().x is 42
+PASS bar().x is 42
+PASS bar().x is 42
+PASS bar().x is 42
+PASS bar().x is 42
+PASS bar().x is 42
+PASS bar().x is 42
+PASS bar().x is 42
+PASS bar().x is 42
+PASS bar().x is 42
+PASS bar().x is 42
+PASS bar().x is 42
+PASS bar().x is 42
+PASS bar().x is 42
+PASS bar().x is 42
+PASS bar().x is 42
+PASS bar().x is 42
+PASS bar().x is 42
+PASS bar().x is 42
+PASS bar().x is 42
+PASS bar().x is 42
+PASS bar().x is 42
+PASS bar().x is 42
+PASS bar().x is 42
+PASS bar().x is 42
+PASS bar().x is 42
+PASS bar().x is 42
+PASS bar().x is 42
+PASS bar().x is 42
+PASS bar().x is 42
+PASS bar().x is 42
+PASS bar().x is 42
+PASS bar().x is 42
+PASS bar().x is 42
+PASS bar().x is 42
+PASS bar().x is 42
+PASS bar().x is 42
+PASS bar().x is 42
+PASS bar().x is 42
+PASS bar().x is 42
+PASS bar().x is 42
+PASS bar().x is 42
+PASS bar().x is 42
+PASS bar().x is 42
+PASS bar().x is 42
+PASS bar().x is 42
+PASS bar().x is 42
+PASS bar().x is 42
+PASS bar().x is 42
+PASS bar().x is 42
+PASS bar().x is 42
+PASS bar().x is 42
+PASS bar().x is 42
+PASS bar().x is 42
+PASS successfullyParsed is true
+
+TEST COMPLETE
+
diff --git a/LayoutTests/fast/js/dfg-inline-constructor-that-uses-arguments.html b/LayoutTests/fast/js/dfg-inline-constructor-that-uses-arguments.html
new file mode 100644 (file)
index 0000000..5c86997
--- /dev/null
@@ -0,0 +1,10 @@
+<!DOCTYPE HTML PUBLIC "-//IETF//DTD HTML//EN">
+<html>
+<head>
+<script src="resources/js-test-pre.js"></script>
+</head>
+<body>
+<script src="script-tests/dfg-inline-constructor-that-uses-arguments.js"></script>
+<script src="resources/js-test-post.js"></script>
+</body>
+</html>
index 7ce8bca..1f44273 100644 (file)
@@ -131,6 +131,7 @@ fast/js/dfg-inline-arguments-use-from-all-the-places
 fast/js/dfg-inline-arguments-use-from-getter
 fast/js/dfg-inline-arguments-use-from-uninlined-code
 fast/js/dfg-inline-constant
+fast/js/dfg-inline-constructor-that-uses-arguments
 fast/js/dfg-inline-early-return
 fast/js/dfg-inline-function-dot-caller
 fast/js/dfg-inline-many-blocks
diff --git a/LayoutTests/fast/js/script-tests/dfg-inline-constructor-that-uses-arguments.js b/LayoutTests/fast/js/script-tests/dfg-inline-constructor-that-uses-arguments.js
new file mode 100644 (file)
index 0000000..8ca9f25
--- /dev/null
@@ -0,0 +1,14 @@
+description(
+"Tests that we can inline a constructor that uses arguments without failing DFG validation."
+);
+
+function Foo() {
+    this.x = arguments[0];
+}
+
+function bar() {
+    return new Foo(42);
+}
+
+for (var i = 0; i < 200; ++i)
+    shouldBe("bar().x", "42");
index 3561b87..1731f89 100644 (file)
@@ -1,5 +1,30 @@
 2013-01-08  Filip Pizlo  <fpizlo@apple.com>
 
+        DFG shouldn't treat the 'this' argument as being captured if a code block uses arguments
+        https://bugs.webkit.org/show_bug.cgi?id=106398
+        <rdar://problem/12439776>
+
+        Reviewed by Mark Hahnenberg.
+        
+        This is a possible optimization for inlined calls, and fixes crashes for inlined constructors, in the case
+        that the inlined code used arguments. The problem was that assuming that 'this' was captured implies the
+        assumption that it was initialized by the caller, which is wrong for constructors and this.
+        
+        Also added a pretty essential DFG IR validation rule: we shouldn't have any live locals at the top of the
+        root block. This helps to catch this bug: our assumption that 'this' was captured in an inlined constructor
+        that used arguments led to liveness for the temporary that would have held 'this' in the caller being
+        propagated all the way up to the entrypoint of the function.
+
+        * bytecode/CodeBlock.h:
+        (JSC::CodeBlock::isCaptured):
+        * dfg/DFGValidate.cpp:
+        (JSC::DFG::Validate::validate):
+        (JSC::DFG::Validate::reportValidationContext):
+        (Validate):
+        (JSC::DFG::Validate::dumpGraphIfAppropriate):
+
+2013-01-08  Filip Pizlo  <fpizlo@apple.com>
+
         REGRESSION (r138921): Crash in JSC::Arguments::create
         https://bugs.webkit.org/show_bug.cgi?id=106329
         <rdar://problem/12974196>
index b84f165..a6fa6d5 100644 (file)
@@ -594,11 +594,11 @@ namespace JSC {
         
         bool isCaptured(int operand, InlineCallFrame* inlineCallFrame = 0) const
         {
-            if (inlineCallFrame && !operandIsArgument(operand))
-                return inlineCallFrame->capturedVars.get(operand);
-
             if (operandIsArgument(operand))
-                return usesArguments();
+                return operandToArgument(operand) && usesArguments();
+
+            if (inlineCallFrame)
+                return inlineCallFrame->capturedVars.get(operand);
 
             // The activation object isn't in the captured region, but it's "captured"
             // in the sense that stores to its location can be observed indirectly.
index 274b544..8e182dd 100644 (file)
@@ -1,5 +1,5 @@
 /*
- * Copyright (C) 2012 Apple Inc. All rights reserved.
+ * Copyright (C) 2012, 2013 Apple Inc. All rights reserved.
  *
  * Redistribution and use in source and binary forms, with or without
  * modification, are permitted provided that the following conditions
@@ -28,6 +28,7 @@
 
 #if ENABLE(DFG_JIT)
 
+#include "CodeBlockWithJITType.h"
 #include <wtf/Assertions.h>
 #include <wtf/BitVector.h>
 
@@ -76,6 +77,11 @@ public:
         // NB. This code is not written for performance, since it is not intended to run
         // in release builds.
         
+        // Validate that all local variables at the head of the root block are dead.
+        BasicBlock* root = m_graph.m_blocks[0].get();
+        for (unsigned i = 0; i < root->variablesAtHead.numberOfLocals(); ++i)
+            V_EQUAL((static_cast<VirtualRegister>(i), 0), NoNode, root->variablesAtHead.local(i));
+        
         // Validate ref counts and uses.
         Vector<unsigned> myRefCounts;
         myRefCounts.fill(0, m_graph.size());
@@ -304,6 +310,11 @@ private:
         dataLogF("@%u -> %s@%u", nodeIndex, useKindToString(edge.useKind()), edge.index());
     }
     
+    void reportValidationContext(VirtualRegister local, BlockIndex blockIndex)
+    {
+        dataLogF("r%d in Block #%u", local, blockIndex);
+    }
+    
     void reportValidationContext(
         VirtualRegister local, BlockIndex sourceBlockIndex, BlockTag, BlockIndex destinationBlockIndex)
     {
@@ -343,7 +354,7 @@ private:
     {
         if (m_graphDumpMode == DontDumpGraph)
             return;
-        dataLogF("Graph at time of failure:\n");
+        dataLog("Graph of ", CodeBlockWithJITType(m_graph.m_codeBlock, JITCode::DFGJIT), " at time of failure:\n");
         m_graph.dump();
     }
 };