Changeset 175653 in webkit

Timestamp:

Nov 5, 2014, 5:53:25 PM (10 years ago)

Author:

mark.lam@apple.com

Message:

PutById inline caches should have a store barrier when it triggers a structure transition.
<​https://webkit.org/b/138441>

Reviewed by Geoffrey Garen.

After r174025, we no longer insert DFG store barriers when the payload of a
PutById operation is not a cell. However, this can lead to a crash when we have
PutById inline cache code transitioning the structure and re-allocating the
butterfly of an old gen object. The lack of a store barrier in that inline
cache results in the old gen object not being noticed during an eden GC scan.
As a result, its newly allocated butterfly will not be kept alive, which leads
to a stale butterfly pointer and, eventually, a crash.

It is also possible that the new structure can be collected by the eden GC if
(at GC time):

1. It is in the eden gen.
2. The inline cache that installed it has been evicted.
3. There are no live eden gen objects referring to it.

The chances of this should be more rare than the butterfly re-allocation, but
it is still possible. Hence, the fix is to always add a store barrier if the
inline caches performs a structure transition.

• jit/Repatch.cpp:

(JSC::emitPutTransitionStub):

• Added store barrier code based on SpeculativeJIT::storeToWriteBarrierBuffer()'s implementation.

Location:

trunk/Source/JavaScriptCore

Files:

• ChangeLog (modified) (1 diff)
• jit/Repatch.cpp (modified) (3 diffs)

trunk/Source/JavaScriptCore/ChangeLog

@@ +1 @@
+2014-11-05  Mark Lam  <mark.lam@apple.com>
+
+        PutById inline caches should have a store barrier when it triggers a structure transition.
+        <https://webkit.org/b/138441>
+
+        Reviewed by Geoffrey Garen.
+
+        After r174025, we no longer insert DFG store barriers when the payload of a
+        PutById operation is not a cell.  However, this can lead to a crash when we have
+        PutById inline cache code transitioning the structure and re-allocating the
+        butterfly of an old gen object.  The lack of a store barrier in that inline
+        cache results in the old gen object not being noticed during an eden GC scan.
+        As a result, its newly allocated butterfly will not be kept alive, which leads
+        to a stale butterfly pointer and, eventually, a crash.
+
+        It is also possible that the new structure can be collected by the eden GC if
+        (at GC time):
+        1. It is in the eden gen.
+        2. The inline cache that installed it has been evicted.
+        3. There are no live eden gen objects referring to it.
+
+        The chances of this should be more rare than the butterfly re-allocation, but
+        it is still possible.  Hence, the fix is to always add a store barrier if the
+        inline caches performs a structure transition.
+
+        * jit/Repatch.cpp:
+        (JSC::emitPutTransitionStub):
+        - Added store barrier code based on SpeculativeJIT::storeToWriteBarrierBuffer()'s
+          implementation.
+
 2014-11-05  Gyuyoung Kim  <gyuyoung.kim@samsung.com>
 

trunk/Source/JavaScriptCore/jit/Repatch.cpp

@@ -1114 +1114 @@
 #endif
     
+    ScratchBuffer* scratchBuffer = nullptr;
+
+#if ENABLE(GGC)
+    MacroAssembler::Call callFlushWriteBarrierBuffer;
+    MacroAssembler::Jump ownerIsRememberedOrInEden = stubJit.jumpIfIsRememberedOrInEden(baseGPR);
+    {
+        WriteBarrierBuffer* writeBarrierBuffer = &stubJit.vm()->heap.writeBarrierBuffer();
+        stubJit.move(MacroAssembler::TrustedImmPtr(writeBarrierBuffer), scratchGPR1);
+        stubJit.load32(MacroAssembler::Address(scratchGPR1, WriteBarrierBuffer::currentIndexOffset()), scratchGPR2);
+        MacroAssembler::Jump needToFlush =
+            stubJit.branch32(MacroAssembler::AboveOrEqual, scratchGPR2, MacroAssembler::Address(scratchGPR1, WriteBarrierBuffer::capacityOffset()));
+
+        stubJit.add32(MacroAssembler::TrustedImm32(1), scratchGPR2);
+        stubJit.store32(scratchGPR2, MacroAssembler::Address(scratchGPR1, WriteBarrierBuffer::currentIndexOffset()));
+
+        stubJit.loadPtr(MacroAssembler::Address(scratchGPR1, WriteBarrierBuffer::bufferOffset()), scratchGPR1);
+        // We use an offset of -sizeof(void*) because we already added 1 to scratchGPR2.
+        stubJit.storePtr(baseGPR, MacroAssembler::BaseIndex(scratchGPR1, scratchGPR2, MacroAssembler::ScalePtr, static_cast<int32_t>(-sizeof(void*))));
+
+        MacroAssembler::Jump doneWithBarrier = stubJit.jump();
+        needToFlush.link(&stubJit);
+
+        scratchBuffer = vm->scratchBufferForSize(allocator.desiredScratchBufferSizeForCall());
+        allocator.preserveUsedRegistersToScratchBufferForCall(stubJit, scratchBuffer, scratchGPR2);
+        stubJit.setupArgumentsWithExecState(baseGPR);
+        callFlushWriteBarrierBuffer = stubJit.call();
+        allocator.restoreUsedRegistersFromScratchBufferForCall(stubJit, scratchBuffer, scratchGPR2);
+
+        doneWithBarrier.link(&stubJit);
+    }
+    ownerIsRememberedOrInEden.link(&stubJit);
+#endif
+
     MacroAssembler::Jump success;
     MacroAssembler::Jump failure;
@@ -1134 +1167 @@
         
         allocator.restoreReusedRegistersByPopping(stubJit);
-        ScratchBuffer* scratchBuffer = vm->scratchBufferForSize(allocator.desiredScratchBufferSizeForCall());
+        if (!scratchBuffer)
+            scratchBuffer = vm->scratchBufferForSize(allocator.desiredScratchBufferSizeForCall());
         allocator.preserveUsedRegistersToScratchBufferForCall(stubJit, scratchBuffer, scratchGPR1);
 #if USE(JSVALUE64)
@@ -1152 +1186 @@
     else
         patchBuffer.link(failureCases, failureLabel);
+#if ENABLE(GGC)
+    patchBuffer.link(callFlushWriteBarrierBuffer, operationFlushWriteBarrierBuffer);
+#endif
     if (structure->outOfLineCapacity() != oldStructure->outOfLineCapacity()) {
         patchBuffer.link(operationCall, operationReallocateStorageAndFinishPut);