From fa60eb754c45e1d3834ec3d08dc00aae42a53084 Mon Sep 17 00:00:00 2001 From: Mark Plesko Date: Fri, 21 Apr 2023 15:38:04 -0700 Subject: [PATCH] Fix type initialization ordering under R2R (#84275) Expand the fix in #80485 to cover helpers including common ones for R2R as well as some generic helpers for JIT. This would cause regressions (100ks code size in diffs) if done alone, so it also includes checks on whether a helper may trigger a cctor and whether it is beforefieldinit. The precise ordering of the fix is only required for a non-beforefieldinit ctor. This recovers the (small) losses from #80485 and mitigates this fix. As before, the primary diff is moving type initialization into (for correctness) or out of (the mitigation optimization) a JIT tree. These leads to secondary effects as values may need to be preserved around the type initialization code. The end result of the fix+mitigations is very little code size change, but viewing the diffs suggests possible future code improvements. However, the impact of any of these should be established first. - A type initializer will never invoke itself - When type initialization is removed/CSEed late, it is too late for forward subst/etc., so the loss of the intact tree can still impact code generation - A custom calling convention for the static helpers (tuned to the common case of no type initialization needing to occur) could help with the impact of calling it in the middle of a calculation - Code can use beforefieldinit in more place Fixes #84007 --- src/coreclr/jit/compiler.h | 3 +- src/coreclr/jit/importer.cpp | 108 +++++++++++++++--- .../CctorsWithSideEffects/CctorForWrite2.cs | 47 ++++++++ .../CctorForWrite2.csproj | 9 ++ src/tests/issues.targets | 3 - 5 files changed, 148 insertions(+), 22 deletions(-) create mode 100644 src/tests/Loader/classloader/TypeInitialization/CctorsWithSideEffects/CctorForWrite2.cs create mode 100644 src/tests/Loader/classloader/TypeInitialization/CctorsWithSideEffects/CctorForWrite2.csproj diff --git a/src/coreclr/jit/compiler.h b/src/coreclr/jit/compiler.h index 69ed2898bd575..426e790f712ba 100644 --- a/src/coreclr/jit/compiler.h +++ b/src/coreclr/jit/compiler.h @@ -3858,7 +3858,8 @@ class Compiler GenTree* impImportStaticFieldAccess(CORINFO_RESOLVED_TOKEN* pResolvedToken, CORINFO_ACCESS_FLAGS access, CORINFO_FIELD_INFO* pFieldInfo, - var_types lclTyp); + var_types lclTyp, + /* OUT */ bool* pIsHoistable = nullptr); static void impBashVarAddrsToI(GenTree* tree1, GenTree* tree2 = nullptr); diff --git a/src/coreclr/jit/importer.cpp b/src/coreclr/jit/importer.cpp index 8a09fbb5a6bf4..3560c05132a8d 100644 --- a/src/coreclr/jit/importer.cpp +++ b/src/coreclr/jit/importer.cpp @@ -4098,19 +4098,35 @@ GenTree* Compiler::impImportCnsTreeFromBuffer(uint8_t* buffer, var_types valueTy return tree; } +//------------------------------------------------------------------------ +// impImportStaticFieldAccess: Generate an access of a static field +// +// Arguments: +// pResolvedToken - resolved token for the static field to access +// access - type of access to the field, distinguishes address-taken vs load/store +// pFieldInfo - EE instructions for accessing the field +// lclTyp - type of the field +// pIsHoistable - optional out parameter - whether any type initialization side effects +// of the returned tree can be hoisted to occur earlier +// +// Return Value: +// Tree representing the access to the static field +// +// Notes: +// Ordinary static fields never overlap. RVA statics, however, can overlap (if they're +// mapped to the same ".data" declaration). That said, such mappings only appear to be +// possible with ILASM, and in ILASM-produced (ILONLY) images, RVA statics are always +// read-only (using "stsfld" on them is UB). In mixed-mode assemblies, RVA statics can +// be mutable, but the only current producer of such images, the C++/CLI compiler, does +// not appear to support mapping different fields to the same address. So we will say +// that "mutable overlapping RVA statics" are UB as well. + GenTree* Compiler::impImportStaticFieldAccess(CORINFO_RESOLVED_TOKEN* pResolvedToken, CORINFO_ACCESS_FLAGS access, CORINFO_FIELD_INFO* pFieldInfo, - var_types lclTyp) + var_types lclTyp, + /* OUT */ bool* pIsHoistable) { - // Ordinary static fields never overlap. RVA statics, however, can overlap (if they're - // mapped to the same ".data" declaration). That said, such mappings only appear to be - // possible with ILASM, and in ILASM-produced (ILONLY) images, RVA statics are always - // read-only (using "stsfld" on them is UB). In mixed-mode assemblies, RVA statics can - // be mutable, but the only current producer of such images, the C++/CLI compiler, does - // not appear to support mapping different fields to the same address. So we will say - // that "mutable overlapping RVA statics" are UB as well. - // For statics that are not "boxed", the initial address tree will contain the field sequence. // For those that are, we will attach it later, when adding the indirection for the box, since // that tree will represent the true address. @@ -4151,6 +4167,7 @@ GenTree* Compiler::impImportStaticFieldAccess(CORINFO_RESOLVED_TOKEN* pResolvedT outerFldSeq = nullptr; } + bool isHoistable = false; bool isStaticReadOnlyInitedRef = false; unsigned typeIndex = 0; GenTree* op1; @@ -4182,6 +4199,8 @@ GenTree* Compiler::impImportStaticFieldAccess(CORINFO_RESOLVED_TOKEN* pResolvedT break; } + isHoistable = !s_helperCallProperties.MayRunCctor(pFieldInfo->helper) || + (info.compCompHnd->getClassAttribs(pResolvedToken->hClass) & CORINFO_FLG_BEFOREFIELDINIT); op1 = gtNewHelperCallNode(pFieldInfo->helper, type, op1); op1 = gtNewOperNode(GT_ADD, type, op1, gtNewIconNode(pFieldInfo->offset, innerFldSeq)); } @@ -4199,8 +4218,10 @@ GenTree* Compiler::impImportStaticFieldAccess(CORINFO_RESOLVED_TOKEN* pResolvedT { GenTreeFlags callFlags = GTF_EMPTY; - if (info.compCompHnd->getClassAttribs(pResolvedToken->hClass) & CORINFO_FLG_BEFOREFIELDINIT) + if (!s_helperCallProperties.MayRunCctor(pFieldInfo->helper) || + (info.compCompHnd->getClassAttribs(pResolvedToken->hClass) & CORINFO_FLG_BEFOREFIELDINIT)) { + isHoistable = true; callFlags |= GTF_CALL_HOISTABLE; } @@ -4225,7 +4246,8 @@ GenTree* Compiler::impImportStaticFieldAccess(CORINFO_RESOLVED_TOKEN* pResolvedT else #endif { - op1 = fgGetStaticsCCtorHelper(pResolvedToken->hClass, pFieldInfo->helper, typeIndex); + op1 = fgGetStaticsCCtorHelper(pResolvedToken->hClass, pFieldInfo->helper, typeIndex); + isHoistable = isHoistable || (op1->gtFlags & GTF_CALL_HOISTABLE); } op1 = gtNewOperNode(GT_ADD, op1->TypeGet(), op1, gtNewIconNode(pFieldInfo->offset, innerFldSeq)); @@ -4249,6 +4271,7 @@ GenTree* Compiler::impImportStaticFieldAccess(CORINFO_RESOLVED_TOKEN* pResolvedT op1 = gtNewIndOfIconHandleNode(TYP_I_IMPL, fldAddr, GTF_ICON_STATIC_ADDR_PTR, true); } GenTree* offset = gtNewIconNode(pFieldInfo->offset, innerFldSeq); + isHoistable = true; op1 = gtNewOperNode(GT_ADD, TYP_I_IMPL, op1, offset); #else unreached(); @@ -4267,14 +4290,17 @@ GenTree* Compiler::impImportStaticFieldAccess(CORINFO_RESOLVED_TOKEN* pResolvedT GenTree* ctxTree = getRuntimeContextTree(kind.runtimeLookupKind); - GenTreeFlags callFlags = GTF_EMPTY; + CorInfoHelpFunc helper = CORINFO_HELP_READYTORUN_GENERIC_STATIC_BASE; + GenTreeFlags callFlags = GTF_EMPTY; - if (info.compCompHnd->getClassAttribs(pResolvedToken->hClass) & CORINFO_FLG_BEFOREFIELDINIT) + if (!s_helperCallProperties.MayRunCctor(helper) || + (info.compCompHnd->getClassAttribs(pResolvedToken->hClass) & CORINFO_FLG_BEFOREFIELDINIT)) { + isHoistable = true; callFlags |= GTF_CALL_HOISTABLE; } var_types type = TYP_BYREF; - op1 = gtNewHelperCallNode(CORINFO_HELP_READYTORUN_GENERIC_STATIC_BASE, type, ctxTree); + op1 = gtNewHelperCallNode(helper, type, ctxTree); op1->gtFlags |= callFlags; op1->AsCall()->setEntryPoint(pFieldInfo->fieldLookup); @@ -4317,7 +4343,8 @@ GenTree* Compiler::impImportStaticFieldAccess(CORINFO_RESOLVED_TOKEN* pResolvedT { handleKind = GTF_ICON_STATIC_HDL; } - op1 = gtNewIconHandleNode(fldAddr, handleKind, innerFldSeq); + isHoistable = true; + op1 = gtNewIconHandleNode(fldAddr, handleKind, innerFldSeq); INDEBUG(op1->AsIntCon()->gtTargetHandle = reinterpret_cast(pResolvedToken->hField)); if (pFieldInfo->fieldFlags & CORINFO_FLG_FIELD_INITCLASS) { @@ -4354,6 +4381,10 @@ GenTree* Compiler::impImportStaticFieldAccess(CORINFO_RESOLVED_TOKEN* pResolvedT } } + if (pIsHoistable) + { + *pIsHoistable = isHoistable; + } return op1; } @@ -9569,10 +9600,49 @@ void Compiler::impImportBlockCode(BasicBlock* block) } if (helperNode != nullptr) { - impAppendTree(helperNode, CHECK_SPILL_ALL, impCurStmtDI); + bool isHoistable = + info.compCompHnd->getClassAttribs(resolvedToken.hClass) & CORINFO_FLG_BEFOREFIELDINIT; + unsigned check_spill = isHoistable ? CHECK_SPILL_NONE : CHECK_SPILL_ALL; + impAppendTree(helperNode, check_spill, impCurStmtDI); } } + // Handle the cases that might trigger type initialization + // (and possibly need to spill the tree for the stored value) + switch (fieldInfo.fieldAccessor) + { + case CORINFO_FIELD_INSTANCE: +#ifdef FEATURE_READYTORUN + case CORINFO_FIELD_INSTANCE_WITH_BASE: +#endif + case CORINFO_FIELD_STATIC_TLS: + case CORINFO_FIELD_STATIC_ADDR_HELPER: + case CORINFO_FIELD_INSTANCE_HELPER: + case CORINFO_FIELD_INSTANCE_ADDR_HELPER: + // Nothing now - handled later + break; + + case CORINFO_FIELD_STATIC_TLS_MANAGED: + case CORINFO_FIELD_STATIC_ADDRESS: + case CORINFO_FIELD_STATIC_RVA_ADDRESS: + case CORINFO_FIELD_STATIC_SHARED_STATIC_HELPER: + case CORINFO_FIELD_STATIC_GENERICS_STATIC_HELPER: + case CORINFO_FIELD_STATIC_READYTORUN_HELPER: + case CORINFO_FIELD_STATIC_RELOCATABLE: + bool isHoistable; + op1 = impImportStaticFieldAccess(&resolvedToken, (CORINFO_ACCESS_FLAGS)aflags, &fieldInfo, + lclTyp, &isHoistable); + + if (!isHoistable) + { + impSpillSideEffects(true, CHECK_SPILL_ALL DEBUGARG("value for stsfld with typeinit")); + } + break; + + default: + assert(!"Unexpected fieldAccessor"); + } + // Pull the value from the stack. StackEntry se = impPopStack(); op2 = se.val; @@ -9606,6 +9676,9 @@ void Compiler::impImportBlockCode(BasicBlock* block) obj = nullptr; } + // Handle the cases that use the stored value (obj). + // Conveniently these don't trigger type initialization, so there aren't + // any ordering issues between it and the tree for the stored value. switch (fieldInfo.fieldAccessor) { case CORINFO_FIELD_INSTANCE: @@ -9674,8 +9747,7 @@ void Compiler::impImportBlockCode(BasicBlock* block) case CORINFO_FIELD_STATIC_GENERICS_STATIC_HELPER: case CORINFO_FIELD_STATIC_READYTORUN_HELPER: case CORINFO_FIELD_STATIC_RELOCATABLE: - op1 = impImportStaticFieldAccess(&resolvedToken, (CORINFO_ACCESS_FLAGS)aflags, &fieldInfo, - lclTyp); + // Handled above break; default: diff --git a/src/tests/Loader/classloader/TypeInitialization/CctorsWithSideEffects/CctorForWrite2.cs b/src/tests/Loader/classloader/TypeInitialization/CctorsWithSideEffects/CctorForWrite2.cs new file mode 100644 index 0000000000000..cd10be102c872 --- /dev/null +++ b/src/tests/Loader/classloader/TypeInitialization/CctorsWithSideEffects/CctorForWrite2.cs @@ -0,0 +1,47 @@ +// Licensed to the .NET Foundation under one or more agreements. +// The .NET Foundation licenses this file to you under the MIT license. + +// Showed a JIT importer bug where a tree would be constructed for +// call / stsfld and then the type initialization call inserted before +// the entire tree. The call needs to happen before type initialization. + +using System; +using System.Runtime.CompilerServices; + +public class CorrectException : Exception +{ +} + +public class CCC +{ + public static int Main() => ClassCallingCctor.Test(); +} + +public class ClassCallingCctor +{ + [MethodImpl(MethodImplOptions.NoInlining)] + static int Call() => throw new CorrectException(); + + [MethodImpl(MethodImplOptions.NoInlining)] + internal static int Test() + { + try + { + ClassWithCctor.StaticField = Call(); + } + catch (CorrectException) + { + return 100; + } + catch + { + } + return 1; + } +} + +public class ClassWithCctor +{ + public static int StaticField; + static ClassWithCctor() => throw new Exception(); +} diff --git a/src/tests/Loader/classloader/TypeInitialization/CctorsWithSideEffects/CctorForWrite2.csproj b/src/tests/Loader/classloader/TypeInitialization/CctorsWithSideEffects/CctorForWrite2.csproj new file mode 100644 index 0000000000000..6946bed81bfd5 --- /dev/null +++ b/src/tests/Loader/classloader/TypeInitialization/CctorsWithSideEffects/CctorForWrite2.csproj @@ -0,0 +1,9 @@ + + + Exe + True + + + + + diff --git a/src/tests/issues.targets b/src/tests/issues.targets index a362cbd6b8650..68a34b15cdc1d 100644 --- a/src/tests/issues.targets +++ b/src/tests/issues.targets @@ -886,9 +886,6 @@ https://github.com/dotnet/runtime/issues/81103 - - https://github.com/dotnet/runtime/issues/84007 -