From eac92c98ff9a58b2b805018f6a2c94632c211250 Mon Sep 17 00:00:00 2001 From: Carol Eidt Date: Fri, 31 May 2019 11:02:02 -0700 Subject: [PATCH] Handle a zero byte cpblk (dotnet/coreclr#24871) Fix dotnet/coreclr#24846 Commit migrated from https://github.com/dotnet/coreclr/commit/0d21b57657f8135622797f0db29460c975dd2339 --- src/coreclr/src/jit/codegenlinear.cpp | 3 +- src/coreclr/src/jit/codegenxarch.cpp | 24 ++----- src/coreclr/src/jit/gentree.cpp | 3 +- src/coreclr/src/jit/lowerxarch.cpp | 6 +- src/coreclr/src/jit/lsraarmarch.cpp | 5 +- src/coreclr/src/jit/lsraxarch.cpp | 5 +- src/coreclr/src/jit/morph.cpp | 5 +- .../JitBlue/GitHub_24846/GitHub_24846.cs | 70 +++++++++++++++++++ .../JitBlue/GitHub_24846/GitHub_24846.csproj | 18 +++++ 9 files changed, 108 insertions(+), 31 deletions(-) create mode 100644 src/coreclr/tests/src/JIT/Regression/JitBlue/GitHub_24846/GitHub_24846.cs create mode 100644 src/coreclr/tests/src/JIT/Regression/JitBlue/GitHub_24846/GitHub_24846.csproj diff --git a/src/coreclr/src/jit/codegenlinear.cpp b/src/coreclr/src/jit/codegenlinear.cpp index 88d3ac3b88a..dac1df9e564 100644 --- a/src/coreclr/src/jit/codegenlinear.cpp +++ b/src/coreclr/src/jit/codegenlinear.cpp @@ -1662,14 +1662,13 @@ void CodeGen::genSetBlockSize(GenTreeBlk* blkNode, regNumber sizeReg) if (sizeReg != REG_NA) { unsigned blockSize = blkNode->Size(); - if (blockSize != 0) + if (!blkNode->OperIs(GT_STORE_DYN_BLK)) { assert((blkNode->gtRsvdRegs & genRegMask(sizeReg)) != 0); genSetRegToIcon(sizeReg, blockSize); } else { - noway_assert(blkNode->gtOper == GT_STORE_DYN_BLK); GenTree* sizeNode = blkNode->AsDynBlk()->gtDynamicSize; if (sizeNode->gtRegNum != sizeReg) { diff --git a/src/coreclr/src/jit/codegenxarch.cpp b/src/coreclr/src/jit/codegenxarch.cpp index 0f07cf13e01..3855e2cfe8f 100644 --- a/src/coreclr/src/jit/codegenxarch.cpp +++ b/src/coreclr/src/jit/codegenxarch.cpp @@ -3270,25 +3270,19 @@ void CodeGen::genCodeForCpBlkRepMovs(GenTreeBlk* cpBlkNode) GenTree* source = cpBlkNode->Data(); GenTree* srcAddr = nullptr; -#ifdef DEBUG assert(dstAddr->isUsedFromReg()); assert(source->isContained()); #ifdef _TARGET_X86_ - if (size == 0) + if (!cpBlkNode->OperIs(GT_STORE_DYN_BLK)) { - noway_assert(cpBlkNode->OperGet() == GT_STORE_DYN_BLK); + assert((size == 0) || (size > CPBLK_UNROLL_LIMIT)); } - else -#endif - { -#ifdef _TARGET_AMD64_ - assert(size > CPBLK_UNROLL_LIMIT && size < CPBLK_MOVS_LIMIT); -#else - assert(size > CPBLK_UNROLL_LIMIT); +#else // _TARGET_AMD64_ + // On x64 we use the helper call for GT_STORE_DYN_BLK. + assert(!cpBlkNode->OperIs(GT_STORE_DYN_BLK)); + assert((size == 0) || ((size > CPBLK_UNROLL_LIMIT) && (size < CPBLK_MOVS_LIMIT))); #endif - } -#endif // DEBUG genConsumeBlockOp(cpBlkNode, REG_RDI, REG_RSI, REG_RCX); instGen(INS_r_movsb); @@ -3791,15 +3785,11 @@ void CodeGen::genCodeForCpBlk(GenTreeBlk* cpBlkNode) GenTree* srcAddr = nullptr; // Size goes in arg2 - if (blockSize != 0) + if (cpBlkNode->gtOper != GT_STORE_DYN_BLK) { assert(blockSize >= CPBLK_MOVS_LIMIT); assert((cpBlkNode->gtRsvdRegs & RBM_ARG_2) != 0); } - else - { - noway_assert(cpBlkNode->gtOper == GT_STORE_DYN_BLK); - } // Source address goes in arg1 if (source->gtOper == GT_IND) diff --git a/src/coreclr/src/jit/gentree.cpp b/src/coreclr/src/jit/gentree.cpp index 420e87c7e54..413db06c05f 100644 --- a/src/coreclr/src/jit/gentree.cpp +++ b/src/coreclr/src/jit/gentree.cpp @@ -15456,9 +15456,8 @@ bool GenTree::DefinesLocal(Compiler* comp, GenTreeLclVarCommon** pLclVarTree, bo GenTree* destAddr = blkNode->Addr(); unsigned width = blkNode->gtBlkSize; // Do we care about whether this assigns the entire variable? - if (pIsEntire != nullptr && width == 0) + if (pIsEntire != nullptr && blkNode->OperIs(GT_DYN_BLK)) { - assert(blkNode->gtOper == GT_DYN_BLK); GenTree* blockWidth = blkNode->AsDynBlk()->gtDynamicSize; if (blockWidth->IsCnsIntOrI()) { diff --git a/src/coreclr/src/jit/lowerxarch.cpp b/src/coreclr/src/jit/lowerxarch.cpp index bcb5bef51f0..d29ea9375f5 100644 --- a/src/coreclr/src/jit/lowerxarch.cpp +++ b/src/coreclr/src/jit/lowerxarch.cpp @@ -329,7 +329,7 @@ void Lowering::LowerBlockStore(GenTreeBlk* blkNode) } else { - assert((blkNode->OperGet() == GT_STORE_BLK) || (blkNode->OperGet() == GT_STORE_DYN_BLK)); + assert(blkNode->OperIs(GT_STORE_BLK, GT_STORE_DYN_BLK)); // CopyBlk // In case of a CpBlk with a constant size and less than CPBLK_MOVS_LIMIT size // we can use rep movs to generate code instead of the helper call. @@ -339,12 +339,12 @@ void Lowering::LowerBlockStore(GenTreeBlk* blkNode) unsigned helperThreshold = max(CPBLK_MOVS_LIMIT, CPBLK_UNROLL_LIMIT); // TODO-X86-CQ: Investigate whether a helper call would be beneficial on x86 - if ((size != 0) && (size <= helperThreshold)) + if ((!blkNode->OperIs(GT_STORE_DYN_BLK)) && (size <= helperThreshold)) { // If we have a buffer between XMM_REGSIZE_BYTES and CPBLK_UNROLL_LIMIT bytes, we'll use SSE2. // Structs and buffer with sizes <= CPBLK_UNROLL_LIMIT bytes are occurring in more than 95% of // our framework assemblies, so this is the main code generation scheme we'll use. - if (size <= CPBLK_UNROLL_LIMIT) + if ((size != 0) && (size <= CPBLK_UNROLL_LIMIT)) { blkNode->gtBlkOpKind = GenTreeBlk::BlkOpKindUnroll; diff --git a/src/coreclr/src/jit/lsraarmarch.cpp b/src/coreclr/src/jit/lsraarmarch.cpp index 9a54c322797..cc12e242f34 100644 --- a/src/coreclr/src/jit/lsraarmarch.cpp +++ b/src/coreclr/src/jit/lsraarmarch.cpp @@ -679,7 +679,7 @@ int LinearScan::BuildBlockStore(GenTreeBlk* blkNode) } } - if ((size != 0) && (blkSizeRegMask != RBM_NONE)) + if (!blkNode->OperIs(GT_STORE_DYN_BLK) && (blkSizeRegMask != RBM_NONE)) { // Reserve a temp register for the block size argument. buildInternalIntRegisterDefForNode(blkNode, blkSizeRegMask); @@ -701,9 +701,8 @@ int LinearScan::BuildBlockStore(GenTreeBlk* blkNode) BuildUse(dstAddr, dstAddrRegMask); } - if (size == 0) + if (blkNode->OperIs(GT_STORE_DYN_BLK)) { - assert(blkNode->OperIs(GT_STORE_DYN_BLK)); // The block size argument is a third argument to GT_STORE_DYN_BLK srcCount++; GenTree* blockSize = blkNode->AsDynBlk()->gtDynamicSize; diff --git a/src/coreclr/src/jit/lsraxarch.cpp b/src/coreclr/src/jit/lsraxarch.cpp index 5f1094ee4f1..c84a479cd0c 100644 --- a/src/coreclr/src/jit/lsraxarch.cpp +++ b/src/coreclr/src/jit/lsraxarch.cpp @@ -1441,7 +1441,7 @@ int LinearScan::BuildBlockStore(GenTreeBlk* blkNode) } } - if ((size != 0) && (blkSizeRegMask != RBM_NONE)) + if (!blkNode->OperIs(GT_STORE_DYN_BLK) && (blkSizeRegMask != RBM_NONE)) { // Reserve a temp register for the block size argument. buildInternalIntRegisterDefForNode(blkNode, blkSizeRegMask); @@ -1463,9 +1463,8 @@ int LinearScan::BuildBlockStore(GenTreeBlk* blkNode) BuildUse(dstAddr, dstAddrRegMask); } - if (size == 0) + if (blkNode->OperIs(GT_STORE_DYN_BLK)) { - assert(blkNode->OperIs(GT_STORE_DYN_BLK)); // The block size argument is a third argument to GT_STORE_DYN_BLK srcCount++; GenTree* blockSize = blkNode->AsDynBlk()->gtDynamicSize; diff --git a/src/coreclr/src/jit/morph.cpp b/src/coreclr/src/jit/morph.cpp index 7040dd0b25d..3bfca379a81 100644 --- a/src/coreclr/src/jit/morph.cpp +++ b/src/coreclr/src/jit/morph.cpp @@ -9099,7 +9099,10 @@ GenTree* Compiler::fgMorphOneAsgBlockOp(GenTree* tree) size = genTypeSize(asgType); } } - noway_assert((size != 0) || dest->OperIs(GT_DYN_BLK)); + if (size == 0) + { + return nullptr; + } // // See if we can do a simple transformation: diff --git a/src/coreclr/tests/src/JIT/Regression/JitBlue/GitHub_24846/GitHub_24846.cs b/src/coreclr/tests/src/JIT/Regression/JitBlue/GitHub_24846/GitHub_24846.cs new file mode 100644 index 00000000000..dbd276f1425 --- /dev/null +++ b/src/coreclr/tests/src/JIT/Regression/JitBlue/GitHub_24846/GitHub_24846.cs @@ -0,0 +1,70 @@ +// 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. + +using System; +using System.Runtime.CompilerServices; + +public class GitHub_24846 +{ + public static void Test(byte[] destination, byte[] source) + { + Unsafe.CopyBlockUnaligned(ref destination[0], ref source[0], 0); + } + + public static int Main(string[] args) + { + int returnVal = 100; + var destination = new byte[1]; + var source = new byte[1]; + try + { + Test(destination, source); + } + catch (Exception e) + { + Console.WriteLine("FAILED: " + e.Message); + returnVal = -1; + } + bool caught = false; + try + { + Test(destination, null); + } + catch (NullReferenceException e) + { + caught = true; + } + catch (Exception e) + { + Console.WriteLine("FAILED: Wrong Exception " + e.Message); + returnVal = -1; + } + if (!caught) + { + Console.WriteLine("FAILED: null destination didn't throw"); + returnVal = -1; + } + caught = false; + try + { + Test(null, source); + } + catch (NullReferenceException e) + { + caught = true; + } + catch (Exception e) + { + Console.WriteLine("FAILED: Wrong Exception " + e.Message); + returnVal = -1; + } + if (!caught) + { + Console.WriteLine("FAILED: null source didn't throw"); + returnVal = -1; + } + return returnVal; + } +} + diff --git a/src/coreclr/tests/src/JIT/Regression/JitBlue/GitHub_24846/GitHub_24846.csproj b/src/coreclr/tests/src/JIT/Regression/JitBlue/GitHub_24846/GitHub_24846.csproj new file mode 100644 index 00000000000..fb6ae363ebe --- /dev/null +++ b/src/coreclr/tests/src/JIT/Regression/JitBlue/GitHub_24846/GitHub_24846.csproj @@ -0,0 +1,18 @@ + + + + + Release + AnyCPU + $(MSBuildProjectName) + Exe + + True + True + + + + + + + -- GitLab