Skip to content

Commit

Permalink
Fix type initialization ordering under R2R (#84275)
Browse files Browse the repository at this point in the history
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
  • Loading branch information
markples committed Apr 21, 2023
1 parent 2032a59 commit fa60eb7
Show file tree
Hide file tree
Showing 5 changed files with 148 additions and 22 deletions.
3 changes: 2 additions & 1 deletion src/coreclr/jit/compiler.h
Original file line number Diff line number Diff line change
Expand Up @@ -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);

Expand Down
108 changes: 90 additions & 18 deletions src/coreclr/jit/importer.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down Expand Up @@ -4151,6 +4167,7 @@ GenTree* Compiler::impImportStaticFieldAccess(CORINFO_RESOLVED_TOKEN* pResolvedT
outerFldSeq = nullptr;
}

bool isHoistable = false;
bool isStaticReadOnlyInitedRef = false;
unsigned typeIndex = 0;
GenTree* op1;
Expand Down Expand Up @@ -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));
}
Expand All @@ -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;
}

Expand All @@ -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));
Expand All @@ -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();
Expand All @@ -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);
Expand Down Expand Up @@ -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<size_t>(pResolvedToken->hField));
if (pFieldInfo->fieldFlags & CORINFO_FLG_FIELD_INITCLASS)
{
Expand Down Expand Up @@ -4354,6 +4381,10 @@ GenTree* Compiler::impImportStaticFieldAccess(CORINFO_RESOLVED_TOKEN* pResolvedT
}
}

if (pIsHoistable)
{
*pIsHoistable = isHoistable;
}
return op1;
}

Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -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:
Expand Down Expand Up @@ -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:
Expand Down
Original file line number Diff line number Diff line change
@@ -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<object>.Test();
}

public class ClassCallingCctor<T>
{
[MethodImpl(MethodImplOptions.NoInlining)]
static int Call() => throw new CorrectException();

[MethodImpl(MethodImplOptions.NoInlining)]
internal static int Test()
{
try
{
ClassWithCctor<T>.StaticField = Call();
}
catch (CorrectException)
{
return 100;
}
catch
{
}
return 1;
}
}

public class ClassWithCctor<T>
{
public static int StaticField;
static ClassWithCctor() => throw new Exception();
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
<Project Sdk="Microsoft.NET.Sdk">
<PropertyGroup>
<OutputType>Exe</OutputType>
<Optimize>True</Optimize>
</PropertyGroup>
<ItemGroup>
<Compile Include="$(MSBuildProjectName).cs" />
</ItemGroup>
</Project>
3 changes: 0 additions & 3 deletions src/tests/issues.targets
Original file line number Diff line number Diff line change
Expand Up @@ -886,9 +886,6 @@
<ExcludeList Include="$(XunitTestBinBase)/JIT/opt/ObjectStackAllocation/ObjectStackAllocationTests/*">
<Issue>https://github.com/dotnet/runtime/issues/81103</Issue>
</ExcludeList>
<ExcludeList Include="$(XunitTestBinBase)/Loader/classloader/TypeInitialization/CctorsWithSideEffects/CctorForWrite/*">
<Issue>https://github.com/dotnet/runtime/issues/84007</Issue>
</ExcludeList>
</ItemGroup>

<!-- Crossgen2 x86 specific excludes -->
Expand Down

0 comments on commit fa60eb7

Please sign in to comment.