Assertion prop defers removing redundant bounds checks until processing
the parent comma, because the rewrite involves the comma as well. The
code currently has a bug in that its check for whether the bounds check
is redundant also occurs when processing reaches the comma, which is wrong
because the assertion set may then include assertions generated by the RHS
of the comma (there is code to compensate for the assertion generated by
the bounds check itself, which is the LHS of the comma, but no compensation
for the RHS).
This change moves the analysis of whether bounds checks are redundant to
the proper spot in the processing order, and delays only the rewrite to
the processing of the comma; the redundancy of the bounds check is
communicated via a new opcode-specific flag: GTF_ARR_BOUND_INBND.
Fixes dotnet/coreclr#6318.
Commit migrated from https://github.com/dotnet/coreclr/commit/
16c65845eef41548c5653f14fede5a8c5c0be30f
const GenTreePtr tree,
const GenTreePtr stmt)
{
- // Process the bounds check as part of the GT_COMMA node since, we need parent pointer to remove nodes.
- if (tree->gtGetOp1()->OperGet() == GT_ARR_BOUNDS_CHECK)
- {
- // Since the GT_COMMA tree gets processed by assertion prop after the child GT_ARR_BOUNDS_CHECK
- // node in execution order, bounds check assertions will be included for the parent GT_COMMA node.
- // Remove the assertion made by the bounds check tree about itself. Assertion only applies to
- // "future" bounds checks.
- AssertionIndex index = (AssertionIndex)tree->gtGetOp1()->GetAssertion();
- if (index != NO_ASSERTION_INDEX && optGetAssertion(index)->IsBoundsCheckNoThrow())
- {
- BitVecOps::RemoveElemD(apTraits, assertions, index - 1);
- GenTreePtr newTree = optAssertionProp_BndsChk(assertions, tree, stmt);
- BitVecOps::AddElemD(apTraits, assertions, index - 1);
- return newTree;
- }
- else
- {
- return optAssertionProp_BndsChk(assertions, tree, stmt);
- }
+ // Remove the bounds check as part of the GT_COMMA node since we need parent pointer to remove nodes.
+ // When processing visits the bounds check, it sets the throw kind to None if the check is redundant.
+ if ((tree->gtGetOp1()->OperGet() == GT_ARR_BOUNDS_CHECK)
+ && ((tree->gtGetOp1()->gtFlags & GTF_ARR_BOUND_INBND) != 0))
+ {
+ optRemoveRangeCheck(tree, stmt, true, GTF_ASG, true /* force remove */);
+ return optAssertionProp_Update(tree, tree, stmt);
}
return nullptr;
}
return nullptr;
}
- assert(tree->gtOper == GT_COMMA && tree->gtGetOp1()->OperGet() == GT_ARR_BOUNDS_CHECK);
+ assert(tree->gtOper == GT_ARR_BOUNDS_CHECK);
BitVecOps::Iter iter(apTraits, assertions);
unsigned index = 0;
continue;
}
- GenTreeBoundsChk* arrBndsChk = tree->gtGetOp1()->AsBoundsChk();
+ GenTreeBoundsChk* arrBndsChk = tree->AsBoundsChk();
// Set 'isRedundant' to true if we can determine that 'arrBndsChk' can be
// classified as a redundant bounds check using 'curAssertion'
gtDispTree(tree, 0, nullptr, true);
}
#endif
- optRemoveRangeCheck(tree, stmt, true, GTF_ASG, true /* force remove */);
- GenTreePtr newTree = optAssertionProp_Update(tree, tree, stmt);
- if (newTree != nullptr)
- {
- BitVecOps::RemoveElemD(apTraits, assertions, index - 1);
- newTree = optAssertionProp(assertions, tree, stmt);
- BitVecOps::AddElemD(apTraits, assertions, index - 1);
- return newTree;
- }
+
+ // Defer actually removing the tree until processing reaches its parent comma, since
+ // optRemoveRangeCheck needs to rewrite the whole comma tree.
+ arrBndsChk->gtFlags |= GTF_ARR_BOUND_INBND;
+ return nullptr;
}
return nullptr;
}
case GT_NULLCHECK:
return optAssertionProp_Ind(assertions, tree, stmt);
+ case GT_ARR_BOUNDS_CHECK:
+ return optAssertionProp_BndsChk(assertions, tree, stmt);
+
case GT_COMMA:
return optAssertionProp_Comma(assertions, tree, stmt);
#define GTF_NO_OP_NO 0x80000000 // GT_NO_OP --Have the codegenerator generate a special nop
+ #define GTF_ARR_BOUND_INBND 0x80000000 // GT_ARR_BOUNDS_CHECK -- have proved this check is always in-bounds
+
//----------------------------------------------------------------
#define GTF_STMT_CMPADD 0x80000000 // GT_STMT -- added by compiler
--- /dev/null
+// 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.Runtime.CompilerServices;
+using System.Numerics;
+
+namespace N
+{
+ public static class C
+ {
+ public static int Main(string[] args)
+ {
+ // Regression test for an issue with assertion prop leading
+ // to the wrong exception being thrown from Vector<T>.CopyTo
+ try
+ {
+ Foo(Vector<int>.Zero);
+ }
+ catch (System.ArgumentOutOfRangeException)
+ {
+ // Caught the right exception
+ return 100;
+ }
+ catch
+ {
+ // Caught the wrong exception
+ return -1;
+ }
+ // Caught no exception
+ return -2;
+ }
+
+ [MethodImpl(MethodImplOptions.NoInlining)]
+ static int Foo(Vector<int> vec)
+ {
+ int[] a = new int[5];
+ // The index [5] is outside the bounds of array 'a',
+ // so this should throw ArgumentOutOfRangeException.
+ // There's a subsequent check for whether the destination
+ // has enough space to receive the vector, which would
+ // raise an ArgumentException; the bug was that assertion
+ // prop was using the later exception check to prove the
+ // prior one "redundant" because the commas confused the
+ // ordering.
+ vec.CopyTo(a, 5);
+ return a[0];
+ }
+ }
+}
--- /dev/null
+<?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>
+ <AssemblyName>$(MSBuildProjectName)</AssemblyName>
+ <SchemaVersion>2.0</SchemaVersion>
+ <ProjectGuid>{80B4796D-0D4C-46A3-9185-6EEA11DD4090}</ProjectGuid>
+ <OutputType>Exe</OutputType>
+ <ProjectTypeGuids>{786C830F-07A1-408B-BD7F-6EE04809D6DB};{FAE04EC0-301F-11D3-BF4B-00C04F79EFBC}</ProjectTypeGuids>
+ <SolutionDir Condition="$(SolutionDir) == '' Or $(SolutionDir) == '*Undefined*'">..\..\</SolutionDir>
+
+ <NuGetPackageImportStamp>7a9bfb7d</NuGetPackageImportStamp>
+ </PropertyGroup>
+ <!-- Default configurations to help VS understand the configurations -->
+ <PropertyGroup Condition=" '$(Configuration)|$(Platform)' == 'Debug|AnyCPU' ">
+ </PropertyGroup>
+ <PropertyGroup Condition=" '$(Configuration)|$(Platform)' == 'Release|AnyCPU' ">
+ </PropertyGroup>
+ <PropertyGroup>
+ <DebugType></DebugType>
+ <Optimize>True</Optimize>
+ </PropertyGroup>
+ <ItemGroup>
+ <Compile Include="$(MSBuildProjectName).cs" />
+ </ItemGroup>
+ <ItemGroup>
+ <None Include="$(JitPackagesConfigFileDirectory)threading+thread\project.json" />
+ <None Include="app.config" />
+ </ItemGroup>
+ <ItemGroup>
+ <Service Include="{82A7F48D-3B50-4B1E-B82E-3ADA8210C358}" />
+ </ItemGroup>
+ <PropertyGroup>
+ <ProjectJson>$(JitPackagesConfigFileDirectory)threading+thread\project.json</ProjectJson>
+ <ProjectLockJson>$(JitPackagesConfigFileDirectory)threading+thread\project.lock.json</ProjectLockJson>
+ </PropertyGroup>
+ <Import Project="$([MSBuild]::GetDirectoryNameOfFileAbove($(MSBuildThisFileDirectory), dir.targets))\dir.targets" />
+ <PropertyGroup Condition=" '$(MsBuildProjectDirOverride)' != '' ">
+ </PropertyGroup>
+</Project>
--- /dev/null
+<?xml version="1.0" encoding="utf-8"?>
+<configuration>
+ <runtime>
+ <assemblyBinding xmlns="urn:schemas-microsoft-com:asm.v1">
+ <dependentAssembly>
+ <assemblyIdentity name="System.Runtime" publicKeyToken="b03f5f7f11d50a3a" culture="neutral" />
+ <bindingRedirect oldVersion="0.0.0.0-4.0.20.0" newVersion="4.0.20.0" />
+ </dependentAssembly>
+ <dependentAssembly>
+ <assemblyIdentity name="System.Text.Encoding" publicKeyToken="b03f5f7f11d50a3a" culture="neutral" />
+ <bindingRedirect oldVersion="0.0.0.0-4.0.10.0" newVersion="4.0.10.0" />
+ </dependentAssembly>
+ <dependentAssembly>
+ <assemblyIdentity name="System.Threading.Tasks" publicKeyToken="b03f5f7f11d50a3a" culture="neutral" />
+ <bindingRedirect oldVersion="0.0.0.0-4.0.10.0" newVersion="4.0.10.0" />
+ </dependentAssembly>
+ <dependentAssembly>
+ <assemblyIdentity name="System.IO" publicKeyToken="b03f5f7f11d50a3a" culture="neutral" />
+ <bindingRedirect oldVersion="0.0.0.0-4.0.10.0" newVersion="4.0.10.0" />
+ </dependentAssembly>
+ <dependentAssembly>
+ <assemblyIdentity name="System.Reflection" publicKeyToken="b03f5f7f11d50a3a" culture="neutral" />
+ <bindingRedirect oldVersion="0.0.0.0-4.0.10.0" newVersion="4.0.10.0" />
+ </dependentAssembly>
+ </assemblyBinding>
+ </runtime>
+</configuration>
projects / platform / upstream / dotnet / runtime.git / commitdiff