diff --git a/src/coreclr/jit/importer.cpp b/src/coreclr/jit/importer.cpp index e1e08d6b0972bd..df7834354af195 100644 --- a/src/coreclr/jit/importer.cpp +++ b/src/coreclr/jit/importer.cpp @@ -10462,7 +10462,15 @@ void Compiler::impImportBlockCode(BasicBlock* block) GenTree* boxPayloadAddress = gtNewOperNode(GT_ADD, TYP_BYREF, cloneOperand, boxPayloadOffset); GenTree* nullcheck = gtNewNullCheck(op1, block); - GenTree* result = gtNewOperNode(GT_COMMA, TYP_BYREF, nullcheck, boxPayloadAddress); + // Add an ordering dependency between the null + // check and forming the byref; the JIT assumes + // in many places that the only legal null + // byref is literally 0, and since the byref + // leaks out here, we need to ensure it is + // nullchecked. + nullcheck->gtFlags |= GTF_ORDER_SIDEEFF; + boxPayloadAddress->gtFlags |= GTF_ORDER_SIDEEFF; + GenTree* result = gtNewOperNode(GT_COMMA, TYP_BYREF, nullcheck, boxPayloadAddress); impPushOnStack(result, tiRetVal); break; } diff --git a/src/coreclr/jit/importercalls.cpp b/src/coreclr/jit/importercalls.cpp index b9e8c3af9d3284..9a2149be4a9c1b 100644 --- a/src/coreclr/jit/importercalls.cpp +++ b/src/coreclr/jit/importercalls.cpp @@ -2728,6 +2728,11 @@ GenTree* Compiler::impIntrinsic(GenTree* newobjThis, // Prepare result var_types resultType = JITtype2varType(sig->retType); assert(resultType == result->TypeGet()); + // Add an ordering dependency between the bounds check and + // forming the byref to prevent these from being reordered. The + // JIT is not allowed to create arbitrary illegal byrefs. + boundsCheck->gtFlags |= GTF_ORDER_SIDEEFF; + result->gtFlags |= GTF_ORDER_SIDEEFF; retNode = gtNewOperNode(GT_COMMA, resultType, boundsCheck, result); break; diff --git a/src/coreclr/jit/morph.cpp b/src/coreclr/jit/morph.cpp index 6d8310403237ca..1177a6aef5b1f1 100644 --- a/src/coreclr/jit/morph.cpp +++ b/src/coreclr/jit/morph.cpp @@ -4608,6 +4608,13 @@ GenTree* Compiler::fgMorphIndexAddr(GenTreeIndexAddr* indexAddr) // Prepend the bounds check and the assignment trees that were created (if any). if (boundsCheck != nullptr) { + // This is changing a value dependency (INDEX_ADDR node) into a flow + // dependency, so make sure this dependency remains visible. Also, the + // JIT is not allowed to create arbitrary byrefs, so we must make sure + // the address is not reordered with the bounds check. + boundsCheck->gtFlags |= GTF_ORDER_SIDEEFF; + addr->gtFlags |= GTF_ORDER_SIDEEFF; + tree = gtNewOperNode(GT_COMMA, tree->TypeGet(), boundsCheck, tree); fgSetRngChkTarget(boundsCheck); } @@ -5190,6 +5197,8 @@ GenTree* Compiler::fgMorphExpandInstanceField(GenTree* tree, MorphAddrContext* m GenTree* lclVar = gtNewLclvNode(lclNum, objRefType); GenTree* nullchk = gtNewNullCheck(lclVar, compCurBB); + nullchk->gtFlags |= GTF_ORDER_SIDEEFF; + if (asg != nullptr) { // Create the "comma" node. @@ -5201,6 +5210,10 @@ GenTree* Compiler::fgMorphExpandInstanceField(GenTree* tree, MorphAddrContext* m } addr = gtNewLclvNode(lclNum, objRefType); // Use "tmpLcl" to create "addr" node. + + // Ensure the creation of the byref does not get reordered with the + // null check, as that could otherwise create an illegal byref. + addr->gtFlags |= GTF_ORDER_SIDEEFF; } else { @@ -10646,7 +10659,7 @@ GenTree* Compiler::fgMorphSmpOp(GenTree* tree, MorphAddrContext* mac, bool* optA // could result in an invalid value number for the newly generated GT_IND node. if ((op1->OperGet() == GT_COMMA) && fgGlobalMorph) { - // Perform the transform IND(COMMA(x, ..., z)) == COMMA(x, ..., IND(z)). + // Perform the transform IND(COMMA(x, ..., z)) -> COMMA(x, ..., IND(z)). // TBD: this transformation is currently necessary for correctness -- it might // be good to analyze the failures that result if we don't do this, and fix them // in other ways. Ideally, this should be optional. @@ -10679,9 +10692,12 @@ GenTree* Compiler::fgMorphSmpOp(GenTree* tree, MorphAddrContext* mac, bool* optA // TODO-1stClassStructs: we often create a struct IND without a handle, fix it. op1 = gtNewIndir(typ, addr); - // Determine flags on the indir. + // GTF_GLOB_EFFECT flags can be recomputed from the child + // nodes. GTF_ORDER_SIDEEFF may be set already and indicate no + // reordering is allowed with sibling nodes, so we cannot + // recompute that. // - op1->gtFlags |= treeFlags & ~GTF_ALL_EFFECT; + op1->gtFlags |= treeFlags & ~GTF_GLOB_EFFECT; op1->gtFlags |= (addr->gtFlags & GTF_ALL_EFFECT); // if this was a non-faulting indir, clear GTF_EXCEPT, diff --git a/src/coreclr/jit/optimizer.cpp b/src/coreclr/jit/optimizer.cpp index be12db114d5782..0e9e43aeeeabf5 100644 --- a/src/coreclr/jit/optimizer.cpp +++ b/src/coreclr/jit/optimizer.cpp @@ -4794,6 +4794,17 @@ bool Compiler::optIfConvert(BasicBlock* block) return false; } + // Evaluating op1/op2 unconditionally effectively has the same effect as + // reordering them with the condition (for example, the condition could be + // an explicit bounds check and the operand could read an array element). + // Disallow this except for some common cases that we know are always side + // effect free. + if (((cond->gtFlags & GTF_ORDER_SIDEEFF) != 0) && !asgNode->gtGetOp2()->IsInvariant() && + !asgNode->gtGetOp2()->OperIsLocal()) + { + return false; + } + #ifdef DEBUG if (verbose) { diff --git a/src/tests/JIT/Regression/JitBlue/Runtime_78554/Runtime_78554.cs b/src/tests/JIT/Regression/JitBlue/Runtime_78554/Runtime_78554.cs new file mode 100644 index 00000000000000..70445fb8f18f4e --- /dev/null +++ b/src/tests/JIT/Regression/JitBlue/Runtime_78554/Runtime_78554.cs @@ -0,0 +1,29 @@ +// Licensed to the .NET Foundation under one or more agreements. +// The .NET Foundation licenses this file to you under the MIT license. + +using System.Runtime.CompilerServices; + +public class Runtime_78554 +{ + [MethodImpl(MethodImplOptions.NoInlining)] + static void Consume(uint op) + { + } + + [MethodImplAttribute(MethodImplOptions.NoInlining)] + static void ArrayIndexConsume(uint[] a, uint i) + { + if (i < a.Length) + { + i = a[i]; + } + Consume(i); + } + + public static int Main() + { + var arr = new uint[] { 1, 42, 3000 }; + ArrayIndexConsume(arr, 0xffffffff); + return 100; + } +} diff --git a/src/tests/JIT/Regression/JitBlue/Runtime_78554/Runtime_78554.csproj b/src/tests/JIT/Regression/JitBlue/Runtime_78554/Runtime_78554.csproj new file mode 100644 index 00000000000000..7a789ed5ed1f58 --- /dev/null +++ b/src/tests/JIT/Regression/JitBlue/Runtime_78554/Runtime_78554.csproj @@ -0,0 +1,13 @@ + + + Exe + True + + + + + + + + +