Misc cleanup of VM threading code (dotnet#108171)

- Delete special-casing of thread finalizer
- Factor out Get/SetApartment of unstarted threads
- Delete some dead code
- Fix comments

Author: jkotas

src/coreclr/System.Private.CoreLib/src/System/Threading/Thread.CoreCLR.cs

@@ -240,10 +240,7 @@ public ThreadPriority Priority
             {
                 Thread _this = this;
                 SetPriority(ObjectHandleOnStack.Create(ref _this), (int)value);
-                if (value != ThreadPriority.Normal)
-                {
-                    _mayNeedResetForThreadPool = true;
-                }
+                _mayNeedResetForThreadPool = true;
             }
         }
 

src/coreclr/nativeaot/System.Private.CoreLib/src/System/Threading/Thread.NativeAot.Windows.cs

@@ -14,9 +14,6 @@ namespace System.Threading
 {
     public sealed partial class Thread
     {
-        [ThreadStatic]
-        private static int t_reentrantWaitSuppressionCount;
-
         [ThreadStatic]
         private static ApartmentType t_apartmentType;
 
@@ -404,24 +401,8 @@ internal static Thread EnsureThreadPoolThreadInitialized()
 
         public void Interrupt() { throw new PlatformNotSupportedException(); }
 
-        //
-        // Suppresses reentrant waits on the current thread, until a matching call to RestoreReentrantWaits.
-        // This should be used by code that's expected to be called inside the STA message pump, so that it won't
-        // reenter itself.  In an ASTA, this should only be the CCW implementations of IUnknown and IInspectable.
-        //
-        internal static void SuppressReentrantWaits()
-        {
-            t_reentrantWaitSuppressionCount++;
-        }
-
-        internal static void RestoreReentrantWaits()
-        {
-            Debug.Assert(t_reentrantWaitSuppressionCount > 0);
-            t_reentrantWaitSuppressionCount--;
-        }
-
         internal static bool ReentrantWaitsEnabled =>
-            GetCurrentApartmentType() == ApartmentType.STA && t_reentrantWaitSuppressionCount == 0;
+            GetCurrentApartmentType() == ApartmentType.STA;
 
         internal static ApartmentType GetCurrentApartmentType()
         {

src/coreclr/vm/assembly.hpp

@@ -372,11 +372,6 @@ class Assembly
     OBJECTHANDLE GetLoaderAllocatorObjectHandle() { WRAPPER_NO_CONTRACT; return GetLoaderAllocator()->GetLoaderAllocatorObjectHandle(); }
 #endif // FEATURE_COLLECTIBLE_TYPES
 
-#ifdef FEATURE_READYTORUN
-    BOOL IsInstrumented();
-    BOOL IsInstrumentedHelper();
-#endif // FEATURE_READYTORUN
-
 #ifdef FEATURE_COMINTEROP
     static ITypeLib * const InvalidTypeLib;
 

src/coreclr/vm/comsynchronizable.cpp

@@ -40,15 +40,7 @@
 static inline BOOL ThreadNotStarted(Thread *t)
 {
     WRAPPER_NO_CONTRACT;
-    return (t && t->IsUnstarted() && !t->HasValidThreadHandle());
-}
-
-static inline BOOL ThreadIsRunning(Thread *t)
-{
-    WRAPPER_NO_CONTRACT;
-    return (t &&
-            (t->m_State & (Thread::TS_ReportDead|Thread::TS_Dead)) == 0 &&
-            (t->HasValidThreadHandle()));
+    return (t && t->IsUnstarted());
 }
 
 static inline BOOL ThreadIsDead(Thread *t)
@@ -255,10 +247,10 @@ void ThreadNative::Start(Thread* pNewThread, int threadStackSize, int priority,
 
 #ifdef FEATURE_COMINTEROP_APARTMENT_SUPPORT
     // Attempt to eagerly set the apartment state during thread startup.
-    Thread::ApartmentState as = pNewThread->GetExplicitApartment();
+    Thread::ApartmentState as = pNewThread->GetApartmentOfUnstartedThread();
     if (as == Thread::AS_Unknown)
     {
-        pNewThread->SetApartment(Thread::AS_InMTA);
+        pNewThread->SetApartmentOfUnstartedThread(Thread::AS_InMTA);
     }
 #endif
 
@@ -553,13 +545,23 @@ extern "C" INT32 QCALLTYPE ThreadNative_SetApartmentState(QCall::ObjectHandleOnS
     // 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) || (GetThread() != thread)))
+    if (ThreadNotStarted(thread))
     {
-        COMPlusThrow(kThreadStateException);
+        // Compat: Disallow resetting the initial apartment state
+        if (thread->GetApartmentOfUnstartedThread() == Thread::AS_Unknown)
+            thread->SetApartmentOfUnstartedThread((Thread::ApartmentState)iState);
+
+        retVal = thread->GetApartmentOfUnstartedThread();
     }
+    else
+    {
+        if (GetThread() != thread)
+        {
+            COMPlusThrow(kThreadStateException);
+        }
 
-    retVal = thread->SetApartment((Thread::ApartmentState)iState);
+        retVal = thread->SetApartment((Thread::ApartmentState)iState);
+    }
 
     END_QCALL;
     return retVal;
@@ -713,19 +715,31 @@ void ThreadBaseObject::InitExisting()
         m_Priority = ThreadNative::PRIORITY_NORMAL;
         break;
     }
-
 }
 
 FCIMPL1(void, ThreadNative::Finalize, ThreadBaseObject* pThisUNSAFE)
 {
     FCALL_CONTRACT;
 
-    // This function is intentionally blank.
-    // See comment in code:MethodTable::CallFinalizer.
+    THREADBASEREF   refThis = (THREADBASEREF)pThisUNSAFE;
+    Thread*         thread  = refThis->GetInternal();
+
+    // Prevent multiple calls to Finalize
+    // Objects can be resurrected after being finalized.  However, there is no
+    // race condition here.  We always check whether an exposed thread object is
+    // still attached to the internal Thread object, before proceeding.
+    if (thread)
+    {
+        refThis->ResetStartHelper();
 
-    _ASSERTE (!"Should not be called");
+        if (GetThreadNULLOk() != thread)
+        {
+            refThis->ClearInternal();
+        }
 
-    FCUnique(0x21);
+        thread->SetThreadState(Thread::TS_Finalized);
+        Thread::SetCleanupNeededForFinalizedThread();
+    }
 }
 FCIMPLEND
 

src/coreclr/vm/comutilnative.cpp

@@ -1401,7 +1401,7 @@ void GCInterface::RemoveMemoryPressure(UINT64 bytesAllocated)
     CONTRACTL
     {
         NOTHROW;
-        GC_TRIGGERS;
+        GC_NOTRIGGER;
         MODE_ANY;
     }
     CONTRACTL_END;
@@ -1437,7 +1437,7 @@ NOINLINE void GCInterface::SendEtwRemoveMemoryPressureEvent(UINT64 bytesAllocate
     CONTRACTL
     {
         NOTHROW;
-        GC_TRIGGERS;
+        GC_NOTRIGGER;
         MODE_ANY;
     }
     CONTRACTL_END;

src/coreclr/vm/finalizerthread.cpp

@@ -52,42 +52,6 @@ BOOL FinalizerThread::HaveExtraWorkForFinalizer()
     return GetFinalizerThread()->HaveExtraWorkForFinalizer();
 }
 
-static void CallFinalizerOnThreadObject(OBJECTREF obj)
-{
-    STATIC_CONTRACT_MODE_COOPERATIVE;
-
-    THREADBASEREF   refThis = (THREADBASEREF)obj;
-    Thread*         thread  = refThis->GetInternal();
-
-    // Prevent multiple calls to Finalize
-    // Objects can be resurrected after being finalized.  However, there is no
-    // race condition here.  We always check whether an exposed thread object is
-    // still attached to the internal Thread object, before proceeding.
-    if (thread)
-    {
-        refThis->ResetStartHelper();
-
-        // During process shutdown, we finalize even reachable objects.  But if we break
-        // the link between the System.Thread and the internal Thread object, the runtime
-        // may not work correctly.  In particular, we won't be able to transition between
-        // contexts and domains to finalize other objects.  Since the runtime doesn't
-        // require that Threads finalize during shutdown, we need to disable this.  If
-        // we wait until phase 2 of shutdown finalization (when the EE is suspended and
-        // will never resume) then we can simply skip the side effects of Thread
-        // finalization.
-        if ((g_fEEShutDown & ShutDown_Finalize2) == 0)
-        {
-            if (GetThreadNULLOk() != thread)
-            {
-                refThis->ClearInternal();
-            }
-
-            thread->SetThreadState(Thread::TS_Finalized);
-            Thread::SetCleanupNeededForFinalizedThread();
-        }
-    }
-}
-
 OBJECTREF FinalizerThread::GetNextFinalizableObject()
 {
     STATIC_CONTRACT_THROWS;
@@ -138,20 +102,6 @@ OBJECTREF FinalizerThread::GetNextFinalizableObject()
         }
         while (pMTCur != NULL);
     }
-  
-    if (pMT == g_pThreadClass)
-    {
-        // Finalizing Thread object requires ThreadStoreLock.  It is expensive if
-        // we keep taking ThreadStoreLock.  This is very bad if we have high retiring
-        // rate of Thread objects.
-        // To avoid taking ThreadStoreLock multiple times, we mark Thread with TS_Finalized
-        // and clean up a batch of them when we take ThreadStoreLock next time.
-
-        // To avoid possible hierarchy requirement between critical finalizers, we call cleanup
-        // code directly.
-        CallFinalizerOnThreadObject(obj);
-        goto Again;
-    }
 
     return obj;
 }
@@ -426,19 +376,8 @@ DWORD WINAPI FinalizerThread::FinalizerThreadStart(void *args)
     ASSERT(args == 0);
     ASSERT(hEventFinalizer->IsValid());
 
-    // TODO: The following line should be removed after contract violation is fixed.
-    // See bug 27409
-    SCAN_IGNORE_THROW;
-    SCAN_IGNORE_TRIGGER;
-
     LOG((LF_GC, LL_INFO10, "Finalizer thread starting...\n"));
 
-#if defined(FEATURE_COMINTEROP_APARTMENT_SUPPORT) && !defined(FEATURE_COMINTEROP)
-    // Make sure the finalizer thread is set to MTA to avoid hitting
-    // DevDiv Bugs 180773 - [Stress Failure] AV at CoreCLR!SafeQueryInterfaceHelper
-    GetFinalizerThread()->SetApartment(Thread::AS_InMTA);
-#endif // FEATURE_COMINTEROP_APARTMENT_SUPPORT && !FEATURE_COMINTEROP
-
     s_FinalizerThreadOK = GetFinalizerThread()->HasStarted();
 
     _ASSERTE(s_FinalizerThreadOK);

src/coreclr/vm/methodtable.h

@@ -2691,15 +2691,6 @@ class MethodTable
     void DebugDumpGCDesc(LPCUTF8 pszClassName, BOOL debug);
 #endif //_DEBUG
 
-    inline BOOL IsAgileAndFinalizable()
-    {
-        LIMITED_METHOD_CONTRACT;
-        // Right now, System.Thread is the only cases of this.
-        // Things should stay this way - please don't change without talking to EE team.
-        return this == g_pThreadClass;
-    }
-
-
     //-------------------------------------------------------------------
     // ENUMS, DELEGATES, VALUE TYPES, ARRAYS
     //