Type initializer call ordering w.r.t. static field stores does not look right

[SingleAccretion]

Reproduction:

using System;
using System.Runtime.CompilerServices;

Problem();

[MethodImpl(MethodImplOptions.NoInlining)]
static void Problem()
{
    [MethodImpl(MethodImplOptions.NoInlining)]
    static int Call() => throw new Exception("Thrown from 'Call'");

    var a = Call();
    ClassWithCctor.StaticField = a;
}

class ClassWithCctor
{
    public static int StaticField;
    static ClassWithCctor() => throw new Exception("Thrown from 'ClassWithCctor'");
}

Compile and run.

Expected result: the exception from Call is thrown.

Actual result: the type initializer throws instead:

Unhandled exception. System.TypeInitializationException: The type initializer for 'ClassWithCctor' threw an exception.
 ---> System.Exception: Thrown from 'ClassWithCctor'

Cause - the insertion point of the helper here:

runtime/src/coreclr/jit/importer.cpp

Lines 16140 to 16143 in aafa910

	if (helperNode != nullptr)
	{
	op1 = gtNewOperNode(GT_COMMA, op1->TypeGet(), helperNode, op1);
	}

category:correctness
theme:basic-cq

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

Issue Details

---

Reproduction:

using System;
using System.Runtime.CompilerServices;

Problem();

[MethodImpl(MethodImplOptions.NoInlining)]
static void Problem()
{
    [MethodImpl(MethodImplOptions.NoInlining)]
    static int Call() => throw new Exception("Thrown from 'Call'");

    var a = Call();
    ClassWithCctor.StaticField = a;
}

class ClassWithCctor
{
    public static int StaticField;
    static ClassWithCctor() => throw new Exception("Thrown from 'ClassWithCctor'");
}

Compile and run.

Expected result: the exception from Call is thrown.

Actual result: the type initializer throws instead:

Unhandled exception. System.TypeInitializationException: The type initializer for 'ClassWithCctor' threw an exception.
 ---> System.Exception: Thrown from 'ClassWithCctor'

Cause - the insertion point of the helper here:

runtime/src/coreclr/jit/importer.cpp

Lines 16140 to 16143 in aafa910

	if (helperNode != nullptr)
	{
	op1 = gtNewOperNode(GT_COMMA, op1->TypeGet(), helperNode, op1);
	}

Author:	SingleAccretion
Assignees:	-
Labels:	bug, area-CodeGen-coreclr
Milestone:	8.0.0

[jakobbotsch]

Is this a bug? I'm not sure we make any guarantees about when static constructors will run except for "some time before the class is accessed".

(edit: but still seems reasonable to change this)

[alexrp]

This seems spec-compliant to me, but probably fairly unintuitive to most users.

[SingleAccretion]

In the standard we have:

If not marked BeforeFieldInit then that type’s initializer method is executed at (i.e., 
is triggered by): 

a. first access to any static field of that type, or
b. first invocation of any static method of that type, or
c. first invocation of any instance or virtual method of that type if it is a value 
type or 
d. first invocation of any constructor for that type.

I. e. not-beforefieldinit types have "precise" semantics on when the .cctor runs.

I agree that it is not clear whether "access" means load instructions only, hence name of the issue.

[markples]

(First, an aside - I don't see that behavior with your test case as Call is invoked before the static field write, but that's irrelevant to the general question of whether a write should trigger type initialization.)

Thanks for pointing this out; it is an interesting observation. However, I believe that this is the intended behavior:

• Type initialization is also triggered by loading the address of a static field. Otherwise reads (and writes) through interior pointers would need some sort of check just in case they are operating on static fields (and maybe something when pinning a value that happens to point to a static field?). Since this address may or may not be used to read or write, it's more consistent to just initialize on any read/write/load-address.

• In cases where the type initializer writes the field in question, a "pre-type-initialization" write couldn't be observed because it would be overwritten before the read. So, static = 5; Console.WriteLine(static); might not print 5.

• Other uses of the word "access" in the spec (emphasis mine) mostly seem to indicate reads or writes, though this list is not complete:

  I.5 "property: A member that defines a named value and the methods that access that value. A property definition defines the accessing contracts on that value. Hence, the property definition specifies which accessing methods exist and their respective method contracts."

  "verification: The checking of both CIL and its related metadata to ensure that the CIL code sequences do not permit any access to memory outside the program’s logical address space. In conjunction with the validation tests, verification ensures that the program cannot access memory or other resources to which it is not granted access."

  I.6.1 "Access to an object is through accessible functions and fields."

  I.8.4.1 "... or they are accessed by an indexing expression, in which case, they are called array elements."

  I.8.5.3 "Access to a member of a type is permitted only if all three of the following conditions are met:"

  II.16.3.2 "The data is then accessed by a program as it would access any other static variable, using instructions such as ldsfld, ldsflda, and so on"

  III.2.6 "Marking an access as volatile. affects only that single access; other accesses to the same location shall be marked separately."

  I did find one case where access did seem to be used only for a read, though it's more that "access" was used for a read but "set" was used for a write.

  II.14.2 "A Get method that takes a sequence of int32 arguments, one for each dimension of the
  array, and returns a value whose type is the element type of the array. This method is used to
  access a specific element of the array where the arguments specify the index into each
  dimension, beginning with the first, of the element to be returned.

  A Set method that takes a sequence of int32 arguments, one for each dimension of the
  array, followed by a value whose type is the element type of the array. The return type of
  Set is void. This method is used to set a specific element of the array where the arguments
  specify the index into each dimension, beginning with the first, of the element to be set and
  the final argument specifies the value to be stored into the target element."

• One other point is that this would be a breaking change. Perhaps that doesn't matter if this doesn't occur in practice, though that also takes away motivation for such a change.

[markples]

cc @SingleAccretion who opened this issue before I close this

[SingleAccretion]

I don't see that behavior with your test case as Call is invoked before the static field write, but that's irrelevant to the general question of whether a write should trigger type initialization.

The main point of the issue is that we see the .cctor throw before Call and not the other way around. We are on the same page about whether "write access" (stsfld) should trigger type initialization (it should), which is encouraging.

Could you share the IR for Problem that you observe?

For reference, I tried with a recent-ish runtime build and have:

***** BB01
STMT00000 ( 0x000[E-] ... 0x005 )
               [000007] --CXG+-----                         *  CALL help long   CORINFO_HELP_GETSHARED_NONGCSTATIC_BASE
               [000005] -----+----- arg0 in rcx             +--*  CNS_INT   long   0x7ffc86b70a88
               [000006] -----+----- arg1 in rdx             \--*  CNS_INT   int    6

***** BB01
STMT00001 ( ??? ... ??? )
               [000003] -ACXG+-----                         *  ASG       int
               [000002] n---G+-N---                         +--*  IND       int
               [000001] I----+-----                         |  \--*  CNS_INT(h) long   0x7ffc86b70b3c static Fseq[StaticField]
               [000000] --CXG+-----                         \--*  CALL      int    RyuJitReproduction.Program:<Problem>g__Call|27_0():int

***** BB01
STMT00002 ( 0x00A[E-] ... 0x00A )
               [000008] -----+-----                         *  RETURN    void

(With a TypeInitializationException as expected)

[markples]

This is a difference between C# code that has been compiled with /optimize vs not. Without it, we get call / stloc / ldloc / stsfld, and it is fine. With it, we get call / stsfld, and the importer incorrectly adds the type initialization call before the tree for them. I'm testing a fix that properly runs this case.