Changeset 164205 in webkit

Timestamp:

Feb 16, 2014, 10:25:05 PM (13 years ago)

Author:

fpizlo@apple.com

Message:

DFG::prepareOSREntry should be nice to the stack
​https://bugs.webkit.org/show_bug.cgi?id=128883

Reviewed by Oliver Hunt.

Previously OSR entry had some FIXME's and some really badly commented-out code for
clearing stack entries to help GC. It also did some permutations on a stack frame
above us, in such a way that it wasn't obviously that we wouldn't clobber our own
stack frame. This function also crashed in ASan.

It just seems like there was too much badness to the whole idea of prepareOSREntry
directly editing the stack. So, I changed it to create a stack frame in a scratch
buffer on the side and then have some assembly code just copy it into place. This
works fine, fixes a FIXME, possibly fixes some stack clobbering, and might help us
make more progress with ASan.

• dfg/DFGOSREntry.cpp:

(JSC::DFG::prepareOSREntry):

• dfg/DFGOSREntry.h:
• dfg/DFGThunks.cpp:

(JSC::DFG::osrEntryThunkGenerator):

• dfg/DFGThunks.h:
• jit/JITOpcodes.cpp:

(JSC::JIT::emitSlow_op_loop_hint):

• jit/JITOperations.cpp:

Location:

trunk/Source/JavaScriptCore

Files:

• ChangeLog (modified) (1 diff)
• dfg/DFGOSREntry.cpp (modified) (3 diffs)
• dfg/DFGOSREntry.h (modified) (1 diff)
• dfg/DFGThunks.cpp (modified) (2 diffs)
• dfg/DFGThunks.h (modified) (2 diffs)
• jit/JITOpcodes.cpp (modified) (1 diff)
• jit/JITOperations.cpp (modified) (2 diffs)

trunk/Source/JavaScriptCore/ChangeLog

@@ +1 @@
+2014-02-16  Filip Pizlo  <fpizlo@apple.com>
+
+        DFG::prepareOSREntry should be nice to the stack
+        https://bugs.webkit.org/show_bug.cgi?id=128883
+
+        Reviewed by Oliver Hunt.
+        
+        Previously OSR entry had some FIXME's and some really badly commented-out code for
+        clearing stack entries to help GC. It also did some permutations on a stack frame
+        above us, in such a way that it wasn't obviously that we wouldn't clobber our own
+        stack frame. This function also crashed in ASan.
+        
+        It just seems like there was too much badness to the whole idea of prepareOSREntry
+        directly editing the stack. So, I changed it to create a stack frame in a scratch
+        buffer on the side and then have some assembly code just copy it into place. This
+        works fine, fixes a FIXME, possibly fixes some stack clobbering, and might help us
+        make more progress with ASan.
+
+        * dfg/DFGOSREntry.cpp:
+        (JSC::DFG::prepareOSREntry):
+        * dfg/DFGOSREntry.h:
+        * dfg/DFGThunks.cpp:
+        (JSC::DFG::osrEntryThunkGenerator):
+        * dfg/DFGThunks.h:
+        * jit/JITOpcodes.cpp:
+        (JSC::JIT::emitSlow_op_loop_hint):
+        * jit/JITOperations.cpp:
+
 2014-02-15  Filip Pizlo  <fpizlo@apple.com>
 

trunk/Source/JavaScriptCore/dfg/DFGOSREntry.cpp

@@ -1 +1 @@
 /*
- * Copyright (C) 2011, 2013 Apple Inc. All rights reserved.
+ * Copyright (C) 2011, 2013, 2014 Apple Inc. All rights reserved.
  *
  * Redistribution and use in source and binary forms, with or without
@@ -189 +189 @@
     //    would have otherwise just kept running albeit less quickly.
     
-    unsigned frameSize = jitCode->common.requiredRegisterCountForExecutionAndExit();
-    if (!vm->interpreter->stack().ensureCapacityFor(&exec->registers()[virtualRegisterForLocal(frameSize - 1).offset()])) {
+    unsigned frameSizeForCheck = jitCode->common.requiredRegisterCountForExecutionAndExit();
+    if (!vm->interpreter->stack().ensureCapacityFor(&exec->registers()[virtualRegisterForLocal(frameSizeForCheck - 1).offset()])) {
         if (Options::verboseOSR())
             dataLogF("    OSR failed because stack growth failed.\n");
@@ -198 +198 @@
     if (Options::verboseOSR())
         dataLogF("    OSR should succeed.\n");
-    
-    // 3) Perform data format conversions.
-    for (size_t local = 0; local < entry->m_expectedValues.numberOfLocals(); ++local) {
-        if (entry->m_localsForcedDouble.get(local))
-            *bitwise_cast<double*>(exec->registers() + virtualRegisterForLocal(local).offset()) = exec->registers()[virtualRegisterForLocal(local).offset()].jsValue().asNumber();
-        if (entry->m_localsForcedMachineInt.get(local))
-            *bitwise_cast<int64_t*>(exec->registers() + virtualRegisterForLocal(local).offset()) = exec->registers()[virtualRegisterForLocal(local).offset()].jsValue().asMachineInt() << JSValue::int52ShiftAmount;
+
+    // At this point we're committed to entering. We will do some work to set things up,
+    // but we also rely on our caller recognizing that when we return a non-null pointer,
+    // that means that we're already past the point of no return and we must succeed at
+    // entering.
+    
+    // 3) Set up the data in the scratch buffer and perform data format conversions.
+
+    unsigned frameSize = jitCode->common.frameRegisterCount;
+    
+    Register* scratch = bitwise_cast<Register*>(vm->scratchBufferForSize(sizeof(Register) * (2 + JSStack::CallFrameHeaderSize + frameSize))->dataBuffer());
+    
+    *bitwise_cast<size_t*>(scratch + 0) = frameSize;
+    
+    void* targetPC = codeBlock->jitCode()->executableAddressAtOffset(entry->m_machineCodeOffset);
+    if (Options::verboseOSR())
+        dataLogF("    OSR using target PC %p.\n", targetPC);
+    RELEASE_ASSERT(targetPC);
+    *bitwise_cast<void**>(scratch + 1) = targetPC;
+    
+    Register* pivot = scratch + 2 + JSStack::CallFrameHeaderSize;
+    
+    for (int index = -JSStack::CallFrameHeaderSize; index < static_cast<int>(frameSize); ++index) {
+        VirtualRegister reg(-1 - index);
+        
+        if (reg.isLocal()) {
+            if (entry->m_localsForcedDouble.get(reg.toLocal())) {
+                *bitwise_cast<double*>(pivot + index) = exec->registers()[reg.offset()].jsValue().asNumber();
+                continue;
+            }
+            
+            if (entry->m_localsForcedMachineInt.get(reg.toLocal())) {
+                *bitwise_cast<int64_t*>(pivot + index) = exec->registers()[reg.offset()].jsValue().asMachineInt() << JSValue::int52ShiftAmount;
+                continue;
+            }
+        }
+        
+        pivot[index] = exec->registers()[reg.offset()].jsValue();
     }
     
     // 4) Reshuffle those registers that need reshuffling.
-    
-    Vector<EncodedJSValue> temporaryLocals(entry->m_reshufflings.size());
-    EncodedJSValue* registers = bitwise_cast<EncodedJSValue*>(exec->registers());
+    Vector<JSValue> temporaryLocals(entry->m_reshufflings.size());
     for (unsigned i = entry->m_reshufflings.size(); i--;)
-        temporaryLocals[i] = registers[entry->m_reshufflings[i].fromOffset];
+        temporaryLocals[i] = pivot[VirtualRegister(entry->m_reshufflings[i].fromOffset).toLocal()].jsValue();
     for (unsigned i = entry->m_reshufflings.size(); i--;)
-        registers[entry->m_reshufflings[i].toOffset] = temporaryLocals[i];
-    
-    // 5) Clear those parts of the call frame that the DFG ain't using. This helps GC on some
-    //    programs by eliminating some stale pointer pathologies.
-
-#if 0 // FIXME: CStack - This needs to be verified before being enabled
+        pivot[VirtualRegister(entry->m_reshufflings[i].toOffset).toLocal()] = temporaryLocals[i];
+    
+    // 5) Clear those parts of the call frame that the DFG ain't using. This helps GC on
+    //    some programs by eliminating some stale pointer pathologies.
     for (unsigned i = frameSize; i--;) {
         if (entry->m_machineStackUsed.get(i))
             continue;
-        registers[virtualRegisterForLocal(i).offset()] = JSValue::encode(JSValue());
-    }
-#endif
-    
-    // 6) Fix the call frame.
-    
-    exec->setCodeBlock(codeBlock);
-    
-    // 7) Find and return the destination machine code address.
-    
-    void* result = codeBlock->jitCode()->executableAddressAtOffset(entry->m_machineCodeOffset);
+        pivot[i] = JSValue();
+    }
+    
+    // 6) Fix the call frame to have the right code block.
+    
+    *bitwise_cast<CodeBlock**>(pivot - 1 - JSStack::CodeBlock) = codeBlock;
     
     if (Options::verboseOSR())
-        dataLogF("    OSR returning machine code address %p.\n", result);
-    
-    return result;
+        dataLogF("    OSR returning data buffer %p.\n", scratch);
+    return scratch;
 }
 

trunk/Source/JavaScriptCore/dfg/DFGOSREntry.h

@@ -68 +68 @@
 }
 
+// Returns a pointer to a data buffer that the OSR entry thunk will recognize and
+// parse. If this returns null, it means 
 void* prepareOSREntry(ExecState*, CodeBlock*, unsigned bytecodeIndex);
 #else

trunk/Source/JavaScriptCore/dfg/DFGThunks.cpp

@@ -1 +1 @@
 /*
- * Copyright (C) 2011 Apple Inc. All rights reserved.
+ * Copyright (C) 2011, 2014 Apple Inc. All rights reserved.
  *
  * Redistribution and use in source and binary forms, with or without
@@ -97 +97 @@
 }
 
+MacroAssemblerCodeRef osrEntryThunkGenerator(VM* vm)
+{
+    MacroAssembler jit;
+    
+    // We get passed the address of a scratch buffer. The first 8-byte slot of the buffer
+    // is the frame size. The second 8-byte slot is the pointer to where we are supposed to
+    // jump. The remaining bytes are the new call frame header followed by the locals.
+    
+    ptrdiff_t offsetOfFrameSize = 0; // This is the DFG frame count.
+    ptrdiff_t offsetOfTargetPC = offsetOfFrameSize + sizeof(EncodedJSValue);
+    ptrdiff_t offsetOfPayload = offsetOfTargetPC + sizeof(EncodedJSValue);
+    ptrdiff_t offsetOfLocals = offsetOfPayload + sizeof(Register) * JSStack::CallFrameHeaderSize;
+    
+    jit.move(GPRInfo::returnValueGPR2, GPRInfo::regT0);
+    jit.loadPtr(MacroAssembler::Address(GPRInfo::regT0, offsetOfFrameSize), GPRInfo::regT1); // Load the frame size.
+    jit.move(GPRInfo::regT1, GPRInfo::regT2);
+    jit.lshiftPtr(MacroAssembler::Imm32(3), GPRInfo::regT2);
+    jit.move(GPRInfo::callFrameRegister, MacroAssembler::stackPointerRegister);
+    jit.subPtr(GPRInfo::regT2, MacroAssembler::stackPointerRegister);
+    
+    MacroAssembler::Label loop = jit.label();
+    jit.subPtr(MacroAssembler::TrustedImm32(1), GPRInfo::regT1);
+    jit.move(GPRInfo::regT1, GPRInfo::regT4);
+    jit.negPtr(GPRInfo::regT4);
+    jit.load32(MacroAssembler::BaseIndex(GPRInfo::regT0, GPRInfo::regT1, MacroAssembler::TimesEight, offsetOfLocals), GPRInfo::regT2);
+    jit.load32(MacroAssembler::BaseIndex(GPRInfo::regT0, GPRInfo::regT1, MacroAssembler::TimesEight, offsetOfLocals + sizeof(int32_t)), GPRInfo::regT3);
+    jit.store32(GPRInfo::regT2, MacroAssembler::BaseIndex(GPRInfo::callFrameRegister, GPRInfo::regT4, MacroAssembler::TimesEight, -static_cast<intptr_t>(sizeof(Register))));
+    jit.store32(GPRInfo::regT3, MacroAssembler::BaseIndex(GPRInfo::callFrameRegister, GPRInfo::regT4, MacroAssembler::TimesEight, -static_cast<intptr_t>(sizeof(Register)) + static_cast<intptr_t>(sizeof(int32_t))));
+    jit.branchPtr(MacroAssembler::NotEqual, GPRInfo::regT1, MacroAssembler::TrustedImmPtr(bitwise_cast<void*>(-static_cast<intptr_t>(JSStack::CallFrameHeaderSize)))).linkTo(loop, &jit);
+    
+    jit.loadPtr(MacroAssembler::Address(GPRInfo::regT0, offsetOfTargetPC), GPRInfo::regT1);
+    MacroAssembler::Jump ok = jit.branchPtr(MacroAssembler::Above, GPRInfo::regT1, MacroAssembler::TrustedImmPtr(bitwise_cast<void*>(static_cast<intptr_t>(1000))));
+    jit.breakpoint();
+    ok.link(&jit);
+    jit.jump(GPRInfo::regT1);
+    
+    LinkBuffer patchBuffer(*vm, &jit, GLOBAL_THUNK_ID);
+    return FINALIZE_CODE(patchBuffer, ("DFG OSR entry thunk"));
+}
+
 } } // namespace JSC::DFG
 

trunk/Source/JavaScriptCore/dfg/DFGThunks.h

@@ -1 +1 @@
 /*
- * Copyright (C) 2011 Apple Inc. All rights reserved.
+ * Copyright (C) 2011, 2014 Apple Inc. All rights reserved.
  *
  * Redistribution and use in source and binary forms, with or without
@@ -40 +40 @@
 
 MacroAssemblerCodeRef osrExitGenerationThunkGenerator(VM*);
+MacroAssemblerCodeRef osrEntryThunkGenerator(VM*);
 
 } } // namespace JSC::DFG

trunk/Source/JavaScriptCore/jit/JITOpcodes.cpp

@@ -1107 +1107 @@
         callOperation(operationOptimize, m_bytecodeOffset);
         Jump noOptimizedEntry = branchTestPtr(Zero, returnValueGPR);
-        move(returnValueGPR2, stackPointerRegister);
+        if (!ASSERT_DISABLED) {
+            Jump ok = branchPtr(MacroAssembler::Above, regT0, TrustedImmPtr(bitwise_cast<void*>(static_cast<intptr_t>(1000))));
+            breakpoint();
+            ok.link(this);
+        }
         jump(returnValueGPR);
         noOptimizedEntry.link(this);

trunk/Source/JavaScriptCore/jit/JITOperations.cpp

@@ -33 +33 @@
 #include "DFGDriver.h"
 #include "DFGOSREntry.h"
+#include "DFGThunks.h"
 #include "DFGWorklist.h"
 #include "Error.h"
@@ -1190 +1191 @@
     ASSERT(JITCode::isOptimizingJIT(optimizedCodeBlock->jitType()));
     
-    if (void* address = DFG::prepareOSREntry(exec, optimizedCodeBlock, bytecodeIndex)) {
+    if (void* dataBuffer = DFG::prepareOSREntry(exec, optimizedCodeBlock, bytecodeIndex)) {
         if (Options::verboseOSR()) {
             dataLog(
-                "Performing OSR ", *codeBlock, " -> ", *optimizedCodeBlock, ", address ",
-                RawPointer(OUR_RETURN_ADDRESS), " -> ", RawPointer(address), ".\n");
+                "Performing OSR ", *codeBlock, " -> ", *optimizedCodeBlock, ".\n");
         }
 
         codeBlock->optimizeSoon();
-        ASSERT(exec->codeBlock() == optimizedCodeBlock);
-        return encodeResult(address, exec->topOfFrame());
+        return encodeResult(vm.getCTIStub(DFG::osrEntryThunkGenerator).code().executableAddress(), dataBuffer);
     }