From 72d49127a0c25e4b931c81e621c2411bfb6633a5 Mon Sep 17 00:00:00 2001 From: Andy Ayers Date: Fri, 12 Apr 2019 00:44:39 -0700 Subject: [PATCH] JIT: update byref null check logic to handle field chains (#23850) The jit was not properly accumulating offsets when figuring out if a byref should be null checked. Use a non-null MorphAddressContext as indication that GT_FIELD and GT_IND nodes are actually part of an ongoing address computation. During field morphing propagate the current address context to the child node, instead of starting a new one. Fixes #23791. --- src/jit/morph.cpp | 31 +++++++++++----- .../JitBlue/GitHub_23791/GitHub_23791.cs | 41 ++++++++++++++++++++++ .../JitBlue/GitHub_23791/GitHub_23791.csproj | 34 ++++++++++++++++++ 3 files changed, 97 insertions(+), 9 deletions(-) create mode 100644 tests/src/JIT/Regression/JitBlue/GitHub_23791/GitHub_23791.cs create mode 100644 tests/src/JIT/Regression/JitBlue/GitHub_23791/GitHub_23791.csproj diff --git a/src/jit/morph.cpp b/src/jit/morph.cpp index 2369d1e..50143d3 100644 --- a/src/jit/morph.cpp +++ b/src/jit/morph.cpp @@ -6607,7 +6607,8 @@ GenTree* Compiler::fgMorphField(GenTree* tree, MorphAddrContext* mac) } noway_assert(tree->gtOper == GT_IND); - GenTree* res = fgMorphSmpOp(tree); + // Pass down the current mac; if non null we are computing an address + GenTree* res = fgMorphSmpOp(tree, mac); if (fldOffset == 0 && res->OperGet() == GT_IND) { @@ -11886,6 +11887,10 @@ GenTree* Compiler::fgMorphSmpOp(GenTree* tree, MorphAddrContext* mac) switch (tree->gtOper) { case GT_ADDR: + // A non-null mac here implies this node is part of an address computation. + // If so, we need to pass the existing mac down to the child node. + // + // Otherwise, use a new mac. if (subMac1 == nullptr) { subMac1 = &subIndMac1; @@ -11901,7 +11906,15 @@ GenTree* Compiler::fgMorphSmpOp(GenTree* tree, MorphAddrContext* mac) case GT_BLK: case GT_DYN_BLK: case GT_IND: - subMac1 = &subIndMac1; + // A non-null mac here implies this node is part of an address computation (the tree parent is + // GT_ADDR). + // If so, we need to pass the existing mac down to the child node. + // + // Otherwise, use a new mac. + if (subMac1 == nullptr) + { + subMac1 = &subIndMac1; + } break; default: break; @@ -11936,16 +11949,16 @@ GenTree* Compiler::fgMorphSmpOp(GenTree* tree, MorphAddrContext* mac) } } - // If gtOp1 is a GT_FIELD, we need to pass down the mac if - // its parent is GT_ADDR, since the address of the field + // If op1 is a GT_FIELD or indir, we need to pass down the mac if + // its parent is GT_ADDR, since the address of op1 // is part of an ongoing address computation. Otherwise // op1 represents the value of the field and so any address // calculations it does are in a new context. - if ((op1->gtOper == GT_FIELD) && (tree->gtOper != GT_ADDR)) + if (((op1->gtOper == GT_FIELD) || op1->OperIsIndir()) && (tree->gtOper != GT_ADDR)) { subMac1 = nullptr; - // The impact of this field's value to any ongoing + // The impact of op1's value to any ongoing // address computation is handled below when looking // at op2. } @@ -12046,11 +12059,11 @@ GenTree* Compiler::fgMorphSmpOp(GenTree* tree, MorphAddrContext* mac) break; } - // If gtOp2 is a GT_FIELD, we must be taking its value, + // If op2 is a GT_FIELD or indir, we must be taking its value, // so it should evaluate its address in a new context. - if (op2->gtOper == GT_FIELD) + if ((op2->gtOper == GT_FIELD) || op2->OperIsIndir()) { - // The impact of this field's value to any ongoing + // The impact of op2's value to any ongoing // address computation is handled above when looking // at op1. mac = nullptr; diff --git a/tests/src/JIT/Regression/JitBlue/GitHub_23791/GitHub_23791.cs b/tests/src/JIT/Regression/JitBlue/GitHub_23791/GitHub_23791.cs new file mode 100644 index 0000000..e724823 --- /dev/null +++ b/tests/src/JIT/Regression/JitBlue/GitHub_23791/GitHub_23791.cs @@ -0,0 +1,41 @@ +// 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; + +// The jit should null check 'this' in NextElement + +unsafe struct GitHub_23791 +{ + fixed byte A[10]; + + [MethodImpl(MethodImplOptions.NoInlining)] + byte NextElement(int i) => A[1+i]; + + static int Main() + { + int result = -1; + GitHub_23791* x = null; + bool threw = true; + + try + { + byte t = x->NextElement(100000); + threw = false; + } + catch (NullReferenceException) + { + result = 100; + } + + if (!threw) + { + Console.WriteLine($"FAIL: did not throw an exception"); + } + + return result; + } +} diff --git a/tests/src/JIT/Regression/JitBlue/GitHub_23791/GitHub_23791.csproj b/tests/src/JIT/Regression/JitBlue/GitHub_23791/GitHub_23791.csproj new file mode 100644 index 0000000..7198ed8 --- /dev/null +++ b/tests/src/JIT/Regression/JitBlue/GitHub_23791/GitHub_23791.csproj @@ -0,0 +1,34 @@ + + + + + Debug + AnyCPU + 2.0 + {2649FAFE-07BF-4F93-8120-BA9A69285ABB} + Exe + {786C830F-07A1-408B-BD7F-6EE04809D6DB};{FAE04EC0-301F-11D3-BF4B-00C04F79EFBC} + ..\..\ + + + + + + None + True + True + + + + False + + + + + + + + + + + -- 2.7.4