Skip to content

Commit

Permalink
Fix duplicated FldSeq in block morphing (#62687)
Browse files Browse the repository at this point in the history
* Fix duplicated FldSeq in block morphing

Reusing the address for the first field is problematic as
the first field is likely to be at a zero offset, thus a
zero-offset field sequence will be attached to it, and
subsequent copies of the same tree will pick it up, which
is incorrect.

Fix by reusing the address for the last field instead.

* Add a test

* Add a description of the issue to the test
  • Loading branch information
SingleAccretion committed Dec 16, 2021
1 parent 3fd8d40 commit d5f5950
Show file tree
Hide file tree
Showing 3 changed files with 71 additions and 4 deletions.
8 changes: 4 additions & 4 deletions src/coreclr/jit/morphblock.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -1259,9 +1259,9 @@ GenTree* MorphCopyBlockHelper::CopyFieldByField()
}
else
{
if (i == 0)
if (i == (fieldCnt - 1))
{
// Use the orginal m_dstAddr tree when i == 0
// Reuse the orginal m_dstAddr tree for the last field.
dstAddrClone = m_dstAddr;
}
else
Expand Down Expand Up @@ -1381,9 +1381,9 @@ GenTree* MorphCopyBlockHelper::CopyFieldByField()
}
else
{
if (i == 0)
if (i == (fieldCnt - 1))
{
// Use the orginal m_srcAddr tree when i == 0
// Reuse the orginal m_srcAddr tree for the last field.
srcAddrClone = m_srcAddr;
}
else
Expand Down
54 changes: 54 additions & 0 deletions src/tests/JIT/Directed/StructPromote/FldSeqsInPromotedCpBlk.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,54 @@
// Licensed to the .NET Foundation under one or more agreements.
// The .NET Foundation licenses this file to you under the MIT license.

using System.Runtime.CompilerServices;

// In this test, we have a block assignment with a source that is a promoted struct and
// an indirect destination. When morphing it, we would decompose that assignment into a series
// of field assignments: `IND(ADDR) = FirstLngValue; IND(ADDR_CLONE + 8) = SecondLngValue`.
// In the process, we would also attach field sequences to the destination addresses so that VN
// knew to analyze them. That was the part which was susceptible to the bug being tested: morphing
// reused the address node (the "firstElem" LCL_VAR) for the first field, and at the same time
// used it to create more copies for subsequent addresses.
//
// Thus:
// 1) A zero-offset field sequence for the first field was attached to ADDR
// 2) ADDR was cloned, the clone still had the sequence attached
// 3) ADD(ADDR_CLONE [FldSeq FirstLngValue], 8 [FldSeq SecondLngValue]) was created.
//
// And so we ended up with an incorrect FldSeq: [FirstLngValue, SecondLngValue], causing
// VN to wrongly treat the "firstElem = b" store as not modifiying SecondLngValue.
//
// The fix was to reuse the address for the last field instead.

class FldSeqsInPromotedCpBlk
{
public static int Main()
{
PromotableStruct s = default;
return Problem(new PromotableStruct[1]) == 2 ? 100 : 101;
}

[MethodImpl(MethodImplOptions.NoInlining)]
private static long Problem(PromotableStruct[] a)
{
ref var firstElem = ref a[0];

firstElem.SecondLngValue = 1;
var b = new PromotableStruct() { SecondLngValue = 2 };

if (firstElem.SecondLngValue == 1)
{
firstElem = b;
return firstElem.SecondLngValue;
}

return -1;
}
}

struct PromotableStruct
{
public long FirstLngValue;
public long SecondLngValue;
}
13 changes: 13 additions & 0 deletions src/tests/JIT/Directed/StructPromote/FldSeqsInPromotedCpBlk.csproj
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
<Project Sdk="Microsoft.NET.Sdk">
<PropertyGroup>
<OutputType>Exe</OutputType>
<CLRTestPriority>1</CLRTestPriority>
</PropertyGroup>
<PropertyGroup>
<DebugType>None</DebugType>
<Optimize>True</Optimize>
</PropertyGroup>
<ItemGroup>
<Compile Include="$(MSBuildProjectName).cs" />
</ItemGroup>
</Project>

0 comments on commit d5f5950

Please sign in to comment.