-
Notifications
You must be signed in to change notification settings - Fork 4.7k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Type initializer call ordering w.r.t. static field stores does not look right #72354
Comments
Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch Issue DetailsReproduction: 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 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
|
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) |
This seems spec-compliant to me, but probably fairly unintuitive to most users. |
In the standard we have:
I. e. not-beforefieldinit types have "precise" semantics on when the I agree that it is not clear whether "access" means load instructions only, hence name of the issue. |
(First, an aside - I don't see that behavior with your test case as Thanks for pointing this out; it is an interesting observation. However, I believe that this is the intended behavior:
|
cc @SingleAccretion who opened this issue before I close this |
The main point of the issue is that we see the Could you share the IR for 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 |
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. |
Insertion of the type initializer before the tree for a stsfld could reorder them inappropriately. This includes those computation in the spill check. Fixes #72354.
Reproduction:
Compile and run.
Expected result: the exception from
Call
is thrown.Actual result: the type initializer throws instead:
Cause - the insertion point of the helper here:
runtime/src/coreclr/jit/importer.cpp
Lines 16140 to 16143 in aafa910
category:correctness
theme:basic-cq
The text was updated successfully, but these errors were encountered: