Changeset 190215 in webkit

Timestamp:

Sep 24, 2015, 12:23:58 PM (10 years ago)

Author:

fpizlo@apple.com

Message:

PolymorphicAccess should remember that it checked an ObjectPropertyCondition with a check on some structure
​https://bugs.webkit.org/show_bug.cgi?id=149514

Reviewed by Oliver Hunt.

When we checked an ObjectPropertyCondition using an explicit structure check, we would forget to
note the structure in any weak reference table and we would attempt to regenerate the condition
check even if the condition became invalid.

We need to account for this better and we need to prune AccessCases that have an invalid condition
set. This change does both.

• bytecode/PolymorphicAccess.cpp:

(JSC::AccessGenerationState::addWatchpoint):
(JSC::AccessCase::alternateBase):
(JSC::AccessCase::couldStillSucceed):
(JSC::AccessCase::canReplace):
(JSC::AccessCase::generate):
(JSC::PolymorphicAccess::regenerateWithCases):
(JSC::PolymorphicAccess::visitWeak):
(JSC::PolymorphicAccess::regenerate):

• bytecode/PolymorphicAccess.h:

(JSC::AccessCase::callLinkInfo):

• tests/stress/make-dictionary-repatch.js: Added. This used to crash on a release assert. If we removed the release assert, this would return bad results.

Location:

trunk/Source/JavaScriptCore

Files:

• ChangeLog (modified) (1 diff)
• bytecode/PolymorphicAccess.cpp (modified) (8 diffs)
• bytecode/PolymorphicAccess.h (modified) (2 diffs)
• tests/stress/make-dictionary-repatch.js (added)

trunk/Source/JavaScriptCore/ChangeLog

@@ +1 @@
+2015-09-23  Filip Pizlo  <fpizlo@apple.com>
+
+        PolymorphicAccess should remember that it checked an ObjectPropertyCondition with a check on some structure
+        https://bugs.webkit.org/show_bug.cgi?id=149514
+
+        Reviewed by Oliver Hunt.
+
+        When we checked an ObjectPropertyCondition using an explicit structure check, we would forget to
+        note the structure in any weak reference table and we would attempt to regenerate the condition
+        check even if the condition became invalid.
+
+        We need to account for this better and we need to prune AccessCases that have an invalid condition
+        set. This change does both.
+
+        * bytecode/PolymorphicAccess.cpp:
+        (JSC::AccessGenerationState::addWatchpoint):
+        (JSC::AccessCase::alternateBase):
+        (JSC::AccessCase::couldStillSucceed):
+        (JSC::AccessCase::canReplace):
+        (JSC::AccessCase::generate):
+        (JSC::PolymorphicAccess::regenerateWithCases):
+        (JSC::PolymorphicAccess::visitWeak):
+        (JSC::PolymorphicAccess::regenerate):
+        * bytecode/PolymorphicAccess.h:
+        (JSC::AccessCase::callLinkInfo):
+        * tests/stress/make-dictionary-repatch.js: Added. This used to crash on a release assert. If we removed the release assert, this would return bad results.
+
 2015-09-24  Mark Lam  <mark.lam@apple.com>
 

trunk/Source/JavaScriptCore/bytecode/PolymorphicAccess.cpp

@@ -62 +62 @@
     const Identifier* ident;
     std::unique_ptr<WatchpointsOnStructureStubInfo> watchpoints;
+    Vector<WriteBarrier<JSCell>> weakReferences;
 
     Watchpoint* addWatchpoint(const ObjectPropertyCondition& condition = ObjectPropertyCondition())
@@ -252 +253 @@
 }
 
+bool AccessCase::couldStillSucceed() const
+{
+    return m_conditionSet.structuresEnsureValidityAssumingImpurePropertyWatchpoint();
+}
+
 bool AccessCase::canReplace(const AccessCase& other)
 {
@@ -376 +382 @@
     CCallHelpers& jit = *state.jit;
     VM& vm = *jit.vm();
+    CodeBlock* codeBlock = jit.codeBlock();
     StructureStubInfo& stubInfo = *state.stubInfo;
     const Identifier& ident = *state.ident;
@@ -399 +406 @@
         }
 
-        RELEASE_ASSERT(condition.structureEnsuresValidityAssumingImpurePropertyWatchpoint(structure));
+        if (!condition.structureEnsuresValidityAssumingImpurePropertyWatchpoint(structure)) {
+            dataLog("This condition is no longer met: ", condition, "\n");
+            RELEASE_ASSERT_NOT_REACHED();
+        }
+
+        // We will emit code that has a weak reference that isn't otherwise listed anywhere.
+        state.weakReferences.append(WriteBarrier<JSCell>(vm, codeBlock->ownerExecutable(), structure));
+        
         jit.move(CCallHelpers::TrustedImmPtr(condition.object()), scratchGPR);
         state.failAndRepatch.append(
@@ -978 +992 @@
     ListType newCases;
     for (auto& oldCase : m_list) {
+        // Ignore old cases that cannot possibly succeed anymore.
+        if (!oldCase->couldStillSucceed())
+            continue;
+
+        // Figure out if this is replaced by any new cases.
         bool found = false;
         for (auto& caseToAdd : casesToAdd) {
@@ -985 +1004 @@
             }
         }
-        if (!found)
-            newCases.append(oldCase->clone());
+        if (found)
+            continue;
+        
+        newCases.append(oldCase->clone());
     }
     for (auto& caseToAdd : casesToAdd)
@@ -1022 +1043 @@
         if (!at(i).visitWeak(vm))
             return false;
+    }
+    if (Vector<WriteBarrier<JSCell>>* weakReferences = m_weakReferences.get()) {
+        for (WriteBarrier<JSCell>& weakReference : *weakReferences) {
+            if (!Heap::isMarked(weakReference.get()))
+                return false;
+        }
     }
     return true;
@@ -1146 +1173 @@
     m_stubRoutine = createJITStubRoutine(code, vm, codeBlock->ownerExecutable(), doesCalls);
     m_watchpoints = WTF::move(state.watchpoints);
+    if (!state.weakReferences.isEmpty())
+        m_weakReferences = std::make_unique<Vector<WriteBarrier<JSCell>>>(WTF::move(state.weakReferences));
     if (verbose)
         dataLog("Returning: ", code.code(), "\n");

trunk/Source/JavaScriptCore/bytecode/PolymorphicAccess.h

@@ -207 +207 @@
     }
 
+    // Is it still possible for this case to ever be taken?
+    bool couldStillSucceed() const;
+
     // If this method returns true, then it's a good idea to remove 'other' from the access once 'this'
     // is added. This method assumes that in case of contradictions, 'this' represents a newer, and so
@@ -302 +305 @@
     RefPtr<JITStubRoutine> m_stubRoutine;
     std::unique_ptr<WatchpointsOnStructureStubInfo> m_watchpoints;
+    std::unique_ptr<Vector<WriteBarrier<JSCell>>> m_weakReferences;
 };