From 2679f96d273c819b49a50e5dbe8679fa3328bf0a Mon Sep 17 00:00:00 2001 From: Aaron R Robinson Date: Thu, 29 Aug 2024 18:34:39 -0700 Subject: [PATCH 01/16] Convert Exception.GetFrozenStackTrace() --- .../src/System/Exception.CoreCLR.cs | 10 +++--- src/coreclr/vm/comutilnative.cpp | 33 ++++++++----------- src/coreclr/vm/comutilnative.h | 3 +- src/coreclr/vm/ecalllist.h | 1 - src/coreclr/vm/qcallentrypoints.cpp | 1 + 5 files changed, 23 insertions(+), 25 deletions(-) diff --git a/src/coreclr/System.Private.CoreLib/src/System/Exception.CoreCLR.cs b/src/coreclr/System.Private.CoreLib/src/System/Exception.CoreCLR.cs index 79944b1cca8e6a..1029f3e570e338 100644 --- a/src/coreclr/System.Private.CoreLib/src/System/Exception.CoreCLR.cs +++ b/src/coreclr/System.Private.CoreLib/src/System/Exception.CoreCLR.cs @@ -117,9 +117,6 @@ internal void InternalPreserveStackTrace() [MethodImpl(MethodImplOptions.InternalCall)] private static extern void PrepareForForeignExceptionRaise(); - [MethodImpl(MethodImplOptions.InternalCall)] - private static extern object? GetFrozenStackTrace(Exception exception); - [MethodImpl(MethodImplOptions.InternalCall)] internal static extern uint GetExceptionCount(); @@ -226,9 +223,14 @@ public DispatchState( } } + [LibraryImport(RuntimeHelpers.QCall, EntryPoint = "ExceptionNative_GetFrozenStackTrace")] + private static partial void GetFrozenStackTrace(ObjectHandleOnStack exception, ObjectHandleOnStack stackTrace); + internal DispatchState CaptureDispatchState() { - object? stackTrace = GetFrozenStackTrace(this); + Exception _this = this; + object? stackTrace = null; + GetFrozenStackTrace(ObjectHandleOnStack.Create(ref _this), ObjectHandleOnStack.Create(ref stackTrace)); return new DispatchState(stackTrace, _remoteStackTraceString, _ipForWatsonBuckets, _watsonBuckets); diff --git a/src/coreclr/vm/comutilnative.cpp b/src/coreclr/vm/comutilnative.cpp index a4f0eca49a3307..426fce8b1baf72 100644 --- a/src/coreclr/vm/comutilnative.cpp +++ b/src/coreclr/vm/comutilnative.cpp @@ -78,29 +78,26 @@ FCIMPLEND // Given an exception object, this method will mark its stack trace as frozen and return it to the caller. // Frozen stack traces are immutable, when a thread attempts to add a frame to it, the stack trace is cloned first. -FCIMPL1(Object *, ExceptionNative::GetFrozenStackTrace, Object* pExceptionObjectUnsafe); +extern "C" void QCALLTYPE ExceptionNative_GetFrozenStackTrace(QCall::ObjectHandleOnStack exception, QCall::ObjectHandleOnStack ret) { - CONTRACTL - { - FCALL_CHECK; - } - CONTRACTL_END; + QCALL_CONTRACT; + + BEGIN_QCALL; - ASSERT(pExceptionObjectUnsafe != NULL); + GCX_COOP(); + + _ASSERTE(exception.Get() != NULL); struct { StackTraceArray stackTrace; EXCEPTIONREF refException = NULL; PTRARRAYREF keepAliveArray = NULL; // Object array of Managed Resolvers / AssemblyLoadContexts - OBJECTREF result = NULL; } gc; - - // GC protect the array reference - HELPER_METHOD_FRAME_BEGIN_RET_PROTECT(gc); + GCPROTECT_BEGIN(gc); // Get the exception object reference - gc.refException = (EXCEPTIONREF)(ObjectToOBJECTREF(pExceptionObjectUnsafe)); + gc.refException = (EXCEPTIONREF)exception.Get(); gc.refException->GetStackTrace(gc.stackTrace, &gc.keepAliveArray); @@ -108,22 +105,20 @@ FCIMPL1(Object *, ExceptionNative::GetFrozenStackTrace, Object* pExceptionObject if (gc.keepAliveArray != NULL) { - gc.result = gc.keepAliveArray; + ret.Set(gc.keepAliveArray); } else { - gc.result = gc.stackTrace.Get(); + ret.Set(gc.stackTrace.Get()); } + GCPROTECT_END(); - HELPER_METHOD_FRAME_END(); - - return OBJECTREFToObject(gc.result); + END_QCALL; } -FCIMPLEND #ifdef FEATURE_COMINTEROP -BSTR BStrFromString(STRINGREF s) +static BSTR BStrFromString(STRINGREF s) { CONTRACTL { diff --git a/src/coreclr/vm/comutilnative.h b/src/coreclr/vm/comutilnative.h index a0cea8d190d597..b7c5bd713cf949 100644 --- a/src/coreclr/vm/comutilnative.h +++ b/src/coreclr/vm/comutilnative.h @@ -41,7 +41,6 @@ class ExceptionNative static FCDECL1(FC_BOOL_RET, IsImmutableAgileException, Object* pExceptionUNSAFE); static FCDECL1(FC_BOOL_RET, IsTransient, INT32 hresult); static FCDECL0(VOID, PrepareForForeignExceptionRaise); - static FCDECL1(Object *, GetFrozenStackTrace, Object* pExceptionObjectUnsafe); #ifdef FEATURE_COMINTEROP // NOTE: caller cleans up any partially initialized BSTRs in pED @@ -54,6 +53,8 @@ class ExceptionNative static FCDECL0(UINT32, GetExceptionCount); }; +extern "C" void QCALLTYPE ExceptionNative_GetFrozenStackTrace(QCall::ObjectHandleOnStack exception, QCall::ObjectHandleOnStack ret); + enum class ExceptionMessageKind { ThreadAbort = 1, ThreadInterrupted = 2, diff --git a/src/coreclr/vm/ecalllist.h b/src/coreclr/vm/ecalllist.h index 69f9747a97cd12..8066d185a881d2 100644 --- a/src/coreclr/vm/ecalllist.h +++ b/src/coreclr/vm/ecalllist.h @@ -83,7 +83,6 @@ FCFuncEnd() FCFuncStart(gExceptionFuncs) FCFuncElement("IsImmutableAgileException", ExceptionNative::IsImmutableAgileException) FCFuncElement("PrepareForForeignExceptionRaise", ExceptionNative::PrepareForForeignExceptionRaise) - FCFuncElement("GetFrozenStackTrace", ExceptionNative::GetFrozenStackTrace) FCFuncElement("GetExceptionCount", ExceptionNative::GetExceptionCount) FCFuncEnd() diff --git a/src/coreclr/vm/qcallentrypoints.cpp b/src/coreclr/vm/qcallentrypoints.cpp index 9eb0584379516b..69029bdf568c1e 100644 --- a/src/coreclr/vm/qcallentrypoints.cpp +++ b/src/coreclr/vm/qcallentrypoints.cpp @@ -99,6 +99,7 @@ static const Entry s_QCall[] = DllImportEntry(Environment_Exit) DllImportEntry(Environment_FailFast) DllImportEntry(Environment_GetProcessorCount) + DllImportEntry(ExceptionNative_GetFrozenStackTrace) DllImportEntry(ExceptionNative_GetMessageFromNativeResources) DllImportEntry(ExceptionNative_GetMethodFromStackTrace) DllImportEntry(ExceptionNative_ThrowAmbiguousResolutionException) From 74aca3569a76751c61c209b96a5c62538697c641 Mon Sep 17 00:00:00 2001 From: Aaron R Robinson Date: Fri, 30 Aug 2024 10:12:59 -0700 Subject: [PATCH 02/16] Convert GC.AllocateNewArray() Removed use of Unsafe.As(). --- .../src/System/GC.CoreCLR.cs | 14 +++++++----- src/coreclr/vm/comutilnative.cpp | 22 +++++++------------ src/coreclr/vm/comutilnative.h | 4 ++-- src/coreclr/vm/ecalllist.h | 2 -- src/coreclr/vm/qcallentrypoints.cpp | 1 + 5 files changed, 20 insertions(+), 23 deletions(-) diff --git a/src/coreclr/System.Private.CoreLib/src/System/GC.CoreCLR.cs b/src/coreclr/System.Private.CoreLib/src/System/GC.CoreCLR.cs index 1bd94635a26c76..dbf7828bb66fce 100644 --- a/src/coreclr/System.Private.CoreLib/src/System/GC.CoreCLR.cs +++ b/src/coreclr/System.Private.CoreLib/src/System/GC.CoreCLR.cs @@ -103,8 +103,8 @@ internal enum GC_ALLOC_FLAGS GC_ALLOC_PINNED_OBJECT_HEAP = 64, }; - [MethodImpl(MethodImplOptions.InternalCall)] - internal static extern Array AllocateNewArray(IntPtr typeHandle, int length, GC_ALLOC_FLAGS flags); + [LibraryImport(RuntimeHelpers.QCall, EntryPoint = "GCInterface_AllocateNewArray")] + private static unsafe partial void AllocateNewArray(MethodTable* pMT, int length, GC_ALLOC_FLAGS flags, ObjectHandleOnStack ret); [LibraryImport(RuntimeHelpers.QCall, EntryPoint = "GCInterface_GetTotalMemory")] private static partial long GetTotalMemory(); @@ -800,7 +800,9 @@ public static unsafe T[] AllocateUninitializedArray(int length, bool pinned = if (pinned) flags |= GC_ALLOC_FLAGS.GC_ALLOC_PINNED_OBJECT_HEAP; - return Unsafe.As(AllocateNewArray(RuntimeTypeHandle.ToIntPtr(typeof(T[]).TypeHandle), length, flags)); + T[]? result = null; + AllocateNewArray(TypeHandle.TypeHandleOf().AsMethodTable(), length, flags, ObjectHandleOnStack.Create(ref result)); + return result!; } /// @@ -809,7 +811,7 @@ public static unsafe T[] AllocateUninitializedArray(int length, bool pinned = /// Specifies the type of the array element. /// Specifies the length of the array. /// Specifies whether the allocated array must be pinned. - public static T[] AllocateArray(int length, bool pinned = false) // T[] rather than T?[] to match `new T[length]` behavior + public static unsafe T[] AllocateArray(int length, bool pinned = false) // T[] rather than T?[] to match `new T[length]` behavior { GC_ALLOC_FLAGS flags = GC_ALLOC_FLAGS.GC_ALLOC_NO_FLAGS; @@ -818,7 +820,9 @@ public static T[] AllocateArray(int length, bool pinned = false) // T[] rathe flags = GC_ALLOC_FLAGS.GC_ALLOC_PINNED_OBJECT_HEAP; } - return Unsafe.As(AllocateNewArray(RuntimeTypeHandle.ToIntPtr(typeof(T[]).TypeHandle), length, flags)); + T[]? result = null; + AllocateNewArray(TypeHandle.TypeHandleOf().AsMethodTable(), length, flags, ObjectHandleOnStack.Create(ref result)); + return result!; } [MethodImpl(MethodImplOptions.InternalCall)] diff --git a/src/coreclr/vm/comutilnative.cpp b/src/coreclr/vm/comutilnative.cpp index 426fce8b1baf72..08afd14ea9c777 100644 --- a/src/coreclr/vm/comutilnative.cpp +++ b/src/coreclr/vm/comutilnative.cpp @@ -927,28 +927,22 @@ FCIMPLEND ** zeroingOptional -> whether caller prefers to skip clearing the content of the array, if possible. **Exceptions: IDS_EE_ARRAY_DIMENSIONS_EXCEEDED when size is too large. OOM if can't allocate. ==============================================================================*/ -FCIMPL3(Object*, GCInterface::AllocateNewArray, void* arrayTypeHandle, INT32 length, INT32 flags) +extern "C" void QCALLTYPE GCInterface_AllocateNewArray(MethodTable* pMT, INT32 length, INT32 flags, QCall::ObjectHandleOnStack ret) { - CONTRACTL { - FCALL_CHECK; - } CONTRACTL_END; + QCALL_CONTRACT; + + _ASSERTE(pMT != NULL); - OBJECTREF pRet = NULL; - TypeHandle arrayType = TypeHandle::FromPtr(arrayTypeHandle); + BEGIN_QCALL; - HELPER_METHOD_FRAME_BEGIN_RET_0(); + GCX_COOP(); //Only the following flags are used by GC.cs, so we'll just assert it here. _ASSERTE((flags & ~(GC_ALLOC_ZEROING_OPTIONAL | GC_ALLOC_PINNED_OBJECT_HEAP)) == 0); + ret.Set(AllocateSzArray(pMT, length, (GC_ALLOC_FLAGS)flags)); - pRet = AllocateSzArray(arrayType, length, (GC_ALLOC_FLAGS)flags); - - HELPER_METHOD_FRAME_END(); - - return OBJECTREFToObject(pRet); + END_QCALL; } -FCIMPLEND - FCIMPL0(INT64, GCInterface::GetTotalAllocatedBytesApproximate) { diff --git a/src/coreclr/vm/comutilnative.h b/src/coreclr/vm/comutilnative.h index b7c5bd713cf949..99a872ae6c176c 100644 --- a/src/coreclr/vm/comutilnative.h +++ b/src/coreclr/vm/comutilnative.h @@ -183,8 +183,6 @@ class GCInterface { static FCDECL0(INT64, GetAllocatedBytesForCurrentThread); static FCDECL0(INT64, GetTotalAllocatedBytesApproximate); - static FCDECL3(Object*, AllocateNewArray, void* elementTypeHandle, INT32 length, INT32 flags); - NOINLINE static void SendEtwRemoveMemoryPressureEvent(UINT64 bytesAllocated); static void SendEtwAddMemoryPressureEvent(UINT64 bytesAllocated); @@ -204,6 +202,8 @@ class GCInterface { extern "C" INT64 QCALLTYPE GCInterface_GetTotalAllocatedBytesPrecise(); +extern "C" void QCALLTYPE GCInterface_AllocateNewArray(MethodTable* pMT, INT32 length, INT32 flags, QCall::ObjectHandleOnStack ret); + extern "C" INT64 QCALLTYPE GCInterface_GetTotalMemory(); extern "C" void QCALLTYPE GCInterface_Collect(INT32 generation, INT32 mode); diff --git a/src/coreclr/vm/ecalllist.h b/src/coreclr/vm/ecalllist.h index 8066d185a881d2..4b74a5f9292795 100644 --- a/src/coreclr/vm/ecalllist.h +++ b/src/coreclr/vm/ecalllist.h @@ -363,8 +363,6 @@ FCFuncStart(gGCInterfaceFuncs) FCFuncElement("GetAllocatedBytesForCurrentThread", GCInterface::GetAllocatedBytesForCurrentThread) FCFuncElement("GetTotalAllocatedBytesApproximate", GCInterface::GetTotalAllocatedBytesApproximate) - - FCFuncElement("AllocateNewArray", GCInterface::AllocateNewArray) FCFuncEnd() FCFuncStart(gGCSettingsFuncs) diff --git a/src/coreclr/vm/qcallentrypoints.cpp b/src/coreclr/vm/qcallentrypoints.cpp index 69029bdf568c1e..0d6002867531fe 100644 --- a/src/coreclr/vm/qcallentrypoints.cpp +++ b/src/coreclr/vm/qcallentrypoints.cpp @@ -259,6 +259,7 @@ static const Entry s_QCall[] = DllImportEntry(GCInterface_WaitForFullGCComplete) DllImportEntry(GCInterface_StartNoGCRegion) DllImportEntry(GCInterface_EndNoGCRegion) + DllImportEntry(GCInterface_AllocateNewArray) DllImportEntry(GCInterface_GetTotalMemory) DllImportEntry(GCInterface_Collect) DllImportEntry(GCInterface_ReRegisterForFinalize) From 249a8e06ee99846684054ceb41308105fad29a59 Mon Sep 17 00:00:00 2001 From: Aaron R Robinson Date: Fri, 30 Aug 2024 11:02:45 -0700 Subject: [PATCH 03/16] Convert Thread.GetApartmentStateNative() and Thread.SetApartmentStateNative() --- .../src/System/Threading/Thread.CoreCLR.cs | 33 ++++-- src/coreclr/vm/comsynchronizable.cpp | 111 ++++++------------ src/coreclr/vm/comsynchronizable.h | 12 +- src/coreclr/vm/ecalllist.h | 4 - src/coreclr/vm/qcallentrypoints.cpp | 4 + 5 files changed, 69 insertions(+), 95 deletions(-) diff --git a/src/coreclr/System.Private.CoreLib/src/System/Threading/Thread.CoreCLR.cs b/src/coreclr/System.Private.CoreLib/src/System/Threading/Thread.CoreCLR.cs index d7d83ba51ac0cb..fa26e6caa7f845 100644 --- a/src/coreclr/System.Private.CoreLib/src/System/Threading/Thread.CoreCLR.cs +++ b/src/coreclr/System.Private.CoreLib/src/System/Threading/Thread.CoreCLR.cs @@ -243,21 +243,31 @@ public ThreadPriority Priority [MethodImpl(MethodImplOptions.InternalCall)] private extern int GetThreadStateNative(); - public ApartmentState GetApartmentState() => -#if FEATURE_COMINTEROP_APARTMENT_SUPPORT - (ApartmentState)GetApartmentStateNative(); -#else // !FEATURE_COMINTEROP_APARTMENT_SUPPORT - ApartmentState.Unknown; -#endif // FEATURE_COMINTEROP_APARTMENT_SUPPORT - /// /// An unstarted thread can be marked to indicate that it will host a /// single-threaded or multi-threaded apartment. /// #if FEATURE_COMINTEROP_APARTMENT_SUPPORT + [LibraryImport(RuntimeHelpers.QCall, EntryPoint = "ThreadNative_GetApartmentState")] + private static partial int GetApartmentState(ThreadHandle t); + + [LibraryImport(RuntimeHelpers.QCall, EntryPoint = "ThreadNative_SetApartmentState")] + private static partial int SetApartmentState(ThreadHandle t, int state); + + public ApartmentState GetApartmentState() + { + var state = (ApartmentState)GetApartmentState(GetNativeHandle()); + GC.KeepAlive(this); + return state; + } + private bool SetApartmentStateUnchecked(ApartmentState state, bool throwOnError) { - ApartmentState retState = (ApartmentState)SetApartmentStateNative((int)state); + ApartmentState retState; + lock (this) // This is also satisfying a GC.KeepAlive(). + { + retState = (ApartmentState)SetApartmentState(GetNativeHandle(), (int)state); + } // Special case where we pass in Unknown and get back MTA. // Once we CoUninitialize the thread, the OS will still @@ -282,12 +292,9 @@ private bool SetApartmentStateUnchecked(ApartmentState state, bool throwOnError) return true; } - [MethodImpl(MethodImplOptions.InternalCall)] - internal extern int GetApartmentStateNative(); - - [MethodImpl(MethodImplOptions.InternalCall)] - internal extern int SetApartmentStateNative(int state); #else // FEATURE_COMINTEROP_APARTMENT_SUPPORT + public ApartmentState GetApartmentState() => ApartmentState.Unknown; + private static bool SetApartmentStateUnchecked(ApartmentState state, bool throwOnError) { if (state != ApartmentState.Unknown) diff --git a/src/coreclr/vm/comsynchronizable.cpp b/src/coreclr/vm/comsynchronizable.cpp index 23dfb4d238803f..5d9e6f8483a519 100644 --- a/src/coreclr/vm/comsynchronizable.cpp +++ b/src/coreclr/vm/comsynchronizable.cpp @@ -591,84 +591,23 @@ FCIMPLEND #ifdef FEATURE_COMINTEROP_APARTMENT_SUPPORT -// Indicate whether the thread will host an STA (this may fail if the thread has -// already been made part of the MTA, use GetApartmentState or the return state -// from this routine to check for this). -FCIMPL2(INT32, ThreadNative::SetApartmentState, ThreadBaseObject* pThisUNSAFE, INT32 iState) -{ - FCALL_CONTRACT; - - if (pThisUNSAFE==NULL) - FCThrowRes(kNullReferenceException, W("NullReference_This")); - - BOOL ok = TRUE; - THREADBASEREF pThis = (THREADBASEREF) pThisUNSAFE; - - HELPER_METHOD_FRAME_BEGIN_RET_1(pThis); - - Thread *thread = pThis->GetInternal(); - if (!thread) - COMPlusThrow(kThreadStateException, IDS_EE_THREAD_CANNOT_GET); - - { - pThis->EnterObjMonitor(); - - // We can only change the apartment if the thread is unstarted or - // running, and if it's running we have to be in the thread's - // context. - if ((!ThreadNotStarted(thread) && !ThreadIsRunning(thread)) || - (!ThreadNotStarted(thread) && (GetThread() != thread))) - ok = FALSE; - else - { - EX_TRY - { - iState = thread->SetApartment((Thread::ApartmentState)iState); - } - EX_CATCH - { - pThis->LeaveObjMonitor(); - EX_RETHROW; - } - EX_END_CATCH_UNREACHABLE; - } - - pThis->LeaveObjMonitor(); - } - - // Now it's safe to throw exceptions again. - if (!ok) - COMPlusThrow(kThreadStateException); - - HELPER_METHOD_FRAME_END(); - - return iState; -} -FCIMPLEND - // Return whether the thread hosts an STA, is a member of the MTA or is not // currently initialized for COM. -FCIMPL1(INT32, ThreadNative::GetApartmentState, ThreadBaseObject* pThisUNSAFE) +extern "C" INT32 QCALLTYPE ThreadNative_GetApartmentState(QCall::ThreadHandle thread) { - FCALL_CONTRACT; - - INT32 retVal = 0; - - THREADBASEREF refThis = (THREADBASEREF) ObjectToOBJECTREF(pThisUNSAFE); - - HELPER_METHOD_FRAME_BEGIN_RET_1(refThis); - - if (refThis == NULL) + CONTRACTL { - COMPlusThrow(kNullReferenceException, W("NullReference_This")); + QCALL_CHECK; + PRECONDITION(thread != NULL); } + CONTRACTL_END; - Thread* thread = refThis->GetInternal(); + INT32 retVal = 0; + + BEGIN_QCALL; if (ThreadIsDead(thread)) - { COMPlusThrow(kThreadStateException, W("ThreadState_Dead_State")); - } retVal = thread->GetApartment(); @@ -686,12 +625,40 @@ FCIMPL1(INT32, ThreadNative::GetApartmentState, ThreadBaseObject* pThisUNSAFE) } #endif // FEATURE_COMINTEROP - HELPER_METHOD_FRAME_END(); - + END_QCALL; return retVal; } -FCIMPLEND +// Indicate whether the thread will host an STA (this may fail if the thread has +// already been made part of the MTA, use GetApartmentState or the return state +// from this routine to check for this). +extern "C" INT32 QCALLTYPE ThreadNative_SetApartmentState(QCall::ThreadHandle thread, INT32 iState) +{ + CONTRACTL + { + QCALL_CHECK; + PRECONDITION(thread != NULL); + } + CONTRACTL_END; + + INT32 retVal = 0; + + BEGIN_QCALL; + + // We can only change the apartment if the thread is unstarted or + // running, and if it's running we have to be in the thread's + // context. + if ((!ThreadNotStarted(thread) && !ThreadIsRunning(thread)) + || (!ThreadNotStarted(thread) && (GetThread() != thread))) + { + COMPlusThrow(kThreadStateException); + } + + retVal = thread->SetApartment((Thread::ApartmentState)iState); + + END_QCALL; + return retVal; +} #endif // FEATURE_COMINTEROP_APARTMENT_SUPPORT void ReleaseThreadExternalCount(Thread * pThread) diff --git a/src/coreclr/vm/comsynchronizable.h b/src/coreclr/vm/comsynchronizable.h index b0f5b72295a155..e50d3181759a09 100644 --- a/src/coreclr/vm/comsynchronizable.h +++ b/src/coreclr/vm/comsynchronizable.h @@ -60,12 +60,6 @@ friend class ThreadBaseObject; static FCDECL1(FC_BOOL_RET, GetIsBackground, ThreadBaseObject* pThisUNSAFE); static FCDECL1(INT32, GetThreadState, ThreadBaseObject* pThisUNSAFE); -#ifdef FEATURE_COMINTEROP_APARTMENT_SUPPORT - static FCDECL1(INT32, GetApartmentState, ThreadBaseObject* pThis); - static FCDECL2(INT32, SetApartmentState, ThreadBaseObject* pThisUNSAFE, INT32 iState); -#endif // FEATURE_COMINTEROP_APARTMENT_SUPPORT - - static FCDECL0(INT32, GetOptimalMaxSpinWaitsPerSpinIteration); static FCDECL0(Object*, GetCurrentThread); static FCDECL1(void, Finalize, ThreadBaseObject* pThis); @@ -92,6 +86,12 @@ extern "C" void QCALLTYPE ThreadNative_SetIsBackground(QCall::ThreadHandle threa extern "C" void QCALLTYPE ThreadNative_InformThreadNameChange(QCall::ThreadHandle thread, LPCWSTR name, INT32 len); extern "C" BOOL QCALLTYPE ThreadNative_YieldThread(); extern "C" UINT64 QCALLTYPE ThreadNative_GetCurrentOSThreadId(); + +#ifdef FEATURE_COMINTEROP_APARTMENT_SUPPORT +extern "C" INT32 QCALLTYPE ThreadNative_GetApartmentState(QCall::ThreadHandle thread); +extern "C" INT32 QCALLTYPE ThreadNative_SetApartmentState(QCall::ThreadHandle thread, INT32 iState); +#endif // FEATURE_COMINTEROP_APARTMENT_SUPPORT + extern "C" void QCALLTYPE ThreadNative_Abort(QCall::ThreadHandle thread); extern "C" void QCALLTYPE ThreadNative_ResetAbort(); extern "C" void QCALLTYPE ThreadNative_SpinWait(INT32 iterations); diff --git a/src/coreclr/vm/ecalllist.h b/src/coreclr/vm/ecalllist.h index 4b74a5f9292795..da41ad240db95a 100644 --- a/src/coreclr/vm/ecalllist.h +++ b/src/coreclr/vm/ecalllist.h @@ -308,10 +308,6 @@ FCFuncStart(gThreadFuncs) FCFuncElement("GetPriorityNative", ThreadNative::GetPriority) FCFuncElement("SetPriorityNative", ThreadNative::SetPriority) FCFuncElement("GetThreadStateNative", ThreadNative::GetThreadState) -#ifdef FEATURE_COMINTEROP_APARTMENT_SUPPORT - FCFuncElement("GetApartmentStateNative", ThreadNative::GetApartmentState) - FCFuncElement("SetApartmentStateNative", ThreadNative::SetApartmentState) -#endif // FEATURE_COMINTEROP_APARTMENT_SUPPORT FCFuncElement("Join", ThreadNative::Join) FCFuncElement("get_OptimalMaxSpinWaitsPerSpinIteration", ThreadNative::GetOptimalMaxSpinWaitsPerSpinIteration) FCFuncEnd() diff --git a/src/coreclr/vm/qcallentrypoints.cpp b/src/coreclr/vm/qcallentrypoints.cpp index 0d6002867531fe..484237808408a7 100644 --- a/src/coreclr/vm/qcallentrypoints.cpp +++ b/src/coreclr/vm/qcallentrypoints.cpp @@ -238,6 +238,10 @@ static const Entry s_QCall[] = DllImportEntry(ThreadNative_InformThreadNameChange) DllImportEntry(ThreadNative_YieldThread) DllImportEntry(ThreadNative_GetCurrentOSThreadId) +#ifdef FEATURE_COMINTEROP_APARTMENT_SUPPORT + DllImportEntry(ThreadNative_GetApartmentState) + DllImportEntry(ThreadNative_SetApartmentState) +#endif // FEATURE_COMINTEROP_APARTMENT_SUPPORT DllImportEntry(ThreadNative_Abort) DllImportEntry(ThreadNative_ResetAbort) DllImportEntry(ThreadNative_SpinWait) From 06f0544797ac82e4b3a047fe17f3bab734249ec7 Mon Sep 17 00:00:00 2001 From: Aaron R Robinson Date: Fri, 30 Aug 2024 12:02:13 -0700 Subject: [PATCH 04/16] Convert Thread.Join() --- .../src/System/Threading/Thread.CoreCLR.cs | 20 +++++- src/coreclr/vm/comsynchronizable.cpp | 62 ++++++++----------- src/coreclr/vm/comsynchronizable.h | 3 +- src/coreclr/vm/ecalllist.h | 1 - src/coreclr/vm/qcallentrypoints.cpp | 1 + 5 files changed, 44 insertions(+), 43 deletions(-) diff --git a/src/coreclr/System.Private.CoreLib/src/System/Threading/Thread.CoreCLR.cs b/src/coreclr/System.Private.CoreLib/src/System/Threading/Thread.CoreCLR.cs index fa26e6caa7f845..c9d5a3868bdbed 100644 --- a/src/coreclr/System.Private.CoreLib/src/System/Threading/Thread.CoreCLR.cs +++ b/src/coreclr/System.Private.CoreLib/src/System/Threading/Thread.CoreCLR.cs @@ -338,6 +338,10 @@ public void Interrupt() [LibraryImport(RuntimeHelpers.QCall, EntryPoint = "ThreadNative_Interrupt")] private static partial void Interrupt(ThreadHandle t); + [LibraryImport(RuntimeHelpers.QCall, EntryPoint = "ThreadNative_Join")] + [return: MarshalAs(UnmanagedType.Bool)] + private static partial bool Join(ThreadHandle t, int millisecondsTimeout); + /// /// Waits for the thread to die or for timeout milliseconds to elapse. /// @@ -345,11 +349,21 @@ public void Interrupt() /// Returns true if the thread died, or false if the wait timed out. If /// -1 is given as the parameter, no timeout will occur. /// - /// if timeout < -1 (Timeout.Infinite) + /// if timeout < -1 (Timeout.Infinite) /// if the thread is interrupted while waiting /// if the thread has not been started yet - [MethodImpl(MethodImplOptions.InternalCall)] - public extern bool Join(int millisecondsTimeout); + public bool Join(int millisecondsTimeout) + { + // Validate the timeout + if (millisecondsTimeout < 0 && millisecondsTimeout != Timeout.Infinite) + { + throw new ArgumentOutOfRangeException(nameof(millisecondsTimeout), SR.ArgumentOutOfRange_NeedNonNegOrNegative1); + } + + bool res = Join(GetNativeHandle(), millisecondsTimeout); + GC.KeepAlive(this); + return res; + } /// /// Max value to be passed into for optimal delaying. This value is normalized to be diff --git a/src/coreclr/vm/comsynchronizable.cpp b/src/coreclr/vm/comsynchronizable.cpp index 5d9e6f8483a519..3b987f22e92ac6 100644 --- a/src/coreclr/vm/comsynchronizable.cpp +++ b/src/coreclr/vm/comsynchronizable.cpp @@ -417,30 +417,6 @@ FCIMPL1(FC_BOOL_RET, ThreadNative::IsAlive, ThreadBaseObject* pThisUNSAFE) } FCIMPLEND -FCIMPL2(FC_BOOL_RET, ThreadNative::Join, ThreadBaseObject* pThisUNSAFE, INT32 Timeout) -{ - FCALL_CONTRACT; - - BOOL retVal = FALSE; - THREADBASEREF pThis = (THREADBASEREF) pThisUNSAFE; - - HELPER_METHOD_FRAME_BEGIN_RET_1(pThis); - - if (pThis==NULL) - COMPlusThrow(kNullReferenceException, W("NullReference_This")); - - // validate the timeout - if ((Timeout < 0) && (Timeout != INFINITE_TIMEOUT)) - COMPlusThrowArgumentOutOfRange(W("millisecondsTimeout"), W("ArgumentOutOfRange_NeedNonNegOrNegative1")); - - retVal = DoJoin(pThis, Timeout); - - HELPER_METHOD_FRAME_END(); - - FC_RETURN_BOOL(retVal); -} -FCIMPLEND - NOINLINE static Object* GetCurrentThreadHelper() { FCALL_CONTRACT; @@ -670,23 +646,22 @@ void ReleaseThreadExternalCount(Thread * pThread) typedef Holder ThreadExternalCountHolder; // Wait for the thread to die -BOOL ThreadNative::DoJoin(THREADBASEREF DyingThread, INT32 timeout) +static BOOL DoJoin(QCall::ThreadHandle dyingThreadHandle, INT32 timeout) { CONTRACTL { THROWS; GC_TRIGGERS; - MODE_COOPERATIVE; - PRECONDITION(DyingThread != NULL); + MODE_PREEMPTIVE; PRECONDITION((timeout >= 0) || (timeout == INFINITE_TIMEOUT)); } CONTRACTL_END; - Thread * DyingInternal = DyingThread->GetInternal(); + Thread * DyingInternal = dyingThreadHandle; // Validate the handle. It's valid to Join a thread that's not running -- so // long as it was once started. - if (DyingInternal == 0 || + if (DyingInternal == NULL || !(DyingInternal->m_State & Thread::TS_LegalToJoin)) { COMPlusThrow(kThreadStateException, W("ThreadState_NotStarted")); @@ -697,12 +672,8 @@ BOOL ThreadNative::DoJoin(THREADBASEREF DyingThread, INT32 timeout) if (ThreadIsDead(DyingInternal) || !DyingInternal->HasValidThreadHandle()) return TRUE; - DWORD dwTimeOut32 = (timeout == INFINITE_TIMEOUT - ? INFINITE - : (DWORD) timeout); - - // There is a race here. DyingThread is going to close its thread handle. - // If we grab the handle and then DyingThread closes it, we will wait forever + // There is a race here. The Thread is going to close its thread handle. + // If we grab the handle and then the Thread closes it, we will wait forever // in DoAppropriateWait. int RefCount = DyingInternal->IncExternalCount(); if (RefCount == 1) @@ -722,9 +693,11 @@ BOOL ThreadNative::DoJoin(THREADBASEREF DyingThread, INT32 timeout) return TRUE; } - GCX_PREEMP(); - DWORD rv = DyingInternal->JoinEx(dwTimeOut32, (WaitMode)(WaitMode_Alertable/*alertable*/|WaitMode_InDeadlock)); + DWORD dwTimeOut32 = (timeout == INFINITE_TIMEOUT + ? INFINITE + : (DWORD) timeout); + DWORD rv = DyingInternal->JoinEx(dwTimeOut32, (WaitMode)(WaitMode_Alertable/*alertable*/|WaitMode_InDeadlock)); switch(rv) { case WAIT_OBJECT_0: @@ -746,6 +719,21 @@ BOOL ThreadNative::DoJoin(THREADBASEREF DyingThread, INT32 timeout) return FALSE; } +extern "C" BOOL QCALLTYPE ThreadNative_Join(QCall::ThreadHandle thread, INT32 Timeout) +{ + QCALL_CONTRACT; + + BOOL retVal = FALSE; + + BEGIN_QCALL; + + retVal = DoJoin(thread, Timeout); + + END_QCALL; + + return retVal; +} + // If the exposed object is created after-the-fact, for an existing thread, we call // InitExisting on it. This is the other "construction", as opposed to SetDelegate. void ThreadBaseObject::InitExisting() diff --git a/src/coreclr/vm/comsynchronizable.h b/src/coreclr/vm/comsynchronizable.h index e50d3181759a09..3341ff84173fa0 100644 --- a/src/coreclr/vm/comsynchronizable.h +++ b/src/coreclr/vm/comsynchronizable.h @@ -55,7 +55,6 @@ friend class ThreadBaseObject; static FCDECL1(INT32, GetPriority, ThreadBaseObject* pThisUNSAFE); static FCDECL2(void, SetPriority, ThreadBaseObject* pThisUNSAFE, INT32 iPriority); static FCDECL1(FC_BOOL_RET, IsAlive, ThreadBaseObject* pThisUNSAFE); - static FCDECL2(FC_BOOL_RET, Join, ThreadBaseObject* pThisUNSAFE, INT32 Timeout); static FCDECL1(void, Initialize, ThreadBaseObject* pThisUNSAFE); static FCDECL1(FC_BOOL_RET, GetIsBackground, ThreadBaseObject* pThisUNSAFE); static FCDECL1(INT32, GetThreadState, ThreadBaseObject* pThisUNSAFE); @@ -78,7 +77,6 @@ friend class ThreadBaseObject; static void KickOffThread_Worker(LPVOID /* KickOffThread_Args* */); static ULONG WINAPI KickOffThread(void *pass); - static BOOL DoJoin(THREADBASEREF DyingThread, INT32 timeout); }; extern "C" void QCALLTYPE ThreadNative_Start(QCall::ThreadHandle thread, int threadStackSize, int priority, PCWSTR pThreadName); @@ -92,6 +90,7 @@ extern "C" INT32 QCALLTYPE ThreadNative_GetApartmentState(QCall::ThreadHandle th extern "C" INT32 QCALLTYPE ThreadNative_SetApartmentState(QCall::ThreadHandle thread, INT32 iState); #endif // FEATURE_COMINTEROP_APARTMENT_SUPPORT +extern "C" BOOL QCALLTYPE ThreadNative_Join(QCall::ThreadHandle thread, INT32 Timeout); extern "C" void QCALLTYPE ThreadNative_Abort(QCall::ThreadHandle thread); extern "C" void QCALLTYPE ThreadNative_ResetAbort(); extern "C" void QCALLTYPE ThreadNative_SpinWait(INT32 iterations); diff --git a/src/coreclr/vm/ecalllist.h b/src/coreclr/vm/ecalllist.h index da41ad240db95a..87c26409ffc76e 100644 --- a/src/coreclr/vm/ecalllist.h +++ b/src/coreclr/vm/ecalllist.h @@ -308,7 +308,6 @@ FCFuncStart(gThreadFuncs) FCFuncElement("GetPriorityNative", ThreadNative::GetPriority) FCFuncElement("SetPriorityNative", ThreadNative::SetPriority) FCFuncElement("GetThreadStateNative", ThreadNative::GetThreadState) - FCFuncElement("Join", ThreadNative::Join) FCFuncElement("get_OptimalMaxSpinWaitsPerSpinIteration", ThreadNative::GetOptimalMaxSpinWaitsPerSpinIteration) FCFuncEnd() diff --git a/src/coreclr/vm/qcallentrypoints.cpp b/src/coreclr/vm/qcallentrypoints.cpp index 484237808408a7..35b7b07ce28a0c 100644 --- a/src/coreclr/vm/qcallentrypoints.cpp +++ b/src/coreclr/vm/qcallentrypoints.cpp @@ -242,6 +242,7 @@ static const Entry s_QCall[] = DllImportEntry(ThreadNative_GetApartmentState) DllImportEntry(ThreadNative_SetApartmentState) #endif // FEATURE_COMINTEROP_APARTMENT_SUPPORT + DllImportEntry(ThreadNative_Join) DllImportEntry(ThreadNative_Abort) DllImportEntry(ThreadNative_ResetAbort) DllImportEntry(ThreadNative_SpinWait) From 910b6af33600e5d87c3ea119f77f9517076a8bc1 Mon Sep 17 00:00:00 2001 From: Aaron R Robinson Date: Fri, 30 Aug 2024 16:30:20 -0700 Subject: [PATCH 05/16] Convert Thread.Priority property --- .../src/System/Threading/Thread.CoreCLR.cs | 40 ++++++-- src/coreclr/vm/comsynchronizable.cpp | 92 ++++++------------- src/coreclr/vm/comsynchronizable.h | 4 +- src/coreclr/vm/ecalllist.h | 2 - src/coreclr/vm/qcallentrypoints.cpp | 2 + 5 files changed, 63 insertions(+), 77 deletions(-) diff --git a/src/coreclr/System.Private.CoreLib/src/System/Threading/Thread.CoreCLR.cs b/src/coreclr/System.Private.CoreLib/src/System/Threading/Thread.CoreCLR.cs index c9d5a3868bdbed..de301e62a972fe 100644 --- a/src/coreclr/System.Private.CoreLib/src/System/Threading/Thread.CoreCLR.cs +++ b/src/coreclr/System.Private.CoreLib/src/System/Threading/Thread.CoreCLR.cs @@ -211,13 +211,43 @@ public extern bool IsThreadPoolThread internal set; } + [LibraryImport(RuntimeHelpers.QCall, EntryPoint = "ThreadNative_ThreadIsDead")] + [return: MarshalAs(UnmanagedType.Bool)] + private static partial bool ThreadIsDead(ThreadHandle t); + + [LibraryImport(RuntimeHelpers.QCall, EntryPoint = "ThreadNative_TrySetPriority")] + [return: MarshalAs(UnmanagedType.Bool)] + private static partial bool TrySetPriority(ThreadHandle t, int priority); + /// Returns the priority of the thread. public ThreadPriority Priority { - get => (ThreadPriority)GetPriorityNative(); + get + { + if (ThreadIsDead(GetNativeHandle())) // GC.KeepAlive() not needed since member fields are touched below. + { + throw new ThreadStateException(SR.ThreadState_Dead_Priority); + } + return (ThreadPriority)_priority; + } set { - SetPriorityNative((int)value); + ThreadHandle handle = GetNativeHandle(); + if (ThreadIsDead(handle)) // GC.KeepAlive() not needed since member fields are touched below. + { + // Note that you can manipulate the priority of a thread that hasn't started yet, + // or one that is running. But you get an exception if you manipulate the priority + // of a thread that has died. + throw new ThreadStateException(SR.ThreadState_Dead_Priority); + } + + int prev = _priority; + _priority = (int)value; + if (!TrySetPriority(handle, _priority)) // GC.KeepAlive() not needed since member fields are touched below. + { + _priority = prev; + throw new ThreadStateException(SR.ThreadState_SetPriorityFailed); + } if (value != ThreadPriority.Normal) { _mayNeedResetForThreadPool = true; @@ -225,12 +255,6 @@ public ThreadPriority Priority } } - [MethodImpl(MethodImplOptions.InternalCall)] - private extern int GetPriorityNative(); - - [MethodImpl(MethodImplOptions.InternalCall)] - private extern void SetPriorityNative(int priority); - [LibraryImport(RuntimeHelpers.QCall, EntryPoint = "ThreadNative_GetCurrentOSThreadId")] private static partial ulong GetCurrentOSThreadId(); diff --git a/src/coreclr/vm/comsynchronizable.cpp b/src/coreclr/vm/comsynchronizable.cpp index 3b987f22e92ac6..18d56116eb4492 100644 --- a/src/coreclr/vm/comsynchronizable.cpp +++ b/src/coreclr/vm/comsynchronizable.cpp @@ -59,44 +59,36 @@ static inline BOOL ThreadIsDead(Thread *t) // Map our exposed notion of thread priorities into the enumeration that NT uses. -static INT32 MapToNTPriority(INT32 ours) +static bool TryMapToNTPriority(INT32 ours, INT32& theirs) { - CONTRACTL - { - GC_NOTRIGGER; - THROWS; - MODE_ANY; - } - CONTRACTL_END; - - INT32 NTPriority = 0; + STANDARD_VM_CONTRACT; switch (ours) { case ThreadNative::PRIORITY_LOWEST: - NTPriority = THREAD_PRIORITY_LOWEST; + theirs = THREAD_PRIORITY_LOWEST; break; case ThreadNative::PRIORITY_BELOW_NORMAL: - NTPriority = THREAD_PRIORITY_BELOW_NORMAL; + theirs = THREAD_PRIORITY_BELOW_NORMAL; break; case ThreadNative::PRIORITY_NORMAL: - NTPriority = THREAD_PRIORITY_NORMAL; + theirs = THREAD_PRIORITY_NORMAL; break; case ThreadNative::PRIORITY_ABOVE_NORMAL: - NTPriority = THREAD_PRIORITY_ABOVE_NORMAL; + theirs = THREAD_PRIORITY_ABOVE_NORMAL; break; case ThreadNative::PRIORITY_HIGHEST: - NTPriority = THREAD_PRIORITY_HIGHEST; + theirs = THREAD_PRIORITY_HIGHEST; break; default: - COMPlusThrow(kArgumentOutOfRangeException, W("Argument_InvalidFlag")); + return false; } - return NTPriority; + return true; } @@ -252,6 +244,8 @@ extern "C" void QCALLTYPE ThreadNative_Start(QCall::ThreadHandle thread, int thr void ThreadNative::Start(Thread* pNewThread, int threadStackSize, int priority, PCWSTR pThreadName) { + STANDARD_VM_CONTRACT; + _ASSERTE(pNewThread != NULL); // Is the thread already started? You can't restart a thread. @@ -292,7 +286,11 @@ void ThreadNative::Start(Thread* pNewThread, int threadStackSize, int priority, // After we have established the thread handle, we can check m_Priority. // This ordering is required to eliminate the race condition on setting the // priority of a thread just as it starts up. - pNewThread->SetThreadPriority(MapToNTPriority(priority)); + INT32 NTPriority; + if (!TryMapToNTPriority(priority, NTPriority)) + COMPlusThrow(kArgumentOutOfRangeException, W("Argument_InvalidFlag")); + + pNewThread->SetThreadPriority(NTPriority); pNewThread->ChooseThreadCPUGroupAffinity(); pNewThread->SetThreadState(Thread::TS_LegalToJoin); @@ -326,65 +324,29 @@ void ThreadNative::Start(Thread* pNewThread, int threadStackSize, int priority, } } -// Note that you can manipulate the priority of a thread that hasn't started yet, -// or one that is running. But you get an exception if you manipulate the priority -// of a thread that has died. -FCIMPL1(INT32, ThreadNative::GetPriority, ThreadBaseObject* pThisUNSAFE) +extern "C" INT32 QCALLTYPE ThreadNative_ThreadIsDead(QCall::ThreadHandle thread) { - FCALL_CONTRACT; - - if (pThisUNSAFE==NULL) - FCThrowRes(kNullReferenceException, W("NullReference_This")); - - // validate the handle - if (ThreadIsDead(pThisUNSAFE->GetInternal())) - FCThrowRes(kThreadStateException, W("ThreadState_Dead_Priority")); + QCALL_CONTRACT; - return pThisUNSAFE->m_Priority; + return ThreadIsDead(thread); } -FCIMPLEND -FCIMPL2(void, ThreadNative::SetPriority, ThreadBaseObject* pThisUNSAFE, INT32 iPriority) +extern "C" BOOL QCALLTYPE ThreadNative_TrySetPriority(QCall::ThreadHandle thread, INT32 iPriority) { - FCALL_CONTRACT; - - int priority; - Thread *thread; + QCALL_CONTRACT; - THREADBASEREF pThis = (THREADBASEREF) pThisUNSAFE; - HELPER_METHOD_FRAME_BEGIN_1(pThis); + BOOL ret = FALSE; - if (pThis==NULL) - { - COMPlusThrow(kNullReferenceException, W("NullReference_This")); - } + BEGIN_QCALL; // translate the priority (validating as well) - priority = MapToNTPriority(iPriority); // can throw; needs a frame - - // validate the thread - thread = pThis->GetInternal(); - - if (ThreadIsDead(thread)) - { - COMPlusThrow(kThreadStateException, W("ThreadState_Dead_Priority")); - } - - INT32 oldPriority = pThis->m_Priority; + INT32 priority; + ret = TryMapToNTPriority(iPriority, priority) && thread->SetThreadPriority(priority); - // Eliminate the race condition by establishing m_Priority before we check for if - // the thread is running. See ThreadNative::Start() for the other half. - pThis->m_Priority = iPriority; - - if (!thread->SetThreadPriority(priority)) - { - pThis->m_Priority = oldPriority; - COMPlusThrow(kThreadStateException, W("ThreadState_SetPriorityFailed")); - } + END_QCALL; - HELPER_METHOD_FRAME_END(); + return ret; } -FCIMPLEND FCIMPL1(FC_BOOL_RET, ThreadNative::IsAlive, ThreadBaseObject* pThisUNSAFE) { diff --git a/src/coreclr/vm/comsynchronizable.h b/src/coreclr/vm/comsynchronizable.h index 3341ff84173fa0..acbc4384ea0a24 100644 --- a/src/coreclr/vm/comsynchronizable.h +++ b/src/coreclr/vm/comsynchronizable.h @@ -52,8 +52,6 @@ friend class ThreadBaseObject; ThreadAbortRequested = 128, }; - static FCDECL1(INT32, GetPriority, ThreadBaseObject* pThisUNSAFE); - static FCDECL2(void, SetPriority, ThreadBaseObject* pThisUNSAFE, INT32 iPriority); static FCDECL1(FC_BOOL_RET, IsAlive, ThreadBaseObject* pThisUNSAFE); static FCDECL1(void, Initialize, ThreadBaseObject* pThisUNSAFE); static FCDECL1(FC_BOOL_RET, GetIsBackground, ThreadBaseObject* pThisUNSAFE); @@ -80,6 +78,8 @@ friend class ThreadBaseObject; }; extern "C" void QCALLTYPE ThreadNative_Start(QCall::ThreadHandle thread, int threadStackSize, int priority, PCWSTR pThreadName); +extern "C" BOOL QCALLTYPE ThreadNative_ThreadIsDead(QCall::ThreadHandle thread); +extern "C" BOOL QCALLTYPE ThreadNative_TrySetPriority(QCall::ThreadHandle thread, INT32 iPriority); extern "C" void QCALLTYPE ThreadNative_SetIsBackground(QCall::ThreadHandle thread, BOOL value); extern "C" void QCALLTYPE ThreadNative_InformThreadNameChange(QCall::ThreadHandle thread, LPCWSTR name, INT32 len); extern "C" BOOL QCALLTYPE ThreadNative_YieldThread(); diff --git a/src/coreclr/vm/ecalllist.h b/src/coreclr/vm/ecalllist.h index 87c26409ffc76e..2b065fd3380341 100644 --- a/src/coreclr/vm/ecalllist.h +++ b/src/coreclr/vm/ecalllist.h @@ -305,8 +305,6 @@ FCFuncStart(gThreadFuncs) FCFuncElement("GetIsBackground", ThreadNative::GetIsBackground) FCFuncElement("get_IsThreadPoolThread", ThreadNative::IsThreadpoolThread) FCFuncElement("set_IsThreadPoolThread", ThreadNative::SetIsThreadpoolThread) - FCFuncElement("GetPriorityNative", ThreadNative::GetPriority) - FCFuncElement("SetPriorityNative", ThreadNative::SetPriority) FCFuncElement("GetThreadStateNative", ThreadNative::GetThreadState) FCFuncElement("get_OptimalMaxSpinWaitsPerSpinIteration", ThreadNative::GetOptimalMaxSpinWaitsPerSpinIteration) FCFuncEnd() diff --git a/src/coreclr/vm/qcallentrypoints.cpp b/src/coreclr/vm/qcallentrypoints.cpp index 35b7b07ce28a0c..c71ef67d9a8c53 100644 --- a/src/coreclr/vm/qcallentrypoints.cpp +++ b/src/coreclr/vm/qcallentrypoints.cpp @@ -234,6 +234,8 @@ static const Entry s_QCall[] = DllImportEntry(String_IsInterned) DllImportEntry(AppDomain_CreateDynamicAssembly) DllImportEntry(ThreadNative_Start) + DllImportEntry(ThreadNative_ThreadIsDead) + DllImportEntry(ThreadNative_TrySetPriority) DllImportEntry(ThreadNative_SetIsBackground) DllImportEntry(ThreadNative_InformThreadNameChange) DllImportEntry(ThreadNative_YieldThread) From 508d31058b32c55276765d29fa6cf73f5443738c Mon Sep 17 00:00:00 2001 From: Aaron R Robinson Date: Fri, 30 Aug 2024 18:51:37 -0700 Subject: [PATCH 06/16] Feedback on GC.AllocateNewArray() --- .../src/System/GC.CoreCLR.cs | 10 +++++----- src/coreclr/vm/comutilnative.cpp | 18 ++++++++++-------- src/coreclr/vm/comutilnative.h | 2 +- 3 files changed, 16 insertions(+), 14 deletions(-) diff --git a/src/coreclr/System.Private.CoreLib/src/System/GC.CoreCLR.cs b/src/coreclr/System.Private.CoreLib/src/System/GC.CoreCLR.cs index dbf7828bb66fce..3d71881eb44e41 100644 --- a/src/coreclr/System.Private.CoreLib/src/System/GC.CoreCLR.cs +++ b/src/coreclr/System.Private.CoreLib/src/System/GC.CoreCLR.cs @@ -104,7 +104,7 @@ internal enum GC_ALLOC_FLAGS }; [LibraryImport(RuntimeHelpers.QCall, EntryPoint = "GCInterface_AllocateNewArray")] - private static unsafe partial void AllocateNewArray(MethodTable* pMT, int length, GC_ALLOC_FLAGS flags, ObjectHandleOnStack ret); + private static partial void AllocateNewArray(IntPtr typeHandlePtr, int length, GC_ALLOC_FLAGS flags, ObjectHandleOnStack ret); [LibraryImport(RuntimeHelpers.QCall, EntryPoint = "GCInterface_GetTotalMemory")] private static partial long GetTotalMemory(); @@ -775,7 +775,7 @@ internal static void UnregisterMemoryLoadChangeNotification(Action notification) /// Specifies the length of the array. /// Specifies whether the allocated array must be pinned. [MethodImpl(MethodImplOptions.AggressiveInlining)] // forced to ensure no perf drop for small memory buffers (hot path) - public static unsafe T[] AllocateUninitializedArray(int length, bool pinned = false) // T[] rather than T?[] to match `new T[length]` behavior + public static T[] AllocateUninitializedArray(int length, bool pinned = false) // T[] rather than T?[] to match `new T[length]` behavior { if (!pinned) { @@ -801,7 +801,7 @@ public static unsafe T[] AllocateUninitializedArray(int length, bool pinned = flags |= GC_ALLOC_FLAGS.GC_ALLOC_PINNED_OBJECT_HEAP; T[]? result = null; - AllocateNewArray(TypeHandle.TypeHandleOf().AsMethodTable(), length, flags, ObjectHandleOnStack.Create(ref result)); + AllocateNewArray(RuntimeTypeHandle.ToIntPtr(typeof(T[]).TypeHandle), length, flags, ObjectHandleOnStack.Create(ref result)); return result!; } @@ -811,7 +811,7 @@ public static unsafe T[] AllocateUninitializedArray(int length, bool pinned = /// Specifies the type of the array element. /// Specifies the length of the array. /// Specifies whether the allocated array must be pinned. - public static unsafe T[] AllocateArray(int length, bool pinned = false) // T[] rather than T?[] to match `new T[length]` behavior + public static T[] AllocateArray(int length, bool pinned = false) // T[] rather than T?[] to match `new T[length]` behavior { GC_ALLOC_FLAGS flags = GC_ALLOC_FLAGS.GC_ALLOC_NO_FLAGS; @@ -821,7 +821,7 @@ public static unsafe T[] AllocateArray(int length, bool pinned = false) // T[ } T[]? result = null; - AllocateNewArray(TypeHandle.TypeHandleOf().AsMethodTable(), length, flags, ObjectHandleOnStack.Create(ref result)); + AllocateNewArray(RuntimeTypeHandle.ToIntPtr(typeof(T[]).TypeHandle), length, flags, ObjectHandleOnStack.Create(ref result)); return result!; } diff --git a/src/coreclr/vm/comutilnative.cpp b/src/coreclr/vm/comutilnative.cpp index 08afd14ea9c777..f15b1085ebab4e 100644 --- a/src/coreclr/vm/comutilnative.cpp +++ b/src/coreclr/vm/comutilnative.cpp @@ -921,25 +921,27 @@ FCIMPLEND /*===============================AllocateNewArray=============================== **Action: Allocates a new array object. Allows passing extra flags -**Returns: The allocated array. -**Arguments: elementTypeHandle -> type of the element, -** length -> number of elements, -** zeroingOptional -> whether caller prefers to skip clearing the content of the array, if possible. +**Arguments: typeHandlePtr -> TypeHandle pointer of array, +** length -> Number of elements, +** flags -> Flags that impact allocated memory, +** ret -> The allocated array. **Exceptions: IDS_EE_ARRAY_DIMENSIONS_EXCEEDED when size is too large. OOM if can't allocate. ==============================================================================*/ -extern "C" void QCALLTYPE GCInterface_AllocateNewArray(MethodTable* pMT, INT32 length, INT32 flags, QCall::ObjectHandleOnStack ret) +extern "C" void QCALLTYPE GCInterface_AllocateNewArray(void* typeHandlePtr, INT32 length, INT32 flags, QCall::ObjectHandleOnStack ret) { QCALL_CONTRACT; - - _ASSERTE(pMT != NULL); + _ASSERTE(typeHandlePtr != NULL); BEGIN_QCALL; GCX_COOP(); + TypeHandle typeHandle = TypeHandle::FromPtr(typeHandlePtr); + _ASSERTE(typeHandle.IsArray()); + //Only the following flags are used by GC.cs, so we'll just assert it here. _ASSERTE((flags & ~(GC_ALLOC_ZEROING_OPTIONAL | GC_ALLOC_PINNED_OBJECT_HEAP)) == 0); - ret.Set(AllocateSzArray(pMT, length, (GC_ALLOC_FLAGS)flags)); + ret.Set(AllocateSzArray(typeHandle, length, (GC_ALLOC_FLAGS)flags)); END_QCALL; } diff --git a/src/coreclr/vm/comutilnative.h b/src/coreclr/vm/comutilnative.h index 99a872ae6c176c..ef41239a6bb0f3 100644 --- a/src/coreclr/vm/comutilnative.h +++ b/src/coreclr/vm/comutilnative.h @@ -202,7 +202,7 @@ class GCInterface { extern "C" INT64 QCALLTYPE GCInterface_GetTotalAllocatedBytesPrecise(); -extern "C" void QCALLTYPE GCInterface_AllocateNewArray(MethodTable* pMT, INT32 length, INT32 flags, QCall::ObjectHandleOnStack ret); +extern "C" void QCALLTYPE GCInterface_AllocateNewArray(void* typeHandlePtr, INT32 length, INT32 flags, QCall::ObjectHandleOnStack ret); extern "C" INT64 QCALLTYPE GCInterface_GetTotalMemory(); From 3627b892e2fe902cd10c26eb919d61487cf363b4 Mon Sep 17 00:00:00 2001 From: Aaron R Robinson Date: Fri, 30 Aug 2024 19:06:48 -0700 Subject: [PATCH 07/16] Add back unsafe for GC.AllocateNewArray(). --- src/coreclr/System.Private.CoreLib/src/System/GC.CoreCLR.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/coreclr/System.Private.CoreLib/src/System/GC.CoreCLR.cs b/src/coreclr/System.Private.CoreLib/src/System/GC.CoreCLR.cs index 3d71881eb44e41..44108bf7413bda 100644 --- a/src/coreclr/System.Private.CoreLib/src/System/GC.CoreCLR.cs +++ b/src/coreclr/System.Private.CoreLib/src/System/GC.CoreCLR.cs @@ -775,7 +775,7 @@ internal static void UnregisterMemoryLoadChangeNotification(Action notification) /// Specifies the length of the array. /// Specifies whether the allocated array must be pinned. [MethodImpl(MethodImplOptions.AggressiveInlining)] // forced to ensure no perf drop for small memory buffers (hot path) - public static T[] AllocateUninitializedArray(int length, bool pinned = false) // T[] rather than T?[] to match `new T[length]` behavior + public static unsafe T[] AllocateUninitializedArray(int length, bool pinned = false) // T[] rather than T?[] to match `new T[length]` behavior { if (!pinned) { From ba71ac70a16a072ef23fbfa2d0c6979b39de2f8c Mon Sep 17 00:00:00 2001 From: Aaron R Robinson Date: Tue, 3 Sep 2024 10:08:13 -0700 Subject: [PATCH 08/16] Remove P/Invoke frame overhead from fast path. - GC.AllocateNewArray() --- .../System.Private.CoreLib/src/System/GC.CoreCLR.cs | 12 +++++++++--- 1 file changed, 9 insertions(+), 3 deletions(-) diff --git a/src/coreclr/System.Private.CoreLib/src/System/GC.CoreCLR.cs b/src/coreclr/System.Private.CoreLib/src/System/GC.CoreCLR.cs index 44108bf7413bda..2c86622ecf6a63 100644 --- a/src/coreclr/System.Private.CoreLib/src/System/GC.CoreCLR.cs +++ b/src/coreclr/System.Private.CoreLib/src/System/GC.CoreCLR.cs @@ -800,9 +800,15 @@ public static unsafe T[] AllocateUninitializedArray(int length, bool pinned = if (pinned) flags |= GC_ALLOC_FLAGS.GC_ALLOC_PINNED_OBJECT_HEAP; - T[]? result = null; - AllocateNewArray(RuntimeTypeHandle.ToIntPtr(typeof(T[]).TypeHandle), length, flags, ObjectHandleOnStack.Create(ref result)); - return result!; + return AllocateNewArrayWorker(length, flags); + + [MethodImpl(MethodImplOptions.NoInlining)] + static U[] AllocateNewArrayWorker(int length, GC_ALLOC_FLAGS flags) + { + U[]? result = null; + AllocateNewArray(RuntimeTypeHandle.ToIntPtr(typeof(U[]).TypeHandle), length, flags, ObjectHandleOnStack.Create(ref result)); + return result!; + } } /// From 243de063099070339f2ecb06a404f044dac402b4 Mon Sep 17 00:00:00 2001 From: Aaron R Robinson Date: Tue, 3 Sep 2024 10:46:41 -0700 Subject: [PATCH 09/16] Update GC.AllocateNewArray() impl. --- .../src/System/GC.CoreCLR.cs | 21 ++++++++++--------- 1 file changed, 11 insertions(+), 10 deletions(-) diff --git a/src/coreclr/System.Private.CoreLib/src/System/GC.CoreCLR.cs b/src/coreclr/System.Private.CoreLib/src/System/GC.CoreCLR.cs index 2c86622ecf6a63..f8246642b9e61b 100644 --- a/src/coreclr/System.Private.CoreLib/src/System/GC.CoreCLR.cs +++ b/src/coreclr/System.Private.CoreLib/src/System/GC.CoreCLR.cs @@ -791,22 +791,23 @@ public static unsafe T[] AllocateUninitializedArray(int length, bool pinned = { return new T[length]; } - #endif } - // Runtime overrides GC_ALLOC_ZEROING_OPTIONAL if the type contains references, so we don't need to worry about that. - GC_ALLOC_FLAGS flags = GC_ALLOC_FLAGS.GC_ALLOC_ZEROING_OPTIONAL; - if (pinned) - flags |= GC_ALLOC_FLAGS.GC_ALLOC_PINNED_OBJECT_HEAP; - - return AllocateNewArrayWorker(length, flags); + return AllocateNewArrayWorker(length, pinned); [MethodImpl(MethodImplOptions.NoInlining)] - static U[] AllocateNewArrayWorker(int length, GC_ALLOC_FLAGS flags) + static T[] AllocateNewArrayWorker(int length, bool pinned) { - U[]? result = null; - AllocateNewArray(RuntimeTypeHandle.ToIntPtr(typeof(U[]).TypeHandle), length, flags, ObjectHandleOnStack.Create(ref result)); + // Runtime overrides GC_ALLOC_ZEROING_OPTIONAL if the type contains references, so we don't need to worry about that. + GC_ALLOC_FLAGS flags = GC_ALLOC_FLAGS.GC_ALLOC_ZEROING_OPTIONAL; + if (pinned) + { + flags |= GC_ALLOC_FLAGS.GC_ALLOC_PINNED_OBJECT_HEAP; + } + + T[]? result = null; + AllocateNewArray(RuntimeTypeHandle.ToIntPtr(typeof(T[]).TypeHandle), length, flags, ObjectHandleOnStack.Create(ref result)); return result!; } } From 67264314511caf9b331db4d6aefce9abc7b57574 Mon Sep 17 00:00:00 2001 From: Aaron R Robinson Date: Tue, 3 Sep 2024 16:09:14 -0700 Subject: [PATCH 10/16] Feedback for Thread.Priority property --- .../src/System/Threading/Thread.CoreCLR.cs | 31 +++----- src/coreclr/vm/comsynchronizable.cpp | 71 +++++++++++-------- src/coreclr/vm/comsynchronizable.h | 3 +- src/coreclr/vm/corelib.h | 1 + src/coreclr/vm/object.h | 9 +++ src/coreclr/vm/qcallentrypoints.cpp | 3 +- src/coreclr/vm/threads.cpp | 4 ++ 7 files changed, 65 insertions(+), 57 deletions(-) diff --git a/src/coreclr/System.Private.CoreLib/src/System/Threading/Thread.CoreCLR.cs b/src/coreclr/System.Private.CoreLib/src/System/Threading/Thread.CoreCLR.cs index de301e62a972fe..e0fa5e5a0914b0 100644 --- a/src/coreclr/System.Private.CoreLib/src/System/Threading/Thread.CoreCLR.cs +++ b/src/coreclr/System.Private.CoreLib/src/System/Threading/Thread.CoreCLR.cs @@ -58,6 +58,9 @@ public sealed partial class Thread // but those types of changes may race with the reset anyway, so this field doesn't need to be synchronized. private bool _mayNeedResetForThreadPool; + // Set in unmanaged and read in managed code. + private bool _isDead; + private Thread() { } public int ManagedThreadId @@ -211,20 +214,16 @@ public extern bool IsThreadPoolThread internal set; } - [LibraryImport(RuntimeHelpers.QCall, EntryPoint = "ThreadNative_ThreadIsDead")] - [return: MarshalAs(UnmanagedType.Bool)] - private static partial bool ThreadIsDead(ThreadHandle t); - - [LibraryImport(RuntimeHelpers.QCall, EntryPoint = "ThreadNative_TrySetPriority")] + [LibraryImport(RuntimeHelpers.QCall, EntryPoint = "ThreadNative_SetPriority")] [return: MarshalAs(UnmanagedType.Bool)] - private static partial bool TrySetPriority(ThreadHandle t, int priority); + private static partial void SetPriority(ObjectHandleOnStack thread, int priority); /// Returns the priority of the thread. public ThreadPriority Priority { get { - if (ThreadIsDead(GetNativeHandle())) // GC.KeepAlive() not needed since member fields are touched below. + if (_isDead) { throw new ThreadStateException(SR.ThreadState_Dead_Priority); } @@ -232,22 +231,8 @@ public ThreadPriority Priority } set { - ThreadHandle handle = GetNativeHandle(); - if (ThreadIsDead(handle)) // GC.KeepAlive() not needed since member fields are touched below. - { - // Note that you can manipulate the priority of a thread that hasn't started yet, - // or one that is running. But you get an exception if you manipulate the priority - // of a thread that has died. - throw new ThreadStateException(SR.ThreadState_Dead_Priority); - } - - int prev = _priority; - _priority = (int)value; - if (!TrySetPriority(handle, _priority)) // GC.KeepAlive() not needed since member fields are touched below. - { - _priority = prev; - throw new ThreadStateException(SR.ThreadState_SetPriorityFailed); - } + Thread _this = this; + SetPriority(ObjectHandleOnStack.Create(ref _this), (int)value); if (value != ThreadPriority.Normal) { _mayNeedResetForThreadPool = true; diff --git a/src/coreclr/vm/comsynchronizable.cpp b/src/coreclr/vm/comsynchronizable.cpp index 18d56116eb4492..a689eb608496c1 100644 --- a/src/coreclr/vm/comsynchronizable.cpp +++ b/src/coreclr/vm/comsynchronizable.cpp @@ -59,39 +59,38 @@ static inline BOOL ThreadIsDead(Thread *t) // Map our exposed notion of thread priorities into the enumeration that NT uses. -static bool TryMapToNTPriority(INT32 ours, INT32& theirs) +static INT32 MapToNTPriority(INT32 ours) { - STANDARD_VM_CONTRACT; + CONTRACTL + { + GC_NOTRIGGER; + THROWS; + MODE_ANY; + } + CONTRACTL_END; switch (ours) { case ThreadNative::PRIORITY_LOWEST: - theirs = THREAD_PRIORITY_LOWEST; - break; + return THREAD_PRIORITY_LOWEST; case ThreadNative::PRIORITY_BELOW_NORMAL: - theirs = THREAD_PRIORITY_BELOW_NORMAL; - break; + return THREAD_PRIORITY_BELOW_NORMAL; case ThreadNative::PRIORITY_NORMAL: - theirs = THREAD_PRIORITY_NORMAL; - break; + return THREAD_PRIORITY_NORMAL; case ThreadNative::PRIORITY_ABOVE_NORMAL: - theirs = THREAD_PRIORITY_ABOVE_NORMAL; - break; + return THREAD_PRIORITY_ABOVE_NORMAL; case ThreadNative::PRIORITY_HIGHEST: - theirs = THREAD_PRIORITY_HIGHEST; - break; + return THREAD_PRIORITY_HIGHEST; default: - return false; + COMPlusThrow(kArgumentOutOfRangeException, W("Argument_InvalidFlag")); } - return true; } - // Map to our exposed notion of thread priorities from the enumeration that NT uses. INT32 MapFromNTPriority(INT32 NTPriority) { @@ -286,9 +285,7 @@ void ThreadNative::Start(Thread* pNewThread, int threadStackSize, int priority, // After we have established the thread handle, we can check m_Priority. // This ordering is required to eliminate the race condition on setting the // priority of a thread just as it starts up. - INT32 NTPriority; - if (!TryMapToNTPriority(priority, NTPriority)) - COMPlusThrow(kArgumentOutOfRangeException, W("Argument_InvalidFlag")); + INT32 NTPriority = MapToNTPriority(priority); pNewThread->SetThreadPriority(NTPriority); pNewThread->ChooseThreadCPUGroupAffinity(); @@ -324,28 +321,42 @@ void ThreadNative::Start(Thread* pNewThread, int threadStackSize, int priority, } } -extern "C" INT32 QCALLTYPE ThreadNative_ThreadIsDead(QCall::ThreadHandle thread) +extern "C" void QCALLTYPE ThreadNative_SetPriority(QCall::ObjectHandleOnStack thread, INT32 iPriority) { QCALL_CONTRACT; - return ThreadIsDead(thread); -} + BEGIN_QCALL; -extern "C" BOOL QCALLTYPE ThreadNative_TrySetPriority(QCall::ThreadHandle thread, INT32 iPriority) -{ - QCALL_CONTRACT; + GCX_COOP(); - BOOL ret = FALSE; + THREADBASEREF threadRef = NULL; + GCPROTECT_BEGIN(threadRef) + threadRef = (THREADBASEREF)thread.Get(); - BEGIN_QCALL; + // Note that you can manipulate the priority of a thread that hasn't started yet, + // or one that is running. But you get an exception if you manipulate the priority + // of a thread that has died. + Thread* th = threadRef->GetInternal(); + if (ThreadIsDead(th)) + COMPlusThrow(kThreadStateException, W("ThreadState_Dead_Priority")); // translate the priority (validating as well) - INT32 priority; - ret = TryMapToNTPriority(iPriority, priority) && thread->SetThreadPriority(priority); + INT32 priority = MapToNTPriority(iPriority); - END_QCALL; + INT32 oldPriority = threadRef->GetPriority(); - return ret; + // Eliminate the race condition by setting priority field before we check for if + // the thread is running. See ThreadNative::Start() for the other half. + threadRef->SetPriority(iPriority); + + if (!th->SetThreadPriority(priority)) + { + threadRef->SetPriority(oldPriority); + COMPlusThrow(kThreadStateException, W("ThreadState_SetPriorityFailed")); + } + + GCPROTECT_END(); + END_QCALL; } FCIMPL1(FC_BOOL_RET, ThreadNative::IsAlive, ThreadBaseObject* pThisUNSAFE) diff --git a/src/coreclr/vm/comsynchronizable.h b/src/coreclr/vm/comsynchronizable.h index acbc4384ea0a24..6f5283f52c20ac 100644 --- a/src/coreclr/vm/comsynchronizable.h +++ b/src/coreclr/vm/comsynchronizable.h @@ -78,8 +78,7 @@ friend class ThreadBaseObject; }; extern "C" void QCALLTYPE ThreadNative_Start(QCall::ThreadHandle thread, int threadStackSize, int priority, PCWSTR pThreadName); -extern "C" BOOL QCALLTYPE ThreadNative_ThreadIsDead(QCall::ThreadHandle thread); -extern "C" BOOL QCALLTYPE ThreadNative_TrySetPriority(QCall::ThreadHandle thread, INT32 iPriority); +extern "C" void QCALLTYPE ThreadNative_SetPriority(QCall::ObjectHandleOnStack thread, INT32 iPriority); extern "C" void QCALLTYPE ThreadNative_SetIsBackground(QCall::ThreadHandle thread, BOOL value); extern "C" void QCALLTYPE ThreadNative_InformThreadNameChange(QCall::ThreadHandle thread, LPCWSTR name, INT32 len); extern "C" BOOL QCALLTYPE ThreadNative_YieldThread(); diff --git a/src/coreclr/vm/corelib.h b/src/coreclr/vm/corelib.h index 25d0163e4f172e..ec5ee0795a504f 100644 --- a/src/coreclr/vm/corelib.h +++ b/src/coreclr/vm/corelib.h @@ -835,6 +835,7 @@ DEFINE_FIELD_U(_name, ThreadBaseObject, m_Name) DEFINE_FIELD_U(_startHelper, ThreadBaseObject, m_StartHelper) DEFINE_FIELD_U(_DONT_USE_InternalThread, ThreadBaseObject, m_InternalThread) DEFINE_FIELD_U(_priority, ThreadBaseObject, m_Priority) +DEFINE_FIELD_U(_isDead, ThreadBaseObject, m_IsDead) DEFINE_CLASS(THREAD, Threading, Thread) DEFINE_METHOD(THREAD, START_CALLBACK, StartCallback, IM_RetVoid) #ifdef FEATURE_OBJCMARSHAL diff --git a/src/coreclr/vm/object.h b/src/coreclr/vm/object.h index 66650b57b2ee1a..a7860bcec4af79 100644 --- a/src/coreclr/vm/object.h +++ b/src/coreclr/vm/object.h @@ -1333,6 +1333,9 @@ class ThreadBaseObject : public Object // Only used by managed code, see comment there bool m_MayNeedResetForThreadPool; + // Set in unmanaged code and read in managed code. + bool m_IsDead; + protected: // the ctor and dtor can do no useful work. ThreadBaseObject() {LIMITED_METHOD_CONTRACT;}; @@ -1384,6 +1387,12 @@ class ThreadBaseObject : public Object LIMITED_METHOD_CONTRACT; return m_Priority; } + + void SetIsDead() + { + LIMITED_METHOD_CONTRACT; + m_IsDead = true; + } }; // MarshalByRefObjectBaseObject diff --git a/src/coreclr/vm/qcallentrypoints.cpp b/src/coreclr/vm/qcallentrypoints.cpp index c71ef67d9a8c53..2d2345ca214c12 100644 --- a/src/coreclr/vm/qcallentrypoints.cpp +++ b/src/coreclr/vm/qcallentrypoints.cpp @@ -234,8 +234,7 @@ static const Entry s_QCall[] = DllImportEntry(String_IsInterned) DllImportEntry(AppDomain_CreateDynamicAssembly) DllImportEntry(ThreadNative_Start) - DllImportEntry(ThreadNative_ThreadIsDead) - DllImportEntry(ThreadNative_TrySetPriority) + DllImportEntry(ThreadNative_SetPriority) DllImportEntry(ThreadNative_SetIsBackground) DllImportEntry(ThreadNative_InformThreadNameChange) DllImportEntry(ThreadNative_YieldThread) diff --git a/src/coreclr/vm/threads.cpp b/src/coreclr/vm/threads.cpp index 8c60b2b5a7982f..7c147df023764c 100644 --- a/src/coreclr/vm/threads.cpp +++ b/src/coreclr/vm/threads.cpp @@ -2765,6 +2765,10 @@ void Thread::CooperativeCleanup() // Clear out the alloc context pointer for this thread. When TLS is gone, this pointer will point into freed memory. m_pRuntimeThreadLocals = nullptr; } + + OBJECTREF threadObjMaybe = GetExposedObjectRaw(); + if (threadObjMaybe != NULL) + ((THREADBASEREF)threadObjMaybe)->SetIsDead(); } // See general comments on thread destruction (code:#threadDestruction) above. From 15146f6d5bb00b1fd2d26eeb94c08396d24a547f Mon Sep 17 00:00:00 2001 From: Aaron R Robinson Date: Wed, 4 Sep 2024 10:25:23 -0700 Subject: [PATCH 11/16] Remove Thread::ResetApartment() --- src/coreclr/vm/threads.cpp | 14 -------------- src/coreclr/vm/threads.h | 4 ---- 2 files changed, 18 deletions(-) diff --git a/src/coreclr/vm/threads.cpp b/src/coreclr/vm/threads.cpp index 7c147df023764c..f9fbfec9bc6611 100644 --- a/src/coreclr/vm/threads.cpp +++ b/src/coreclr/vm/threads.cpp @@ -4785,20 +4785,6 @@ Thread::ApartmentState Thread::GetFinalApartment() return as; } -// when we get apartment tear-down notification, -// we want reset the apartment state we cache on the thread -VOID Thread::ResetApartment() -{ - CONTRACTL { - NOTHROW; - GC_NOTRIGGER; - } - CONTRACTL_END; - - // reset the TS_InSTA bit and TS_InMTA bit - ResetThreadState((Thread::ThreadState)(TS_InSTA | TS_InMTA)); -} - // Attempt to set current thread's apartment state. The actual apartment state // achieved is returned and may differ from the input state if someone managed // to call CoInitializeEx on this thread first (note that calls to SetApartment diff --git a/src/coreclr/vm/threads.h b/src/coreclr/vm/threads.h index 6aa3e04b004652..f3b7894c3dea64 100644 --- a/src/coreclr/vm/threads.h +++ b/src/coreclr/vm/threads.h @@ -2168,10 +2168,6 @@ class Thread // call CoInitializeEx on this thread first (note that calls to SetApartment made // before the thread has started are guaranteed to succeed). ApartmentState SetApartment(ApartmentState state); - - // when we get apartment tear-down notification, - // we want reset the apartment state we cache on the thread - VOID ResetApartment(); #endif // FEATURE_COMINTEROP_APARTMENT_SUPPORT // Either perform WaitForSingleObject or MsgWaitForSingleObject as appropriate. From c47add4ec7d5ae916f19ffb902efaa8dde3f4444 Mon Sep 17 00:00:00 2001 From: Aaron R Robinson Date: Wed, 4 Sep 2024 10:39:31 -0700 Subject: [PATCH 12/16] Change Thread.GetNativeHandle() to use ThreadStateException. --- .../src/System/Threading/Thread.CoreCLR.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/coreclr/System.Private.CoreLib/src/System/Threading/Thread.CoreCLR.cs b/src/coreclr/System.Private.CoreLib/src/System/Threading/Thread.CoreCLR.cs index e0fa5e5a0914b0..e69436fa33e4e4 100644 --- a/src/coreclr/System.Private.CoreLib/src/System/Threading/Thread.CoreCLR.cs +++ b/src/coreclr/System.Private.CoreLib/src/System/Threading/Thread.CoreCLR.cs @@ -77,7 +77,7 @@ internal ThreadHandle GetNativeHandle() // This should never happen under normal circumstances. if (thread == IntPtr.Zero) { - throw new ArgumentException(null, SR.Argument_InvalidHandle); + throw new ThreadStateException(SR.Argument_InvalidHandle); } return new ThreadHandle(thread); From 9e6a2c997c31bd7418bac3f168789f5693018c2c Mon Sep 17 00:00:00 2001 From: Aaron R Robinson Date: Wed, 4 Sep 2024 10:40:25 -0700 Subject: [PATCH 13/16] Rework Thread.Join() conversion. --- .../src/System/Threading/Thread.CoreCLR.cs | 7 +++---- src/coreclr/vm/comsynchronizable.cpp | 13 ++++++++----- src/coreclr/vm/comsynchronizable.h | 2 +- 3 files changed, 12 insertions(+), 10 deletions(-) diff --git a/src/coreclr/System.Private.CoreLib/src/System/Threading/Thread.CoreCLR.cs b/src/coreclr/System.Private.CoreLib/src/System/Threading/Thread.CoreCLR.cs index e69436fa33e4e4..441de1198503f5 100644 --- a/src/coreclr/System.Private.CoreLib/src/System/Threading/Thread.CoreCLR.cs +++ b/src/coreclr/System.Private.CoreLib/src/System/Threading/Thread.CoreCLR.cs @@ -349,7 +349,7 @@ public void Interrupt() [LibraryImport(RuntimeHelpers.QCall, EntryPoint = "ThreadNative_Join")] [return: MarshalAs(UnmanagedType.Bool)] - private static partial bool Join(ThreadHandle t, int millisecondsTimeout); + private static partial bool Join(ObjectHandleOnStack thread, int millisecondsTimeout); /// /// Waits for the thread to die or for timeout milliseconds to elapse. @@ -369,9 +369,8 @@ public bool Join(int millisecondsTimeout) throw new ArgumentOutOfRangeException(nameof(millisecondsTimeout), SR.ArgumentOutOfRange_NeedNonNegOrNegative1); } - bool res = Join(GetNativeHandle(), millisecondsTimeout); - GC.KeepAlive(this); - return res; + Thread _this = this; + return Join(ObjectHandleOnStack.Create(ref _this), millisecondsTimeout); } /// diff --git a/src/coreclr/vm/comsynchronizable.cpp b/src/coreclr/vm/comsynchronizable.cpp index a689eb608496c1..52044032a7eab6 100644 --- a/src/coreclr/vm/comsynchronizable.cpp +++ b/src/coreclr/vm/comsynchronizable.cpp @@ -619,18 +619,19 @@ void ReleaseThreadExternalCount(Thread * pThread) typedef Holder ThreadExternalCountHolder; // Wait for the thread to die -static BOOL DoJoin(QCall::ThreadHandle dyingThreadHandle, INT32 timeout) +static BOOL DoJoin(THREADBASEREF dyingThread, INT32 timeout) { CONTRACTL { THROWS; GC_TRIGGERS; - MODE_PREEMPTIVE; + MODE_COOPERATIVE; + PRECONDITION(dyingThread != NULL); PRECONDITION((timeout >= 0) || (timeout == INFINITE_TIMEOUT)); } CONTRACTL_END; - Thread * DyingInternal = dyingThreadHandle; + Thread* DyingInternal = dyingThread->GetInternal(); // Validate the handle. It's valid to Join a thread that's not running -- so // long as it was once started. @@ -666,6 +667,7 @@ static BOOL DoJoin(QCall::ThreadHandle dyingThreadHandle, INT32 timeout) return TRUE; } + GCX_PREEMP(); DWORD dwTimeOut32 = (timeout == INFINITE_TIMEOUT ? INFINITE : (DWORD) timeout); @@ -692,7 +694,7 @@ static BOOL DoJoin(QCall::ThreadHandle dyingThreadHandle, INT32 timeout) return FALSE; } -extern "C" BOOL QCALLTYPE ThreadNative_Join(QCall::ThreadHandle thread, INT32 Timeout) +extern "C" BOOL QCALLTYPE ThreadNative_Join(QCall::ObjectHandleOnStack thread, INT32 Timeout) { QCALL_CONTRACT; @@ -700,7 +702,8 @@ extern "C" BOOL QCALLTYPE ThreadNative_Join(QCall::ThreadHandle thread, INT32 Ti BEGIN_QCALL; - retVal = DoJoin(thread, Timeout); + GCX_COOP(); + retVal = DoJoin((THREADBASEREF)thread.Get(), Timeout); END_QCALL; diff --git a/src/coreclr/vm/comsynchronizable.h b/src/coreclr/vm/comsynchronizable.h index 6f5283f52c20ac..24cbef44125d7d 100644 --- a/src/coreclr/vm/comsynchronizable.h +++ b/src/coreclr/vm/comsynchronizable.h @@ -89,7 +89,7 @@ extern "C" INT32 QCALLTYPE ThreadNative_GetApartmentState(QCall::ThreadHandle th extern "C" INT32 QCALLTYPE ThreadNative_SetApartmentState(QCall::ThreadHandle thread, INT32 iState); #endif // FEATURE_COMINTEROP_APARTMENT_SUPPORT -extern "C" BOOL QCALLTYPE ThreadNative_Join(QCall::ThreadHandle thread, INT32 Timeout); +extern "C" BOOL QCALLTYPE ThreadNative_Join(QCall::ObjectHandleOnStack thread, INT32 Timeout); extern "C" void QCALLTYPE ThreadNative_Abort(QCall::ThreadHandle thread); extern "C" void QCALLTYPE ThreadNative_ResetAbort(); extern "C" void QCALLTYPE ThreadNative_SpinWait(INT32 iterations); From 937a9a0db3f8814c613b95bef94059e561daf302 Mon Sep 17 00:00:00 2001 From: Aaron R Robinson Date: Wed, 4 Sep 2024 15:34:16 -0700 Subject: [PATCH 14/16] Fix trimmer issue with System.Array. --- src/tools/illink/src/linker/Linker.Steps/MarkStep.cs | 1 + 1 file changed, 1 insertion(+) diff --git a/src/tools/illink/src/linker/Linker.Steps/MarkStep.cs b/src/tools/illink/src/linker/Linker.Steps/MarkStep.cs index 2e41771ee39634..a687214e1c17a8 100644 --- a/src/tools/illink/src/linker/Linker.Steps/MarkStep.cs +++ b/src/tools/illink/src/linker/Linker.Steps/MarkStep.cs @@ -2598,6 +2598,7 @@ protected virtual bool AlwaysMarkTypeAsInstantiated (TypeDefinition td) case "MulticastDelegate": case "ValueType": case "Enum": + case "Array": return td.Namespace == "System"; } From b60a2b4b128d779079891598156e416f64370a00 Mon Sep 17 00:00:00 2001 From: Aaron R Robinson Date: Wed, 4 Sep 2024 15:34:46 -0700 Subject: [PATCH 15/16] Feedback for Thread.GetApartmentState() --- .../src/System/Threading/Thread.CoreCLR.cs | 7 +++---- src/coreclr/vm/comsynchronizable.cpp | 20 ++++++++++--------- src/coreclr/vm/comsynchronizable.h | 2 +- 3 files changed, 15 insertions(+), 14 deletions(-) diff --git a/src/coreclr/System.Private.CoreLib/src/System/Threading/Thread.CoreCLR.cs b/src/coreclr/System.Private.CoreLib/src/System/Threading/Thread.CoreCLR.cs index 441de1198503f5..ef8a6d5e718cfb 100644 --- a/src/coreclr/System.Private.CoreLib/src/System/Threading/Thread.CoreCLR.cs +++ b/src/coreclr/System.Private.CoreLib/src/System/Threading/Thread.CoreCLR.cs @@ -258,16 +258,15 @@ public ThreadPriority Priority /// #if FEATURE_COMINTEROP_APARTMENT_SUPPORT [LibraryImport(RuntimeHelpers.QCall, EntryPoint = "ThreadNative_GetApartmentState")] - private static partial int GetApartmentState(ThreadHandle t); + private static partial int GetApartmentState(ObjectHandleOnStack t); [LibraryImport(RuntimeHelpers.QCall, EntryPoint = "ThreadNative_SetApartmentState")] private static partial int SetApartmentState(ThreadHandle t, int state); public ApartmentState GetApartmentState() { - var state = (ApartmentState)GetApartmentState(GetNativeHandle()); - GC.KeepAlive(this); - return state; + Thread _this = this; + return (ApartmentState)GetApartmentState(ObjectHandleOnStack.Create(ref _this)); } private bool SetApartmentStateUnchecked(ApartmentState state, bool throwOnError) diff --git a/src/coreclr/vm/comsynchronizable.cpp b/src/coreclr/vm/comsynchronizable.cpp index 52044032a7eab6..9db69543f12ca3 100644 --- a/src/coreclr/vm/comsynchronizable.cpp +++ b/src/coreclr/vm/comsynchronizable.cpp @@ -542,21 +542,23 @@ FCIMPLEND // Return whether the thread hosts an STA, is a member of the MTA or is not // currently initialized for COM. -extern "C" INT32 QCALLTYPE ThreadNative_GetApartmentState(QCall::ThreadHandle thread) +extern "C" INT32 QCALLTYPE ThreadNative_GetApartmentState(QCall::ObjectHandleOnStack t) { - CONTRACTL - { - QCALL_CHECK; - PRECONDITION(thread != NULL); - } - CONTRACTL_END; + QCALL_CONTRACT; INT32 retVal = 0; BEGIN_QCALL; - if (ThreadIsDead(thread)) - COMPlusThrow(kThreadStateException, W("ThreadState_Dead_State")); + Thread* thread = NULL; + { + GCX_COOP(); + THREADBASEREF threadRef = (THREADBASEREF)t.Get(); + thread = threadRef->GetInternal(); + + if (ThreadIsDead(thread)) + COMPlusThrow(kThreadStateException, W("ThreadState_Dead_State")); + } retVal = thread->GetApartment(); diff --git a/src/coreclr/vm/comsynchronizable.h b/src/coreclr/vm/comsynchronizable.h index 24cbef44125d7d..a345480b18aa29 100644 --- a/src/coreclr/vm/comsynchronizable.h +++ b/src/coreclr/vm/comsynchronizable.h @@ -85,7 +85,7 @@ extern "C" BOOL QCALLTYPE ThreadNative_YieldThread(); extern "C" UINT64 QCALLTYPE ThreadNative_GetCurrentOSThreadId(); #ifdef FEATURE_COMINTEROP_APARTMENT_SUPPORT -extern "C" INT32 QCALLTYPE ThreadNative_GetApartmentState(QCall::ThreadHandle thread); +extern "C" INT32 QCALLTYPE ThreadNative_GetApartmentState(QCall::ObjectHandleOnStack t); extern "C" INT32 QCALLTYPE ThreadNative_SetApartmentState(QCall::ThreadHandle thread, INT32 iState); #endif // FEATURE_COMINTEROP_APARTMENT_SUPPORT From 1d6a41bccccb9ca90a8c9e9feb54d75843450b52 Mon Sep 17 00:00:00 2001 From: Aaron R Robinson Date: Fri, 6 Sep 2024 10:54:23 -0700 Subject: [PATCH 16/16] Feedback on Thread method conversions. --- .../src/System/Threading/Thread.CoreCLR.cs | 7 +++-- src/coreclr/vm/comsynchronizable.cpp | 29 +++++++++++++------ src/coreclr/vm/comsynchronizable.h | 2 +- 3 files changed, 25 insertions(+), 13 deletions(-) diff --git a/src/coreclr/System.Private.CoreLib/src/System/Threading/Thread.CoreCLR.cs b/src/coreclr/System.Private.CoreLib/src/System/Threading/Thread.CoreCLR.cs index ef8a6d5e718cfb..486e5a505abeb7 100644 --- a/src/coreclr/System.Private.CoreLib/src/System/Threading/Thread.CoreCLR.cs +++ b/src/coreclr/System.Private.CoreLib/src/System/Threading/Thread.CoreCLR.cs @@ -261,7 +261,7 @@ public ThreadPriority Priority private static partial int GetApartmentState(ObjectHandleOnStack t); [LibraryImport(RuntimeHelpers.QCall, EntryPoint = "ThreadNative_SetApartmentState")] - private static partial int SetApartmentState(ThreadHandle t, int state); + private static partial int SetApartmentState(ObjectHandleOnStack t, int state); public ApartmentState GetApartmentState() { @@ -272,9 +272,10 @@ public ApartmentState GetApartmentState() private bool SetApartmentStateUnchecked(ApartmentState state, bool throwOnError) { ApartmentState retState; - lock (this) // This is also satisfying a GC.KeepAlive(). + lock (this) // This lock is only needed when the this is not the current thread. { - retState = (ApartmentState)SetApartmentState(GetNativeHandle(), (int)state); + Thread _this = this; + retState = (ApartmentState)SetApartmentState(ObjectHandleOnStack.Create(ref _this), (int)state); } // Special case where we pass in Unknown and get back MTA. diff --git a/src/coreclr/vm/comsynchronizable.cpp b/src/coreclr/vm/comsynchronizable.cpp index 9db69543f12ca3..73cc949ec66d5c 100644 --- a/src/coreclr/vm/comsynchronizable.cpp +++ b/src/coreclr/vm/comsynchronizable.cpp @@ -333,6 +333,9 @@ extern "C" void QCALLTYPE ThreadNative_SetPriority(QCall::ObjectHandleOnStack th GCPROTECT_BEGIN(threadRef) threadRef = (THREADBASEREF)thread.Get(); + if (threadRef == NULL) + COMPlusThrow(kNullReferenceException, W("NullReference_This")); + // Note that you can manipulate the priority of a thread that hasn't started yet, // or one that is running. But you get an exception if you manipulate the priority // of a thread that has died. @@ -554,6 +557,9 @@ extern "C" INT32 QCALLTYPE ThreadNative_GetApartmentState(QCall::ObjectHandleOnS { GCX_COOP(); THREADBASEREF threadRef = (THREADBASEREF)t.Get(); + if (threadRef == NULL) + COMPlusThrow(kNullReferenceException, W("NullReference_This")); + thread = threadRef->GetInternal(); if (ThreadIsDead(thread)) @@ -583,24 +589,29 @@ extern "C" INT32 QCALLTYPE ThreadNative_GetApartmentState(QCall::ObjectHandleOnS // Indicate whether the thread will host an STA (this may fail if the thread has // already been made part of the MTA, use GetApartmentState or the return state // from this routine to check for this). -extern "C" INT32 QCALLTYPE ThreadNative_SetApartmentState(QCall::ThreadHandle thread, INT32 iState) +extern "C" INT32 QCALLTYPE ThreadNative_SetApartmentState(QCall::ObjectHandleOnStack t, INT32 iState) { - CONTRACTL - { - QCALL_CHECK; - PRECONDITION(thread != NULL); - } - CONTRACTL_END; + QCALL_CONTRACT; INT32 retVal = 0; BEGIN_QCALL; + Thread* thread = NULL; + { + GCX_COOP(); + THREADBASEREF threadRef = (THREADBASEREF)t.Get(); + if (threadRef == NULL) + COMPlusThrow(kNullReferenceException, W("NullReference_This")); + + thread = threadRef->GetInternal(); + } + // We can only change the apartment if the thread is unstarted or // running, and if it's running we have to be in the thread's // context. - if ((!ThreadNotStarted(thread) && !ThreadIsRunning(thread)) - || (!ThreadNotStarted(thread) && (GetThread() != thread))) + if (!ThreadNotStarted(thread) + && (!ThreadIsRunning(thread) || (GetThread() != thread))) { COMPlusThrow(kThreadStateException); } diff --git a/src/coreclr/vm/comsynchronizable.h b/src/coreclr/vm/comsynchronizable.h index a345480b18aa29..9180216deeea90 100644 --- a/src/coreclr/vm/comsynchronizable.h +++ b/src/coreclr/vm/comsynchronizable.h @@ -86,7 +86,7 @@ extern "C" UINT64 QCALLTYPE ThreadNative_GetCurrentOSThreadId(); #ifdef FEATURE_COMINTEROP_APARTMENT_SUPPORT extern "C" INT32 QCALLTYPE ThreadNative_GetApartmentState(QCall::ObjectHandleOnStack t); -extern "C" INT32 QCALLTYPE ThreadNative_SetApartmentState(QCall::ThreadHandle thread, INT32 iState); +extern "C" INT32 QCALLTYPE ThreadNative_SetApartmentState(QCall::ObjectHandleOnStack t, INT32 iState); #endif // FEATURE_COMINTEROP_APARTMENT_SUPPORT extern "C" BOOL QCALLTYPE ThreadNative_Join(QCall::ObjectHandleOnStack thread, INT32 Timeout);