From: Sergey Andreenko Date: Tue, 9 Oct 2018 22:03:19 +0000 (-0700) Subject: Do not promote struct field with type mistmatch. (dotnet/coreclr#20085) X-Git-Tag: submit/tizen/20210909.063632~11030^2~3734 X-Git-Url: http://review.tizen.org/git/?a=commitdiff_plain;h=5eddbd3a25afce95dc287bf7d702492f944c8c46;p=platform%2Fupstream%2Fdotnet%2Fruntime.git Do not promote struct field with type mistmatch. (dotnet/coreclr#20085) * Add a test that reads int field as double. It fails in `void CodeGen::genCodeForStoreInd(GenTreeStoreInd* tree)` on `assert(!varTypeIsFloating(targetType) || (targetType == data->TypeGet()));`. * Fix for the previous test. No asm diffs. * Add a test that reads struct field as another struct type. It fails with assert `Compiler::StructPromotionHelper::CheckFakedType` `assert(fakedFieldsMap.Lookup(fieldHnd));`. * Fix the previous test. Check that we promote with the same class. * Add a test case for accessing an invalid offset. It fails in `fgMorphStructField` with `noway_assert(fieldLclIndex != BAD_VAR_NUM);`. * Fix the previous test. Reject field promotion if offset is invalid. No asm diffs. Commit migrated from https://github.com/dotnet/coreclr/commit/13dc153a7718a6d9297b757f5969c403856ef618 --- diff --git a/src/coreclr/src/jit/morph.cpp b/src/coreclr/src/jit/morph.cpp index f928479..080a0fb 100644 --- a/src/coreclr/src/jit/morph.cpp +++ b/src/coreclr/src/jit/morph.cpp @@ -17123,7 +17123,13 @@ void Compiler::fgMorphStructField(GenTree* tree, GenTree* parent) // Promoted struct unsigned fldOffset = field->gtFldOffset; unsigned fieldLclIndex = lvaGetFieldLocal(varDsc, fldOffset); - noway_assert(fieldLclIndex != BAD_VAR_NUM); + + if (fieldLclIndex == BAD_VAR_NUM) + { + // Access a promoted struct's field with an offset that doesn't correspond to any field. + // It can happen if the struct was cast to another struct with different offsets. + return; + } const LclVarDsc* fieldDsc = &lvaTable[fieldLclIndex]; var_types fieldType = fieldDsc->TypeGet(); @@ -17135,11 +17141,24 @@ void Compiler::fgMorphStructField(GenTree* tree, GenTree* parent) { // This is going to be an incorrect instruction promotion. // For example when we try to read int as long. - // Tolerate this case now to keep no asm difs in this PR. - // TODO make a return. + return; + } + + if (field->gtFldHnd != fieldDsc->lvFieldHnd) + { + CORINFO_CLASS_HANDLE fieldTreeClass = nullptr, fieldDscClass = nullptr; + + CorInfoType fieldTreeType = info.compCompHnd->getFieldType(field->gtFldHnd, &fieldTreeClass); + CorInfoType fieldDscType = info.compCompHnd->getFieldType(fieldDsc->lvFieldHnd, &fieldDscClass); + if (fieldTreeType != fieldDscType || fieldTreeClass != fieldDscClass) + { + // Access the promoted field with a different class handle, can't check that types match. + return; + } + // Access the promoted field as a field of a non-promoted struct with the same class handle. } #ifdef DEBUG - if (tree->TypeGet() == TYP_STRUCT) + else if (tree->TypeGet() == TYP_STRUCT) { // 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 diff --git a/src/coreclr/tests/src/JIT/Directed/StructPromote/Unsafe/AccessInvalidFieldOffset.cs b/src/coreclr/tests/src/JIT/Directed/StructPromote/Unsafe/AccessInvalidFieldOffset.cs new file mode 100644 index 0000000..2c03529 --- /dev/null +++ b/src/coreclr/tests/src/JIT/Directed/StructPromote/Unsafe/AccessInvalidFieldOffset.cs @@ -0,0 +1,62 @@ +// 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. + +// The test came from #20085. It tries to access field from a promoted struct with an offset that +// is not valid for the promoted struct type. + +using System.Runtime.CompilerServices; +using System.Runtime.InteropServices; +using System.Diagnostics; +using System; + +class TestStructAccessThroughRef +{ + + [StructLayout(LayoutKind.Explicit)] + struct NotPromotedStruct + { + + [FieldOffset(0)] + public long anotherField; + [FieldOffset(4)] // Offset doesn't correspond to a valid offset in PromotedStructSize. + public long overlappingField; + + public static ref PromotedStruct AsPromotedStructSize20(ref NotPromotedStruct d) => ref Unsafe.As(ref d); + } + + [StructLayout(LayoutKind.Explicit)] + struct PromotedStruct + { + [FieldOffset(0)] + public long anotherField; + [FieldOffset(8)] + public int smallField; + + public static ref NotPromotedStruct AsNotPromotedStruct(ref PromotedStruct d) => ref Unsafe.As(ref d); + } + + [MethodImpl(MethodImplOptions.NoInlining)] + static void TestStructCasts() + { + PromotedStruct a = new PromotedStruct(); // Addr-exposed, cannot be independent promoted. + a.anotherField = 5; + a.smallField = 6; + + Debug.Assert(PromotedStruct.AsNotPromotedStruct(ref a).anotherField == 0x5); + + // This access will ask LclVariable with type of `PromotedStruct` about the field with offset == 4, that doesn't exist there. + Debug.Assert(PromotedStruct.AsNotPromotedStruct(ref a).overlappingField == 0x600000000); + a.smallField = 6; + Debug.Assert(PromotedStruct.AsNotPromotedStruct(ref a).overlappingField == 0x600000000); + PromotedStruct.AsNotPromotedStruct(ref a).overlappingField = 0x700000000; + Debug.Assert(a.smallField == 0x7); + } + + static int Main() + { + TestStructCasts(); + return 100; + } + +} diff --git a/src/coreclr/tests/src/JIT/Directed/StructPromote/Unsafe/AccessInvalidFieldOffset.csproj b/src/coreclr/tests/src/JIT/Directed/StructPromote/Unsafe/AccessInvalidFieldOffset.csproj new file mode 100644 index 0000000..b25553e --- /dev/null +++ b/src/coreclr/tests/src/JIT/Directed/StructPromote/Unsafe/AccessInvalidFieldOffset.csproj @@ -0,0 +1,35 @@ + + + + + Debug + AnyCPU + $(MSBuildProjectName) + 2.0 + {9B2CCDC2-D6CE-4568-A419-AC79613BC235} + Exe + {786C830F-07A1-408B-BD7F-6EE04809D6DB};{FAE04EC0-301F-11D3-BF4B-00C04F79EFBC} + ..\..\ + latest + + + + + + + False + + + + None + True + + + + + + + + + + diff --git a/src/coreclr/tests/src/JIT/Directed/StructPromote/Unsafe/ReadDoubleFromIntOffset.cs b/src/coreclr/tests/src/JIT/Directed/StructPromote/Unsafe/ReadDoubleFromIntOffset.cs new file mode 100644 index 0000000..f81e879 --- /dev/null +++ b/src/coreclr/tests/src/JIT/Directed/StructPromote/Unsafe/ReadDoubleFromIntOffset.cs @@ -0,0 +1,88 @@ +// 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. + +// The test came from #20085. It tests that we do access overlapping fields with the correct types. +// Espessialy if the stuct was casted by 'Unsafe.As` from a promoted type +// and the promoted type had another field on the same offset but with a different type/size. + +using System.Runtime.CompilerServices; +using System.Runtime.InteropServices; +using System.Diagnostics; +using System; + +class TestReadIntAsDouble +{ + private struct Dec + { + public int uflags; + public int uhi; + public int ulo; + public int umid; + } + + [StructLayout(LayoutKind.Explicit)] + private struct DecCalc1 + { + [FieldOffset(0)] + public int uflags; + [FieldOffset(4)] + public int uhi; + [FieldOffset(8)] + public int ulo; + [FieldOffset(12)] + public int umid; + [FieldOffset(8)] + public double ulomidLE; + } + + public struct Data + { + public int x, y, z; + public double m; + } + + + [MethodImpl(MethodImplOptions.NoInlining)] + public static void TestDoubleAssignment(Data d) + { + Dec p = default; + p.ulo = d.x; + p.umid = d.y; + // The jit gets field's type based on offset, so it will return `ulo` as int. + d.m = Unsafe.As(ref p).ulomidLE; + } + + [StructLayout(LayoutKind.Explicit)] + private struct DecCalc2 + { + [FieldOffset(0)] + public int uflags; + [FieldOffset(4)] + public int uhi; + [FieldOffset(8)] + public double ulomidLE; + [FieldOffset(8)] + public int ulo; + [FieldOffset(12)] + public int umid; + + } + + [MethodImpl(MethodImplOptions.NoInlining)] + public static void TestIntAssignment(Data d) + { + Dec p = default; + p.ulo = d.x; + p.umid = d.y; + // The jit gets field's type based on offset, so it will return `ulomidLE` as double. + d.x = Unsafe.As(ref p).ulo; + } + + static int Main() + { + TestDoubleAssignment(default); + TestIntAssignment(default); + return 100; + } +} diff --git a/src/coreclr/tests/src/JIT/Directed/StructPromote/Unsafe/ReadDoubleFromIntOffset.csproj b/src/coreclr/tests/src/JIT/Directed/StructPromote/Unsafe/ReadDoubleFromIntOffset.csproj new file mode 100644 index 0000000..b25553e --- /dev/null +++ b/src/coreclr/tests/src/JIT/Directed/StructPromote/Unsafe/ReadDoubleFromIntOffset.csproj @@ -0,0 +1,35 @@ + + + + + Debug + AnyCPU + $(MSBuildProjectName) + 2.0 + {9B2CCDC2-D6CE-4568-A419-AC79613BC235} + Exe + {786C830F-07A1-408B-BD7F-6EE04809D6DB};{FAE04EC0-301F-11D3-BF4B-00C04F79EFBC} + ..\..\ + latest + + + + + + + False + + + + None + True + + + + + + + + + + diff --git a/src/coreclr/tests/src/JIT/Directed/StructPromote/Unsafe/ReadStructAsAnotherType.cs b/src/coreclr/tests/src/JIT/Directed/StructPromote/Unsafe/ReadStructAsAnotherType.cs new file mode 100644 index 0000000..b88af7f --- /dev/null +++ b/src/coreclr/tests/src/JIT/Directed/StructPromote/Unsafe/ReadStructAsAnotherType.cs @@ -0,0 +1,85 @@ +// 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. + +// The test came from #20085. It tests that we do access overlapping fields with the correct types. +// Espessialy if the stuct was casted by 'Unsafe.As` from a promoted type +// and the promoted type had another field on the same offset but with a different type/size. + + +using System.Runtime.CompilerServices; +using System.Runtime.InteropServices; +using System.Diagnostics; +using System; + +class TestAssignFieldsBetweenPromotedNotPromotedStructs +{ + + struct PrimitiveStruct // a struct of single field of scalar types aligned at their natural boundary. + { + public long pointerSizedField; + } + + struct NonPrimitiveStruct + { + public byte a; + public long b; + } + + [StructLayout(LayoutKind.Explicit)] + struct NotPromotedStruct + { + [FieldOffset(0)] + public PrimitiveStruct notPromotedField; + [FieldOffset(0)] + public NonPrimitiveStruct anotherOverlappingStruct; + + [FieldOffset(8)] + public long anotherField; + + + public static ref PromotedStruct AsPromotedStructSize20(ref NotPromotedStruct d) => ref Unsafe.As(ref d); + } + + [StructLayout(LayoutKind.Explicit)] + struct PromotedStruct + { + [FieldOffset(0)] + public PrimitiveStruct promotedField; + [FieldOffset(8)] + public long anotherField; + + + public static ref NotPromotedStruct AsNotPromotedStruct(ref PromotedStruct d) => ref Unsafe.As(ref d); + } + + // Some simple tests that check that lcl variables + [MethodImpl(MethodImplOptions.NoInlining)] + static void TestStructCasts() + { + PromotedStruct a = new PromotedStruct(); // Addr-exposed, cannot be promoted.promotedField. + a.promotedField.pointerSizedField = 4; + a.anotherField = 5; + NotPromotedStruct b = PromotedStruct.AsNotPromotedStruct(ref a); + // The cast can be inlined and the field handle will refer to the `PromotedStruct.pointerSizedField`, + // in this case we can promote it because `NotPromotedStruct.notPromotedField.pointerSizedField` has + // the same class handle. + Debug.Assert(b.notPromotedField.pointerSizedField == 0x4); + + NotPromotedStruct c = PromotedStruct.AsNotPromotedStruct(ref a); + // The cast can be inlined and the field handle will refer to the `PromotedStruct.pointerSizedField`, + // in this case we cannot promote it because `NotPromotedStruct.anotherOverlappingStruct.a` has + // a different class handle (`NotPromotedStruct.anotherOverlappingStruct`). + Debug.Assert(c.anotherOverlappingStruct.a == 0x4); + + Debug.Assert(c.anotherOverlappingStruct.b == 0x5); + } + + public static int Main() + { + TestStructCasts(); + return 100; + } + +} + diff --git a/src/coreclr/tests/src/JIT/Directed/StructPromote/Unsafe/ReadStructAsAnotherType.csproj b/src/coreclr/tests/src/JIT/Directed/StructPromote/Unsafe/ReadStructAsAnotherType.csproj new file mode 100644 index 0000000..b25553e --- /dev/null +++ b/src/coreclr/tests/src/JIT/Directed/StructPromote/Unsafe/ReadStructAsAnotherType.csproj @@ -0,0 +1,35 @@ + + + + + Debug + AnyCPU + $(MSBuildProjectName) + 2.0 + {9B2CCDC2-D6CE-4568-A419-AC79613BC235} + Exe + {786C830F-07A1-408B-BD7F-6EE04809D6DB};{FAE04EC0-301F-11D3-BF4B-00C04F79EFBC} + ..\..\ + latest + + + + + + + False + + + + None + True + + + + + + + + + +