Skip to content

Commit 856c771

Browse files
saambaratisaambarati
committed
mergeOSREntryValue is wrong when the incoming value does not match up with the flush format
https://bugs.webkit.org/show_bug.cgi?id=196918 Reviewed by Yusuke Suzuki. r244238 lead to some debug failures because we were calling checkConsistency() before doing fixTypeForRepresentation when merging in must handle values in CFA. This patch fixes that. However, as I was reading over mergeOSREntryValue, I realized it was wrong. It was possible it could merge in a value/type outside of the variable's flushed type. Once the flush format types are locked in, we can't introduce a type out of that range. This probably never lead to any crashes as our profiling injection and speculation decision code is solid. However, what we were doing is clearly wrong, and something a fuzzer could have found if we fuzzed the must handle values inside prediction injection. We should do that fuzzing: https://bugs.webkit.org/show_bug.cgi?id=196924 * dfg/DFGAbstractValue.cpp: (JSC::DFG::AbstractValue::mergeOSREntryValue): * dfg/DFGAbstractValue.h: * dfg/DFGCFAPhase.cpp: (JSC::DFG::CFAPhase::injectOSR): Canonical link: https://commits.webkit.org/211188@main git-svn-id: https://svn.webkit.org/repository/webkit/trunk@244287 268f45cc-cd09-0410-ab3c-d52691b4dbfc
1 parent d4a85d4 commit 856c771

4 files changed

Lines changed: 45 additions & 6 deletions

File tree

‎Source/JavaScriptCore/ChangeLog‎

Lines changed: 26 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,29 @@
1+
2019-04-15 Saam barati <sbarati@apple.com>
2+
3+
mergeOSREntryValue is wrong when the incoming value does not match up with the flush format
4+
https://bugs.webkit.org/show_bug.cgi?id=196918
5+
6+
Reviewed by Yusuke Suzuki.
7+
8+
r244238 lead to some debug failures because we were calling checkConsistency()
9+
before doing fixTypeForRepresentation when merging in must handle values in
10+
CFA. This patch fixes that.
11+
12+
However, as I was reading over mergeOSREntryValue, I realized it was wrong. It
13+
was possible it could merge in a value/type outside of the variable's flushed type.
14+
Once the flush format types are locked in, we can't introduce a type out of
15+
that range. This probably never lead to any crashes as our profiling injection
16+
and speculation decision code is solid. However, what we were doing is clearly
17+
wrong, and something a fuzzer could have found if we fuzzed the must handle
18+
values inside prediction injection. We should do that fuzzing:
19+
https://bugs.webkit.org/show_bug.cgi?id=196924
20+
21+
* dfg/DFGAbstractValue.cpp:
22+
(JSC::DFG::AbstractValue::mergeOSREntryValue):
23+
* dfg/DFGAbstractValue.h:
24+
* dfg/DFGCFAPhase.cpp:
25+
(JSC::DFG::CFAPhase::injectOSR):
26+
127
2019-04-15 Robin Morisset <rmorisset@apple.com>
228

329
Several structures and enums in the Yarr interpreter can be shrunk

‎Source/JavaScriptCore/dfg/DFGAbstractValue.cpp‎

Lines changed: 16 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -180,8 +180,19 @@ void AbstractValue::fixTypeForRepresentation(Graph& graph, Node* node)
180180
fixTypeForRepresentation(graph, node->result(), node);
181181
}
182182

183-
bool AbstractValue::mergeOSREntryValue(Graph& graph, JSValue value)
183+
bool AbstractValue::mergeOSREntryValue(Graph& graph, JSValue value, VariableAccessData* variable, Node* node)
184184
{
185+
FlushFormat flushFormat = variable->flushFormat();
186+
187+
{
188+
if (flushFormat == FlushedDouble && value.isNumber())
189+
value = jsDoubleNumber(value.asNumber());
190+
SpeculatedType incomingType = resultFor(flushFormat) == NodeResultInt52 ? int52AwareSpeculationFromValue(value) : speculationFromValue(value);
191+
SpeculatedType requiredType = typeFilterFor(flushFormat);
192+
if (incomingType & ~requiredType)
193+
return false;
194+
}
195+
185196
AbstractValue oldMe = *this;
186197

187198
if (isClear()) {
@@ -207,8 +218,11 @@ bool AbstractValue::mergeOSREntryValue(Graph& graph, JSValue value)
207218
m_value = JSValue();
208219
}
209220

210-
checkConsistency();
211221
assertIsRegistered(graph);
222+
223+
fixTypeForRepresentation(graph, resultFor(flushFormat), node);
224+
225+
checkConsistency();
212226

213227
return oldMe != *this;
214228
}

‎Source/JavaScriptCore/dfg/DFGAbstractValue.h‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -48,6 +48,7 @@ namespace DFG {
4848

4949
class Graph;
5050
struct Node;
51+
class VariableAccessData;
5152

5253
struct AbstractValue {
5354
AbstractValue()
@@ -303,7 +304,7 @@ struct AbstractValue {
303304
return result;
304305
}
305306

306-
bool mergeOSREntryValue(Graph&, JSValue);
307+
bool mergeOSREntryValue(Graph&, JSValue, VariableAccessData*, Node*);
307308

308309
void merge(SpeculatedType type)
309310
{

‎Source/JavaScriptCore/dfg/DFGCFAPhase.cpp‎

Lines changed: 1 addition & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -188,9 +188,7 @@ class CFAPhase : public Phase {
188188
dataLog(" Widening ", VirtualRegister(operand), " with ", value.value(), "\n");
189189

190190
AbstractValue& target = block->valuesAtHead.operand(operand);
191-
changed |= target.mergeOSREntryValue(m_graph, value.value());
192-
target.fixTypeForRepresentation(
193-
m_graph, resultFor(node->variableAccessData()->flushFormat()), node);
191+
changed |= target.mergeOSREntryValue(m_graph, value.value(), node->variableAccessData(), node);
194192
}
195193

196194
if (changed || !block->cfaHasVisited) {

0 commit comments

Comments
 (0)