DFG::freezeFragile should register the frozen value's structure
authorfpizlo@apple.com <fpizlo@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Thu, 2 Jul 2015 01:30:28 +0000 (01:30 +0000)
committerfpizlo@apple.com <fpizlo@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Thu, 2 Jul 2015 01:30:28 +0000 (01:30 +0000)
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.

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

Source/JavaScriptCore/ChangeLog
Source/JavaScriptCore/dfg/DFGAbstractValue.cpp
Source/JavaScriptCore/dfg/DFGGraph.cpp
Source/JavaScriptCore/dfg/DFGStructureRegistrationPhase.cpp

index 99f306c..bc2a365 100644 (file)
@@ -1,3 +1,29 @@
+2015-06-30  Filip Pizlo  <fpizlo@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  <mmirman@apple.com>
 
         Unreviewed, rolling out r185889
index 21ebdba..7cc48a7 100644 (file)
@@ -51,7 +51,6 @@ void AbstractValue::setOSREntryValue(Graph& graph, const FrozenValue& value)
 {
     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 @@ void AbstractValue::set(Graph& graph, const FrozenValue& value, StructureClobber
 {
     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) {
index 3e5084b..0cc93a5 100644 (file)
@@ -1214,7 +1214,11 @@ FrozenValue* Graph::freezeFragile(JSValue value)
     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 @@ StructureRegistrationResult Graph::registerStructure(Structure* structure)
 
 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;
     
index 28646d1..875e0d3 100644 (file)
@@ -44,6 +44,11 @@ public:
     
     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 @@ public:
         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 @@ public:
             }
         }
         
-        m_graph.m_structureRegistrationState = AllStructuresAreRegistered;
-        
         return true;
     }