Changeset 153208 in webkit for trunk/Source/JavaScriptCore/dfg/DFGAbstractValue.h

Timestamp:

Jul 24, 2013, 9:02:00 PM (12 years ago)

Author:

[email protected]

Message:

fourthTier: Clean up AbstractValue
​https://bugs.webkit.org/show_bug.cgi?id=117217

Reviewed by Oliver Hunt.

This started as an attempt to make it so that when AbstractValue becomes empty,
its m_type always becomes SpecNone. I wanted this to happen naturally. That turns
out to be basically impossible, since AbstractValue is a set that is dynamically
computed from the intersection of several internal sets: so the value becomes
empty when any of the sets go empty. It's OK if we're imprecise here because it's
always safe for the AbstractValue to seem to overapproximate the set of values
that we see. So I mostly gave up on cleaning up that aspect of AbstractValue. But
while trying to make this happen, I encountered two bugs:

• filterValueByType() ignores the case when m_type contravenes m_value. Namely, we might filter the AbstractValue against a SpeculatedType leading to m_value becoming inconsistent with the new m_type. This change fixes that case. This wasn't a symptomatic bug but it was a silly oversight.

• filterFuturePossibleStructure() was never right. The one call to this method, in filter(Graph&, const StructureSet&), assumed that the previous notions of what structures the value could have in the future were still relevant. This could lead to a bug where we:

1) CheckStructure(@foo, S1)

Where S1 has a valid watchpoint. Now @foo's abstract value will have current
and future structure = S1.

2) Clobber the world.

Now @foo's abstract value will have current structure = TOP, and future
possible structure = S1.

3) CheckStructure(@foo, S2)

Now @foo's abstract value will have current structure = S2 and future
possible structure = S1 intersect S2 = BOTTOM.

Now we will think that any subsequent watchpoint on @foo is valid because the
value is effectively BOTTOM. That would only be correct if we had actually set
a watchpoint on S1. If we had done so, then (3) would only pass (i.e. @foo
would only have structure S2) if S1's watchpoint fired, in which case (3)
wouldn't have been reachable. But we didn't actually set a watchpoint on S1:
we just observed that we *could* have set the watchpoint. Hence future possible
structure should only be set to either the known structure at compile-time, or
it should be the structure we just checked; in both cases it should only be set
if the structure is watchable.

Then, in addition to all of this, I changed AbstractValue's filtering methods to
call clear() if the AbstractValue is effectively clear. This is just meant to
simplify the recognition of truly empty AbstractValues, but doesn't actually have
any other implications.

• bytecode/StructureSet.h:

(JSC::StructureSet::dump):

• dfg/DFGAbstractValue.cpp:

(JSC::DFG::AbstractValue::filter):
(DFG):
(JSC::DFG::AbstractValue::filterArrayModes):
(JSC::DFG::AbstractValue::filterValueByType):
(JSC::DFG::AbstractValue::filterArrayModesByType):
(JSC::DFG::AbstractValue::shouldBeClear):
(JSC::DFG::AbstractValue::normalizeClarity):
(JSC::DFG::AbstractValue::checkConsistency):

• dfg/DFGAbstractValue.h:

(JSC::DFG::AbstractValue::isClear):
(AbstractValue):

File:

• trunk/Source/JavaScriptCore/dfg/DFGAbstractValue.h (modified) (5 diffs)

trunk/Source/JavaScriptCore/dfg/DFGAbstractValue.h

@@ -58 +58 @@
     }
     
-    bool isClear() const
-    {
-        bool result = m_type == SpecNone && !m_arrayModes && m_currentKnownStructure.isClear() && m_futurePossibleStructure.isClear();
-        if (result)
-            ASSERT(!m_value);
-        return result;
-    }
+    bool isClear() const { return m_type == SpecNone; }
     
     void makeTop()
@@ -191 +185 @@
     void filter(Graph&, const StructureSet&);
     
-    void filterArrayModes(ArrayModes arrayModes)
-    {
-        ASSERT(arrayModes);
-        
-        m_type &= SpecCell;
-        m_arrayModes &= arrayModes;
-        
-        // I could do more fancy filtering here. But it probably won't make any difference.
-        
-        checkConsistency();
-    }
-    
-    void filter(SpeculatedType type)
-    {
-        if (type == SpecTop)
-            return;
-        m_type &= type;
-        
-        // It's possible that prior to this filter() call we had, say, (Final, TOP), and
-        // the passed type is Array. At this point we'll have (None, TOP). The best way
-        // to ensure that the structure filtering does the right thing is to filter on
-        // the new type (None) rather than the one passed (Array).
-        m_currentKnownStructure.filter(m_type);
-        m_futurePossibleStructure.filter(m_type);
-        
-        filterArrayModesByType();
-        filterValueByType();
-        
-        checkConsistency();
-    }
+    void filterArrayModes(ArrayModes arrayModes);
+    
+    void filter(SpeculatedType type);
     
     void filterByValue(JSValue value)
@@ -281 +248 @@
     }
     
-    void checkConsistency() const
-    {
-        if (!(m_type & SpecCell)) {
-            ASSERT(m_currentKnownStructure.isClear());
-            ASSERT(m_futurePossibleStructure.isClear());
-            ASSERT(!m_arrayModes);
-        }
-        
-        if (isClear())
-            ASSERT(!m_value);
-        
-        if (!!m_value)
-            ASSERT(mergeSpeculations(m_type, speculationFromValue(m_value)) == m_type);
-        
-        // Note that it's possible for a prediction like (Final, []). This really means that
-        // the value is bottom and that any code that uses the value is unreachable. But
-        // we don't want to get pedantic about this as it would only increase the computational
-        // complexity of the code.
-    }
+    void checkConsistency() const;
     
     void dump(PrintStream&) const;
@@ -315 +264 @@
     //
     //    Where x will later have a new property added to it, 'g'. Because of the
-    //    known but not-yet-executed property addition, x's currently structure will
+    //    known but not-yet-executed property addition, x's current structure will
     //    not be watchpointable; hence we have no way of statically bounding the set
     //    of possible structures that x may have if a clobbering event happens. So,
@@ -411 +360 @@
     
     void setFuturePossibleStructure(Graph&, Structure* structure);
-    void filterFuturePossibleStructure(Graph&, Structure* structure);
-
-    // We could go further, and ensure that if the futurePossibleStructure contravenes
-    // the value, then we could clear both of those things. But that's unlikely to help
-    // in any realistic scenario, so we don't do it. Simpler is better.
-    void filterValueByType()
-    {
-        if (!!m_type) {
-            // The type is still non-empty. This implies that regardless of what filtering
-            // was done, we either didn't have a value to begin with, or that value is still
-            // valid.
-            ASSERT(!m_value || validateType(m_value));
-            return;
-        }
-        
-        // The type has been rendered empty. That means that the value must now be invalid,
-        // as well.
-        ASSERT(!m_value || !validateType(m_value));
-        m_value = JSValue();
-    }
-    
-    void filterArrayModesByType()
-    {
-        if (!(m_type & SpecCell))
-            m_arrayModes = 0;
-        else if (!(m_type & ~SpecArray))
-            m_arrayModes &= ALL_ARRAY_ARRAY_MODES;
-
-        // NOTE: If m_type doesn't have SpecArray set, that doesn't mean that the
-        // array modes have to be a subset of ALL_NON_ARRAY_ARRAY_MODES, since
-        // in the speculated type type-system, RegExpMatchesArry and ArrayPrototype
-        // are Otherobj (since they are not *exactly* JSArray) but in the ArrayModes
-        // type system they are arrays (since they expose the magical length
-        // property and are otherwise allocated using array allocation). Hence the
-        // following would be wrong:
-        //
-        // if (!(m_type & SpecArray))
-        //    m_arrayModes &= ALL_NON_ARRAY_ARRAY_MODES;
-    }
+
+    void filterValueByType();
+    void filterArrayModesByType();
+    
+    bool shouldBeClear() const;
+    void normalizeClarity();
 };