From 69a04fc694f52d415f5dbefae729f09c135bdf36 Mon Sep 17 00:00:00 2001 From: Brian Sullivan Date: Fri, 11 Dec 2020 12:58:06 -0800 Subject: [PATCH] Fix for Issue 44895 (#45284) * Fix for Issue 44895 - Fix: Don't allow an unwrapped promoted field of TYP_REF to be returned when we are expecting a TYP_STRUCT Backout change in gtGetStructHandleIfPresent for GT_RETURN as it isn't needed for this fix Deoptimize all GT_RETURN's with mismatched types for promoted struct fields. * Allow both GT_ADDR and GT_ASG as a parent node * Add second test case Repro2_44895.cs * Change assert about Incompatible types to be a noway_assert in gtNewTempAssign * Only use the smaller repro case for Runtime_44895.cs * Added noway_assert in release build for an assignment of a TYP_REF to a TYP_STRUCT * rerun jit-format --- src/coreclr/src/jit/gentree.cpp | 7 ++++ src/coreclr/src/jit/morph.cpp | 37 ++++++++++++++++--- .../JitBlue/Runtime_44895/Runtime_44895.cs | 37 +++++++++++++++++++ .../Runtime_44895/Runtime_44895.csproj | 10 +++++ 4 files changed, 85 insertions(+), 6 deletions(-) create mode 100644 src/tests/JIT/Regression/JitBlue/Runtime_44895/Runtime_44895.cs create mode 100644 src/tests/JIT/Regression/JitBlue/Runtime_44895/Runtime_44895.csproj diff --git a/src/coreclr/src/jit/gentree.cpp b/src/coreclr/src/jit/gentree.cpp index 3c1c1446e9280a..ef2a668abf0aeb 100644 --- a/src/coreclr/src/jit/gentree.cpp +++ b/src/coreclr/src/jit/gentree.cpp @@ -15512,6 +15512,13 @@ GenTree* Compiler::gtNewTempAssign( } #endif + // Added this noway_assert for runtime\issue 44895, to protect against silent bad codegen + // + if ((dstTyp == TYP_STRUCT) && (valTyp == TYP_REF)) + { + noway_assert(!"Incompatible types for gtNewTempAssign"); + } + // Floating Point assignments can be created during inlining // see "Zero init inlinee locals:" in fgInlinePrependStatements // thus we may need to set compFloatingPointUsed to true here. diff --git a/src/coreclr/src/jit/morph.cpp b/src/coreclr/src/jit/morph.cpp index b3547979766860..57910ff498d30d 100644 --- a/src/coreclr/src/jit/morph.cpp +++ b/src/coreclr/src/jit/morph.cpp @@ -17619,16 +17619,38 @@ void Compiler::fgMorphStructField(GenTree* tree, GenTree* parent) } // Access the promoted field as a field of a non-promoted struct with the same class handle. } -#ifdef DEBUG - else if (tree->TypeGet() == TYP_STRUCT) + else { - // The field tree accesses it as a struct, but the promoted lcl var for the field - // says that it has another type. It can happen only if struct promotion faked - // field type for a struct of single field of scalar type aligned at their natural boundary. + // As we already checked this above, we must have a tree with a TYP_STRUCT type + // + assert(tree->TypeGet() == TYP_STRUCT); + + // The field tree accesses it as a struct, but the promoted LCL_VAR field + // says that it has another type. This happens when struct promotion unwraps + // a single field struct to get to its ultimate type. + // + // Note that currently, we cannot have a promoted LCL_VAR field with a struct type. + // + // This mismatch in types can lead to problems for some parent node type like GT_RETURN. + // So we check the parent node and only allow this optimization when we have + // a GT_ADDR or a GT_ASG. + // + // Note that for a GT_ASG we have to do some additional work, + // see below after the SetOper(GT_LCL_VAR) + // + if (!parent->OperIs(GT_ADDR, GT_ASG)) + { + // Don't transform other operations such as GT_RETURN + // + return; + } +#ifdef DEBUG + // This is an additional DEBUG-only sanity check + // assert(structPromotionHelper != nullptr); structPromotionHelper->CheckRetypedAsScalar(field->gtFldHnd, fieldType); - } #endif // DEBUG + } } tree->SetOper(GT_LCL_VAR); @@ -17638,6 +17660,9 @@ void Compiler::fgMorphStructField(GenTree* tree, GenTree* parent) if (parent->gtOper == GT_ASG) { + // If we are changing the left side of an assignment, we need to set + // these two flags: + // if (parent->AsOp()->gtOp1 == tree) { tree->gtFlags |= GTF_VAR_DEF; diff --git a/src/tests/JIT/Regression/JitBlue/Runtime_44895/Runtime_44895.cs b/src/tests/JIT/Regression/JitBlue/Runtime_44895/Runtime_44895.cs new file mode 100644 index 00000000000000..3c33f894ddcf4f --- /dev/null +++ b/src/tests/JIT/Regression/JitBlue/Runtime_44895/Runtime_44895.cs @@ -0,0 +1,37 @@ +// Licensed to the .NET Foundation under one or more agreements. +// The .NET Foundation licenses this file to you under the MIT license. + +using System; + +public struct Text +{ + private readonly string _value; + public Text(string value) => _value = value; + public string Value => _value ?? string.Empty; +} + +public class TextProperty +{ + public Text GetValue(Text? a = null, Text? b = null, Text? c = null, Text? d = null) + { + if (a.HasValue) return a.Value; + if (b.HasValue) return b.Value; + if (c.HasValue) return c.Value; + if (d.HasValue) return d.Value; + return default; + } +} + +public class Repro +{ + public static int Main() + { + string test = "test"; + TextProperty t = new TextProperty(); + Text gv = t.GetValue(new Text(test)); + bool result = test.Equals(gv.Value); + Console.WriteLine(result ? "Pass" : "Fail"); + if (!result) Console.WriteLine($"got '{gv.Value}', expected '{test}'"); + return result ? 100 : -1; + } +} \ No newline at end of file diff --git a/src/tests/JIT/Regression/JitBlue/Runtime_44895/Runtime_44895.csproj b/src/tests/JIT/Regression/JitBlue/Runtime_44895/Runtime_44895.csproj new file mode 100644 index 00000000000000..1100f420532dc8 --- /dev/null +++ b/src/tests/JIT/Regression/JitBlue/Runtime_44895/Runtime_44895.csproj @@ -0,0 +1,10 @@ + + + Exe + None + True + + + + +