Debug JSC test failure: stress/multi-put-by-offset-reallocation-butterfly-cse.js...
authorfpizlo@apple.com <fpizlo@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Sat, 9 Apr 2016 20:41:04 +0000 (20:41 +0000)
committerfpizlo@apple.com <fpizlo@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Sat, 9 Apr 2016 20:41:04 +0000 (20:41 +0000)
https://bugs.webkit.org/show_bug.cgi?id=156406

Reviewed by Saam Barati.

The failure was because the GC ran from within the butterfly allocation call in a put_by_id
transition AccessCase that had to deal with indexing storage. When the GC runs in a call from a stub,
then we need to be extra careful:

1) The GC may reset the IC and delete the stub. So, the stub needs to tell the GC that it might be on
   the stack during GC, so that the GC keeps it alive if it's currently running.

2) If the stub uses (dereferences or stores) some object after the call, then we need to ensure that
   the stub routine knows about that object independently of the IC.

In the case of put_by_id transitions that use a helper to allocate the butterfly, we have both
issues. A long time ago, we had to deal with (2), and we still had code to handle that case, although
it appears to be dead. This change revives that code and glues it together with PolymorphicAccess.

* bytecode/PolymorphicAccess.cpp:
(JSC::AccessCase::alternateBase):
(JSC::AccessCase::doesCalls):
(JSC::AccessCase::couldStillSucceed):
(JSC::AccessCase::generate):
(JSC::PolymorphicAccess::regenerate):
* bytecode/PolymorphicAccess.h:
(JSC::AccessCase::customSlotBase):
(JSC::AccessCase::isGetter):
(JSC::AccessCase::doesCalls): Deleted.
* jit/GCAwareJITStubRoutine.cpp:
(JSC::GCAwareJITStubRoutine::markRequiredObjectsInternal):
(JSC::MarkingGCAwareJITStubRoutine::MarkingGCAwareJITStubRoutine):
(JSC::MarkingGCAwareJITStubRoutine::~MarkingGCAwareJITStubRoutine):
(JSC::MarkingGCAwareJITStubRoutine::markRequiredObjectsInternal):
(JSC::GCAwareJITStubRoutineWithExceptionHandler::GCAwareJITStubRoutineWithExceptionHandler):
(JSC::createJITStubRoutine):
(JSC::MarkingGCAwareJITStubRoutineWithOneObject::MarkingGCAwareJITStubRoutineWithOneObject): Deleted.
(JSC::MarkingGCAwareJITStubRoutineWithOneObject::~MarkingGCAwareJITStubRoutineWithOneObject): Deleted.
(JSC::MarkingGCAwareJITStubRoutineWithOneObject::markRequiredObjectsInternal): Deleted.
* jit/GCAwareJITStubRoutine.h:
(JSC::createJITStubRoutine):

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

Source/JavaScriptCore/ChangeLog
Source/JavaScriptCore/bytecode/PolymorphicAccess.cpp
Source/JavaScriptCore/bytecode/PolymorphicAccess.h
Source/JavaScriptCore/jit/GCAwareJITStubRoutine.cpp
Source/JavaScriptCore/jit/GCAwareJITStubRoutine.h

index 2f27f32..3bec5c0 100644 (file)
@@ -1,3 +1,47 @@
+2016-04-09  Filip Pizlo  <fpizlo@apple.com>
+
+        Debug JSC test failure: stress/multi-put-by-offset-reallocation-butterfly-cse.js.ftl-no-cjit-small-pool
+        https://bugs.webkit.org/show_bug.cgi?id=156406
+
+        Reviewed by Saam Barati.
+
+        The failure was because the GC ran from within the butterfly allocation call in a put_by_id
+        transition AccessCase that had to deal with indexing storage. When the GC runs in a call from a stub,
+        then we need to be extra careful:
+
+        1) The GC may reset the IC and delete the stub. So, the stub needs to tell the GC that it might be on
+           the stack during GC, so that the GC keeps it alive if it's currently running.
+        
+        2) If the stub uses (dereferences or stores) some object after the call, then we need to ensure that
+           the stub routine knows about that object independently of the IC.
+        
+        In the case of put_by_id transitions that use a helper to allocate the butterfly, we have both
+        issues. A long time ago, we had to deal with (2), and we still had code to handle that case, although
+        it appears to be dead. This change revives that code and glues it together with PolymorphicAccess.
+
+        * bytecode/PolymorphicAccess.cpp:
+        (JSC::AccessCase::alternateBase):
+        (JSC::AccessCase::doesCalls):
+        (JSC::AccessCase::couldStillSucceed):
+        (JSC::AccessCase::generate):
+        (JSC::PolymorphicAccess::regenerate):
+        * bytecode/PolymorphicAccess.h:
+        (JSC::AccessCase::customSlotBase):
+        (JSC::AccessCase::isGetter):
+        (JSC::AccessCase::doesCalls): Deleted.
+        * jit/GCAwareJITStubRoutine.cpp:
+        (JSC::GCAwareJITStubRoutine::markRequiredObjectsInternal):
+        (JSC::MarkingGCAwareJITStubRoutine::MarkingGCAwareJITStubRoutine):
+        (JSC::MarkingGCAwareJITStubRoutine::~MarkingGCAwareJITStubRoutine):
+        (JSC::MarkingGCAwareJITStubRoutine::markRequiredObjectsInternal):
+        (JSC::GCAwareJITStubRoutineWithExceptionHandler::GCAwareJITStubRoutineWithExceptionHandler):
+        (JSC::createJITStubRoutine):
+        (JSC::MarkingGCAwareJITStubRoutineWithOneObject::MarkingGCAwareJITStubRoutineWithOneObject): Deleted.
+        (JSC::MarkingGCAwareJITStubRoutineWithOneObject::~MarkingGCAwareJITStubRoutineWithOneObject): Deleted.
+        (JSC::MarkingGCAwareJITStubRoutineWithOneObject::markRequiredObjectsInternal): Deleted.
+        * jit/GCAwareJITStubRoutine.h:
+        (JSC::createJITStubRoutine):
+
 2016-04-08  Joseph Pecoraro  <pecoraro@apple.com>
 
         Web Inspector: XHRs and Web Worker scripts are not searchable
index 08bc101..ae9885c 100644 (file)
@@ -413,6 +413,29 @@ JSObject* AccessCase::alternateBase() const
     return conditionSet().slotBaseCondition().object();
 }
 
+bool AccessCase::doesCalls(Vector<JSCell*>* cellsToMark) const
+{
+    switch (type()) {
+    case Getter:
+    case Setter:
+    case CustomValueGetter:
+    case CustomAccessorGetter:
+    case CustomValueSetter:
+    case CustomAccessorSetter:
+        return true;
+    case Transition:
+        if (newStructure()->outOfLineCapacity() != structure()->outOfLineCapacity()
+            && structure()->couldHaveIndexingHeader()) {
+            if (cellsToMark)
+                cellsToMark->append(newStructure());
+            return true;
+        }
+        return false;
+    default:
+        return false;
+    }
+}
+
 bool AccessCase::couldStillSucceed() const
 {
     return m_conditionSet.structuresEnsureValidityAssumingImpurePropertyWatchpoint();
@@ -1093,6 +1116,8 @@ void AccessCase::generate(AccessGenerationState& state)
         } else if (verbose)
             dataLog("Don't have type.\n");
         
+        // NOTE: This logic is duplicated in AccessCase::doesCalls(). It's important that doesCalls() knows
+        // exactly when this would make calls.
         bool allocating = newStructure()->outOfLineCapacity() != structure()->outOfLineCapacity();
         bool reallocating = allocating && structure()->outOfLineCapacity();
         bool allocatingInline = allocating && !structure()->couldHaveIndexingHeader();
@@ -1632,10 +1657,11 @@ MacroAssemblerCodePtr PolymorphicAccess::regenerate(
         ("%s", toCString("Access stub for ", *codeBlock, " ", stubInfo.codeOrigin, " with return point ", successLabel, ": ", listDump(cases)).data()));
 
     bool doesCalls = false;
+    Vector<JSCell*> cellsToMark;
     for (auto& entry : cases)
-        doesCalls |= entry->doesCalls();
+        doesCalls |= entry->doesCalls(&cellsToMark);
     
-    m_stubRoutine = createJITStubRoutine(code, vm, codeBlock, doesCalls, nullptr, codeBlockThatOwnsExceptionHandlers, callSiteIndexForExceptionHandling);
+    m_stubRoutine = createJITStubRoutine(code, vm, codeBlock, doesCalls, cellsToMark, codeBlockThatOwnsExceptionHandlers, callSiteIndexForExceptionHandling);
     m_watchpoints = WTFMove(state.watchpoints);
     if (!state.weakReferences.isEmpty())
         m_weakReferences = std::make_unique<Vector<WriteBarrier<JSCell>>>(WTFMove(state.weakReferences));
index 3229799..0eaa75c 100644 (file)
@@ -153,21 +153,10 @@ public:
     }
 
     JSObject* alternateBase() const;
-    
-    bool doesCalls() const
-    {
-        switch (type()) {
-        case Getter:
-        case Setter:
-        case CustomValueGetter:
-        case CustomAccessorGetter:
-        case CustomValueSetter:
-        case CustomAccessorSetter:
-            return true;
-        default:
-            return false;
-        }
-    }
+
+    // If you supply the optional vector, this will append the set of cells that this will need to keep alive
+    // past the call.
+    bool doesCalls(Vector<JSCell*>* cellsToMark = nullptr) const;
 
     bool isGetter() const
     {
index 60c0c55..dece24f 100644 (file)
@@ -1,5 +1,5 @@
 /*
- * Copyright (C) 2012 Apple Inc. All rights reserved.
+ * Copyright (C) 2012, 2016 Apple Inc. All rights reserved.
  *
  * Redistribution and use in source and binary forms, with or without
  * modification, are permitted provided that the following conditions
@@ -79,28 +79,31 @@ void GCAwareJITStubRoutine::markRequiredObjectsInternal(SlotVisitor&)
 {
 }
 
-MarkingGCAwareJITStubRoutineWithOneObject::MarkingGCAwareJITStubRoutineWithOneObject(
+MarkingGCAwareJITStubRoutine::MarkingGCAwareJITStubRoutine(
     const MacroAssemblerCodeRef& code, VM& vm, const JSCell* owner,
-    JSCell* object)
+    const Vector<JSCell*>& cells)
     : GCAwareJITStubRoutine(code, vm)
-    , m_object(vm, owner, object)
+    , m_cells(cells.size())
 {
+    for (unsigned i = cells.size(); i--;)
+        m_cells[i].set(vm, owner, cells[i]);
 }
 
-MarkingGCAwareJITStubRoutineWithOneObject::~MarkingGCAwareJITStubRoutineWithOneObject()
+MarkingGCAwareJITStubRoutine::~MarkingGCAwareJITStubRoutine()
 {
 }
 
-void MarkingGCAwareJITStubRoutineWithOneObject::markRequiredObjectsInternal(SlotVisitor& visitor)
+void MarkingGCAwareJITStubRoutine::markRequiredObjectsInternal(SlotVisitor& visitor)
 {
-    visitor.append(&m_object);
+    for (auto& entry : m_cells)
+        visitor.append(&entry);
 }
 
 
 GCAwareJITStubRoutineWithExceptionHandler::GCAwareJITStubRoutineWithExceptionHandler(
-    const MacroAssemblerCodeRef& code, VM& vm, 
+    const MacroAssemblerCodeRef& code, VM& vm,  const JSCell* owner, const Vector<JSCell*>& cells,
     CodeBlock* codeBlockForExceptionHandlers, CallSiteIndex exceptionHandlerCallSiteIndex)
-    : GCAwareJITStubRoutine(code, vm)
+    : MarkingGCAwareJITStubRoutine(code, vm, owner, cells)
     , m_codeBlockWithExceptionHandler(codeBlockForExceptionHandlers)
     , m_exceptionHandlerCallSiteIndex(exceptionHandlerCallSiteIndex)
 {
@@ -132,7 +135,7 @@ PassRefPtr<JITStubRoutine> createJITStubRoutine(
     VM& vm,
     const JSCell* owner,
     bool makesCalls,
-    JSCell* object,
+    const Vector<JSCell*>& cells,
     CodeBlock* codeBlockForExceptionHandlers,
     CallSiteIndex exceptionHandlerCallSiteIndex)
 {
@@ -140,19 +143,18 @@ PassRefPtr<JITStubRoutine> createJITStubRoutine(
         return adoptRef(new JITStubRoutine(code));
     
     if (codeBlockForExceptionHandlers) {
-        RELEASE_ASSERT(!object); // We're not a marking stub routine.
         RELEASE_ASSERT(JITCode::isOptimizingJIT(codeBlockForExceptionHandlers->jitType()));
         return static_pointer_cast<JITStubRoutine>(
-            adoptRef(new GCAwareJITStubRoutineWithExceptionHandler(code, vm, codeBlockForExceptionHandlers, exceptionHandlerCallSiteIndex)));
+            adoptRef(new GCAwareJITStubRoutineWithExceptionHandler(code, vm, owner, cells, codeBlockForExceptionHandlers, exceptionHandlerCallSiteIndex)));
     }
 
-    if (!object) {
+    if (cells.isEmpty()) {
         return static_pointer_cast<JITStubRoutine>(
             adoptRef(new GCAwareJITStubRoutine(code, vm)));
     }
     
     return static_pointer_cast<JITStubRoutine>(
-        adoptRef(new MarkingGCAwareJITStubRoutineWithOneObject(code, vm, owner, object)));
+        adoptRef(new MarkingGCAwareJITStubRoutine(code, vm, owner, cells)));
 }
 
 } // namespace JSC
index c84a168..8da3e82 100644 (file)
@@ -1,5 +1,5 @@
 /*
- * Copyright (C) 2012, 2014 Apple Inc. All rights reserved.
+ * Copyright (C) 2012, 2014, 2016 Apple Inc. All rights reserved.
  *
  * Redistribution and use in source and binary forms, with or without
  * modification, are permitted provided that the following conditions
@@ -76,28 +76,28 @@ private:
 
 // Use this if you want to mark one additional object during GC if your stub
 // routine is known to be executing.
-class MarkingGCAwareJITStubRoutineWithOneObject : public GCAwareJITStubRoutine {
+class MarkingGCAwareJITStubRoutine : public GCAwareJITStubRoutine {
 public:
-    MarkingGCAwareJITStubRoutineWithOneObject(
-        const MacroAssemblerCodeRef&, VM&, const JSCell* owner, JSCell*);
-    virtual ~MarkingGCAwareJITStubRoutineWithOneObject();
+    MarkingGCAwareJITStubRoutine(
+        const MacroAssemblerCodeRef&, VM&, const JSCell* owner, const Vector<JSCell*>&);
+    virtual ~MarkingGCAwareJITStubRoutine();
     
 protected:
     void markRequiredObjectsInternal(SlotVisitor&) override;
 
 private:
-    WriteBarrier<JSCell> m_object;
+    Vector<WriteBarrier<JSCell>> m_cells;
 };
 
 
 // The stub has exception handlers in it. So it clears itself from exception
 // handling table when it dies. It also frees space in CodeOrigin table
 // for new exception handlers to use the same CallSiteIndex.
-class GCAwareJITStubRoutineWithExceptionHandler : public GCAwareJITStubRoutine {
+class GCAwareJITStubRoutineWithExceptionHandler : public MarkingGCAwareJITStubRoutine {
 public:
     typedef GCAwareJITStubRoutine Base;
 
-    GCAwareJITStubRoutineWithExceptionHandler(const MacroAssemblerCodeRef&, VM&, CodeBlock*, CallSiteIndex);
+    GCAwareJITStubRoutineWithExceptionHandler(const MacroAssemblerCodeRef&, VM&, const JSCell* owner, const Vector<JSCell*>&, CodeBlock*, CallSiteIndex);
 
     void aboutToDie() override;
     void observeZeroRefCount() override;
@@ -128,14 +128,9 @@ private:
 
 PassRefPtr<JITStubRoutine> createJITStubRoutine(
     const MacroAssemblerCodeRef&, VM&, const JSCell* owner, bool makesCalls,
-    JSCell* = nullptr
+    const Vector<JSCell*>& = { }
     CodeBlock* codeBlockForExceptionHandlers = nullptr, CallSiteIndex exceptionHandlingCallSiteIndex = CallSiteIndex(std::numeric_limits<unsigned>::max()));
 
-// Helper for the creation of simple stub routines that need no help from the GC. Note
-// that codeBlock gets "executed" more than once.
-#define FINALIZE_CODE_FOR_GC_AWARE_STUB(codeBlock, patchBuffer, makesCalls, cell, dataLogFArguments) \
-    (createJITStubRoutine(FINALIZE_CODE_FOR((codeBlock), (patchBuffer), dataLogFArguments), *(codeBlock)->vm(), (codeBlock), (makesCalls), (cell)))
-
 } // namespace JSC
 
 #endif // ENABLE(JIT)