sub IC does not properly handle exception handling now that try/catch is compiled...
authorsbarati@apple.com <sbarati@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Fri, 13 Nov 2015 21:36:18 +0000 (21:36 +0000)
committersbarati@apple.com <sbarati@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Fri, 13 Nov 2015 21:36:18 +0000 (21:36 +0000)
https://bugs.webkit.org/show_bug.cgi?id=151080

Reviewed by Geoffrey Garen.

This patch implements proper exception handling for ArithSubs with binaryUseKind as
UntypedUse inside try/catch.  We implement this in a very similar way as we implement
exception handling from GetById/PutById/LazySlowPaths callOperation calls. We do the needed
spilling if the result is the same register as the left/right register because in the event
of an exception, we don't want to do value recovery on the garbage result, we
want to do it on the original lhs/rhs of the operation.

This patch also does some refactoring. No longer does OSRExitDescriptor
have a long list of booleans that correspond to different OSR exit types.
It now just has an enum property called ExceptionType that tells us
what type of exception handler the OSR exit is. Then, we can just ask
interesting questions about the OSR exit and get answers based on its
ExceptionType property.

* ftl/FTLCompile.cpp:
(JSC::FTL::mmAllocateDataSection):
* ftl/FTLExceptionHandlerManager.cpp:
(JSC::FTL::ExceptionHandlerManager::addNewExit):
(JSC::FTL::ExceptionHandlerManager::callOperationExceptionTarget):
(JSC::FTL::ExceptionHandlerManager::lazySlowPathExceptionTarget):
(JSC::FTL::ExceptionHandlerManager::getByIdOSRExit):
(JSC::FTL::ExceptionHandlerManager::subOSRExit):
(JSC::FTL::ExceptionHandlerManager::getCallOSRExitCommon):
(JSC::FTL::ExceptionHandlerManager::getOrPutByIdCallOperationExceptionTarget): Deleted.
* ftl/FTLExceptionHandlerManager.h:
* ftl/FTLExitThunkGenerator.cpp:
(JSC::FTL::ExitThunkGenerator::emitThunk):
* ftl/FTLLowerDFGToLLVM.cpp:
(JSC::FTL::DFG::LowerDFGToLLVM::lower):
(JSC::FTL::DFG::LowerDFGToLLVM::compileArithAddOrSub):
(JSC::FTL::DFG::LowerDFGToLLVM::compilePutById):
(JSC::FTL::DFG::LowerDFGToLLVM::compileCallOrConstruct):
(JSC::FTL::DFG::LowerDFGToLLVM::compileCallOrConstructVarargs):
(JSC::FTL::DFG::LowerDFGToLLVM::compileInvalidationPoint):
(JSC::FTL::DFG::LowerDFGToLLVM::getById):
(JSC::FTL::DFG::LowerDFGToLLVM::lazySlowPath):
(JSC::FTL::DFG::LowerDFGToLLVM::callCheck):
(JSC::FTL::DFG::LowerDFGToLLVM::appendOSRExitArgumentsForPatchpointIfWillCatchException):
(JSC::FTL::DFG::LowerDFGToLLVM::emitBranchToOSRExitIfWillCatchException):
(JSC::FTL::DFG::LowerDFGToLLVM::lowBlock):
(JSC::FTL::DFG::LowerDFGToLLVM::appendOSRExitDescriptor):
(JSC::FTL::DFG::LowerDFGToLLVM::appendOSRExit):
(JSC::FTL::DFG::LowerDFGToLLVM::buildExitArguments):
* ftl/FTLOSRExit.cpp:
(JSC::FTL::OSRExitDescriptor::OSRExitDescriptor):
(JSC::FTL::OSRExitDescriptor::willArriveAtExitFromIndirectExceptionCheck):
(JSC::FTL::OSRExitDescriptor::mightArriveAtOSRExitFromGenericUnwind):
(JSC::FTL::OSRExitDescriptor::mightArriveAtOSRExitFromCallOperation):
(JSC::FTL::OSRExitDescriptor::needsRegisterRecoveryOnGenericUnwindOSRExitPath):
(JSC::FTL::OSRExitDescriptor::isExceptionHandler):
(JSC::FTL::OSRExitDescriptor::validateReferences):
(JSC::FTL::OSRExit::OSRExit):
(JSC::FTL::OSRExit::codeLocationForRepatch):
(JSC::FTL::OSRExit::gatherRegistersToSpillForCallIfException):
(JSC::FTL::OSRExit::spillRegistersToSpillSlot):
(JSC::FTL::OSRExit::recoverRegistersFromSpillSlot):
* ftl/FTLOSRExit.h:
* ftl/FTLOSRExitCompilationInfo.h:
(JSC::FTL::OSRExitCompilationInfo::OSRExitCompilationInfo):
* ftl/FTLOSRExitCompiler.cpp:
(JSC::FTL::compileFTLOSRExit):
* tests/stress/ftl-try-catch-arith-sub-exception.js: Added.
(assert):
(let.o.valueOf):
(baz):
(foo):
(bar):

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

Source/JavaScriptCore/ChangeLog
Source/JavaScriptCore/ftl/FTLCompile.cpp
Source/JavaScriptCore/ftl/FTLExceptionHandlerManager.cpp
Source/JavaScriptCore/ftl/FTLExceptionHandlerManager.h
Source/JavaScriptCore/ftl/FTLExitThunkGenerator.cpp
Source/JavaScriptCore/ftl/FTLLowerDFGToLLVM.cpp
Source/JavaScriptCore/ftl/FTLOSRExit.cpp
Source/JavaScriptCore/ftl/FTLOSRExit.h
Source/JavaScriptCore/ftl/FTLOSRExitCompilationInfo.h
Source/JavaScriptCore/ftl/FTLOSRExitCompiler.cpp
Source/JavaScriptCore/tests/stress/ftl-try-catch-arith-sub-exception.js [new file with mode: 0644]

index 35b2226..3da2331 100644 (file)
@@ -1,3 +1,78 @@
+2015-11-13  Saam barati  <sbarati@apple.com>
+
+        sub IC does not properly handle exception handling now that try/catch is compiled in the FTL
+        https://bugs.webkit.org/show_bug.cgi?id=151080
+
+        Reviewed by Geoffrey Garen.
+
+        This patch implements proper exception handling for ArithSubs with binaryUseKind as
+        UntypedUse inside try/catch.  We implement this in a very similar way as we implement 
+        exception handling from GetById/PutById/LazySlowPaths callOperation calls. We do the needed 
+        spilling if the result is the same register as the left/right register because in the event
+        of an exception, we don't want to do value recovery on the garbage result, we
+        want to do it on the original lhs/rhs of the operation.
+
+        This patch also does some refactoring. No longer does OSRExitDescriptor
+        have a long list of booleans that correspond to different OSR exit types.
+        It now just has an enum property called ExceptionType that tells us
+        what type of exception handler the OSR exit is. Then, we can just ask
+        interesting questions about the OSR exit and get answers based on its
+        ExceptionType property.
+
+        * ftl/FTLCompile.cpp:
+        (JSC::FTL::mmAllocateDataSection):
+        * ftl/FTLExceptionHandlerManager.cpp:
+        (JSC::FTL::ExceptionHandlerManager::addNewExit):
+        (JSC::FTL::ExceptionHandlerManager::callOperationExceptionTarget):
+        (JSC::FTL::ExceptionHandlerManager::lazySlowPathExceptionTarget):
+        (JSC::FTL::ExceptionHandlerManager::getByIdOSRExit):
+        (JSC::FTL::ExceptionHandlerManager::subOSRExit):
+        (JSC::FTL::ExceptionHandlerManager::getCallOSRExitCommon):
+        (JSC::FTL::ExceptionHandlerManager::getOrPutByIdCallOperationExceptionTarget): Deleted.
+        * ftl/FTLExceptionHandlerManager.h:
+        * ftl/FTLExitThunkGenerator.cpp:
+        (JSC::FTL::ExitThunkGenerator::emitThunk):
+        * ftl/FTLLowerDFGToLLVM.cpp:
+        (JSC::FTL::DFG::LowerDFGToLLVM::lower):
+        (JSC::FTL::DFG::LowerDFGToLLVM::compileArithAddOrSub):
+        (JSC::FTL::DFG::LowerDFGToLLVM::compilePutById):
+        (JSC::FTL::DFG::LowerDFGToLLVM::compileCallOrConstruct):
+        (JSC::FTL::DFG::LowerDFGToLLVM::compileCallOrConstructVarargs):
+        (JSC::FTL::DFG::LowerDFGToLLVM::compileInvalidationPoint):
+        (JSC::FTL::DFG::LowerDFGToLLVM::getById):
+        (JSC::FTL::DFG::LowerDFGToLLVM::lazySlowPath):
+        (JSC::FTL::DFG::LowerDFGToLLVM::callCheck):
+        (JSC::FTL::DFG::LowerDFGToLLVM::appendOSRExitArgumentsForPatchpointIfWillCatchException):
+        (JSC::FTL::DFG::LowerDFGToLLVM::emitBranchToOSRExitIfWillCatchException):
+        (JSC::FTL::DFG::LowerDFGToLLVM::lowBlock):
+        (JSC::FTL::DFG::LowerDFGToLLVM::appendOSRExitDescriptor):
+        (JSC::FTL::DFG::LowerDFGToLLVM::appendOSRExit):
+        (JSC::FTL::DFG::LowerDFGToLLVM::buildExitArguments):
+        * ftl/FTLOSRExit.cpp:
+        (JSC::FTL::OSRExitDescriptor::OSRExitDescriptor):
+        (JSC::FTL::OSRExitDescriptor::willArriveAtExitFromIndirectExceptionCheck):
+        (JSC::FTL::OSRExitDescriptor::mightArriveAtOSRExitFromGenericUnwind):
+        (JSC::FTL::OSRExitDescriptor::mightArriveAtOSRExitFromCallOperation):
+        (JSC::FTL::OSRExitDescriptor::needsRegisterRecoveryOnGenericUnwindOSRExitPath):
+        (JSC::FTL::OSRExitDescriptor::isExceptionHandler):
+        (JSC::FTL::OSRExitDescriptor::validateReferences):
+        (JSC::FTL::OSRExit::OSRExit):
+        (JSC::FTL::OSRExit::codeLocationForRepatch):
+        (JSC::FTL::OSRExit::gatherRegistersToSpillForCallIfException):
+        (JSC::FTL::OSRExit::spillRegistersToSpillSlot):
+        (JSC::FTL::OSRExit::recoverRegistersFromSpillSlot):
+        * ftl/FTLOSRExit.h:
+        * ftl/FTLOSRExitCompilationInfo.h:
+        (JSC::FTL::OSRExitCompilationInfo::OSRExitCompilationInfo):
+        * ftl/FTLOSRExitCompiler.cpp:
+        (JSC::FTL::compileFTLOSRExit):
+        * tests/stress/ftl-try-catch-arith-sub-exception.js: Added.
+        (assert):
+        (let.o.valueOf):
+        (baz):
+        (foo):
+        (bar):
+
 2015-11-13  Caitlin Potter  <caitpotter88@gmail.com>
 
         Allow any LeftHandSideExpression as a valid AssignmentElement
index ff81bdf..a752b07 100644 (file)
@@ -639,21 +639,36 @@ static void fixFunctionBasedOnStackMaps(
             }
 
             OSRExit& exit = state.jitCode->osrExit.last();
-            if (exitDescriptor.m_willArriveAtOSRExitFromGenericUnwind || exitDescriptor.m_isExceptionFromLazySlowPath) {
+            if (exitDescriptor.willArriveAtExitFromIndirectExceptionCheck()) {
                 StackMaps::Record& record = iter->value[j].record;
                 RELEASE_ASSERT(exit.m_descriptor.m_semanticCodeOriginForCallFrameHeader.isSet());
                 CallSiteIndex callSiteIndex = state.jitCode->common.addUniqueCallSiteIndex(exit.m_descriptor.m_semanticCodeOriginForCallFrameHeader);
                 exit.m_exceptionHandlerCallSiteIndex = callSiteIndex;
                 exceptionHandlerManager.addNewExit(iter->value[j].index, state.jitCode->osrExit.size() - 1);
 
-                if (exitDescriptor.m_isExceptionFromJSCall)
+                // Subs and GetByIds have an interesting register preservation story,
+                // see comment below at GetById to read about it.
+                //
+                // We set the registers needing spillage here because they need to be set
+                // before we generate OSR exits so the exit knows to do the proper recovery.
+                if (exitDescriptor.m_exceptionType == ExceptionType::JSCall) {
+                    // Call patchpoints might have values we want to do value recovery
+                    // on inside volatile registers. We need to collect the volatile
+                    // registers we want to do value recovery on here because they must
+                    // be preserved to the stack before the call, that way the OSR exit
+                    // exception handler can recover them into the proper registers.
                     exit.gatherRegistersToSpillForCallIfException(stackmaps, record);
-                if (exitDescriptor.m_isExceptionFromGetById) {
+                } else if (exitDescriptor.m_exceptionType == ExceptionType::GetById) {
                     GPRReg result = record.locations[0].directGPR();
                     GPRReg base = record.locations[1].directGPR();
-                    // This has an interesting story, see comment below describing it.
-                    if (result == base)
+                    if (base == result)
                         exit.registersToPreserveForCallThatMightThrow.set(base);
+                } else if (exitDescriptor.m_exceptionType == ExceptionType::SubGenerator) {
+                    GPRReg result = record.locations[0].directGPR();
+                    GPRReg left = record.locations[1].directGPR();
+                    GPRReg right = record.locations[2].directGPR();
+                    if (result == left || result == right)
+                        exit.registersToPreserveForCallThatMightThrow.set(result);
                 }
             }
         }
@@ -684,7 +699,7 @@ static void fixFunctionBasedOnStackMaps(
             info.m_thunkAddress = linkBuffer->locationOf(info.m_thunkLabel);
             exit.m_patchableCodeOffset = linkBuffer->offsetOf(info.m_thunkJump);
 
-            if (exit.m_descriptor.m_willArriveAtOSRExitFromGenericUnwind) {
+            if (exit.m_descriptor.mightArriveAtOSRExitFromGenericUnwind()) {
                 HandlerInfo newHandler = exit.m_descriptor.m_baselineExceptionHandler;
                 newHandler.start = exit.m_exceptionHandlerCallSiteIndex.bits();
                 newHandler.end = exit.m_exceptionHandlerCallSiteIndex.bits() + 1;
@@ -717,7 +732,7 @@ static void fixFunctionBasedOnStackMaps(
 
         Vector<std::pair<CCallHelpers::JumpList, CodeLocationLabel>> exceptionJumpsToLink;
         auto addNewExceptionJumpIfNecessary = [&] (uint32_t recordIndex) {
-            CodeLocationLabel exceptionTarget = exceptionHandlerManager.getOrPutByIdCallOperationExceptionTarget(recordIndex);
+            CodeLocationLabel exceptionTarget = exceptionHandlerManager.callOperationExceptionTarget(recordIndex);
             if (!exceptionTarget)
                 return false;
             exceptionJumpsToLink.append(
@@ -877,8 +892,15 @@ static void fixFunctionBasedOnStackMaps(
                 GPRReg right = record.locations[2].directGPR();
 
                 arithSub.m_slowPathStarts.append(slowPathJIT.label());
+                bool addedUniqueExceptionJump = addNewExceptionJumpIfNecessary(iter->value[i].index);
+                if (result == left || result == right) {
+                    // This situation has a really interesting register preservation story.
+                    // See comment above for GetByIds.
+                    if (OSRExit* exit = exceptionHandlerManager.subOSRExit(iter->value[i].index))
+                        exit->spillRegistersToSpillSlot(slowPathJIT, jsCallThatMightThrowSpillOffset);
+                }
 
-                callOperation(state, usedRegisters, slowPathJIT, codeOrigin, &exceptionTarget,
+                callOperation(state, usedRegisters, slowPathJIT, codeOrigin, addedUniqueExceptionJump ? &exceptionJumpsToLink.last().first : &exceptionTarget,
                     operationValueSub, result, left, right).call();
 
                 arithSub.m_slowPathDone.append(slowPathJIT.jump());
@@ -1058,7 +1080,7 @@ static void fixFunctionBasedOnStackMaps(
         OSRExit& exit = jitCode->osrExit[exitIndex];
         Vector<const void*> codeAddresses;
 
-        if (exit.m_descriptor.m_willArriveAtOSRExitFromGenericUnwind || exit.m_descriptor.m_isExceptionFromLazySlowPath) // This is reached by a jump from genericUnwind or a jump from a lazy slow path.
+        if (exit.m_descriptor.willArriveAtExitFromIndirectExceptionCheck()) // This jump doesn't happen directly from a patchpoint/stackmap we compile. It happens indirectly through an exception check somewhere.
             continue;
         
         StackMaps::Record& record = jitCode->stackmaps.records[exit.m_stackmapRecordIndex];
index 710db2d..7093037 100644 (file)
@@ -41,20 +41,20 @@ void ExceptionHandlerManager::addNewExit(uint32_t stackmapRecordIndex, size_t os
 {
     m_map.add(stackmapRecordIndex, osrExitIndex);
     OSRExit& exit = m_state.jitCode->osrExit[osrExitIndex];
-    RELEASE_ASSERT(exit.m_descriptor.m_willArriveAtOSRExitFromGenericUnwind || exit.m_descriptor.m_isExceptionFromLazySlowPath);
+    RELEASE_ASSERT(exit.m_descriptor.willArriveAtExitFromIndirectExceptionCheck());
 }
 
-CodeLocationLabel ExceptionHandlerManager::getOrPutByIdCallOperationExceptionTarget(uint32_t stackmapRecordIndex)
+CodeLocationLabel ExceptionHandlerManager::callOperationExceptionTarget(uint32_t stackmapRecordIndex)
 {
     auto findResult = m_map.find(stackmapRecordIndex);
     if (findResult == m_map.end())
         return CodeLocationLabel();
 
     size_t osrExitIndex = findResult->value;
-    RELEASE_ASSERT(m_state.jitCode->osrExit[osrExitIndex].m_descriptor.m_willArriveAtOSRExitFromGenericUnwind);
+    RELEASE_ASSERT(m_state.jitCode->osrExit[osrExitIndex].m_descriptor.mightArriveAtOSRExitFromCallOperation());
     OSRExitCompilationInfo& info = m_state.finalizer->osrExit[osrExitIndex];
-    RELEASE_ASSERT(info.m_getAndPutByIdCallOperationExceptionOSRExitEntrance.isSet());
-    return m_state.finalizer->exitThunksLinkBuffer->locationOf(info.m_getAndPutByIdCallOperationExceptionOSRExitEntrance);
+    RELEASE_ASSERT(info.m_callOperationExceptionOSRExitEntrance.isSet());
+    return m_state.finalizer->exitThunksLinkBuffer->locationOf(info.m_callOperationExceptionOSRExitEntrance);
 }
 
 CodeLocationLabel ExceptionHandlerManager::lazySlowPathExceptionTarget(uint32_t stackmapRecordIndex)
@@ -64,7 +64,7 @@ CodeLocationLabel ExceptionHandlerManager::lazySlowPathExceptionTarget(uint32_t
         return CodeLocationLabel();
 
     size_t osrExitIndex = findResult->value;
-    RELEASE_ASSERT(m_state.jitCode->osrExit[osrExitIndex].m_descriptor.m_isExceptionFromLazySlowPath);
+    RELEASE_ASSERT(m_state.jitCode->osrExit[osrExitIndex].m_descriptor.m_exceptionType == ExceptionType::LazySlowPath);
     OSRExitCompilationInfo& info = m_state.finalizer->osrExit[osrExitIndex];
     RELEASE_ASSERT(info.m_thunkLabel.isSet());
     return m_state.finalizer->exitThunksLinkBuffer->locationOf(info.m_thunkLabel);
@@ -77,7 +77,18 @@ OSRExit* ExceptionHandlerManager::getByIdOSRExit(uint32_t stackmapRecordIndex)
         return nullptr;
     size_t osrExitIndex = findResult->value;
     OSRExit* exit = &m_state.jitCode->osrExit[osrExitIndex];
-    RELEASE_ASSERT(exit->m_descriptor.m_isExceptionFromGetById);
+    RELEASE_ASSERT(exit->m_descriptor.m_exceptionType == ExceptionType::GetById);
+    return exit; 
+}
+
+OSRExit* ExceptionHandlerManager::subOSRExit(uint32_t stackmapRecordIndex)
+{
+    auto findResult = m_map.find(stackmapRecordIndex);
+    if (findResult == m_map.end())
+        return nullptr;
+    size_t osrExitIndex = findResult->value;
+    OSRExit* exit = &m_state.jitCode->osrExit[osrExitIndex];
+    RELEASE_ASSERT(exit->m_descriptor.m_exceptionType == ExceptionType::SubGenerator);
     return exit; 
 }
 
@@ -88,7 +99,7 @@ OSRExit* ExceptionHandlerManager::getCallOSRExitCommon(uint32_t stackmapRecordIn
         return nullptr;
     size_t osrExitIndex = findResult->value;
     OSRExit* exit = &m_state.jitCode->osrExit[osrExitIndex];
-    RELEASE_ASSERT(exit->m_descriptor.m_isExceptionFromJSCall);
+    RELEASE_ASSERT(exit->m_descriptor.m_exceptionType == ExceptionType::JSCall);
     return exit; 
 }
 
index f5309f7..75d1a17 100644 (file)
@@ -57,10 +57,11 @@ public:
 
     // These functions only make sense to be called after we've generated the OSR
     // exit thunks and allocated the OSR exit thunks' link buffer.
-    CodeLocationLabel getOrPutByIdCallOperationExceptionTarget(uint32_t stackmapRecordIndex);
+    CodeLocationLabel callOperationExceptionTarget(uint32_t stackmapRecordIndex);
     CodeLocationLabel lazySlowPathExceptionTarget(uint32_t stackmapRecordIndex);
 
     OSRExit* getByIdOSRExit(uint32_t stackmapRecordIndex);
+    OSRExit* subOSRExit(uint32_t stackmapRecordIndex);
     OSRExit* getCallOSRExit(uint32_t stackmapRecordIndex, const JSCall&);
     OSRExit* getCallOSRExit(uint32_t stackmapRecordIndex, const JSTailCall&);
     OSRExit* getCallOSRExit(uint32_t stackmapRecordIndex, const JSCallVarargs&);
index ab0b178..74eec56 100644 (file)
@@ -53,23 +53,26 @@ void ExitThunkGenerator::emitThunk(unsigned index, int32_t osrExitFromGenericUnw
     
     info.m_thunkLabel = label();
 
-    if (exit.m_descriptor.m_willArriveAtOSRExitFromGenericUnwind) {
+    Jump jumpToPushIndexFromGenericUnwind;
+    if (exit.m_descriptor.mightArriveAtOSRExitFromGenericUnwind()) {
         restoreCalleeSavesFromVMCalleeSavesBuffer();
         loadPtr(vm()->addressOfCallFrameForCatch(), framePointerRegister);
         addPtr(TrustedImm32(- static_cast<int64_t>(m_state.jitCode->stackmaps.stackSizeForLocals())), 
             framePointerRegister, stackPointerRegister);
 
-        if (exit.m_descriptor.m_isExceptionFromJSCall)
+        if (exit.m_descriptor.needsRegisterRecoveryOnGenericUnwindOSRExitPath())
             exit.recoverRegistersFromSpillSlot(*this, osrExitFromGenericUnwindStackSpillSlot);
 
-        Jump skipGetAndPutByIdSlowPathEntrance = jump();
+        jumpToPushIndexFromGenericUnwind = jump();
+    }
 
-        info.m_getAndPutByIdCallOperationExceptionOSRExitEntrance = label();
-        if (exit.m_descriptor.m_isExceptionFromGetById)
-            exit.recoverRegistersFromSpillSlot(*this, osrExitFromGenericUnwindStackSpillSlot);
-        
-        skipGetAndPutByIdSlowPathEntrance.link(this);
+    if (exit.m_descriptor.mightArriveAtOSRExitFromCallOperation()) {
+        info.m_callOperationExceptionOSRExitEntrance = label();
+        exit.recoverRegistersFromSpillSlot(*this, osrExitFromGenericUnwindStackSpillSlot);
     }
+    
+    if (exit.m_descriptor.mightArriveAtOSRExitFromGenericUnwind())
+        jumpToPushIndexFromGenericUnwind.link(this);
 
     pushToSaveImmediateWithoutTouchingRegisters(TrustedImm32(index));
     info.m_thunkJump = patchableJump();
index 53a9950..c9a689c 100644 (file)
@@ -208,17 +208,19 @@ public:
                             maxNumberOfCatchSpills = std::max(maxNumberOfCatchSpills, m_graph.localsLiveInBytecode(opCatchOrigin).bitCount());
                         break;
                     }
+                    case ArithSub:
                     case GetById:
                     case GetByIdFlush: {
-                        // We may have to flush one thing for GetByIds when the base and result
-                        // are assigned the same register. For a more comprehensive overview, look
-                        // at the comment in FTLCompile.cpp
+                        // We may have to flush one thing for GetByIds/ArithSubs when the base and result or the left/right and the result
+                        // are assigned the same register. For a more comprehensive overview, look at the comment in FTLCompile.cpp
+                        if (node->op() == ArithSub && node->binaryUseKind() != UntypedUse)
+                            break; // We only compile patchpoints for ArithSub UntypedUse.
                         CodeOrigin opCatchOrigin;
                         HandlerInfo* exceptionHandler;
                         bool willCatchException = m_graph.willCatchExceptionInMachineFrame(node->origin.forExit, opCatchOrigin, exceptionHandler);
                         if (willCatchException) {
-                            static const size_t numberOfGetByIdSpills = 1;
-                            maxNumberOfCatchSpills = std::max(maxNumberOfCatchSpills, numberOfGetByIdSpills);
+                            static const size_t numberOfGetByIdOrSubSpills = 1;
+                            maxNumberOfCatchSpills = std::max(maxNumberOfCatchSpills, numberOfGetByIdOrSubSpills);
                         }
                         break;
                     }
@@ -1543,10 +1545,17 @@ private:
             LValue right = lowJSValue(m_node->child2());
 
             // Arguments: id, bytes, target, numArgs, args...
-            LValue call = m_out.call(
-                m_out.patchpointInt64Intrinsic(),
-                m_out.constInt64(stackmapID), m_out.constInt32(sizeOfArithSub()),
-                constNull(m_out.ref8), m_out.constInt32(2), left, right);
+            StackmapArgumentList arguments;
+            arguments.append(m_out.constInt64(stackmapID));
+            arguments.append(m_out.constInt32(sizeOfArithSub()));
+            arguments.append(constNull(m_out.ref8));
+            arguments.append(m_out.constInt32(2));
+            arguments.append(left);
+            arguments.append(right);
+
+            appendOSRExitArgumentsForPatchpointIfWillCatchException(arguments, ExceptionType::SubGenerator, 3); // left, right, and result show up in the stackmap locations.
+
+            LValue call = m_out.call(m_out.patchpointInt64Intrinsic(), arguments);
             setInstructionCallingConvention(call, LLVMAnyRegCallConv);
 
             m_ftlState.arithSubs.append(ArithSubDescriptor(stackmapID, m_node->origin.semantic,
@@ -2365,7 +2374,7 @@ private:
         arguments.append(base); 
         arguments.append(value);
 
-        appendOSRExitArgumentsForPatchpointIfWillCatchException(arguments, LLVMAnyRegCallConv, 2); // 2 arguments show up in the stackmap locations: the base and the value.
+        appendOSRExitArgumentsForPatchpointIfWillCatchException(arguments, ExceptionType::PutById, 2); // 2 arguments show up in the stackmap locations: the base and the value.
 
         arguments.insert(0, m_out.constInt32(2)); 
         arguments.insert(0, constNull(m_out.ref8)); 
@@ -4589,7 +4598,7 @@ private:
         for (unsigned i = 0; i < padding; ++i)
             arguments.append(getUndef(m_out.int64));
 
-        appendOSRExitArgumentsForPatchpointIfWillCatchException(arguments, LLVMWebKitJSCallConv, 0); // No call arguments show up in the stackmap locations.
+        appendOSRExitArgumentsForPatchpointIfWillCatchException(arguments, ExceptionType::JSCall, 0); // No call arguments show up in the stackmap locations.
 
         arguments.insert(0, m_out.constInt32(1 + alignedFrameSize - JSStack::CallerFrameAndPCSize));
         arguments.insert(0, constNull(m_out.ref8));
@@ -4672,7 +4681,7 @@ private:
         ASSERT(thisArg);
         arguments.append(thisArg);
 
-        appendOSRExitArgumentsForPatchpointIfWillCatchException(arguments, LLVMCCallConv, 0); // No call arguments show up in stackmap locations.
+        appendOSRExitArgumentsForPatchpointIfWillCatchException(arguments, ExceptionType::JSCall, 0); // No call arguments show up in stackmap locations.
 
         arguments.insert(0, m_out.constInt32(2 + !!jsArguments));
         arguments.insert(0, constNull(m_out.ref8));
@@ -5054,11 +5063,8 @@ private:
 
         DFG_ASSERT(m_graph, m_node, m_origin.exitOK);
         
-        m_ftlState.jitCode->osrExitDescriptors.append(OSRExitDescriptor(
-            UncountableInvalidation, DataFormatNone, MethodOfGettingAValueProfile(),
-            m_origin.forExit, m_origin.semantic,
-            availabilityMap().m_locals.numberOfArguments(),
-            availabilityMap().m_locals.numberOfLocals()));
+
+        appendOSRExitDescriptor(UncountableInvalidation, ExceptionType::None, noValue(), nullptr, m_origin);
         
         OSRExitDescriptor& exitDescriptor = m_ftlState.jitCode->osrExitDescriptors.last();
         
@@ -6241,7 +6247,7 @@ private:
         StackmapArgumentList arguments;
         arguments.append(base);
 
-        appendOSRExitArgumentsForPatchpointIfWillCatchException(arguments, LLVMAnyRegCallConv, 2, false, true); // 2 arguments show up in the stackmap locations: the result and the base.
+        appendOSRExitArgumentsForPatchpointIfWillCatchException(arguments, ExceptionType::GetById, 2); // 2 arguments show up in the stackmap locations: the result and the base.
 
         arguments.insert(0, m_out.constInt32(1)); 
         arguments.insert(0, constNull(m_out.ref8));
@@ -7596,7 +7602,7 @@ private:
         arguments.append(m_out.constInt32(userArguments.size()));
         arguments.appendVector(userArguments);
 
-        appendOSRExitArgumentsForPatchpointIfWillCatchException(arguments, LLVMAnyRegCallConv, userArguments.size() + 1, true); // All the arguments plus the result show up in the stackmap locations.
+        appendOSRExitArgumentsForPatchpointIfWillCatchException(arguments, ExceptionType::LazySlowPath, userArguments.size() + 1); // All the arguments plus the result show up in the stackmap locations.
 
         LValue call = m_out.call(m_out.patchpointInt64Intrinsic(), arguments);
         setInstructionCallingConvention(call, LLVMAnyRegCallConv);
@@ -8845,7 +8851,7 @@ private:
         m_out.appendTo(continuation);
     }
 
-    void appendOSRExitArgumentsForPatchpointIfWillCatchException(StackmapArgumentList& arguments, LCallConv callingConvention, unsigned offsetOfExitArguments, bool isLazySlowPath = false, bool isGetBydId = false)
+    void appendOSRExitArgumentsForPatchpointIfWillCatchException(StackmapArgumentList& arguments, ExceptionType exceptionType, unsigned offsetOfExitArguments)
     {
         CodeOrigin opCatchOrigin;
         HandlerInfo* exceptionHandler;
@@ -8853,27 +8859,15 @@ private:
         if (!willCatchException)
             return;
 
-        appendOSRExitDescriptor(Uncountable, noValue(), nullptr, m_origin.withForExitAndExitOK(opCatchOrigin, true));
+        appendOSRExitDescriptor(Uncountable, exceptionType, noValue(), nullptr, m_origin.withForExitAndExitOK(opCatchOrigin, true));
         OSRExitDescriptor& exitDescriptor = m_ftlState.jitCode->osrExitDescriptors.last();
         exitDescriptor.m_semanticCodeOriginForCallFrameHeader = codeOriginDescriptionOfCallSite();
-        exitDescriptor.m_isExceptionHandler = true;
-        exitDescriptor.m_willArriveAtOSRExitFromGenericUnwind = !isLazySlowPath;
-        exitDescriptor.m_isExceptionFromLazySlowPath = isLazySlowPath;
-        exitDescriptor.m_isExceptionFromJSCall = callingConvention == LLVMWebKitJSCallConv || callingConvention == LLVMCCallConv;
-        exitDescriptor.m_isExceptionFromGetById = isGetBydId;
         exitDescriptor.m_baselineExceptionHandler = *exceptionHandler;
         exitDescriptor.m_stackmapID = m_stackmapIDs - 1;
 
         StackmapArgumentList freshList;
-        buildExitArguments(exitDescriptor, freshList, noValue(), exitDescriptor.m_codeOrigin);
+        buildExitArguments(exitDescriptor, freshList, noValue(), exitDescriptor.m_codeOrigin, offsetOfExitArguments);
         arguments.appendVector(freshList);
-
-        if (offsetOfExitArguments) {
-            for (size_t i = 0; i < exitDescriptor.m_values.size(); i++) {
-                if (exitDescriptor.m_values[i].hasIndexInStackmapLocations())
-                    exitDescriptor.m_values[i].adjustStackmapLocationsIndexByOffset(offsetOfExitArguments);
-            }
-        }
     }
 
     bool emitBranchToOSRExitIfWillCatchException(LValue hadException)
@@ -8893,10 +8887,10 @@ private:
         return m_blocks.get(block);
     }
 
-    void appendOSRExitDescriptor(ExitKind kind, FormattedValue lowValue, Node* highValue, NodeOrigin origin)
+    void appendOSRExitDescriptor(ExitKind kind, ExceptionType exceptionType, FormattedValue lowValue, Node* highValue, NodeOrigin origin)
     {
         m_ftlState.jitCode->osrExitDescriptors.append(OSRExitDescriptor(
-            kind, lowValue.format(), m_graph.methodOfGettingAValueProfileFor(highValue),
+            kind, exceptionType, lowValue.format(), m_graph.methodOfGettingAValueProfileFor(highValue),
             origin.forExit, origin.semantic,
             availabilityMap().m_locals.numberOfArguments(),
             availabilityMap().m_locals.numberOfLocals()));
@@ -8936,9 +8930,8 @@ private:
         if (failCondition == m_out.booleanFalse)
             return;
 
-        appendOSRExitDescriptor(kind, lowValue, highValue, origin);
+        appendOSRExitDescriptor(kind, isExceptionHandler ? ExceptionType::CCallException : ExceptionType::None, lowValue, highValue, origin);
         OSRExitDescriptor& exitDescriptor = m_ftlState.jitCode->osrExitDescriptors.last();
-        exitDescriptor.m_isExceptionHandler = isExceptionHandler;
 
         if (failCondition == m_out.booleanTrue) {
             emitOSRExitCall(exitDescriptor, lowValue);
@@ -8975,7 +8968,7 @@ private:
     
     void buildExitArguments(
         OSRExitDescriptor& exitDescriptor, StackmapArgumentList& arguments, FormattedValue lowValue,
-        CodeOrigin codeOrigin)
+        CodeOrigin codeOrigin, unsigned offsetOfExitArgumentsInStackmapLocations = 0)
     {
         if (!!lowValue)
             arguments.append(lowValue.value());
@@ -9010,16 +9003,21 @@ private:
                     m_graph, m_node,
                     (!(availability.isDead() && m_graph.isLiveInBytecode(VirtualRegister(operand), codeOrigin))) || m_graph.m_plan.mode == FTLForOSREntryMode);
             }
-            
-            exitDescriptor.m_values[i] = exitValueForAvailability(arguments, map, availability);
+            ExitValue exitValue = exitValueForAvailability(arguments, map, availability);
+            if (exitValue.hasIndexInStackmapLocations())
+                exitValue.adjustStackmapLocationsIndexByOffset(offsetOfExitArgumentsInStackmapLocations);
+            exitDescriptor.m_values[i] = exitValue;
         }
         
         for (auto heapPair : availabilityMap.m_heap) {
             Node* node = heapPair.key.base();
             ExitTimeObjectMaterialization* materialization = map.get(node);
+            ExitValue exitValue = exitValueForAvailability(arguments, map, heapPair.value);
+            if (exitValue.hasIndexInStackmapLocations())
+                exitValue.adjustStackmapLocationsIndexByOffset(offsetOfExitArgumentsInStackmapLocations);
             materialization->add(
                 heapPair.key.descriptor(),
-                exitValueForAvailability(arguments, map, heapPair.value));
+                exitValue);
         }
         
         if (verboseCompilationEnabled()) {
index 4a37195..7ec03bd 100644 (file)
@@ -41,25 +41,82 @@ namespace JSC { namespace FTL {
 using namespace DFG;
 
 OSRExitDescriptor::OSRExitDescriptor(
-    ExitKind exitKind, DataFormat profileDataFormat,
+    ExitKind exitKind, ExceptionType exceptionType, DataFormat profileDataFormat,
     MethodOfGettingAValueProfile valueProfile, CodeOrigin codeOrigin,
     CodeOrigin originForProfile, unsigned numberOfArguments,
     unsigned numberOfLocals)
     : m_kind(exitKind)
+    , m_exceptionType(exceptionType)
     , m_codeOrigin(codeOrigin)
     , m_codeOriginForExitProfile(originForProfile)
     , m_profileDataFormat(profileDataFormat)
     , m_valueProfile(valueProfile)
     , m_values(numberOfArguments, numberOfLocals)
     , m_isInvalidationPoint(false)
-    , m_isExceptionHandler(false)
-    , m_willArriveAtOSRExitFromGenericUnwind(false)
-    , m_isExceptionFromJSCall(false)
-    , m_isExceptionFromGetById(false)
-    , m_isExceptionFromLazySlowPath(false)
 {
 }
 
+bool OSRExitDescriptor::willArriveAtExitFromIndirectExceptionCheck() const
+{
+    switch (m_exceptionType) {
+    case ExceptionType::JSCall:
+    case ExceptionType::GetById:
+    case ExceptionType::PutById:
+    case ExceptionType::LazySlowPath:
+    case ExceptionType::SubGenerator:
+        return true;
+    default:
+        return false;
+    }
+    RELEASE_ASSERT_NOT_REACHED();
+}
+
+bool OSRExitDescriptor::mightArriveAtOSRExitFromGenericUnwind() const
+{
+    switch (m_exceptionType) {
+    case ExceptionType::JSCall:
+    case ExceptionType::GetById:
+    case ExceptionType::PutById:
+        return true;
+    default:
+        return false;
+    }
+    RELEASE_ASSERT_NOT_REACHED();
+}
+
+bool OSRExitDescriptor::mightArriveAtOSRExitFromCallOperation() const
+{
+    switch (m_exceptionType) {
+    case ExceptionType::GetById:
+    case ExceptionType::PutById:
+    case ExceptionType::SubGenerator:
+        return true;
+    default:
+        return false;
+    }
+    RELEASE_ASSERT_NOT_REACHED();
+}
+
+bool OSRExitDescriptor::needsRegisterRecoveryOnGenericUnwindOSRExitPath() const
+{
+    // Calls/PutByIds/GetByIds all have a generic unwind osr exit paths.
+    // But, GetById and PutById ICs will do register recovery themselves
+    // because they're responsible for spilling necessary registers, so
+    // they also must recover registers themselves.
+    // Calls don't work this way. We compile Calls as patchpoints in LLVM.
+    // A call patchpoint might pass us volatile registers for locations
+    // we will do value recovery on. Therefore, before we make the call,
+    // we must spill these registers. Otherwise, the call will clobber them.
+    // Therefore, the corresponding OSR exit for the call will need to
+    // recover the spilled registers.
+    return m_exceptionType == ExceptionType::JSCall;
+}
+
+bool OSRExitDescriptor::isExceptionHandler() const
+{
+    return m_exceptionType != ExceptionType::None;
+}
+
 void OSRExitDescriptor::validateReferences(const TrackedReferences& trackedReferences)
 {
     for (unsigned i = m_values.size(); i--;)
@@ -75,7 +132,7 @@ OSRExit::OSRExit(OSRExitDescriptor& descriptor, uint32_t stackmapRecordIndex)
     , m_descriptor(descriptor)
     , m_stackmapRecordIndex(stackmapRecordIndex)
 {
-    m_isExceptionHandler = descriptor.m_isExceptionHandler;
+    m_isExceptionHandler = descriptor.isExceptionHandler();
 }
 
 CodeLocationJump OSRExit::codeLocationForRepatch(CodeBlock* ftlCodeBlock) const
@@ -88,7 +145,7 @@ CodeLocationJump OSRExit::codeLocationForRepatch(CodeBlock* ftlCodeBlock) const
 
 void OSRExit::gatherRegistersToSpillForCallIfException(StackMaps& stackmaps, StackMaps::Record& record)
 {
-    RELEASE_ASSERT(m_descriptor.m_isExceptionFromJSCall);
+    RELEASE_ASSERT(m_descriptor.m_exceptionType == ExceptionType::JSCall);
 
     RegisterSet volatileRegisters = RegisterSet::volatileRegistersForJSCall();
 
@@ -122,7 +179,7 @@ void OSRExit::gatherRegistersToSpillForCallIfException(StackMaps& stackmaps, Sta
 
 void OSRExit::spillRegistersToSpillSlot(CCallHelpers& jit, int32_t stackSpillSlot)
 {
-    RELEASE_ASSERT(m_descriptor.m_isExceptionFromJSCall || m_descriptor.m_isExceptionFromGetById);
+    RELEASE_ASSERT(m_descriptor.mightArriveAtOSRExitFromGenericUnwind() || m_descriptor.mightArriveAtOSRExitFromCallOperation());
     unsigned count = 0;
     for (GPRReg reg = MacroAssembler::firstRegister(); reg <= MacroAssembler::lastRegister(); reg = MacroAssembler::nextRegister(reg)) {
         if (registersToPreserveForCallThatMightThrow.get(reg)) {
@@ -140,7 +197,7 @@ void OSRExit::spillRegistersToSpillSlot(CCallHelpers& jit, int32_t stackSpillSlo
 
 void OSRExit::recoverRegistersFromSpillSlot(CCallHelpers& jit, int32_t stackSpillSlot)
 {
-    RELEASE_ASSERT(m_descriptor.m_isExceptionFromJSCall || m_descriptor.m_isExceptionFromGetById);
+    RELEASE_ASSERT(m_descriptor.mightArriveAtOSRExitFromGenericUnwind() || m_descriptor.mightArriveAtOSRExitFromCallOperation());
     unsigned count = 0;
     for (GPRReg reg = MacroAssembler::firstRegister(); reg <= MacroAssembler::lastRegister(); reg = MacroAssembler::nextRegister(reg)) {
         if (registersToPreserveForCallThatMightThrow.get(reg)) {
index 47dc659..8bf59a6 100644 (file)
@@ -136,13 +136,30 @@ namespace FTL {
 //   intrinsics (or meta-data, or something) to inform the backend that it's safe to
 //   make the predicate passed to 'exitIf()' more truthy.
 
+enum class ExceptionType {
+    None,
+    CCallException,
+    JSCall,
+    GetById,
+    PutById,
+    LazySlowPath,
+    SubGenerator
+};
+
 struct OSRExitDescriptor {
     OSRExitDescriptor(
-        ExitKind, DataFormat profileDataFormat, MethodOfGettingAValueProfile,
+        ExitKind, ExceptionType, DataFormat profileDataFormat, MethodOfGettingAValueProfile,
         CodeOrigin, CodeOrigin originForProfile,
         unsigned numberOfArguments, unsigned numberOfLocals);
 
+    bool willArriveAtExitFromIndirectExceptionCheck() const;
+    bool mightArriveAtOSRExitFromGenericUnwind() const;
+    bool mightArriveAtOSRExitFromCallOperation() const;
+    bool needsRegisterRecoveryOnGenericUnwindOSRExitPath() const;
+    bool isExceptionHandler() const;
+
     ExitKind m_kind;
+    ExceptionType m_exceptionType;
     CodeOrigin m_codeOrigin;
     CodeOrigin m_codeOriginForExitProfile;
     CodeOrigin m_semanticCodeOriginForCallFrameHeader;
@@ -161,11 +178,6 @@ struct OSRExitDescriptor {
     uint32_t m_stackmapID;
     HandlerInfo m_baselineExceptionHandler;
     bool m_isInvalidationPoint : 1;
-    bool m_isExceptionHandler : 1;
-    bool m_willArriveAtOSRExitFromGenericUnwind : 1;
-    bool m_isExceptionFromJSCall : 1;
-    bool m_isExceptionFromGetById : 1;
-    bool m_isExceptionFromLazySlowPath : 1;
     
     void validateReferences(const TrackedReferences&);
 };
index 50c5422..93660fa 100644 (file)
@@ -39,7 +39,7 @@ struct OSRExitCompilationInfo {
     }
     
     MacroAssembler::Label m_thunkLabel;
-    MacroAssembler::Label m_getAndPutByIdCallOperationExceptionOSRExitEntrance;
+    MacroAssembler::Label m_callOperationExceptionOSRExitEntrance;
     MacroAssembler::PatchableJump m_thunkJump;
     CodeLocationLabel m_thunkAddress;
 };
index 4650ed6..4deb1e8 100644 (file)
@@ -547,8 +547,8 @@ extern "C" void* compileFTLOSRExit(ExecState* exec, unsigned exitID)
         dataLog("    Exit stackmap ID: ", exit.m_descriptor.m_stackmapID, "\n");
         dataLog("    Current call site index: ", exec->callSiteIndex().bits(), "\n");
         dataLog("    Exit is exception handler: ", exit.m_isExceptionHandler,
-            " will arrive at exit from genericUnwind(): ", exit.m_descriptor.m_willArriveAtOSRExitFromGenericUnwind
-            " will arrive at exit from lazy slow path: ", exit.m_descriptor.m_isExceptionFromLazySlowPath, "\n");
+            " might arrive at exit from genericUnwind(): ", exit.m_descriptor.mightArriveAtOSRExitFromGenericUnwind()
+            " will arrive at exit from lazy slow path: ", exit.m_descriptor.m_exceptionType == ExceptionType::LazySlowPath, "\n");
         dataLog("    Exit values: ", exit.m_descriptor.m_values, "\n");
         if (!exit.m_descriptor.m_materializations.isEmpty()) {
             dataLog("    Materializations:\n");
diff --git a/Source/JavaScriptCore/tests/stress/ftl-try-catch-arith-sub-exception.js b/Source/JavaScriptCore/tests/stress/ftl-try-catch-arith-sub-exception.js
new file mode 100644 (file)
index 0000000..c6c7185
--- /dev/null
@@ -0,0 +1,59 @@
+function assert(b) {
+    if (!b)
+        throw new Error("uh oh");
+}
+
+let flag = false;
+let o = {
+    valueOf() {
+        if (flag)
+            throw new Error("by by");
+        return 13.5;
+    }
+};
+noInline(o.valueOf);
+
+function baz() { return 1.5; }
+noInline(baz);
+
+function foo(x, o) {
+    let r = baz();
+    try {
+        r = x - o - r;
+    } catch(e) { }
+    return r;
+}
+noInline(foo);
+
+let x = 20.5;
+for (let i = 0; i < 10000; i++) {
+    assert(foo(x, o) === 5.5);
+}
+flag = true;
+assert(foo(x, o) === 1.5);
+
+
+function bar(x, o) {
+    let caughtException = false;
+    var r = null;
+    try {
+        // This tests aliasing of left/right with result register in a SubGenerator
+        // and ensures that the sub will spill the register properly and that we value
+        // recover properly.
+        r = x - o;
+    } catch(e) {
+        caughtException = true;
+        assert(r === null);
+    }
+    if (!caughtException)
+        assert(r === 7);
+    return caughtException;
+} 
+noInline(bar);
+
+flag = false;
+for (let i = 0; i < 10000; i++) {
+    assert(bar(x, o) === false);
+}
+flag = true;
+assert(bar(x, o) === true);