From 7db899944d82c5c001c3d02757fd12c096c4e612 Mon Sep 17 00:00:00 2001 From: Jan Kotas Date: Mon, 7 Feb 2022 21:20:02 -0800 Subject: [PATCH] Delete StructLayoutAttribute.Pack adjustment This adjustment was trying to minic adjustment performed by type loader, but the logic has been out-of-sync with the type loader evolution for years. It does not make sense for custom attribute reading via reflection to perform this adjustment. Fixes #12480 --- .../System/Reflection/RuntimeCustomAttributeData.cs | 6 ------ .../Runtime/TypeInfos/RuntimeNamedTypeInfo.cs | 12 +----------- .../Reflection/TypeLoading/Types/RoDefinitionType.cs | 8 -------- .../Tests/Type/TypeTests.StructLayoutAttribute.cs | 6 +++--- .../System.Reflection/tests/TypeInfoTests.cs | 4 ++-- .../src/System/RuntimeType.Mono.cs | 11 +---------- 6 files changed, 7 insertions(+), 40 deletions(-) diff --git a/src/coreclr/System.Private.CoreLib/src/System/Reflection/RuntimeCustomAttributeData.cs b/src/coreclr/System.Private.CoreLib/src/System/Reflection/RuntimeCustomAttributeData.cs index edf7aeb269c26..b62c9edaa6e47 100644 --- a/src/coreclr/System.Private.CoreLib/src/System/Reflection/RuntimeCustomAttributeData.cs +++ b/src/coreclr/System.Private.CoreLib/src/System/Reflection/RuntimeCustomAttributeData.cs @@ -1873,12 +1873,6 @@ internal static bool IsDefined(RuntimeFieldInfo field, RuntimeType? caType) } type.GetRuntimeModule().MetadataImport.GetClassLayout(type.MetadataToken, out int pack, out int size); - // Metadata parameter checking should not have allowed 0 for packing size. - // The runtime later converts a packing size of 0 to 8 so do the same here - // because it's more useful from a user perspective. - if (pack == 0) - pack = 8; // DEFAULT_PACKING_SIZE - StructLayoutAttribute attribute = new StructLayoutAttribute(layoutKind); attribute.Pack = pack; diff --git a/src/coreclr/nativeaot/System.Private.Reflection.Core/src/System/Reflection/Runtime/TypeInfos/RuntimeNamedTypeInfo.cs b/src/coreclr/nativeaot/System.Private.Reflection.Core/src/System/Reflection/Runtime/TypeInfos/RuntimeNamedTypeInfo.cs index fdbcf0b53685d..3622630315ae3 100644 --- a/src/coreclr/nativeaot/System.Private.Reflection.Core/src/System/Reflection/Runtime/TypeInfos/RuntimeNamedTypeInfo.cs +++ b/src/coreclr/nativeaot/System.Private.Reflection.Core/src/System/Reflection/Runtime/TypeInfos/RuntimeNamedTypeInfo.cs @@ -129,8 +129,6 @@ public sealed override StructLayoutAttribute StructLayoutAttribute { get { - const int DefaultPackingSize = 8; - // Note: CoreClr checks HasElementType and IsGenericParameter in addition to IsInterface but those properties cannot be true here as this // RuntimeTypeInfo subclass is solely for TypeDef types.) if (IsInterface) @@ -156,15 +154,7 @@ public sealed override StructLayoutAttribute StructLayoutAttribute default: charSet = CharSet.None; break; } - int pack; - int size; - GetPackSizeAndSize(out pack, out size); - - // Metadata parameter checking should not have allowed 0 for packing size. - // The runtime later converts a packing size of 0 to 8 so do the same here - // because it's more useful from a user perspective. - if (pack == 0) - pack = DefaultPackingSize; + GetPackSizeAndSize(out int pack, out int size); return new StructLayoutAttribute(layoutKind) { diff --git a/src/libraries/System.Reflection.MetadataLoadContext/src/System/Reflection/TypeLoading/Types/RoDefinitionType.cs b/src/libraries/System.Reflection.MetadataLoadContext/src/System/Reflection/TypeLoading/Types/RoDefinitionType.cs index 94837cbfa2476..394f495487122 100644 --- a/src/libraries/System.Reflection.MetadataLoadContext/src/System/Reflection/TypeLoading/Types/RoDefinitionType.cs +++ b/src/libraries/System.Reflection.MetadataLoadContext/src/System/Reflection/TypeLoading/Types/RoDefinitionType.cs @@ -146,8 +146,6 @@ public sealed override StructLayoutAttribute? StructLayoutAttribute { get { - const int DefaultPackingSize = 8; - // Note: CoreClr checks HasElementType and IsGenericParameter in addition to IsInterface but those properties cannot be true here as this // RoType subclass is solely for TypeDef types.) if (IsInterface) @@ -170,12 +168,6 @@ public sealed override StructLayoutAttribute? StructLayoutAttribute }; GetPackSizeAndSize(out int pack, out int size); - // Metadata parameter checking should not have allowed 0 for packing size. - // The runtime later converts a packing size of 0 to 8 so do the same here - // because it's more useful from a user perspective. - if (pack == 0) - pack = DefaultPackingSize; - return new StructLayoutAttribute(layoutKind) { CharSet = charSet, diff --git a/src/libraries/System.Reflection.MetadataLoadContext/tests/src/Tests/Type/TypeTests.StructLayoutAttribute.cs b/src/libraries/System.Reflection.MetadataLoadContext/tests/src/Tests/Type/TypeTests.StructLayoutAttribute.cs index 865e51f8fbc16..ec138772fcc6e 100644 --- a/src/libraries/System.Reflection.MetadataLoadContext/tests/src/Tests/Type/TypeTests.StructLayoutAttribute.cs +++ b/src/libraries/System.Reflection.MetadataLoadContext/tests/src/Tests/Type/TypeTests.StructLayoutAttribute.cs @@ -15,7 +15,7 @@ public static void Test_UndecoratedClass() StructLayoutAttribute s = t.StructLayoutAttribute; Assert.Equal(LayoutKind.Auto, s.Value); Assert.Equal(CharSet.Ansi, s.CharSet); - Assert.Equal(8, s.Pack); + Assert.Equal(0, s.Pack); Assert.Equal(0, s.Size); } @@ -71,7 +71,7 @@ public static void TestSequentialAutoZeroZero() StructLayoutAttribute s = t.StructLayoutAttribute; Assert.Equal(LayoutKind.Sequential, s.Value); Assert.Equal(CharSet.Auto, s.CharSet); - Assert.Equal(8, s.Pack); // Not an error: Pack=0 is treated as if it were Pack=8. + Assert.Equal(0, s.Pack); Assert.Equal(0, s.Size); } @@ -149,7 +149,7 @@ public static void Test_Derived() StructLayoutAttribute s = t.StructLayoutAttribute; Assert.Equal(LayoutKind.Auto, s.Value); Assert.Equal(CharSet.Ansi, s.CharSet); - Assert.Equal(8, s.Pack); + Assert.Equal(0, s.Pack); Assert.Equal(0, s.Size); } diff --git a/src/libraries/System.Reflection/tests/TypeInfoTests.cs b/src/libraries/System.Reflection/tests/TypeInfoTests.cs index f3b9240505c69..a9b4292c2fb4b 100644 --- a/src/libraries/System.Reflection/tests/TypeInfoTests.cs +++ b/src/libraries/System.Reflection/tests/TypeInfoTests.cs @@ -1490,9 +1490,9 @@ public void IsNestedFamORAssem(Type type, bool expected) } [Theory] - [InlineData(typeof(StructWithoutExplicitStructLayout), LayoutKind.Sequential, CharSet.Ansi, 8)] + [InlineData(typeof(StructWithoutExplicitStructLayout), LayoutKind.Sequential, CharSet.Ansi, 0)] [InlineData(typeof(StructWithExplicitStructLayout), LayoutKind.Explicit, CharSet.Ansi, 1)] - [InlineData(typeof(ClassWithoutExplicitStructLayout), LayoutKind.Auto, CharSet.Ansi, 8)] + [InlineData(typeof(ClassWithoutExplicitStructLayout), LayoutKind.Auto, CharSet.Ansi, 0)] [InlineData(typeof(ClassWithExplicitStructLayout), LayoutKind.Explicit, CharSet.Unicode, 2)] public void StructLayoutAttribute(Type type, LayoutKind kind, CharSet charset, int pack) { diff --git a/src/mono/System.Private.CoreLib/src/System/RuntimeType.Mono.cs b/src/mono/System.Private.CoreLib/src/System/RuntimeType.Mono.cs index 289b471450cb6..095d82d13659b 100644 --- a/src/mono/System.Private.CoreLib/src/System/RuntimeType.Mono.cs +++ b/src/mono/System.Private.CoreLib/src/System/RuntimeType.Mono.cs @@ -2317,14 +2317,11 @@ public override bool IsSubclassOf(Type type) return RuntimeTypeHandle.IsSubclassOf(this, rtType); } - private const int DEFAULT_PACKING_SIZE = 8; - internal StructLayoutAttribute? GetStructLayoutAttribute() { if (IsInterface || HasElementType || IsGenericParameter) return null; - int pack, size; LayoutKind layoutKind = LayoutKind.Auto; switch (Attributes & TypeAttributes.LayoutMask) { @@ -2343,13 +2340,7 @@ public override bool IsSubclassOf(Type type) default: break; } - GetPacking(out pack, out size); - - // Metadata parameter checking should not have allowed 0 for packing size. - // The runtime later converts a packing size of 0 to 8 so do the same here - // because it's more useful from a user perspective. - if (pack == 0) - pack = DEFAULT_PACKING_SIZE; + GetPacking(out int pack, out int size); return new StructLayoutAttribute(layoutKind) { Pack = pack, Size = size, CharSet = charSet }; }