From af5a23cea6cf99842187d028946d2be2f62b7001 Mon Sep 17 00:00:00 2001 From: Jan Kotas Date: Mon, 5 Aug 2024 19:39:13 -0700 Subject: [PATCH] Fix comments around static constructor memory model (#105911) - Fix comments around static constructor memory model - Delete unnecessary MemoryBarrier Contributes to #81151 --- .../Runtime/CompilerServices/ClassConstructorRunner.cs | 9 +-------- .../CompilerServices/StaticClassConstructionContext.cs | 2 ++ .../Runtime/CompilerServices/ClassConstructorRunner.cs | 7 ------- .../CompilerServices/StaticClassConstructionContext.cs | 4 +++- 4 files changed, 6 insertions(+), 16 deletions(-) diff --git a/src/coreclr/nativeaot/System.Private.CoreLib/src/System/Runtime/CompilerServices/ClassConstructorRunner.cs b/src/coreclr/nativeaot/System.Private.CoreLib/src/System/Runtime/CompilerServices/ClassConstructorRunner.cs index c3baa5a7dad04..6ea1c4c03d3b7 100644 --- a/src/coreclr/nativeaot/System.Private.CoreLib/src/System/Runtime/CompilerServices/ClassConstructorRunner.cs +++ b/src/coreclr/nativeaot/System.Private.CoreLib/src/System/Runtime/CompilerServices/ClassConstructorRunner.cs @@ -54,7 +54,7 @@ public static unsafe void EnsureClassConstructorRun(StaticClassConstructionConte IntPtr pfnCctor = pContext->cctorMethodAddress; NoisyLog("EnsureClassConstructorRun, context={0}, thread={1}", pContext, CurrentManagedThreadId); - // If we were called from MRT, this check is redundant but harmless. This is in case someone within classlib + // If we were called from JIT helper, this check is redundant but harmless. This is in case someone within classlib // (cough, Reflection) needs to call this explicitly. if (pfnCctor == 0) { @@ -87,13 +87,6 @@ public static unsafe void EnsureClassConstructorRun(StaticClassConstructionConte ((delegate*)pfnCctor)(); - // Insert a memory barrier here to order any writes executed as part of static class - // construction above with respect to the initialized flag update we're about to make - // below. This is important since the fast path for checking the cctor uses a normal read - // and doesn't come here so without the barrier it could observe initialized == 1 but - // still see uninitialized static fields on the class. - Interlocked.MemoryBarrier(); - NoisyLog("Set type inited, context={0}, thread={1}", pContext, currentManagedThreadId); pContext->cctorMethodAddress = 0; diff --git a/src/coreclr/nativeaot/System.Private.CoreLib/src/System/Runtime/CompilerServices/StaticClassConstructionContext.cs b/src/coreclr/nativeaot/System.Private.CoreLib/src/System/Runtime/CompilerServices/StaticClassConstructionContext.cs index c458a7d04a12a..36a44fb45bb76 100644 --- a/src/coreclr/nativeaot/System.Private.CoreLib/src/System/Runtime/CompilerServices/StaticClassConstructionContext.cs +++ b/src/coreclr/nativeaot/System.Private.CoreLib/src/System/Runtime/CompilerServices/StaticClassConstructionContext.cs @@ -16,6 +16,8 @@ namespace System.Runtime.CompilerServices public struct StaticClassConstructionContext { // Pointer to the code for the static class constructor method. Set to 0 once the cctor has run. + // Volatile to insert memory barriers to order any writes executed as part of static class + // constructor with respect to this flag. public volatile IntPtr cctorMethodAddress; } } diff --git a/src/coreclr/nativeaot/Test.CoreLib/src/System/Runtime/CompilerServices/ClassConstructorRunner.cs b/src/coreclr/nativeaot/Test.CoreLib/src/System/Runtime/CompilerServices/ClassConstructorRunner.cs index 8c8de3b2b462b..fd6e77dde9a43 100644 --- a/src/coreclr/nativeaot/Test.CoreLib/src/System/Runtime/CompilerServices/ClassConstructorRunner.cs +++ b/src/coreclr/nativeaot/Test.CoreLib/src/System/Runtime/CompilerServices/ClassConstructorRunner.cs @@ -66,13 +66,6 @@ private static unsafe void CheckStaticClassConstruction(ref StaticClassConstruct ((delegate*)oldInitializationState)(); - // Insert a memory barrier here to order any writes executed as part of static class - // construction above with respect to the initialized flag update we're about to make - // below. This is important since the fast path for checking the cctor uses a normal read - // and doesn't come here so without the barrier it could observe initialized == 1 but - // still see uninitialized static fields on the class. - Interlocked.MemoryBarrier(); - // Set the cctorMethodAddress to 0 to indicate to the runtime and other threads that this cctor has now // been run. context.cctorMethodAddress = (IntPtr)0; diff --git a/src/coreclr/nativeaot/Test.CoreLib/src/System/Runtime/CompilerServices/StaticClassConstructionContext.cs b/src/coreclr/nativeaot/Test.CoreLib/src/System/Runtime/CompilerServices/StaticClassConstructionContext.cs index 1f8ba767345f5..22590daa57f9f 100644 --- a/src/coreclr/nativeaot/Test.CoreLib/src/System/Runtime/CompilerServices/StaticClassConstructionContext.cs +++ b/src/coreclr/nativeaot/Test.CoreLib/src/System/Runtime/CompilerServices/StaticClassConstructionContext.cs @@ -12,7 +12,9 @@ namespace System.Runtime.CompilerServices [StructLayout(LayoutKind.Sequential)] public struct StaticClassConstructionContext { - // Pointer to the code for the static class constructor method. Set to 0 . + // Pointer to the code for the static class constructor method. Set to 0 once the cctor has run. + // Volatile to insert memory barriers to order any writes executed as part of static class + // constructor with respect to this flag. public volatile IntPtr cctorMethodAddress; } }