From 167e9f4d70e5754fce2a884461122b88f5cf4f7a Mon Sep 17 00:00:00 2001 From: Fadi Hanna Date: Mon, 2 Dec 2019 15:31:52 -0800 Subject: [PATCH] Fix RVA field ordering and emission issue. (#375) * Fix RVA field ordering and emission issue. * CopiedFieldRvaNode have to be sorted by rva (Managed C++ binaries depend on it) * Proper handling for overlapping fields, even with different sizes (updated test to cover this). * Move field RVA overlap handling to CopiedFieldRvaNode --- .../src/tools/Common/TypeSystem/Ecma/EcmaField.cs | 13 ++++- .../ReadyToRun/CopiedFieldRvaNode.cs | 65 ++++++++++++++++++---- .../ReadyToRunCodegenNodeFactory.cs | 28 ++++++++-- .../src/JIT/Directed/rvastatics/RVAOrderingTest.il | 10 ++-- 4 files changed, 93 insertions(+), 23 deletions(-) diff --git a/src/coreclr/src/tools/Common/TypeSystem/Ecma/EcmaField.cs b/src/coreclr/src/tools/Common/TypeSystem/Ecma/EcmaField.cs index ee492d0..81ed1ec 100644 --- a/src/coreclr/src/tools/Common/TypeSystem/Ecma/EcmaField.cs +++ b/src/coreclr/src/tools/Common/TypeSystem/Ecma/EcmaField.cs @@ -8,8 +8,6 @@ using System.Reflection; using System.Reflection.Metadata; using System.Runtime.CompilerServices; -using Internal.TypeSystem; - namespace Internal.TypeSystem.Ecma { public sealed partial class EcmaField : FieldDesc, EcmaModule.IEntityHandleObject @@ -278,6 +276,15 @@ namespace Internal.TypeSystem.Ecma public static class EcmaFieldExtensions { /// + /// Returns the RVA associated with an RVA mapped field from the PE module. + /// + public static int GetFieldRvaValue(this EcmaField field) + { + Debug.Assert(field.HasRva); + return field.MetadataReader.GetFieldDefinition(field.Handle).GetRelativeVirtualAddress(); + } + + /// /// Retrieves the data associated with an RVA mapped field from the PE module. /// public static byte[] GetFieldRvaData(this EcmaField field) @@ -291,7 +298,7 @@ namespace Internal.TypeSystem.Ecma throw new BadImageFormatException(); byte[] result = new byte[size]; - memBlock.CopyTo(0, result, 0, result.Length); + memBlock.CopyTo(0, result, 0, size); return result; } diff --git a/src/coreclr/src/tools/crossgen2/ILCompiler.ReadyToRun/Compiler/DependencyAnalysis/ReadyToRun/CopiedFieldRvaNode.cs b/src/coreclr/src/tools/crossgen2/ILCompiler.ReadyToRun/Compiler/DependencyAnalysis/ReadyToRun/CopiedFieldRvaNode.cs index 3aec94b..cd4b3e7 100644 --- a/src/coreclr/src/tools/crossgen2/ILCompiler.ReadyToRun/Compiler/DependencyAnalysis/ReadyToRun/CopiedFieldRvaNode.cs +++ b/src/coreclr/src/tools/crossgen2/ILCompiler.ReadyToRun/Compiler/DependencyAnalysis/ReadyToRun/CopiedFieldRvaNode.cs @@ -3,23 +3,25 @@ // See the LICENSE file in the project root for more information. using System; - +using System.Collections.Immutable; +using System.Reflection.Metadata; +using System.Reflection.Metadata.Ecma335; using Internal.Text; +using Internal.TypeSystem; using Internal.TypeSystem.Ecma; - using Debug = System.Diagnostics.Debug; namespace ILCompiler.DependencyAnalysis.ReadyToRun { public class CopiedFieldRvaNode : ObjectNode, ISymbolDefinitionNode { - private EcmaField _field; + private int _rva; + private EcmaModule _module; - public CopiedFieldRvaNode(EcmaField field) + public CopiedFieldRvaNode(EcmaModule module, int rva) { - Debug.Assert(field.HasRva); - - _field = field; + _rva = rva; + _module = module; } public override ObjectNodeSection Section => ObjectNodeSection.TextSection; @@ -46,10 +48,47 @@ namespace ILCompiler.DependencyAnalysis.ReadyToRun ObjectDataBuilder builder = new ObjectDataBuilder(factory, relocsOnly); builder.RequireInitialPointerAlignment(); builder.AddSymbol(this); + builder.EmitBytes(GetRvaData(factory.Target.PointerSize)); + return builder.ToObjectData(); + } - builder.EmitBytes(_field.GetFieldRvaData()); + private unsafe byte[] GetRvaData(int targetPointerSize) + { + int size = 0; + byte[] result = Array.Empty(); - return builder.ToObjectData(); + MetadataReader metadataReader = _module.MetadataReader; + BlobReader metadataBlob = new BlobReader(_module.PEReader.GetMetadata().Pointer, _module.PEReader.GetMetadata().Length); + metadataBlob.Offset = metadataReader.GetTableMetadataOffset(TableIndex.FieldRva); + + ImmutableArray memBlock = _module.PEReader.GetSectionData(_rva).GetContent(); + + for (int i = 1; i <= metadataReader.GetTableRowCount(TableIndex.FieldRva); i++) + { + int currentFieldRva = metadataBlob.ReadInt32(); + short currentFieldRid = metadataBlob.ReadInt16(); + if (currentFieldRva != _rva) + continue; + + EcmaField field = (EcmaField)_module.GetField(MetadataTokens.FieldDefinitionHandle(currentFieldRid)); + Debug.Assert(field.HasRva); + + int currentSize = field.FieldType.GetElementSize().AsInt; + if (currentSize > size) + { + if (currentSize > memBlock.Length) + throw new BadImageFormatException(); + + // We need to handle overlapping fields by reusing blobs based on the rva, and just update + // the size and contents + size = currentSize; + result = new byte[AlignmentHelper.AlignUp(size, targetPointerSize)]; + memBlock.CopyTo(0, result, 0, size); + } + } + + Debug.Assert(size > 0); + return result; } protected override string GetName(NodeFactory factory) => this.GetMangledName(factory.NameMangler); @@ -57,12 +96,16 @@ namespace ILCompiler.DependencyAnalysis.ReadyToRun public void AppendMangledName(NameMangler nameMangler, Utf8StringBuilder sb) { sb.Append(nameMangler.CompilationUnitPrefix); - sb.Append($"_FieldRva_{nameMangler.GetMangledFieldName(_field)}"); + sb.Append($"_FieldRvaData_{_module.Assembly.GetName().Name}_{_rva}"); } public override int CompareToImpl(ISortableNode other, CompilerComparer comparer) { - return comparer.Compare(_field, ((CopiedFieldRvaNode)other)._field); + int result = _module.CompareTo(((CopiedFieldRvaNode)other)._module); + if (result != 0) + return result; + + return _rva - ((CopiedFieldRvaNode)other)._rva; } } } diff --git a/src/coreclr/src/tools/crossgen2/ILCompiler.ReadyToRun/Compiler/DependencyAnalysis/ReadyToRunCodegenNodeFactory.cs b/src/coreclr/src/tools/crossgen2/ILCompiler.ReadyToRun/Compiler/DependencyAnalysis/ReadyToRunCodegenNodeFactory.cs index 33a39ba..0eb39b4 100644 --- a/src/coreclr/src/tools/crossgen2/ILCompiler.ReadyToRun/Compiler/DependencyAnalysis/ReadyToRunCodegenNodeFactory.cs +++ b/src/coreclr/src/tools/crossgen2/ILCompiler.ReadyToRun/Compiler/DependencyAnalysis/ReadyToRunCodegenNodeFactory.cs @@ -199,6 +199,26 @@ namespace ILCompiler.DependencyAnalysis public override bool Equals(object obj) => obj is ReadOnlyDataBlobKey && Equals((ReadOnlyDataBlobKey)obj); public override int GetHashCode() => Name.GetHashCode(); } + + protected struct FieldRvaKey : IEquatable + { + public readonly int Rva; + public readonly EcmaModule Module; + + public FieldRvaKey(int rva, EcmaModule module) + { + Rva = rva; + Module = module; + } + + public bool Equals(FieldRvaKey other) => Rva == other.Rva && Module.Equals(other.Module); + public override bool Equals(object obj) => obj is FieldRvaKey && Equals((FieldRvaKey)obj); + public override int GetHashCode() + { + int hashCode = Rva * 0x5498341 + 0x832424; + return hashCode * 23 + Module.GetHashCode(); + } + } } // To make the code future compatible to the composite R2R story @@ -293,9 +313,9 @@ namespace ILCompiler.DependencyAnalysis return new CopiedMethodILNode((EcmaMethod)method); }); - _copiedFieldRvas = new NodeCache(ecmaField => + _copiedFieldRvas = new NodeCache(key => { - return new CopiedFieldRvaNode(ecmaField); + return new CopiedFieldRvaNode(key.Module, key.Rva); }); _copiedStrongNameSignatures = new NodeCache(module => @@ -843,7 +863,7 @@ namespace ILCompiler.DependencyAnalysis return _copiedMethodIL.GetOrAdd(method); } - private NodeCache _copiedFieldRvas; + private NodeCache _copiedFieldRvas; public CopiedFieldRvaNode CopiedFieldRva(FieldDesc field) { @@ -861,7 +881,7 @@ namespace ILCompiler.DependencyAnalysis throw new NotSupportedException($"{ecmaField} ... {string.Join("; ", TypeSystemContext.InputFilePaths.Keys)}"); } - return _copiedFieldRvas.GetOrAdd(ecmaField); + return _copiedFieldRvas.GetOrAdd(new FieldRvaKey(ecmaField.GetFieldRvaValue(), ecmaField.Module)); } private NodeCache _copiedStrongNameSignatures; diff --git a/src/coreclr/tests/src/JIT/Directed/rvastatics/RVAOrderingTest.il b/src/coreclr/tests/src/JIT/Directed/rvastatics/RVAOrderingTest.il index 3f8faf7..9916451 100644 --- a/src/coreclr/tests/src/JIT/Directed/rvastatics/RVAOrderingTest.il +++ b/src/coreclr/tests/src/JIT/Directed/rvastatics/RVAOrderingTest.il @@ -44,8 +44,8 @@ IL_0018: ldloc.3 IL_0019: call int32 RVAOrderingTest::AddFields(int32*, int32*) - ldsflda int32 RVAOrderingTest::s_Another1 - ldind.i4 + ldsflda int8 RVAOrderingTest::s_Another1 + ldind.i1 add IL_001e: stloc.0 @@ -62,7 +62,7 @@ object) IL_0036: nop IL_0037: ldloc.0 - IL_0038: ldc.i4 0x600000f2 + IL_0038: ldc.i4 0x500000f2 IL_003d: ceq IL_003f: stloc.s V_5 IL_0041: ldloc.s V_5 @@ -139,13 +139,13 @@ } // end of method RVAOrderingTest::.ctor .field public static int32 s_First at D_00014000 + .field public static int32 s_Last at D_00014030 .field public static int32 s_1 at D_00014008 - .field public static int32 s_Another1 at D_00014008 + .field public static int8 s_Another1 at D_00014008 .field public static int32 s_2 at D_00014010 .field public static int32 s_3 at D_00014018 .field public static int32 s_4 at D_00014020 .field public static int32 s_5 at D_00014028 - .field public static int32 s_Last at D_00014030 } // end of class RVAOrderingTest -- 2.7.4