Log Message
DFG CFA filters CheckFunction in a really weird way, and assumes that the function's structure won't change https://bugs.webkit.org/show_bug.cgi?id=115077
Source/_javascript_Core:
Reviewed by Oliver Hunt.
The filtering did three things that are unusual:
1) AbstractValue::filterByValue() assumed that the passed value's structure wouldn't change, in
the sense that at it assumed it could use that value's *current* structure to do structure
filtering. Filtering by structure only makes sense if you can prove that the given value will
always have that structure (for example by either using a watchpoing or emitting code that
checks that structure at run-time).
2) AbstractValue::filterByValue() and the CheckFunction case in AbstractState::executeEffects()
tried to invalidate the CFA based on whether the filtration led to an empty value. This is
well-intentioned, but it's not how the CFA currently works. It's inconsistent with other
parts of the CFA. We shouldn't introduce this feature into just one kind of filtration and
not have it elsewhere.
3) The attempt to detect when the value was empty was actually implemented incorrectly. It
relied on AbstractValue::validate(). That method says that a concrete value does not belong
to the abstract value if it has a different structure. This makes sense for the other place
where AbstractValue::validate() is called: during OSR entry, where we are talking about a
JSValue that we see *right now*. It doesn't make sense in the CFA, since in the CFA any
value we observe in the code is a value whose structure may change when the code starts
running, and so we cannot use the value's current structure to infer things about the code
when it starts running.
I fixed the above problems by (1) changing filterByValue() to not filter the structure, (2)
changing filterByValue() and the CheckFunction case to not invalidate the CFA, and (3)
making sure that nobody else was misusing AbstractValue::validate() (they weren't).
* dfg/DFGAbstractState.cpp:
(JSC::DFG::AbstractState::executeEffects):
* dfg/DFGAbstractValue.h:
(JSC::DFG::AbstractValue::filterByValue):
LayoutTests:
Reviewed by Oliver Hunt.
This hilarious test fails prior to the rest of this patch.
* fast/js/dfg-check-function-change-structure-expected.txt: Added.
* fast/js/dfg-check-function-change-structure.html: Added.
* fast/js/jsc-test-list:
* fast/js/script-tests/dfg-check-function-change-structure.js: Added.
(foo):
(bar):
Modified Paths
- trunk/LayoutTests/ChangeLog
- trunk/LayoutTests/fast/js/jsc-test-list
- trunk/Source/_javascript_Core/ChangeLog
- trunk/Source/_javascript_Core/dfg/DFGAbstractState.cpp
- trunk/Source/_javascript_Core/dfg/DFGAbstractValue.h
Added Paths
Diff
Modified: trunk/LayoutTests/ChangeLog (149015 => 149016)
--- trunk/LayoutTests/ChangeLog 2013-04-24 01:59:21 UTC (rev 149015)
+++ trunk/LayoutTests/ChangeLog 2013-04-24 03:18:04 UTC (rev 149016)
@@ -1,3 +1,19 @@
+2013-04-23 Filip Pizlo <[email protected]>
+
+ DFG CFA filters CheckFunction in a really weird way, and assumes that the function's structure won't change
+ https://bugs.webkit.org/show_bug.cgi?id=115077
+
+ Reviewed by Oliver Hunt.
+
+ This hilarious test fails prior to the rest of this patch.
+
+ * fast/js/dfg-check-function-change-structure-expected.txt: Added.
+ * fast/js/dfg-check-function-change-structure.html: Added.
+ * fast/js/jsc-test-list:
+ * fast/js/script-tests/dfg-check-function-change-structure.js: Added.
+ (foo):
+ (bar):
+
2013-04-23 Mihai Tica <[email protected]>
Add platform support for -webkit-background-blend-mode to CG context with background color
Added: trunk/LayoutTests/fast/js/dfg-check-function-change-structure-expected.txt (0 => 149016)
--- trunk/LayoutTests/fast/js/dfg-check-function-change-structure-expected.txt (rev 0)
+++ trunk/LayoutTests/fast/js/dfg-check-function-change-structure-expected.txt 2013-04-24 03:18:04 UTC (rev 149016)
@@ -0,0 +1,104 @@
+PASS foo([bar]) is [42, 23]
+PASS foo([bar]) is [42, 23]
+PASS foo([bar]) is [42, 23]
+PASS foo([bar]) is [42, 23]
+PASS foo([bar]) is [42, 23]
+PASS foo([bar]) is [42, 23]
+PASS foo([bar]) is [42, 23]
+PASS foo([bar]) is [42, 23]
+PASS foo([bar]) is [42, 23]
+PASS foo([bar]) is [42, 23]
+PASS foo([bar]) is [42, 23]
+PASS foo([bar]) is [42, 23]
+PASS foo([bar]) is [42, 23]
+PASS foo([bar]) is [42, 23]
+PASS foo([bar]) is [42, 23]
+PASS foo([bar]) is [42, 23]
+PASS foo([bar]) is [42, 23]
+PASS foo([bar]) is [42, 23]
+PASS foo([bar]) is [42, 23]
+PASS foo([bar]) is [42, 23]
+PASS foo([bar]) is [42, 23]
+PASS foo([bar]) is [42, 23]
+PASS foo([bar]) is [42, 23]
+PASS foo([bar]) is [42, 23]
+PASS foo([bar]) is [42, 23]
+PASS foo([bar]) is [42, 23]
+PASS foo([bar]) is [42, 23]
+PASS foo([bar]) is [42, 23]
+PASS foo([bar]) is [42, 23]
+PASS foo([bar]) is [42, 23]
+PASS foo([bar]) is [42, 23]
+PASS foo([bar]) is [42, 23]
+PASS foo([bar]) is [42, 23]
+PASS foo([bar]) is [42, 23]
+PASS foo([bar]) is [42, 23]
+PASS foo([bar]) is [42, 23]
+PASS foo([bar]) is [42, 23]
+PASS foo([bar]) is [42, 23]
+PASS foo([bar]) is [42, 23]
+PASS foo([bar]) is [42, 23]
+PASS foo([bar]) is [42, 23]
+PASS foo([bar]) is [42, 23]
+PASS foo([bar]) is [42, 23]
+PASS foo([bar]) is [42, 23]
+PASS foo([bar]) is [42, 23]
+PASS foo([bar]) is [42, 23]
+PASS foo([bar]) is [42, 23]
+PASS foo([bar]) is [42, 23]
+PASS foo([bar]) is [42, 23]
+PASS foo([bar]) is [42, 23]
+PASS foo([bar]) is [42, 23]
+PASS foo([bar]) is [42, 23]
+PASS foo([bar]) is [42, 23]
+PASS foo([bar]) is [42, 23]
+PASS foo([bar]) is [42, 23]
+PASS foo([bar]) is [42, 23]
+PASS foo([bar]) is [42, 23]
+PASS foo([bar]) is [42, 23]
+PASS foo([bar]) is [42, 23]
+PASS foo([bar]) is [42, 23]
+PASS foo([bar]) is [42, 23]
+PASS foo([bar]) is [42, 23]
+PASS foo([bar]) is [42, 23]
+PASS foo([bar]) is [42, 23]
+PASS foo([bar]) is [42, 23]
+PASS foo([bar]) is [42, 23]
+PASS foo([bar]) is [42, 23]
+PASS foo([bar]) is [42, 23]
+PASS foo([bar]) is [42, 23]
+PASS foo([bar]) is [42, 23]
+PASS foo([bar]) is [42, 23]
+PASS foo([bar]) is [42, 23]
+PASS foo([bar]) is [42, 23]
+PASS foo([bar]) is [42, 23]
+PASS foo([bar]) is [42, 23]
+PASS foo([bar]) is [42, 23]
+PASS foo([bar]) is [42, 23]
+PASS foo([bar]) is [42, 23]
+PASS foo([bar]) is [42, 23]
+PASS foo([bar]) is [42, 23]
+PASS foo([bar]) is [42, 23]
+PASS foo([bar]) is [42, 23]
+PASS foo([bar]) is [42, 23]
+PASS foo([bar]) is [42, 23]
+PASS foo([bar]) is [42, 23]
+PASS foo([bar]) is [42, 23]
+PASS foo([bar]) is [42, 23]
+PASS foo([bar]) is [42, 23]
+PASS foo([bar]) is [42, 23]
+PASS foo([bar]) is [42, 23]
+PASS foo([bar]) is [42, 23]
+PASS foo([bar]) is [42, 23]
+PASS foo([bar]) is [42, 23]
+PASS foo([bar]) is [42, 23]
+PASS foo([bar]) is [42, 23]
+PASS foo([bar]) is [42, void 0]
+PASS foo([bar]) is [42, void 0]
+PASS foo([bar]) is [42, void 0]
+PASS foo([bar]) is [42, void 0]
+PASS foo([bar]) is [42, void 0]
+PASS successfullyParsed is true
+
+TEST COMPLETE
+
Added: trunk/LayoutTests/fast/js/dfg-check-function-change-structure.html (0 => 149016)
--- trunk/LayoutTests/fast/js/dfg-check-function-change-structure.html (rev 0)
+++ trunk/LayoutTests/fast/js/dfg-check-function-change-structure.html 2013-04-24 03:18:04 UTC (rev 149016)
@@ -0,0 +1,10 @@
+<!DOCTYPE HTML PUBLIC "-//IETF//DTD HTML//EN">
+<html>
+<head>
+<script src=""
+</head>
+<body>
+<script src=""
+<script src=""
+</body>
+</html>
Modified: trunk/LayoutTests/fast/js/jsc-test-list (149015 => 149016)
--- trunk/LayoutTests/fast/js/jsc-test-list 2013-04-24 01:59:21 UTC (rev 149015)
+++ trunk/LayoutTests/fast/js/jsc-test-list 2013-04-24 03:18:04 UTC (rev 149016)
@@ -105,8 +105,9 @@
fast/js/dfg-cfg-simplify-eliminate-set-local-type-check-then-typeof
fast/js/dfg-cfg-simplify-phantom-get-local-on-same-block-set-local
fast/js/dfg-cfg-simplify-redundant-dead-get-local
+fast/js/dfg-check-function-change-structure
+fast/js/dfg-check-structure-elimination-for-non-cell
fast/js/dfg-check-two-structures
-fast/js/dfg-check-structure-elimination-for-non-cell
fast/js/dfg-constant-fold-first-local-read-after-block-merge
fast/js/dfg-constant-fold-uncaptured-variable-that-is-later-captured
fast/js/dfg-convert-this-dom-window
Added: trunk/LayoutTests/fast/js/script-tests/dfg-check-function-change-structure.js (0 => 149016)
--- trunk/LayoutTests/fast/js/script-tests/dfg-check-function-change-structure.js (rev 0)
+++ trunk/LayoutTests/fast/js/script-tests/dfg-check-function-change-structure.js 2013-04-24 03:18:04 UTC (rev 149016)
@@ -0,0 +1,21 @@
+function foo(o) {
+ var f = o[0];
+ return [f(), f.f];
+}
+
+function bar() {
+ return 42;
+}
+
+bar.f = 23;
+
+var expected = "[42, 23]";
+
+for (var i = 0; i < 100; ++i) {
+ if (i == 95) {
+ delete bar.f;
+ bar.g = 36;
+ expected = "[42, void 0]";
+ }
+ shouldBe("foo([bar])", expected);
+}
Modified: trunk/Source/_javascript_Core/ChangeLog (149015 => 149016)
--- trunk/Source/_javascript_Core/ChangeLog 2013-04-24 01:59:21 UTC (rev 149015)
+++ trunk/Source/_javascript_Core/ChangeLog 2013-04-24 03:18:04 UTC (rev 149016)
@@ -1,3 +1,42 @@
+2013-04-23 Filip Pizlo <[email protected]>
+
+ DFG CFA filters CheckFunction in a really weird way, and assumes that the function's structure won't change
+ https://bugs.webkit.org/show_bug.cgi?id=115077
+
+ Reviewed by Oliver Hunt.
+
+ The filtering did three things that are unusual:
+
+ 1) AbstractValue::filterByValue() assumed that the passed value's structure wouldn't change, in
+ the sense that at it assumed it could use that value's *current* structure to do structure
+ filtering. Filtering by structure only makes sense if you can prove that the given value will
+ always have that structure (for example by either using a watchpoing or emitting code that
+ checks that structure at run-time).
+
+ 2) AbstractValue::filterByValue() and the CheckFunction case in AbstractState::executeEffects()
+ tried to invalidate the CFA based on whether the filtration led to an empty value. This is
+ well-intentioned, but it's not how the CFA currently works. It's inconsistent with other
+ parts of the CFA. We shouldn't introduce this feature into just one kind of filtration and
+ not have it elsewhere.
+
+ 3) The attempt to detect when the value was empty was actually implemented incorrectly. It
+ relied on AbstractValue::validate(). That method says that a concrete value does not belong
+ to the abstract value if it has a different structure. This makes sense for the other place
+ where AbstractValue::validate() is called: during OSR entry, where we are talking about a
+ JSValue that we see *right now*. It doesn't make sense in the CFA, since in the CFA any
+ value we observe in the code is a value whose structure may change when the code starts
+ running, and so we cannot use the value's current structure to infer things about the code
+ when it starts running.
+
+ I fixed the above problems by (1) changing filterByValue() to not filter the structure, (2)
+ changing filterByValue() and the CheckFunction case to not invalidate the CFA, and (3)
+ making sure that nobody else was misusing AbstractValue::validate() (they weren't).
+
+ * dfg/DFGAbstractState.cpp:
+ (JSC::DFG::AbstractState::executeEffects):
+ * dfg/DFGAbstractValue.h:
+ (JSC::DFG::AbstractValue::filterByValue):
+
2013-04-23 Oliver Hunt <[email protected]>
Default ParserError() initialiser doesn't initialise all fields
Modified: trunk/Source/_javascript_Core/dfg/DFGAbstractState.cpp (149015 => 149016)
--- trunk/Source/_javascript_Core/dfg/DFGAbstractState.cpp 2013-04-24 01:59:21 UTC (rev 149015)
+++ trunk/Source/_javascript_Core/dfg/DFGAbstractState.cpp 2013-04-24 03:18:04 UTC (rev 149016)
@@ -1458,10 +1458,7 @@
}
node->setCanExit(true); // Lies! We can do better.
- if (!forNode(node->child1()).filterByValue(node->function())) {
- m_isValid = false;
- break;
- }
+ forNode(node->child1()).filterByValue(node->function());
break;
}
Modified: trunk/Source/_javascript_Core/dfg/DFGAbstractValue.h (149015 => 149016)
--- trunk/Source/_javascript_Core/dfg/DFGAbstractValue.h 2013-04-24 01:59:21 UTC (rev 149015)
+++ trunk/Source/_javascript_Core/dfg/DFGAbstractValue.h 2013-04-24 03:18:04 UTC (rev 149016)
@@ -290,19 +290,11 @@
checkConsistency();
}
- bool filterByValue(JSValue value)
+ void filterByValue(JSValue value)
{
- if (!validate(value))
- return false;
-
- if (!!value && value.isCell())
- filter(StructureSet(value.asCell()->structure()));
- else
- filter(speculationFromValue(value));
-
- m_value = value;
-
- return true;
+ filter(speculationFromValue(value));
+ if (m_type)
+ m_value = value;
}
bool validateType(JSValue value) const
_______________________________________________ webkit-changes mailing list [email protected] https://lists.webkit.org/mailman/listinfo/webkit-changes
