JIT: treat static base helpers that may run a cctor as heap writes

src/coreclr/jit/gentree.h

@@ -462,7 +462,8 @@ enum GenTreeFlags : unsigned
 
     GTF_CALL_NULLCHECK          = 0x08000000, // GT_CALL -- must check instance pointer for null
     GTF_CALL_POP_ARGS           = 0x04000000, // GT_CALL -- caller pop arguments?
-    GTF_CALL_HOISTABLE          = 0x02000000, // GT_CALL -- call is hoistable
+    GTF_CALL_HOISTABLE          = 0x02000000, // GT_CALL -- call is hoistable (any cctor it runs may run earlier;
+                                              //            the call still may modify the heap)
     GTF_TLS_GET_ADDR            = 0x01000000, // GT_CALL -- call is tls_get_addr
 
     GTF_MEMORYBARRIER_LOAD      = 0x40000000, // GT_MEMORYBARRIER -- Load barrier

src/coreclr/jit/optimizer.cpp

@@ -4659,7 +4659,7 @@ void Compiler::optHoistLoopBlocks(FlowGraphNaturalLoop* loop,
 
                 // If it's a call, it must be a helper call, and be pure.
                 // Further, if it may run a cctor, it must be labeled as "Hoistable"
-                // (meaning it won't run a cctor because the class is not precise-init).
+                // (meaning the class is not precise-init, so the cctor may run earlier).
                 if (treeIsHoistable && tree->IsCall())
                 {
                     GenTreeCall* call = tree->AsCall();
@@ -4740,7 +4740,7 @@ void Compiler::optHoistLoopBlocks(FlowGraphNaturalLoop* loop,
                 {
                     // If it's a call, it must be a helper call that does not mutate the heap.
                     // Further, if it may run a cctor, it must be labeled as "Hoistable"
-                    // (meaning it won't run a cctor because the class is not precise-init).
+                    // (meaning the class is not precise-init, so the cctor may run earlier).
                     GenTreeCall* call = tree->AsCall();
                     if (!call->IsHelperCall())
                     {
@@ -5448,21 +5448,12 @@ void Compiler::optComputeLoopSideEffectsOfBlock(BasicBlock* blk, FlowGraphNatura
                     if (call->IsHelperCall())
                     {
                         CorInfoHelpFunc helpFunc = call->GetHelperNum();
-                        if (s_helperCallProperties.MutatesHeap(helpFunc))
+                        // A class constructor run by the helper may modify the heap.
+                        if (s_helperCallProperties.MutatesHeap(helpFunc) ||
+                            s_helperCallProperties.MayRunCctor(helpFunc))
                         {
                             memoryHavoc |= memoryKindSet(GcHeap, ByrefExposed);
                         }
-                        else if (s_helperCallProperties.MayRunCctor(helpFunc))
-                        {
-                            // If the call is labeled as "Hoistable", then we've checked the
-                            // class that would be constructed, and it is not precise-init, so
-                            // the cctor will not be run by this call.  Otherwise, it might be,
-                            // and might have arbitrary side effects.
-                            if ((tree->gtFlags & GTF_CALL_HOISTABLE) == 0)
-                            {
-                                memoryHavoc |= memoryKindSet(GcHeap, ByrefExposed);
-                            }
-                        }
                     }
                     else
                     {

src/coreclr/jit/valuenum.cpp

@@ -15546,8 +15546,8 @@ bool Compiler::fgValueNumberHelperCall(GenTreeCall* call)
 
     bool pure        = s_helperCallProperties.IsPure(helpFunc);
     bool isAlloc     = s_helperCallProperties.IsAllocator(helpFunc);
-    bool modHeap     = s_helperCallProperties.MutatesHeap(helpFunc);
     bool mayRunCctor = s_helperCallProperties.MayRunCctor(helpFunc);
+    bool modHeap     = s_helperCallProperties.MutatesHeap(helpFunc) || mayRunCctor; // a cctor may modify the heap
     bool noThrow     = s_helperCallProperties.NoThrow(helpFunc);
 
     ValueNumPair vnpExc = ValueNumStore::VNPForEmptyExcSet();
@@ -15697,14 +15697,6 @@ bool Compiler::fgValueNumberHelperCall(GenTreeCall* call)
         {
             VNFunc vnf = fgValueNumberJitHelperMethodVNFunc(helpFunc);
 
-            if (mayRunCctor)
-            {
-                if ((call->gtFlags & GTF_CALL_HOISTABLE) == 0)
-                {
-                    modHeap = true;
-                }
-            }
-
             if (isAlloc && (call->gtArgs.FindWellKnownArg(WellKnownArg::StackArrayLocal) != nullptr))
             {
                 if (vnf == VNF_JitNewArr)

src/tests/JIT/Regression_ro_2/Runtime_136042.cs

@@ -0,0 +1,175 @@
+// Licensed to the .NET Foundation under one or more agreements.
+// The .NET Foundation licenses this file to you under the MIT license.
+
+namespace Runtime_136042;
+
+using System.Runtime.CompilerServices;
+using Xunit;
+
+// Accessing a static of a beforefieldinit class may run its static constructor, which can
+// modify the heap. A static read after the access must observe that effect.
+// Methods without AggressiveOptimization are precompiled when running with crossgen2.
+public static class Runtime_136042
+{
+    static class Straight
+    {
+        public static int Counter;
+        public class G { public static int Value = ++Counter; }
+    }
+
+    static class StraightGen
+    {
+        public static int Counter;
+        public class G<T> { public static int Value = ++Counter; }
+    }
+
+    static class StraightOpt
+    {
+        public static int Counter;
+        public class G { public static int Value = ++Counter; }
+    }
+
+    static class StraightOptGen
+    {
+        public static int Counter;
+        public class G<T> { public static int Value = ++Counter; }
+    }
+
+    static class Loop
+    {
+        public static int Counter;
+        public class G { public static int Value = ++Counter; }
+    }
+
+    static class LoopGen
+    {
+        public static int Counter;
+        public class G<T> { public static int Value = ++Counter; }
+    }
+
+    static class LoopOpt
+    {
+        public static int Counter;
+        public class G { public static int Value = ++Counter; }
+    }
+
+    static class LoopOptGen
+    {
+        public static int Counter;
+        public class G<T> { public static int Value = ++Counter; }
+    }
+
+    [MethodImpl(MethodImplOptions.NoInlining)]
+    static (int, int, int) StraightNonGeneric()
+    {
+        int before = Straight.Counter;
+        int value = Straight.G.Value;
+        return (before, value, Straight.Counter);
+    }
+
+    [MethodImpl(MethodImplOptions.NoInlining)]
+    static (int, int, int) StraightGeneric<T>()
+    {
+        int before = StraightGen.Counter;
+        int value = StraightGen.G<T>.Value;
+        return (before, value, StraightGen.Counter);
+    }
+
+    [MethodImpl(MethodImplOptions.NoInlining | MethodImplOptions.AggressiveOptimization)]
+    static (int, int, int) StraightOptNonGeneric()
+    {
+        int before = StraightOpt.Counter;
+        int value = StraightOpt.G.Value;
+        return (before, value, StraightOpt.Counter);
+    }
+
+    [MethodImpl(MethodImplOptions.NoInlining | MethodImplOptions.AggressiveOptimization)]
+    static (int, int, int) StraightOptGeneric<T>()
+    {
+        int before = StraightOptGen.Counter;
+        int value = StraightOptGen.G<T>.Value;
+        return (before, value, StraightOptGen.Counter);
+    }
+
+    [MethodImpl(MethodImplOptions.NoInlining)]
+    static int LoopNonGeneric(int n)
+    {
+        int seen = 0;
+        int sum = 0;
+        for (int i = 0; i < n; i++)
+        {
+            seen = seen * 10 + Loop.Counter;
+            sum += Loop.G.Value;
+        }
+        return seen * 100 + sum;
+    }
+
+    [MethodImpl(MethodImplOptions.NoInlining)]
+    static int LoopGeneric<T>(int n)
+    {
+        int seen = 0;
+        int sum = 0;
+        for (int i = 0; i < n; i++)
+        {
+            seen = seen * 10 + LoopGen.Counter;
+            sum += LoopGen.G<T>.Value;
+        }
+        return seen * 100 + sum;
+    }
+
+    [MethodImpl(MethodImplOptions.NoInlining | MethodImplOptions.AggressiveOptimization)]
+    static int LoopOptNonGeneric(int n)
+    {
+        int seen = 0;
+        int sum = 0;
+        for (int i = 0; i < n; i++)
+        {
+            seen = seen * 10 + LoopOpt.Counter;
+            sum += LoopOpt.G.Value;
+        }
+        return seen * 100 + sum;
+    }
+
+    [MethodImpl(MethodImplOptions.NoInlining | MethodImplOptions.AggressiveOptimization)]
+    static int LoopOptGeneric<T>(int n)
+    {
+        int seen = 0;
+        int sum = 0;
+        for (int i = 0; i < n; i++)
+        {
+            seen = seen * 10 + LoopOptGen.Counter;
+            sum += LoopOptGen.G<T>.Value;
+        }
+        return seen * 100 + sum;
+    }
+
+    static void CheckStraight((int Before, int Value, int After) result, int expectedValue)
+    {
+        Assert.Equal(expectedValue, result.Value);
+        Assert.Equal(expectedValue, result.After);
+        Assert.True(result.Before == expectedValue - 1 || result.Before == expectedValue);
+    }
+
+    static void CheckLoop(int result, int expectedValue)
+    {
+        // Two iterations: the first may observe the counter before or after the static constructor ran,
+        // the second must observe it after.
+        int before = expectedValue - 1;
+        int seenEarly = (before * 10 + expectedValue) * 100 + 2 * expectedValue;
+        int seenLate = (expectedValue * 10 + expectedValue) * 100 + 2 * expectedValue;
+        Assert.True(result == seenEarly || result == seenLate, $"Unexpected result {result}");
+    }
+
+    [Fact]
+    public static void TestEntryPoint()
+    {
+        CheckStraight(StraightNonGeneric(), 1);
+        CheckStraight(StraightGeneric<object>(), 1);
+        CheckStraight(StraightOptNonGeneric(), 1);
+        CheckStraight(StraightOptGeneric<object>(), 1);
+        CheckLoop(LoopNonGeneric(2), 1);
+        CheckLoop(LoopGeneric<object>(2), 1);
+        CheckLoop(LoopOptNonGeneric(2), 1);
+        CheckLoop(LoopOptGeneric<object>(2), 1);
+    }
+}