Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add managed blocking info for lock and monitor waits #101192

Merged
merged 3 commits into from
Apr 24, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,8 @@ private static Waiter GetWaiterForCurrentThread()
private Waiter? _waitersHead;
private Waiter? _waitersTail;

internal Lock AssociatedLock => _lock;

private unsafe void AssertIsInList(Waiter waiter)
{
Debug.Assert(_waitersHead != null && _waitersTail != null);
Expand Down Expand Up @@ -106,6 +108,8 @@ public unsafe bool Wait(int millisecondsTimeout, object? associatedObjectForMoni
if (!_lock.IsHeldByCurrentThread)
throw new SynchronizationLockException();

using ThreadBlockingInfo.Scope threadBlockingScope = new(this, millisecondsTimeout);

Waiter waiter = GetWaiterForCurrentThread();
AddWaiter(waiter);

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,12 @@ public sealed partial class Lock
/// </summary>
public Lock() => _spinCount = SpinCountNotInitialized;

#pragma warning disable CA1822 // can be marked as static - varies between runtimes
internal ulong OwningOSThreadId => 0;
#pragma warning restore CA1822

internal int OwningManagedThreadId => (int)_owningThreadId;

[MethodImpl(MethodImplOptions.AggressiveInlining)]
internal bool TryEnterOneShot(int currentManagedThreadId)
{
Expand Down
7 changes: 7 additions & 0 deletions src/coreclr/vm/appdomain.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -1158,6 +1158,13 @@ void SystemDomain::Init()

// Finish loading CoreLib now.
m_pSystemAssembly->GetDomainAssembly()->EnsureActive();

// Set AwareLock's offset of the holding OS thread ID field into ThreadBlockingInfo's static field. That can be used
// when doing managed debugging to get the OS ID of the thread holding the lock. The offset is currently not zero, and
// zero is used in managed code to determine if the static variable has been initialized.
_ASSERTE(AwareLock::GetOffsetOfHoldingOSThreadId() != 0);
CoreLibBinder::GetField(FIELD__THREAD_BLOCKING_INFO__OFFSET_OF_LOCK_OWNER_OS_THREAD_ID)
->SetStaticValue32(AwareLock::GetOffsetOfHoldingOSThreadId());
}

#ifdef _DEBUG
Expand Down
4 changes: 4 additions & 0 deletions src/coreclr/vm/corelib.h
Original file line number Diff line number Diff line change
Expand Up @@ -583,6 +583,10 @@ END_ILLINK_FEATURE_SWITCH()
DEFINE_CLASS(MONITOR, Threading, Monitor)
DEFINE_METHOD(MONITOR, ENTER, Enter, SM_Obj_RetVoid)

DEFINE_CLASS(THREAD_BLOCKING_INFO, Threading, ThreadBlockingInfo)
DEFINE_FIELD(THREAD_BLOCKING_INFO, OFFSET_OF_LOCK_OWNER_OS_THREAD_ID, s_monitorObjectOffsetOfLockOwnerOSThreadId)
DEFINE_FIELD(THREAD_BLOCKING_INFO, FIRST, t_first)

DEFINE_CLASS(PARAMETER, Reflection, ParameterInfo)

DEFINE_CLASS(PARAMETER_MODIFIER, Reflection, ParameterModifier)
Expand Down
17 changes: 9 additions & 8 deletions src/coreclr/vm/syncblk.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -2851,22 +2851,23 @@ BOOL SyncBlock::Wait(INT32 timeOut)

_ASSERTE ((SyncBlock*)((DWORD_PTR)walk->m_Next->m_WaitSB & ~1)== this);

PendingSync syncState(walk);

OBJECTREF obj = m_Monitor.GetOwningObject();
syncState.m_Object = OBJECTREFToObject(obj);

m_Monitor.IncrementTransientPrecious();

// While we are in this frame the thread is considered blocked on the
// event of the monitor lock according to the debugger
// event of the monitor lock according to the debugger. DebugBlockingItemHolder
// can trigger a GC, so set it up before accessing the owning object.
DebugBlockingItem blockingMonitorInfo;
blockingMonitorInfo.dwTimeout = timeOut;
blockingMonitorInfo.pMonitor = &m_Monitor;
blockingMonitorInfo.pAppDomain = SystemDomain::GetCurrentDomain();
blockingMonitorInfo.type = DebugBlock_MonitorEvent;
DebugBlockingItemHolder holder(pCurThread, &blockingMonitorInfo);

PendingSync syncState(walk);

OBJECTREF obj = m_Monitor.GetOwningObject();
syncState.m_Object = OBJECTREFToObject(obj);

m_Monitor.IncrementTransientPrecious();

GCPROTECT_BEGIN(obj);
{
GCX_PREEMP();
Expand Down
6 changes: 6 additions & 0 deletions src/coreclr/vm/syncblk.h
Original file line number Diff line number Diff line change
Expand Up @@ -602,6 +602,12 @@ class AwareLock
LIMITED_METHOD_CONTRACT;
return m_HoldingThread;
}

static int GetOffsetOfHoldingOSThreadId()
{
LIMITED_METHOD_CONTRACT;
return (int)offsetof(AwareLock, m_HoldingOSThreadId);
}
};

#ifdef FEATURE_COMINTEROP
Expand Down
43 changes: 41 additions & 2 deletions src/coreclr/vm/threaddebugblockinginfo.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -72,9 +72,37 @@ VOID ThreadDebugBlockingInfo::VisitBlockingItems(DebugBlockingItemVisitor visito
// Holder constructor pushes a blocking item on the blocking info stack
#ifndef DACCESS_COMPILE
DebugBlockingItemHolder::DebugBlockingItemHolder(Thread *pThread, DebugBlockingItem *pItem) :
m_pThread(pThread)
m_pThread(pThread), m_ppFirstBlockingInfo(nullptr)
{
LIMITED_METHOD_CONTRACT;
CONTRACTL
{
THROWS;
GC_TRIGGERS;
MODE_COOPERATIVE;
}
CONTRACTL_END;

// Try to get the address of the thread-local slot for the managed ThreadBlockingInfo.t_first
EX_TRY
{
FieldDesc *pFD = CoreLibBinder::GetField(FIELD__THREAD_BLOCKING_INFO__FIRST);
m_ppFirstBlockingInfo = (ThreadBlockingInfo **)Thread::GetStaticFieldAddress(pFD);
}
EX_CATCH
{
}
EX_END_CATCH(RethrowTerminalExceptions);

if (m_ppFirstBlockingInfo != nullptr)
{
// Push info for the managed ThreadBlockingInfo
m_blockingInfo.objectPtr = pItem->pMonitor;
m_blockingInfo.objectKind = (ThreadBlockingInfo::ObjectKind)pItem->type;
m_blockingInfo.timeoutMs = (INT32)pItem->dwTimeout;
m_blockingInfo.next = *m_ppFirstBlockingInfo;
*m_ppFirstBlockingInfo = &m_blockingInfo;
}

pThread->DebugBlockingInfo.PushBlockingItem(pItem);
}
#endif //DACCESS_COMPILE
Expand All @@ -84,6 +112,17 @@ m_pThread(pThread)
DebugBlockingItemHolder::~DebugBlockingItemHolder()
{
LIMITED_METHOD_CONTRACT;

m_pThread->DebugBlockingInfo.PopBlockingItem();

if (m_ppFirstBlockingInfo != nullptr)
{
// Pop info for the managed ThreadBlockingInfo
_ASSERTE(
m_ppFirstBlockingInfo ==
(void *)m_pThread->GetStaticFieldAddrNoCreate(CoreLibBinder::GetField(FIELD__THREAD_BLOCKING_INFO__FIRST)));
_ASSERTE(*m_ppFirstBlockingInfo == &m_blockingInfo);
*m_ppFirstBlockingInfo = m_blockingInfo.next;
}
}
#endif //DACCESS_COMPILE
24 changes: 22 additions & 2 deletions src/coreclr/vm/threaddebugblockinginfo.h
Original file line number Diff line number Diff line change
Expand Up @@ -14,8 +14,8 @@
// Different ways thread can block that the debugger will expose
enum DebugBlockingItemType
{
DebugBlock_MonitorCriticalSection,
DebugBlock_MonitorEvent,
DebugBlock_MonitorCriticalSection, // maps to ThreadBlockingInfo.ObjectKind.MonitorLock below and in managed code
DebugBlock_MonitorEvent, // maps to ThreadBlockingInfo.ObjectKind.MonitorWait below and in managed code
};

typedef DPTR(struct DebugBlockingItem) PTR_DebugBlockingItem;
Expand Down Expand Up @@ -65,15 +65,35 @@ class ThreadDebugBlockingInfo
};

#ifndef DACCESS_COMPILE

// This is the equivalent of the managed ThreadBlockingInfo (see ThreadBlockingInfo.cs), which is used for tracking blocking
// info from the managed side, similarly to DebugBlockingItem
struct ThreadBlockingInfo
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need a 2nd definition of these types? Could we update DebugBlockingItem and DebugBlockingItemType to match instead?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The current definition appears to be used by debugger APIs to enumerate the blocking items. It seems there would need to be a second object on the stack for maintaining a separate linked list that contains info only about Monitor blocking, as the current APIs to get info from a blocking object would not work with Lock. I was thinking it might be easier to remove the current definitions in the future after debuggers switch to using the managed surface, and maybe deprecate some of the relevant debugger APIs. Thoughts?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The types that get exposed publicly should be CorDebugBlockingObject and CorDebugBlockingReason in the ICorDebug interface. The types exposed in the DAC IDacDbiInterface are not public and can be changed as needed. The code to convert from the internal to public types occurs here:

SIZE_T dacObjIndex = dacBlockingObjects.Size()-i-1;
.

However if we removed the domain field off the internal type we'd have to repopulate it from somewhere else and it looks like we've never done the work to convert ICorDebug to understand that CoreCLR just has a single AppDomain. Its probably not that hard, but I wouldn't feel right asking you to do that refactoring when the ground work isn't there yet. So I'd say lets not worry about this, it was just a potential for some minor deduplication.

In terms of deprecation we've rarely ever done it in the past but it is possible if supporting the old APIs is ongoing hassle and we've allowed some time to pass so that the new API is easily available. I'd think next release at the earliest unless there is some pressing need. We'd need to remove the usage from our tests, remove code from the runtime, and put out notifications of the breaking change. Thanks Kount!

{
enum class ObjectKind : INT32
{
MonitorLock, // maps to DebugBlockingItemType::DebugBlock_MonitorCriticalSection
MonitorWait // maps to DebugBlockingItemType::DebugBlock_MonitorEvent
};

void *objectPtr;
ObjectKind objectKind;
INT32 timeoutMs;
ThreadBlockingInfo *next;
};

class DebugBlockingItemHolder
{
private:
Thread *m_pThread;
ThreadBlockingInfo **m_ppFirstBlockingInfo;
ThreadBlockingInfo m_blockingInfo;

public:
DebugBlockingItemHolder(Thread *pThread, DebugBlockingItem *pItem);
~DebugBlockingItemHolder();
};

#endif //!DACCESS_COMPILE

#endif // __ThreadBlockingInfo__
Original file line number Diff line number Diff line change
Expand Up @@ -1266,6 +1266,7 @@
<Compile Include="$(MSBuildThisFileDirectory)System\Threading\Thread.cs" />
<Compile Include="$(MSBuildThisFileDirectory)System\Threading\ProcessorIdCache.cs" />
<Compile Include="$(MSBuildThisFileDirectory)System\Threading\ThreadAbortException.cs" />
<Compile Include="$(MSBuildThisFileDirectory)System\Threading\ThreadBlockingInfo.cs" />
<Compile Include="$(MSBuildThisFileDirectory)System\Threading\ThreadExceptionEventArgs.cs" />
<Compile Include="$(MSBuildThisFileDirectory)System\Threading\ThreadInt64PersistentCounter.cs" />
<Compile Include="$(MSBuildThisFileDirectory)System\Threading\ThreadInterruptedException.cs" />
Expand Down Expand Up @@ -2766,4 +2767,4 @@
<Compile Include="$(MSBuildThisFileDirectory)System\Numerics\IUnaryPlusOperators.cs" />
<Compile Include="$(MSBuildThisFileDirectory)System\Numerics\IUnsignedNumber.cs" />
</ItemGroup>
</Project>
</Project>
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,12 @@ public sealed partial class Lock
/// </summary>
public Lock() => _spinCount = s_maxSpinCount;

internal ulong OwningOSThreadId => _owningThreadId;

#pragma warning disable CA1822 // can be marked as static - varies between runtimes
internal int OwningManagedThreadId => 0;
#pragma warning restore CA1822

private static TryLockResult LazyInitializeOrEnter() => TryLockResult.Spin;
private static bool IsSingleProcessor => Environment.IsSingleProcessor;

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -477,6 +477,8 @@ private ThreadId TryEnterSlow(int timeoutMs, ThreadId currentThreadId)
waitStartTimeTicks = Stopwatch.GetTimestamp();
}

using ThreadBlockingInfo.Scope threadBlockingScope = new(this, timeoutMs);
noahfalk marked this conversation as resolved.
Show resolved Hide resolved

bool acquiredLock = false;
int waitStartTimeMs = timeoutMs < 0 ? 0 : Environment.TickCount;
int remainingTimeoutMs = timeoutMs;
Expand Down
Loading
Loading