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.

Sign up for GitHub

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

Fix class construction cycle in Lock on NativeAOT #100374

Merged
merged 1 commit into from
Apr 23, 2024
Merged

Fix class construction cycle in Lock on NativeAOT #100374

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
75 changes: 43 additions & 32 deletions src/coreclr/nativeaot/System.Private.CoreLib/src/System/Threading/Lock.NativeAot.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -92,18 +92,6 @@ internal void Reenter(uint previousRecursionCount)
_recursionCount = previousRecursionCount;
}

private static bool IsFullyInitialized
{
get
{
// If NativeRuntimeEventSource is already being class-constructed by this thread earlier in the stack, Log can
// be null. This property is used to avoid going down the wait path in that case to avoid null checks in several
// other places.
Debug.Assert((StaticsInitializationStage)s_staticsInitializationStage == StaticsInitializationStage.Complete);
return NativeRuntimeEventSource.Log != null;
}
}

[MethodImpl(MethodImplOptions.AggressiveInlining)]
private TryLockResult LazyInitializeOrEnter()
{
Expand All @@ -113,10 +101,6 @@ private TryLockResult LazyInitializeOrEnter()
case StaticsInitializationStage.Complete:
if (_spinCount == SpinCountNotInitialized)
{
if (!IsFullyInitialized)
{
goto case StaticsInitializationStage.Started;
}
_spinCount = s_maxSpinCount;
}
return TryLockResult.Spin;
Expand All @@ -137,7 +121,7 @@ private TryLockResult LazyInitializeOrEnter()
}

stage = (StaticsInitializationStage)Volatile.Read(ref s_staticsInitializationStage);
if (stage == StaticsInitializationStage.Complete && IsFullyInitialized)
if (stage == StaticsInitializationStage.Complete)
{
goto case StaticsInitializationStage.Complete;
}
Expand All @@ -155,7 +139,9 @@ private TryLockResult LazyInitializeOrEnter()
}

default:
Debug.Assert(stage == StaticsInitializationStage.NotStarted);
Debug.Assert(
stage == StaticsInitializationStage.NotStarted ||
stage == StaticsInitializationStage.PartiallyComplete);
if (TryInitializeStatics())
{
goto case StaticsInitializationStage.Complete;
Expand All @@ -169,29 +155,49 @@ private static bool TryInitializeStatics()
{
// Since Lock is used to synchronize class construction, and some of the statics initialization may involve class
// construction, update the stage first to avoid infinite recursion
switch (
(StaticsInitializationStage)
Interlocked.CompareExchange(
ref s_staticsInitializationStage,
(int)StaticsInitializationStage.Started,
(int)StaticsInitializationStage.NotStarted))
var oldStage = (StaticsInitializationStage)s_staticsInitializationStage;
while (true)
{
case StaticsInitializationStage.Started:
return false;
case StaticsInitializationStage.Complete:
if (oldStage == StaticsInitializationStage.Complete)
{
return true;
}

var stageBeforeUpdate =
(StaticsInitializationStage)Interlocked.CompareExchange(
ref s_staticsInitializationStage,
(int)StaticsInitializationStage.Started,
(int)oldStage);
if (stageBeforeUpdate == StaticsInitializationStage.Started)
{
return false;
}
if (stageBeforeUpdate == oldStage)
{
Debug.Assert(
oldStage == StaticsInitializationStage.NotStarted ||
oldStage == StaticsInitializationStage.PartiallyComplete);
VSadov marked this conversation as resolved.
Show resolved Hide resolved
break;
}

oldStage = stageBeforeUpdate;
}

bool isFullyInitialized;
try
{
s_isSingleProcessor = Environment.IsSingleProcessor;
s_maxSpinCount = DetermineMaxSpinCount();
s_minSpinCount = DetermineMinSpinCount();
if (oldStage == StaticsInitializationStage.NotStarted)
{
// If the stage is PartiallyComplete, these will have already been initialized
Copy link
Member

Choose a reason for hiding this comment

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

if they have already been initialized, why is it initializing them again?

Copy link
Member Author

Choose a reason for hiding this comment

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

These are initialized when the stage is changed from NotStarted. The stage doesn't go back to NotStarted once changed, so they would only be initialized once.

s_isSingleProcessor = Environment.IsSingleProcessor;
s_maxSpinCount = DetermineMaxSpinCount();
Copy link
Member

Choose a reason for hiding this comment

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

What happens if DetermineMaxSpinCount() takes a lock?

Copy link
Member Author

Choose a reason for hiding this comment

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

It would attempt to take the lock and may have to spin-wait to acquire it. Not sure what you're actually asking though, can you clarify?

Copy link
Member

Choose a reason for hiding this comment

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

I was thinking of a possibility that DetermineMaxSpinCount somehow directly or indirectly takes a lock and end up getting here.

But we are not holding anything so we are not introducing a deadlock just by reentering, so the recursive locking like this should work. Just hard to think of all the cases.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes the idea is that there is only one attempt at initialization at any given time, so this path would not reenter even in the same thread.

s_minSpinCount = DetermineMinSpinCount();
}

// Also initialize some types that are used later to prevent potential class construction cycles. If
// NativeRuntimeEventSource is already being class-constructed by this thread earlier in the stack, Log can be
// null. Avoid going down the wait path in that case to avoid null checks in several other places.
// null. Avoid going down the wait path in that case to avoid null checks in several other places. If not fully
// initialized, the stage will also be set to PartiallyComplete to try again.
isFullyInitialized = NativeRuntimeEventSource.Log != null;
}
catch
Expand All @@ -200,7 +206,11 @@ private static bool TryInitializeStatics()
throw;
}

Volatile.Write(ref s_staticsInitializationStage, (int)StaticsInitializationStage.Complete);
Volatile.Write(
ref s_staticsInitializationStage,
isFullyInitialized
? (int)StaticsInitializationStage.Complete
: (int)StaticsInitializationStage.PartiallyComplete);
return isFullyInitialized;
}

Expand Down Expand Up @@ -242,6 +252,7 @@ private enum StaticsInitializationStage
{
NotStarted,
Started,
PartiallyComplete,
Complete
}
}
Expand Down