Skip to content

Commit

Permalink
Delete StructLayoutAttribute.Pack adjustment
Browse files Browse the repository at this point in the history
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 dotnet#12480
  • Loading branch information
jkotas committed Feb 8, 2022
1 parent 07a7215 commit 7db8999
Show file tree
Hide file tree
Showing 6 changed files with 7 additions and 40 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand All @@ -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)
{
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand All @@ -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,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}

Expand Down Expand Up @@ -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);
}

Expand Down Expand Up @@ -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);
}

Expand Down
4 changes: 2 additions & 2 deletions src/libraries/System.Reflection/tests/TypeInfoTests.cs
Original file line number Diff line number Diff line change
Expand Up @@ -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)
{
Expand Down
11 changes: 1 addition & 10 deletions src/mono/System.Private.CoreLib/src/System/RuntimeType.Mono.cs
Original file line number Diff line number Diff line change
Expand Up @@ -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)
{
Expand All @@ -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 };
}
Expand Down

0 comments on commit 7db8999

Please sign in to comment.