Title: [186215] trunk/Source/_javascript_Core
- Revision
- 186215
- Author
- fpi...@apple.com
- Date
- 2015-07-01 18:30:28 -0700 (Wed, 01 Jul 2015)
Log Message
DFG::freezeFragile should register the frozen value's structure
https://bugs.webkit.org/show_bug.cgi?id=136055
rdar://problem/21042120
Reviewed by Mark Lam and Geoffrey Garen.
This fixes weird concurrency bugs where the constant folding phase tries to convert
something to a constant but then crashes because the constant's structure wasn't
registered. The AI was registering the structure of any value it saw, but constant folding
wasn't - and that's fine so long as there ain't no concurrency.
The best fix is to just make it impossible to introduce a constant into the IR without
registering its structure. That's what this change does. This is not only a great
concurrency fix - it also makes the compiler somewhat easier to hack on because it's one
less case of structure registering that you have to remember about.
* dfg/DFGAbstractValue.cpp:
(JSC::DFG::AbstractValue::setOSREntryValue): No need to register.
(JSC::DFG::AbstractValue::set): We still call register, but just to get the watchpoint state.
* dfg/DFGGraph.cpp:
(JSC::DFG::Graph::freezeFragile): Register the structure.
* dfg/DFGStructureRegistrationPhase.cpp:
(JSC::DFG::StructureRegistrationPhase::run): Assert that these are all registered.
Modified Paths
Diff
Modified: trunk/Source/_javascript_Core/ChangeLog (186214 => 186215)
--- trunk/Source/_javascript_Core/ChangeLog 2015-07-02 00:51:15 UTC (rev 186214)
+++ trunk/Source/_javascript_Core/ChangeLog 2015-07-02 01:30:28 UTC (rev 186215)
@@ -1,3 +1,29 @@
+2015-06-30 Filip Pizlo <fpi...@apple.com>
+
+ DFG::freezeFragile should register the frozen value's structure
+ https://bugs.webkit.org/show_bug.cgi?id=136055
+ rdar://problem/21042120
+
+ Reviewed by Mark Lam and Geoffrey Garen.
+
+ This fixes weird concurrency bugs where the constant folding phase tries to convert
+ something to a constant but then crashes because the constant's structure wasn't
+ registered. The AI was registering the structure of any value it saw, but constant folding
+ wasn't - and that's fine so long as there ain't no concurrency.
+
+ The best fix is to just make it impossible to introduce a constant into the IR without
+ registering its structure. That's what this change does. This is not only a great
+ concurrency fix - it also makes the compiler somewhat easier to hack on because it's one
+ less case of structure registering that you have to remember about.
+
+ * dfg/DFGAbstractValue.cpp:
+ (JSC::DFG::AbstractValue::setOSREntryValue): No need to register.
+ (JSC::DFG::AbstractValue::set): We still call register, but just to get the watchpoint state.
+ * dfg/DFGGraph.cpp:
+ (JSC::DFG::Graph::freezeFragile): Register the structure.
+ * dfg/DFGStructureRegistrationPhase.cpp:
+ (JSC::DFG::StructureRegistrationPhase::run): Assert that these are all registered.
+
2015-07-01 Matthew Mirman <mmir...@apple.com>
Unreviewed, rolling out r185889
Modified: trunk/Source/_javascript_Core/dfg/DFGAbstractValue.cpp (186214 => 186215)
--- trunk/Source/_javascript_Core/dfg/DFGAbstractValue.cpp 2015-07-02 00:51:15 UTC (rev 186214)
+++ trunk/Source/_javascript_Core/dfg/DFGAbstractValue.cpp 2015-07-02 01:30:28 UTC (rev 186215)
@@ -51,7 +51,6 @@
{
if (!!value && value.value().isCell()) {
Structure* structure = value.structure();
- graph.registerStructure(structure);
m_structure = structure;
m_arrayModes = asArrayModes(structure->indexingType());
} else {
@@ -70,9 +69,6 @@
{
if (!!value && value.value().isCell()) {
Structure* structure = value.structure();
- // FIXME: This check may not be necessary since any frozen value should have its structure
- // watched already.
- // https://bugs.webkit.org/show_bug.cgi?id=136055
if (graph.registerStructure(structure) == StructureRegisteredAndWatched) {
m_structure = structure;
if (clobberState == StructuresAreClobbered) {
Modified: trunk/Source/_javascript_Core/dfg/DFGGraph.cpp (186214 => 186215)
--- trunk/Source/_javascript_Core/dfg/DFGGraph.cpp 2015-07-02 00:51:15 UTC (rev 186214)
+++ trunk/Source/_javascript_Core/dfg/DFGGraph.cpp 2015-07-02 01:30:28 UTC (rev 186215)
@@ -1214,7 +1214,11 @@
if (value.isUInt32())
m_uint32ValuesInUse.append(value.asUInt32());
- return result.iterator->value = m_frozenValues.add(FrozenValue::freeze(value));
+ FrozenValue frozenValue = FrozenValue::freeze(value);
+ if (Structure* structure = frozenValue.structure())
+ registerStructure(structure);
+
+ return result.iterator->value = m_frozenValues.add(frozenValue);
}
FrozenValue* Graph::freeze(JSValue value)
@@ -1258,6 +1262,10 @@
void Graph::assertIsRegistered(Structure* structure)
{
+ // It's convenient to be able to call this with a maybe-null structure.
+ if (!structure)
+ return;
+
if (m_structureRegistrationState == HaveNotStartedRegistering)
return;
Modified: trunk/Source/_javascript_Core/dfg/DFGStructureRegistrationPhase.cpp (186214 => 186215)
--- trunk/Source/_javascript_Core/dfg/DFGStructureRegistrationPhase.cpp 2015-07-02 00:51:15 UTC (rev 186214)
+++ trunk/Source/_javascript_Core/dfg/DFGStructureRegistrationPhase.cpp 2015-07-02 01:30:28 UTC (rev 186215)
@@ -44,6 +44,11 @@
bool run()
{
+ // We need to set this before this phase finishes. This phase doesn't do anything
+ // conditioned on this field, except for assertIsRegistered() below. We intend for that
+ // method to behave as if the phase was already finished. So, we set this up here.
+ m_graph.m_structureRegistrationState = AllStructuresAreRegistered;
+
// These are pretty dumb, but needed to placate subsequent assertions. We don't actually
// have to watch these because there is no way to transition away from it, but they are
// watchable and so we will assert if they aren't watched.
@@ -52,7 +57,7 @@
registerStructure(m_graph.m_vm.getterSetterStructure.get());
for (FrozenValue* value : m_graph.m_frozenValues)
- registerStructure(value->structure());
+ m_graph.assertIsRegistered(value->structure());
for (BlockIndex blockIndex = m_graph.numBlocks(); blockIndex--;) {
BasicBlock* block = m_graph.block(blockIndex);
@@ -141,8 +146,6 @@
}
}
- m_graph.m_structureRegistrationState = AllStructuresAreRegistered;
-
return true;
}
_______________________________________________
webkit-changes mailing list
webkit-changes@lists.webkit.org
https://lists.webkit.org/mailman/listinfo/webkit-changes
