From 1be0fb324d37c4672ef9cef86254ce0214100f0b Mon Sep 17 00:00:00 2001 From: Buyaa Namnan Date: Thu, 11 Jan 2024 11:15:33 -0800 Subject: [PATCH] Apply feedbacks, fix RVA alignment --- .../Emit/GenericTypeParameterBuilderImpl.cs | 4 +- .../Reflection/Emit/MethodBuilderImpl.cs | 8 +-- .../Reflection/Emit/ModuleBuilderImpl.cs | 15 +++-- .../Emit/PseudoCustomAttributesData.cs | 4 +- .../System/Reflection/Emit/TypeBuilderImpl.cs | 2 +- .../AssemblySaveModuleBuilderTests.cs | 12 +++- .../AssemblySaveTypeBuilderAPIsTests.cs | 66 ------------------- .../PortableExecutable/ManagedTextSection.cs | 2 +- 8 files changed, 28 insertions(+), 85 deletions(-) diff --git a/src/libraries/System.Reflection.Emit/src/System/Reflection/Emit/GenericTypeParameterBuilderImpl.cs b/src/libraries/System.Reflection.Emit/src/System/Reflection/Emit/GenericTypeParameterBuilderImpl.cs index 0e02794e4c29b..fccd56de74e79 100644 --- a/src/libraries/System.Reflection.Emit/src/System/Reflection/Emit/GenericTypeParameterBuilderImpl.cs +++ b/src/libraries/System.Reflection.Emit/src/System/Reflection/Emit/GenericTypeParameterBuilderImpl.cs @@ -29,12 +29,12 @@ internal GenericTypeParameterBuilderImpl(string name, int genParamPosition, Type _type = typeBuilder; } - public GenericTypeParameterBuilderImpl(string name, int genParamPosition, MethodBuilderImpl methodBuilder) + public GenericTypeParameterBuilderImpl(string name, int genParamPosition, MethodBuilderImpl methodBuilder, TypeBuilderImpl typeBuilder) { _name = name; _genParamPosition = genParamPosition; _methodBuilder = methodBuilder; - _type = (TypeBuilder)methodBuilder.DeclaringType!; + _type = typeBuilder; } protected override void SetBaseTypeConstraintCore([DynamicallyAccessedMembers(DynamicallyAccessedMemberTypes.All)] Type? baseTypeConstraint) diff --git a/src/libraries/System.Reflection.Emit/src/System/Reflection/Emit/MethodBuilderImpl.cs b/src/libraries/System.Reflection.Emit/src/System/Reflection/Emit/MethodBuilderImpl.cs index fe28bce5568c3..a20e067e6b006 100644 --- a/src/libraries/System.Reflection.Emit/src/System/Reflection/Emit/MethodBuilderImpl.cs +++ b/src/libraries/System.Reflection.Emit/src/System/Reflection/Emit/MethodBuilderImpl.cs @@ -76,9 +76,9 @@ internal MethodBuilderImpl(string name, MethodAttributes attributes, CallingConv _initLocals = true; } - internal void CreateDllData(string dllName, string entryName, CallingConvention nativeCallConv, CharSet nativeCharSet) + internal void CreateDllImportData(string dllName, string entryName, CallingConvention nativeCallConv, CharSet nativeCharSet) { - _dllImportData = DllImportData.CreateDllImportData(dllName, entryName, nativeCallConv, nativeCharSet); + _dllImportData = DllImportData.Create(dllName, entryName, nativeCallConv, nativeCharSet); } internal int ParameterCount => _parameterTypes == null ? 0 : _parameterTypes.Length; @@ -122,7 +122,7 @@ protected override GenericTypeParameterBuilder[] DefineGenericParametersCore(par { string name = names[i]; ArgumentNullException.ThrowIfNull(names, nameof(names)); - typeParameters[i] = new GenericTypeParameterBuilderImpl(name, i, this); + typeParameters[i] = new GenericTypeParameterBuilderImpl(name, i, this, _declaringType); } return _typeParameters = typeParameters; @@ -182,7 +182,7 @@ protected override void SetCustomAttributeCore(ConstructorInfo con, ReadOnlySpan return; case "System.Runtime.InteropServices.DllImportAttribute": { - _dllImportData = DllImportData.CreateDllImportData(CustomAttributeInfo.DecodeCustomAttribute(con, binaryAttribute), out var preserveSig); + _dllImportData = DllImportData.Create(CustomAttributeInfo.DecodeCustomAttribute(con, binaryAttribute), out var preserveSig); _attributes |= MethodAttributes.PinvokeImpl; _canBeRuntimeImpl = true; if (preserveSig) diff --git a/src/libraries/System.Reflection.Emit/src/System/Reflection/Emit/ModuleBuilderImpl.cs b/src/libraries/System.Reflection.Emit/src/System/Reflection/Emit/ModuleBuilderImpl.cs index 5c7ec3c220894..338dfcb600215 100644 --- a/src/libraries/System.Reflection.Emit/src/System/Reflection/Emit/ModuleBuilderImpl.cs +++ b/src/libraries/System.Reflection.Emit/src/System/Reflection/Emit/ModuleBuilderImpl.cs @@ -130,6 +130,7 @@ internal void AppendMetadata(MethodBodyStreamEncoder methodBodyEncoder, BlobBuil // All generic parameters for all types and methods should be written in specific order List genericParams = new(); PopulateTokensForTypesAndItsMembers(); + // Add global members WriteFields(_globalTypeBuilder, fieldDataBuilder); WriteMethods(_globalTypeBuilder._methodDefinitions, genericParams, methodBodyEncoder); @@ -449,6 +450,7 @@ private void WriteFields(TypeBuilderImpl typeBuilder, BlobBuilder fieldDataBuild { _metadataBuilder.AddFieldRelativeVirtualAddress(handle, fieldDataBuilder.Count); fieldDataBuilder.WriteBytes(field._rvaData!); + fieldDataBuilder.Align(8); } } } @@ -516,18 +518,19 @@ private MethodSpecificationHandle AddMethodSpecification(EntityHandle methodHand method: methodHandle, instantiation: _metadataBuilder.GetOrAddBlob(MetadataSignatureHelper.GetMethodSpecificationSignature(genericArgs, this))); - private EntityHandle GetMemberReferenceHandle(MemberInfo member) + private EntityHandle GetMemberReferenceHandle(MemberInfo memberInfo) { - if (!_memberReferences.TryGetValue(member, out var memberHandle)) + if (!_memberReferences.TryGetValue(memberInfo, out var memberHandle)) { + MemberInfo member = GetOriginalMemberIfConstructedType(memberInfo); switch (member) { case FieldInfo field: - memberHandle = AddMemberReference(field.Name, GetTypeHandle(field.DeclaringType!), + memberHandle = AddMemberReference(field.Name, GetTypeHandle(memberInfo.DeclaringType!), MetadataSignatureHelper.GetFieldSignature(field.FieldType, field.GetRequiredCustomModifiers(), field.GetOptionalCustomModifiers(), this)); break; case ConstructorInfo ctor: - memberHandle = AddMemberReference(ctor.Name, GetTypeHandle(ctor.DeclaringType!), MetadataSignatureHelper.GetConstructorSignature(ctor.GetParameters(), this)); + memberHandle = AddMemberReference(ctor.Name, GetTypeHandle(memberInfo.DeclaringType!), MetadataSignatureHelper.GetConstructorSignature(ctor.GetParameters(), this)); break; case MethodInfo method: if (method.IsConstructedGenericMethod) @@ -536,7 +539,7 @@ private EntityHandle GetMemberReferenceHandle(MemberInfo member) } else { - memberHandle = AddMemberReference(method.Name, GetTypeHandle(method.DeclaringType!), GetMethodSignature(method, null)); + memberHandle = AddMemberReference(method.Name, GetTypeHandle(memberInfo.DeclaringType!), GetMethodSignature(method, null)); } break; default: @@ -544,7 +547,7 @@ private EntityHandle GetMemberReferenceHandle(MemberInfo member) } - _memberReferences.Add(member, memberHandle); + _memberReferences.Add(memberInfo, memberHandle); } return memberHandle; diff --git a/src/libraries/System.Reflection.Emit/src/System/Reflection/Emit/PseudoCustomAttributesData.cs b/src/libraries/System.Reflection.Emit/src/System/Reflection/Emit/PseudoCustomAttributesData.cs index 7afbe18552a4f..e6f3b480f8952 100644 --- a/src/libraries/System.Reflection.Emit/src/System/Reflection/Emit/PseudoCustomAttributesData.cs +++ b/src/libraries/System.Reflection.Emit/src/System/Reflection/Emit/PseudoCustomAttributesData.cs @@ -26,7 +26,7 @@ internal DllImportData(string moduleName, string? entryPoint, MethodImportAttrib public MethodImportAttributes Flags => _flags; - internal static DllImportData CreateDllImportData(CustomAttributeInfo attr, out bool preserveSig) + internal static DllImportData Create(CustomAttributeInfo attr, out bool preserveSig) { string? moduleName = (string?)attr._ctorArgs[0]; if (string.IsNullOrEmpty(moduleName)) @@ -93,7 +93,7 @@ internal static DllImportData CreateDllImportData(CustomAttributeInfo attr, out return new DllImportData(moduleName, entryPoint, importAttributes); } - internal static DllImportData CreateDllImportData(string moduleName, string entryName, CallingConvention nativeCallConv, CharSet nativeCharSet) + internal static DllImportData Create(string moduleName, string entryName, CallingConvention nativeCallConv, CharSet nativeCharSet) { if (string.IsNullOrEmpty(moduleName)) { diff --git a/src/libraries/System.Reflection.Emit/src/System/Reflection/Emit/TypeBuilderImpl.cs b/src/libraries/System.Reflection.Emit/src/System/Reflection/Emit/TypeBuilderImpl.cs index 35496c9dedc95..3b3bb96bf434b 100644 --- a/src/libraries/System.Reflection.Emit/src/System/Reflection/Emit/TypeBuilderImpl.cs +++ b/src/libraries/System.Reflection.Emit/src/System/Reflection/Emit/TypeBuilderImpl.cs @@ -517,7 +517,7 @@ protected override MethodBuilder DefinePInvokeMethodCore(string name, string dll attributes |= MethodAttributes.PinvokeImpl; MethodBuilderImpl method = new MethodBuilderImpl(name, attributes, callingConvention, returnType, returnTypeRequiredCustomModifiers, returnTypeOptionalCustomModifiers, parameterTypes, parameterTypeRequiredCustomModifiers, parameterTypeOptionalCustomModifiers, _module, this); - method.CreateDllData(dllName, entryName, nativeCallConv, nativeCharSet); + method.CreateDllImportData(dllName, entryName, nativeCallConv, nativeCharSet); if (_methodDefinitions.Contains(method)) { diff --git a/src/libraries/System.Reflection.Emit/tests/PersistableAssemblyBuilder/AssemblySaveModuleBuilderTests.cs b/src/libraries/System.Reflection.Emit/tests/PersistableAssemblyBuilder/AssemblySaveModuleBuilderTests.cs index 294092cd2dbb3..e731d7636f883 100644 --- a/src/libraries/System.Reflection.Emit/tests/PersistableAssemblyBuilder/AssemblySaveModuleBuilderTests.cs +++ b/src/libraries/System.Reflection.Emit/tests/PersistableAssemblyBuilder/AssemblySaveModuleBuilderTests.cs @@ -194,7 +194,7 @@ public void DefineInitializedData_EnsureAlignmentIsMinimumNeededForUseOfCreateSp { AssemblyBuilder ab = AssemblySaveTools.PopulateAssemblyBuilderAndSaveMethod(new AssemblyName("MyAssembly"), out MethodInfo saveMethod); ModuleBuilder module = ab.DefineDynamicModule("MyModule"); - + TypeBuilder tb = module.DefineType("MyType", TypeAttributes.Public); // 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. @@ -204,11 +204,13 @@ public void DefineInitializedData_EnsureAlignmentIsMinimumNeededForUseOfCreateSp 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 tbField4Byte_1 = tb.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); + FieldBuilder tbField8Byte_2 = tb.DefineInitializedData("Field8Bytes_2", field8Byte_2_data, FieldAttributes.Public); module.CreateGlobalFunctions(); - + tb.CreateType(); Assert.Null(field4Byte_1.DeclaringType); Assert.Null(field8Byte_1.DeclaringType); Assert.Null(field4Byte_2.DeclaringType); @@ -217,9 +219,11 @@ public void DefineInitializedData_EnsureAlignmentIsMinimumNeededForUseOfCreateSp var checkTypeBuilder = module.DefineType("CheckType", TypeAttributes.Public); CreateLoadAddressMethod("LoadAddress1", field1Byte); CreateLoadAddressMethod("LoadAddress4_1", field4Byte_1); + CreateLoadAddressMethod("LoadAddress4_3", tbField4Byte_1); CreateLoadAddressMethod("LoadAddress4_2", field4Byte_2); CreateLoadAddressMethod("LoadAddress8_1", field8Byte_1); CreateLoadAddressMethod("LoadAddress8_2", field8Byte_2); + CreateLoadAddressMethod("LoadAddress8_3", tbField8Byte_2); void CreateLoadAddressMethod(string name, FieldBuilder fieldBuilder) { @@ -236,9 +240,11 @@ void CreateLoadAddressMethod(string name, FieldBuilder fieldBuilder) Type checkType = assemblyFromDisk.GetType("CheckType"); CheckMethod("LoadAddress4_1", 4, field4Byte_1_data); + CheckMethod("LoadAddress4_3", 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); + CheckMethod("LoadAddress8_3", 8, field8Byte_2_data); void CheckMethod(string name, int minAlignmentRequired, byte[] dataToVerify) { @@ -249,7 +255,7 @@ void CheckMethod(string name, int minAlignmentRequired, byte[] dataToVerify) { Assert.Equal(dataToVerify[i], Marshal.ReadByte(address + (nint)i)); } - //Assert.Equal(name + "_0" + "_" + address.ToString(), name + "_" + (address % minAlignmentRequired).ToString() + "_" + address.ToString()); + Assert.Equal(name + "_0" + "_" + address.ToString(), name + "_" + (address % minAlignmentRequired).ToString() + "_" + address.ToString()); } } diff --git a/src/libraries/System.Reflection.Emit/tests/PersistableAssemblyBuilder/AssemblySaveTypeBuilderAPIsTests.cs b/src/libraries/System.Reflection.Emit/tests/PersistableAssemblyBuilder/AssemblySaveTypeBuilderAPIsTests.cs index 5d4ca2a740f92..7363d46f64bbb 100644 --- a/src/libraries/System.Reflection.Emit/tests/PersistableAssemblyBuilder/AssemblySaveTypeBuilderAPIsTests.cs +++ b/src/libraries/System.Reflection.Emit/tests/PersistableAssemblyBuilder/AssemblySaveTypeBuilderAPIsTests.cs @@ -691,71 +691,5 @@ public void DefineTypeInitializer() return RemoteExecutor.SuccessExitCode; }).Dispose(); } - - [ConditionalFact(typeof(RemoteExecutor), nameof(RemoteExecutor.IsSupported))] - public void DefineInitializedData_EnsureAlignmentIsMinimumNeededForUseOfCreateSpan() - { - RemoteExecutor.Invoke(() => - { - using (TempFile file = TempFile.Create()) - { - AssemblyBuilder ab = AssemblySaveTools.PopulateAssemblyBuilderTypeBuilderAndSaveMethod(out TypeBuilder tb, out MethodInfo saveMethod); - - // 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 = tb.DefineInitializedData("Field1Byte", new byte[] { 1 }, FieldAttributes.Public); - byte[] field4Byte_1_data = [1, 2, 3, 4]; - byte[] field8Byte_1_data = [1, 2, 3, 4, 5, 6, 7, 8]; - byte[] field4Byte_2_data = [5, 6, 7, 8]; - byte[] field8Byte_2_data = [9, 10, 11, 12, 13, 14, 15, 16]; - FieldBuilder field4Byte_1 = tb.DefineInitializedData("Field4Bytes_1", field4Byte_1_data, FieldAttributes.Public); - FieldBuilder field8Byte_1 = tb.DefineInitializedData("Field8Bytes_1", field8Byte_1_data, FieldAttributes.Public); - FieldBuilder field4Byte_2 = tb.DefineInitializedData("Field4Bytes_2", field4Byte_2_data, FieldAttributes.Public); - FieldBuilder field8Byte_2 = tb.DefineInitializedData("Field8Bytes_2", field8Byte_2_data, FieldAttributes.Public); - tb.CreateType(); - - var checkTypeBuilder = ab.GetDynamicModule("MyModule").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); - - 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); - } - - checkTypeBuilder.CreateType(); - saveMethod.Invoke(ab, [file.Path]); - - Assembly assemblyFromDisk = Assembly.LoadFile(file.Path); - Type checkType = assemblyFromDisk.GetType("CheckType"); - - 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 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()); - } - } - - return RemoteExecutor.SuccessExitCode; - }).Dispose(); - } } } diff --git a/src/libraries/System.Reflection.Metadata/src/System/Reflection/PortableExecutable/ManagedTextSection.cs b/src/libraries/System.Reflection.Metadata/src/System/Reflection/PortableExecutable/ManagedTextSection.cs index 90564c8932748..bd30dc7e49102 100644 --- a/src/libraries/System.Reflection.Metadata/src/System/Reflection/PortableExecutable/ManagedTextSection.cs +++ b/src/libraries/System.Reflection.Metadata/src/System/Reflection/PortableExecutable/ManagedTextSection.cs @@ -187,7 +187,7 @@ private int ComputeOffsetToMetadata() public int ComputeSizeOfTextSection() { - // Debug.Assert(MappedFieldDataSize % MappedFieldDataAlignment == 0); + Debug.Assert(MappedFieldDataSize % MappedFieldDataAlignment == 0); return CalculateOffsetToMappedFieldDataStream() + MappedFieldDataSize; }