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.
chakra-core · Oct 1, 2017 · d2e0a4e · d2e0a4e
1 parent bf2d8af
commit d2e0a4e
Show file tree

Hide file tree

Showing 9 changed files with 192 additions and 4 deletions.
diff --git a/lib/Runtime/Base/ThreadContext.cpp b/lib/Runtime/Base/ThreadContext.cpp
@@ -136,6 +136,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
@@ -684,7 +684,7 @@ class ThreadContext sealed :
     ThreadServiceWrapper* threadServiceWrapper;
     uint functionCount;
     uint sourceInfoCount;
-
+    void * tryCatchFrameAddr;
     enum RedeferralState
     {
         InitialRedeferralState,
@@ -1259,6 +1259,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/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, 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,34 @@ 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
+        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
@@ -0,0 +1,45 @@
+//-------------------------------------------------------------------------------------------------------
+// Copyright (C) Microsoft. All rights reserved.
+// Licensed under the MIT license. See LICENSE.txt file in the project root for full license information.
+//-------------------------------------------------------------------------------------------------------
+
+function shapeyConstructor() {
+  y = iczqcn;
+}
+
+function test1() {
+  for (var w in [1,2]) {
+    try {
+      new shapeyConstructor(w);
+    } catch (e) {
+    }
+  }
+}
+
+function throwFunc() {
+   // dummy try-catch so that this function does not get inlined
+   try {
+   }
+   catch (ex) {
+   }
+   throw "ex" ;
+}
+
+function caller() {
+   throwFunc(w);
+}
+
+function test2() {
+  for (var w in [1,2]) {
+    try {
+      new caller();
+    } catch (e) {
+    }
+  }
+}
+
+test1();
+test2();
+WScript.Echo("PASS");
+
+
diff --git a/test/EH/nestedinlinestackwalkbug.js b/test/EH/nestedinlinestackwalkbug.js
@@ -0,0 +1,46 @@
+//-------------------------------------------------------------------------------------------------------
+// Copyright (C) Microsoft. All rights reserved.
+// Licensed under the MIT license. See LICENSE.txt file in the project root for full license information.
+//-------------------------------------------------------------------------------------------------------
+
+function throwFunc() {
+   // dummy try-catch so that this function does not get inlined
+   try {
+   }
+   catch (ex) {
+   }
+   throw "ex" ;
+}
+
+function caller() {
+   throwFunc(w);
+}
+
+function shapeyConstructor() {
+  y = iczqcn;
+}
+
+function test() {
+  for (var w in [1,2]) {
+    try {
+      new caller();
+    } catch (e) {
+    }
+  }
+}
+
+function toptest() {
+  try {
+    test();
+    new shapeyConstructor();
+  }
+  catch (ex) {
+  }
+}
+
+toptest();
+toptest();
+toptest();
+WScript.Echo("PASS");
+
+
diff --git a/test/EH/rlexe.xml b/test/EH/rlexe.xml
@@ -130,4 +130,14 @@
       <compile-flags> -force:inline </compile-flags>
     </default>
   </test>
+  <test>
+    <default>
+      <files>inlinestackwalkbug.js</files>
+    </default>
+  </test>
+  <test>
+    <default>
+      <files>nestedinlinestackwalkbug.js</files>
+    </default>
+  </test>
 </regress-exe>