[webkit-changes] [161863] branches/jsCStack/Source/JavaScriptCore

Sun, 12 Jan 2014 23:16:59 -0800

Title: [161863] branches/jsCStack/Source/_javascript_Core
Revision
161863
Author
[email protected]
Date
2014-01-12 23:20:55 -0800 (Sun, 12 Jan 2014)

Log Message

internal-js-tests.yaml/Octane/mandreel.js.default-ftl fails about 1/30 times with "TypeError: undefined is not an object"
https://bugs.webkit.org/show_bug.cgi?id=126797

Not yet reviewed.
        
This bug was a hilarious combination of concurrent JIT (making it a flake),
LLInt->JIT OSR, and the FTL's register preservation wrapper.
        
Consider that a CodeBlock with a LLInt JITCode may get called, and while running,
it may decide to tier up to the baseline JIT. At that point, the old JITCode gets
summarily deleted and replaced by the baseline JITCode. This used to be "correct"
because the LLInt's JITCode only held a reference to an immortal entrypoint thunk.
But not anymore: now the LLInt's JITCode may also hold references to register
preservation wrappers, which get generated lazily.
        
So consider that the FTL calls a LLInt'd CodeBlock and that LLInt JITCode lazily
creates a register preservation wrapper. That register preservation wrapper is on
the stack and the stack will have a return PC that points to it. Then the LLInt
code decides to tier up to the baseline JIT. This causes the LLInt JITCode to get
deleted. That means that the register preservation wrapper also gets deleted. Now
all it takes is for some more executable memory to get allocated, and when that
baseline JIT frame (that used to be a LLInt frame) returns, it returns into what
it *thinks* is a register preservation wrapper - except now it's something else!
        
Hilariously, the thing that we'd return into in mandreel was a PutByVal stub. When
the concurrent JIT interleaved itself in such a way as to make this case happen
(i.e. the FTL code for runMandreel was compiled just as some mandreel init routine
was *about* to get tiered up to baseline JIT), then the baseline code would
*always* allocate a PutByVal stub right where the register preservation wrapper's
return site used to be. Thus, returning from the baseline code for that
initialization function would cause it to jump to a PutByVal stub for one of the
PutByVal's in that same function. Somehow, we'd always end up returning to a valid
instruction rather than into the middle of something. That PutByVal stub would
then do a type check on some random variable that it thought was the base - it
would of course fail - and then the slow path would try to do conversions - that
would result in it seeing undefined - and then usually we would crash while trying
to throw the exception.
        
The solution is to make the return site of the register preservation thunk be
always immortal. We already had a standalone immortal return ramp for that thunk
for use by FTL OSR exit. I figure it doesn't hurt to just always use that immortal
return site.
        
This bug took several days to track down. I don't think I can add more tests for
this, although I'll think about it. Basically the lesson here is that running big
tests like Mandreel over and over again with the concurrent JIT enabled shakes out
interesting bugs.
        
This change also adds some useful debug flags, like --breakOnThrow=true, which
causes us to CRASH() whenever an exception is thrown. Without this, this bug was
basically impossible to figure out: we would wind up somewhere in
handleUncaughtException and then that code would go *completely berserk* because
the stack is of course totally screwed up once we "returned" into that PutByVal
stub.

* bytecode/CodeBlock.cpp:
(JSC::CodeBlock::dumpAssumingJITType):
(JSC::CodeBlock::CodeBlock):
* dfg/DFGCompilationMode.h:
(JSC::DFG::isFTL):
* dfg/DFGPlan.cpp:
(JSC::DFG::Plan::reportCompileTimes):
(JSC::DFG::Plan::compileInThread):
(JSC::DFG::Plan::compileInThreadImpl):
* dfg/DFGPlan.h:
* ftl/FTLCompile.cpp:
(JSC::FTL::compile):
* jit/JITExceptions.cpp:
(JSC::genericUnwind):
* jit/RegisterPreservationWrapperGenerator.cpp:
(JSC::generateRegisterPreservationWrapper):
(JSC::generateRegisterRestoration):
* jit/RegisterPreservationWrapperGenerator.h:
* runtime/Options.cpp:
(JSC::recomputeDependentOptions):
* runtime/Options.h:
* runtime/VM.cpp:
(JSC::VM::VM):
(JSC::VM::throwException):

Modified Paths

Diff

Modified: branches/jsCStack/Source/_javascript_Core/ChangeLog (161862 => 161863)


--- branches/jsCStack/Source/_javascript_Core/ChangeLog	2014-01-13 06:29:18 UTC (rev 161862)
+++ branches/jsCStack/Source/_javascript_Core/ChangeLog	2014-01-13 07:20:55 UTC (rev 161863)
@@ -1,3 +1,85 @@
+2014-01-12  Filip Pizlo  <[email protected]>
+
+        internal-js-tests.yaml/Octane/mandreel.js.default-ftl fails about 1/30 times with "TypeError: undefined is not an object"
+        https://bugs.webkit.org/show_bug.cgi?id=126797
+
+        Not yet reviewed.
+        
+        This bug was a hilarious combination of concurrent JIT (making it a flake),
+        LLInt->JIT OSR, and the FTL's register preservation wrapper.
+        
+        Consider that a CodeBlock with a LLInt JITCode may get called, and while running,
+        it may decide to tier up to the baseline JIT. At that point, the old JITCode gets
+        summarily deleted and replaced by the baseline JITCode. This used to be "correct"
+        because the LLInt's JITCode only held a reference to an immortal entrypoint thunk.
+        But not anymore: now the LLInt's JITCode may also hold references to register
+        preservation wrappers, which get generated lazily.
+        
+        So consider that the FTL calls a LLInt'd CodeBlock and that LLInt JITCode lazily
+        creates a register preservation wrapper. That register preservation wrapper is on
+        the stack and the stack will have a return PC that points to it. Then the LLInt
+        code decides to tier up to the baseline JIT. This causes the LLInt JITCode to get
+        deleted. That means that the register preservation wrapper also gets deleted. Now
+        all it takes is for some more executable memory to get allocated, and when that
+        baseline JIT frame (that used to be a LLInt frame) returns, it returns into what
+        it *thinks* is a register preservation wrapper - except now it's something else!
+        
+        Hilariously, the thing that we'd return into in mandreel was a PutByVal stub. When
+        the concurrent JIT interleaved itself in such a way as to make this case happen
+        (i.e. the FTL code for runMandreel was compiled just as some mandreel init routine
+        was *about* to get tiered up to baseline JIT), then the baseline code would
+        *always* allocate a PutByVal stub right where the register preservation wrapper's
+        return site used to be. Thus, returning from the baseline code for that
+        initialization function would cause it to jump to a PutByVal stub for one of the
+        PutByVal's in that same function. Somehow, we'd always end up returning to a valid
+        instruction rather than into the middle of something. That PutByVal stub would
+        then do a type check on some random variable that it thought was the base - it
+        would of course fail - and then the slow path would try to do conversions - that
+        would result in it seeing undefined - and then usually we would crash while trying
+        to throw the exception.
+        
+        The solution is to make the return site of the register preservation thunk be
+        always immortal. We already had a standalone immortal return ramp for that thunk
+        for use by FTL OSR exit. I figure it doesn't hurt to just always use that immortal
+        return site.
+        
+        This bug took several days to track down. I don't think I can add more tests for
+        this, although I'll think about it. Basically the lesson here is that running big
+        tests like Mandreel over and over again with the concurrent JIT enabled shakes out
+        interesting bugs.
+        
+        This change also adds some useful debug flags, like --breakOnThrow=true, which
+        causes us to CRASH() whenever an exception is thrown. Without this, this bug was
+        basically impossible to figure out: we would wind up somewhere in
+        handleUncaughtException and then that code would go *completely berserk* because
+        the stack is of course totally screwed up once we "returned" into that PutByVal
+        stub.
+
+        * bytecode/CodeBlock.cpp:
+        (JSC::CodeBlock::dumpAssumingJITType):
+        (JSC::CodeBlock::CodeBlock):
+        * dfg/DFGCompilationMode.h:
+        (JSC::DFG::isFTL):
+        * dfg/DFGPlan.cpp:
+        (JSC::DFG::Plan::reportCompileTimes):
+        (JSC::DFG::Plan::compileInThread):
+        (JSC::DFG::Plan::compileInThreadImpl):
+        * dfg/DFGPlan.h:
+        * ftl/FTLCompile.cpp:
+        (JSC::FTL::compile):
+        * jit/JITExceptions.cpp:
+        (JSC::genericUnwind):
+        * jit/RegisterPreservationWrapperGenerator.cpp:
+        (JSC::generateRegisterPreservationWrapper):
+        (JSC::generateRegisterRestoration):
+        * jit/RegisterPreservationWrapperGenerator.h:
+        * runtime/Options.cpp:
+        (JSC::recomputeDependentOptions):
+        * runtime/Options.h:
+        * runtime/VM.cpp:
+        (JSC::VM::VM):
+        (JSC::VM::throwException):
+
 2014-01-11  Filip Pizlo  <[email protected]>
 
         Eliminate obviously redundant InvalidationPoints

Modified: branches/jsCStack/Source/_javascript_Core/bytecode/CodeBlock.cpp (161862 => 161863)


--- branches/jsCStack/Source/_javascript_Core/bytecode/CodeBlock.cpp	2014-01-13 06:29:18 UTC (rev 161862)
+++ branches/jsCStack/Source/_javascript_Core/bytecode/CodeBlock.cpp	2014-01-13 07:20:55 UTC (rev 161863)
@@ -128,10 +128,15 @@
 
 void CodeBlock::dumpAssumingJITType(PrintStream& out, JITCode::JITType jitType) const
 {
+    out.print(inferredName(), "#");
     if (hasHash() || isSafeToComputeHash())
-        out.print(inferredName(), "#", hash(), ":[", RawPointer(this), "->", RawPointer(ownerExecutable()), ", ", jitType, codeType());
+        out.print(hash());
     else
-        out.print(inferredName(), "#<no-hash>:[", RawPointer(this), "->", RawPointer(ownerExecutable()), ", ", jitType, codeType());
+        out.print("<no-hash>");
+    out.print(":[", RawPointer(this), "->");
+    if (!!m_alternative)
+        out.print(RawPointer(m_alternative.get()), "->");
+    out.print(RawPointer(ownerExecutable()), ", ", jitType, codeType());
 
     if (codeType() == FunctionCode)
         out.print(specializationKind());
@@ -1849,6 +1854,7 @@
     if (Options::dumpGeneratedBytecodes())
         dumpBytecode();
 
+    
     m_heap->m_codeBlocks.add(this);
     m_heap->reportExtraMemoryCost(sizeof(CodeBlock) + m_instructions.size() * sizeof(Instruction));
 }

Modified: branches/jsCStack/Source/_javascript_Core/dfg/DFGCompilationMode.h (161862 => 161863)


--- branches/jsCStack/Source/_javascript_Core/dfg/DFGCompilationMode.h	2014-01-13 06:29:18 UTC (rev 161862)
+++ branches/jsCStack/Source/_javascript_Core/dfg/DFGCompilationMode.h	2014-01-13 07:20:55 UTC (rev 161863)
@@ -37,6 +37,17 @@
     FTLForOSREntryMode
 };
 
+inline bool isFTL(CompilationMode mode)
+{
+    switch (mode) {
+    case FTLMode:
+    case FTLForOSREntryMode:
+        return true;
+    default:
+        return false;
+    }
+}
+
 } } // namespace JSC::DFG
 
 namespace WTF {

Modified: branches/jsCStack/Source/_javascript_Core/dfg/DFGPlan.cpp (161862 => 161863)


--- branches/jsCStack/Source/_javascript_Core/dfg/DFGPlan.cpp	2014-01-13 06:29:18 UTC (rev 161862)
+++ branches/jsCStack/Source/_javascript_Core/dfg/DFGPlan.cpp	2014-01-13 07:20:55 UTC (rev 161863)
@@ -125,10 +125,16 @@
 {
 }
 
+bool Plan::reportCompileTimes() const
+{
+    return Options::reportCompileTimes()
+        || (Options::reportFTLCompileTimes() && isFTL(mode));
+}
+
 void Plan::compileInThread(LongLivedState& longLivedState)
 {
     double before = 0;
-    if (Options::reportCompileTimes())
+    if (reportCompileTimes())
         before = currentTimeMS();
     
     SamplingRegion samplingRegion("DFG Compilation (Plan)");
@@ -141,7 +147,7 @@
 
     RELEASE_ASSERT(finalizer);
     
-    if (Options::reportCompileTimes()) {
+    if (reportCompileTimes()) {
         const char* pathName;
         switch (path) {
         case FailPath:
@@ -159,7 +165,7 @@
             break;
         }
         double now = currentTimeMS();
-        dataLog("Optimized ", *codeBlock->alternative(), " using ", mode, " with ", pathName, " in ", now - before, " ms");
+        dataLog("Optimized ", *codeBlock, " using ", mode, " with ", pathName, " in ", now - before, " ms");
         if (path == FTLPath)
             dataLog(" (DFG: ", beforeFTL - before, ", LLVM: ", now - beforeFTL, ")");
         dataLog(".\n");
@@ -307,7 +313,7 @@
         FTL::State state(dfg);
         FTL::lowerDFGToLLVM(state);
         
-        if (Options::reportCompileTimes())
+        if (reportCompileTimes())
             beforeFTL = currentTimeMS();
         
         if (Options::llvmAlwaysFailsBeforeCompile()) {

Modified: branches/jsCStack/Source/_javascript_Core/dfg/DFGPlan.h (161862 => 161863)


--- branches/jsCStack/Source/_javascript_Core/dfg/DFGPlan.h	2014-01-13 06:29:18 UTC (rev 161862)
+++ branches/jsCStack/Source/_javascript_Core/dfg/DFGPlan.h	2014-01-13 07:20:55 UTC (rev 161863)
@@ -92,6 +92,8 @@
     RefPtr<DeferredCompilationCallback> callback;
 
 private:
+    bool reportCompileTimes() const;
+    
     enum CompilationPath { FailPath, DFGPath, FTLPath };
     CompilationPath compileInThreadImpl(LongLivedState&);

Modified: branches/jsCStack/Source/_javascript_Core/ftl/FTLCompile.cpp (161862 => 161863)


--- branches/jsCStack/Source/_javascript_Core/ftl/FTLCompile.cpp	2014-01-13 06:29:18 UTC (rev 161862)
+++ branches/jsCStack/Source/_javascript_Core/ftl/FTLCompile.cpp	2014-01-13 07:20:55 UTC (rev 161863)
@@ -495,7 +495,8 @@
                 dataLog(
                     "Generated LLVM code after stackmap-based fix-up for ",
                     CodeBlockWithJITType(state.graph.m_codeBlock, JITCode::FTLJIT),
-                    " #", i, ", ", state.codeSectionNames[i], ":\n");
+                    " in ", state.graph.m_plan.mode, " #", i, ", ",
+                    state.codeSectionNames[i], ":\n");
                 disassemble(
                     MacroAssemblerCodePtr(handle->start()), handle->sizeInBytes(),
                     "    ", WTF::dataFile(), LLVMSubset);

Modified: branches/jsCStack/Source/_javascript_Core/jit/JITExceptions.cpp (161862 => 161863)


--- branches/jsCStack/Source/_javascript_Core/jit/JITExceptions.cpp	2014-01-13 06:29:18 UTC (rev 161862)
+++ branches/jsCStack/Source/_javascript_Core/jit/JITExceptions.cpp	2014-01-13 07:20:55 UTC (rev 161863)
@@ -43,6 +43,11 @@
 
 void genericUnwind(VM* vm, ExecState* callFrame, JSValue exceptionValue)
 {
+    if (Options::breakOnThrow()) {
+        dataLog("In call frame ", RawPointer(callFrame), " for code block ", *callFrame->codeBlock(), "\n");
+        CRASH();
+    }
+    
     RELEASE_ASSERT(exceptionValue);
     HandlerInfo* handler = vm->interpreter->unwind(callFrame, exceptionValue); // This may update callFrame.

Modified: branches/jsCStack/Source/_javascript_Core/jit/RegisterPreservationWrapperGenerator.cpp (161862 => 161863)


--- branches/jsCStack/Source/_javascript_Core/jit/RegisterPreservationWrapperGenerator.cpp	2014-01-13 06:29:18 UTC (rev 161862)
+++ branches/jsCStack/Source/_javascript_Core/jit/RegisterPreservationWrapperGenerator.cpp	2014-01-13 07:20:55 UTC (rev 161863)
@@ -110,12 +110,15 @@
     jit.move(AssemblyHelpers::TrustedImm64(TagTypeNumber), GPRInfo::tagTypeNumberRegister);
     jit.add64(AssemblyHelpers::TrustedImm32(TagMask - TagTypeNumber), GPRInfo::tagTypeNumberRegister, GPRInfo::tagMaskRegister);
     
-    AssemblyHelpers::Call call = jit.nearCall();
+    jit.move(
+        AssemblyHelpers::TrustedImmPtr(
+            vm.getCTIStub(registerRestorationThunkGenerator).code().executableAddress()),
+        GPRInfo::nonArgGPR0);
+    jit.restoreReturnAddressBeforeReturn(GPRInfo::nonArgGPR0);
+    AssemblyHelpers::Jump jump = jit.jump();
     
-    generateRegisterRestoration(jit);
-    
     LinkBuffer linkBuffer(vm, &jit, GLOBAL_THUNK_ID);
-    linkBuffer.link(call, CodeLocationLabel(target));
+    linkBuffer.link(jump, CodeLocationLabel(target));
 
     if (Options::verboseFTLToJSThunk())
         dataLog("Need a thunk for calls from FTL to non-FTL version of ", *executable, "\n");
@@ -135,7 +138,7 @@
 #endif // ENABLE(FTL_JIT)
 }
 
-void generateRegisterRestoration(AssemblyHelpers& jit)
+static void generateRegisterRestoration(AssemblyHelpers& jit)
 {
 #if ENABLE(FTL_JIT)
     RegisterSet toSave = registersToPreserve();

Modified: branches/jsCStack/Source/_javascript_Core/jit/RegisterPreservationWrapperGenerator.h (161862 => 161863)


--- branches/jsCStack/Source/_javascript_Core/jit/RegisterPreservationWrapperGenerator.h	2014-01-13 06:29:18 UTC (rev 161862)
+++ branches/jsCStack/Source/_javascript_Core/jit/RegisterPreservationWrapperGenerator.h	2014-01-13 07:20:55 UTC (rev 161863)
@@ -41,8 +41,6 @@
 
 MacroAssemblerCodeRef generateRegisterPreservationWrapper(VM&, ExecutableBase*, MacroAssemblerCodePtr target);
 
-void generateRegisterRestoration(AssemblyHelpers&);
-
 MacroAssemblerCodeRef registerRestorationThunkGenerator(VM*);
 
 } // namespace JSC

Modified: branches/jsCStack/Source/_javascript_Core/runtime/Options.cpp (161862 => 161863)


--- branches/jsCStack/Source/_javascript_Core/runtime/Options.cpp	2014-01-13 06:29:18 UTC (rev 161862)
+++ branches/jsCStack/Source/_javascript_Core/runtime/Options.cpp	2014-01-13 07:20:55 UTC (rev 161863)
@@ -197,6 +197,7 @@
         || Options::verboseOSR()
         || Options::verboseCompilationQueue()
         || Options::reportCompileTimes()
+        || Options::reportFTLCompileTimes()
         || Options::verboseCFA()
         || Options::verboseFTLFailure())
         Options::alwaysComputeHash() = true;

Modified: branches/jsCStack/Source/_javascript_Core/runtime/Options.h (161862 => 161863)


--- branches/jsCStack/Source/_javascript_Core/runtime/Options.h	2014-01-13 06:29:18 UTC (rev 161862)
+++ branches/jsCStack/Source/_javascript_Core/runtime/Options.h	2014-01-13 07:20:55 UTC (rev 161863)
@@ -123,6 +123,7 @@
     v(bool, verboseCallLink, false) \
     v(bool, verboseCompilationQueue, false) \
     v(bool, reportCompileTimes, false) \
+    v(bool, reportFTLCompileTimes, false) \
     v(bool, verboseCFA, false) \
     v(bool, verboseFTLToJSThunk, false) \
     v(bool, verboseFTLFailure, false) \
@@ -156,6 +157,8 @@
     \
     v(bool, enableArchitectureSpecificOptimizations, true) \
     \
+    v(bool, breakOnThrow, false) \
+    \
     v(unsigned, maximumOptimizationCandidateInstructionCount, 10000) \
     \
     v(unsigned, maximumFunctionForCallInlineCandidateInstructionCount, 180) \

Modified: branches/jsCStack/Source/_javascript_Core/runtime/VM.cpp (161862 => 161863)


--- branches/jsCStack/Source/_javascript_Core/runtime/VM.cpp	2014-01-13 06:29:18 UTC (rev 161862)
+++ branches/jsCStack/Source/_javascript_Core/runtime/VM.cpp	2014-01-13 07:20:55 UTC (rev 161863)
@@ -288,7 +288,7 @@
 #endif
 
     heap.notifyIsSafeToCollect();
-
+    
     LLInt::Data::performAssertions(*this);
     
     if (Options::enableProfiler()) {
@@ -656,6 +656,11 @@
     
 JSValue VM::throwException(ExecState* exec, JSValue error)
 {
+    if (Options::breakOnThrow()) {
+        dataLog("In call frame ", RawPointer(exec), " for code block ", *exec->codeBlock(), "\n");
+        CRASH();
+    }
+    
     ASSERT(exec == topCallFrame || exec == exec->lexicalGlobalObject()->globalExec() || exec == exec->vmEntryGlobalObject()->globalExec());
     
     Vector<StackFrame> stackTrace;

_______________________________________________
webkit-changes mailing list
[email protected]
https://lists.webkit.org/mailman/listinfo/webkit-changes

Reply via email to