From f94b8478c671c34af1f3537b9ad9f99fdbff2fa0 Mon Sep 17 00:00:00 2001 From: David Wrighton Date: Wed, 26 May 2021 11:29:34 -0700 Subject: [PATCH 1/3] Add new test to cover override involving generics and multidimensional arrays --- .../Devirtualization/GenericArrayOverride.il | 297 ++++++++++++++++++ .../GenericArrayOverride.ilproj | 13 + 2 files changed, 310 insertions(+) create mode 100644 src/tests/JIT/opt/Devirtualization/GenericArrayOverride.il create mode 100644 src/tests/JIT/opt/Devirtualization/GenericArrayOverride.ilproj diff --git a/src/tests/JIT/opt/Devirtualization/GenericArrayOverride.il b/src/tests/JIT/opt/Devirtualization/GenericArrayOverride.il new file mode 100644 index 00000000000000..7349f2e5b7b977 --- /dev/null +++ b/src/tests/JIT/opt/Devirtualization/GenericArrayOverride.il @@ -0,0 +1,297 @@ + +// Licensed to the .NET Foundation under one or more agreements. +// The .NET Foundation licenses this file to you under the MIT license. + + +// Metadata version: v4.0.30319 +.assembly extern System.Runtime +{ + .publickeytoken = (B0 3F 5F 7F 11 D5 0A 3A ) // .?_....: + .ver 5:0:0:0 +} +.assembly extern System.Collections +{ + .publickeytoken = (B0 3F 5F 7F 11 D5 0A 3A ) // .?_....: + .ver 5:0:0:0 +} +.assembly extern System.Console +{ + .publickeytoken = (B0 3F 5F 7F 11 D5 0A 3A ) // .?_....: + .ver 5:0:0:0 +} +.assembly GenericArrayOverride +{ + .custom instance void [System.Runtime]System.Runtime.CompilerServices.CompilationRelaxationsAttribute::.ctor(int32) = ( 01 00 08 00 00 00 00 00 ) + .custom instance void [System.Runtime]System.Runtime.CompilerServices.RuntimeCompatibilityAttribute::.ctor() = ( 01 00 01 00 54 02 16 57 72 61 70 4E 6F 6E 45 78 // ....T..WrapNonEx + 63 65 70 74 69 6F 6E 54 68 72 6F 77 73 01 ) // ceptionThrows. + + // --- The following custom attribute is added automatically, do not uncomment ------- + // .custom instance void [System.Runtime]System.Diagnostics.DebuggableAttribute::.ctor(valuetype [System.Runtime]System.Diagnostics.DebuggableAttribute/DebuggingModes) = ( 01 00 07 01 00 00 00 00 ) + + .custom instance void [System.Runtime]System.Runtime.Versioning.TargetFrameworkAttribute::.ctor(string) = ( 01 00 18 2E 4E 45 54 43 6F 72 65 41 70 70 2C 56 // ....NETCoreApp,V + 65 72 73 69 6F 6E 3D 76 35 2E 30 01 00 54 0E 14 // ersion=v5.0..T.. + 46 72 61 6D 65 77 6F 72 6B 44 69 73 70 6C 61 79 // FrameworkDisplay + 4E 61 6D 65 00 ) // Name. + .custom instance void [System.Runtime]System.Reflection.AssemblyCompanyAttribute::.ctor(string) = ( 01 00 14 47 65 6E 65 72 69 63 41 72 72 61 79 4F // ...GenericArrayO + 76 65 72 72 69 64 65 00 00 ) // verride.. + .custom instance void [System.Runtime]System.Reflection.AssemblyConfigurationAttribute::.ctor(string) = ( 01 00 05 44 65 62 75 67 00 00 ) // ...Debug.. + .custom instance void [System.Runtime]System.Reflection.AssemblyFileVersionAttribute::.ctor(string) = ( 01 00 07 31 2E 30 2E 30 2E 30 00 00 ) // ...1.0.0.0.. + .custom instance void [System.Runtime]System.Reflection.AssemblyInformationalVersionAttribute::.ctor(string) = ( 01 00 05 31 2E 30 2E 30 00 00 ) // ...1.0.0.. + .custom instance void [System.Runtime]System.Reflection.AssemblyProductAttribute::.ctor(string) = ( 01 00 14 47 65 6E 65 72 69 63 41 72 72 61 79 4F // ...GenericArrayO + 76 65 72 72 69 64 65 00 00 ) // verride.. + .custom instance void [System.Runtime]System.Reflection.AssemblyTitleAttribute::.ctor(string) = ( 01 00 14 47 65 6E 65 72 69 63 41 72 72 61 79 4F // ...GenericArrayO + 76 65 72 72 69 64 65 00 00 ) // verride.. + .hash algorithm 0x00008004 + .ver 1:0:0:0 +} +.module GenericArrayOverride.dll +// MVID: {36F29C33-E2D3-47BA-B888-D30052D5C374} +.imagebase 0x00400000 +.file alignment 0x00000200 +.stackreserve 0x00100000 +.subsystem 0x0003 // WINDOWS_CUI +.corflags 0x00000001 // ILONLY +// Image base: 0x06DD0000 + + +// =============== CLASS MEMBERS DECLARATION =================== + +.class private auto ansi beforefieldinit GenericArrayOverride.Base`1 + extends [System.Runtime]System.Object +{ + .method public hidebysig newslot virtual + instance string Function(!T param) cil managed + { + // Code size 11 (0xb) + .maxstack 1 + .locals init (string V_0) + IL_0000: nop + IL_0001: ldstr "Base" + IL_0006: stloc.0 + IL_0007: br.s IL_0009 + + IL_0009: ldloc.0 + IL_000a: ret + } // end of method Base`1::Function + + .method public hidebysig specialname rtspecialname + instance void .ctor() cil managed + { + // Code size 8 (0x8) + .maxstack 8 + IL_0000: ldarg.0 + IL_0001: call instance void [System.Runtime]System.Object::.ctor() + IL_0006: nop + IL_0007: ret + } // end of method Base`1::.ctor + +} // end of class GenericArrayOverride.Base`1 + +.class interface private abstract auto ansi GenericArrayOverride.IFace`1 +{ + .method public hidebysig newslot abstract virtual + instance string IFaceFunction(class [System.Collections]System.Collections.Generic.List`1 param) cil managed + { + } // end of method IFace`1::IFaceFunction + +} // end of class GenericArrayOverride.IFace`1 + +.class private auto ansi beforefieldinit GenericArrayOverride.Derived + extends class GenericArrayOverride.Base`1 + implements class GenericArrayOverride.IFace`1 +{ + .method public hidebysig virtual instance string + Function(class GenericArrayOverride.Derived[1...,0...,0...,0...] param) cil managed + { + // Code size 11 (0xb) + .maxstack 1 + .locals init (string V_0) + IL_0000: nop + IL_0001: ldstr "DerivedWrong1" + IL_0006: stloc.0 + IL_0007: br.s IL_0009 + + IL_0009: ldloc.0 + IL_000a: ret + } // end of method Derived::Function + + .method public hidebysig virtual instance string + Function(class GenericArrayOverride.Derived[0...,0...,0...,0...] param) cil managed + { + // Code size 11 (0xb) + .maxstack 1 + .locals init (string V_0) + IL_0000: nop + IL_0001: ldstr "Derived" + IL_0006: stloc.0 + IL_0007: br.s IL_0009 + + IL_0009: ldloc.0 + IL_000a: ret + } // end of method Derived::Function + + .method public hidebysig virtual instance string + Function(class GenericArrayOverride.Derived[2...,0...,0...,0...] param) cil managed + { + // Code size 11 (0xb) + .maxstack 1 + .locals init (string V_0) + IL_0000: nop + IL_0001: ldstr "DerivedWrong2" + IL_0006: stloc.0 + IL_0007: br.s IL_0009 + + IL_0009: ldloc.0 + IL_000a: ret + } // end of method Derived::Function + + .method public hidebysig newslot virtual + instance string IFaceFunction(class [System.Collections]System.Collections.Generic.List`1 param) cil managed + { + // Code size 11 (0xb) + .maxstack 1 + .locals init (string V_0) + IL_0000: nop + IL_0001: ldstr "IFaceFuncWrong1" + IL_0006: stloc.0 + IL_0007: br.s IL_0009 + + IL_0009: ldloc.0 + IL_000a: ret + } // end of method Derived::IFaceFunction + + .method public hidebysig newslot virtual + instance string IFaceFunction(class [System.Collections]System.Collections.Generic.List`1 param) cil managed + { + // Code size 11 (0xb) + .maxstack 1 + .locals init (string V_0) + IL_0000: nop + IL_0001: ldstr "IFaceFunc" + IL_0006: stloc.0 + IL_0007: br.s IL_0009 + + IL_0009: ldloc.0 + IL_000a: ret + } // end of method Derived::IFaceFunction + + .method public hidebysig newslot virtual + instance string IFaceFunction(class [System.Collections]System.Collections.Generic.List`1 param) cil managed + { + // Code size 11 (0xb) + .maxstack 1 + .locals init (string V_0) + IL_0000: nop + IL_0001: ldstr "IFaceFuncWrong2" + IL_0006: stloc.0 + IL_0007: br.s IL_0009 + + IL_0009: ldloc.0 + IL_000a: ret + } // end of method Derived::IFaceFunction + + .method public hidebysig specialname rtspecialname + instance void .ctor() cil managed + { + // Code size 8 (0x8) + .maxstack 8 + IL_0000: ldarg.0 + IL_0001: call instance void class GenericArrayOverride.Base`1::.ctor() + IL_0006: nop + IL_0007: ret + } // end of method Derived::.ctor + +} // end of class GenericArrayOverride.Derived + +.class private auto ansi beforefieldinit GenericArrayOverride.Program + extends [System.Runtime]System.Object +{ + .method private hidebysig static int32 + Main() cil managed + { + .entrypoint + // Code size 120 (0x78) + .maxstack 5 + .locals init (class GenericArrayOverride.Base`1 V_0, + class GenericArrayOverride.IFace`1 V_1, + bool V_2, + int32 V_3, + bool V_4) + IL_0000: nop + IL_0001: newobj instance void GenericArrayOverride.Derived::.ctor() + IL_0006: stloc.0 + IL_0007: ldloc.0 + IL_0008: castclass class GenericArrayOverride.IFace`1 + IL_000d: stloc.1 + IL_000e: ldloc.0 + IL_000f: ldc.i4.1 + IL_0010: ldc.i4.1 + IL_0011: ldc.i4.1 + IL_0012: ldc.i4.1 + IL_0013: newobj instance void class GenericArrayOverride.Derived[0...,0...,0...,0...]::.ctor(int32, + int32, + int32, + int32) + IL_0018: callvirt instance string class GenericArrayOverride.Base`1::Function(!0) + IL_001d: ldstr "Derived" + IL_0022: call bool [System.Runtime]System.String::op_Inequality(string, + string) + IL_0027: stloc.2 + IL_0028: ldloc.2 + IL_0029: brfalse.s IL_003b + + IL_002b: nop + IL_002c: ldstr "Virtual override failed" + IL_0031: call void [System.Console]System.Console::WriteLine(string) + IL_0036: nop + IL_0037: ldc.i4.1 + IL_0038: stloc.3 + IL_0039: br.s IL_0076 + + IL_003b: ldloc.1 + IL_003c: newobj instance void class [System.Collections]System.Collections.Generic.List`1::.ctor() + IL_0041: callvirt instance string class GenericArrayOverride.IFace`1::IFaceFunction(class [System.Collections]System.Collections.Generic.List`1) + IL_0046: ldstr "IFaceFunc" + IL_004b: call bool [System.Runtime]System.String::op_Inequality(string, + string) + IL_0050: stloc.s V_4 + IL_0052: ldloc.s V_4 + IL_0054: brfalse.s IL_0066 + + IL_0056: nop + IL_0057: ldstr "Interface override failed" + IL_005c: call void [System.Console]System.Console::WriteLine(string) + IL_0061: nop + IL_0062: ldc.i4.2 + IL_0063: stloc.3 + IL_0064: br.s IL_0076 + + IL_0066: ldstr "PASS" + IL_006b: call void [System.Console]System.Console::WriteLine(string) + IL_0070: nop + IL_0071: ldc.i4.s 100 + IL_0073: stloc.3 + IL_0074: br.s IL_0076 + + IL_0076: ldloc.3 + IL_0077: ret + } // end of method Program::Main + + .method public hidebysig specialname rtspecialname + instance void .ctor() cil managed + { + // Code size 8 (0x8) + .maxstack 8 + IL_0000: ldarg.0 + IL_0001: call instance void [System.Runtime]System.Object::.ctor() + IL_0006: nop + IL_0007: ret + } // end of method Program::.ctor + +} // end of class GenericArrayOverride.Program + + +// ============================================================= + +// *********** DISASSEMBLY COMPLETE *********************** +// WARNING: Created Win32 resource file C:\git\runtime\src\tests\JIT\opt\Devirtualization\GenericArrayOverride.res diff --git a/src/tests/JIT/opt/Devirtualization/GenericArrayOverride.ilproj b/src/tests/JIT/opt/Devirtualization/GenericArrayOverride.ilproj new file mode 100644 index 00000000000000..2250129c9ce9c9 --- /dev/null +++ b/src/tests/JIT/opt/Devirtualization/GenericArrayOverride.ilproj @@ -0,0 +1,13 @@ + + + Exe + 1 + + + None + True + + + + + From 12720cf05ffcd245002ce693ca8ff2a4c4a20cdf Mon Sep 17 00:00:00 2001 From: David Wrighton Date: Thu, 27 May 2021 18:34:18 -0700 Subject: [PATCH 2/3] Change up the definition of an MDArray which doesn't need a special descriptor in the embedded signature data to match the type of mdarray thay everyone always uses. This fixes the virtual function handling behavior for all reasonable cases. --- .../Common/TypeSystem/Ecma/EcmaSignatureParser.cs | 10 ++++++++-- .../MetadataEmitter/TypeSystemMetadataEmitter.cs | 14 +++++++++++++- 2 files changed, 21 insertions(+), 3 deletions(-) diff --git a/src/coreclr/tools/Common/TypeSystem/Ecma/EcmaSignatureParser.cs b/src/coreclr/tools/Common/TypeSystem/Ecma/EcmaSignatureParser.cs index 9705ccdd179749..48ca23cf1ea26f 100644 --- a/src/coreclr/tools/Common/TypeSystem/Ecma/EcmaSignatureParser.cs +++ b/src/coreclr/tools/Common/TypeSystem/Ecma/EcmaSignatureParser.cs @@ -169,10 +169,16 @@ private TypeDesc ParseTypeImpl(SignatureTypeCode typeCode) var lowerBoundsCount = _reader.ReadCompressedInteger(); int []lowerBounds = lowerBoundsCount > 0 ? new int[lowerBoundsCount] : Array.Empty(); + bool nonZeroLowerBounds = false; for (int j = 0; j < lowerBoundsCount; j++) - lowerBounds[j] = _reader.ReadCompressedSignedInteger(); + { + int loBound = _reader.ReadCompressedSignedInteger(); + if (loBound != 0) + nonZeroLowerBounds = true; + lowerBounds[j] = loBound; + } - if (boundsCount != 0 || lowerBoundsCount != 0) + if (boundsCount != 0 || lowerBoundsCount != rank || nonZeroLowerBounds) { StringBuilder arrayShapeString = new StringBuilder(); arrayShapeString.Append(string.Join(",", bounds)); diff --git a/src/coreclr/tools/Common/TypeSystem/MetadataEmitter/TypeSystemMetadataEmitter.cs b/src/coreclr/tools/Common/TypeSystem/MetadataEmitter/TypeSystemMetadataEmitter.cs index 0d05bb92e09dc3..38616f5d0cf280 100644 --- a/src/coreclr/tools/Common/TypeSystem/MetadataEmitter/TypeSystemMetadataEmitter.cs +++ b/src/coreclr/tools/Common/TypeSystem/MetadataEmitter/TypeSystemMetadataEmitter.cs @@ -424,10 +424,22 @@ public void EmitArrayShapeAtCurrentIndexStack(BlobBuilder signatureBuilder, int if (!emittedWithShape) { - shapeEncoder.Shape(rank, ImmutableArray.Empty, ImmutableArray.Empty); + shapeEncoder.Shape(rank, ImmutableArray.Empty, ImmutableArraysFilledWithZeroes[rank]); } } + static ImmutableArray[] ImmutableArraysFilledWithZeroes = CreateStaticArrayOfImmutableArraysFilledWithZeroes(33); // The max rank of an array is 32 + + static ImmutableArray[] CreateStaticArrayOfImmutableArraysFilledWithZeroes(int count) + { + ImmutableArray[] result = new ImmutableArray[count]; + for (int i = 0; i < result.Length; i++) + { + result[i] = new int[i].ToImmutableArray(); + } + return result; + } + public void EmitAtCurrentIndexStack(BlobBuilder signatureBuilder) { if (!Complete) From de67e64c7ee98ff83e9ac08fb446d293aa0332f8 Mon Sep 17 00:00:00 2001 From: David Wrighton Date: Fri, 28 May 2021 11:53:43 -0700 Subject: [PATCH 3/3] Code review feedback --- .../TypeSystemMetadataEmitter.cs | 13 +++++++++--- .../ILTestAssembly/Signature.il | 5 +++++ .../SignatureTests.cs | 20 +++++++++++++++++-- 3 files changed, 33 insertions(+), 5 deletions(-) diff --git a/src/coreclr/tools/Common/TypeSystem/MetadataEmitter/TypeSystemMetadataEmitter.cs b/src/coreclr/tools/Common/TypeSystem/MetadataEmitter/TypeSystemMetadataEmitter.cs index 38616f5d0cf280..5a47094605c9e1 100644 --- a/src/coreclr/tools/Common/TypeSystem/MetadataEmitter/TypeSystemMetadataEmitter.cs +++ b/src/coreclr/tools/Common/TypeSystem/MetadataEmitter/TypeSystemMetadataEmitter.cs @@ -424,13 +424,20 @@ public void EmitArrayShapeAtCurrentIndexStack(BlobBuilder signatureBuilder, int if (!emittedWithShape) { - shapeEncoder.Shape(rank, ImmutableArray.Empty, ImmutableArraysFilledWithZeroes[rank]); + shapeEncoder.Shape(rank, ImmutableArray.Empty, GetZeroedImmutableArrayOfSize(rank)); } } - static ImmutableArray[] ImmutableArraysFilledWithZeroes = CreateStaticArrayOfImmutableArraysFilledWithZeroes(33); // The max rank of an array is 32 + private static ImmutableArray[] ImmutableArraysFilledWithZeroes = CreateStaticArrayOfImmutableArraysFilledWithZeroes(33); // The max rank of an array is 32 - static ImmutableArray[] CreateStaticArrayOfImmutableArraysFilledWithZeroes(int count) + private static ImmutableArray GetZeroedImmutableArrayOfSize(int rank) + { + if (rank < ImmutableArraysFilledWithZeroes.Length) + return ImmutableArraysFilledWithZeroes[rank]; + + return new int[rank].ToImmutableArray(); + } + private static ImmutableArray[] CreateStaticArrayOfImmutableArraysFilledWithZeroes(int count) { ImmutableArray[] result = new ImmutableArray[count]; for (int i = 0; i < result.Length; i++) diff --git a/src/coreclr/tools/aot/ILCompiler.TypeSystem.ReadyToRun.Tests/ILTestAssembly/Signature.il b/src/coreclr/tools/aot/ILCompiler.TypeSystem.ReadyToRun.Tests/ILTestAssembly/Signature.il index 0b9a489f4b8eb0..32e307b7036c87 100644 --- a/src/coreclr/tools/aot/ILCompiler.TypeSystem.ReadyToRun.Tests/ILTestAssembly/Signature.il +++ b/src/coreclr/tools/aot/ILCompiler.TypeSystem.ReadyToRun.Tests/ILTestAssembly/Signature.il @@ -34,6 +34,11 @@ { ret } + + .method public hidebysig instance int32 [0...,0...] Method5(int32 [0...,], int32 [,]) cil managed + { + ret + } } .class private auto ansi beforefieldinit Atom diff --git a/src/coreclr/tools/aot/ILCompiler.TypeSystem.ReadyToRun.Tests/SignatureTests.cs b/src/coreclr/tools/aot/ILCompiler.TypeSystem.ReadyToRun.Tests/SignatureTests.cs index 0ea96171119616..41362ee585673d 100644 --- a/src/coreclr/tools/aot/ILCompiler.TypeSystem.ReadyToRun.Tests/SignatureTests.cs +++ b/src/coreclr/tools/aot/ILCompiler.TypeSystem.ReadyToRun.Tests/SignatureTests.cs @@ -95,11 +95,27 @@ public void TestSignatureMatchesForArrayShapeDetails() MetadataType modOptTester = _testModule.GetType("", "ModOptTester"); MethodSignature methodWithModOpt = modOptTester.GetMethods().Single(m => string.Equals(m.Name, "Method4")).Signature; - Assert.Equal(6, methodWithModOpt.GetEmbeddedSignatureData().Length); + _output.WriteLine($"Found ModOptData '{GetModOptMethodSignatureInfo(methodWithModOpt)}'"); + Assert.Equal(7, methodWithModOpt.GetEmbeddedSignatureData().Length); Assert.Equal(MethodSignature.GetIndexOfCustomModifierOnPointedAtTypeByParameterIndex(0), methodWithModOpt.GetEmbeddedSignatureData()[0].index); Assert.Equal(MethodSignature.GetIndexOfCustomModifierOnPointedAtTypeByParameterIndex(1), methodWithModOpt.GetEmbeddedSignatureData()[2].index); Assert.Equal(MethodSignature.GetIndexOfCustomModifierOnPointedAtTypeByParameterIndex(2), methodWithModOpt.GetEmbeddedSignatureData()[4].index); - Assert.Equal("OptionalCustomModifier0.1.1.2.1.1VoidArrayShape1.2.1.1|3,4|0,1OptionalCustomModifier0.1.1.2.2.1FooModifierArrayShape1.2.2.1|0,9|2,0OptionalCustomModifier0.1.1.2.3.1FooModifierArrayShape1.2.3.1||0", GetModOptMethodSignatureInfo(methodWithModOpt)); + Assert.Equal("OptionalCustomModifier0.1.1.2.1.1VoidArrayShape1.2.1.1|3,4|0,1OptionalCustomModifier0.1.1.2.2.1FooModifierArrayShape1.2.2.1|0,9|2,0OptionalCustomModifier0.1.1.2.3.1FooModifierArrayShape1.2.3.1||0ArrayShape1.2.4.1||", GetModOptMethodSignatureInfo(methodWithModOpt)); + } + + [Fact] + public void TestSignatureMatchesForArrayShapeDetails_HandlingOfCasesWhichDoNotNeedEmbeddeSignatureData() + { + // Test that ensure the typical case (where the loBounds is 0, and the hibounds is unspecified) doesn't produce an + // EmbeddedSignatureData, but that other cases do. This isn't a complete test as ilasm won't actually properly generate the metadata for many of these cases + MetadataType modOptTester = _testModule.GetType("", "ModOptTester"); + MethodSignature methodWithModOpt = modOptTester.GetMethods().Single(m => string.Equals(m.Name, "Method5")).Signature; + + _output.WriteLine($"Found ModOptData '{GetModOptMethodSignatureInfo(methodWithModOpt)}'"); + Assert.Equal(2, methodWithModOpt.GetEmbeddedSignatureData().Length); + Assert.EndsWith(methodWithModOpt.GetEmbeddedSignatureData()[0].index.Split('|')[0], MethodSignature.GetIndexOfCustomModifierOnPointedAtTypeByParameterIndex(1)); + Assert.EndsWith(methodWithModOpt.GetEmbeddedSignatureData()[1].index.Split('|')[0], MethodSignature.GetIndexOfCustomModifierOnPointedAtTypeByParameterIndex(2)); + Assert.Equal("ArrayShape1.2.2.1||0ArrayShape1.2.3.1||", GetModOptMethodSignatureInfo(methodWithModOpt)); } [Fact]