From: Fadi Hanna Date: Thu, 25 Apr 2019 14:17:19 +0000 (-0700) Subject: Fixing 23941 (#24199) X-Git-Tag: accepted/tizen/unified/20190813.215958~42^2~417 X-Git-Url: http://review.tizen.org/git/?a=commitdiff_plain;h=e966022128c67163e04be4ab2360d314d7d9c062;p=platform%2Fupstream%2Fcoreclr.git Fixing 23941 (#24199) * Fixing 23941 The issue here is that in R2R code, unlike jitted IL code, the pinvoke calls are wrapped by a pair of calls to the JIT_PInvokeBegin and JIT_PInvokeEnd helpers, which link and unlink the InlinedCallFrame to the current thread. We do not initialize and link the ICF in the method prolog, and unlink it in the epilog, like jitted code does. We do this in the JIT helpers, right before/after the pinvoke call. When an exception is thrown, the JIT_PInvokeEnd helper will be skipped since execution will resume at the nearest valid catch block, and the ICF will remain linked to the thread, which poses a problem if the method attempts another pinvoke operation (it will try to link the ICF which is already the top frame, and we'll end up in an infinite loop). Therefore, for the R2R case, we need to pop the ICF from the chain during exception unwinding. --- diff --git a/src/vm/codeman.cpp b/src/vm/codeman.cpp index 00ed67a..f475541 100644 --- a/src/vm/codeman.cpp +++ b/src/vm/codeman.cpp @@ -4357,6 +4357,31 @@ BOOL ExecutionManager::IsManagedCodeWorker(PCODE currentPC) return FALSE; } +//************************************************************************** +// Assumes that it is safe not to take it the ExecutionManager reader/writer lock +BOOL ExecutionManager::IsReadyToRunCode(PCODE currentPC) +{ + CONTRACTL{ + NOTHROW; + GC_NOTRIGGER; + } CONTRACTL_END; + + // This may get called for arbitrary code addresses. Note that the lock is + // taken over the call to JitCodeToMethodInfo too so that nobody pulls out + // the range section from underneath us. + +#ifdef FEATURE_READYTORUN + RangeSection * pRS = GetRangeSection(currentPC); + if (pRS != NULL && (pRS->flags & RangeSection::RANGE_SECTION_READYTORUN)) + { + if (dac_cast(pRS->pjit)->JitCodeToMethodInfo(pRS, currentPC, NULL, NULL)) + return TRUE; + } +#endif + + return FALSE; +} + #ifndef DACCESS_COMPILE //************************************************************************** diff --git a/src/vm/codeman.h b/src/vm/codeman.h index 04e7dd1..1dc8d8f 100644 --- a/src/vm/codeman.h +++ b/src/vm/codeman.h @@ -1245,6 +1245,9 @@ public: // Special version with profiler hook static BOOL IsManagedCode(PCODE currentPC, HostCallPreference hostCallPreference, BOOL *pfFailedReaderLock); + // Returns true if currentPC is ready to run codegen + static BOOL IsReadyToRunCode(PCODE currentPC); + // Returns method's start address for a given PC static PCODE GetCodeStartAddress(PCODE currentPC); diff --git a/src/vm/exceptionhandling.cpp b/src/vm/exceptionhandling.cpp index 8574caf..948913c 100644 --- a/src/vm/exceptionhandling.cpp +++ b/src/vm/exceptionhandling.cpp @@ -1839,6 +1839,12 @@ CLRUnwindStatus ExceptionTracker::ProcessOSExceptionNotification( // // Thus, ICF::ExceptionUnwind should not do anything significant. If any of these assumptions // break, then the next best thing will be to make the JIT link/unlink the frame dynamically. + // + // If the current method executing is from precompiled ReadyToRun code, then the above is no longer + // applicable because each PInvoke is wrapped by calls to the JIT_PInvokeBegin and JIT_PInvokeEnd + // helpers, which push and pop the ICF to the current thread. Unlike jitted code, the ICF is not + // linked during the method prolog, and unlinked at the epilog (it looks more like the X64 case). + // In that case, we need to unlink the ICF during unwinding here. if (fTargetUnwind && (pFrame->GetVTablePtr() == InlinedCallFrame::GetMethodFrameVPtr())) { @@ -1851,9 +1857,21 @@ CLRUnwindStatus ExceptionTracker::ProcessOSExceptionNotification( ((UINT_PTR)pICF < uCallerSP)) { pICFForUnwindTarget = pFrame; + + // When unwinding an exception in ReadyToRun, the JIT_PInvokeEnd helper which unlinks the ICF from + // the thread will be skipped. This is because unlike jitted code, each pinvoke is wrapped by calls + // to the JIT_PInvokeBegin and JIT_PInvokeEnd helpers, which push and pop the ICF on the thread. The + // ICF is not linked at the method prolog and unlined at the epilog when running R2R code. Since the + // JIT_PInvokeEnd helper will be skipped, we need to unlink the ICF here. If the executing method + // has another pinovoke, it will re-link the ICF again when the JIT_PInvokeBegin helper is called + + if (ExecutionManager::IsReadyToRunCode(((InlinedCallFrame*)pFrame)->m_pCallerReturnAddress)) + { + pICFForUnwindTarget = pICFForUnwindTarget->Next(); + } } } -#endif // defined(_TARGET_ARM_) +#endif // USE_PER_FRAME_PINVOKE_INIT cfThisFrame.CheckGSCookies(); diff --git a/src/vm/i386/excepx86.cpp b/src/vm/i386/excepx86.cpp index ee131c8..01c216d 100644 --- a/src/vm/i386/excepx86.cpp +++ b/src/vm/i386/excepx86.cpp @@ -1868,6 +1868,21 @@ NOINLINE LPVOID COMPlusEndCatchWorker(Thread * pThread) // Sync managed exception state, for the managed thread, based upon any active exception tracker pThread->SyncManagedExceptionState(fIsDebuggerHelperThread); + if (InlinedCallFrame::FrameHasActiveCall(pThread->m_pFrame)) + { + // When unwinding an exception in ReadyToRun, the JIT_PInvokeEnd helper which unlinks the ICF from + // the thread will be skipped. This is because unlike jitted code, each pinvoke is wrapped by calls + // to the JIT_PInvokeBegin and JIT_PInvokeEnd helpers, which push and pop the ICF on the thread. The + // ICF is not linked at the method prolog and unlined at the epilog when running R2R code. Since the + // JIT_PInvokeEnd helper will be skipped, we need to unlink the ICF here. If the executing method + // has another pinovoke, it will re-link the ICF again when the JIT_PInvokeBegin helper is called + + if (ExecutionManager::IsReadyToRunCode(((InlinedCallFrame*)pThread->m_pFrame)->m_pCallerReturnAddress)) + { + pThread->m_pFrame->Pop(pThread); + } + } + LOG((LF_EH, LL_INFO1000, "COMPlusPEndCatch: esp=%p\n", esp)); return esp; diff --git a/tests/src/Interop/NativeLibraryResolveCallback/CallbackStressTest.cs b/tests/src/Interop/NativeLibraryResolveCallback/CallbackStressTest.cs new file mode 100644 index 0000000..bcbf9d7 --- /dev/null +++ b/tests/src/Interop/NativeLibraryResolveCallback/CallbackStressTest.cs @@ -0,0 +1,200 @@ +// Licensed to the .NET Foundation under one or more agreements. +// The .NET Foundation licenses this file to you under the MIT license. +// See the LICENSE file in the project root for more information. +using System; +using System.IO; +using System.Reflection; +using System.Diagnostics; +using System.Runtime.InteropServices; +using System.Runtime.CompilerServices; +using System.Threading; + +using Console = Internal.Console; + +[assembly: DefaultDllImportSearchPaths(DllImportSearchPath.SafeDirectories)] +public class CallbackStressTest +{ + static volatile bool s_RunGC = true; + + static int s_LoopCounter = 25; + static int s_FinallyCalled = 0; + static int s_CatchCalled = 0; + static int s_OtherExceptionCatchCalled = 0; + static int s_SEHExceptionCatchCalled = 0; + static int s_WrongPInvokesExecuted = 0; + static int s_PInvokesExecuted = 0; + + public static void Collector() + { + while(s_RunGC) GC.Collect(); + } + + [MethodImpl(MethodImplOptions.NoInlining)] + public static void SetResolve() + { + Console.WriteLine("Setting PInvoke Resolver"); + + DllImportResolver resolver = + (string libraryName, Assembly asm, DllImportSearchPath? dllImportSearchPath) => + { + if (dllImportSearchPath != DllImportSearchPath.System32) + { + Console.WriteLine($"Unexpected dllImportSearchPath: {dllImportSearchPath.ToString()}"); + throw new ArgumentException(); + } + + return NativeLibrary.Load("ResolveLib", asm, null); + }; + + NativeLibrary.SetDllImportResolver( + Assembly.GetExecutingAssembly(), + resolver); + } + + [MethodImpl(MethodImplOptions.NoInlining)] + public static void DoCall() + { + NativeSum(10, 10); + s_WrongPInvokesExecuted++; + } + + [MethodImpl(MethodImplOptions.NoInlining)] + public static void DoCallTryCatch(bool shouldThrow) + { + try + { + var a = NativeSum(10, 10); + if (shouldThrow) + s_WrongPInvokesExecuted++; + else + s_PInvokesExecuted += (a == 20 ? 1 : 0); + } + catch (DllNotFoundException) { s_CatchCalled++; } + + throw new ArgumentException(); + } + + [MethodImpl(MethodImplOptions.NoInlining)] + public static void DoCallTryRethrowInCatch() + { + try + { + var a = NativeSum(10, 10); + s_WrongPInvokesExecuted++; + } + catch (DllNotFoundException) { s_CatchCalled++; throw; } + } + + [MethodImpl(MethodImplOptions.NoInlining)] + public static void DoCallTryRethrowDifferentExceptionInCatch() + { + try + { + var a = NativeSum(10, 10); + s_WrongPInvokesExecuted++; + } + catch (DllNotFoundException) { s_CatchCalled++; throw new InvalidOperationException(); } + } + + [MethodImpl(MethodImplOptions.NoInlining)] + public static void DoCallTryFinally() + { + try + { + NativeSum(10, 10); + s_WrongPInvokesExecuted++; + } + finally { s_FinallyCalled++; } + } + + [MethodImpl(MethodImplOptions.NoInlining)] + public static void ManualRaiseException() + { +#if WINDOWS + try + { + RaiseException(5, 0, 0, IntPtr.Zero); + } + catch(SEHException ex) { s_SEHExceptionCatchCalled++; } +#else + // TODO: test on Unix when implementing pinvoke inlining + s_SEHExceptionCatchCalled++; +#endif + } + + public static int Main() + { + new Thread(Collector).Start(); + + for(int i = 0; i < s_LoopCounter; i++) + { + try + { + NativeSum(10, 10); + s_WrongPInvokesExecuted++; + } + catch (DllNotFoundException) { s_CatchCalled++; } + + try { DoCall(); } + catch (DllNotFoundException) { s_CatchCalled++; } + + try { DoCallTryFinally(); } + catch (DllNotFoundException) { s_CatchCalled++; } + + try { DoCallTryCatch(true); } + catch (ArgumentException) { s_OtherExceptionCatchCalled++; } + + try { DoCallTryRethrowInCatch(); } + catch (DllNotFoundException) { s_CatchCalled++; } + + try { DoCallTryRethrowDifferentExceptionInCatch(); } + catch (InvalidOperationException) { s_OtherExceptionCatchCalled++; } + + ManualRaiseException(); + } + + SetResolve(); + + for(int i = 0; i < s_LoopCounter; i++) + { + var a = NativeSum(10, 10); + var b = NativeSum(10, 10); + s_PInvokesExecuted += (a == b && a == 20)? 2 : 0; + + try { DoCallTryCatch(false); } + catch (ArgumentException) { s_OtherExceptionCatchCalled++; } + + ManualRaiseException(); + } + + s_RunGC = false; + + if (s_FinallyCalled == s_LoopCounter && + s_CatchCalled == (s_LoopCounter * 7) && + s_OtherExceptionCatchCalled == (s_LoopCounter * 3) && + s_WrongPInvokesExecuted == 0 && + s_PInvokesExecuted == (s_LoopCounter * 3) && + s_SEHExceptionCatchCalled == (s_LoopCounter * 2)) + { + Console.WriteLine("PASS"); + return 100; + } + + Console.WriteLine("s_FinallyCalled = " + s_FinallyCalled); + Console.WriteLine("s_CatchCalled = " + s_CatchCalled); + Console.WriteLine("s_OtherExceptionCatchCalled = " + s_OtherExceptionCatchCalled); + Console.WriteLine("s_SEHExceptionCatchCalled = " + s_SEHExceptionCatchCalled); + Console.WriteLine("s_WrongPInvokesExecuted = " + s_WrongPInvokesExecuted); + Console.WriteLine("s_PInvokesExecuted = " + s_PInvokesExecuted); + return -1; + } + + [DllImport("NativeLib")] + [DefaultDllImportSearchPaths(DllImportSearchPath.System32)] + static extern int NativeSum(int arg1, int arg2); + +#if WINDOWS + [DllImport("kernel32")] + static extern void RaiseException(uint dwExceptionCode, uint dwExceptionFlags, uint nNumberOfArguments, IntPtr lpArguments); +#endif +} diff --git a/tests/src/Interop/NativeLibraryResolveCallback/CallbackStressTest.csproj b/tests/src/Interop/NativeLibraryResolveCallback/CallbackStressTest.csproj new file mode 100644 index 0000000..aec940d --- /dev/null +++ b/tests/src/Interop/NativeLibraryResolveCallback/CallbackStressTest.csproj @@ -0,0 +1,37 @@ + + + + + Debug + AnyCPU + CallbackStressTest + 2.0 + {F1E66554-8C8E-4141-85CF-D0CD6A0CD0B0} + Exe + {786C830F-07A1-408B-BD7F-6EE04809D6DB};{FAE04EC0-301F-11D3-BF4B-00C04F79EFBC} + ..\..\ + true + true + WINDOWS + + + + + + + + + False + + + + + + + + + + + + + diff --git a/tests/src/Interop/NativeLibraryResolveCallback/CallbackTests.csproj b/tests/src/Interop/NativeLibraryResolveCallback/CallbackTests.csproj index c947a50..dd6a4ab 100644 --- a/tests/src/Interop/NativeLibraryResolveCallback/CallbackTests.csproj +++ b/tests/src/Interop/NativeLibraryResolveCallback/CallbackTests.csproj @@ -24,7 +24,7 @@ - +