Turn on inlining for functions with trycatch/tryfinally

meg-gupta · meg-gupta · commit e0bed612a773 · 2017-10-11T13:27:41.000-07:00
- Avoid copy prop s0 (return symbol) into inlined functions

- Clear inlinee callinfo : When there is an exception in an inlinee which is inside an EH region, we end up leaving datastructures in stackwalker dirty.
If the EH handler was a try catch, and if the same code is executed in a loop  in the interprerter after BailOnNoException,
the stack walker due to dirty datastructures assumes that there are inlinee frames on the stack.
diff --git a/lib/Backend/BailOut.cpp b/lib/Backend/BailOut.cpp
@@ -1608,6 +1608,10 @@ BailOutRecord::BailOutHelper(Js::JavascriptCallStackLayout * layout, Js::ScriptF
     {
         newInstance->OrFlags(Js::InterpreterStackFrameFlags_ProcessingBailOutOnArrayAccessHelperCall);
     }
+    if (isInlinee)
+    {
+        newInstance->OrFlags(Js::InterpreterStackFrameFlags_FromInlineeCodeInEHBailOut);
+    }
 
     ThreadContext *threadContext = newInstance->GetScriptContext()->GetThreadContext();
 
diff --git a/lib/Backend/Func.h b/lib/Backend/Func.h
@@ -208,7 +208,7 @@ class Func
 
     bool DoInline() const
     {
-        return DoGlobOpt() && !GetTopFunc()->HasTry();
+        return DoGlobOpt();
     }
 
     bool DoOptimizeTry() const
diff --git a/lib/Backend/GlobOpt.cpp b/lib/Backend/GlobOpt.cpp
@@ -3674,6 +3674,18 @@ GlobOpt::CopyProp(IR::Opnd *opnd, IR::Instr *instr, Value *val, IR::IndirOpnd *p
 
     ValueInfo *valueInfo = val->GetValueInfo();
 
+
+    if (this->func->HasFinally())
+    {
+        // s0 = undefined was added on functions with early exit in try-finally functions, that can get copy-proped and case incorrect results
+        if (instr->m_opcode == Js::OpCode::ArgOut_A_Inline && valueInfo->GetSymStore() &&
+            valueInfo->GetSymStore()->m_id == 0)
+        {
+            // We don't want to copy-prop s0 (return symbol) into inlinee code
+            return opnd;
+        }
+    }
+
     // Constant prop?
     int32 intConstantValue;
     int64 int64ConstantValue;
diff --git a/lib/Backend/Inline.cpp b/lib/Backend/Inline.cpp
@@ -1399,8 +1399,6 @@ bool
 Inline::TryOptimizeCallInstrWithFixedMethod(IR::Instr *callInstr, const FunctionJITTimeInfo * inlineeInfo, bool isPolymorphic, bool isBuiltIn, bool isCtor, bool isInlined, bool &safeThis,
                                             bool dontOptimizeJustCheck, uint i /*i-th inlinee at a polymorphic call site*/)
 {
-    Assert(!callInstr->m_func->GetJITFunctionBody()->HasTry());
-
     if (PHASE_OFF(Js::FixedMethodsPhase, callInstr->m_func))
     {
         return false;
diff --git a/lib/Backend/InliningDecider.cpp b/lib/Backend/InliningDecider.cpp
@@ -24,17 +24,6 @@ bool InliningDecider::InlineIntoTopFunc() const
         return false;
     }
 
-    if (this->topFunc->GetHasTry())
-    {
-#if defined(DBG_DUMP) || defined(ENABLE_DEBUG_CONFIG_OPTIONS)
-        char16 debugStringBuffer[MAX_FUNCTION_BODY_DEBUG_STRING_SIZE];
-#endif
-        INLINE_TESTTRACE(_u("INLINING: Skip Inline: Has try\tCaller: %s (%s)\n"), this->topFunc->GetDisplayName(),
-            this->topFunc->GetDebugNumberSet(debugStringBuffer));
-        // Glob opt doesn't run on function with try, so we can't generate bailout for it
-        return false;
-    }
-
     return InlineIntoInliner(topFunc);
 }
 
@@ -218,14 +207,6 @@ Js::FunctionInfo *InliningDecider::Inline(Js::FunctionBody *const inliner, Js::F
             return nullptr;
         }
 
-        if (inlinee->GetHasTry())
-        {
-            INLINE_TESTTRACE(_u("INLINING: Skip Inline: Has try\tInlinee: %s (%s)\tCaller: %s (%s)\n"),
-                inlinee->GetDisplayName(), inlinee->GetDebugNumberSet(debugStringBuffer), inliner->GetDisplayName(),
-                inliner->GetDebugNumberSet(debugStringBuffer2));
-            return nullptr;
-        }
-
         // This is a hard limit as the argOuts array is statically sized.
         if (inlinee->GetInParamsCount() > Js::InlineeCallInfo::MaxInlineeArgoutCount)
         {
diff --git a/lib/Backend/LinearScan.cpp b/lib/Backend/LinearScan.cpp
@@ -1412,6 +1412,14 @@ LinearScan::FillBailOutRecord(IR::Instr * instr)
         // To indicate this is a subsequent bailout from an inlinee
         bailOutRecord->bailOutOpcode = Js::OpCode::InlineeEnd;
 #endif
+        if (this->func->HasTry())
+        {
+            RegionType currentRegionType = this->currentRegion->GetType();
+            if (currentRegionType == RegionTypeTry || currentRegionType == RegionTypeCatch || currentRegionType == RegionTypeFinally)
+            {
+                bailOutRecord->ehBailoutData = this->currentRegion->ehBailoutData;
+            }
+        }
         funcBailOutData[funcIndex].bailOutRecord->parent = bailOutRecord;
         funcIndex--;
         funcBailOutData[funcIndex].bailOutRecord = bailOutRecord;
diff --git a/lib/Backend/Lower.cpp b/lib/Backend/Lower.cpp
@@ -19995,7 +19995,13 @@ Lowerer::EHBailoutPatchUp()
                 this->InsertReturnThunkForRegion(this->currentRegion, restoreReturnValueFromBailoutLabel);
                 if (instr->HasBailOutInfo())
                 {
-                    this->SetHasBailedOut(instr);
+                    if (instr->GetBailOutInfo()->bailOutFunc == this->m_func)
+                    {
+                        // We dont set this bit for inlined code, if there was a bailout in the inlined code,
+                        // and an exception was thrown, we want the caller's handler to handle the exception accordingly.
+                        // TODO : Revisit when we start inlining functions with try-catch/try-finally
+                        this->SetHasBailedOut(instr);
+                    }
                     tmpInstr = this->EmitEHBailoutStackRestore(instr);
                     this->EmitSaveEHBailoutReturnValueAndJumpToRetThunk(tmpInstr);
                     if (!restoreReturnFromBailoutEmitted)
diff --git a/lib/Runtime/Base/ThreadContext.cpp b/lib/Runtime/Base/ThreadContext.cpp
@@ -125,6 +125,7 @@ ThreadContext::ThreadContext(AllocationPolicyManager * allocationPolicyManager,
     entryExitRecord(nullptr),
     leafInterpreterFrame(nullptr),
     threadServiceWrapper(nullptr),
+    tryCatchFrameAddr(nullptr),
     temporaryArenaAllocatorCount(0),
     temporaryGuestArenaAllocatorCount(0),
     crefSContextForDiag(0),
diff --git a/lib/Runtime/Base/ThreadContext.h b/lib/Runtime/Base/ThreadContext.h
@@ -678,7 +678,7 @@ class ThreadContext sealed :
     ThreadServiceWrapper* threadServiceWrapper;
     uint functionCount;
     uint sourceInfoCount;
-
+    void * tryCatchFrameAddr;
     enum RedeferralState
     {
         InitialRedeferralState,
@@ -1253,6 +1253,9 @@ class ThreadContext sealed :
     uint EnterScriptStart(Js::ScriptEntryExitRecord *, bool doCleanup);
     void EnterScriptEnd(Js::ScriptEntryExitRecord *, bool doCleanup);
 
+    void * GetTryCatchFrameAddr() { return this->tryCatchFrameAddr; }
+    void SetTryCatchFrameAddr(void * frameAddr) { this->tryCatchFrameAddr = frameAddr; }
+
     template <bool leaveForHost>
     void LeaveScriptStart(void *);
     template <bool leaveForHost>
diff --git a/lib/Runtime/Language/EHBailoutData.h b/lib/Runtime/Language/EHBailoutData.h
@@ -33,7 +33,15 @@ namespace Js
             this->parent = parent;
             this->child = nullptr;
         }
-
+        EHBailoutData(EHBailoutData * other)
+        {
+            this->nestingDepth = other->nestingDepth;
+            this->catchOffset = other->catchOffset;
+            this->finallyOffset = other->finallyOffset;
+            this->ht = other->ht;
+            this->parent = other->parent;
+            this->child = nullptr;
+        }
 #if ENABLE_NATIVE_CODEGEN
         void Fixup(NativeCodeData::DataChunk* chunkList)
         {
diff --git a/lib/Runtime/Language/InterpreterStackFrame.cpp b/lib/Runtime/Language/InterpreterStackFrame.cpp
@@ -3284,7 +3284,7 @@ namespace Js
         } autoRestore(this);
 #endif
 
-        if (this->ehBailoutData)
+        if (this->ehBailoutData && !(m_flags & InterpreterStackFrameFlags_FromInlineeCodeInEHBailOut))
         {
             if ((m_flags & Js::InterpreterStackFrameFlags_FromBailOut) && !(m_flags & InterpreterStackFrameFlags_ProcessingBailOutFromEHCode))
             {
@@ -6752,7 +6752,6 @@ const byte * InterpreterStackFrame::OP_ProfiledLoopBodyStart(const byte * ip)
                 // Generator return scenario, so no need to go into the catch block and we must rethrow to propagate the exception to down level
                 JavascriptExceptionOperators::DoThrow(exception, scriptContext);
             }
-
             if (catchOffset != 0)
             {
                 exception = exception->CloneIfStaticExceptionObject(scriptContext);
diff --git a/lib/Runtime/Language/InterpreterStackFrame.h b/lib/Runtime/Language/InterpreterStackFrame.h
@@ -27,6 +27,7 @@ namespace Js
         InterpreterStackFrameFlags_FromBailOut = 8,
         InterpreterStackFrameFlags_ProcessingBailOutOnArrayAccessHelperCall = 0x10,
         InterpreterStackFrameFlags_ProcessingBailOutFromEHCode = 0x20,
+        InterpreterStackFrameFlags_FromInlineeCodeInEHBailOut = 0x40,
         InterpreterStackFrameFlags_All = 0xFFFF,
     };
 
diff --git a/lib/Runtime/Language/JavascriptExceptionOperators.cpp b/lib/Runtime/Language/JavascriptExceptionOperators.cpp
@@ -60,6 +60,18 @@ namespace Js
         m_threadContext->SetIsUserCode(m_previousCatchHandlerToUserCodeStatus);
     }
 
+    JavascriptExceptionOperators::TryCatchFrameAddrStack::TryCatchFrameAddrStack(ScriptContext* scriptContext, void *frameAddr)
+    {
+        m_threadContext = scriptContext->GetThreadContext();
+        m_prevTryCatchFrameAddr = m_threadContext->GetTryCatchFrameAddr();
+        scriptContext->GetThreadContext()->SetTryCatchFrameAddr(frameAddr);
+    }
+
+    JavascriptExceptionOperators::TryCatchFrameAddrStack::~TryCatchFrameAddrStack()
+    {
+        m_threadContext->SetTryCatchFrameAddr(m_prevTryCatchFrameAddr);
+    }
+
     bool JavascriptExceptionOperators::CrawlStackForWER(Js::ScriptContext& scriptContext)
     {
         return Js::Configuration::Global.flags.WERExceptionSupport && !scriptContext.GetThreadContext()->HasCatchHandler();
@@ -87,6 +99,7 @@ namespace Js
         try
         {
             Js::JavascriptExceptionOperators::AutoCatchHandlerExists autoCatchHandlerExists(scriptContext);
+            Js::JavascriptExceptionOperators::TryCatchFrameAddrStack tryCatchFrameAddrStack(scriptContext, frame);
             continuation = amd64_CallWithFakeFrame(tryAddr, frame, spillSize, argsSize);
         }
         catch (const Js::JavascriptException& err)
@@ -215,6 +228,7 @@ namespace Js
         try
         {
             Js::JavascriptExceptionOperators::AutoCatchHandlerExists autoCatchHandlerExists(scriptContext);
+            Js::JavascriptExceptionOperators::TryCatchFrameAddrStack tryCatchFrameAddrStack(scriptContext, framePtr);
 #if defined(_M_ARM)
             continuation = arm_CallEhFrame(tryAddr, framePtr, localsPtr, argsSize);
 #elif defined(_M_ARM64)
@@ -365,6 +379,7 @@ namespace Js
         try
         {
             Js::JavascriptExceptionOperators::AutoCatchHandlerExists autoCatchHandlerExists(scriptContext);
+            Js::JavascriptExceptionOperators::TryCatchFrameAddrStack tryCatchFrameAddrStack(scriptContext, framePtr);
 
             // Adjust the frame pointer and call into the try.
             // If the try completes without raising an exception, it will pass back the continuation address.
@@ -875,7 +890,16 @@ namespace Js
 
         JavascriptExceptionOperators::ThrowExceptionObject(exceptionObject, scriptContext, /*considerPassingToDebugger=*/ true, /*returnAddress=*/ nullptr, resetStack);
     }
+#if ENABLE_NATIVE_CODEGEN
+    void JavascriptExceptionOperators::WalkStackForCleaningUpInlineeInfo(ScriptContext *scriptContext, PVOID returnAddress)
+    {
+        JavascriptStackWalker walker(scriptContext, /*useEERContext*/ true, returnAddress);
 
+        // We have to walk the inlinee frames and clear callinfo count on them on an exception
+        // At this point inlinedFrameWalker is closed, so we should build it again by calling InlinedFrameWalker::FromPhysicalFrame
+        walker.WalkAndClearInlineeFrameCallInfoOnException();
+    }
+#endif
     void
         JavascriptExceptionOperators::WalkStackForExceptionContext(ScriptContext& scriptContext, JavascriptExceptionContext& exceptionContext, Var thrownObject, uint64 stackCrawlLimit, PVOID returnAddress, bool isThrownException, bool resetSatck)
     {
@@ -1090,6 +1114,20 @@ namespace Js
                 WalkStackForExceptionContext(*scriptContext, exceptionContext, thrownObject, StackCrawlLimitOnThrow(thrownObject, *scriptContext), returnAddress, /*isThrownException=*/ true, resetStack);
                 exceptionObject->FillError(exceptionContext, scriptContext);
                 AddStackTraceToObject(thrownObject, exceptionContext.GetStackTrace(), *scriptContext, /*isThrownException=*/ true, resetStack);
+
+                // We need to clear callinfo on inlinee virtual frames on an exception.
+                // We now allow inlining of functions into callers that have try-catch/try-finally.
+                // When there is an exception inside the inlinee with caller having a try-catch, clear the inlinee callinfo by walking the stack.
+                // If not, we might have the try-catch inside a loop, and when we execute the loop next time in the interpreter on BailOnException,
+                // we will see inlined frames as being present even though they are not, because we depend on FrameInfo's callinfo to tell if an inlinee is on the stack,
+                // and we haven't cleared those bits due to the exception
+
+#if ENABLE_NATIVE_CODEGEN
+                if (scriptContext->GetThreadContext()->GetTryCatchFrameAddr() != nullptr)
+                {
+                    WalkStackForCleaningUpInlineeInfo(scriptContext, returnAddress);
+                }
+#endif
             }
             Assert(!scriptContext ||
                    // If we disabled implicit calls and we did record an implicit call, do not throw.
diff --git a/lib/Runtime/Language/JavascriptExceptionOperators.h b/lib/Runtime/Language/JavascriptExceptionOperators.h
@@ -43,6 +43,17 @@ namespace Js
             ~AutoCatchHandlerExists();
         };
 
+        class TryCatchFrameAddrStack
+        {
+          private:
+            void * m_prevTryCatchFrameAddr;
+            ThreadContext* m_threadContext;
+
+          public:
+            TryCatchFrameAddrStack(ScriptContext* scriptContext, void *frameAddr);
+            ~TryCatchFrameAddrStack();
+        };
+
         static void __declspec(noreturn) OP_Throw(Var object, ScriptContext* scriptContext);
         static void __declspec(noreturn) Throw(Var object, ScriptContext* scriptContext);
         static void __declspec(noreturn) ThrowExceptionObject(Js::JavascriptExceptionObject* exceptionObject, ScriptContext* scriptContext, bool considerPassingToDebugger = false, PVOID returnAddress = NULL, bool resetStack = false);
@@ -80,6 +91,9 @@ namespace Js
         static Var ThrowTypeErrorRestrictedPropertyAccessor(RecyclableObject* function, CallInfo callInfo, ...);
         static Var StackTraceAccessor(RecyclableObject* function, CallInfo callInfo, ...);
         static void WalkStackForExceptionContext(ScriptContext& scriptContext, JavascriptExceptionContext& exceptionContext, Var thrownObject, uint64 stackCrawlLimit, PVOID returnAddress, bool isThrownException = true, bool resetSatck = false);
+#if ENABLE_NATIVE_CODEGEN
+        static void WalkStackForCleaningUpInlineeInfo(ScriptContext *scriptContext, PVOID returnAddress);
+#endif
         static void AddStackTraceToObject(Var obj, JavascriptExceptionContext::StackTrace* stackTrace, ScriptContext& scriptContext, bool isThrownException = true, bool resetSatck = false);
         static uint64 StackCrawlLimitOnThrow(Var thrownObject, ScriptContext& scriptContext);
 
diff --git a/lib/Runtime/Language/JavascriptStackWalker.cpp b/lib/Runtime/Language/JavascriptStackWalker.cpp
@@ -616,7 +616,35 @@ namespace Js
         }
         return nullptr;
     }
-
+#if ENABLE_NATIVE_CODEGEN
+    void JavascriptStackWalker::WalkAndClearInlineeFrameCallInfoOnException()
+    {
+        // Walk the stack and when we find the first JavascriptFrame, we clear the inlinee's callinfo for this frame
+        // It is sufficient we stop at the first Javascript frame which had the enclosing try-catch
+        // TODO : Revisit when we start inlining functions with try-catch/try-finally
+        while (this->Walk(true))
+        {
+            if (this->IsJavascriptFrame())
+            {
+                if (!this->isNativeLibraryFrame)
+                {
+                    if (HasInlinedFramesOnStack())
+                    {
+                        for (int index = inlinedFrameWalker.GetFrameCount() - 1; index >= 0; index--)
+                        {
+                            auto inlinedFrame = inlinedFrameWalker.GetFrameAtIndex(index);
+                            inlinedFrame->callInfo.Clear();
+                        }
+                    }
+                    if (this->currentFrame.GetFrame() == this->scriptContext->GetThreadContext()->GetTryCatchFrameAddr())
+                    {
+                        break;
+                    }
+                }
+            }
+        }
+    }
+#endif
     // Note: noinline is to make sure that when we unwind to the unwindToAddress, there is at least one frame to unwind.
     _NOINLINE
     JavascriptStackWalker::JavascriptStackWalker(ScriptContext * scriptContext, bool useEERContext, PVOID returnAddress, bool _forceFullWalk /*=false*/) :
diff --git a/lib/Runtime/Language/JavascriptStackWalker.h b/lib/Runtime/Language/JavascriptStackWalker.h
@@ -114,6 +114,7 @@ namespace Js
         uint32                  GetBottomMostInlineeOffset() const;
         Js::JavascriptFunction *GetBottomMostFunctionObject() const;
         void                    FinalizeStackValues(__in_ecount(argCount) Js::Var args[], size_t argCount) const;
+        int32                   GetFrameCount() { return frameCount; }
 
     private:
         enum {
@@ -135,11 +136,13 @@ namespace Js
 
         };
 
-        void Initialize(int32 frameCount, __in_ecount(frameCount) InlinedFrame **frames, Js::ScriptFunction *parent);
+    public:
+        InlinedFrame *const     GetFrameAtIndex(signed index) const;
 
+    private:
+        void Initialize(int32 frameCount, __in_ecount(frameCount) InlinedFrame **frames, Js::ScriptFunction *parent);
         void MoveNext();
         InlinedFrame *const GetCurrentFrame() const;
-        InlinedFrame *const GetFrameAtIndex(signed index) const;
 
         Js::ScriptFunction *parentFunction;
         InlinedFrame          **frames;
@@ -233,6 +236,7 @@ namespace Js
         void ClearCachedInternalFrameInfo();
         void SetCachedInternalFrameInfo(InternalFrameType frameType, JavascriptFunction* function, bool hasInlinedFramesOnStack, bool prevIntFrameIsFromBailout);
         InternalFrameInfo GetCachedInternalFrameInfo() const { return this->lastInternalFrameInfo; }
+        void WalkAndClearInlineeFrameCallInfoOnException();
 #endif
         bool IsCurrentPhysicalFrameForLoopBody() const;
 
diff --git a/test/EH/inlinestackwalkbug.js b/test/EH/inlinestackwalkbug.js
diff --git a/test/EH/nestedinlinestackwalkbug.js b/test/EH/nestedinlinestackwalkbug.js
diff --git a/test/EH/rlexe.xml b/test/EH/rlexe.xml
diff --git a/test/EH/tfinlinebug.js b/test/EH/tfinlinebug.js

Original file line number	Diff line number	Diff line change
`@@ -1608,6 +1608,10 @@ BailOutRecord::BailOutHelper(Js::JavascriptCallStackLayout * layout, Js::ScriptF`
`1608`	`1608`	`{`
`1609`	`1609`	`newInstance->OrFlags(Js::InterpreterStackFrameFlags_ProcessingBailOutOnArrayAccessHelperCall);`
`1610`	`1610`	`}`
	`1611`	`+ if (isInlinee)`
	`1612`	`+ {`
	`1613`	`+ newInstance->OrFlags(Js::InterpreterStackFrameFlags_FromInlineeCodeInEHBailOut);`
	`1614`	`+ }`
`1611`	`1615`
`1612`	`1616`	`ThreadContext *threadContext = newInstance->GetScriptContext()->GetThreadContext();`
`1613`	`1617`
Original file line number	Diff line number	Diff line change
`@@ -208,7 +208,7 @@ class Func`
`208`	`208`
`209`	`209`	`bool DoInline() const`
`210`	`210`	`{`
`211`		`- return DoGlobOpt() && !GetTopFunc()->HasTry();`
	`211`	`+ return DoGlobOpt();`
`212`	`212`	`}`
`213`	`213`
`214`	`214`	`bool DoOptimizeTry() const`
Original file line number	Diff line number	Diff line change
`@@ -1399,8 +1399,6 @@ bool`
`1399`	`1399`	`Inline::TryOptimizeCallInstrWithFixedMethod(IR::Instr callInstr, const FunctionJITTimeInfo inlineeInfo, bool isPolymorphic, bool isBuiltIn, bool isCtor, bool isInlined, bool &safeThis,`
`1400`	`1400`	`bool dontOptimizeJustCheck, uint i /i-th inlinee at a polymorphic call site/)`
`1401`	`1401`	`{`
`1402`		`- Assert(!callInstr->m_func->GetJITFunctionBody()->HasTry());`
`1403`		`-`
`1404`	`1402`	`if (PHASE_OFF(Js::FixedMethodsPhase, callInstr->m_func))`
`1405`	`1403`	`{`
`1406`	`1404`	`return false;`