From a057ec8df813effd8ddd1af54ee480bd76dce847 Mon Sep 17 00:00:00 2001 From: Simon Nattress Date: Mon, 13 Apr 2020 16:26:56 -0700 Subject: [PATCH 1/5] Emit type layout check fixups When a ready-to-run compiled method uses value types from other version bubbles, emit a precode fixup to check at runtime that the type is still compatible (same size, alignment, GC layout). Bring over `GCPointerMap*.cs` from CoreRT repo to get the compute the GC layout bytes that match what the runtime expects in the layout blob. Simplify the behavior of the Crossgen test scripting. If tests opt out of crossgen by setting `false`, skip emitting the crossgen commands into the test run script. Also, stop setting the `RunCrossGen` and `RunCrossGen2` environment variables in the test script based on compile-time settings. These get set by runtest.py anyway. This set of tweaks allow for crossgen tests which we don't want to run the default crossgen commands on, and allow choosing either crossgen or crossgen2 as part of test execution. --- .../tools/Common/JitInterface/CorInfoImpl.cs | 17 ++ .../Utilities/GCPointerMap.Algorithm.cs | 124 ++++++++ .../Common/Utilities/GCPointerMap.cs | 272 ++++++++++++++++++ .../ReadyToRun/TypeFixupSignature.cs | 86 ++++++ .../ReadyToRunSymbolNodeFactory.cs | 15 + .../ILCompiler.TypeSystem.ReadyToRun.csproj | 6 + .../tests/src/CLRTest.CrossGen.targets | 18 +- .../tests/src/readytorun/tests/mainv1.csproj | 36 ++- .../tests/src/readytorun/tests/mainv2.csproj | 36 ++- 9 files changed, 587 insertions(+), 23 deletions(-) create mode 100644 src/coreclr/src/tools/Common/TypeSystem/Common/Utilities/GCPointerMap.Algorithm.cs create mode 100644 src/coreclr/src/tools/Common/TypeSystem/Common/Utilities/GCPointerMap.cs diff --git a/src/coreclr/src/tools/Common/JitInterface/CorInfoImpl.cs b/src/coreclr/src/tools/Common/JitInterface/CorInfoImpl.cs index a14006de297c87..a54efb0f77621a 100644 --- a/src/coreclr/src/tools/Common/JitInterface/CorInfoImpl.cs +++ b/src/coreclr/src/tools/Common/JitInterface/CorInfoImpl.cs @@ -1396,10 +1396,27 @@ private uint getClassSize(CORINFO_CLASS_STRUCT_* cls) { throw new RequiresRuntimeJitException(type); } + + if (NeedsTypeLayoutCheck(type)) + { + ISymbolNode node = _compilation.SymbolNodeFactory.CheckTypeLayout(type); + ((MethodWithGCInfo)_methodCodeNode).Fixups.Add(node); + } #endif return (uint)classSize.AsInt; } + bool NeedsTypeLayoutCheck(TypeDesc type) + { + if (!type.IsDefType) + return false; + + if (!type.IsValueType) + return false; + + return !IsLayoutFixedInCurrentVersionBubble(type); + } + private uint getHeapClassSize(CORINFO_CLASS_STRUCT_* cls) { TypeDesc type = HandleToObject(cls); diff --git a/src/coreclr/src/tools/Common/TypeSystem/Common/Utilities/GCPointerMap.Algorithm.cs b/src/coreclr/src/tools/Common/TypeSystem/Common/Utilities/GCPointerMap.Algorithm.cs new file mode 100644 index 00000000000000..8f9a1cd346e509 --- /dev/null +++ b/src/coreclr/src/tools/Common/TypeSystem/Common/Utilities/GCPointerMap.Algorithm.cs @@ -0,0 +1,124 @@ +// Licensed to the .NET Foundation under one or more agreements. +// The .NET Foundation licenses this file to you under the MIT license. +// See the LICENSE file in the project root for more information. + +using Debug = System.Diagnostics.Debug; + +namespace Internal.TypeSystem +{ + partial struct GCPointerMap + { + /// + /// Computes the GC pointer map for the instance fields of . + /// + public static GCPointerMap FromInstanceLayout(DefType type) + { + Debug.Assert(type.ContainsGCPointers); + + GCPointerMapBuilder builder = new GCPointerMapBuilder(type.InstanceByteCount.AsInt, type.Context.Target.PointerSize); + FromInstanceLayoutHelper(ref builder, type); + + return builder.ToGCMap(); + } + + /// + /// Computes the GC pointer map of the GC static region of the type. + /// + public static GCPointerMap FromStaticLayout(DefType type) + { + GCPointerMapBuilder builder = new GCPointerMapBuilder(type.GCStaticFieldSize.AsInt, type.Context.Target.PointerSize); + + foreach (FieldDesc field in type.GetFields()) + { + if (!field.IsStatic || field.HasRva || field.IsLiteral + || field.IsThreadStatic || !field.HasGCStaticBase) + continue; + + TypeDesc fieldType = field.FieldType; + if (fieldType.IsGCPointer) + { + builder.MarkGCPointer(field.Offset.AsInt); + } + else + { + Debug.Assert(fieldType.IsValueType); + var fieldDefType = (DefType)fieldType; + if (fieldDefType.ContainsGCPointers) + { + GCPointerMapBuilder innerBuilder = + builder.GetInnerBuilder(field.Offset.AsInt, fieldDefType.InstanceByteCount.AsInt); + FromInstanceLayoutHelper(ref innerBuilder, fieldDefType); + } + } + } + + Debug.Assert(builder.ToGCMap().Size * type.Context.Target.PointerSize >= type.GCStaticFieldSize.AsInt); + return builder.ToGCMap(); + } + + /// + /// Computes the GC pointer map of the thread static region of the type. + /// + public static GCPointerMap FromThreadStaticLayout(DefType type) + { + GCPointerMapBuilder builder = new GCPointerMapBuilder(type.ThreadGcStaticFieldSize.AsInt, type.Context.Target.PointerSize); + + foreach (FieldDesc field in type.GetFields()) + { + if (!field.IsStatic || field.HasRva || field.IsLiteral || !field.IsThreadStatic || !field.HasGCStaticBase) + continue; + + TypeDesc fieldType = field.FieldType; + if (fieldType.IsGCPointer) + { + builder.MarkGCPointer(field.Offset.AsInt); + } + else if (fieldType.IsValueType) + { + var fieldDefType = (DefType)fieldType; + if (fieldDefType.ContainsGCPointers) + { + GCPointerMapBuilder innerBuilder = + builder.GetInnerBuilder(field.Offset.AsInt, fieldDefType.InstanceByteCount.AsInt); + FromInstanceLayoutHelper(ref innerBuilder, fieldDefType); + } + } + } + + Debug.Assert(builder.ToGCMap().Size * type.Context.Target.PointerSize >= type.ThreadGcStaticFieldSize.AsInt); + return builder.ToGCMap(); + } + + private static void FromInstanceLayoutHelper(ref GCPointerMapBuilder builder, DefType type) + { + if (!type.IsValueType && type.HasBaseType) + { + DefType baseType = type.BaseType; + GCPointerMapBuilder baseLayoutBuilder = builder.GetInnerBuilder(0, baseType.InstanceByteCount.AsInt); + FromInstanceLayoutHelper(ref baseLayoutBuilder, baseType); + } + + foreach (FieldDesc field in type.GetFields()) + { + if (field.IsStatic) + continue; + + TypeDesc fieldType = field.FieldType; + if (fieldType.IsGCPointer) + { + builder.MarkGCPointer(field.Offset.AsInt); + } + else if (fieldType.IsValueType) + { + var fieldDefType = (DefType)fieldType; + if (fieldDefType.ContainsGCPointers) + { + GCPointerMapBuilder innerBuilder = + builder.GetInnerBuilder(field.Offset.AsInt, fieldDefType.InstanceByteCount.AsInt); + FromInstanceLayoutHelper(ref innerBuilder, fieldDefType); + } + } + } + } + } +} diff --git a/src/coreclr/src/tools/Common/TypeSystem/Common/Utilities/GCPointerMap.cs b/src/coreclr/src/tools/Common/TypeSystem/Common/Utilities/GCPointerMap.cs new file mode 100644 index 00000000000000..30a4aab3e89a50 --- /dev/null +++ b/src/coreclr/src/tools/Common/TypeSystem/Common/Utilities/GCPointerMap.cs @@ -0,0 +1,272 @@ +// Licensed to the .NET Foundation under one or more agreements. +// The .NET Foundation licenses this file to you under the MIT license. +// See the LICENSE file in the project root for more information. + +using System; +using System.Text; + +using Debug = System.Diagnostics.Debug; + +namespace Internal.TypeSystem +{ + /// + /// Represents a bitmap of GC pointers within a memory region divided into + /// pointer-sized cells. + /// + public partial struct GCPointerMap : IEquatable, IComparable + { + // Each bit in this array represents a pointer-sized cell. + private int[] _gcFlags; + + private int _numCells; + + /// + /// Gets a value indicating whether this map is initialized. + /// + public bool IsInitialized + { + get + { + return _gcFlags != null; + } + } + + /// + /// Gets the size (in cells) of the pointer map. + /// + public int Size + { + get + { + return _numCells; + } + } + + /// + /// Gets the number of continuous runs of GC pointers within the map. + /// + public int NumSeries + { + get + { + int numSeries = 0; + for (int i = 0; i < _numCells; i++) + { + if (this[i]) + { + numSeries++; + while (++i < _numCells && this[i]) ; + } + } + return numSeries; + } + } + + /// + /// Returns true if the map is all pointers + /// + public bool IsAllGCPointers + { + get + { + for (int i = 0; i < _numCells; i++) + { + if (!this[i]) + return false; + } + return true; + } + } + + public bool this[int index] + { + get + { + return (_gcFlags[index >> 5] & (1 << (index & 0x1F))) != 0; + } + } + + public GCPointerMap(int[] gcFlags, int numCells) + { + Debug.Assert(numCells <= gcFlags.Length << 5); + _gcFlags = gcFlags; + _numCells = numCells; + } + + public BitEnumerator GetEnumerator() + { + return new BitEnumerator(_gcFlags, 0, _numCells); + } + + public override bool Equals(object obj) + { + return obj is GCPointerMap && Equals((GCPointerMap)obj); + } + + public bool Equals(GCPointerMap other) + { + if (_numCells != other._numCells) + return false; + + for (int i = 0; i < _gcFlags.Length; i++) + if (_gcFlags[i] != other._gcFlags[i]) + return false; + + return true; + } + + public override int GetHashCode() + { + int hashCode = 0; + for (int i = 0; i < _gcFlags.Length; i++) + hashCode ^= _gcFlags[i]; + return hashCode; + } + + public override string ToString() + { + var sb = new StringBuilder(_numCells); + foreach (var bit in this) + sb.Append(bit ? '1' : '0'); + return sb.ToString(); + } + + public int CompareTo(GCPointerMap other) + { + if (_numCells != other._numCells) + return _numCells - other._numCells; + + for (int i = 0; i < _gcFlags.Length; i++) + { + if (_gcFlags[i] != other._gcFlags[i]) + return _gcFlags[i] - other._gcFlags[i]; + } + + Debug.Assert(Equals(other)); + return 0; + } + } + + /// + /// Utility class to assist in building . + /// + public struct GCPointerMapBuilder + { + // Each bit in this array represents a pointer-sized cell. + // Bits start at the least significant bit. + private int[] _gcFlags; + + private int _pointerSize; + + // Both of these are in bytes. + private int _delta; + private int _limit; + + public GCPointerMapBuilder(int numBytes, int pointerSize) + { + // Align the size up. The size of the pointer map is used to infer the statics storage size that has + // to include space for non-GC statics smaller than pointer size. + int numPointerSizedCells = (numBytes + pointerSize - 1) / pointerSize; + + if (numPointerSizedCells > 0) + { + // Given the number of cells, how many Int32's do we need to represent them? + // (It's one bit per cell, but this time we need to round up.) + _gcFlags = new int[((numPointerSizedCells - 1) >> 5) + 1]; + } + else + { + // Not big enough to fit even a single pointer. + _gcFlags = Array.Empty(); + } + + _pointerSize = pointerSize; + + _delta = 0; + _limit = numBytes; + } + + public void MarkGCPointer(int offset) + { + Debug.Assert(offset >= 0); + + int absoluteOffset = _delta + offset; + + Debug.Assert(absoluteOffset % _pointerSize == 0); + Debug.Assert(absoluteOffset <= (_limit - _pointerSize)); + + int cellIndex = absoluteOffset / _pointerSize; + + _gcFlags[cellIndex >> 5] |= 1 << (cellIndex & 0x1F); + } + + public GCPointerMapBuilder GetInnerBuilder(int offset, int size) + { + Debug.Assert(offset >= 0); + + int absoluteOffset = _delta + offset; + + Debug.Assert(absoluteOffset + size <= _limit); + + return new GCPointerMapBuilder + { + _gcFlags = this._gcFlags, + _pointerSize = this._pointerSize, + _delta = absoluteOffset, + _limit = absoluteOffset + size + }; + } + + public GCPointerMap ToGCMap() + { + Debug.Assert(_delta == 0); + return new GCPointerMap(_gcFlags, (_limit + _pointerSize - 1) / _pointerSize); + } + + public BitEnumerator GetEnumerator() + { + int numCells = (_limit - _delta) / _pointerSize; + int startCell = _delta / _pointerSize; + return new BitEnumerator(_gcFlags, startCell, numCells); + } + + public override string ToString() + { + var sb = new StringBuilder(); + foreach (var bit in this) + sb.Append(bit ? '1' : '0'); + return sb.ToString(); + } + } + + public struct BitEnumerator + { + private int[] _buffer; + private int _limitBit; + private int _currentBit; + + public BitEnumerator(int[] buffer, int startBit, int numBits) + { + Debug.Assert(startBit >= 0 && numBits >= 0); + Debug.Assert(startBit + numBits <= buffer.Length << 5); + + _buffer = buffer; + _currentBit = startBit - 1; + _limitBit = startBit + numBits; + } + + public bool Current + { + get + { + return (_buffer[_currentBit >> 5] & (1 << (_currentBit & 0x1F))) != 0; + } + } + + public bool MoveNext() + { + _currentBit++; + return _currentBit < _limitBit; + } + } +} diff --git a/src/coreclr/src/tools/crossgen2/ILCompiler.ReadyToRun/Compiler/DependencyAnalysis/ReadyToRun/TypeFixupSignature.cs b/src/coreclr/src/tools/crossgen2/ILCompiler.ReadyToRun/Compiler/DependencyAnalysis/ReadyToRun/TypeFixupSignature.cs index 3ae821d2dbf77d..a93d9034533c8b 100644 --- a/src/coreclr/src/tools/crossgen2/ILCompiler.ReadyToRun/Compiler/DependencyAnalysis/ReadyToRun/TypeFixupSignature.cs +++ b/src/coreclr/src/tools/crossgen2/ILCompiler.ReadyToRun/Compiler/DependencyAnalysis/ReadyToRun/TypeFixupSignature.cs @@ -3,10 +3,12 @@ // See the LICENSE file in the project root for more information. using System; +using System.Diagnostics; using Internal.Text; using Internal.TypeSystem; using Internal.TypeSystem.Ecma; +using Internal.TypeSystem.Interop; using Internal.ReadyToRunConstants; namespace ILCompiler.DependencyAnalysis.ReadyToRun @@ -39,11 +41,95 @@ public override ObjectData GetData(NodeFactory factory, bool relocsOnly = false) EcmaModule targetModule = factory.SignatureContext.GetTargetModule(_typeDesc); SignatureContext innerContext = dataBuilder.EmitFixup(factory, _fixupKind, targetModule, factory.SignatureContext); dataBuilder.EmitTypeSignature(_typeDesc, innerContext); + + if (_fixupKind == ReadyToRunFixupKind.Check_TypeLayout) + { + EncodeTypeLayout(dataBuilder, _typeDesc); + } } return dataBuilder.ToObjectData(); } + private static void EncodeTypeLayout(ObjectDataSignatureBuilder dataBuilder, TypeDesc type) + { + Debug.Assert(type.IsValueType); + MetadataType defType = (MetadataType)type; + + int pointerSize = type.Context.Target.PointerSize; + int size = defType.InstanceFieldSize.AsInt; + int alignment = GetClassAlignmentRequirement(defType); + ReadyToRunTypeLayoutFlags flags = ReadyToRunTypeLayoutFlags.READYTORUN_LAYOUT_Alignment | ReadyToRunTypeLayoutFlags.READYTORUN_LAYOUT_GCLayout; + if (alignment == pointerSize) + { + flags |= ReadyToRunTypeLayoutFlags.READYTORUN_LAYOUT_Alignment_Native; + } + + if (!defType.ContainsGCPointers) + { + flags |= ReadyToRunTypeLayoutFlags.READYTORUN_LAYOUT_GCLayout_Empty; + } + + dataBuilder.EmitUInt((uint)flags); + dataBuilder.EmitUInt((uint)size); + + if (alignment != pointerSize) + { + dataBuilder.EmitUInt((uint)alignment); + } + + if (defType.ContainsGCPointers) + { + // Encode the GC pointer map + GCPointerMap gcMap = GCPointerMap.FromInstanceLayout(defType); + + byte[] encodedGCRefMap = new byte[(size / pointerSize + 7) / 8]; + int bitIndex = 0; + foreach (bool bit in gcMap) + { + if (bit) + { + encodedGCRefMap[bitIndex / 8] += (byte)(1 << (bitIndex & 7)); + } + + ++bitIndex; + } + + dataBuilder.EmitBytes(encodedGCRefMap); + } + } + + /// + /// Managed implementation of CEEInfo::getClassAlignmentRequirementStatic + /// + private static int GetClassAlignmentRequirement(MetadataType type) + { + int alignment = type.Context.Target.PointerSize; + + if (((MetadataType)type).HasLayout()) + { + if (type.IsSequentialLayout || MarshalUtils.IsBlittableType(type)) + { + alignment = type.InstanceFieldAlignment.AsInt; + } + } + + if (type.Context.Target.Architecture == TargetArchitecture.ARM && + alignment < 8 && type.RequiresAlign8()) + { + // If the structure contains 64-bit primitive fields and the platform requires 8-byte alignment for + // such fields then make sure we return at least 8-byte alignment. Note that it's technically possible + // to create unmanaged APIs that take unaligned structures containing such fields and this + // unconditional alignment bump would cause us to get the calling convention wrong on platforms such + // as ARM. If we see such cases in the future we'd need to add another control (such as an alignment + // property for the StructLayout attribute or a marshaling directive attribute for p/invoke arguments) + // that allows more precise control. For now we'll go with the likely scenario. + alignment = 8; + } + + return alignment; + } + public override void AppendMangledName(NameMangler nameMangler, Utf8StringBuilder sb) { sb.Append(nameMangler.CompilationUnitPrefix); diff --git a/src/coreclr/src/tools/crossgen2/ILCompiler.ReadyToRun/Compiler/DependencyAnalysis/ReadyToRunSymbolNodeFactory.cs b/src/coreclr/src/tools/crossgen2/ILCompiler.ReadyToRun/Compiler/DependencyAnalysis/ReadyToRunSymbolNodeFactory.cs index 1130220fbc94db..da85b633322882 100644 --- a/src/coreclr/src/tools/crossgen2/ILCompiler.ReadyToRun/Compiler/DependencyAnalysis/ReadyToRunSymbolNodeFactory.cs +++ b/src/coreclr/src/tools/crossgen2/ILCompiler.ReadyToRun/Compiler/DependencyAnalysis/ReadyToRunSymbolNodeFactory.cs @@ -125,6 +125,14 @@ private void CreateNodeCaches() new DelegateCtorSignature(ctorKey.Type, targetMethodNode, ctorKey.Method.Token)); }); + _checkTypeLayoutCache = new NodeCache(key => + { + return new PrecodeHelperImport( + _codegenNodeFactory, + _codegenNodeFactory.TypeSignature(ReadyToRunFixupKind.Check_TypeLayout, key) + ); + }); + _genericLookupHelpers = new NodeCache(key => { return new DelayLoadHelperImport( @@ -434,6 +442,13 @@ public ISymbolNode DelegateCtor(TypeDesc delegateType, MethodWithToken method) return _delegateCtors.GetOrAdd(ctorKey); } + private NodeCache _checkTypeLayoutCache; + + public ISymbolNode CheckTypeLayout(TypeDesc type) + { + return _checkTypeLayoutCache.GetOrAdd(type); + } + struct MethodAndCallSite : IEquatable { public readonly MethodWithToken Method; diff --git a/src/coreclr/src/tools/crossgen2/ILCompiler.TypeSystem.ReadyToRun/ILCompiler.TypeSystem.ReadyToRun.csproj b/src/coreclr/src/tools/crossgen2/ILCompiler.TypeSystem.ReadyToRun/ILCompiler.TypeSystem.ReadyToRun.csproj index 1b4b6f9cd9348d..6d1d7e6ed4a3cb 100644 --- a/src/coreclr/src/tools/crossgen2/ILCompiler.TypeSystem.ReadyToRun/ILCompiler.TypeSystem.ReadyToRun.csproj +++ b/src/coreclr/src/tools/crossgen2/ILCompiler.TypeSystem.ReadyToRun/ILCompiler.TypeSystem.ReadyToRun.csproj @@ -150,6 +150,12 @@ Utilities\CustomAttributeTypeNameParser.cs + + Utilities\GCPointerMap.Algorithm.cs + + + Utilities\GCPointerMap.cs + Utilities\DebugNameFormatter.cs diff --git a/src/coreclr/tests/src/CLRTest.CrossGen.targets b/src/coreclr/tests/src/CLRTest.CrossGen.targets index 76cb30a76e39b3..00c9990b19d278 100644 --- a/src/coreclr/tests/src/CLRTest.CrossGen.targets +++ b/src/coreclr/tests/src/CLRTest.CrossGen.targets @@ -22,22 +22,18 @@ WARNING: When setting properties based on their current state (for example: $(BashScriptSnippetGen);GetCrossgenBashScript $(BatchScriptSnippetGen);GetCrossgenBatchScript - - - - - - - - - + - + - + Date: Tue, 14 Apr 2020 15:10:25 -0700 Subject: [PATCH 2/5] Cleanup from Michal's feedback --- .../tools/Common/JitInterface/CorInfoImpl.cs | 11 --- .../Utilities/GCPointerMap.Algorithm.cs | 68 ------------------- .../JitInterface/CorInfoImpl.ReadyToRun.cs | 11 +++ 3 files changed, 11 insertions(+), 79 deletions(-) diff --git a/src/coreclr/src/tools/Common/JitInterface/CorInfoImpl.cs b/src/coreclr/src/tools/Common/JitInterface/CorInfoImpl.cs index a54efb0f77621a..ceacd232b456bd 100644 --- a/src/coreclr/src/tools/Common/JitInterface/CorInfoImpl.cs +++ b/src/coreclr/src/tools/Common/JitInterface/CorInfoImpl.cs @@ -1406,17 +1406,6 @@ private uint getClassSize(CORINFO_CLASS_STRUCT_* cls) return (uint)classSize.AsInt; } - bool NeedsTypeLayoutCheck(TypeDesc type) - { - if (!type.IsDefType) - return false; - - if (!type.IsValueType) - return false; - - return !IsLayoutFixedInCurrentVersionBubble(type); - } - private uint getHeapClassSize(CORINFO_CLASS_STRUCT_* cls) { TypeDesc type = HandleToObject(cls); diff --git a/src/coreclr/src/tools/Common/TypeSystem/Common/Utilities/GCPointerMap.Algorithm.cs b/src/coreclr/src/tools/Common/TypeSystem/Common/Utilities/GCPointerMap.Algorithm.cs index 8f9a1cd346e509..4376f9e25d9700 100644 --- a/src/coreclr/src/tools/Common/TypeSystem/Common/Utilities/GCPointerMap.Algorithm.cs +++ b/src/coreclr/src/tools/Common/TypeSystem/Common/Utilities/GCPointerMap.Algorithm.cs @@ -21,74 +21,6 @@ public static GCPointerMap FromInstanceLayout(DefType type) return builder.ToGCMap(); } - /// - /// Computes the GC pointer map of the GC static region of the type. - /// - public static GCPointerMap FromStaticLayout(DefType type) - { - GCPointerMapBuilder builder = new GCPointerMapBuilder(type.GCStaticFieldSize.AsInt, type.Context.Target.PointerSize); - - foreach (FieldDesc field in type.GetFields()) - { - if (!field.IsStatic || field.HasRva || field.IsLiteral - || field.IsThreadStatic || !field.HasGCStaticBase) - continue; - - TypeDesc fieldType = field.FieldType; - if (fieldType.IsGCPointer) - { - builder.MarkGCPointer(field.Offset.AsInt); - } - else - { - Debug.Assert(fieldType.IsValueType); - var fieldDefType = (DefType)fieldType; - if (fieldDefType.ContainsGCPointers) - { - GCPointerMapBuilder innerBuilder = - builder.GetInnerBuilder(field.Offset.AsInt, fieldDefType.InstanceByteCount.AsInt); - FromInstanceLayoutHelper(ref innerBuilder, fieldDefType); - } - } - } - - Debug.Assert(builder.ToGCMap().Size * type.Context.Target.PointerSize >= type.GCStaticFieldSize.AsInt); - return builder.ToGCMap(); - } - - /// - /// Computes the GC pointer map of the thread static region of the type. - /// - public static GCPointerMap FromThreadStaticLayout(DefType type) - { - GCPointerMapBuilder builder = new GCPointerMapBuilder(type.ThreadGcStaticFieldSize.AsInt, type.Context.Target.PointerSize); - - foreach (FieldDesc field in type.GetFields()) - { - if (!field.IsStatic || field.HasRva || field.IsLiteral || !field.IsThreadStatic || !field.HasGCStaticBase) - continue; - - TypeDesc fieldType = field.FieldType; - if (fieldType.IsGCPointer) - { - builder.MarkGCPointer(field.Offset.AsInt); - } - else if (fieldType.IsValueType) - { - var fieldDefType = (DefType)fieldType; - if (fieldDefType.ContainsGCPointers) - { - GCPointerMapBuilder innerBuilder = - builder.GetInnerBuilder(field.Offset.AsInt, fieldDefType.InstanceByteCount.AsInt); - FromInstanceLayoutHelper(ref innerBuilder, fieldDefType); - } - } - } - - Debug.Assert(builder.ToGCMap().Size * type.Context.Target.PointerSize >= type.ThreadGcStaticFieldSize.AsInt); - return builder.ToGCMap(); - } - private static void FromInstanceLayoutHelper(ref GCPointerMapBuilder builder, DefType type) { if (!type.IsValueType && type.HasBaseType) diff --git a/src/coreclr/src/tools/crossgen2/ILCompiler.ReadyToRun/JitInterface/CorInfoImpl.ReadyToRun.cs b/src/coreclr/src/tools/crossgen2/ILCompiler.ReadyToRun/JitInterface/CorInfoImpl.ReadyToRun.cs index befd8c9946c238..433b69a0d56156 100644 --- a/src/coreclr/src/tools/crossgen2/ILCompiler.ReadyToRun/JitInterface/CorInfoImpl.ReadyToRun.cs +++ b/src/coreclr/src/tools/crossgen2/ILCompiler.ReadyToRun/JitInterface/CorInfoImpl.ReadyToRun.cs @@ -1938,6 +1938,17 @@ private bool IsLayoutFixedInCurrentVersionBubble(TypeDesc type) return true; } + private bool NeedsTypeLayoutCheck(TypeDesc type) + { + if (!type.IsDefType) + return false; + + if (!type.IsValueType) + return false; + + return !IsLayoutFixedInCurrentVersionBubble(type); + } + /// /// Is field layout of the inheritance chain fixed within the current version bubble? /// From 2d3e4b6e4a311d6bc309470f48313f9b983f8d7e Mon Sep 17 00:00:00 2001 From: Simon Nattress Date: Thu, 16 Apr 2020 13:05:12 -0700 Subject: [PATCH 3/5] Add Hfa flags and type to layout --- .../ReadyToRun/TypeFixupSignature.cs | 20 +++++++++++++++++++ 1 file changed, 20 insertions(+) diff --git a/src/coreclr/src/tools/crossgen2/ILCompiler.ReadyToRun/Compiler/DependencyAnalysis/ReadyToRun/TypeFixupSignature.cs b/src/coreclr/src/tools/crossgen2/ILCompiler.ReadyToRun/Compiler/DependencyAnalysis/ReadyToRun/TypeFixupSignature.cs index a93d9034533c8b..0bb2c227b738eb 100644 --- a/src/coreclr/src/tools/crossgen2/ILCompiler.ReadyToRun/Compiler/DependencyAnalysis/ReadyToRun/TypeFixupSignature.cs +++ b/src/coreclr/src/tools/crossgen2/ILCompiler.ReadyToRun/Compiler/DependencyAnalysis/ReadyToRun/TypeFixupSignature.cs @@ -10,6 +10,7 @@ using Internal.TypeSystem.Ecma; using Internal.TypeSystem.Interop; using Internal.ReadyToRunConstants; +using Internal.CorConstants; namespace ILCompiler.DependencyAnalysis.ReadyToRun { @@ -70,9 +71,28 @@ private static void EncodeTypeLayout(ObjectDataSignatureBuilder dataBuilder, Typ flags |= ReadyToRunTypeLayoutFlags.READYTORUN_LAYOUT_GCLayout_Empty; } + bool isHfa = defType.IsHfa && defType.Context.Target.Architecture == TargetArchitecture.ARM || defType.Context.Target.Architecture == TargetArchitecture.ARM64; + if (isHfa) + { + flags |= ReadyToRunTypeLayoutFlags.READYTORUN_LAYOUT_HFA; + } + dataBuilder.EmitUInt((uint)flags); dataBuilder.EmitUInt((uint)size); + if (isHfa) + { + switch (defType.HfaElementType.Category) + { + case TypeFlags.Single: + dataBuilder.EmitUInt((uint)CorElementType.ELEMENT_TYPE_R4); + break; + case TypeFlags.Double: + dataBuilder.EmitUInt((uint)CorElementType.ELEMENT_TYPE_R4); + break; + } + } + if (alignment != pointerSize) { dataBuilder.EmitUInt((uint)alignment); From 35bf8366efe8b115eae4dd272e0504d2666454b6 Mon Sep 17 00:00:00 2001 From: Simon Nattress Date: Fri, 17 Apr 2020 12:47:56 -0700 Subject: [PATCH 4/5] Code review feedback * Move architecture filtering for Hfas to `MetadataFieldLayoutAlgorithm` * Switch `GCPointerMap` to use `uint` for flags instead of `int` --- .../Common/MetadataFieldLayoutAlgorithm.cs | 3 +++ .../Common/Utilities/GCPointerMap.cs | 20 +++++++++---------- .../ReadyToRun/TypeFixupSignature.cs | 11 +++++----- 3 files changed, 18 insertions(+), 16 deletions(-) diff --git a/src/coreclr/src/tools/Common/TypeSystem/Common/MetadataFieldLayoutAlgorithm.cs b/src/coreclr/src/tools/Common/TypeSystem/Common/MetadataFieldLayoutAlgorithm.cs index 8dddf295adc465..6c211ede555014 100644 --- a/src/coreclr/src/tools/Common/TypeSystem/Common/MetadataFieldLayoutAlgorithm.cs +++ b/src/coreclr/src/tools/Common/TypeSystem/Common/MetadataFieldLayoutAlgorithm.cs @@ -843,6 +843,9 @@ private ValueTypeShapeCharacteristics ComputeHomogeneousFloatAggregateCharacteri MetadataType metadataType = (MetadataType)type; + if (type.Context.Target.Architecture != TargetArchitecture.ARM && type.Context.Target.Architecture != TargetArchitecture.ARM64) + return ValueTypeShapeCharacteristics.None; + // No HFAs with explicit layout. There may be cases where explicit layout may be still // eligible for HFA, but it is hard to tell the real intent. Make it simple and just // unconditionally disable HFAs for explicit layout. diff --git a/src/coreclr/src/tools/Common/TypeSystem/Common/Utilities/GCPointerMap.cs b/src/coreclr/src/tools/Common/TypeSystem/Common/Utilities/GCPointerMap.cs index 30a4aab3e89a50..482e502894d179 100644 --- a/src/coreclr/src/tools/Common/TypeSystem/Common/Utilities/GCPointerMap.cs +++ b/src/coreclr/src/tools/Common/TypeSystem/Common/Utilities/GCPointerMap.cs @@ -16,7 +16,7 @@ namespace Internal.TypeSystem public partial struct GCPointerMap : IEquatable, IComparable { // Each bit in this array represents a pointer-sized cell. - private int[] _gcFlags; + private uint[] _gcFlags; private int _numCells; @@ -86,7 +86,7 @@ public bool this[int index] } } - public GCPointerMap(int[] gcFlags, int numCells) + public GCPointerMap(uint[] gcFlags, int numCells) { Debug.Assert(numCells <= gcFlags.Length << 5); _gcFlags = gcFlags; @@ -119,7 +119,7 @@ public override int GetHashCode() { int hashCode = 0; for (int i = 0; i < _gcFlags.Length; i++) - hashCode ^= _gcFlags[i]; + hashCode ^= (int)_gcFlags[i]; return hashCode; } @@ -139,7 +139,7 @@ public int CompareTo(GCPointerMap other) for (int i = 0; i < _gcFlags.Length; i++) { if (_gcFlags[i] != other._gcFlags[i]) - return _gcFlags[i] - other._gcFlags[i]; + return (int)(_gcFlags[i] - other._gcFlags[i]); } Debug.Assert(Equals(other)); @@ -154,7 +154,7 @@ public struct GCPointerMapBuilder { // Each bit in this array represents a pointer-sized cell. // Bits start at the least significant bit. - private int[] _gcFlags; + private uint[] _gcFlags; private int _pointerSize; @@ -172,12 +172,12 @@ public GCPointerMapBuilder(int numBytes, int pointerSize) { // Given the number of cells, how many Int32's do we need to represent them? // (It's one bit per cell, but this time we need to round up.) - _gcFlags = new int[((numPointerSizedCells - 1) >> 5) + 1]; + _gcFlags = new uint[((numPointerSizedCells - 1) >> 5) + 1]; } else { // Not big enough to fit even a single pointer. - _gcFlags = Array.Empty(); + _gcFlags = Array.Empty(); } _pointerSize = pointerSize; @@ -197,7 +197,7 @@ public void MarkGCPointer(int offset) int cellIndex = absoluteOffset / _pointerSize; - _gcFlags[cellIndex >> 5] |= 1 << (cellIndex & 0x1F); + _gcFlags[cellIndex >> 5] |= 1u << (cellIndex & 0x1F); } public GCPointerMapBuilder GetInnerBuilder(int offset, int size) @@ -241,11 +241,11 @@ public override string ToString() public struct BitEnumerator { - private int[] _buffer; + private uint[] _buffer; private int _limitBit; private int _currentBit; - public BitEnumerator(int[] buffer, int startBit, int numBits) + public BitEnumerator(uint[] buffer, int startBit, int numBits) { Debug.Assert(startBit >= 0 && numBits >= 0); Debug.Assert(startBit + numBits <= buffer.Length << 5); diff --git a/src/coreclr/src/tools/crossgen2/ILCompiler.ReadyToRun/Compiler/DependencyAnalysis/ReadyToRun/TypeFixupSignature.cs b/src/coreclr/src/tools/crossgen2/ILCompiler.ReadyToRun/Compiler/DependencyAnalysis/ReadyToRun/TypeFixupSignature.cs index 0bb2c227b738eb..359d14126cbd5f 100644 --- a/src/coreclr/src/tools/crossgen2/ILCompiler.ReadyToRun/Compiler/DependencyAnalysis/ReadyToRun/TypeFixupSignature.cs +++ b/src/coreclr/src/tools/crossgen2/ILCompiler.ReadyToRun/Compiler/DependencyAnalysis/ReadyToRun/TypeFixupSignature.cs @@ -71,8 +71,7 @@ private static void EncodeTypeLayout(ObjectDataSignatureBuilder dataBuilder, Typ flags |= ReadyToRunTypeLayoutFlags.READYTORUN_LAYOUT_GCLayout_Empty; } - bool isHfa = defType.IsHfa && defType.Context.Target.Architecture == TargetArchitecture.ARM || defType.Context.Target.Architecture == TargetArchitecture.ARM64; - if (isHfa) + if (defType.IsHfa) { flags |= ReadyToRunTypeLayoutFlags.READYTORUN_LAYOUT_HFA; } @@ -80,7 +79,7 @@ private static void EncodeTypeLayout(ObjectDataSignatureBuilder dataBuilder, Typ dataBuilder.EmitUInt((uint)flags); dataBuilder.EmitUInt((uint)size); - if (isHfa) + if (defType.IsHfa) { switch (defType.HfaElementType.Category) { @@ -88,7 +87,7 @@ private static void EncodeTypeLayout(ObjectDataSignatureBuilder dataBuilder, Typ dataBuilder.EmitUInt((uint)CorElementType.ELEMENT_TYPE_R4); break; case TypeFlags.Double: - dataBuilder.EmitUInt((uint)CorElementType.ELEMENT_TYPE_R4); + dataBuilder.EmitUInt((uint)CorElementType.ELEMENT_TYPE_R8); break; } } @@ -109,7 +108,7 @@ private static void EncodeTypeLayout(ObjectDataSignatureBuilder dataBuilder, Typ { if (bit) { - encodedGCRefMap[bitIndex / 8] += (byte)(1 << (bitIndex & 7)); + encodedGCRefMap[bitIndex / 8] |= (byte)(1 << (bitIndex & 7)); } ++bitIndex; @@ -126,7 +125,7 @@ private static int GetClassAlignmentRequirement(MetadataType type) { int alignment = type.Context.Target.PointerSize; - if (((MetadataType)type).HasLayout()) + if (type.HasLayout()) { if (type.IsSequentialLayout || MarshalUtils.IsBlittableType(type)) { From 08eae552124dbc2aea2acabdc639f7475cea0f53 Mon Sep 17 00:00:00 2001 From: Simon Nattress Date: Mon, 20 Apr 2020 11:51:26 -0700 Subject: [PATCH 5/5] Remove unneeded cast --- src/coreclr/src/tools/Common/JitInterface/CorInfoImpl.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/coreclr/src/tools/Common/JitInterface/CorInfoImpl.cs b/src/coreclr/src/tools/Common/JitInterface/CorInfoImpl.cs index ceacd232b456bd..3901cd30a3b42d 100644 --- a/src/coreclr/src/tools/Common/JitInterface/CorInfoImpl.cs +++ b/src/coreclr/src/tools/Common/JitInterface/CorInfoImpl.cs @@ -1400,7 +1400,7 @@ private uint getClassSize(CORINFO_CLASS_STRUCT_* cls) if (NeedsTypeLayoutCheck(type)) { ISymbolNode node = _compilation.SymbolNodeFactory.CheckTypeLayout(type); - ((MethodWithGCInfo)_methodCodeNode).Fixups.Add(node); + _methodCodeNode.Fixups.Add(node); } #endif return (uint)classSize.AsInt;