From 4f161de987fa496da237840704260c1d001f5412 Mon Sep 17 00:00:00 2001 From: Jakob Botsch Nielsen Date: Wed, 21 Apr 2021 18:13:37 +0200 Subject: [PATCH 01/12] Poison address exposed user variables in debug code Fix #13072 --- src/coreclr/jit/codegen.h | 3 + src/coreclr/jit/codegencommon.cpp | 88 ++++++++++++++++++- src/coreclr/jit/codegenlinear.cpp | 7 ++ src/coreclr/jit/compiler.h | 6 ++ src/coreclr/jit/flowgraph.cpp | 3 +- .../src/System/Decimal.DecCalc.cs | 15 ---- 6 files changed, 105 insertions(+), 17 deletions(-) diff --git a/src/coreclr/jit/codegen.h b/src/coreclr/jit/codegen.h index 02971fbde2b8c..1cd8d6bef3c6e 100644 --- a/src/coreclr/jit/codegen.h +++ b/src/coreclr/jit/codegen.h @@ -348,6 +348,9 @@ class CodeGen final : public CodeGenInterface void genAllocLclFrame(unsigned frameSize, regNumber initReg, bool* pInitRegZeroed, regMaskTP maskArgRegsLiveIn); + void genSetRegsModifiedForPoisoning(); + void genPoisonFrame(regMaskTP bbRegLiveIn); + #if defined(TARGET_ARM) bool genInstrWithConstant( diff --git a/src/coreclr/jit/codegencommon.cpp b/src/coreclr/jit/codegencommon.cpp index e99f047c061a5..aa4be9c839714 100644 --- a/src/coreclr/jit/codegencommon.cpp +++ b/src/coreclr/jit/codegencommon.cpp @@ -4842,7 +4842,7 @@ void CodeGen::genCheckUseBlockInit() } } - // Record number of 4 byte slots that need zeroing. + // Record number of stack slots that need zeroing. genInitStkLclCnt = initStkLclCnt; // Decide if we will do block initialization in the prolog, or use @@ -6733,6 +6733,12 @@ void CodeGen::genFinalizeFrame() } #endif // TARGET_ARM + // When poisoning on ARM we need to use callee-preserved registers. + if (compiler->compShouldPoisonFrame()) + { + genSetRegsModifiedForPoisoning(); + } + #ifdef DEBUG if (verbose) { @@ -12528,3 +12534,83 @@ void CodeGenInterface::VariableLiveKeeper::dumpLvaVariableLiveRanges() const } #endif // DEBUG #endif // USING_VARIABLE_LIVE_RANGE + +void CodeGen::genSetRegsModifiedForPoisoning() +{ + // For ARM32 we need a callee-reserved register since the scratch registers may have args. +#if defined(TARGET_ARM) + regSet.rsSetRegsModified(RBM_R4); +#endif +} + +//----------------------------------------------------------------------------- +// genPoisonFrame: Generate code that places a recognizable value into address exposed variables. +// +// Remarks: +// This function emits code to poison address exposed non-zero-inited local variables. We expect this function +// to be called when emitting code for the scratch BB that comes right after the prolog. +// The variables are poisoned using 0xcccccccc. This value is stored in EAX/RAX on xarch and r/x0 on ARM, +// so we expect that this reg is not live. +void CodeGen::genPoisonFrame(regMaskTP regLiveIn) +{ +#if defined(TARGET_XARCH) + const regNumber immRegNum = REG_EAX; +#elif defined(TARGET_ARM) + const regNumber immRegNum = REG_R4; +#elif defined(TARGET_ARM64) + const regNumber immRegNum = REG_R9; +#else + return; +#endif + + assert(compiler->compShouldPoisonFrame()); + assert((regLiveIn & genRegMask(immRegNum)) == 0); + + // The first time we need to poison something we will initialize a register to the largest immediate cccccccc that we can fit. + bool hasPoisonImm = false; + for (unsigned varNum = 0; varNum < compiler->info.compLocalsCount; varNum++) + { + LclVarDsc* varDsc = compiler->lvaGetDesc(varNum); + if (varDsc->lvIsParam || varDsc->lvMustInit || !varDsc->lvAddrExposed) + { + continue; + } + + assert(varDsc->lvOnFrame); + + if (!hasPoisonImm) + { +#ifdef TARGET_64BIT + instGen_Set_Reg_To_Imm(EA_8BYTE, immRegNum, (ssize_t)0xcccccccccccccccc); +#else + instGen_Set_Reg_To_Imm(EA_4BYTE, immRegNum, (ssize_t)0xcccccccc); +#endif + hasPoisonImm = true; + } + + // For 64-bit we check if the local is 8-byte aligned. For 32-bit, we assume everything is always 4-byte aligned. +#ifdef TARGET_64BIT + bool fpBased; + int addr = compiler->lvaFrameAddress((int)varNum, &fpBased); +#else + int addr = 0; +#endif + int size = (int)compiler->lvaLclSize(varNum); + int end = addr + size; + for (int offs = addr; offs < end;) + { +#ifdef TARGET_64BIT + if ((offs & 7) == 0 && end - offs >= 8) + { + GetEmitter()->emitIns_S_R(ins_Store(TYP_LONG), EA_8BYTE, immRegNum, (int)varNum, offs - addr); + offs += 8; + continue; + } +#endif + + assert((offs & 3) == 0 && end - offs >= 4); + GetEmitter()->emitIns_S_R(ins_Store(TYP_INT), EA_4BYTE, immRegNum, (int)varNum, offs - addr); + offs += 4; + } + } +} diff --git a/src/coreclr/jit/codegenlinear.cpp b/src/coreclr/jit/codegenlinear.cpp index 796c128b98eac..8bf1f852ffd4d 100644 --- a/src/coreclr/jit/codegenlinear.cpp +++ b/src/coreclr/jit/codegenlinear.cpp @@ -395,6 +395,13 @@ void CodeGen::genCodeForBBlist() compiler->compCurStmt = nullptr; compiler->compCurLifeTree = nullptr; + // Emit poisoning into scratch BB that comes right after prolog. + // We cannot emit this code in the prolog as it might make the prolog too large. + if (compiler->compShouldPoisonFrame() && compiler->fgBBisScratch(block)) + { + genPoisonFrame(newLiveRegSet); + } + // Traverse the block in linear order, generating code for each node as we // as we encounter it. CLANG_FORMAT_COMMENT_ANCHOR; diff --git a/src/coreclr/jit/compiler.h b/src/coreclr/jit/compiler.h index fe3f987667d38..914e3424fa742 100644 --- a/src/coreclr/jit/compiler.h +++ b/src/coreclr/jit/compiler.h @@ -9828,6 +9828,12 @@ XXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXX return (info.compUnmanagedCallCountWithGCTransition > 0); } + // Returns true if address-exposed user variables should be poisoned with a recognizable value + bool compShouldPoisonFrame() + { + return !info.compInitMem && opts.MinOpts() && opts.compDbgCode; + } + #if defined(DEBUG) void compDispLocalVars(); diff --git a/src/coreclr/jit/flowgraph.cpp b/src/coreclr/jit/flowgraph.cpp index e7a281cdecaa1..ffe08f967bc95 100644 --- a/src/coreclr/jit/flowgraph.cpp +++ b/src/coreclr/jit/flowgraph.cpp @@ -2604,7 +2604,8 @@ void Compiler::fgAddInternal() // The backend requires a scratch BB into which it can safely insert a P/Invoke method prolog if one is // required. Create it here. - if (compMethodRequiresPInvokeFrame()) + // Similarly, for poisoning we also need a scratch BB. + if (compMethodRequiresPInvokeFrame() || compShouldPoisonFrame()) { fgEnsureFirstBBisScratch(); fgFirstBB->bbFlags |= BBF_DONT_REMOVE; diff --git a/src/libraries/System.Private.CoreLib/src/System/Decimal.DecCalc.cs b/src/libraries/System.Private.CoreLib/src/System/Decimal.DecCalc.cs index d56ce8ebb7fc7..a966b35b83c4a 100644 --- a/src/libraries/System.Private.CoreLib/src/System/Decimal.DecCalc.cs +++ b/src/libraries/System.Private.CoreLib/src/System/Decimal.DecCalc.cs @@ -153,13 +153,6 @@ private ulong Low64 1e80 }; - // Used to fill uninitialized stack variables with non-zero pattern in debug builds - [Conditional("DEBUG")] - private static unsafe void DebugPoison(ref T s) where T : unmanaged - { - MemoryMarshal.AsBytes(MemoryMarshal.CreateSpan(ref s, 1)).Fill(0xCD); - } - #region Decimal Math Helpers private static unsafe uint GetExponent(float f) @@ -990,7 +983,6 @@ internal static unsafe void DecAddSub(ref DecCalc d1, ref DecCalc d2, bool sign) // Have to scale by a bunch. Move the number to a buffer where it has room to grow as it's scaled. // Unsafe.SkipInit(out Buf24 bufNum); - DebugPoison(ref bufNum); bufNum.Low64 = low64; bufNum.Mid64 = tmp64; @@ -1339,7 +1331,6 @@ internal static unsafe void VarDecMul(ref DecCalc d1, ref DecCalc d2) ulong tmp; uint hiProd; Unsafe.SkipInit(out Buf24 bufProd); - DebugPoison(ref bufProd); if ((d1.High | d1.Mid) == 0) { @@ -1920,7 +1911,6 @@ internal static int GetHashCode(in decimal d) internal static unsafe void VarDecDiv(ref DecCalc d1, ref DecCalc d2) { Unsafe.SkipInit(out Buf12 bufQuo); - DebugPoison(ref bufQuo); uint power; int curScale; @@ -2021,7 +2011,6 @@ internal static unsafe void VarDecDiv(ref DecCalc d1, ref DecCalc d2) // Shift both dividend and divisor left by curScale. // Unsafe.SkipInit(out Buf16 bufRem); - DebugPoison(ref bufRem); bufRem.Low64 = d1.Low64 << curScale; bufRem.High64 = (d1.Mid + ((ulong)d1.High << 32)) >> (32 - curScale); @@ -2090,7 +2079,6 @@ internal static unsafe void VarDecDiv(ref DecCalc d1, ref DecCalc d2) // Start by finishing the shift left by curScale. // Unsafe.SkipInit(out Buf12 bufDivisor); - DebugPoison(ref bufDivisor); bufDivisor.Low64 = divisor; bufDivisor.U2 = (uint)((d2.Mid + ((ulong)d2.High << 32)) >> (32 - curScale)); @@ -2243,7 +2231,6 @@ internal static void VarDecMod(ref DecCalc d1, ref DecCalc d2) d1.uflags = d2.uflags; // Try to scale up dividend to match divisor. Unsafe.SkipInit(out Buf12 bufQuo); - DebugPoison(ref bufQuo); bufQuo.Low64 = d1.Low64; bufQuo.U2 = d1.High; @@ -2303,7 +2290,6 @@ private static unsafe void VarDecModFull(ref DecCalc d1, ref DecCalc d2, int sca int shift = BitOperations.LeadingZeroCount(tmp); Unsafe.SkipInit(out Buf28 b); - DebugPoison(ref b); b.Buf24.Low64 = d1.Low64 << shift; b.Buf24.Mid64 = (d1.Mid + ((ulong)d1.High << 32)) >> (32 - shift); @@ -2358,7 +2344,6 @@ private static unsafe void VarDecModFull(ref DecCalc d1, ref DecCalc d2, int sca else { Unsafe.SkipInit(out Buf12 bufDivisor); - DebugPoison(ref bufDivisor); bufDivisor.Low64 = d2.Low64 << shift; bufDivisor.U2 = (uint)((d2.Mid + ((ulong)d2.High << 32)) >> (32 - shift)); From 7e6b5cf45c711010a08f31983c602dc44370ad04 Mon Sep 17 00:00:00 2001 From: Jakob Botsch Nielsen Date: Thu, 24 Jun 2021 20:39:54 +0200 Subject: [PATCH 02/12] Run jit-format --- src/coreclr/jit/codegencommon.cpp | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-) diff --git a/src/coreclr/jit/codegencommon.cpp b/src/coreclr/jit/codegencommon.cpp index aa4be9c839714..f0338cf12cba5 100644 --- a/src/coreclr/jit/codegencommon.cpp +++ b/src/coreclr/jit/codegencommon.cpp @@ -12537,7 +12537,7 @@ void CodeGenInterface::VariableLiveKeeper::dumpLvaVariableLiveRanges() const void CodeGen::genSetRegsModifiedForPoisoning() { - // For ARM32 we need a callee-reserved register since the scratch registers may have args. +// For ARM32 we need a callee-reserved register since the scratch registers may have args. #if defined(TARGET_ARM) regSet.rsSetRegsModified(RBM_R4); #endif @@ -12566,7 +12566,8 @@ void CodeGen::genPoisonFrame(regMaskTP regLiveIn) assert(compiler->compShouldPoisonFrame()); assert((regLiveIn & genRegMask(immRegNum)) == 0); - // The first time we need to poison something we will initialize a register to the largest immediate cccccccc that we can fit. + // The first time we need to poison something we will initialize a register to the largest immediate cccccccc that + // we can fit. bool hasPoisonImm = false; for (unsigned varNum = 0; varNum < compiler->info.compLocalsCount; varNum++) { @@ -12588,15 +12589,15 @@ void CodeGen::genPoisonFrame(regMaskTP regLiveIn) hasPoisonImm = true; } - // For 64-bit we check if the local is 8-byte aligned. For 32-bit, we assume everything is always 4-byte aligned. +// For 64-bit we check if the local is 8-byte aligned. For 32-bit, we assume everything is always 4-byte aligned. #ifdef TARGET_64BIT bool fpBased; - int addr = compiler->lvaFrameAddress((int)varNum, &fpBased); + int addr = compiler->lvaFrameAddress((int)varNum, &fpBased); #else int addr = 0; #endif int size = (int)compiler->lvaLclSize(varNum); - int end = addr + size; + int end = addr + size; for (int offs = addr; offs < end;) { #ifdef TARGET_64BIT @@ -12613,4 +12614,4 @@ void CodeGen::genPoisonFrame(regMaskTP regLiveIn) offs += 4; } } -} +} From 1f3d65408d065c055b466a42e0aaef1223b7989b Mon Sep 17 00:00:00 2001 From: Jakob Botsch Nielsen Date: Fri, 25 Jun 2021 17:37:05 +0200 Subject: [PATCH 03/12] Use named scratch register and kill it in LSRA --- src/coreclr/jit/codegencommon.cpp | 20 +++++--------------- src/coreclr/jit/lsrabuild.cpp | 9 +++++++++ 2 files changed, 14 insertions(+), 15 deletions(-) diff --git a/src/coreclr/jit/codegencommon.cpp b/src/coreclr/jit/codegencommon.cpp index f0338cf12cba5..1e9dd22bf996a 100644 --- a/src/coreclr/jit/codegencommon.cpp +++ b/src/coreclr/jit/codegencommon.cpp @@ -12553,18 +12553,8 @@ void CodeGen::genSetRegsModifiedForPoisoning() // so we expect that this reg is not live. void CodeGen::genPoisonFrame(regMaskTP regLiveIn) { -#if defined(TARGET_XARCH) - const regNumber immRegNum = REG_EAX; -#elif defined(TARGET_ARM) - const regNumber immRegNum = REG_R4; -#elif defined(TARGET_ARM64) - const regNumber immRegNum = REG_R9; -#else - return; -#endif - assert(compiler->compShouldPoisonFrame()); - assert((regLiveIn & genRegMask(immRegNum)) == 0); + assert((regLiveIn & genRegMask(REG_SCRATCH)) == 0); // The first time we need to poison something we will initialize a register to the largest immediate cccccccc that // we can fit. @@ -12582,9 +12572,9 @@ void CodeGen::genPoisonFrame(regMaskTP regLiveIn) if (!hasPoisonImm) { #ifdef TARGET_64BIT - instGen_Set_Reg_To_Imm(EA_8BYTE, immRegNum, (ssize_t)0xcccccccccccccccc); + genSetRegToIcon(REG_SCRATCH, (ssize_t)0xcdcdcdcdcdcdcdcd, TYP_LONG); #else - instGen_Set_Reg_To_Imm(EA_4BYTE, immRegNum, (ssize_t)0xcccccccc); + genSetRegToIcon(REG_SCRATCH, (ssize_t)0xcdcdcdcd, TYP_INT); #endif hasPoisonImm = true; } @@ -12603,14 +12593,14 @@ void CodeGen::genPoisonFrame(regMaskTP regLiveIn) #ifdef TARGET_64BIT if ((offs & 7) == 0 && end - offs >= 8) { - GetEmitter()->emitIns_S_R(ins_Store(TYP_LONG), EA_8BYTE, immRegNum, (int)varNum, offs - addr); + GetEmitter()->emitIns_S_R(ins_Store(TYP_LONG), EA_8BYTE, REG_SCRATCH, (int)varNum, offs - addr); offs += 8; continue; } #endif assert((offs & 3) == 0 && end - offs >= 4); - GetEmitter()->emitIns_S_R(ins_Store(TYP_INT), EA_4BYTE, immRegNum, (int)varNum, offs - addr); + GetEmitter()->emitIns_S_R(ins_Store(TYP_INT), EA_4BYTE, REG_SCRATCH, (int)varNum, offs - addr); offs += 4; } } diff --git a/src/coreclr/jit/lsrabuild.cpp b/src/coreclr/jit/lsrabuild.cpp index dcea881311919..6bb36a7ef1e40 100644 --- a/src/coreclr/jit/lsrabuild.cpp +++ b/src/coreclr/jit/lsrabuild.cpp @@ -2333,6 +2333,15 @@ void LinearScan::buildIntervals() // assert(block->isRunRarely()); } + // For frame poisoning we generate code into scratch BB right after prolog since + // otherwise the prolog might become too large. In this case we will put the poison immediate + // into the scratch register, so it will be killed here. + if (compiler->compShouldPoisonFrame() && compiler->fgFirstBBisScratch() && block == compiler->fgFirstBB) + { + addRefsForPhysRegMask(genRegMask(REG_SCRATCH), currentLoc + 1, RefTypeKill, true); + currentLoc += 2; + } + LIR::Range& blockRange = LIR::AsRange(block); for (GenTree* node : blockRange) { From 5122ec0c04d9ee0fa7ab392d86938f8a2794eacd Mon Sep 17 00:00:00 2001 From: Jakob Botsch Nielsen Date: Fri, 25 Jun 2021 17:37:34 +0200 Subject: [PATCH 04/12] Enable it unconditionally for testing purposes --- src/coreclr/jit/compiler.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/coreclr/jit/compiler.h b/src/coreclr/jit/compiler.h index 914e3424fa742..0adb04ac5ff40 100644 --- a/src/coreclr/jit/compiler.h +++ b/src/coreclr/jit/compiler.h @@ -9831,7 +9831,7 @@ XXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXX // Returns true if address-exposed user variables should be poisoned with a recognizable value bool compShouldPoisonFrame() { - return !info.compInitMem && opts.MinOpts() && opts.compDbgCode; + return true; //!info.compInitMem&& opts.MinOpts() && opts.compDbgCode; } #if defined(DEBUG) From 8acf5ea0495998d8829e7bf2613060e275f07a48 Mon Sep 17 00:00:00 2001 From: Jakob Botsch Nielsen Date: Fri, 25 Jun 2021 19:16:29 +0200 Subject: [PATCH 05/12] Remove unnecessary modified reg on ARM --- src/coreclr/jit/codegencommon.cpp | 14 -------------- 1 file changed, 14 deletions(-) diff --git a/src/coreclr/jit/codegencommon.cpp b/src/coreclr/jit/codegencommon.cpp index 1e9dd22bf996a..9d32fba7d4ecf 100644 --- a/src/coreclr/jit/codegencommon.cpp +++ b/src/coreclr/jit/codegencommon.cpp @@ -6733,12 +6733,6 @@ void CodeGen::genFinalizeFrame() } #endif // TARGET_ARM - // When poisoning on ARM we need to use callee-preserved registers. - if (compiler->compShouldPoisonFrame()) - { - genSetRegsModifiedForPoisoning(); - } - #ifdef DEBUG if (verbose) { @@ -12535,14 +12529,6 @@ void CodeGenInterface::VariableLiveKeeper::dumpLvaVariableLiveRanges() const #endif // DEBUG #endif // USING_VARIABLE_LIVE_RANGE -void CodeGen::genSetRegsModifiedForPoisoning() -{ -// For ARM32 we need a callee-reserved register since the scratch registers may have args. -#if defined(TARGET_ARM) - regSet.rsSetRegsModified(RBM_R4); -#endif -} - //----------------------------------------------------------------------------- // genPoisonFrame: Generate code that places a recognizable value into address exposed variables. // From 94fe61d39e8d665483ee06528a23abf487157009 Mon Sep 17 00:00:00 2001 From: Jakob Botsch Nielsen Date: Fri, 25 Jun 2021 22:49:00 +0200 Subject: [PATCH 06/12] Fix OSR and get rid of test code --- src/coreclr/jit/codegencommon.cpp | 3 +-- src/coreclr/jit/compiler.h | 6 +++++- 2 files changed, 6 insertions(+), 3 deletions(-) diff --git a/src/coreclr/jit/codegencommon.cpp b/src/coreclr/jit/codegencommon.cpp index 9d32fba7d4ecf..e800ead6ce0ba 100644 --- a/src/coreclr/jit/codegencommon.cpp +++ b/src/coreclr/jit/codegencommon.cpp @@ -12535,8 +12535,7 @@ void CodeGenInterface::VariableLiveKeeper::dumpLvaVariableLiveRanges() const // Remarks: // This function emits code to poison address exposed non-zero-inited local variables. We expect this function // to be called when emitting code for the scratch BB that comes right after the prolog. -// The variables are poisoned using 0xcccccccc. This value is stored in EAX/RAX on xarch and r/x0 on ARM, -// so we expect that this reg is not live. +// The variables are poisoned using 0xcdcdcdcd. void CodeGen::genPoisonFrame(regMaskTP regLiveIn) { assert(compiler->compShouldPoisonFrame()); diff --git a/src/coreclr/jit/compiler.h b/src/coreclr/jit/compiler.h index 0adb04ac5ff40..47cff55ced8c5 100644 --- a/src/coreclr/jit/compiler.h +++ b/src/coreclr/jit/compiler.h @@ -9831,7 +9831,11 @@ XXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXX // Returns true if address-exposed user variables should be poisoned with a recognizable value bool compShouldPoisonFrame() { - return true; //!info.compInitMem&& opts.MinOpts() && opts.compDbgCode; +#ifdef FEATURE_ON_STACK_REPLACEMENT + if (opts.IsOSR()) + return false; +#endif + return !info.compInitMem && opts.compDbgCode; } #if defined(DEBUG) From 4db5c5f5596e7a401034f5dfd075991cb09f8181 Mon Sep 17 00:00:00 2001 From: Jakob Botsch Nielsen Date: Sat, 26 Jun 2021 01:30:04 +0200 Subject: [PATCH 07/12] Remove a declaration --- src/coreclr/jit/codegen.h | 1 - 1 file changed, 1 deletion(-) diff --git a/src/coreclr/jit/codegen.h b/src/coreclr/jit/codegen.h index 1cd8d6bef3c6e..73bb2a0b247a6 100644 --- a/src/coreclr/jit/codegen.h +++ b/src/coreclr/jit/codegen.h @@ -348,7 +348,6 @@ class CodeGen final : public CodeGenInterface void genAllocLclFrame(unsigned frameSize, regNumber initReg, bool* pInitRegZeroed, regMaskTP maskArgRegsLiveIn); - void genSetRegsModifiedForPoisoning(); void genPoisonFrame(regMaskTP bbRegLiveIn); #if defined(TARGET_ARM) From 90b01339a63801d6aaf229f1517382964bce5538 Mon Sep 17 00:00:00 2001 From: Jakob Botsch Nielsen Date: Tue, 29 Jun 2021 13:22:55 +0200 Subject: [PATCH 08/12] Undo modified comment and use modulo instead of and --- src/coreclr/jit/codegencommon.cpp | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/coreclr/jit/codegencommon.cpp b/src/coreclr/jit/codegencommon.cpp index e800ead6ce0ba..cfbdf1e9033b8 100644 --- a/src/coreclr/jit/codegencommon.cpp +++ b/src/coreclr/jit/codegencommon.cpp @@ -4842,7 +4842,7 @@ void CodeGen::genCheckUseBlockInit() } } - // Record number of stack slots that need zeroing. + // Record number of 4 byte slots that need zeroing. genInitStkLclCnt = initStkLclCnt; // Decide if we will do block initialization in the prolog, or use @@ -12576,7 +12576,7 @@ void CodeGen::genPoisonFrame(regMaskTP regLiveIn) for (int offs = addr; offs < end;) { #ifdef TARGET_64BIT - if ((offs & 7) == 0 && end - offs >= 8) + if ((offs % 8) == 0 && end - offs >= 8) { GetEmitter()->emitIns_S_R(ins_Store(TYP_LONG), EA_8BYTE, REG_SCRATCH, (int)varNum, offs - addr); offs += 8; @@ -12584,7 +12584,7 @@ void CodeGen::genPoisonFrame(regMaskTP regLiveIn) } #endif - assert((offs & 3) == 0 && end - offs >= 4); + assert((offs % 4) == 0 && end - offs >= 4); GetEmitter()->emitIns_S_R(ins_Store(TYP_INT), EA_4BYTE, REG_SCRATCH, (int)varNum, offs - addr); offs += 4; } From 3d9fd8c6462b0c63839220ff155c5b3311db2b89 Mon Sep 17 00:00:00 2001 From: Jakob Botsch Nielsen Date: Tue, 29 Jun 2021 20:22:44 +0200 Subject: [PATCH 09/12] Add a test --- src/tests/JIT/Directed/debugging/poison.cs | 56 +++++++++++++++++++ .../JIT/Directed/debugging/poison.csproj | 11 ++++ 2 files changed, 67 insertions(+) create mode 100644 src/tests/JIT/Directed/debugging/poison.cs create mode 100644 src/tests/JIT/Directed/debugging/poison.csproj diff --git a/src/tests/JIT/Directed/debugging/poison.cs b/src/tests/JIT/Directed/debugging/poison.cs new file mode 100644 index 0000000000000..463894a4e6a35 --- /dev/null +++ b/src/tests/JIT/Directed/debugging/poison.cs @@ -0,0 +1,56 @@ +using System; +using System.Runtime.CompilerServices; + +public class Program +{ + [SkipLocalsInit] + public static unsafe int Main() + { + bool result = true; + + int poisoned; + Unsafe.SkipInit(out poisoned); + result &= VerifyPoison(&poisoned, sizeof(int)); + + GCRef zeroed; + Unsafe.SkipInit(out zeroed); + result &= VerifyZero(Unsafe.AsPointer(ref zeroed), Unsafe.SizeOf()); + + WithoutGCRef poisoned2; + Unsafe.SkipInit(out poisoned2); + result &= VerifyPoison(&poisoned2, sizeof(WithoutGCRef)); + + return result ? 100 : 101; + } + + [MethodImpl(MethodImplOptions.NoInlining)] + private static unsafe bool VerifyPoison(void* val, int size) + => AllEq(new Span(val, size), 0xCD); + + [MethodImpl(MethodImplOptions.NoInlining)] + private static unsafe bool VerifyZero(void* val, int size) + => AllEq(new Span(val, size), 0); + + private static unsafe bool AllEq(Span span, byte byteVal) + { + foreach (byte b in span) + { + if (b != byteVal) + return false; + } + + return true; + } + + private struct GCRef + { + public object ARef; + } + + private struct WithoutGCRef + { + public double ADouble; + public int ANumber; + public float AFloat; + } +} diff --git a/src/tests/JIT/Directed/debugging/poison.csproj b/src/tests/JIT/Directed/debugging/poison.csproj new file mode 100644 index 0000000000000..b2691ab4cab22 --- /dev/null +++ b/src/tests/JIT/Directed/debugging/poison.csproj @@ -0,0 +1,11 @@ + + + Exe + PdbOnly + False + True + + + + + From 8c565780fb2a00b58d77509cf38c27dab60aa4d3 Mon Sep 17 00:00:00 2001 From: Jakob Botsch Nielsen Date: Tue, 29 Jun 2021 20:35:08 +0200 Subject: [PATCH 10/12] Rephrase comment Co-authored-by: Kunal Pathak --- src/coreclr/jit/flowgraph.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/coreclr/jit/flowgraph.cpp b/src/coreclr/jit/flowgraph.cpp index ffe08f967bc95..ee1ca1c1cd9bd 100644 --- a/src/coreclr/jit/flowgraph.cpp +++ b/src/coreclr/jit/flowgraph.cpp @@ -2603,7 +2603,7 @@ void Compiler::fgAddInternal() noway_assert(!compIsForInlining()); // The backend requires a scratch BB into which it can safely insert a P/Invoke method prolog if one is - // required. Create it here. + // required. Similarly, we need a scratch BB for poisoning. Create it here. // Similarly, for poisoning we also need a scratch BB. if (compMethodRequiresPInvokeFrame() || compShouldPoisonFrame()) { From 302bfba14740322985b4daaec2ff31791a48931a Mon Sep 17 00:00:00 2001 From: Jakob Botsch Nielsen Date: Tue, 29 Jun 2021 22:41:54 +0200 Subject: [PATCH 11/12] Disable poisoning test on mono --- src/tests/issues.targets | 3 +++ 1 file changed, 3 insertions(+) diff --git a/src/tests/issues.targets b/src/tests/issues.targets index bac25dd48638a..6e16937b7f74a 100644 --- a/src/tests/issues.targets +++ b/src/tests/issues.targets @@ -1668,6 +1668,9 @@ needs triage + + Tests coreclr JIT's debug poisoning of address taken variables + From a63669e868a0c2aab02801d4742a36f1ad96e06e Mon Sep 17 00:00:00 2001 From: Jakob Botsch Nielsen Date: Tue, 29 Jun 2021 23:02:14 +0200 Subject: [PATCH 12/12] Remove outdated line --- src/coreclr/jit/flowgraph.cpp | 1 - 1 file changed, 1 deletion(-) diff --git a/src/coreclr/jit/flowgraph.cpp b/src/coreclr/jit/flowgraph.cpp index ee1ca1c1cd9bd..04faed33f50a8 100644 --- a/src/coreclr/jit/flowgraph.cpp +++ b/src/coreclr/jit/flowgraph.cpp @@ -2604,7 +2604,6 @@ void Compiler::fgAddInternal() // The backend requires a scratch BB into which it can safely insert a P/Invoke method prolog if one is // required. Similarly, we need a scratch BB for poisoning. Create it here. - // Similarly, for poisoning we also need a scratch BB. if (compMethodRequiresPInvokeFrame() || compShouldPoisonFrame()) { fgEnsureFirstBBisScratch();