From 2dd930129c1410bd7ee43e2dc549af435af51515 Mon Sep 17 00:00:00 2001 From: Sergey Andreenko Date: Tue, 3 Dec 2019 10:12:39 -0800 Subject: [PATCH] Fix `genFreeLclFrame` for arm32 when using CIL jmp. (#369) * Add a repro test. * Fix `genJmpCallArgMask` for multireg arguments. * Start actually using result of `genJmpCallArgMask`. * GC * Add a comment about `intRegState.rsCalleeRegArgMaskLiveIn`. * always use `REG_R12` as our temp reg.` Only one statement can be true: 1) There are cases where R12 can't be used and these cases were missed in the previous version, because we never excluded it from `grabMask`. That means the previous version had a bug; 2) R12 is always available and can always be used as a temp here. * GC. * add more info about the test failure. * Update the header. --- src/coreclr/src/jit/codegen.h | 3 +- src/coreclr/src/jit/codegencommon.cpp | 55 ++---- src/coreclr/src/jit/target.h | 8 - .../JitBlue/GitHub_27678/GitHub_27678.il | 215 +++++++++++++++++++++ .../JitBlue/GitHub_27678/GitHub_27678.ilproj | 12 ++ 5 files changed, 244 insertions(+), 49 deletions(-) create mode 100644 src/coreclr/tests/src/JIT/Regression/JitBlue/GitHub_27678/GitHub_27678.il create mode 100644 src/coreclr/tests/src/JIT/Regression/JitBlue/GitHub_27678/GitHub_27678.ilproj diff --git a/src/coreclr/src/jit/codegen.h b/src/coreclr/src/jit/codegen.h index 13ee79c..7cd039c 100644 --- a/src/coreclr/src/jit/codegen.h +++ b/src/coreclr/src/jit/codegen.h @@ -334,8 +334,7 @@ protected: regMaskTP genJmpCallArgMask(); void genFreeLclFrame(unsigned frameSize, - /* IN OUT */ bool* pUnwindStarted, - bool jmpEpilog); + /* IN OUT */ bool* pUnwindStarted); void genMov32RelocatableDisplacement(BasicBlock* block, regNumber reg); void genMov32RelocatableDataLabel(unsigned value, regNumber reg); diff --git a/src/coreclr/src/jit/codegencommon.cpp b/src/coreclr/src/jit/codegencommon.cpp index dbef1fe..7eb808f 100644 --- a/src/coreclr/src/jit/codegencommon.cpp +++ b/src/coreclr/src/jit/codegencommon.cpp @@ -5422,36 +5422,18 @@ void CodeGen::genPopFltRegs(regMaskTP regMask) GetEmitter()->emitIns_R_I(INS_vpop, EA_8BYTE, lowReg, slots / 2); } -/*----------------------------------------------------------------------------- - * - * If we have a jmp call, then the argument registers cannot be used in the - * epilog. So return the current call's argument registers as the argument - * registers for the jmp call. - */ -regMaskTP CodeGen::genJmpCallArgMask() -{ - assert(compiler->compGeneratingEpilog); - - regMaskTP argMask = RBM_NONE; - for (unsigned varNum = 0; varNum < compiler->info.compArgsCount; ++varNum) - { - const LclVarDsc& desc = compiler->lvaTable[varNum]; - if (desc.lvIsRegArg) - { - argMask |= genRegMask(desc.GetArgReg()); - } - } - return argMask; -} - -/*----------------------------------------------------------------------------- - * - * Free the local stack frame: add to SP. - * If epilog unwind hasn't been started, and we generate code, we start unwind - * and set *pUnwindStarted = true. - */ - -void CodeGen::genFreeLclFrame(unsigned frameSize, /* IN OUT */ bool* pUnwindStarted, bool jmpEpilog) +//------------------------------------------------------------------------ +// genFreeLclFrame: free the local stack frame by adding `frameSize` to SP. +// +// Arguments: +// frameSize - the frame size to free; +// pUnwindStarted - was epilog unwind started or not. +// +// Notes: +// If epilog unwind hasn't been started, and we generate code, we start unwind +// and set* pUnwindStarted = true. +// +void CodeGen::genFreeLclFrame(unsigned frameSize, /* IN OUT */ bool* pUnwindStarted) { assert(compiler->compGeneratingEpilog); @@ -5481,13 +5463,8 @@ void CodeGen::genFreeLclFrame(unsigned frameSize, /* IN OUT */ bool* pUnwindStar } else { - regMaskTP grabMask = RBM_INT_CALLEE_TRASH; - if (jmpEpilog) - { - // Do not use argument registers as scratch registers in the jmp epilog. - grabMask &= ~genJmpCallArgMask(); - } - regNumber tmpReg = REG_TMP_0; + // R12 doesn't hold arguments or return values, so can be used as temp. + regNumber tmpReg = REG_R12; instGen_Set_Reg_To_Imm(EA_PTRSIZE, tmpReg, frameSize); if (*pUnwindStarted) { @@ -7950,7 +7927,7 @@ void CodeGen::genFnEpilog(BasicBlock* block) genStackAllocRegisterMask(compiler->compLclFrameSize, regSet.rsGetModifiedRegsMask() & RBM_FLT_CALLEE_SAVED) == RBM_NONE) { - genFreeLclFrame(compiler->compLclFrameSize, &unwindStarted, jmpEpilog); + genFreeLclFrame(compiler->compLclFrameSize, &unwindStarted); } if (!unwindStarted) @@ -8794,7 +8771,7 @@ void CodeGen::genFuncletEpilog() if (maskStackAlloc == RBM_NONE) { - genFreeLclFrame(genFuncletInfo.fiSpDelta, &unwindStarted, false); + genFreeLclFrame(genFuncletInfo.fiSpDelta, &unwindStarted); } if (!unwindStarted) diff --git a/src/coreclr/src/jit/target.h b/src/coreclr/src/jit/target.h index 0c224b6..838fc81 100644 --- a/src/coreclr/src/jit/target.h +++ b/src/coreclr/src/jit/target.h @@ -337,8 +337,6 @@ typedef unsigned char regNumberSmall; #define CALLEE_SAVED_REG_MAXSZ (CNT_CALLEE_SAVED*REGSIZE_BYTES) // EBX,ESI,EDI,EBP - #define REG_TMP_0 REG_EAX - #define REG_LNGARG_LO REG_EAX #define RBM_LNGARG_LO RBM_EAX #define REG_LNGARG_HI REG_EDX @@ -660,8 +658,6 @@ typedef unsigned char regNumberSmall; #define CALLEE_SAVED_REG_MAXSZ (CNT_CALLEE_SAVED*REGSIZE_BYTES) #define CALLEE_SAVED_FLOAT_MAXSZ (CNT_CALLEE_SAVED_FLOAT*16) - #define REG_TMP_0 REG_EAX - // register to hold shift amount #define REG_SHIFT REG_ECX #define RBM_SHIFT RBM_ECX @@ -980,8 +976,6 @@ typedef unsigned char regNumberSmall; #define CALLEE_SAVED_REG_MAXSZ (CNT_CALLEE_SAVED*REGSIZE_BYTES) #define CALLEE_SAVED_FLOAT_MAXSZ (CNT_CALLEE_SAVED_FLOAT*sizeof(float)) - #define REG_TMP_0 REG_R3 - // Temporary registers used for the GS cookie check. #define REG_GSCOOKIE_TMP_0 REG_R12 #define REG_GSCOOKIE_TMP_1 REG_LR @@ -1297,8 +1291,6 @@ typedef unsigned char regNumberSmall; #define CALLEE_SAVED_REG_MAXSZ (CNT_CALLEE_SAVED * REGSIZE_BYTES) #define CALLEE_SAVED_FLOAT_MAXSZ (CNT_CALLEE_SAVED_FLOAT * FPSAVE_REGSIZE_BYTES) - #define REG_TMP_0 REG_R9 - // Temporary registers used for the GS cookie check. #define REG_GSCOOKIE_TMP_0 REG_R9 #define REG_GSCOOKIE_TMP_1 REG_R10 diff --git a/src/coreclr/tests/src/JIT/Regression/JitBlue/GitHub_27678/GitHub_27678.il b/src/coreclr/tests/src/JIT/Regression/JitBlue/GitHub_27678/GitHub_27678.il new file mode 100644 index 0000000..93ccc35 --- /dev/null +++ b/src/coreclr/tests/src/JIT/Regression/JitBlue/GitHub_27678/GitHub_27678.il @@ -0,0 +1,215 @@ +// 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. + +// Do an IL jmp instruction in a method with a big local frame, +// where SP restore needs a free register to free local frame (sp += frameSize). +// In the past codegen was always using R3 as a temp without checking that it could +// be occupied by an argument value. + +// asm example for arm32 windows: +// +// movw r10, 0x1040 // [V01 arg0] <- restore 8 byte V01 arg (r2, r2) from the stack. +// str r2, [sp+r10] // [V01 arg0] +// movw r10, 0x1044 // [V01 arg0+0x04] +// str r3, [sp+r10] // [V01 arg0+0x04] +// __epilog: +// movw r3, frameSize +// add sp, r3 <- corrupt r3 value to free local frame. +// pop {r10,lr} +// bx jumpAddress + +.assembly extern System.Runtime {} +.assembly extern System.Diagnostics.Debug {} +.assembly GitHub_27678 {} + +.class private sequential ansi sealed beforefieldinit AStruct + extends [System.Runtime]System.ValueType +{ + .field public int64 a + .field public int64 b + .field public int64 c + .field public int64 d +} // end of class AStruct + +.class private sequential ansi sealed beforefieldinit BStruct + extends [System.Runtime]System.ValueType +{ + .field public valuetype AStruct a + .field public valuetype AStruct b + .field public valuetype AStruct c + .field public valuetype AStruct d +} // end of class BStruct + +.class private sequential ansi sealed beforefieldinit CStruct + extends [System.Runtime]System.ValueType +{ + .field public valuetype BStruct a + .field public valuetype BStruct b + .field public valuetype BStruct c + .field public valuetype BStruct d +} // end of class CStruct + +.class private sequential ansi sealed beforefieldinit DStruct + extends [System.Runtime]System.ValueType +{ + .field public valuetype CStruct a + .field public valuetype CStruct b + .field public valuetype CStruct c + .field public valuetype CStruct d +} // end of class DStruct + +.class private sequential ansi sealed beforefieldinit EStruct + extends [System.Runtime]System.ValueType +{ + .field public valuetype DStruct a + .field public valuetype DStruct b + .field public valuetype DStruct c + .field public valuetype DStruct d +} // end of class EStruct + +.class private sequential ansi sealed beforefieldinit FStruct + extends [System.Runtime]System.ValueType +{ + .field public valuetype EStruct a + .field public valuetype EStruct b + .field public valuetype EStruct c + .field public valuetype EStruct d +} // end of class FStruct + +.class private auto ansi beforefieldinit GitHub_27678 + extends [System.Runtime]System.Object +{ + .method private hidebysig static bool JumpTarget(int64 a, + int64 b, + valuetype FStruct c) cil managed noinlining + { + // Code size 108 (0x6c) + .maxstack 2 + IL_0000: ldarg.0 + IL_0001: ldc.i4.1 + IL_0002: conv.i8 + IL_0003: ceq + IL_0005: call void [System.Diagnostics.Debug]System.Diagnostics.Debug::Assert(bool) + IL_000a: ldarg.1 + IL_000b: ldc.i4.2 + IL_000c: conv.i8 + IL_000d: ceq + IL_000f: call void [System.Diagnostics.Debug]System.Diagnostics.Debug::Assert(bool) + IL_0014: ldarg.2 + IL_0015: ldfld valuetype EStruct FStruct::a + IL_001a: ldfld valuetype DStruct EStruct::a + IL_001f: ldfld valuetype CStruct DStruct::a + IL_0024: ldfld valuetype BStruct CStruct::a + IL_0029: ldfld valuetype AStruct BStruct::a + IL_002e: ldfld int64 AStruct::a + IL_0033: ldc.i4.3 + IL_0034: conv.i8 + IL_0035: ceq + IL_0037: call void [System.Diagnostics.Debug]System.Diagnostics.Debug::Assert(bool) + IL_003c: ldarg.0 + IL_003d: ldc.i4.1 + IL_003e: conv.i8 + IL_003f: bne.un.s IL_006a + + IL_0041: ldarg.1 + IL_0042: ldc.i4.2 + IL_0043: conv.i8 + IL_0044: bne.un.s IL_006a + + IL_0046: ldarg.2 + IL_0047: ldfld valuetype EStruct FStruct::a + IL_004c: ldfld valuetype DStruct EStruct::a + IL_0051: ldfld valuetype CStruct DStruct::a + IL_0056: ldfld valuetype BStruct CStruct::a + IL_005b: ldfld valuetype AStruct BStruct::a + IL_0060: ldfld int64 AStruct::a + IL_0065: ldc.i4.3 + IL_0066: conv.i8 + IL_0067: ceq + IL_0069: ret + + IL_006a: ldc.i4.0 + IL_006b: ret + } // end of method GitHub_27678::JumpTarget + + .method private hidebysig static bool Test(int64 a, + int64 b, + valuetype FStruct c) cil managed noinlining + { + // Code size 63 (0x3f) + .maxstack 3 + .locals init ([0] valuetype FStruct d) + IL_0000: ldarg.2 + IL_0001: stloc.0 + IL_0002: ldloca.s d + IL_0004: ldloc.0 + IL_0005: ldfld valuetype EStruct FStruct::a + IL_000a: stfld valuetype EStruct FStruct::b + IL_000f: ldloca.s d + IL_0011: ldloc.0 + IL_0012: ldfld valuetype EStruct FStruct::d + IL_0017: stfld valuetype EStruct FStruct::c + IL_001c: ldloca.s d + IL_001e: ldloc.0 + IL_001f: ldfld valuetype EStruct FStruct::b + IL_0024: stfld valuetype EStruct FStruct::a + IL_0029: ldarga.s c + IL_002b: ldloc.0 + IL_002c: ldfld valuetype EStruct FStruct::a + IL_0031: stfld valuetype EStruct FStruct::a + + IL_0039: jmp bool GitHub_27678::JumpTarget(int64, + int64, + valuetype FStruct) + IL_003e: ret + } // end of method GitHub_27678::Test + + .method public hidebysig static int32 Main() cil managed + { + .entrypoint + // Code size 62 (0x3e) + .maxstack 3 + .locals init ([0] valuetype FStruct a, + [1] bool r) + IL_0000: ldloca.s a + IL_0002: initobj FStruct + IL_0008: ldloca.s a + IL_000a: ldflda valuetype EStruct FStruct::a + IL_000f: ldflda valuetype DStruct EStruct::a + IL_0014: ldflda valuetype CStruct DStruct::a + IL_0019: ldflda valuetype BStruct CStruct::a + IL_001e: ldflda valuetype AStruct BStruct::a + IL_0023: ldc.i4.3 + IL_0024: conv.i8 + IL_0025: stfld int64 AStruct::a + IL_002a: ldc.i4.1 + IL_002b: conv.i8 + IL_002c: ldc.i4.2 + IL_002d: conv.i8 + IL_002e: ldloc.0 + IL_002f: call bool GitHub_27678::Test(int64, + int64, + valuetype FStruct) + IL_0034: stloc.1 + IL_0035: ldloc.1 + IL_0036: brfalse.s IL_003b + + IL_0038: ldc.i4.s 100 + IL_003a: ret + + IL_003b: ldc.i4.s 101 + IL_003d: ret + } // end of method GitHub_27678::Main + + .method public hidebysig specialname rtspecialname + instance void .ctor() cil managed + { + // Code size 7 (0x7) + .maxstack 8 + IL_0000: ldarg.0 + IL_0001: call instance void [System.Runtime]System.Object::.ctor() + IL_0006: ret + } // end of method GitHub_27678::.ctor + +} // end of class GitHub_27678 diff --git a/src/coreclr/tests/src/JIT/Regression/JitBlue/GitHub_27678/GitHub_27678.ilproj b/src/coreclr/tests/src/JIT/Regression/JitBlue/GitHub_27678/GitHub_27678.ilproj new file mode 100644 index 0000000..e7c67cc --- /dev/null +++ b/src/coreclr/tests/src/JIT/Regression/JitBlue/GitHub_27678/GitHub_27678.ilproj @@ -0,0 +1,12 @@ + + + Exe + + + None + True + + + + + -- 2.7.4