From 6fb5ef73e123dc411e24bef9623a4583633053e0 Mon Sep 17 00:00:00 2001 From: SingleAccretion <62474226+SingleAccretion@users.noreply.github.com> Date: Mon, 4 Oct 2021 18:14:46 +0300 Subject: [PATCH] Stop retyping RHSes to byrefs on type mismatch (#58297) * Stop retyping RHSes to byrefs on type mismatch This is an interesting piece of code. Using it, it is possible to give TYP_BYREF to any node that would ordinarily have TYP_I_IMPL. In particular, I encountered it in my GT_CAST work: we have tests in the SPMI collections with CAST(byref <- long <- int) in them. Ordinarily such a tree would assert in VN, but in that case optimizations were off and so everything went smoothly. It is possible to have TYP_BYREF UMOD/MUL_OVF_UN and similar with this code. Naturally, even though this is legal IL, the compiler does not like it and fails with asserts in multiple places. So, just delete the retyping. The cited assert either no longer exists (though I had a look at the legacy codegen and could not find it there...) or got reworked at some point. There are GC Info diffs with this change. They consist of cases when a constant zero node was retyped to TYP_BYREF and thus its register's liveness was recorded. There are few code size diffs with this change. It does pessimize VN a little because we will now have more cases where a cast VN is added for an assignment, but VN is already very pessimistic for byrefs, so it seems acceptable. * Add a test covering the discovered problem VN-based copy propagation checks the compatibility of types using the type of node at use and the RHS of the new local's def. Thus, when we retype a TYP_I_IMPL RHS to TYP_BYREF, we risk substituting a TYP_BYREF local with a TYP_I_IMPL one, dangerously shortening the lifetime of the managed pointer. --- src/coreclr/jit/importer.cpp | 9 - .../Coverage/copy_prop_byref_to_native_int.il | 155 ++++++++++++++++++ .../copy_prop_byref_to_native_int.ilproj | 12 ++ 3 files changed, 167 insertions(+), 9 deletions(-) create mode 100644 src/tests/JIT/Methodical/Coverage/copy_prop_byref_to_native_int.il create mode 100644 src/tests/JIT/Methodical/Coverage/copy_prop_byref_to_native_int.ilproj diff --git a/src/coreclr/jit/importer.cpp b/src/coreclr/jit/importer.cpp index 512e0eeb5bfc5..5e852e059208c 100644 --- a/src/coreclr/jit/importer.cpp +++ b/src/coreclr/jit/importer.cpp @@ -12064,15 +12064,6 @@ void Compiler::impImportBlockCode(BasicBlock* block) } else { - // The code generator generates GC tracking information - // based on the RHS of the assignment. Later the LHS (which is - // is a BYREF) gets used and the emitter checks that that variable - // is being tracked. It is not (since the RHS was an int and did - // not need tracking). To keep this assert happy, we change the RHS - if (lclTyp == TYP_BYREF && !varTypeIsGC(op1->gtType)) - { - op1->gtType = TYP_BYREF; - } op1 = gtNewAssignNode(op2, op1); } diff --git a/src/tests/JIT/Methodical/Coverage/copy_prop_byref_to_native_int.il b/src/tests/JIT/Methodical/Coverage/copy_prop_byref_to_native_int.il new file mode 100644 index 0000000000000..412a725815497 --- /dev/null +++ b/src/tests/JIT/Methodical/Coverage/copy_prop_byref_to_native_int.il @@ -0,0 +1,155 @@ +// Licensed to the .NET Foundation under one or more agreements. +// The .NET Foundation licenses this file to you under the MIT license. + +.assembly extern System.Console { auto } +.assembly extern System.Runtime { auto } + +.assembly CopyPropByrefToNativeInt { } + +.typedef [System.Runtime]System.WeakReference as WeakRef +.typedef [System.Runtime]System.GC as GC + +.class Program extends [System.Runtime]System.Object +{ + .field private static class WeakRef s_weakArrRef + .field private static bool s_arrIsAlive + + .method private static int32 Main() cil managed + { + .entrypoint + .locals ( [0] int32 result, [1] int32[] arr ) + + ldc.i4 1 + ldc.i4 1 // The array will be pinned. + call !!0[] GC::AllocateArray(int32, bool) + stloc arr + + ldloc arr + newobj instance void WeakRef::.ctor(object) + stsfld class WeakRef Program::s_weakArrRef + + ldc.i4 1 + stsfld bool Program::s_arrIsAlive + + ldloc arr + ldc.i4 0 + ldelema int32 + dup + ldc.i4 1 + call unsigned int8& Program::Problem(unsigned int8&, native int, int32) + brtrue SKIP + + // Unreachable, exists to not have "arr"'s liveness extended. + call unsigned int8& Program::SideEffect() + pop + + SKIP: + ldsfld bool Program::s_arrIsAlive + brtrue SUCCESS + + ldc.i4 101 + ret + + SUCCESS: + ldc.i4 100 + ret + } + + .method private static unsigned int8& Problem(unsigned int8& byrefAddrArg, native int nintAddr, int32 notZeroArg) cil managed noinlining + { + .locals ( [0] int32 i, [1] native int nintAddrCopy, [2] unsigned int8& byrefAddr ) + + ldarg nintAddr + stloc byrefAddr + + // Before this point, array is kept alive by "byrefAddrArg". + ldarg byrefAddrArg + call void Program::Use(unsigned int8&) + // Now the array should be kept alive by "byrefAddr". + + ldc.i4 0 + stloc i + LOOP: + ldloc i + ldc.i4 1 + add + stloc i + + call void Program::TryCollectArr() + + ldloc i + ldc.i4 10 + ble LOOP + + ldarg nintAddr + stloc nintAddrCopy + + ldarg notZeroArg + ldc.i4 0 + bne.un SKIP + + // This path is unreachable and only exists so + // that local copy propagation does not substitute + // "nintAddrCopy" with "nintAddr" too early. We + // want the fact they're equal to be discovered in VN. + call unsigned int8& Program::SideEffect() + ret + + SKIP: + // The below sequence is meant to reduce to just "ldloc byrefAddr; ret;" + // It exists because for copy propagation to perform the (wrong) substituion + // of "byrefAddr" with "nintAddr", we need to make it seem that "nintAddr" + // is live at the point of substition (#Use), so we fake it up with some + // instructions that VN knows will evaluate to nothing, but it'll take + // until constant VN-based propagation for that fact to be acted on. + + ldarg nintAddr + ldloc nintAddrCopy + ceq + ldc.i4 0 + ceq + conv.u + + ldloc byrefAddr // #Use + ldarg nintAddr + ldloc nintAddrCopy + ceq + ldc.i4 0 + ceq + conv.u + add + + add + ret + } + + .method private static void TryCollectArr() cil managed noinlining + { + .locals ( [0] int32 arrValue ) + + ldc.i4 2 + ldc.i4 1 // GCCollectionMode.Forced + call void GC::Collect(int32, valuetype [System.Runtime]System.GCCollectionMode) + call void GC::WaitForPendingFinalizers() + + ldsfld class WeakRef Program::s_weakArrRef + callvirt instance bool WeakRef::get_IsAlive() + ldsfld bool Program::s_arrIsAlive + and + stsfld bool Program::s_arrIsAlive + + ret + } + + .method private static unsigned int8& SideEffect() cil managed noinlining + { + ldc.i4 0 + conv.u + ret + } + + .method private static void Use(unsigned int8& byref) cil managed noinlining + { + ret + } +} diff --git a/src/tests/JIT/Methodical/Coverage/copy_prop_byref_to_native_int.ilproj b/src/tests/JIT/Methodical/Coverage/copy_prop_byref_to_native_int.ilproj new file mode 100644 index 0000000000000..e7c67cc80e853 --- /dev/null +++ b/src/tests/JIT/Methodical/Coverage/copy_prop_byref_to_native_int.ilproj @@ -0,0 +1,12 @@ + + + Exe + + + None + True + + + + +