From c4e93e277b8f5102dd3f854f61e2fbc1ae9c510e Mon Sep 17 00:00:00 2001 From: SingleAccretion Date: Sat, 11 Dec 2021 22:42:08 +0300 Subject: [PATCH 1/3] 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. --- src/coreclr/jit/morphblock.cpp | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/coreclr/jit/morphblock.cpp b/src/coreclr/jit/morphblock.cpp index a8482726a5da2..3511c878827ef 100644 --- a/src/coreclr/jit/morphblock.cpp +++ b/src/coreclr/jit/morphblock.cpp @@ -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 @@ -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 From f48008b0385d6c274579dfd3cf0980b72d807971 Mon Sep 17 00:00:00 2001 From: SingleAccretion Date: Sun, 12 Dec 2021 16:10:34 +0300 Subject: [PATCH 2/3] Add a test --- .../StructPromote/FldSeqsInPromotedCpBlk.cs | 36 +++++++++++++++++++ .../FldSeqsInPromotedCpBlk.csproj | 13 +++++++ 2 files changed, 49 insertions(+) create mode 100644 src/tests/JIT/Directed/StructPromote/FldSeqsInPromotedCpBlk.cs create mode 100644 src/tests/JIT/Directed/StructPromote/FldSeqsInPromotedCpBlk.csproj diff --git a/src/tests/JIT/Directed/StructPromote/FldSeqsInPromotedCpBlk.cs b/src/tests/JIT/Directed/StructPromote/FldSeqsInPromotedCpBlk.cs new file mode 100644 index 0000000000000..80eb6829edc98 --- /dev/null +++ b/src/tests/JIT/Directed/StructPromote/FldSeqsInPromotedCpBlk.cs @@ -0,0 +1,36 @@ +// 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; + +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; +} diff --git a/src/tests/JIT/Directed/StructPromote/FldSeqsInPromotedCpBlk.csproj b/src/tests/JIT/Directed/StructPromote/FldSeqsInPromotedCpBlk.csproj new file mode 100644 index 0000000000000..5d8fe22529764 --- /dev/null +++ b/src/tests/JIT/Directed/StructPromote/FldSeqsInPromotedCpBlk.csproj @@ -0,0 +1,13 @@ + + + Exe + 1 + + + None + True + + + + + From 7dde072708eaba7f55a1aee7a7c5df8f3e7977f0 Mon Sep 17 00:00:00 2001 From: SingleAccretion Date: Thu, 16 Dec 2021 20:24:17 +0300 Subject: [PATCH 3/3] Add a description of the issue to the test --- .../StructPromote/FldSeqsInPromotedCpBlk.cs | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+) diff --git a/src/tests/JIT/Directed/StructPromote/FldSeqsInPromotedCpBlk.cs b/src/tests/JIT/Directed/StructPromote/FldSeqsInPromotedCpBlk.cs index 80eb6829edc98..65be5229e25eb 100644 --- a/src/tests/JIT/Directed/StructPromote/FldSeqsInPromotedCpBlk.cs +++ b/src/tests/JIT/Directed/StructPromote/FldSeqsInPromotedCpBlk.cs @@ -3,6 +3,24 @@ 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()