diff options
author | Eugene Rozenfeld <erozen@microsoft.com> | 2019-02-14 14:43:26 -0800 |
---|---|---|
committer | GitHub <noreply@github.com> | 2019-02-14 14:43:26 -0800 |
commit | 6c9ad257e78977c9fd5d2429490b5d2f72c1b612 (patch) | |
tree | cb52a07d50601f0255c2d537552689435defe084 | |
parent | 171be182943cd4633d5408c5b36b4cf56eefe9e1 (diff) | |
download | coreclr-6c9ad257e78977c9fd5d2429490b5d2f72c1b612.tar.gz coreclr-6c9ad257e78977c9fd5d2429490b5d2f72c1b612.tar.bz2 coreclr-6c9ad257e78977c9fd5d2429490b5d2f72c1b612.zip |
Fix check for memory containment safety. (#22563)
This change ensures that if an operand can produce an exception
and any instructions executed after the operand evaluation but before
the operand's parent can also produce an exception, the operand
shouldn't be contained. The reason is that in this case operand
containment may reorder exceptions.
With `strict` set to true the containment is blocked here:
https://github.com/dotnet/coreclr/blob/d27fff3f65193dd71c6197e9876101f496bbd28b/src/jit/sideeffects.cpp#L485-L488
Also, make the check for ordering side-effect interference less
conservative.
Fixes #22556.
-rw-r--r-- | src/jit/lower.cpp | 3 | ||||
-rw-r--r-- | src/jit/sideeffects.cpp | 12 | ||||
-rw-r--r-- | tests/src/JIT/Regression/JitBlue/GitHub_22556/GitHub_22556.cs | 46 | ||||
-rw-r--r-- | tests/src/JIT/Regression/JitBlue/GitHub_22556/GitHub_22556.csproj | 34 |
4 files changed, 91 insertions, 4 deletions
diff --git a/src/jit/lower.cpp b/src/jit/lower.cpp index 2c4c071a35..fc507c4c50 100644 --- a/src/jit/lower.cpp +++ b/src/jit/lower.cpp @@ -87,7 +87,8 @@ bool Lowering::IsSafeToContainMem(GenTree* parentNode, GenTree* childNode) for (GenTree* node = childNode->gtNext; node != parentNode; node = node->gtNext) { - if (m_scratchSideEffects.InterferesWith(comp, node, false)) + const bool strict = true; + if (m_scratchSideEffects.InterferesWith(comp, node, strict)) { return false; } diff --git a/src/jit/sideeffects.cpp b/src/jit/sideeffects.cpp index 75b7cc7b7f..10fa5d5c49 100644 --- a/src/jit/sideeffects.cpp +++ b/src/jit/sideeffects.cpp @@ -450,7 +450,7 @@ void SideEffectSet::AddNode(Compiler* compiler, GenTree* node) // Two side effect sets interfere under any of the following // conditions: // - If the analysis is strict, and: -// - Either set contains a compiler barrier, or +// - One set contains a compiler barrier and the other set contains a global reference, or // - Both sets produce an exception // - Whether or not the analysis is strict: // - One set produces an exception and the other set contains a @@ -475,8 +475,14 @@ bool SideEffectSet::InterferesWith(unsigned otherSideEffectFlags, if (strict) { - // If either set contains a compiler barrier, the sets interfere. - if (((m_sideEffectFlags | otherSideEffectFlags) & GTF_ORDER_SIDEEFF) != 0) + // If either set contains a compiler barrier, and the other set contains a global reference, + // the sets interfere. + if (((m_sideEffectFlags & GTF_ORDER_SIDEEFF) != 0) && ((otherSideEffectFlags & GTF_GLOB_REF) != 0)) + { + return true; + } + + if (((otherSideEffectFlags & GTF_ORDER_SIDEEFF) != 0) && ((m_sideEffectFlags & GTF_GLOB_REF) != 0)) { return true; } diff --git a/tests/src/JIT/Regression/JitBlue/GitHub_22556/GitHub_22556.cs b/tests/src/JIT/Regression/JitBlue/GitHub_22556/GitHub_22556.cs new file mode 100644 index 0000000000..e66288b244 --- /dev/null +++ b/tests/src/JIT/Regression/JitBlue/GitHub_22556/GitHub_22556.cs @@ -0,0 +1,46 @@ +// 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 Test +{ + int f; + + Test(int f) + { + this.f = f; + } + + public static int Main() + { + try + { + Add(null, 0); + } + catch (Exception e) + { + if (e is NullReferenceException) + { + Console.WriteLine("PASS"); + return 100; + } + } + Console.WriteLine("FAIL"); + return -1; + } + + [MethodImpl(MethodImplOptions.NoInlining)] + static int Add(Test t, int i) + { + // When t is null and i is 0, this should throw + // NullReferenceException since the operands of + // addition have to be evaluated left to right. + int x = t.f + 1 / i; + return x; + } +} + diff --git a/tests/src/JIT/Regression/JitBlue/GitHub_22556/GitHub_22556.csproj b/tests/src/JIT/Regression/JitBlue/GitHub_22556/GitHub_22556.csproj new file mode 100644 index 0000000000..c46f1c3549 --- /dev/null +++ b/tests/src/JIT/Regression/JitBlue/GitHub_22556/GitHub_22556.csproj @@ -0,0 +1,34 @@ +<?xml version="1.0" encoding="utf-8"?> +<Project ToolsVersion="12.0" DefaultTargets="Build" xmlns="http://schemas.microsoft.com/developer/msbuild/2003"> + <Import Project="$([MSBuild]::GetDirectoryNameOfFileAbove($(MSBuildThisFileDirectory), dir.props))\dir.props" /> + <PropertyGroup> + <Configuration Condition=" '$(Configuration)' == '' ">Debug</Configuration> + <Platform Condition=" '$(Platform)' == '' ">AnyCPU</Platform> + <SchemaVersion>2.0</SchemaVersion> + <ProjectGuid>{2649FAFE-07BF-4F93-8120-BA9A69285ABB}</ProjectGuid> + <OutputType>Exe</OutputType> + <ProjectTypeGuids>{786C830F-07A1-408B-BD7F-6EE04809D6DB};{FAE04EC0-301F-11D3-BF4B-00C04F79EFBC}</ProjectTypeGuids> + <SolutionDir Condition="$(SolutionDir) == '' Or $(SolutionDir) == '*Undefined*'">..\..\</SolutionDir> + </PropertyGroup> + <!-- Default configurations to help VS understand the configurations --> + <PropertyGroup Condition=" '$(Configuration)|$(Platform)' == 'Debug|AnyCPU' "></PropertyGroup> + <PropertyGroup Condition=" '$(Configuration)|$(Platform)' == 'Release|AnyCPU' "></PropertyGroup> + <PropertyGroup> + <LangVersion>latest</LangVersion> + <DebugType>None</DebugType> + <Optimize>True</Optimize> + </PropertyGroup> + <ItemGroup> + <CodeAnalysisDependentAssemblyPaths Condition=" '$(VS100COMNTOOLS)' != '' " Include="$(VS100COMNTOOLS)..\IDE\PrivateAssemblies"> + <Visible>False</Visible> + </CodeAnalysisDependentAssemblyPaths> + </ItemGroup> + <ItemGroup> + <Service Include="{82A7F48D-3B50-4B1E-B82E-3ADA8210C358}" /> + </ItemGroup> + <ItemGroup> + <Compile Include="$(MSBuildProjectName).cs" /> + </ItemGroup> + <Import Project="$([MSBuild]::GetDirectoryNameOfFileAbove($(MSBuildThisFileDirectory), dir.targets))\dir.targets" /> + <PropertyGroup Condition=" '$(MsBuildProjectDirOverride)' != '' "></PropertyGroup> +</Project>
\ No newline at end of file |