Skip to content

Commit

Permalink
Align rva fields in reflection emit (#63430)
Browse files Browse the repository at this point in the history
In support of CreateSpan (#60948), improve alignment for RVA static pre-initialized fields to align memory blocks which may contain long, ulong, or double primitive arrays on 8 byte boundaries.

Mono fix is more involved
- Fix Ref-Emit generated RVA statics
 - Set the HasFieldRVA bit. NOTE: earlier code that attempts to set the bit doesn't actually do that as the FieldRVA bit is in  FieldAttributes.ReservedMask which is masked away in the FieldBuilder constructor
 - Fix the Swizzle lookup. We should not be using the 8 byte swizzle in the final else clause.
- Enhance ref-emitted field to allow for use with CreateSpan
  - Ref-emitted fields should specify a pack size appropriate for minimum alignment of the CreateSpan targetted data
  - Respect the packing_size specified so that RVA static fields generated for CreateSpan can be properly aligned

Fixes #62314
  • Loading branch information
davidwrighton committed Jan 27, 2022
1 parent 4c09cb9 commit 014b084
Show file tree
Hide file tree
Showing 7 changed files with 82 additions and 6 deletions.
10 changes: 8 additions & 2 deletions src/coreclr/vm/commodule.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -499,14 +499,20 @@ extern "C" void QCALLTYPE ModuleBuilder_SetFieldRVAContent(QCall::ModuleHandle p
if (pReflectionModule->m_sdataSection == 0)
IfFailThrow( pGen->GetSectionCreate (".sdata", sdReadWrite, &pReflectionModule->m_sdataSection) );

// Define the alignment that the rva will be set to. Since the CoreCLR runtime only has hard alignment requirements
// up to 8 bytes, the highest alignment we may need is 8 byte alignment. This hard alignment requirement is only needed
// by Runtime.Helpers.CreateSpan<T>. Since the previous alignment was 4 bytes before CreateSpan was implemented, if the
// data isn't itself of size divisible by 8, just align to 4 to the memory cost of excess alignment.
DWORD alignment = (length % 8 == 0) ? 8 : 4;

// Get the size of current .sdata section. This will be the RVA for this field within the section
DWORD dwRVA = 0;
IfFailThrow( pGen->GetSectionDataLen(pReflectionModule->m_sdataSection, &dwRVA) );
dwRVA = (dwRVA + sizeof(DWORD)-1) & ~(sizeof(DWORD)-1);
dwRVA = (dwRVA + alignment-1) & ~(alignment-1);

// allocate the space in .sdata section
void * pvBlob;
IfFailThrow( pGen->GetSectionBlock(pReflectionModule->m_sdataSection, length, sizeof(DWORD), (void**) &pvBlob) );
IfFailThrow( pGen->GetSectionBlock(pReflectionModule->m_sdataSection, length, alignment, (void**) &pvBlob) );

// copy over the initialized data if specified
if (pContent != NULL)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@
// The .NET Foundation licenses this file to you under the MIT license.

using Xunit;
using System.Runtime.InteropServices;

namespace System.Reflection.Emit.Tests
{
Expand Down Expand Up @@ -59,5 +60,59 @@ public void DefineInitializedData_CreateGlobalFunctionsCalled_ThrowsInvalidOpera
module.CreateGlobalFunctions();
Assert.Throws<InvalidOperationException>(() => module.DefineInitializedData("MyField2", new byte[] { 1, 0, 1 }, FieldAttributes.Public));
}

[Fact]
public void DefineInitializedData_EnsureAlignmentIsMinimumNeededForUseOfCreateSpan()
{
ModuleBuilder module = Helpers.DynamicModule();

// Create static field data in a variety of orders that requires the runtime to actively apply alignment
// RuntimeHelpers.CreateSpan requires data to be naturally aligned within the "PE" file. At this time CreateSpan only
// requires alignments up to 8 bytes.
FieldBuilder field1Byte = module.DefineInitializedData("Field1Byte", new byte[] { 1 }, FieldAttributes.Public);
byte[] field4Byte_1_data = new byte[] { 1, 2, 3, 4 };
byte[] field8Byte_1_data = new byte[] { 1, 2, 3, 4, 5, 6, 7, 8 };
byte[] field4Byte_2_data = new byte[] { 5, 6, 7, 8 };
byte[] field8Byte_2_data = new byte[] { 9, 10, 11, 12, 13, 14, 15, 16 };
FieldBuilder field4Byte_1 = module.DefineInitializedData("Field4Bytes_1", field4Byte_1_data, FieldAttributes.Public);
FieldBuilder field8Byte_1 = module.DefineInitializedData("Field8Bytes_1", field8Byte_1_data, FieldAttributes.Public);
FieldBuilder field4Byte_2 = module.DefineInitializedData("Field4Bytes_2", field4Byte_2_data, FieldAttributes.Public);
FieldBuilder field8Byte_2 = module.DefineInitializedData("Field8Bytes_2", field8Byte_2_data, FieldAttributes.Public);
module.CreateGlobalFunctions();

var checkTypeBuilder = module.DefineType("CheckType", TypeAttributes.Public);
CreateLoadAddressMethod("LoadAddress1", field1Byte);
CreateLoadAddressMethod("LoadAddress4_1", field4Byte_1);
CreateLoadAddressMethod("LoadAddress4_2", field4Byte_2);
CreateLoadAddressMethod("LoadAddress8_1", field8Byte_1);
CreateLoadAddressMethod("LoadAddress8_2", field8Byte_2);

var checkType = checkTypeBuilder.CreateType();

CheckMethod("LoadAddress4_1", 4, field4Byte_1_data);
CheckMethod("LoadAddress4_2", 4, field4Byte_2_data);
CheckMethod("LoadAddress8_1", 8, field8Byte_1_data);
CheckMethod("LoadAddress8_2", 8, field8Byte_2_data);

void CreateLoadAddressMethod(string name, FieldBuilder fieldBuilder)
{
var loadAddressMethod = checkTypeBuilder.DefineMethod(name, MethodAttributes.Public | MethodAttributes.Static, typeof(IntPtr), null);
var methodIL = loadAddressMethod.GetILGenerator();
methodIL.Emit(OpCodes.Ldsflda, fieldBuilder);
methodIL.Emit(OpCodes.Ret);
}

void CheckMethod(string name, int minAlignmentRequired, byte[] dataToVerify)
{
var methodToCall = checkType.GetMethod(name);
nint address = (nint)methodToCall.Invoke(null, null);

for (int i = 0; i < dataToVerify.Length; i++)
{
Assert.Equal(dataToVerify[i], Marshal.ReadByte(address + (nint)i));
}
Assert.Equal(name + "_0" + "_" + address.ToString(), name + "_" + (address % minAlignmentRequired).ToString() + "_" + address.ToString());
}
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -149,9 +149,18 @@ internal override int GetFieldOffset()

internal void SetRVAData(byte[] data)
{
attrs = attrs | FieldAttributes.HasFieldRVA;
rva_data = (byte[])data.Clone();
}

internal static PackingSize RVADataPackingSize(int size)
{
if ((size % 8) == 0) return PackingSize.Size8;
if ((size % 4) == 0) return PackingSize.Size4;
if ((size % 2) == 0) return PackingSize.Size2;
return PackingSize.Size1;
}

public void SetConstant(object? defaultValue)
{
RejectIfCreated();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -165,7 +165,7 @@ private FieldBuilder DefineDataImpl(string name, int size, FieldAttributes attri
{
TypeBuilder tb = DefineType(typeName,
TypeAttributes.Public | TypeAttributes.ExplicitLayout | TypeAttributes.Sealed,
typeof(ValueType), null, PackingSize.Size1, size);
typeof(ValueType), null, FieldBuilder.RVADataPackingSize(size), size);
tb.CreateType();
datablobtype = tb;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -1608,7 +1608,7 @@ public FieldBuilder DefineUninitializedData(string name, int size, FieldAttribut
{
TypeBuilder tb = DefineNestedType(typeName,
TypeAttributes.NestedPrivate | TypeAttributes.ExplicitLayout | TypeAttributes.Sealed,
typeof(ValueType), null, PackingSize.Size1, size);
typeof(ValueType), null, FieldBuilder.RVADataPackingSize(size), size);
tb.CreateType();
datablobtype = tb;
}
Expand Down
4 changes: 2 additions & 2 deletions src/mono/mono/metadata/class-accessors.c
Original file line number Diff line number Diff line change
Expand Up @@ -396,7 +396,7 @@ mono_class_get_field_def_values_with_swizzle (MonoClass *klass, int swizzle)
dataKind = PROP_FIELD_DEF_VALUES_2BYTESWIZZLE;
else if (swizzle == 4)
dataKind = PROP_FIELD_DEF_VALUES_4BYTESWIZZLE;
else
else if (swizzle == 8)
dataKind = PROP_FIELD_DEF_VALUES_8BYTESWIZZLE;
return (MonoFieldDefaultValue*)get_pointer_property (klass, dataKind);
}
Expand All @@ -416,7 +416,7 @@ mono_class_set_field_def_values_with_swizzle (MonoClass *klass, MonoFieldDefault
dataKind = PROP_FIELD_DEF_VALUES_2BYTESWIZZLE;
else if (swizzle == 4)
dataKind = PROP_FIELD_DEF_VALUES_4BYTESWIZZLE;
else
else if (swizzle == 8)
dataKind = PROP_FIELD_DEF_VALUES_8BYTESWIZZLE;
set_pointer_property (klass, dataKind, values);
}
Expand Down
6 changes: 6 additions & 0 deletions src/mono/mono/metadata/class-init.c
Original file line number Diff line number Diff line change
Expand Up @@ -2017,6 +2017,12 @@ mono_class_layout_fields (MonoClass *klass, int base_instance_size, int packing_
min_align = 16;
*/

/* Respect the specified packing size at least to the extent necessary to align double variables.
* This should avoid any GC problems, and will allow packing_size to be respected to support
* CreateSpan<T>
*/
min_align = MIN (MONO_ABI_ALIGNOF (double), MAX (min_align, packing_size));

/*
* When we do generic sharing we need to have layout
* information for open generic classes (either with a generic
Expand Down

0 comments on commit 014b084

Please sign in to comment.