Fix RVA field ordering and emission issue. (#375)
authorFadi Hanna <fadim@microsoft.com>
Mon, 2 Dec 2019 23:31:52 +0000 (15:31 -0800)
committerGitHub <noreply@github.com>
Mon, 2 Dec 2019 23:31:52 +0000 (15:31 -0800)
* 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/coreclr/src/tools/Common/TypeSystem/Ecma/EcmaField.cs
src/coreclr/src/tools/crossgen2/ILCompiler.ReadyToRun/Compiler/DependencyAnalysis/ReadyToRun/CopiedFieldRvaNode.cs
src/coreclr/src/tools/crossgen2/ILCompiler.ReadyToRun/Compiler/DependencyAnalysis/ReadyToRunCodegenNodeFactory.cs
src/coreclr/tests/src/JIT/Directed/rvastatics/RVAOrderingTest.il

index ee492d0..81ed1ec 100644 (file)
@@ -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
     {
         /// <summary>
+        /// Returns the RVA associated with an RVA mapped field from the PE module.
+        /// </summary>
+        public static int GetFieldRvaValue(this EcmaField field)
+        {
+            Debug.Assert(field.HasRva);
+            return field.MetadataReader.GetFieldDefinition(field.Handle).GetRelativeVirtualAddress();
+        }
+
+        /// <summary>
         /// Retrieves the data associated with an RVA mapped field from the PE module.
         /// </summary>
         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;
         }
index 3aec94b..cd4b3e7 100644 (file)
@@ -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<byte>();
 
-            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<byte> 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;
         }
     }
 }
index 33a39ba..0eb39b4 100644 (file)
@@ -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<FieldRvaKey>
+        {
+            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, CopiedFieldRvaNode>(ecmaField =>
+            _copiedFieldRvas = new NodeCache<FieldRvaKey, CopiedFieldRvaNode>(key =>
             {
-                return new CopiedFieldRvaNode(ecmaField);
+                return new CopiedFieldRvaNode(key.Module, key.Rva);
             });
 
             _copiedStrongNameSignatures = new NodeCache<EcmaModule, CopiedStrongNameSignatureNode>(module =>
@@ -843,7 +863,7 @@ namespace ILCompiler.DependencyAnalysis
             return _copiedMethodIL.GetOrAdd(method);
         }
 
-        private NodeCache<EcmaField, CopiedFieldRvaNode> _copiedFieldRvas;
+        private NodeCache<FieldRvaKey, CopiedFieldRvaNode> _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<EcmaModule, CopiedStrongNameSignatureNode> _copiedStrongNameSignatures;
index 3f8faf7..9916451 100644 (file)
@@ -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
   } // 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