Skip to content

Commit

Permalink
Green thread objects (#2023)
Browse files Browse the repository at this point in the history
This adds managed `Thread` objects for green threads as well as unique managed thread ids. There is a test for that. 

In addition this adds logic to moves handling of Monitor locks to a per-green thread structure. There is a simple test for that scenario as well.

In addition it fixes a number of bugs

- In the reverse p/invoke logic where the current native `Thread*` pointer is cached, and when we hop threads isn't properly restored. (See change in jithelpers.cpp)
- In the forward P/Invoke case, this change disables standard PInvoke inlining in favor of the helper method approach. This works for everything but IL stubs (as the IL stub codegen sets specific values used for performing callstack walking.)
- This also fixes bugs where the stack limits were not properly updated on resume
- This fixes bugs where in the presence of optimization, when the thread was resumed on a different thread the Thread* was not refreshed from the underlying TLS data structure. This issue was found in several functions in C++ (which are now marked with optimizations disabled)

Fixes #2014
  • Loading branch information
davidwrighton committed Oct 18, 2022
1 parent 54d3bf2 commit 8fcf914
Show file tree
Hide file tree
Showing 41 changed files with 555 additions and 227 deletions.
Expand Up @@ -41,6 +41,7 @@ internal static unsafe void GreenThreadExecutorFunc(object? obj)
try
{
var action = (Action?)obj;
Thread.t_currentThread = null; // TODO Once we have Thread statics working we probably don't need to do this.
var suspendedThread = GreenThread_StartThread(&GreenThreadStartFunc, Unsafe.AsPointer(ref action));
Debug.Assert((GreenThreadStatics.t_TaskToWaitFor != null) == (suspendedThread != null));
if (suspendedThread != null)
Expand All @@ -54,6 +55,7 @@ internal static unsafe void GreenThreadExecutorFunc(object? obj)
{
GreenThreadStatics.t_TaskToWaitFor = null;
Thread.t_IsGreenThread = false;
Thread.t_currentThread = null; // TODO Once we have Thread statics working we probably don't need to do this.
}
}

Expand All @@ -70,6 +72,7 @@ public void Resume()
Thread.t_IsGreenThread = true;
try
{
Thread.t_currentThread = null; // TODO Once we have Thread statics working we probably don't need to do this.
_suspendedThread = GreenThread_ResumeThread(_suspendedThread);
Debug.Assert((GreenThreadStatics.t_TaskToWaitFor != null) == (_suspendedThread != null));
if (_suspendedThread != null)
Expand All @@ -82,6 +85,7 @@ public void Resume()
{
GreenThreadStatics.t_TaskToWaitFor = null;
Thread.t_IsGreenThread = false;
Thread.t_currentThread = null; // TODO Once we have Thread statics working we probably don't need to do this.
}
}
}
Expand All @@ -92,6 +96,10 @@ public void Resume()
ThreadPool.QueueUserWorkItem(GreenThreadExecutorFunc, action);
}

// No-inlining is used here and on ClearTaskToWaitFor, to avoid the problem
// of the t_TaskToWaitFor variable not being correctly handled. As we fix
// thread static for Green threads, the need for this tweak should disappear.
[MethodImpl(MethodImplOptions.NoInlining)]
static partial void YieldGreenThread(Task taskToWaitForCompletion, ref bool yielded)
{
if (Thread.t_IsGreenThread)
Expand All @@ -103,9 +111,15 @@ public void Resume()
}
finally
{
GreenThreadStatics.t_TaskToWaitFor = null;
ClearTaskToWaitFor();
}
}
}

[MethodImpl(MethodImplOptions.NoInlining)]
private static void ClearTaskToWaitFor()
{
GreenThreadStatics.t_TaskToWaitFor = null;
}
}
}
Expand Up @@ -58,6 +58,11 @@ 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;

#pragma warning disable CA1823, 169 // These fields are not used from managed.
// Used to indicate if this thread is a green thread
private bool _isGreenThread;
#pragma warning restore CA1823, 169

private Thread() { }

public extern int ManagedThreadId
Expand Down
2 changes: 1 addition & 1 deletion src/coreclr/classlibnative/bcltype/objectnative.cpp
Expand Up @@ -315,7 +315,7 @@ FCIMPL1(FC_BOOL_RET, ObjectNative::IsLockHeld, Object* pThisUNSAFE)
//
retVal = pThisUNSAFE->GetThreadOwningMonitorLock(&owningThreadId, &acquisitionCount);
if (retVal)
retVal = GetThread()->GetThreadId() == owningThreadId;
retVal = GetThread()->GetActiveManagedThreadId() == owningThreadId;

FC_RETURN_BOOL(retVal);
}
Expand Down
2 changes: 1 addition & 1 deletion src/coreclr/debug/daccess/daccess.cpp
Expand Up @@ -5594,7 +5594,7 @@ ClrDataAccess::FindClrThreadByTaskId(ULONG64 taskId)

while ((thread = ThreadStore::GetAllThreadList(thread, 0, 0)))
{
if (thread->GetThreadId() == (DWORD)taskId)
if (thread->GetPermanentManagedThreadId() == (DWORD)taskId)
{
return thread;
}
Expand Down
2 changes: 1 addition & 1 deletion src/coreclr/debug/daccess/dacdbiimpl.cpp
Expand Up @@ -6265,7 +6265,7 @@ MonitorLockInfo DacDbiInterfaceImpl::GetThreadOwningMonitorLock(VMPTR_Object vmO
Thread *pThread = ThreadStore::GetThreadList(NULL);
while (pThread != NULL)
{
if(pThread->GetThreadId() == threadId)
if(pThread->GetPermanentManagedThreadId() == threadId) // TODO This doesn't work for green threads
{
info.lockOwner.SetDacTargetPtr(PTR_HOST_TO_TADDR(pThread));
info.acquisitionCount = acquisitionCount;
Expand Down
2 changes: 2 additions & 0 deletions src/coreclr/debug/daccess/enummem.cpp
Expand Up @@ -807,11 +807,13 @@ HRESULT ClrDataAccess::EnumMemWalkStackHelper(CLRDataEnumMemoryFlags flags,
break;
}

#ifndef FEATURE_GREENTHREADS
if (!pThread->IsAddressInStack(currentSP))
{
_ASSERTE(!"Target stack has been corrupted, SP must in the stack range.");
break;
}
#endif // FEATURE_GREENTHREADS
}
}
else
Expand Down
4 changes: 2 additions & 2 deletions src/coreclr/debug/daccess/request.cpp
Expand Up @@ -644,7 +644,7 @@ ClrDataAccess::GetThreadFromThinlockID(UINT thinLockId, CLRDATA_ADDRESS *pThread

SOSDacEnter();

Thread *thread = g_pThinLockThreadIdDispenser->IdToThread(thinLockId);
ThreadBase *thread = g_pThinLockThreadIdDispenser->IdToThread(thinLockId);
*pThread = PTR_HOST_TO_TADDR(thread);

SOSDacLeave();
Expand Down Expand Up @@ -713,7 +713,7 @@ ClrDataAccess::GetThreadData(CLRDATA_ADDRESS threadAddr, struct DacpThreadData *

// initialize our local copy from the marshaled target Thread instance
ZeroMemory (threadData, sizeof(DacpThreadData));
threadData->corThreadId = thread->m_ThreadId;
threadData->corThreadId = thread->GetPermanentManagedThreadId();
threadData->osThreadId = (DWORD)thread->m_OSThreadId;
threadData->state = thread->m_State;
threadData->preemptiveGCDisabled = thread->m_fPreemptiveGCDisabled;
Expand Down
2 changes: 1 addition & 1 deletion src/coreclr/debug/daccess/task.cpp
Expand Up @@ -183,7 +183,7 @@ ClrDataTask::GetUniqueID(

EX_TRY
{
*id = m_thread->GetThreadId();
*id = m_thread->GetPermanentManagedThreadId();
status = S_OK;
}
EX_CATCH
Expand Down
2 changes: 1 addition & 1 deletion src/coreclr/debug/ee/debugger.cpp
Expand Up @@ -9015,7 +9015,7 @@ void Debugger::ThreadCreated(Thread* pRuntimeThread)

// Sanity check the thread.
_ASSERTE(pRuntimeThread != NULL);
_ASSERTE(pRuntimeThread->GetThreadId() != 0);
_ASSERTE(pRuntimeThread->GetPermanentManagedThreadId() != 0);


// Create a thread starter and enable its WillEnterManaged code
Expand Down
7 changes: 7 additions & 0 deletions src/coreclr/vm/amd64/AsmHelpers.asm
Expand Up @@ -913,6 +913,13 @@ NESTED_ENTRY ResumeSuspendedThreadHelper2, _TEXT
; as well as where to stash the stack_limit and stack_base data
call GetResumptionStackPointerAndSaveOSStackPointer

; Update stack limit and base to be in the green thread
mov r11, [rbp - 030h] ; Load the new stack limit
mov gs:[10h], r11
mov r11, [rbp - 028h] ; Load the old stack base
mov gs:[8h], r11

mov rsp, rax ; Set stack pointer to back into the green thread
jmp resume_point ; Now that the stack is in position,

Expand Down
4 changes: 2 additions & 2 deletions src/coreclr/vm/amd64/asmconstants.h
Expand Up @@ -108,12 +108,12 @@ ASMCONSTANTS_C_ASSERT(OFFSETOF__ComPlusCallInfo__m_pILStub

#endif // FEATURE_COMINTEROP

#define OFFSETOF__Thread__m_fPreemptiveGCDisabled 0x0C
#define OFFSETOF__Thread__m_fPreemptiveGCDisabled 0x14
ASMCONSTANTS_C_ASSERT(OFFSETOF__Thread__m_fPreemptiveGCDisabled
== offsetof(Thread, m_fPreemptiveGCDisabled));
#define Thread_m_fPreemptiveGCDisabled OFFSETOF__Thread__m_fPreemptiveGCDisabled

#define OFFSETOF__Thread__m_pFrame 0x10
#define OFFSETOF__Thread__m_pFrame 0x18
ASMCONSTANTS_C_ASSERT(OFFSETOF__Thread__m_pFrame
== offsetof(Thread, m_pFrame));
#define Thread_m_pFrame OFFSETOF__Thread__m_pFrame
Expand Down
1 change: 1 addition & 0 deletions src/coreclr/vm/common.h
Expand Up @@ -136,6 +136,7 @@ typedef DPTR(class CoreLibBinder) PTR_CoreLibBinder;
typedef VPTR(class Module) PTR_Module;
typedef DPTR(class NDirectMethodDesc) PTR_NDirectMethodDesc;
typedef DPTR(class Thread) PTR_Thread;
typedef DPTR(class ThreadBase) PTR_ThreadBase;
typedef DPTR(class Object) PTR_Object;
typedef DPTR(PTR_Object) PTR_PTR_Object;
typedef DPTR(class DelegateObject) PTR_DelegateObject;
Expand Down
44 changes: 42 additions & 2 deletions src/coreclr/vm/comsynchronizable.cpp
Expand Up @@ -336,6 +336,9 @@ FCIMPL1(INT32, ThreadNative::GetPriority, ThreadBaseObject* pThisUNSAFE)
if (pThisUNSAFE==NULL)
FCThrowRes(kNullReferenceException, W("NullReference_This"));

if (pThisUNSAFE->IsGreenThread())
FCThrowRes(kThreadStateException, W("ThreadState_GreenThread"));

// validate the handle
if (ThreadIsDead(pThisUNSAFE->GetInternal()))
FCThrowRes(kThreadStateException, W("ThreadState_Dead_Priority"));
Expand All @@ -359,6 +362,9 @@ FCIMPL2(void, ThreadNative::SetPriority, ThreadBaseObject* pThisUNSAFE, INT32 iP
COMPlusThrow(kNullReferenceException, W("NullReference_This"));
}

if (pThis->IsGreenThread())
COMPlusThrow(kThreadStateException, W("ThreadState_GreenThread"));

// translate the priority (validating as well)
priority = MapToNTPriority(iPriority); // can throw; needs a frame

Expand Down Expand Up @@ -397,6 +403,9 @@ FCIMPL1(void, ThreadNative::Interrupt, ThreadBaseObject* pThisUNSAFE)

Thread *thread = pThisUNSAFE->GetInternal();

if (pThisUNSAFE->IsGreenThread())
FCThrowResVoid(kThreadStateException, W("ThreadState_GreenThread"));

if (thread == 0)
FCThrowExVoid(kThreadStateException, IDS_EE_THREAD_CANNOT_GET, NULL, NULL, NULL);

Expand Down Expand Up @@ -425,6 +434,9 @@ FCIMPL1(FC_BOOL_RET, ThreadNative::IsAlive, ThreadBaseObject* pThisUNSAFE)
// Thread* to NULL in the GC's ScanForFinalization method.
HELPER_METHOD_FRAME_BEGIN_RET_1(thisRef);

if (thisRef->IsGreenThread())
COMPlusThrow(kThreadStateException, W("ThreadState_GreenThread"));

Thread *thread = thisRef->GetInternal();

if (thread == 0)
Expand Down Expand Up @@ -514,7 +526,7 @@ NOINLINE static Object* GetCurrentThreadHelper()
FCIMPL0(Object*, ThreadNative::GetCurrentThread)
{
FCALL_CONTRACT;
OBJECTHANDLE ExposedObject = GetThread()->m_ExposedObject;
OBJECTHANDLE ExposedObject = GetThread()->GetActiveThreadBase()->m_ExposedObject;
_ASSERTE(ExposedObject != 0); //Thread's constructor always initializes its GCHandle
Object* result = *((Object**) ExposedObject);
if (result != 0)
Expand Down Expand Up @@ -569,7 +581,7 @@ FCIMPL1(void, ThreadNative::Initialize, ThreadBaseObject* pThisUNSAFE)
}

pThis->SetInternal(unstarted);
pThis->SetManagedThreadId(unstarted->GetThreadId());
pThis->SetManagedThreadId(unstarted->GetPermanentManagedThreadId());
unstarted->SetExposedObject(pThis);

// Initialize the thread priority to normal.
Expand All @@ -588,6 +600,9 @@ FCIMPL2(void, ThreadNative::SetBackground, ThreadBaseObject* pThisUNSAFE, CLR_BO
if (pThisUNSAFE==NULL)
FCThrowResVoid(kNullReferenceException, W("NullReference_This"));

if (pThisUNSAFE->IsGreenThread())
FCThrowResVoid(kThreadStateException, W("ThreadState_GreenThread"));

// validate the thread
Thread *thread = pThisUNSAFE->GetInternal();

Expand All @@ -610,6 +625,9 @@ FCIMPL1(FC_BOOL_RET, ThreadNative::IsBackground, ThreadBaseObject* pThisUNSAFE)
if (pThisUNSAFE==NULL)
FCThrowRes(kNullReferenceException, W("NullReference_This"));

if (pThisUNSAFE->IsGreenThread())
FC_RETURN_BOOL(TRUE);

// validate the thread
Thread *thread = pThisUNSAFE->GetInternal();

Expand All @@ -635,6 +653,9 @@ FCIMPL1(INT32, ThreadNative::GetThreadState, ThreadBaseObject* pThisUNSAFE)
if (pThisUNSAFE==NULL)
FCThrowRes(kNullReferenceException, W("NullReference_This"));

if (pThisUNSAFE->IsGreenThread())
FCThrowRes(kThreadStateException, W("ThreadState_GreenThread"));

// validate the thread. Failure here implies that the thread was finalized
// and then resurrected.
Thread *thread = pThisUNSAFE->GetInternal();
Expand Down Expand Up @@ -691,6 +712,9 @@ FCIMPL2(INT32, ThreadNative::SetApartmentState, ThreadBaseObject* pThisUNSAFE, I

HELPER_METHOD_FRAME_BEGIN_RET_1(pThis);

if (pThis->IsGreenThread())
COMPlusThrow(kThreadStateException, W("ThreadState_GreenThread"));

Thread *thread = pThis->GetInternal();
if (!thread)
COMPlusThrow(kThreadStateException, IDS_EE_THREAD_CANNOT_GET);
Expand Down Expand Up @@ -748,6 +772,9 @@ FCIMPL1(INT32, ThreadNative::GetApartmentState, ThreadBaseObject* pThisUNSAFE)
COMPlusThrow(kNullReferenceException, W("NullReference_This"));
}

if (refThis->IsGreenThread())
COMPlusThrow(kThreadStateException, W("ThreadState_GreenThread"));

Thread* thread = refThis->GetInternal();

if (ThreadIsDead(thread))
Expand Down Expand Up @@ -800,6 +827,9 @@ BOOL ThreadNative::DoJoin(THREADBASEREF DyingThread, INT32 timeout)
}
CONTRACTL_END;

if (DyingThread->IsGreenThread())
COMPlusThrow(kThreadStateException, W("ThreadState_GreenThread"));

Thread * DyingInternal = DyingThread->GetInternal();

// Validate the handle. It's valid to Join a thread that's not running -- so
Expand Down Expand Up @@ -934,6 +964,10 @@ FCIMPL1(void, ThreadNative::DisableComObjectEagerCleanup, ThreadBaseObject* pThi

_ASSERTE(pThisUNSAFE != NULL);
VALIDATEOBJECT(pThisUNSAFE);

if (pThisUNSAFE->IsGreenThread())
FCThrowResVoid(kThreadStateException, W("ThreadState_GreenThread"));

Thread *pThread = pThisUNSAFE->GetInternal();

HELPER_METHOD_FRAME_BEGIN_0();
Expand Down Expand Up @@ -1020,6 +1054,9 @@ FCIMPL1(FC_BOOL_RET, ThreadNative::IsThreadpoolThread, ThreadBaseObject* thread)
if (thread==NULL)
FCThrowRes(kNullReferenceException, W("NullReference_This"));

if (thread->IsGreenThread())
FC_RETURN_BOOL(TRUE);

Thread *pThread = thread->GetInternal();

if (pThread == NULL)
Expand All @@ -1040,6 +1077,9 @@ FCIMPL1(void, ThreadNative::SetIsThreadpoolThread, ThreadBaseObject* thread)
if (thread == NULL)
FCThrowResVoid(kNullReferenceException, W("NullReference_This"));

if (thread->IsGreenThread())
FCThrowResVoid(kThreadStateException, W("ThreadState_GreenThread"));

Thread *pThread = thread->GetInternal();

if (pThread == NULL)
Expand Down
4 changes: 2 additions & 2 deletions src/coreclr/vm/eedbginterfaceimpl.cpp
Expand Up @@ -1408,7 +1408,7 @@ void EEDbgInterfaceImpl::SetDebugState(Thread *pThread,

_ASSERTE(state == THREAD_SUSPEND || state == THREAD_RUN);

LOG((LF_CORDB,LL_INFO10000,"EEDbg:Setting thread 0x%x (ID:0x%x) to 0x%x\n", pThread, pThread->GetThreadId(), state));
LOG((LF_CORDB,LL_INFO10000,"EEDbg:Setting thread 0x%x (ID:0x%x) to 0x%x\n", pThread, pThread->GetPermanentManagedThreadId(), state));

if (state == THREAD_SUSPEND)
{
Expand Down Expand Up @@ -1480,7 +1480,7 @@ CorDebugUserState EEDbgInterfaceImpl::GetPartialUserState(Thread *pThread)
}

LOG((LF_CORDB,LL_INFO1000, "EEDbgII::GUS: thread 0x%x (id:0x%x)"
" userThreadState is 0x%x\n", pThread, pThread->GetThreadId(), ret));
" userThreadState is 0x%x\n", pThread, pThread->GetPermanentManagedThreadId(), ret));

return (CorDebugUserState)ret;
}
Expand Down
2 changes: 1 addition & 1 deletion src/coreclr/vm/eventing/eventpipe/ep-rt-coreclr.h
Expand Up @@ -2023,7 +2023,7 @@ ep_rt_thread_create (
thread_params->thread_params.thread->SetBackground (TRUE);
thread_params->thread_params.thread->StartThread ();
if (id)
*reinterpret_cast<DWORD *>(id) = thread_params->thread_params.thread->GetThreadId ();
*reinterpret_cast<DWORD *>(id) = thread_params->thread_params.thread->GetPermanentManagedThreadId ();
result = true;
}
} else if (thread_type == EP_THREAD_TYPE_SERVER) {
Expand Down
2 changes: 1 addition & 1 deletion src/coreclr/vm/eventstore.hpp
Expand Up @@ -17,7 +17,7 @@ typedef DPTR(WaitEventLink) PTR_WaitEventLink;
struct WaitEventLink {
SyncBlock *m_WaitSB;
CLREvent *m_EventWait;
PTR_Thread m_Thread; // Owner of this WaitEventLink.
PTR_ThreadBase m_Thread; // Owner of this WaitEventLink.
PTR_WaitEventLink m_Next; // Chain to the next waited SyncBlock.
SLink m_LinkSB; // Chain to the next thread waiting on the same SyncBlock.
DWORD m_RefCount; // How many times Object::Wait is called on the same SyncBlock.
Expand Down
4 changes: 2 additions & 2 deletions src/coreclr/vm/eventtrace.cpp
Expand Up @@ -2593,7 +2593,7 @@ VOID ETW::ThreadLog::FireThreadCreated(Thread * pThread)
(ULONGLONG)pThread,
(ULONGLONG)pThread->GetDomain(),
GetEtwThreadFlags(pThread),
pThread->GetThreadId(),
pThread->GetPermanentManagedThreadId(),
pThread->GetOSThreadId(),
GetClrInstanceId());
}
Expand All @@ -2606,7 +2606,7 @@ VOID ETW::ThreadLog::FireThreadDC(Thread * pThread)
(ULONGLONG)pThread,
(ULONGLONG)pThread->GetDomain(),
GetEtwThreadFlags(pThread),
pThread->GetThreadId(),
pThread->GetPermanentManagedThreadId(),
pThread->GetOSThreadId(),
GetClrInstanceId());
}
Expand Down
2 changes: 1 addition & 1 deletion src/coreclr/vm/excep.cpp
Expand Up @@ -4856,7 +4856,7 @@ lDone: ;
#ifdef _DEBUG
char buffer[200];
sprintf_s(buffer, 200, "\nInternal error: Uncaught exception was thrown from IP = %p in UnhandledExceptionFilter_Worker on thread 0x%08x\n",
param.ExceptionEIP, ((GetThreadNULLOk() == NULL) ? NULL : GetThread()->GetThreadId()));
param.ExceptionEIP, ((GetThreadNULLOk() == NULL) ? NULL : GetThread()->GetPermanentManagedThreadId()));
PrintToStdErrA(buffer);
_ASSERTE(!"Unexpected exception in UnhandledExceptionFilter_Worker");
#endif
Expand Down

0 comments on commit 8fcf914

Please sign in to comment.