From 0710555e6e5a866ea1b67609dfc83b38b3ecdb80 Mon Sep 17 00:00:00 2001 From: Tomas Rylek Date: Wed, 3 Jul 2019 10:28:29 +0200 Subject: [PATCH 1/5] Miscellaneous CPAOT fixes for several Pri# bug buckets 1) We were missing support for structs with explicit layout. I have added the appropriate logic. 2) Nullable is not blittable (however it is managed sequential if its instantiation type is managed sequential). 3) Structs with explicit layout aren't managed sequential. 4) We were putting the cctor trigger helper into the wrong import table - DispatchImports emits GC ref map and cctor trigger doesn't need one because it's not a method call helper. This was crashing R2RDump when trying to disassemble some R2R executables. 5) I have added two unit tests to the R2R unit test suite by adapting two CoreCLR tests that were crashing in interesting ways when manipulating explicit layouts and nullables. 6) Instantiated type signature encoding was incorrect in large version bubble case. As JanV described earlier, the context for encoding of the instantiation type parameters is the outer context, not the generic type context. 7) Always emit all import tables even when they are empty, otherwise the fixup encoding gets out of sync as it refers to import table indices. 8) When SuperIlc calls R2RDump in the large version bubble mode, it needs to pass CORE_ROOT as the "reference path" parameter so that R2RDump can resolve framework assembly references. 9) Field encoding was doing an unnecessarily strong check - replaced ContainsType with VersionsWithType. Thanks Tomas --- .../Common/MetadataFieldLayoutAlgorithm.cs | 2 +- .../src/TypeSystem/Interop/IL/MarshalUtils.cs | 6 +- .../ReadyToRun/ImportSectionNode.cs | 27 +++-- .../ReadyToRun/ImportSectionsTableNode.cs | 2 +- .../ReadyToRun/SignatureBuilder.cs | 3 +- .../ReadyToRunSymbolNodeFactory.cs | 2 +- .../ReadyToRunMetadataFieldLayoutAlgorithm.cs | 6 ++ src/JitInterface/src/CorInfoImpl.cs | 2 +- tests/src/Simple/ReadyToRunUnit/Program.cs | 100 ++++++++++++++++++ .../ReadyToRun.SuperIlc/CompilerRunner.cs | 6 ++ 10 files changed, 138 insertions(+), 18 deletions(-) diff --git a/src/Common/src/TypeSystem/Common/MetadataFieldLayoutAlgorithm.cs b/src/Common/src/TypeSystem/Common/MetadataFieldLayoutAlgorithm.cs index b1329685c0f..921812658ac 100644 --- a/src/Common/src/TypeSystem/Common/MetadataFieldLayoutAlgorithm.cs +++ b/src/Common/src/TypeSystem/Common/MetadataFieldLayoutAlgorithm.cs @@ -297,7 +297,7 @@ protected virtual void FinalizeRuntimeSpecificStaticFieldLayout(TypeSystemContex { } - private static ComputedInstanceFieldLayout ComputeExplicitFieldLayout(MetadataType type, int numInstanceFields) + protected static ComputedInstanceFieldLayout ComputeExplicitFieldLayout(MetadataType type, int numInstanceFields) { // Instance slice size is the total size of instance not including the base type. // It is calculated as the field whose offset and size add to the greatest value. diff --git a/src/Common/src/TypeSystem/Interop/IL/MarshalUtils.cs b/src/Common/src/TypeSystem/Interop/IL/MarshalUtils.cs index 5fff97bbcfb..bfd1e6a7cee 100644 --- a/src/Common/src/TypeSystem/Interop/IL/MarshalUtils.cs +++ b/src/Common/src/TypeSystem/Interop/IL/MarshalUtils.cs @@ -15,7 +15,7 @@ public static bool IsBlittableType(TypeDesc type) { type = type.UnderlyingType; - if (type.IsValueType) + if (type.IsValueType && !type.IsNullable) { if (type.IsPrimitive) { @@ -59,6 +59,10 @@ public static bool IsBlittableType(TypeDesc type) public static bool IsManagedSequentialType(TypeDesc type) { type = type.UnderlyingType; + if (type is MetadataType metadataType && metadataType.IsExplicitLayout) + { + return false; + } if (type.IsPrimitive || type.Category == TypeFlags.Pointer) { return true; diff --git a/src/ILCompiler.ReadyToRun/src/Compiler/DependencyAnalysis/ReadyToRun/ImportSectionNode.cs b/src/ILCompiler.ReadyToRun/src/Compiler/DependencyAnalysis/ReadyToRun/ImportSectionNode.cs index eeed8216518..6e54bc08597 100644 --- a/src/ILCompiler.ReadyToRun/src/Compiler/DependencyAnalysis/ReadyToRun/ImportSectionNode.cs +++ b/src/ILCompiler.ReadyToRun/src/Compiler/DependencyAnalysis/ReadyToRun/ImportSectionNode.cs @@ -9,7 +9,14 @@ namespace ILCompiler.DependencyAnalysis.ReadyToRun { public class ImportSectionNode : EmbeddedObjectNode { - private readonly ArrayOfEmbeddedDataNode _imports; + private class ImportTable : ArrayOfEmbeddedDataNode + { + public ImportTable(string startSymbol, string endSymbol) : base(startSymbol, endSymbol, nodeSorter: null) {} + + public override bool ShouldSkipEmittingObjectNode(NodeFactory factory) => false; + } + + private readonly ImportTable _imports; // TODO: annoying - today there's no way to put signature RVA's into R/O data section private readonly ArrayOfEmbeddedPointersNode _signatures; // TODO: annoying - cannot enumerate the ArrayOfEmbeddedPointersNode so we must keep a copy. @@ -34,7 +41,7 @@ public ImportSectionNode(string name, CorCompileImportType importType, CorCompil _emitPrecode = emitPrecode; _emitGCRefMap = emitGCRefMap; - _imports = new ArrayOfEmbeddedDataNode(_name + "_ImportBegin", _name + "_ImportEnd", null); + _imports = new ImportTable(_name + "_ImportBegin", _name + "_ImportEnd"); _signatures = new ArrayOfEmbeddedPointersNode(_name + "_SigBegin", _name + "_SigEnd", null); _signatureList = new List(); _gcRefMap = (_emitGCRefMap ? new GCRefMapNode(this) : null); @@ -78,20 +85,17 @@ public void AddImport(NodeFactory factory, Import import) public override int ClassCode => -62839441; - public bool ShouldSkipEmittingTable(NodeFactory factory) - { - return _imports.ShouldSkipEmittingObjectNode(factory); - } - public override void EncodeData(ref ObjectDataBuilder dataBuilder, NodeFactory factory, bool relocsOnly) { - if (!relocsOnly && _imports.ShouldSkipEmittingObjectNode(factory)) + if (!_imports.ShouldSkipEmittingObjectNode(factory)) + { + dataBuilder.EmitReloc(_imports.StartSymbol, RelocType.IMAGE_REL_BASED_ADDR32NB, 0); + } + else { - // Don't emit import section node at all if there are no entries in it - return; + dataBuilder.EmitUInt(0); } - dataBuilder.EmitReloc(_imports.StartSymbol, RelocType.IMAGE_REL_BASED_ADDR32NB, 0); if (!relocsOnly) { dataBuilder.EmitInt(_imports.GetData(factory, false).Data.Length); @@ -100,6 +104,7 @@ public override void EncodeData(ref ObjectDataBuilder dataBuilder, NodeFactory f dataBuilder.EmitByte((byte)_type); dataBuilder.EmitByte(_entrySize); } + if (!_signatures.ShouldSkipEmittingObjectNode(factory)) { dataBuilder.EmitReloc(_signatures.StartSymbol, RelocType.IMAGE_REL_BASED_ADDR32NB, 0); diff --git a/src/ILCompiler.ReadyToRun/src/Compiler/DependencyAnalysis/ReadyToRun/ImportSectionsTableNode.cs b/src/ILCompiler.ReadyToRun/src/Compiler/DependencyAnalysis/ReadyToRun/ImportSectionsTableNode.cs index 98a37ca62e1..beaafb48c5f 100644 --- a/src/ILCompiler.ReadyToRun/src/Compiler/DependencyAnalysis/ReadyToRun/ImportSectionsTableNode.cs +++ b/src/ILCompiler.ReadyToRun/src/Compiler/DependencyAnalysis/ReadyToRun/ImportSectionsTableNode.cs @@ -37,7 +37,7 @@ protected override void GetElementDataForNodes(ref ObjectDataBuilder builder, No int index = 0; foreach (ImportSectionNode node in NodesList) { - if (!relocsOnly && !node.ShouldSkipEmittingTable(factory)) + if (!relocsOnly) { node.InitializeOffsetFromBeginningOfArray(builder.CountBytes); node.InitializeIndexFromBeginningOfArray(index++); diff --git a/src/ILCompiler.ReadyToRun/src/Compiler/DependencyAnalysis/ReadyToRun/SignatureBuilder.cs b/src/ILCompiler.ReadyToRun/src/Compiler/DependencyAnalysis/ReadyToRun/SignatureBuilder.cs index e970d0a675d..ce623fc3855 100644 --- a/src/ILCompiler.ReadyToRun/src/Compiler/DependencyAnalysis/ReadyToRun/SignatureBuilder.cs +++ b/src/ILCompiler.ReadyToRun/src/Compiler/DependencyAnalysis/ReadyToRun/SignatureBuilder.cs @@ -341,9 +341,8 @@ private void EmitInstantiatedTypeSignature(InstantiatedType type, SignatureConte { EcmaModule targetModule = context.GetTargetModule(type); EmitModuleOverride(targetModule, context); - context = context.InnerContext(targetModule); EmitElementType(CorElementType.ELEMENT_TYPE_GENERICINST); - EmitTypeSignature(type.GetTypeDefinition(), context); + EmitTypeSignature(type.GetTypeDefinition(), context.InnerContext(targetModule)); EmitUInt((uint)type.Instantiation.Length); for (int paramIndex = 0; paramIndex < type.Instantiation.Length; paramIndex++) { diff --git a/src/ILCompiler.ReadyToRun/src/Compiler/DependencyAnalysis/ReadyToRunSymbolNodeFactory.cs b/src/ILCompiler.ReadyToRun/src/Compiler/DependencyAnalysis/ReadyToRunSymbolNodeFactory.cs index 44dcfa0658e..ec1cb73c885 100644 --- a/src/ILCompiler.ReadyToRun/src/Compiler/DependencyAnalysis/ReadyToRunSymbolNodeFactory.cs +++ b/src/ILCompiler.ReadyToRun/src/Compiler/DependencyAnalysis/ReadyToRunSymbolNodeFactory.cs @@ -253,7 +253,7 @@ private ISymbolNode CreateCctorTrigger(TypeDesc type, SignatureContext signature { return new DelayLoadHelperImport( _codegenNodeFactory, - _codegenNodeFactory.DispatchImports, + _codegenNodeFactory.HelperImports, ILCompiler.DependencyAnalysis.ReadyToRun.ReadyToRunHelper.READYTORUN_HELPER_DelayLoad_Helper, _codegenNodeFactory.TypeSignature(ReadyToRunFixupKind.READYTORUN_FIXUP_CctorTrigger, type, signatureContext)); } diff --git a/src/ILCompiler.ReadyToRun/src/Compiler/ReadyToRunMetadataFieldLayoutAlgorithm.cs b/src/ILCompiler.ReadyToRun/src/Compiler/ReadyToRunMetadataFieldLayoutAlgorithm.cs index ffaa0cfd675..facf27e2403 100644 --- a/src/ILCompiler.ReadyToRun/src/Compiler/ReadyToRunMetadataFieldLayoutAlgorithm.cs +++ b/src/ILCompiler.ReadyToRun/src/Compiler/ReadyToRunMetadataFieldLayoutAlgorithm.cs @@ -15,6 +15,7 @@ using Internal.TypeSystem; using Internal.TypeSystem.Ecma; using Internal.TypeSystem.Interop; +using ILCompiler.DependencyAnalysis; namespace ILCompiler { @@ -688,6 +689,11 @@ public void AddDynamicLayout(DefType instantiatedType, FieldAndOffset[] fieldMap protected override ComputedInstanceFieldLayout ComputeInstanceFieldLayout(MetadataType type, int numInstanceFields) { + if (type.IsExplicitLayout) + { + return ComputeExplicitFieldLayout(type, numInstanceFields); + } + else if (type.IsValueType && (MarshalUtils.IsBlittableType(type) || MarshalUtils.IsManagedSequentialType(type))) { return ComputeSequentialFieldLayout(type, numInstanceFields); diff --git a/src/JitInterface/src/CorInfoImpl.cs b/src/JitInterface/src/CorInfoImpl.cs index ec583e98255..2cf28c2f534 100644 --- a/src/JitInterface/src/CorInfoImpl.cs +++ b/src/JitInterface/src/CorInfoImpl.cs @@ -2177,7 +2177,7 @@ private void getFieldInfo(ref CORINFO_RESOLVED_TOKEN pResolvedToken, CORINFO_MET } #if READYTORUN - if (!_compilation.NodeFactory.CompilationModuleGroup.ContainsType(field.OwningType) && + if (!_compilation.NodeFactory.CompilationModuleGroup.VersionsWithType(field.OwningType) && fieldAccessor == CORINFO_FIELD_ACCESSOR.CORINFO_FIELD_STATIC_SHARED_STATIC_HELPER) { PreventRecursiveFieldInlinesOutsideVersionBubble(field, callerMethod); diff --git a/tests/src/Simple/ReadyToRunUnit/Program.cs b/tests/src/Simple/ReadyToRunUnit/Program.cs index 79859835101..19a73c1a252 100644 --- a/tests/src/Simple/ReadyToRunUnit/Program.cs +++ b/tests/src/Simple/ReadyToRunUnit/Program.cs @@ -8,6 +8,7 @@ using System.Linq.Expressions; using System.Numerics; using System.Reflection; +using System.Runtime.InteropServices; using System.Text; internal class ClassWithStatic @@ -136,7 +137,104 @@ private static bool BoxUnbox() } return success; } + + [StructLayout(LayoutKind.Explicit)] + private struct ExplicitFieldOffsetStruct + { + [FieldOffset(0)] + public int Field00; + [FieldOffset(0x0f)] + public int Field15; + } + + private static ExplicitFieldOffsetStruct HelperCreateExplicitLayoutStruct() + { + ExplicitFieldOffsetStruct epl = new ExplicitFieldOffsetStruct(); + epl.Field00 = 40; + epl.Field15 = 15; + return epl; + } + + private static bool HelperCompare(ExplicitFieldOffsetStruct val, ExplicitFieldOffsetStruct val1) + { + bool match = true; + if (val.Field00 != val1.Field00) + { + match = false; + Console.WriteLine("ExplicitLayout: val.Field00 = {0}, val1.Field00 = {1}", val.Field00, val1.Field00); + } + if (val.Field15 != val1.Field15) + { + match = false; + Console.WriteLine("ExplicitLayout: val.Field15 = {0}, val1.Field15 = {1}", val.Field15, val1.Field15); + } + return match; + } + + private static bool HelperCompare(ExplicitFieldOffsetStruct? val, ExplicitFieldOffsetStruct val1) + { + return val == null ? false : HelperCompare(val.Value, val1); + } + + private static bool BoxUnboxToQ2(ExplicitFieldOffsetStruct? val) + { + return HelperCompare(val, HelperCreateExplicitLayoutStruct()); + } + + private static bool BoxUnboxToQ1(ValueType vt) + { + return BoxUnboxToQ2((ExplicitFieldOffsetStruct?)vt); + } + + private static bool BoxUnboxToQ(object o) + { + return BoxUnboxToQ1((ValueType)o); + } + + private static bool NullableWithExplicitLayoutTest() + { + ExplicitFieldOffsetStruct? s = HelperCreateExplicitLayoutStruct(); + return BoxUnboxToQ(s); + } + + private static char HelperCreateChar() + { + return 'c'; + } + + private static bool HelperCompare(char val, char val1) + { + if (val == val1) + { + return true; + } + Console.Error.WriteLine("val = {0} = 0x{1:x2}, val1 = {2} = 0x{3:x2}", val, (int)val, val1, (int)val1); + return false; + } + + private static bool BoxUnboxToNQ2(char c) + { + return HelperCompare(c, HelperCreateChar()); + } + + private static bool BoxUnboxToNQ1(ValueType vt) + { + Console.WriteLine("BoxUnboxToNQ1: {0}", vt); + return BoxUnboxToNQ2((char)vt); + } + + private static bool BoxUnboxToNQ(object o) + { + Console.WriteLine("BoxUnboxToNQ: {0}", o); + return BoxUnboxToNQ1((ValueType)o); + } + private static bool CastClassWithCharTest() + { + char? s = HelperCreateChar(); + return BoxUnboxToNQ(s); + } + private static bool TypeHandle() { Console.WriteLine(TextFileName.GetType().ToString()); @@ -1063,6 +1161,8 @@ public static int Main(string[] args) RunTest("ChkCast", ChkCast()); RunTest("ChkCastValueType", ChkCastValueType()); RunTest("BoxUnbox", BoxUnbox()); + RunTest("NullableWithExplicitLayoutTest", NullableWithExplicitLayoutTest()); + RunTest("CastClassWithCharTest", CastClassWithCharTest()); RunTest("TypeHandle", TypeHandle()); RunTest("RuntimeTypeHandle", RuntimeTypeHandle()); RunTest("ReadAllText", ReadAllText()); diff --git a/tests/src/tools/ReadyToRun.SuperIlc/CompilerRunner.cs b/tests/src/tools/ReadyToRun.SuperIlc/CompilerRunner.cs index 1d69503901f..5adcf4231d2 100644 --- a/tests/src/tools/ReadyToRun.SuperIlc/CompilerRunner.cs +++ b/tests/src/tools/ReadyToRun.SuperIlc/CompilerRunner.cs @@ -94,6 +94,12 @@ public ProcessParameters CompilationR2RDumpProcess(string compiledExecutable, bo { commonBuilder.Append($@" --rp ""{referencePath}"""); } + + if (_options.CoreRootDirectory != null) + { + commonBuilder.Append($@" --rp ""{_options.CoreRootDirectory.FullName}"""); + } + commonBuilder.Append($@" --in ""{compiledExecutable}"""); StringBuilder builder = new StringBuilder(commonBuilder.ToString()); From c648a0f8c6e00135e36034c69d0fbf119a21807b Mon Sep 17 00:00:00 2001 From: Tomas Rylek Date: Sun, 7 Jul 2019 23:21:05 +0200 Subject: [PATCH 2/5] Address JanK's initial PR feedback --- .../src/TypeSystem/Interop/IL/MarshalUtils.cs | 15 ++++++++++++--- 1 file changed, 12 insertions(+), 3 deletions(-) diff --git a/src/Common/src/TypeSystem/Interop/IL/MarshalUtils.cs b/src/Common/src/TypeSystem/Interop/IL/MarshalUtils.cs index bfd1e6a7cee..a6b229d4e68 100644 --- a/src/Common/src/TypeSystem/Interop/IL/MarshalUtils.cs +++ b/src/Common/src/TypeSystem/Interop/IL/MarshalUtils.cs @@ -15,7 +15,7 @@ public static bool IsBlittableType(TypeDesc type) { type = type.UnderlyingType; - if (type.IsValueType && !type.IsNullable) + if (type.IsValueType) { if (type.IsPrimitive) { @@ -59,9 +59,18 @@ public static bool IsBlittableType(TypeDesc type) public static bool IsManagedSequentialType(TypeDesc type) { type = type.UnderlyingType; - if (type is MetadataType metadataType && metadataType.IsExplicitLayout) + if (type is MetadataType metadataType) { - return false; + if (!metadataType.IsSequentialLayout) + { + return false; + } + if (metadataType.HasBaseType && + !metadataType.BaseType.IsWellKnownType(WellKnownType.ValueType) && + !IsManagedSequentialType(metadataType.BaseType)) + { + return false; + } } if (type.IsPrimitive || type.Category == TypeFlags.Pointer) { From c08a0879283a9884a7f127d37b2e29c91ce7d547 Mon Sep 17 00:00:00 2001 From: Tomas Rylek Date: Tue, 9 Jul 2019 17:28:59 +0200 Subject: [PATCH 3/5] Revert all changes to MarshalUtils.cs I'm reverting this part of the change; I'm going to send it out for a new PR after performing the instrumented measurements as suggested by Jan Kotas in the PR discussion. Thanks Tomas --- .../src/TypeSystem/Interop/IL/MarshalUtils.cs | 13 ------------- 1 file changed, 13 deletions(-) diff --git a/src/Common/src/TypeSystem/Interop/IL/MarshalUtils.cs b/src/Common/src/TypeSystem/Interop/IL/MarshalUtils.cs index a6b229d4e68..5fff97bbcfb 100644 --- a/src/Common/src/TypeSystem/Interop/IL/MarshalUtils.cs +++ b/src/Common/src/TypeSystem/Interop/IL/MarshalUtils.cs @@ -59,19 +59,6 @@ public static bool IsBlittableType(TypeDesc type) public static bool IsManagedSequentialType(TypeDesc type) { type = type.UnderlyingType; - if (type is MetadataType metadataType) - { - if (!metadataType.IsSequentialLayout) - { - return false; - } - if (metadataType.HasBaseType && - !metadataType.BaseType.IsWellKnownType(WellKnownType.ValueType) && - !IsManagedSequentialType(metadataType.BaseType)) - { - return false; - } - } if (type.IsPrimitive || type.Category == TypeFlags.Pointer) { return true; From 86de9008093588dd0d4a5ebb73a3055cbefabd2b Mon Sep 17 00:00:00 2001 From: Tomas Rylek Date: Tue, 9 Jul 2019 20:02:16 +0200 Subject: [PATCH 4/5] Remove unneded using statements per Michal's PR feedback --- .../src/Compiler/ReadyToRunMetadataFieldLayoutAlgorithm.cs | 2 -- 1 file changed, 2 deletions(-) diff --git a/src/ILCompiler.ReadyToRun/src/Compiler/ReadyToRunMetadataFieldLayoutAlgorithm.cs b/src/ILCompiler.ReadyToRun/src/Compiler/ReadyToRunMetadataFieldLayoutAlgorithm.cs index facf27e2403..47d7a51ece7 100644 --- a/src/ILCompiler.ReadyToRun/src/Compiler/ReadyToRunMetadataFieldLayoutAlgorithm.cs +++ b/src/ILCompiler.ReadyToRun/src/Compiler/ReadyToRunMetadataFieldLayoutAlgorithm.cs @@ -5,7 +5,6 @@ using System; using System.Diagnostics; using System.Collections.Generic; -using System.Collections.Immutable; using System.Reflection; using System.Reflection.Metadata; using System.Reflection.Metadata.Ecma335; @@ -15,7 +14,6 @@ using Internal.TypeSystem; using Internal.TypeSystem.Ecma; using Internal.TypeSystem.Interop; -using ILCompiler.DependencyAnalysis; namespace ILCompiler { From 6e1d9a1ea6eaeeb62bb70ef78fec6a4d648118b2 Mon Sep 17 00:00:00 2001 From: Tomas Rylek Date: Tue, 9 Jul 2019 21:57:27 +0200 Subject: [PATCH 5/5] Temporarily disable the NullableWithExplicitLayout test This test requires changes to the IsManagedSequential method I have temporarily reverted from CPAOT because of extensive PR feedback suggesting additional analytic work. In light of this fact I'm temporarily disabling this method before the IsManagedSequential issue is completely resolved. Thanks Tomas --- tests/src/Simple/ReadyToRunUnit/Program.cs | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/tests/src/Simple/ReadyToRunUnit/Program.cs b/tests/src/Simple/ReadyToRunUnit/Program.cs index 19a73c1a252..ea62198f079 100644 --- a/tests/src/Simple/ReadyToRunUnit/Program.cs +++ b/tests/src/Simple/ReadyToRunUnit/Program.cs @@ -1161,7 +1161,9 @@ public static int Main(string[] args) RunTest("ChkCast", ChkCast()); RunTest("ChkCastValueType", ChkCastValueType()); RunTest("BoxUnbox", BoxUnbox()); - RunTest("NullableWithExplicitLayoutTest", NullableWithExplicitLayoutTest()); + // TODO: enabling this test requires fixes to IsManagedSequential I'm going to send out + // in a subsequent PR together with removal of this temporary clause [trylek] + // RunTest("NullableWithExplicitLayoutTest", NullableWithExplicitLayoutTest()); RunTest("CastClassWithCharTest", CastClassWithCharTest()); RunTest("TypeHandle", TypeHandle()); RunTest("RuntimeTypeHandle", RuntimeTypeHandle());