Allocate static boxes on FOH #2

[EgorBo]

Reverts 2nd part of #78296 revert.

NOTE: this doesn't fix the flaky NRE as I'm able to reproduce it locally

[EgorBo]

@jkotas maybe it rings a bell to you - so when I set StaticBoxInitLock crst scope I might still hit a

Consistency check failed: Crst Level violation: Can't take level 12 lock CrstAssemblyLoader because you already holding level 4 lock CrstStaticBoxInit

My understanding that it happens because of pFieldMT->EnsureInstanceActive(); inside AllocateStaticBox - but if I move this call to a place before I open the CrstStaticBoxInit it won't help, because it doesn't block the thread and wait till the assembly is loaded, right? So the question is - should I somehow wait field's type to be fully loaded before I open the scope or in this particular case it's fine to modify Crst to account for CrstAssemblyLoader ?

[jkotas]

AssemblyLoader can run arbitrary code (e.g. due to user overridable ResolveAssembly event). You do not want to be holding any runtime locks when the assembly loader runs.

[EgorBo]

@jkotas it looks like the issue is fixed via GCPROTECT_BEGININTERIOR(boxedStaticHandle); - can't repro it anymore

[EgorBo]

/azp run runtime-coreclr outerloop, runtime-coreclr gcstress0x3-gcstress0xc

[azure-pipelines]

Azure Pipelines successfully started running 2 pipeline(s).

[jkotas]

it looks like the issue is fixed via GCPROTECT_BEGININTERIOR(boxedStaticHandle); - can't repro it anymore

If it is the case, it means that fieldAddress can be moved by the GC and the problems is spread over more places. For example, pResult->fieldLookup.addr at https://github.com/dotnet/runtime/pull/79188/files#diff-98b4e379205d747e922689cb993f985be190fedb509fd486e99280c842703472R1574 can be moved by the GC as well.

[jkotas]

What guarantees that pResult->fieldLookup.addr is not going to move once it is return to the JIT?

[EgorBo]

@jkotas I don't have a good explanation yet, what I understand so far:

1. GetGCStaticsBasePointer is assumed to be moveable already: https://github.com/dotnet/runtime/blob/main/src/coreclr/vm/methodtable.cpp#L3485
2. I thought that it supposed to return an address to a pinned area so that's how we can give it to JIT
   but apparently it works differently for collectible contexts? And in case of collectible context is not supposed to be consumed by JIT (should go through the CORINFO_HELP_GETSHARED_GCSTATIC_BASE_DYNAMICCLASS helper)
3. the places where it's used already seem to be safe? because of COOP and nothing triggers GC (and e.g. BulkStaticsLogger::LogAllStatics() has GC_NOTRIGGER)

[jkotas]

Yes, the statics for collectible types do not seem to be pinned:

runtime/src/coreclr/vm/appdomain.cpp

Line 4184 in 1ad1022

voidDomainLocalModule::AllocateDynamicClass(MethodTable *pMT)

. I assume that it is to prevent GC heap fragmentation that the long-term pinning would cause.

I think that this means that we need to avoid giving the JIT address of the collectible type statics?

[EgorBo]

I think that this means that we need to avoid giving the JIT address of the collectible type statics?

Looks like we already don't do it, at least we don't hit that path where we set the address.

[jkotas]

LGTM otherwise. Thank you!

[EgorBo]

Failures are #79255