summaryrefslogtreecommitdiff
diff options
context:
space:
mode:
authorEugene Rozenfeld <erozen@microsoft.com>2019-02-14 14:43:26 -0800
committerGitHub <noreply@github.com>2019-02-14 14:43:26 -0800
commit6c9ad257e78977c9fd5d2429490b5d2f72c1b612 (patch)
treecb52a07d50601f0255c2d537552689435defe084
parent171be182943cd4633d5408c5b36b4cf56eefe9e1 (diff)
downloadcoreclr-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.cpp3
-rw-r--r--src/jit/sideeffects.cpp12
-rw-r--r--tests/src/JIT/Regression/JitBlue/GitHub_22556/GitHub_22556.cs46
-rw-r--r--tests/src/JIT/Regression/JitBlue/GitHub_22556/GitHub_22556.csproj34
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