Skip to content
This repository has been archived by the owner on Nov 1, 2020. It is now read-only.

[CppCodeGen] Use ClassConstructorRunner to run cctor #6635

Merged
merged 2 commits into from
Dec 10, 2018

Conversation

kbaladurin
Copy link
Member

This patch makes static class initialization thread-safe: #6404

@kbaladurin
Copy link
Member Author

@jkotas @MichalStrehovsky PTAL

@@ -87,13 +87,23 @@ public static void Print(string format, params object[] args)
[System.Diagnostics.Conditional("DEBUG")]
public static void Assert(bool condition)
{
Assert(condition, string.Empty, string.Empty);
// Do not trigger string cctor if it's unnecessary otherwise we can call ClassConstructorRunner.EnsureClassConstructorRun
Copy link
Member

Choose a reason for hiding this comment

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

This comment does not quite make sense for CoreCLR or Mono that this file is shared with.

I would fix the problem you are seeing by either:

  • Passing "" here instead of string.Empty
  • Or pass in null instead of string.Empty. It may need a tweak in the implementation of Debug.Fail to compensate.
  • Or treating string.Empty as intrinsic in CppCodeGen - replace it with "" under the covers

Copy link
Member

Choose a reason for hiding this comment

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

This is not a problem for the RyuJIT backend because we run the string class constructor eagerly (the class is marked with [System.Runtime.CompilerServices.EagerStaticClassConstructionAttribute]). Those classes don't use the lazy class constructor runner because their class constructor runs at startup time.

We should port that logic to CppCodegen too (I would think we'll hit similar recursions elsewhere, not just for strings without it).

The related pull request is #889 from a couple years ago. The gist is basically:

  • We use Compilation.HasLazyStaticConstructor to check whether there's a cctor that would need checking at runtime. This abstracts away the static constructors for preinitialized types. If it returns false, there's no need to trigger a cctor check.
  • We keep track of the need to generate eager class constructor method body in the nodes that wrap the static bases. CppCodegen doesn't have those dependency nodes, so we would need a separate mechanism. Maybe just adding factory.EagerCctorIndirection(type.GetStaticConstructor()) as a dependency of the method body if factory.TypeSystemContext.HasEagerStaticConstructor returns true would be sufficient.
  • There is a mapping table referenced from the ready to run header (EagerCctorTable) that has all the eager class constructors in a sorted order. Startup code will trigger all of them to run (in RunEagerClassConstructors).

Copy link
Member Author

Choose a reason for hiding this comment

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

@MichalStrehovsky thank you! Seems like in my case Debug.Assert is called before RunEagerClassConstructors:

Thread 1 "Hello" received signal SIGSEGV, Segmentation fault.
0x0000000000e4adec in PalInterlockedCompareExchange (pDst=0x8, iValue=1, iComparand=0) at /media/kbaladurin/data/dotnet/forked/corert/src/Native/Runtime/unix/PalRedhawkInline.h:41
41	    return __sync_val_compare_and_swap(pDst, iComparand, iValue);
(gdb) bt
#0  0x0000000000e4adec in PalInterlockedCompareExchange (pDst=0x8, iValue=1, iComparand=0) at /media/kbaladurin/data/dotnet/forked/corert/src/Native/Runtime/unix/PalRedhawkInline.h:41
#1  RhpLockCmpXchg32 (location=0x8, value=1, comparand=0) at /media/kbaladurin/data/dotnet/forked/corert/src/Native/Runtime/portable.cpp:364
#2  0x0000000000995a21 in System_Private_CoreLib::System::Runtime::RuntimeImports::InterlockedCompareExchange (location1=0x8, value=1, comparand=0)
    at /media/kbaladurin/data/dotnet/forked/corert/src/System.Private.CoreLib/src/System/Threading/Lock.cs:397
#3  0x000000000098928c in System_Private_CoreLib::System::Threading::Interlocked::CompareExchange (location1=0x8, value=1, comparand=0)
    at /media/kbaladurin/data/dotnet/forked/corert/src/System.Private.CoreLib/src/System/Threading/Interlocked.cs:290
#4  0x000000000093e0d4 in System_Private_CoreLib::System::Threading::Lock::Acquire (___this=0x0) at /media/kbaladurin/data/dotnet/forked/corert/src/System.Private.CoreLib/src/System/Threading/Lock.cs:83
#5  0x0000000000936c49 in System_Private_CoreLib::System::Threading::LockHolder::Hold (l=0x0)
    at /media/kbaladurin/data/dotnet/forked/corert/src/System.Private.CoreLib/src/System/Threading/LockHolder.cs:18
#6  0x0000000000954d95 in System_Private_CoreLib::System::Runtime::CompilerServices::ClassConstructorRunner_Cctor::GetCctor (pContext=0x1321600 <_System_Private_CoreLib_System_String_statics>)
    at /media/kbaladurin/data/dotnet/forked/corert/src/System.Private.CoreLib/src/System/Runtime/CompilerServices/ClassConstructorRunner.cs:295
#7  0x0000000000938ab4 in System_Private_CoreLib::System::Runtime::CompilerServices::ClassConstructorRunner::EnsureClassConstructorRun (pContext=0x1321600 <_System_Private_CoreLib_System_String_statics>)
    at /media/kbaladurin/data/dotnet/forked/corert/src/System.Private.CoreLib/src/System/Runtime/CompilerServices/ClassConstructorRunner.cs:74
#8  0x00000000007607a9 in System_Private_CoreLib::System::Runtime::CompilerServices::ClassConstructorRunner::CheckStaticClassConstructionReturnNonGCStaticBase (
    context=0x1321600 <_System_Private_CoreLib_System_String_statics>, nonGcStaticBase=20059648)
    at /media/kbaladurin/data/dotnet/forked/corert/src/System.Private.CoreLib/src/System/Runtime/CompilerServices/ClassConstructorRunner.cs:49
#9  0x00000000008f7e5d in System_Private_CoreLib::System::Diagnostics::Debug::Assert (condition=1 '\001')
    at /media/kbaladurin/data/dotnet/forked/corert/src/System.Private.CoreLib/shared/System/Diagnostics/Debug.cs:90
#10 0x00000000008fd1b0 in System_Private_CoreLib::System::Runtime::RuntimeExports::RhNewArray (pEEType=..., length=1)
    at /media/kbaladurin/data/dotnet/forked/corert/src/Runtime.Base/src/System/Runtime/RuntimeExports.cs:68
#11 0x00000000008fd243 in RhNewArray (pEEType=..., length=1) at /media/kbaladurin/data/dotnet/forked/corert/src/Runtime.Base/src/System/Runtime/RuntimeExports.cs:80
#12 0x0000000000e3d4b1 in __allocate_array (elements=1, pMT=0x13fe9e8 <__Array_A_::System_Private_CoreLib::System::Runtime::TypeManagerHandle_V_::__getMethodTable()::mt>)
    at /media/kbaladurin/data/dotnet/forked/corert/src/Native/Bootstrap/main.cpp:110
#13 0x00000000009055e0 in System_Private_CoreLib::Internal::Runtime::CompilerHelpers::StartupCodeHelpers::CreateTypeManagers (osModule=0, pModuleHeaders=0x148c4a8 <RtRHeaderWrapper::rtrHeaderWrapper>, 
    count=2, pClasslibFunctions=0x1320370 <c_classlibFunctions>, nClasslibFunctions=9)
    at /media/kbaladurin/data/dotnet/forked/corert/src/Common/src/Internal/Runtime/CompilerHelpers/StartupCodeHelpers.cs:119
#14 0x0000000000905004 in System_Private_CoreLib::Internal::Runtime::CompilerHelpers::StartupCodeHelpers::InitializeModules (osModule=0, pModuleHeaders=0x148c4a8 <RtRHeaderWrapper::rtrHeaderWrapper>, 
    count=2, pClasslibFunctions=0x1320370 <c_classlibFunctions>, nClasslibFunctions=9)
    at /media/kbaladurin/data/dotnet/forked/corert/src/Common/src/Internal/Runtime/CompilerHelpers/StartupCodeHelpers.cs:35
#15 0x0000000000906113 in InitializeModules (osModule=0, pModuleHeaders=0x148c4a8 <RtRHeaderWrapper::rtrHeaderWrapper>, count=2, pClasslibFunctions=0x1320370 <c_classlibFunctions>, nClasslibFunctions=9)
    at /media/kbaladurin/data/dotnet/forked/corert/src/Common/src/Internal/Runtime/CompilerHelpers/StartupCodeHelpers.cs:66
#16 0x0000000000e3dae1 in InitializeRuntime () at /media/kbaladurin/data/dotnet/forked/corert/src/Native/Bootstrap/main.cpp:349
#17 0x0000000000e3d99b in main (argc=1, argv=0x7fffffffdb28) at /media/kbaladurin/data/dotnet/forked/corert/src/Native/Bootstrap/main.cpp:367

string's constructor is called from CreateTypeManagers and InitializeEagerClassConstructorsForModule is called after it:

[NativeCallable(EntryPoint = "InitializeModules", CallingConvention = CallingConvention.Cdecl)]
internal static unsafe void InitializeModules(IntPtr osModule, IntPtr* pModuleHeaders, int count, IntPtr* pClasslibFunctions, int nClasslibFunctions)
{
    RuntimeImports.RhpRegisterOsModule(osModule);
    TypeManagerHandle[] modules = CreateTypeManagers(osModule, pModuleHeaders, count, pClasslibFunctions, nClasslibFunctions);

    for (int i = 0; i < modules.Length; i++)
    {
        InitializeGlobalTablesForModule(modules[i], i);
    }

    // We are now at a stage where we can use GC statics - publish the list of modules
    // so that the eager constructors can access it.
    if (s_modules != null)
    {
        for (int i = 0; i < modules.Length; i++)
        {
            AddModule(modules[i]);
        }
    }
    else
    {
        s_modules = modules;
        s_moduleCount = modules.Length;
    }

    // These two loops look funny but it's important to initialize the global tables before running
    // the first class constructor to prevent them calling into another uninitialized module
    for (int i = 0; i < modules.Length; i++)
    {
        InitializeEagerClassConstructorsForModule(modules[i]);
    }
}

Copy link
Member

Choose a reason for hiding this comment

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

Hm, we are in an odd situation because of INPLACE_RUNTIME. Normally, the managed portions of the runtime are defined in the Runtime.Base assembly that we compile as a separate thing. It has its own version of System.Object, and its own version of System.Diagnostic.Debug in https://github.com/dotnet/corert/blob/master/src/Runtime.Base/src/System/Diagnostics/Debug.cs. This is the version of System.Diagnostics.Debug that RhNewArray is calling into.

In CoreRT, we took a shortcut and de facto merged Runtime.Base with System.Private.CoreLib (you can see files from Runtime.Base included in System.Private.CoreLib). You can see places that needed to adjust for this under INPLACE_RUNTIME ifdef. We could fix the assert in RhNewArray with some extra INPLACE_RUNTIME ifdeffing, but it looks harmless (what is essentially happening is that we access the static base of System.String before the cctor ran, but since null and an empty string literal are interchangeable for this code path, it doesn't matter).

The only thing that needs to happen to make this go away is on the CppCodegen side: not trigger cctors if Compilation.HasLazyStaticConstructor returns false.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thank you! I've added support for eager cctors.

Copy link
Member

@MichalStrehovsky MichalStrehovsky left a comment

Choose a reason for hiding this comment

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

Thank you!

@jkotas jkotas merged commit 1d8ba42 into dotnet:master Dec 10, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants