[cDAC] Fix bug in GetThreadData

docs/design/datacontracts/Thread.md

@@ -23,7 +23,7 @@ enum ThreadState
     Hijacked            = 0x00000080,    // Return address has been hijacked
     Background          = 0x00000200,    // Thread is a background thread
     Unstarted           = 0x00000400,    // Thread has never been started
-    Dead                = 0x00000800,    // Thread is dead
+    Stopped             = 0x00010000,    // Thread has started to shut down
     ThreadPoolWorker    = 0x01000000,    // is this a threadpool worker thread?
 }
 
@@ -179,7 +179,7 @@ ThreadData GetThreadData(TargetPointer address)
     return new ThreadData(
         Id: target.Read<uint>(address + /* Thread::Id offset */),
         OSId: target.ReadNUInt(address + /* Thread::OSId offset */),
-        State: target.Read<uint>(address + /* Thread::State offset */),
+        State: target.Read<uint>(address + /* Thread::State offset */) /* -> convert to contract enum */,
         PreemptiveGCDisabled: (target.Read<uint>(address + /* Thread::PreemptiveGCDisabled offset */) & 0x1) != 0,
         AllocContextPointer: allocContextPointer,
         AllocContextLimit: allocContextLimit,

src/coreclr/debug/daccess/dacdbiimpl.cpp

@@ -4905,8 +4905,8 @@ HRESULT STDMETHODCALLTYPE DacDbiInterfaceImpl::EnumerateThreads(FP_THREAD_ENUMER
 
             // Don't want to publish threads via enumeration before they're ready to be inspected.
             // Use the same window that we used in whidbey.
-            Thread::ThreadState threadState = pThread->GetSnapshotState();
-            if (!((IsThreadMarkedDeadWorker(pThread)) || (threadState & Thread::TS_Unstarted)))
+            Thread::ThreadState threadState = pThread->GetState();
+            if (!((threadState & Thread::TS_Stopped) || (threadState & Thread::TS_Unstarted)))
             {
                 VMPTR_Thread vmThread = VMPTR_Thread::NullPtr();
                 vmThread.SetHostPtr(pThread);
@@ -4929,35 +4929,12 @@ HRESULT STDMETHODCALLTYPE DacDbiInterfaceImpl::IsThreadMarkedDead(VMPTR_Thread v
     EX_TRY
     {
         Thread * pThread = vmThread.GetDacPtr();
-        *pResult = IsThreadMarkedDeadWorker(pThread);
+        *pResult = (pThread->GetState() & Thread::TS_Stopped) != 0;
     }
     EX_CATCH_HRESULT(hr);
     return hr;
 }
 
-// Private worker for IsThreadMarkedDead
-//
-// Arguments:
-//    pThread - valid thread to check if dead
-//
-// Returns:
-//    true iff thread is marked as dead.
-//
-// Notes:
-//    This is an internal method that skips public validation.
-//    See code:IDacDbiInterface::#IsThreadMarkedDead for purpose.
-bool DacDbiInterfaceImpl::IsThreadMarkedDeadWorker(Thread * pThread)
-{
-    _ASSERTE(pThread != NULL);
-
-    Thread::ThreadState threadState = pThread->GetSnapshotState();
-
-    bool fIsDead = (threadState & Thread::TS_Dead) != 0;
-
-    return fIsDead;
-}
-
-
 // Return the handle of the specified thread.
 HRESULT STDMETHODCALLTYPE DacDbiInterfaceImpl::GetThreadHandle(VMPTR_Thread vmThread, OUT HANDLE * pRetVal)
 {
@@ -4984,9 +4961,9 @@ HRESULT STDMETHODCALLTYPE DacDbiInterfaceImpl::GetThreadObject(VMPTR_Thread vmTh
     {
 
         Thread * pThread = vmThread.GetDacPtr();
-        Thread::ThreadState threadState = pThread->GetSnapshotState();
+        Thread::ThreadState threadState = pThread->GetState();
 
-        if ( (threadState & Thread::TS_Dead) ||
+        if ( (threadState & Thread::TS_Stopped) ||
              (threadState & Thread::TS_Unstarted) ||
              (threadState & Thread::TS_Detached) ||
              g_fProcessDetach )
@@ -5985,7 +5962,7 @@ HRESULT STDMETHODCALLTYPE DacDbiInterfaceImpl::GetPartialUserState(VMPTR_Thread
     {
 
         Thread * pThread = vmThread.GetDacPtr();
-        Thread::ThreadState ts = pThread->GetSnapshotState();
+        Thread::ThreadState ts = pThread->GetState();
 
         UINT result = 0;
         if (ts & Thread::TS_Background)
@@ -5999,7 +5976,7 @@ HRESULT STDMETHODCALLTYPE DacDbiInterfaceImpl::GetPartialUserState(VMPTR_Thread
         }
 
         // Don't report a StopRequested if the thread has actually stopped.
-        if (ts & Thread::TS_Dead)
+        if (ts & Thread::TS_Stopped)
         {
             result |= USER_STOPPED;
         }

src/coreclr/debug/daccess/dacdbiimpl.h

@@ -933,8 +933,6 @@ class DacDbiInterfaceImpl :
     HRESULT STDMETHODCALLTYPE GetAppDomainIdFromVmObjectHandle(VMPTR_OBJECTHANDLE vmHandle, OUT ULONG * pRetVal);
 
 private:
-    bool IsThreadMarkedDeadWorker(Thread * pThread);
-
     // Check whether the specified thread is at a GC-safe place, i.e. in an interruptible region.
     BOOL IsThreadAtGCSafePlace(VMPTR_Thread vmThread);
 

src/coreclr/debug/daccess/dacdbiimplstackwalk.cpp

@@ -1390,7 +1390,7 @@ HRESULT STDMETHODCALLTYPE DacDbiInterfaceImpl::IsThreadSuspendedOrHijacked(VMPTR
     {
 
         Thread * pThread = vmThread.GetDacPtr();
-        Thread::ThreadState ts = pThread->GetSnapshotState();
+        Thread::ThreadState ts = pThread->GetState();
         if ((ts & Thread::TS_SyncSuspended) != 0)
         {
             *pResult = TRUE;

src/coreclr/debug/daccess/request.cpp

@@ -865,7 +865,6 @@ HRESULT ClrDataAccess::GetThreadData(CLRDATA_ADDRESS threadAddr, struct DacpThre
     ZeroMemory (threadData, sizeof(DacpThreadData));
     threadData->corThreadId = thread->m_ThreadId;
     threadData->osThreadId = (DWORD)thread->m_OSThreadId;
-    threadData->state = thread->m_State;
     threadData->preemptiveGCDisabled = thread->m_fPreemptiveGCDisabled;
 
     gc_alloc_context* allocContext = thread->GetAllocContext();

src/coreclr/debug/ee/frameinfo.cpp

@@ -1842,7 +1842,7 @@ bool ShouldSendUMLeafChain(Thread * pThread)
     // - at a managed-only stop, preemptive threads are still live. Thus a thread
     // may not have this state set, run a little, try to enter the GC, and then get
     // this state set. Thus we'll lose the UM chain right out from under our noses.
-    Thread::ThreadState ts = pThread->GetSnapshotState();
+    Thread::ThreadState ts = pThread->GetState();
     if ((ts & Thread::TS_SyncSuspended) != 0)
     {
         // If we've been stopped inside the runtime (eg, at a gc-toggle) but

src/coreclr/vm/comsynchronizable.cpp

@@ -406,9 +406,9 @@ extern "C" INT32 QCALLTYPE ThreadNative_GetThreadState(QCall::ThreadHandle threa
     INT32 res = 0;
 
     // grab a snapshot
-    Thread::ThreadState state = thread->GetSnapshotState();
+    Thread::ThreadState state = thread->GetState();
 
-    if (state & Thread::TS_Dead)
+    if (state & Thread::TS_Stopped)
         res |= ThreadNative::ThreadStopped;
 
     if (state & Thread::TS_Background)

src/coreclr/vm/eedbginterfaceimpl.cpp

@@ -1432,7 +1432,7 @@ CorDebugUserState EEDbgInterfaceImpl::GetPartialUserState(Thread *pThread)
     }
     CONTRACTL_END;
 
-    Thread::ThreadState ts = pThread->GetSnapshotState();
+    Thread::ThreadState ts = pThread->GetState();
     unsigned ret = 0;
 
     if (ts & Thread::TS_Background)
@@ -1446,7 +1446,7 @@ CorDebugUserState EEDbgInterfaceImpl::GetPartialUserState(Thread *pThread)
     }
 
     // Don't report a StopRequested if the thread has actually stopped.
-    if (ts & Thread::TS_Dead)
+    if (ts & Thread::TS_Stopped)
     {
         ret |= (unsigned)USER_STOPPED;
     }

src/coreclr/vm/profilingenumerators.cpp

@@ -562,11 +562,11 @@ HRESULT ProfilerThreadEnum::Init()
     // Because the thread enumeration status need to change before the ThreadCreated/ThreadDestroyed
     // callback, we need to:
     // 1. Include Thread::TS_FullyInitialized threads for ThreadCreated
-    // 2. Exclude Thread::TS_Dead | Thread::TS_ReportDead for ThreadDestroyed
+    // 2. Exclude Thread::TS_Dead | Thread::TS_Stopped for ThreadDestroyed
     //
     while ((pThread = ThreadStore::GetAllThreadList(
         pThread,
-        Thread::TS_Dead | Thread::TS_ReportDead | Thread::TS_FullyInitialized,
+        Thread::TS_Dead | Thread::TS_Stopped | Thread::TS_FullyInitialized,
         Thread::TS_FullyInitialized
         )))
     {

src/coreclr/vm/threads.cpp

@@ -833,7 +833,7 @@ void DestroyThread(Thread *th)
         th->UnmarkThreadForAbort();
     }
 
-    th->SetThreadState(Thread::TS_ReportDead);
+    th->SetThreadState(Thread::TS_Stopped);
     th->OnThreadTerminate(FALSE);
 }
 
@@ -911,7 +911,7 @@ HRESULT Thread::DetachThread(BOOL inTerminationCallback)
     // We need to make sure that TLS are touched last here.
     SetThread(NULL);
 
-    SetThreadState((Thread::ThreadState)(Thread::TS_Detached | Thread::TS_ReportDead));
+    SetThreadState((Thread::ThreadState)(Thread::TS_Detached | Thread::TS_Stopped));
     // Do not touch Thread object any more.  It may be destroyed.
 
     // These detached threads will be cleaned up by finalizer thread.
@@ -2575,14 +2575,13 @@ void Thread::OnThreadTerminate(BOOL holdingLock)
     }
     CONTRACTL_END;
 
-    // #ReportDeadOnThreadTerminate
-    // Caller should have put the TS_ReportDead bit on by now.
+    // #StoppedOnThreadTerminate
+    // Caller should have put the TS_Stopped bit on by now.
     // We don't want any windows after the exit event but before the thread is marked dead.
     // If a debugger attached during such a window (or even took a dump at the exit event),
     // then it may not realize the thread is dead.
     // So ensure we mark the thread as dead before we send the tool notifications.
-    // The TS_ReportDead bit will cause the debugger to view this as TS_Dead.
-    _ASSERTE(HasThreadState(TS_ReportDead));
+    _ASSERTE(HasThreadState(TS_Stopped));
 
     // Should not use OSThreadId:
     // OSThreadId may change for the current thread is the thread is blocked and rescheduled
@@ -4291,33 +4290,6 @@ Thread *ThreadStore::GetThreadList(Thread *cursor)
     return GetAllThreadList(cursor, (Thread::TS_Unstarted | Thread::TS_Dead), 0);
 }
 
-//---------------------------------------------------------------------------------------
-//
-// Grab a consistent snapshot of the thread's state, for reporting purposes only.
-//
-// Return Value:
-//    the current state of the thread
-//
-
-Thread::ThreadState Thread::GetSnapshotState()
-{
-    CONTRACTL {
-        NOTHROW;
-        GC_NOTRIGGER;
-        SUPPORTS_DAC;
-    }
-    CONTRACTL_END;
-
-    ThreadState res = m_State;
-
-    if (res & TS_ReportDead)
-    {
-        res = (ThreadState) (res | TS_Dead);
-    }
-
-    return res;
-}
-
 #ifndef DACCESS_COMPILE
 
 BOOL CLREventWaitWithTry(CLREventBase *pEvent, DWORD timeout, BOOL fAlertable, DWORD *pStatus)
@@ -4368,7 +4340,7 @@ void ThreadStore::WaitForOtherThreads()
     {
         TSLockHolder.Release();
 
-        pCurThread->SetThreadState(Thread::TS_ReportDead);
+        pCurThread->SetThreadState(Thread::TS_Stopped);
 
         DWORD ret = WAIT_OBJECT_0;
         while (CLREventWaitWithTry(&m_TerminationEvent, INFINITE, TRUE, &ret))

src/coreclr/vm/threads.h

@@ -523,7 +523,7 @@ class Thread
         // unused                 = 0x00000100,
         TS_Background             = 0x00000200,    // Thread is a background thread. [cDAC] [Thread]: Contract depends on this value.
         TS_Unstarted              = 0x00000400,    // Thread has never been started. [cDAC] [Thread]: Contract depends on this value.
-        TS_Dead                   = 0x00000800,    // Thread is dead. [cDAC] [Thread]: Contract depends on this value.
+        TS_Dead                   = 0x00000800,    // .NET runtime has finished shutting down this thread, and it is about to be terminated by the OS.
 
         TS_WeOwn                  = 0x00001000,    // Exposed object initiated this thread
 #ifdef FEATURE_COMINTEROP_APARTMENT_SUPPORT
@@ -534,7 +534,7 @@ class Thread
 #endif // FEATURE_COMINTEROP_APARTMENT_SUPPORT
 
         // Some bits that only have meaning for reporting the state to clients.
-        TS_ReportDead             = 0x00010000,    // in WaitForOtherThreads()
+        TS_Stopped                = 0x00010000,    // Thread has started to shut down and should not run managed code. Equivalent to ThreadState.Stopped. [cDAC] [Thread]: Contract depends on this value.
         TS_FullyInitialized       = 0x00020000,    // Thread is fully initialized and we are ready to broadcast its existence to external clients
 
         // unused                 = 0x00040000,
@@ -835,8 +835,11 @@ class Thread
         return (m_State & TS_WeOwn);
     }
 
-    // For reporting purposes, grab a consistent snapshot of the thread's state
-    ThreadState GetSnapshotState();
+    ThreadState GetState()
+    {
+        LIMITED_METHOD_CONTRACT;
+        return m_State;
+    }
 
     // For delayed destruction of threads
     DWORD           IsDetached()

src/coreclr/vm/threadsuspend.cpp

@@ -300,7 +300,7 @@ Thread::SuspendThreadResult Thread::SuspendThread(BOOL fOneTryOnly, DWORD *pdwSu
                         "Thread::SuspendThread[%p]:  EIP=%p. nCnt=%d. result=%d.\n"
                         "\t\t\t\t\t\t\t\t\t     forbidSuspend=%d. coop=%d. state=%x.\n",
                         this, GetIP(&ctx), nCnt, dwSuspendCount,
-                        (LONG)this->m_dwForbidSuspendThread, (ULONG)this->m_fPreemptiveGCDisabled, this->GetSnapshotState());
+                        (LONG)this->m_dwForbidSuspendThread, (ULONG)this->m_fPreemptiveGCDisabled, this->GetState());
 
                     // Enable a preemptive assert in diagnostic mode: before we
                     // resume the target thread to get its current state in the debugger

src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Abstractions/Contracts/IThread.cs

@@ -24,7 +24,7 @@ public enum ThreadState
     Hijacked            = 0x00000080,   // Return address has been hijacked
     Background          = 0x00000200,   // Thread is a background thread
     Unstarted           = 0x00000400,   // Thread has never been started
-    Dead                = 0x00000800,   // Thread is dead
+    Stopped             = 0x00010000,   // Thread has started to shut down
     ThreadPoolWorker    = 0x01000000,   // Thread is a thread pool worker thread
 }
 

src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Contracts/Contracts/Thread_1.cs

@@ -19,6 +19,16 @@ private enum TLSIndexType
         DirectOnThreadLocalData = 2, // IndexOffset for this form of TLS index is an offset into the ThreadLocalData structure itself. This is used for very high performance scenarios, and scenario where the runtime native code needs to hold a TLS pointer to a managed TLS slot. Each one of these is hand-opted into this model.
     };
 
+    [Flags]
+    private enum ThreadState_1
+    {
+        Hijacked = 0x80,
+        Background = 0x200,
+        Unstarted = 0x400,
+        Stopped = 0x10000,
+        ThreadPoolWorker = 0x1000000
+    }
+
     internal Thread_1(Target target)
     {
         _target = target;
@@ -50,6 +60,22 @@ ThreadStoreCounts IThread.GetThreadCounts()
             threadStore.DeadCount);
     }
 
+    private static Contracts.ThreadState GetThreadState(ThreadState_1 state)
+    {
+        Contracts.ThreadState result = Contracts.ThreadState.Unknown;
+        if (state.HasFlag(ThreadState_1.Hijacked))
+            result |= Contracts.ThreadState.Hijacked;
+        if (state.HasFlag(ThreadState_1.Background))
+            result |= Contracts.ThreadState.Background;
+        if (state.HasFlag(ThreadState_1.Unstarted))
+            result |= Contracts.ThreadState.Unstarted;
+        if (state.HasFlag(ThreadState_1.Stopped))
+            result |= Contracts.ThreadState.Stopped;
+        if (state.HasFlag(ThreadState_1.ThreadPoolWorker))
+            result |= Contracts.ThreadState.ThreadPoolWorker;
+        return result;
+    }
+
     ThreadData IThread.GetThreadData(TargetPointer threadPointer)
     {
         Data.Thread thread = _target.ProcessedData.GetOrAdd<Data.Thread>(threadPointer);
@@ -66,7 +92,7 @@ ThreadData IThread.GetThreadData(TargetPointer threadPointer)
             threadPointer,
             thread.Id,
             thread.OSId,
-            (ThreadState)thread.State,
+            GetThreadState((ThreadState_1)thread.State),
             (thread.PreemptiveGCDisabled & 0x1) != 0,
             thread.RuntimeThreadLocals?.AllocContext.GCAllocationContext.Pointer ?? TargetPointer.Null,
             thread.RuntimeThreadLocals?.AllocContext.GCAllocationContext.Limit ?? TargetPointer.Null,

src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Legacy/Dbi/DacDbiImpl.cs

@@ -338,8 +338,8 @@ public int EnumerateThreads(nint fpCallback, nint pUserData)
             while (currentThread != TargetPointer.Null)
             {
                 Contracts.ThreadData threadData = threadContract.GetThreadData(currentThread);
-                // Match native: skip dead and unstarted threads
-                if ((threadData.State & (Contracts.ThreadState.Dead | Contracts.ThreadState.Unstarted)) == 0)
+                // Match native: skip stopped and unstarted threads
+                if ((threadData.State & (Contracts.ThreadState.Stopped | Contracts.ThreadState.Unstarted)) == 0)
                 {
                     callback(currentThread.Value, pUserData);
 #if DEBUG
@@ -390,7 +390,7 @@ public int IsThreadMarkedDead(ulong vmThread, Interop.BOOL* pResult)
         try
         {
             Contracts.ThreadData threadData = _target.Contracts.Thread.GetThreadData(new TargetPointer(vmThread));
-            *pResult = (threadData.State & Contracts.ThreadState.Dead) != 0 ? Interop.BOOL.TRUE : Interop.BOOL.FALSE;
+            *pResult = (threadData.State & Contracts.ThreadState.Stopped) != 0 ? Interop.BOOL.TRUE : Interop.BOOL.FALSE;
         }
         catch (System.Exception ex)
         {

src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Legacy/SOSDacImpl.cs

@@ -4260,7 +4260,7 @@ int ISOSDacInterface.GetThreadData(ClrDataAddress thread, DacpThreadData* data)
             Contracts.ThreadData threadData = contract.GetThreadData(thread.ToTargetPointer(_target));
             data->corThreadId = (int)threadData.Id;
             data->osThreadId = (int)threadData.OSId.Value;
-            data->state = (int)threadData.State;
+            data->state = 0; // Set to 0, nobody uses this
             data->preemptiveGCDisabled = (uint)(threadData.PreemptiveGCDisabled ? 1 : 0);
             data->allocContextPtr = threadData.AllocContextPointer.ToClrDataAddress(_target);
             data->allocContextLimit = threadData.AllocContextLimit.ToClrDataAddress(_target);