[NativeAOT] ordering issue around ensuring static constructor execution

[VSadov]

Re:

runtime/src/coreclr/nativeaot/System.Private.CoreLib/src/System/Runtime/CompilerServices/ClassConstructorRunner.cs

Line 59 in 983c8f2

if (pContext->initialized == 1)

We check pContext->initialized with an ordinary read, before accessing the static fields. There is a possibility that we will read the fields before reading the flag on weak-ordered architectures.

The code orders the write to the flag after running constructors by doing full memory fence, but that is both too much and not sufficient.
Too much is because just doing Volatile.Write(ref pContext->initialized, 1) is enough to guarantee the order of writes.
Not sufficient because the read side can still read ahead of the flag check.

A simple fix would be to do volatile read of the flag when checking.
However the same flag check is emitted by the JIT. Not sure if we emit ldar there. I suspect we do not. If that is a case, emitting ldar would be a simple solution.

If ultimately, we do not want to emit ldar, an alternative could be using a slightly different pattern where initialized instead of being a condition flag, could be a pointer used to access the statics. I.E. if it is null the cctor has not run yet, and if it is not null, use it as a pointer (or an offset even if it is fixed, as long as it is used to compute the address) to access static state, then we could benefit from data dependency ordering the reads.

Replacing Interlocked.MemoryBarrier() with a process-wide barrier would work too, but seems too heavy.

I couldn't figure out the best area label to add to this issue. If you have write-permissions please help me learn by adding exactly one area label.

Tagging subscribers to this area: @agocke, @MichalStrehovsky, @jkotas
See info in area-owners.md if you want to be subscribed.

Issue Details

---

Re:

runtime/src/coreclr/nativeaot/System.Private.CoreLib/src/System/Runtime/CompilerServices/ClassConstructorRunner.cs

Line 59 in 983c8f2

if (pContext->initialized == 1)

We check pContext->initialized with an ordinary read, before accessing the static fields. There is a possibility that we will read the fields before reading the flag on weak-ordered architectures.

The code orders the write to the flag after running constructors by doing full memory fence, but that is both too much and not sufficient.
Too much is because just doing Volatile.Write(ref pContext->initialized, 1) is enough to guarantee the order of writes.
Not sufficient because the read side can still read ahead of the flag check.

A simple fix would be to do volatile read of the flag when checking.
However the same flag check is emitted by the JIT. Not sure if we emit ldar there. I suspect we do not. If that is a case emitting ldar would be a simple solution.

If ultimately, we do not want to emit ldar, an alternative could be using slightly different pattern where initialized instead of being a condition flag, could be a pointer used to access the statics. I.E. if it is null the cctor has not run yet, and if it is not null, use it as a pointer (or an offset even if it is fixed, as long as it is used to compute the address) to access static state, then we could benefit from data dependency ordering the reads.

Replacing Interlocked.MemoryBarrier() witha process-wide barrier would work too, but seems too heavy.

Author:	VSadov
Assignees:	-
Labels:	untriaged, area-NativeAOT-coreclr
Milestone:	-

[jkotas]

Does standard CoreCLR have the same problem? I do not see any barriers there either.

[VSadov]

There is even less synchronization in CoreCLR. Setting the flag (ClassInitFlags::INITIALIZED_FLAG) does not try to order it after running the cctor. If I am looking at the right place.

CoreCLR implementation is more complex too. Perhaps it is just timing between completion of cctor and setting the flag, that makes it unlikely that inconsistent state can be observed.

This is how the full fence could be helping in NativeAOT. The fence could be introducing enough delay that we get lucky most of the time.
We do not need that fence on x64 though, so I wonder if there is any history for adding it - did we see a race/failure on arm64 and adding the fence helped?

A race for a cctor completion would be a relatively rare thing, since for a given class you get just one chance per program execution.

[jkotas]

I think this should be fixed for 9.0 given that we have an evidence that these race conditions are a real problem and we are fixing them in the JIT.

[VSadov]

Are we going to fix this for ARM32 as well?

[jkotas]

I think so. The JIT fix #105832 is arch neural, it should work for arm32 as well assuming that we emit correct code for volatile on arm32.