From: Eugene Rozenfeld Date: Thu, 2 Jun 2016 00:38:53 +0000 (-0700) Subject: JIT_TailCall helper has an implicit assumption that all tail call arguments live X-Git-Tag: accepted/tizen/base/20180629.140029~4452^2 X-Git-Url: http://review.tizen.org/git/?a=commitdiff_plain;h=3a97871a149dc116c80710ff51ecc85f62763909;p=platform%2Fupstream%2Fcoreclr.git JIT_TailCall helper has an implicit assumption that all tail call arguments live on the caller's frame. If an argument lives on the caller caller's frame, it may get overwritten if that frame is reused for the tail call. Therefore, we should always copy struct parameters if they are passed as arguments to a tail call. The simple il regression test has a scenario similar to that of the F# repro in #5164. Closes #5164. --- diff --git a/src/jit/morph.cpp b/src/jit/morph.cpp index e09e96b..1cd9eab 100644 --- a/src/jit/morph.cpp +++ b/src/jit/morph.cpp @@ -4875,7 +4875,11 @@ Compiler::fgMakeOutgoingStructArgCopy(GenTreeCall* call, if (lvaIsImplicitByRefLocal(varNum)) { LclVarDsc* varDsc = &lvaTable[varNum]; - if (varDsc->lvRefCnt == 1 && !fgMightHaveLoop()) + // JIT_TailCall helper has an implicit assumption that all tail call arguments live + // on the caller's frame. If an argument lives on the caller caller's frame, it may get + // overwritten if that frame is reused for the tail call. Therefore, we should always copy + // struct parameters if they are passed as arguments to a tail call. + if (!call->IsTailCallViaHelper() && (varDsc->lvRefCnt == 1) && !fgMightHaveLoop()) { varDsc->lvRefCnt = 0; args->gtOp.gtOp1 = lcl; @@ -4883,13 +4887,8 @@ Compiler::fgMakeOutgoingStructArgCopy(GenTreeCall* call, fp->node = lcl; JITDUMP("did not have to make outgoing copy for V%2d", varNum); - varDsc->lvRefCnt = 0; return; } - else - { - varDsc->lvRefCnt = 0; - } } } } diff --git a/tests/src/JIT/Regression/JitBlue/GitHub_5164/GitHub_5164.il b/tests/src/JIT/Regression/JitBlue/GitHub_5164/GitHub_5164.il new file mode 100644 index 0000000..d2cb5bd --- /dev/null +++ b/tests/src/JIT/Regression/JitBlue/GitHub_5164/GitHub_5164.il @@ -0,0 +1,150 @@ +// Licensed to the .NET Foundation under one or more agreements. +// The .NET Foundation licenses this file to you under the MIT license. +// See the LICENSE file in the project root for more information. + +// The tail call helper JIT_TailCall has an implicit assumption that all +// arguments of the tail call are allocated on the caller's frame. +// In this test A tail-calls B and B tail-calls A: Main --> A --> B --> A --> B --> A. +// In the last B --> A tail call the frame from the preceding A --> B tail call is resused. +// It's big enough because of the first B --> A tail call. +// The bug was that the code for B wasn't copying the incoming struct parameter to its frame. +// The struct lived on the caller's frame and when it was reused for the B --> A tail call, +// the struct was overwritten. + +.assembly extern mscorlib { } + +.assembly GitHub_5164 { } + +.class public sequential ansi sealed beforefieldinit LargeStruct + extends [mscorlib]System.ValueType +{ + .field public int64 l1 + .field public int64 l2 + .method public hidebysig specialname rtspecialname + instance void .ctor(int64 l1, + int64 l2) cil managed + { + // Code size 15 (0xf) + .maxstack 8 + IL_0000: ldarg.0 + IL_0001: ldarg.1 + IL_0002: stfld int64 LargeStruct::l1 + IL_0007: ldarg.0 + IL_0008: ldarg.2 + IL_0009: stfld int64 LargeStruct::l2 + IL_000e: ret + } // end of method LargeStruct::.ctor + +} // end of class LargeStruct + +.class private auto ansi beforefieldinit Test + extends [mscorlib]System.Object +{ + .method public hidebysig static int32 Main() cil managed + { + .entrypoint + // Code size 48 (0x30) + .maxstack 3 + .locals init (valuetype LargeStruct V_0, + valuetype LargeStruct V_1, + valuetype LargeStruct V_2) + IL_0000: ldloca.s V_0 + IL_0002: ldc.i4.1 + IL_0003: conv.i8 + IL_0004: ldc.i4.2 + IL_0005: conv.i8 + IL_0006: call instance void LargeStruct::.ctor(int64, + int64) + IL_000b: ldloca.s V_1 + IL_000d: ldc.i4.3 + IL_000e: conv.i8 + IL_000f: ldc.i4.4 + IL_0010: conv.i8 + IL_0011: call instance void LargeStruct::.ctor(int64, + int64) + IL_0016: ldc.i4.0 + IL_0017: ldloc.0 + IL_0018: ldloc.1 + IL_0019: call valuetype LargeStruct Test::A(int32, + valuetype LargeStruct, + valuetype LargeStruct) + IL_001e: stloc.2 + IL_001f: ldloca.s V_2 + IL_0021: ldfld int64 LargeStruct::l1 + IL_0026: ldloca.s V_2 + IL_0028: ldfld int64 LargeStruct::l2 + IL_002d: add + IL_002e: conv.i4 + IL_002f: ret + } // end of method Test::Main + + .method public hidebysig static valuetype LargeStruct + A(int32 count, + valuetype LargeStruct s1, + valuetype LargeStruct s2) cil managed noinlining + { + // Code size 58 (0x3a) + .maxstack 4 + .locals init (valuetype LargeStruct V_0) + IL_0000: ldloca.s V_0 + IL_0002: ldarga.s s1 + IL_0004: ldfld int64 LargeStruct::l1 + IL_0009: ldarga.s s2 + IL_000b: ldfld int64 LargeStruct::l1 + IL_0010: add + IL_0011: ldarga.s s1 + IL_0013: ldfld int64 LargeStruct::l2 + IL_0018: ldarga.s s2 + IL_001a: ldfld int64 LargeStruct::l2 + IL_001f: add + IL_0020: call instance void LargeStruct::.ctor(int64, + int64) + IL_0025: ldarg.0 + IL_0026: ldc.i4.3 + IL_0027: bge.s IL_0038 + + IL_0029: ldarg.0 + IL_002a: ldc.i4.1 + IL_002b: add + IL_002c: dup + IL_002d: starg.s count + IL_002f: ldloc.0 + IL_0030: tail. + IL_0032: call valuetype LargeStruct Test::B(int32, + valuetype LargeStruct) + IL_0037: ret + + IL_0038: ldloc.0 + IL_0039: ret + } // end of method Test::A + + .method public hidebysig static valuetype LargeStruct + B(int32 count, + valuetype LargeStruct s) cil managed noinlining + { + // Code size 28 (0x1c) + .maxstack 3 + .locals init (valuetype LargeStruct V_0) + IL_0000: ldloca.s V_0 + IL_0002: ldc.i4.1 + IL_0003: conv.i8 + IL_0004: ldc.i4.s 44 + IL_0006: conv.i8 + IL_0007: call instance void LargeStruct::.ctor(int64, + int64) + IL_000c: ldarg.0 + IL_000d: ldc.i4.1 + IL_000e: add + IL_000f: dup + IL_0010: starg.s count + IL_0012: ldloc.0 + IL_0013: ldarg.1 + IL_0014: tail. + IL_0016: call valuetype LargeStruct Test::A(int32, + valuetype LargeStruct, + valuetype LargeStruct) + IL_001b: ret + } // end of method Test::B + +} // end of class Test + diff --git a/tests/src/JIT/Regression/JitBlue/GitHub_5164/GitHub_5164.ilproj b/tests/src/JIT/Regression/JitBlue/GitHub_5164/GitHub_5164.ilproj new file mode 100644 index 0000000..b5d5ee9 --- /dev/null +++ b/tests/src/JIT/Regression/JitBlue/GitHub_5164/GitHub_5164.ilproj @@ -0,0 +1,44 @@ + + + + + Debug + AnyCPU + $(MSBuildProjectName) + 2.0 + {95DFC527-4DC1-495E-97D7-E94EE1F7140D} + Exe + Properties + 512 + {786C830F-07A1-408B-BD7F-6EE04809D6DB};{FAE04EC0-301F-11D3-BF4B-00C04F79EFBC} + $(ProgramFiles)\Common Files\microsoft shared\VSTT .0\UITestExtensionPackages + ..\..\ + 7a9bfb7d + + + + + + + + + False + + + + None + True + + + + + + + + + + + + + + diff --git a/tests/src/JIT/Regression/JitBlue/GitHub_5164/app.config b/tests/src/JIT/Regression/JitBlue/GitHub_5164/app.config new file mode 100644 index 0000000..8077c95 --- /dev/null +++ b/tests/src/JIT/Regression/JitBlue/GitHub_5164/app.config @@ -0,0 +1,27 @@ + + + + + + + + + + + + + + + + + + + + + + + + + + + \ No newline at end of file