Skip to content
This repository has been archived by the owner on Jan 23, 2023. It is now read-only.

Commit

Permalink
fix checkLclVarSemantics (#14053)
Browse files Browse the repository at this point in the history
* extract CheckLclVarSemantics from CheckLIR.

* add a test that shows the silent bad execution.

* fix the checker.

* add the test to the exclude list.

* rename consumed to used
  • Loading branch information
Sergey Andreenko authored Sep 22, 2017
1 parent 8c33e99 commit 43cf34f
Show file tree
Hide file tree
Showing 7 changed files with 278 additions and 72 deletions.
10 changes: 0 additions & 10 deletions src/jit/gentree.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -15663,16 +15663,6 @@ bool GenTree::isContained() const
assert(!isMarkedContained);
}

#if !defined(_TARGET_64BIT_)
if (OperGet() == GT_LONG)
{
// GT_LONG nodes are normally contained. The only exception is when the result
// of a TYP_LONG operation is not used and this can only happen if the GT_LONG
// has no parent.
assert(isMarkedContained || (gtGetParent(nullptr) == nullptr));
}
#endif

// if it's contained it better have a user
if (isMarkedContained)
{
Expand Down
139 changes: 101 additions & 38 deletions src/jit/lir.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -1406,6 +1406,105 @@ LIR::ReadOnlyRange LIR::Range::GetRangeOfOperandTrees(GenTree* root, bool* isClo

#ifdef DEBUG

//------------------------------------------------------------------------
// CheckLclVarSemanticsHelper checks lclVar semantics.
//
// Specifically, ensure that an unaliasable lclVar is not redefined between the
// point at which a use appears in linear order and the point at which it is used by its user.
// This ensures that it is always safe to treat a lclVar use as happening at the user (rather than at
// the lclVar node).
class CheckLclVarSemanticsHelper
{
public:
//------------------------------------------------------------------------
// CheckLclVarSemanticsHelper constructor: Init arguments for the helper.
//
// This needs unusedDefs because unused lclVar reads may otherwise appear as outstanding reads
// and produce false indications that a write to a lclVar occurs while outstanding reads of that lclVar
// exist.
//
// Arguments:
// compiler - A compiler context.
// range - a range to do the check.
// unusedDefs - map of defs that do no have users.
//
CheckLclVarSemanticsHelper(Compiler* compiler,
const LIR::Range* range,
SmallHashTable<GenTreePtr, bool, 32U>& unusedDefs)
: compiler(compiler), range(range), unusedDefs(unusedDefs), unusedLclVarReads(compiler)
{
}

//------------------------------------------------------------------------
// Check: do the check.
// Return Value:
// 'true' if the Local variables semantics for the specified range is legal.
bool Check()
{
for (GenTreePtr node : *range)
{
if (!node->isContained()) // a contained node reads operands in the parent.
{
UseNodeOperands(node);
}

AliasSet::NodeInfo nodeInfo(compiler, node);
if (nodeInfo.IsLclVarRead() && !unusedDefs.Contains(node))
{
int count = 0;
unusedLclVarReads.TryGetValue(nodeInfo.LclNum(), &count);
unusedLclVarReads.AddOrUpdate(nodeInfo.LclNum(), count + 1);
}

// If this node is a lclVar write, it must be to a lclVar that does not have an outstanding read.
assert(!nodeInfo.IsLclVarWrite() || !unusedLclVarReads.Contains(nodeInfo.LclNum()));
}

return true;
}

private:
//------------------------------------------------------------------------
// UseNodeOperands: mark the node's operands as used.
//
// Arguments:
// node - the node to use operands from.
void UseNodeOperands(GenTreePtr node)
{
for (GenTreePtr operand : node->Operands())
{
if (!operand->IsLIR())
{
// ARGPLACE nodes are not represented in the LIR sequence. Ignore them.
assert(operand->OperIs(GT_ARGPLACE));
continue;
}
if (operand->isContained())
{
UseNodeOperands(operand);
}
AliasSet::NodeInfo operandInfo(compiler, operand);
if (operandInfo.IsLclVarRead())
{
int count;
const bool removed = unusedLclVarReads.TryRemove(operandInfo.LclNum(), &count);
assert(removed);

if (count > 1)
{
unusedLclVarReads.AddOrUpdate(operandInfo.LclNum(), count - 1);
}
}
}
}

private:
Compiler* compiler;
const LIR::Range* range;
SmallHashTable<GenTree*, bool, 32U>& unusedDefs;
SmallHashTable<int, int, 32U> unusedLclVarReads;
};

//------------------------------------------------------------------------
// LIR::Range::CheckLIR: Performs a set of correctness checks on the LIR
// contained in this range.
Expand Down Expand Up @@ -1561,44 +1660,8 @@ bool LIR::Range::CheckLIR(Compiler* compiler, bool checkUnusedValues) const
}
}

// Check lclVar semantics: specifically, ensure that an unaliasable lclVar is not redefined between the
// point at which a use appears in linear order and the point at which that use is consumed by its user.
// This ensures that it is always safe to treat a lclVar use as happening at the user (rather than at
// the lclVar node).
//
// This happens as a second pass because unused lclVar reads may otherwise appear as outstanding reads
// and produce false indications that a write to a lclVar occurs while outstanding reads of that lclVar
// exist.
SmallHashTable<int, int, 32> unconsumedLclVarReads(compiler);
for (GenTree* node : *this)
{
for (GenTree* operand : node->Operands())
{
AliasSet::NodeInfo operandInfo(compiler, operand);
if (operandInfo.IsLclVarRead())
{
int count;
const bool removed = unconsumedLclVarReads.TryRemove(operandInfo.LclNum(), &count);
assert(removed);

if (count > 1)
{
unconsumedLclVarReads.AddOrUpdate(operandInfo.LclNum(), count - 1);
}
}
}

AliasSet::NodeInfo nodeInfo(compiler, node);
if (nodeInfo.IsLclVarRead() && !unusedDefs.Contains(node))
{
int count = 0;
unconsumedLclVarReads.TryGetValue(nodeInfo.LclNum(), &count);
unconsumedLclVarReads.AddOrUpdate(nodeInfo.LclNum(), count + 1);
}

// If this node is a lclVar write, it must be to a lclVar that does not have an outstanding read.
assert(!nodeInfo.IsLclVarWrite() || !unconsumedLclVarReads.Contains(nodeInfo.LclNum()));
}
CheckLclVarSemanticsHelper checkLclVarSemanticsHelper(compiler, this, unusedDefs);
assert(checkLclVarSemanticsHelper.Check());

return true;
}
Expand Down
16 changes: 4 additions & 12 deletions src/jit/lsraarm.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -441,19 +441,11 @@ void LinearScan::TreeNodeInfoInit(GenTree* tree)
break;

case GT_LONG:
if (tree->IsUnusedValue())
{
// An unused GT_LONG node needs to consume its sources.
info->srcCount = 2;
info->dstCount = 0;
}
else
{
// Passthrough. Should have been marked contained.
info->srcCount = 0;
assert(info->dstCount == 0);
}
assert(tree->IsUnusedValue()); // Contained nodes are already processed, only unused GT_LONG can reach here.

// An unused GT_LONG node needs to consume its sources.
info->srcCount = 2;
info->dstCount = 0;
break;

case GT_CNS_DBL:
Expand Down
17 changes: 5 additions & 12 deletions src/jit/lsraxarch.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -193,18 +193,11 @@ void LinearScan::TreeNodeInfoInit(GenTree* tree)
#if !defined(_TARGET_64BIT_)

case GT_LONG:
if (tree->IsUnusedValue())
{
// An unused GT_LONG node needs to consume its sources.
info->srcCount = 2;
info->dstCount = 0;
}
else
{
// Passthrough. Should have been marked contained.
info->srcCount = 0;
assert(info->dstCount == 0);
}
assert(tree->IsUnusedValue()); // Contained nodes are already processed, only unused GT_LONG can reach here.

// An unused GT_LONG node needs to consume its sources.
info->srcCount = 2;
info->dstCount = 0;
break;

#endif // !defined(_TARGET_64BIT_)
Expand Down
3 changes: 3 additions & 0 deletions tests/issues.targets
Original file line number Diff line number Diff line change
Expand Up @@ -184,6 +184,9 @@
<ExcludeList Include="$(XunitTestBinBase)\Loader\classloader\TypeGeneratorTests\TypeGeneratorTest683\Generated683\*">
<Issue>6707</Issue>
</ExcludeList>
<ExcludeList Include="$(XunitTestBinBase)\JIT\Regression\JitBlue\GitHub_13910\GitHub_13910\GitHub_13910.cmd">
<Issue>13910</Issue>
</ExcludeList>
</ItemGroup>

<!-- The following are x86 failures -->
Expand Down
120 changes: 120 additions & 0 deletions tests/src/JIT/Regression/JitBlue/GitHub_13910/GitHub_13910.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,120 @@
// 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.InteropServices;

// Represents a problem with contained nodes chains, that contain lclVar reads, that were moved through lclVar stores.
// Notice that the project file sets complus_JitStressModeNames.

[StructLayout(LayoutKind.Explicit)]
internal class AA
{

[FieldOffset(8)]
public QQ q;


public static AA[] a_init = new AA[101];

public static AA[] a_zero = new AA[101];


public AA(int qq)
{

this.q = new QQ(qq);
}


public static void reset()
{
AA.a_init[100] = new AA(1);
AA.a_zero[100] = new AA(2);
}
}

internal class QQ
{
public int val;

public QQ(int vv)
{
this.val = vv;
}

public int ret_code()
{
return 100;
}
}

internal class TestApp
{

private static int test_2_2(int num)
{
int result;
if (AA.a_init[num].q != AA.a_zero[num].q)
// Access field with contained IND instruction.
// EQ marks its operands as contained too.
// AA.a_init[num].q and AA.a_zero[num].q are allocated to the same lclVar.
// So we calculate AA.a_init[num].q and store as tmp0, use this temp to do nullCheck.
// Then store AA.a_zero[num].q as tmp0, destroy the old value and try to do EQ thinking that
// tmp0 is AA.a_init[num].q.
// It needs stress (complus_JitStressModeNames=STRESS_NULL_OBJECT_CHECK, STRESS_MAKE_CSE)
// to force the compiler to do implicit null checks and store values as local variables.
// Bad IL example, t53 is set as contained, t143 is set as contained, it means they will be calculated as part of their parent t9.
// But at that moment V02, that is read in t143 is already modified by [000056].
// N035 ( 1, 1) [000035] ------------ t35 = LCL_VAR ref V02 tmp0 u:3 eax (last use) REG eax <l:$149, c:$182>
// /--* t35 ref
// N037 (???,???) [000143] -c---------- t143 = * LEA(b+12) byref REG NA
// /--* t143 byref
// N039 ( 4, 4) [000054] Rc---O------ t54 = * IND ref REG NA <l:$155, c:$184> // This contained flag is invalid because
// // the value will be read after the store 000056.
// *********************************************************************************************
// /--* t117 ref
// N073 ( 18, 22) [000056] DA-XG------- * STORE_LCL_VAR ref V02 tmp0 d:4 eax REG eax // the store that corrupts t54 value.
// N075 ( 1, 1) [000057] ------------ t57 = LCL_VAR ref V02 tmp0 u:4 eax REG eax <l:$160, c:$187>
// /--* t57 ref
// N077 ( 2, 2) [000058] ---X---N---- * NULLCHECK byte REG NA <l:$166, c:$165>
// N079 ( 1, 1) [000060] ------------ t60 = LCL_VAR ref V02 tmp0 u:4 eax REG eax <l:$160, c:$187>
// /--* t60 ref
// N081 (???,???) [000146] -c---------- t146 = * LEA(b+12) byref REG NA
// /--* t146 byref
// N083 ( 4, 4) [000079] R----O------ t79 = * IND ref REG ecx <l:$16b, c:$189>
// /--* t79 ref
// N085 ( 4, 4) [000121] DA---O------ * STORE_LCL_VAR ref V07 cse1 ecx REG ecx
// N087 ( 1, 1) [000122] ------------ t122 = LCL_VAR ref V07 cse1 ecx REG ecx <l:$16b, c:$189>
// /--* t54 ref // reads V02.
// +--* t122 ref
// N089 ( 48, 56) [000009] J--XGO-N---- t9 = * EQ int REG NA <l:$1c5, c:$1c4>
{
result = 100;
}
else
{
result = AA.a_zero[num].q.val;
}
return result;
}

private static int Main()
{
AA.reset();
int result;

int r = TestApp.test_2_2(100);
if (r != 100)
{
Console.WriteLine("Failed.");
result = 101;
}
else
{
Console.WriteLine("Passed.");
result = 100;
}
return result;
}
}
45 changes: 45 additions & 0 deletions tests/src/JIT/Regression/JitBlue/GitHub_13910/GitHub_13910.csproj
Original file line number Diff line number Diff line change
@@ -0,0 +1,45 @@
<?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>{E2C84853-0100-4C5D-88C0-355F70483779}</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>
<ItemGroup>
<CodeAnalysisDependentAssemblyPaths Condition=" '$(VS100COMNTOOLS)' != '' " Include="$(VS100COMNTOOLS)..\IDE\PrivateAssemblies">
<Visible>False</Visible>
</CodeAnalysisDependentAssemblyPaths>
</ItemGroup>
<PropertyGroup>
<DebugType></DebugType>
<Optimize>True</Optimize>
</PropertyGroup>
<ItemGroup>
<Compile Include="$(MSBuildProjectName).cs" />
</ItemGroup>
<ItemGroup>
<Service Include="{82A7F48D-3B50-4B1E-B82E-3ADA8210C358}" />
</ItemGroup>
<PropertyGroup>
<!-- Set JitStreess variables, Linux requires them to be properly capitalized. -->
<CLRTestBatchPreCommands><![CDATA[
$(CLRTestBatchPreCommands)
set COMPlus_JitStressModeNames=STRESS_NULL_OBJECT_CHECK, STRESS_MAKE_CSE
]]></CLRTestBatchPreCommands>
<BashCLRTestPreCommands><![CDATA[
$(BashCLRTestPreCommands)
export COMPlus_JitStressModeNames=STRESS_NULL_OBJECT_CHECK, STRESS_MAKE_CSE
]]></BashCLRTestPreCommands>
</PropertyGroup>
<Import Project="$([MSBuild]::GetDirectoryNameOfFileAbove($(MSBuildThisFileDirectory), dir.targets))\dir.targets" />
<PropertyGroup Condition=" '$(MsBuildProjectDirOverride)' != '' "></PropertyGroup>
</Project>

0 comments on commit 43cf34f

Please sign in to comment.