From ff0e821e4bd8da63902860fa3d42d3c6cbdfcbb4 Mon Sep 17 00:00:00 2001 From: David Wrighton Date: Thu, 13 Apr 2023 13:49:16 -0700 Subject: [PATCH 01/23] Skip type validation by default in ReadyToRun images - Technically, this is a breaking change, so I've provided a means for disabling the type validation skip - The model is that the C# compile won't get these details wrong, so disable the checks when run through crossgen2. The idea is that we'll get these things checked during normal, non-R2R usage of the app, and publish won't check these details. --- .../Internal/Runtime/ReadyToRunConstants.cs | 4 +- .../ReadyToRunCodegenNodeFactory.cs | 1 + .../Compiler/ReadyToRunCodegenCompilation.cs | 4 ++ .../ReadyToRunCodegenCompilationBuilder.cs | 4 ++ .../aot/crossgen2/Crossgen2RootCommand.cs | 3 + src/coreclr/tools/aot/crossgen2/Program.cs | 1 + .../aot/crossgen2/Properties/Resources.resx | 59 ++++++++++--------- 7 files changed, 46 insertions(+), 30 deletions(-) diff --git a/src/coreclr/tools/Common/Internal/Runtime/ReadyToRunConstants.cs b/src/coreclr/tools/Common/Internal/Runtime/ReadyToRunConstants.cs index 36e50e01857368..4383c99b25ca66 100644 --- a/src/coreclr/tools/Common/Internal/Runtime/ReadyToRunConstants.cs +++ b/src/coreclr/tools/Common/Internal/Runtime/ReadyToRunConstants.cs @@ -12,8 +12,8 @@ namespace Internal.ReadyToRunConstants public enum ReadyToRunFlags { READYTORUN_FLAG_PlatformNeutralSource = 0x00000001, // Set if the original IL assembly was platform-neutral - READYTORUN_FLAG_SkipTypeValidation = 0x00000002, // Set of methods with native code was determined using profile data - READYTORUN_FLAG_Partial = 0x00000004, + READYTORUN_FLAG_SkipTypeValidation = 0x00000002, // Runtime should trust that the metadata for the types defined in this module is correct + READYTORUN_FLAG_Partial = 0x00000004, // Set of methods with native code was determined using profile data READYTORUN_FLAG_NonSharedPInvokeStubs = 0x00000008, // PInvoke stubs compiled into image are non-shareable (no secret parameter) READYTORUN_FLAG_EmbeddedMSIL = 0x00000010, // MSIL is embedded in the composite R2R executable READYTORUN_FLAG_Component = 0x00000020, // This is the header describing a component assembly of composite R2R diff --git a/src/coreclr/tools/aot/ILCompiler.ReadyToRun/Compiler/DependencyAnalysis/ReadyToRunCodegenNodeFactory.cs b/src/coreclr/tools/aot/ILCompiler.ReadyToRun/Compiler/DependencyAnalysis/ReadyToRunCodegenNodeFactory.cs index 5ab12a41e6150e..cf51ab9090d2c1 100644 --- a/src/coreclr/tools/aot/ILCompiler.ReadyToRun/Compiler/DependencyAnalysis/ReadyToRunCodegenNodeFactory.cs +++ b/src/coreclr/tools/aot/ILCompiler.ReadyToRun/Compiler/DependencyAnalysis/ReadyToRunCodegenNodeFactory.cs @@ -47,6 +47,7 @@ public TValue GetOrAdd(TKey key) public sealed class NodeFactoryOptimizationFlags { public bool OptimizeAsyncMethods; + public bool SkipTypeValidation; } // To make the code future compatible to the composite R2R story diff --git a/src/coreclr/tools/aot/ILCompiler.ReadyToRun/Compiler/ReadyToRunCodegenCompilation.cs b/src/coreclr/tools/aot/ILCompiler.ReadyToRun/Compiler/ReadyToRunCodegenCompilation.cs index 8b1b8d7c30e2e0..dde8c9a87f4d45 100644 --- a/src/coreclr/tools/aot/ILCompiler.ReadyToRun/Compiler/ReadyToRunCodegenCompilation.cs +++ b/src/coreclr/tools/aot/ILCompiler.ReadyToRun/Compiler/ReadyToRunCodegenCompilation.cs @@ -407,6 +407,10 @@ private void RewriteComponentFile(string inputFile, string outputFile, string ow { flags |= ReadyToRunFlags.READYTORUN_FLAG_PlatformNeutralSource; } + if (_nodeFactory.OptimizationFlags.SkipTypeValidation) + { + flags |= ReadyToRunFlags.READYTORUN_FLAG_SkipTypeValidation; + } flags |= _nodeFactory.CompilationModuleGroup.GetReadyToRunFlags() & ReadyToRunFlags.READYTORUN_FLAG_MultiModuleVersionBubble; diff --git a/src/coreclr/tools/aot/ILCompiler.ReadyToRun/Compiler/ReadyToRunCodegenCompilationBuilder.cs b/src/coreclr/tools/aot/ILCompiler.ReadyToRun/Compiler/ReadyToRunCodegenCompilationBuilder.cs index 33388962e8679d..86ce06f86227c9 100644 --- a/src/coreclr/tools/aot/ILCompiler.ReadyToRun/Compiler/ReadyToRunCodegenCompilationBuilder.cs +++ b/src/coreclr/tools/aot/ILCompiler.ReadyToRun/Compiler/ReadyToRunCodegenCompilationBuilder.cs @@ -242,6 +242,10 @@ public override ICompilation ToCompilation() { flags |= ReadyToRunFlags.READYTORUN_FLAG_PlatformNeutralSource; } + if (_nodeFactoryOptimizationFlags.SkipTypeValidation) + { + flags |= ReadyToRunFlags.READYTORUN_FLAG_SkipTypeValidation; + } flags |= _compilationGroup.GetReadyToRunFlags(); NodeFactory factory = new NodeFactory( diff --git a/src/coreclr/tools/aot/crossgen2/Crossgen2RootCommand.cs b/src/coreclr/tools/aot/crossgen2/Crossgen2RootCommand.cs index e3918372e42dc9..6511e7587c507c 100644 --- a/src/coreclr/tools/aot/crossgen2/Crossgen2RootCommand.cs +++ b/src/coreclr/tools/aot/crossgen2/Crossgen2RootCommand.cs @@ -37,6 +37,8 @@ internal class Crossgen2RootCommand : RootCommand new(new[] { "--optimize-space", "--Os" }, SR.OptimizeSpaceOption); public Option OptimizeTime { get; } = new(new[] { "--optimize-time", "--Ot" }, SR.OptimizeSpeedOption); + public Option SkipTypeValidation { get; } = + new(new[] { "--skip-type-validation"}, () => true, SR.SkipTypeValidation); public Option InputBubble { get; } = new(new[] { "--inputbubble" }, SR.InputBubbleOption); public Option> InputBubbleReferenceFilePaths { get; } = @@ -200,6 +202,7 @@ public Crossgen2RootCommand(string[] args) : base(SR.Crossgen2BannerText) AddOption(OptimizeDisabled); AddOption(OptimizeSpace); AddOption(OptimizeTime); + AddOption(SkipTypeValidation); AddOption(InputBubble); AddOption(InputBubbleReferenceFilePaths); AddOption(Composite); diff --git a/src/coreclr/tools/aot/crossgen2/Program.cs b/src/coreclr/tools/aot/crossgen2/Program.cs index b044eea0b47438..51c5be712a8e91 100644 --- a/src/coreclr/tools/aot/crossgen2/Program.cs +++ b/src/coreclr/tools/aot/crossgen2/Program.cs @@ -580,6 +580,7 @@ private void RunSingleCompilation(Dictionary inFilePaths, Instru NodeFactoryOptimizationFlags nodeFactoryFlags = new NodeFactoryOptimizationFlags(); nodeFactoryFlags.OptimizeAsyncMethods = Get(_command.AsyncMethodOptimization); + nodeFactoryFlags.SkipTypeValidation = Get(_command.SkipTypeValidation); builder .UseMapFile(Get(_command.Map)) diff --git a/src/coreclr/tools/aot/crossgen2/Properties/Resources.resx b/src/coreclr/tools/aot/crossgen2/Properties/Resources.resx index b899f77a8f1147..53baa343f11af2 100644 --- a/src/coreclr/tools/aot/crossgen2/Properties/Resources.resx +++ b/src/coreclr/tools/aot/crossgen2/Properties/Resources.resx @@ -1,17 +1,17 @@  - @@ -399,4 +399,7 @@ .NET Crossgen2 Compiler - + + Configure the runtime so that it does not validate metadata for types defined in compiled assemblies + + \ No newline at end of file From 3b8143cc3e49e860b0803cfaf73f51ce8b16dd61 Mon Sep 17 00:00:00 2001 From: David Wrighton Date: Thu, 20 Apr 2023 14:12:24 -0700 Subject: [PATCH 02/23] Replace expensive lookups of generic parameter and nested class data with R2R optimized forms --- src/coreclr/inc/readytorun.h | 18 ++ .../Common/Internal/Runtime/ModuleHeaders.cs | 3 + .../Internal/Runtime/ReadyToRunConstants.cs | 17 ++ .../ReadyToRun/EnclosingTypeMapNode.cs | 73 +++++++ .../ReadyToRun/MethodIsGenericMapNode.cs | 71 +++++++ .../ReadyToRun/TypeGenericInfoMapNode.cs | 108 ++++++++++ .../ReadyToRunCodegenNodeFactory.cs | 18 ++ .../ILCompiler.ReadyToRun.csproj | 3 + src/coreclr/tools/r2rdump/TextDumper.cs | 71 +++++++ src/coreclr/vm/ceeload.cpp | 5 +- src/coreclr/vm/ceeload.h | 4 + src/coreclr/vm/class.cpp | 57 ++++-- src/coreclr/vm/clsload.cpp | 9 +- src/coreclr/vm/methodtablebuilder.cpp | 139 +++++++------ src/coreclr/vm/readytoruninfo.cpp | 190 +++++++++++++++++- src/coreclr/vm/readytoruninfo.h | 40 +++- src/coreclr/vm/siginfo.cpp | 24 ++- src/coreclr/vm/typedesc.cpp | 8 + 18 files changed, 749 insertions(+), 109 deletions(-) create mode 100644 src/coreclr/tools/aot/ILCompiler.ReadyToRun/Compiler/DependencyAnalysis/ReadyToRun/EnclosingTypeMapNode.cs create mode 100644 src/coreclr/tools/aot/ILCompiler.ReadyToRun/Compiler/DependencyAnalysis/ReadyToRun/MethodIsGenericMapNode.cs create mode 100644 src/coreclr/tools/aot/ILCompiler.ReadyToRun/Compiler/DependencyAnalysis/ReadyToRun/TypeGenericInfoMapNode.cs diff --git a/src/coreclr/inc/readytorun.h b/src/coreclr/inc/readytorun.h index 4754e0a294fe8f..7aa4f19a776056 100644 --- a/src/coreclr/inc/readytorun.h +++ b/src/coreclr/inc/readytorun.h @@ -91,6 +91,9 @@ enum class ReadyToRunSectionType : uint32_t ManifestAssemblyMvids = 118, // Added in V5.3 CrossModuleInlineInfo = 119, // Added in V6.2 HotColdMap = 120, // Added in V8.0 + MethodIsGenericMap = 121, // Added in V9.0 + EnclosingTypeMap = 122, // Added in V9.0 + TypeGenericInfoMap = 123, // Added in V9.0 // If you add a new section consider whether it is a breaking or non-breaking change. // Usually it is non-breaking, but if it is preferable to have older runtimes fail @@ -120,6 +123,21 @@ enum class ReadyToRunImportSectionFlags : uint16_t PCode = 0x0004, // Section contains pointers to code }; +enum class ReadyToRunTypeGenericInfo : uint8_t +{ + GenericCountMask = 0x3, + HasConstraints = 0x4, + HasVariance = 0x8, +}; + +enum class ReadyToRunGenericInfoGenericCount : uint32_t +{ + Zero = 0, + One = 1, + Two = 2, + MoreThanTwo = 3 +}; + // // READYTORUN_IMPORT_SECTION describes image range with references to code or runtime data structures // diff --git a/src/coreclr/tools/Common/Internal/Runtime/ModuleHeaders.cs b/src/coreclr/tools/Common/Internal/Runtime/ModuleHeaders.cs index f1985389496664..a9867d903a3f0e 100644 --- a/src/coreclr/tools/Common/Internal/Runtime/ModuleHeaders.cs +++ b/src/coreclr/tools/Common/Internal/Runtime/ModuleHeaders.cs @@ -71,6 +71,9 @@ public enum ReadyToRunSectionType ManifestAssemblyMvids = 118, // Added in 5.3 CrossModuleInlineInfo = 119, // Added in 6.3 HotColdMap = 120, // Added in 8.0 + MethodIsGenericMap = 121, // Added in V9.0 + EnclosingTypeMap = 122, // Added in V9.0 + TypeGenericInfoMap = 123, // Added in V9.0 // // NativeAOT ReadyToRun sections diff --git a/src/coreclr/tools/Common/Internal/Runtime/ReadyToRunConstants.cs b/src/coreclr/tools/Common/Internal/Runtime/ReadyToRunConstants.cs index 4383c99b25ca66..6ee1c8a596099c 100644 --- a/src/coreclr/tools/Common/Internal/Runtime/ReadyToRunConstants.cs +++ b/src/coreclr/tools/Common/Internal/Runtime/ReadyToRunConstants.cs @@ -89,6 +89,23 @@ internal enum ReadyToRunCrossModuleInlineFlags : uint InlinerRidShift = 1, }; + [Flags] + public enum ReadyToRunTypeGenericInfo : byte + { + GenericCountMask = 0x3, + HasConstraints = 0x4, + HasVariance = 0x8, + } + + public enum ReadyToRunGenericInfoGenericCount : uint + { + Zero = 0, + One = 1, + Two = 2, + MoreThanTwo = 3 + } + + public enum DictionaryEntryKind { EmptySlot = 0, diff --git a/src/coreclr/tools/aot/ILCompiler.ReadyToRun/Compiler/DependencyAnalysis/ReadyToRun/EnclosingTypeMapNode.cs b/src/coreclr/tools/aot/ILCompiler.ReadyToRun/Compiler/DependencyAnalysis/ReadyToRun/EnclosingTypeMapNode.cs new file mode 100644 index 00000000000000..4433dfffd3a2f4 --- /dev/null +++ b/src/coreclr/tools/aot/ILCompiler.ReadyToRun/Compiler/DependencyAnalysis/ReadyToRun/EnclosingTypeMapNode.cs @@ -0,0 +1,73 @@ +// Licensed to the .NET Foundation under one or more agreements. +// The .NET Foundation licenses this file to you under the MIT license. + +using System; + +using Internal.Text; +using System.Diagnostics; +using System.Reflection; +using System.Reflection.Metadata; +using System.Reflection.Metadata.Ecma335; +using Internal.ReadyToRunConstants; + +namespace ILCompiler.DependencyAnalysis.ReadyToRun +{ + public class EnclosingTypeMapNode : HeaderTableNode + { + private MetadataReader _metadata; + + public EnclosingTypeMapNode(MetadataReader metadata) + { + _metadata = metadata; + // This map is only valid for assemblies with <= 0xFFFE types defined within + if (!IsSupported(_metadata)) + throw new InternalCompilerErrorException("EnclosingTypeMap made for assembly with more than 0xFFFE types"); + } + + public static bool IsSupported(MetadataReader metadata) + { + // This map is only valid for assemblies with <= 0xFFFE types defined within + // and really shouldn't be generated for tiny assemblies, as the map provides very little to no value + // in those situations + int typeDefinitionCount = metadata.TypeDefinitions.Count; + + return ((typeDefinitionCount > 10) && (typeDefinitionCount <= 0xFFFE)); + } + + public override int ClassCode => 990540812; + + public override void AppendMangledName(NameMangler nameMangler, Utf8StringBuilder sb) + { + sb.Append(nameMangler.CompilationUnitPrefix); + sb.Append("__EnclosingTypeMap"); + } + + public override ObjectData GetData(NodeFactory factory, bool relocsOnly = false) + { + // This node does not trigger generation of other nodes. + if (relocsOnly) + return new ObjectData(Array.Empty(), Array.Empty(), 1, new ISymbolDefinitionNode[] { this }); + + ObjectDataBuilder builder = new ObjectDataBuilder(factory, relocsOnly); + builder.AddSymbol(this); + + // This map is only valid for assemblies with <= 0xFFFE types defined within + builder.EmitUShort(checked((ushort)_metadata.TypeDefinitions.Count)); + + foreach (var typeDefinitionHandle in _metadata.TypeDefinitions) + { + var typeDefinition = _metadata.GetTypeDefinition(typeDefinitionHandle); + if (!typeDefinition.IsNested) + { + builder.EmitUShort(0); + } + else + { + builder.EmitUShort(checked((ushort)MetadataTokens.GetRowNumber(typeDefinition.GetDeclaringType()))); + } + } + + return builder.ToObjectData(); + } + } +} diff --git a/src/coreclr/tools/aot/ILCompiler.ReadyToRun/Compiler/DependencyAnalysis/ReadyToRun/MethodIsGenericMapNode.cs b/src/coreclr/tools/aot/ILCompiler.ReadyToRun/Compiler/DependencyAnalysis/ReadyToRun/MethodIsGenericMapNode.cs new file mode 100644 index 00000000000000..4f79f6aa33ca20 --- /dev/null +++ b/src/coreclr/tools/aot/ILCompiler.ReadyToRun/Compiler/DependencyAnalysis/ReadyToRun/MethodIsGenericMapNode.cs @@ -0,0 +1,71 @@ +// Licensed to the .NET Foundation under one or more agreements. +// The .NET Foundation licenses this file to you under the MIT license. + +using System; + +using Internal.Text; +using System.Diagnostics; +using System.Reflection.Metadata; + +namespace ILCompiler.DependencyAnalysis.ReadyToRun +{ + public class MethodIsGenericMapNode : HeaderTableNode + { + private MetadataReader _metadata; + + public MethodIsGenericMapNode(MetadataReader metadata) + { + _metadata = metadata; + } + + public static bool IsSupported(MetadataReader metadata) + { + // Only support this map with R2R images of some size + return metadata.MethodDefinitions.Count > 32; + } + + public override int ClassCode => 606284890; + + public override void AppendMangledName(NameMangler nameMangler, Utf8StringBuilder sb) + { + sb.Append(nameMangler.CompilationUnitPrefix); + sb.Append("__MethodIsGenericMap"); + } + + public override ObjectData GetData(NodeFactory factory, bool relocsOnly = false) + { + // This node does not trigger generation of other nodes. + if (relocsOnly) + return new ObjectData(Array.Empty(), Array.Empty(), 1, new ISymbolDefinitionNode[] { this }); + + ObjectDataBuilder builder = new ObjectDataBuilder(factory, relocsOnly); + builder.AddSymbol(this); + + builder.EmitInt(_metadata.MethodDefinitions.Count); + + int usedBits = 0; + byte curByte = 0; + foreach (var methodDefinitionHandle in _metadata.MethodDefinitions) + { + var methodDefinition = _metadata.GetMethodDefinition(methodDefinitionHandle); + bool isGeneric = methodDefinition.GetGenericParameters().Count > 0; + curByte |= isGeneric ? (byte)1 : (byte)0; + usedBits++; + if (usedBits == 8) + { + builder.EmitByte(curByte); + usedBits = 0; + curByte = 0; + } + else + { + curByte <<= 1; + } + } + if (usedBits != 0) + builder.EmitByte((byte)(curByte << (7 - usedBits))); + + return builder.ToObjectData(); + } + } +} diff --git a/src/coreclr/tools/aot/ILCompiler.ReadyToRun/Compiler/DependencyAnalysis/ReadyToRun/TypeGenericInfoMapNode.cs b/src/coreclr/tools/aot/ILCompiler.ReadyToRun/Compiler/DependencyAnalysis/ReadyToRun/TypeGenericInfoMapNode.cs new file mode 100644 index 00000000000000..7e01d89db7012d --- /dev/null +++ b/src/coreclr/tools/aot/ILCompiler.ReadyToRun/Compiler/DependencyAnalysis/ReadyToRun/TypeGenericInfoMapNode.cs @@ -0,0 +1,108 @@ +// Licensed to the .NET Foundation under one or more agreements. +// The .NET Foundation licenses this file to you under the MIT license. + +using System; + +using Internal.Text; +using System.Diagnostics; +using System.Reflection; +using System.Reflection.Metadata; +using Internal.ReadyToRunConstants; +using Internal.TypeSystem; + +namespace ILCompiler.DependencyAnalysis.ReadyToRun +{ + public class TypeGenericInfoMapNode : HeaderTableNode + { + private MetadataReader _metadata; + + public TypeGenericInfoMapNode(MetadataReader metadata) + { + _metadata = metadata; + } + + public static bool IsSupported(MetadataReader metadata) + { + // Only support this map with R2R images of some size + return metadata.TypeDefinitions.Count > 16; + } + + public override int ClassCode => 1329419084; + + public override void AppendMangledName(NameMangler nameMangler, Utf8StringBuilder sb) + { + sb.Append(nameMangler.CompilationUnitPrefix); + sb.Append("__TypeGenericInfoMapNode"); + } + + public override ObjectData GetData(NodeFactory factory, bool relocsOnly = false) + { + // This node does not trigger generation of other nodes. + if (relocsOnly) + return new ObjectData(Array.Empty(), Array.Empty(), 1, new ISymbolDefinitionNode[] { this }); + + ObjectDataBuilder builder = new ObjectDataBuilder(factory, relocsOnly); + builder.AddSymbol(this); + + builder.EmitUInt((uint)_metadata.TypeDefinitions.Count); + + int usedBits = 0; + byte curByte = 0; + foreach (var typeDefinitionHandle in _metadata.TypeDefinitions) + { + var typeDefinition = _metadata.GetTypeDefinition(typeDefinitionHandle); + bool isGeneric = typeDefinition.GetGenericParameters().Count > 0; + ReadyToRunTypeGenericInfo genericInfo = default(ReadyToRunTypeGenericInfo); + bool hasVariance = false; + bool hasConstraints = false; + + foreach (var genericParameterHandle in typeDefinition.GetGenericParameters()) + { + var genericParameter = _metadata.GetGenericParameter(genericParameterHandle); + if ((genericParameter.Attributes & GenericParameterAttributes.VarianceMask) != GenericParameterAttributes.None) + hasVariance = true; + + if ((genericParameter.Attributes & (GenericParameterAttributes.SpecialConstraintMask | (GenericParameterAttributes)GenericConstraints.AcceptByRefLike)) != default(GenericParameterAttributes) || + (genericParameter.GetConstraints().Count > 0)) + { + hasConstraints = true; + } + } + + ReadyToRunGenericInfoGenericCount count; + switch (typeDefinition.GetGenericParameters().Count) + { + case 0: + count = ReadyToRunGenericInfoGenericCount.Zero; break; + case 1: + count = ReadyToRunGenericInfoGenericCount.One; break; + case 2: + count = ReadyToRunGenericInfoGenericCount.Two; break; + default: + count = ReadyToRunGenericInfoGenericCount.MoreThanTwo; break; + } + + genericInfo = (ReadyToRunTypeGenericInfo)count | + (hasVariance ? ReadyToRunTypeGenericInfo.HasVariance : default(ReadyToRunTypeGenericInfo)) | + (hasConstraints ? ReadyToRunTypeGenericInfo.HasConstraints : default(ReadyToRunTypeGenericInfo)); + + curByte |= (byte)genericInfo; + usedBits += 4; + if (usedBits == 8) + { + builder.EmitByte(curByte); + usedBits = 0; + curByte = 0; + } + else + { + curByte <<= 4; + } + } + if (usedBits != 0) + builder.EmitByte(curByte); + + return builder.ToObjectData(); + } + } +} diff --git a/src/coreclr/tools/aot/ILCompiler.ReadyToRun/Compiler/DependencyAnalysis/ReadyToRunCodegenNodeFactory.cs b/src/coreclr/tools/aot/ILCompiler.ReadyToRun/Compiler/DependencyAnalysis/ReadyToRunCodegenNodeFactory.cs index cf51ab9090d2c1..10899f8ea5befb 100644 --- a/src/coreclr/tools/aot/ILCompiler.ReadyToRun/Compiler/DependencyAnalysis/ReadyToRunCodegenNodeFactory.cs +++ b/src/coreclr/tools/aot/ILCompiler.ReadyToRun/Compiler/DependencyAnalysis/ReadyToRunCodegenNodeFactory.cs @@ -732,6 +732,24 @@ public void AttachToDependencyGraph(DependencyAnalyzerBase graph, I AttributePresenceFilterNode attributePresenceTable = new AttributePresenceFilterNode(inputModule); tableHeader.Add(Internal.Runtime.ReadyToRunSectionType.AttributePresence, attributePresenceTable, attributePresenceTable); } + + if (EnclosingTypeMapNode.IsSupported(inputModule.MetadataReader)) + { + var node = new EnclosingTypeMapNode(inputModule.MetadataReader); + tableHeader.Add(Internal.Runtime.ReadyToRunSectionType.EnclosingTypeMap, node, node); + } + + if (TypeGenericInfoMapNode.IsSupported(inputModule.MetadataReader)) + { + var node = new TypeGenericInfoMapNode(inputModule.MetadataReader); + tableHeader.Add(Internal.Runtime.ReadyToRunSectionType.TypeGenericInfoMap, node, node); + } + + if (MethodIsGenericMapNode.IsSupported(inputModule.MetadataReader)) + { + var node = new MethodIsGenericMapNode(inputModule.MetadataReader); + tableHeader.Add(Internal.Runtime.ReadyToRunSectionType.MethodIsGenericMap, node, node); + } } InliningInfoNode crossModuleInliningInfoTable = new InliningInfoNode(null, diff --git a/src/coreclr/tools/aot/ILCompiler.ReadyToRun/ILCompiler.ReadyToRun.csproj b/src/coreclr/tools/aot/ILCompiler.ReadyToRun/ILCompiler.ReadyToRun.csproj index 874104ecb86821..df753c01a79aa4 100644 --- a/src/coreclr/tools/aot/ILCompiler.ReadyToRun/ILCompiler.ReadyToRun.csproj +++ b/src/coreclr/tools/aot/ILCompiler.ReadyToRun/ILCompiler.ReadyToRun.csproj @@ -141,6 +141,9 @@ + + + diff --git a/src/coreclr/tools/r2rdump/TextDumper.cs b/src/coreclr/tools/r2rdump/TextDumper.cs index 403c28b3ca27cf..e9c658d935fa6d 100644 --- a/src/coreclr/tools/r2rdump/TextDumper.cs +++ b/src/coreclr/tools/r2rdump/TextDumper.cs @@ -10,6 +10,7 @@ using System.Reflection.PortableExecutable; using System.Text; using ILCompiler.Reflection.ReadyToRun; +using Internal.ReadyToRunConstants; using Internal.Runtime; namespace R2RDump @@ -528,6 +529,76 @@ public override void DumpSectionContents(ReadyToRunSection section) _writer.WriteLine(NativeReader.ReadInt32(_r2r.Image, ref hotColdMapOffset)); } break; + case ReadyToRunSectionType.MethodIsGenericMap: + { + int mapOffset = _r2r.GetOffset(section.RelativeVirtualAddress); + int mapDone = section.Size + mapOffset; + int countMethods = NativeReader.ReadInt32(_r2r.Image, ref mapOffset); + int curMethod = 1; + while ((curMethod <= countMethods) && mapDone > mapOffset) + { + byte curByte = NativeReader.ReadByte(_r2r.Image, ref mapOffset); + for (int i = 0; i < 8 && (curMethod <= countMethods); i++) + { + bool isGeneric = (curByte & 0x80) == 0x80; + _writer.WriteLine($"{curMethod | 0x06000000:x8} : {isGeneric}"); + curByte <<= 1; + curMethod++; + } + } + if (curMethod != (countMethods + 1)) + { + Program.WriteWarning("MethodIsGenericMap malformed"); + System.Diagnostics.Debug.Assert(false, "MethodIsGenericMap malformed"); + } + break; + } + case ReadyToRunSectionType.EnclosingTypeMap: + { + int mapOffset = _r2r.GetOffset(section.RelativeVirtualAddress); + int mapDone = section.Size + mapOffset; + uint countTypes = checked((uint)((section.Size / 2 - 1))); // 2 bytes per nested type. This data structure is only used for IL images where there are <= 0xFFFE types. + if (countTypes != NativeReader.ReadUInt16(_r2r.Image, ref mapOffset)) + { + Program.WriteWarning("EnclosingTypeMap malformed"); + System.Diagnostics.Debug.Assert(false, "EnclosingTypeMap malformed"); + } + int curType = 1; + while (curType <= (countTypes + 1)) + { + _writer.WriteLine($"{curType | 0x02000000:x8} : {NativeReader.ReadUInt16(_r2r.Image, ref mapOffset) | 0x02000000:x8}"); + curType++; + } + break; + } + case ReadyToRunSectionType.TypeGenericInfoMap: + { + int mapOffset = _r2r.GetOffset(section.RelativeVirtualAddress); + int mapDone = section.Size + mapOffset; + int countTypes = NativeReader.ReadInt32(_r2r.Image, ref mapOffset); + int curType = 1; + while ((curType <= countTypes) && mapDone > mapOffset) + { + byte curByte = NativeReader.ReadByte(_r2r.Image, ref mapOffset); + for (int i = 0; i < 2 && (curType <= countTypes); i++) + { + var genericInfo = (ReadyToRunTypeGenericInfo)((curByte & 0xF0) >> 4); + bool hasConstraints = genericInfo.HasFlag(ReadyToRunTypeGenericInfo.HasConstraints); + bool hasVariance = genericInfo.HasFlag(ReadyToRunTypeGenericInfo.HasVariance); + var genericCount = (ReadyToRunGenericInfoGenericCount)(genericInfo & ReadyToRunTypeGenericInfo.GenericCountMask); + _writer.WriteLine($"{curType | 0x06000000:x8} : GenericArgumentCount: {genericCount} HasVariance {hasVariance} HasConstraints {hasConstraints}"); + curByte <<= 4; + curType++; + } + } + if (curType != (countTypes + 1)) + { + Program.WriteWarning("TypeGenericInfoMap malformed"); + System.Diagnostics.Debug.Assert(false, "TypeGenericInfoMap malformed"); + } + break; + } + default: _writer.WriteLine("Unsupported section type {0}", section.Type); break; diff --git a/src/coreclr/vm/ceeload.cpp b/src/coreclr/vm/ceeload.cpp index f9b466a19c8625..7d0c1bc2098588 100644 --- a/src/coreclr/vm/ceeload.cpp +++ b/src/coreclr/vm/ceeload.cpp @@ -926,10 +926,7 @@ void Module::BuildStaticsOffsets(AllocMemTracker *pamTracker) DWORD dwClassGCHandles[2] = { 0, 0 }; // need to check if the type is generic and if so exclude it from iteration as we don't know the size - HENUMInternalHolder hGenericEnum(pImport); - hGenericEnum.EnumInit(mdtGenericParam, type); - ULONG cGenericParams = pImport->EnumGetCount(&hGenericEnum); - if (cGenericParams == 0) + if (!m_pTypeGenericInfoMap->IsGeneric(type, pImport)) { HENUMInternalHolder hFieldEnum(pImport); hFieldEnum.EnumInit(mdtFieldDef, type); diff --git a/src/coreclr/vm/ceeload.h b/src/coreclr/vm/ceeload.h index c81a5a226bb8bc..62826d34b0d9fa 100644 --- a/src/coreclr/vm/ceeload.h +++ b/src/coreclr/vm/ceeload.h @@ -560,6 +560,7 @@ class ModuleBase return NULL; }; + ReadyToRun_EnclosingTypeMap *m_pEnclosingTypeMap = &ReadyToRun_EnclosingTypeMap::EmptyInstance; #ifndef DACCESS_COMPILE // The vtable needs to match between DAC and non-DAC, but we don't want any use of ThrowTypeLoadException in the DAC @@ -1612,6 +1613,9 @@ class Module : public ModuleBase // Self-initializing accessor for domain-independent IJW thunk heap LoaderHeap *GetDllThunkHeap(); + ReadyToRun_MethodIsGenericMap *m_pMethodIsGenericMap = &ReadyToRun_MethodIsGenericMap::EmptyInstance; + ReadyToRun_TypeGenericInfoMap *m_pTypeGenericInfoMap = &ReadyToRun_TypeGenericInfoMap::EmptyInstance; + protected: void BuildStaticsOffsets (AllocMemTracker *pamTracker); diff --git a/src/coreclr/vm/class.cpp b/src/coreclr/vm/class.cpp index 4e8ca18fa09f6b..976604bae2ca44 100644 --- a/src/coreclr/vm/class.cpp +++ b/src/coreclr/vm/class.cpp @@ -783,35 +783,52 @@ EEClass::CheckVarianceInSig( if (!ClassLoader::ResolveTokenToTypeDefThrowing(pModule, typeref, &pDefModule, &typeDef)) return TRUE; - HENUMInternal hEnumGenericPars; - if (FAILED(pDefModule->GetMDImport()->EnumInit(mdtGenericParam, typeDef, &hEnumGenericPars))) + bool foundHasVarianceResult; + + if (!pDefModule->m_pTypeGenericInfoMap->HasVariance(typeDef, &foundHasVarianceResult) && foundHasVarianceResult) { - pDefModule->GetAssembly()->ThrowTypeLoadException(pDefModule->GetMDImport(), typeDef, IDS_CLASSLOAD_BADFORMAT); - } + // Fast path, now that we know there isn't variance + uint32_t genericArgCount = pDefModule->m_pTypeGenericInfoMap->GetGenericArgumentCount(typeDef, pDefModule->GetMDImport()); + for (uint32_t iGenericArgCount = 0; iGenericArgCount < genericArgCount; iGenericArgCount++) + { + if (!CheckVarianceInSig(numGenericArgs, pVarianceInfo, pModule, psig, gpNonVariant)) + return FALSE; - for (unsigned i = 0; i < ntypars; i++) + IfFailThrow(psig.SkipExactlyOne()); + } + } + else { - mdGenericParam tkTyPar; - pDefModule->GetMDImport()->EnumNext(&hEnumGenericPars, &tkTyPar); - DWORD flags; - if (FAILED(pDefModule->GetMDImport()->GetGenericParamProps(tkTyPar, NULL, &flags, NULL, NULL, NULL))) + HENUMInternal hEnumGenericPars; + if (FAILED(pDefModule->GetMDImport()->EnumInit(mdtGenericParam, typeDef, &hEnumGenericPars))) { pDefModule->GetAssembly()->ThrowTypeLoadException(pDefModule->GetMDImport(), typeDef, IDS_CLASSLOAD_BADFORMAT); } - CorGenericParamAttr genPosition = (CorGenericParamAttr) (flags & gpVarianceMask); - // If the surrounding context is contravariant then we need to flip the variance of this parameter - if (position == gpContravariant) + + for (unsigned i = 0; i < ntypars; i++) { - genPosition = genPosition == gpCovariant ? gpContravariant - : genPosition == gpContravariant ? gpCovariant - : gpNonVariant; + mdGenericParam tkTyPar; + pDefModule->GetMDImport()->EnumNext(&hEnumGenericPars, &tkTyPar); + DWORD flags; + if (FAILED(pDefModule->GetMDImport()->GetGenericParamProps(tkTyPar, NULL, &flags, NULL, NULL, NULL))) + { + pDefModule->GetAssembly()->ThrowTypeLoadException(pDefModule->GetMDImport(), typeDef, IDS_CLASSLOAD_BADFORMAT); + } + CorGenericParamAttr genPosition = (CorGenericParamAttr) (flags & gpVarianceMask); + // If the surrounding context is contravariant then we need to flip the variance of this parameter + if (position == gpContravariant) + { + genPosition = genPosition == gpCovariant ? gpContravariant + : genPosition == gpContravariant ? gpCovariant + : gpNonVariant; + } + if (!CheckVarianceInSig(numGenericArgs, pVarianceInfo, pModule, psig, genPosition)) + return FALSE; + + IfFailThrow(psig.SkipExactlyOne()); } - if (!CheckVarianceInSig(numGenericArgs, pVarianceInfo, pModule, psig, genPosition)) - return FALSE; - - IfFailThrow(psig.SkipExactlyOne()); + pDefModule->GetMDImport()->EnumClose(&hEnumGenericPars); } - pDefModule->GetMDImport()->EnumClose(&hEnumGenericPars); } return TRUE; diff --git a/src/coreclr/vm/clsload.cpp b/src/coreclr/vm/clsload.cpp index 2953398ae9ac16..792e640accaf51 100644 --- a/src/coreclr/vm/clsload.cpp +++ b/src/coreclr/vm/clsload.cpp @@ -2144,12 +2144,7 @@ TypeHandle ClassLoader::LoadTypeDefThrowing(Module *pModule, if (typeHnd.IsNull() && pTargetInstantiation != NULL) { // If the type is not loaded yet, we have to do heavy weight arity verification based on metadata - HENUMInternal hEnumGenericPars; - HRESULT hr = pInternalImport->EnumInit(mdtGenericParam, typeDef, &hEnumGenericPars); - if (FAILED(hr)) - pModule->GetAssembly()->ThrowTypeLoadException(pInternalImport, typeDef, IDS_CLASSLOAD_BADFORMAT); - DWORD nGenericClassParams = pInternalImport->EnumGetCount(&hEnumGenericPars); - pInternalImport->EnumClose(&hEnumGenericPars); + uint32_t nGenericClassParams = pModule->m_pTypeGenericInfoMap->GetGenericArgumentCount(typeDef, pInternalImport); if (pTargetInstantiation->GetNumArgs() != nGenericClassParams) pModule->GetAssembly()->ThrowTypeLoadException(pInternalImport, typeDef, IDS_CLASSLOAD_TYPEWRONGNUMGENERICARGS); @@ -2658,7 +2653,7 @@ ClassLoader::GetEnclosingClassThrowing( _ASSERTE(tdEnclosing); *tdEnclosing = mdTypeDefNil; - HRESULT hr = pInternalImport->GetNestedClassProps(cl, tdEnclosing); + HRESULT hr = pModule->m_pEnclosingTypeMap->GetEnclosingTypeNoThrow(cl, tdEnclosing, pInternalImport); if (FAILED(hr)) { diff --git a/src/coreclr/vm/methodtablebuilder.cpp b/src/coreclr/vm/methodtablebuilder.cpp index bd867cddbacb68..ecddcd2ddee979 100644 --- a/src/coreclr/vm/methodtablebuilder.cpp +++ b/src/coreclr/vm/methodtablebuilder.cpp @@ -822,8 +822,7 @@ MethodTableBuilder::bmtRTType::GetEnclosingTypeToken() const if (IsNested()) { // This is guaranteed to succeed because the EEClass would not have been // set as nested unless a valid token was stored in metadata. - if (FAILED(GetModule()->GetMDImport()->GetNestedClassProps( - GetTypeDefToken(), &tok))) + if (FAILED(GetModule()->m_pEnclosingTypeMap->GetEnclosingTypeNoThrow(GetTypeDefToken(), &tok, GetModule()->GetMDImport()))) { return mdTypeDefNil; } @@ -969,7 +968,8 @@ MethodTableBuilder::bmtMDType::bmtMDType( IfFailThrow(m_pModule->GetMDImport()->GetTypeDefProps(m_tok, &m_dwAttrs, NULL)); - HRESULT hr = m_pModule->GetMDImport()->GetNestedClassProps(m_tok, &m_enclTok); + HRESULT hr = m_pModule->m_pEnclosingTypeMap->GetEnclosingTypeNoThrow(m_tok, &m_enclTok, m_pModule->GetMDImport()); + if (FAILED(hr)) { if (hr != CLDB_E_RECORD_NOTFOUND) @@ -1440,17 +1440,17 @@ MethodTableBuilder::BuildMethodTableThrowing( } mdTypeDef tdEnclosing = mdTypeDefNil; - HRESULT hr = GetMDImport()->GetNestedClassProps(GetCl(), &tdEnclosing); + HRESULT hr = GetModule()->m_pEnclosingTypeMap->GetEnclosingTypeNoThrow(GetCl(), &tdEnclosing, GetModule()->GetMDImport()); if (FAILED(hr)) ThrowHR(hr, BFA_UNABLE_TO_GET_NESTED_PROPS); - HENUMInternalHolder hEnumGenericPars(GetMDImport()); - if (FAILED(hEnumGenericPars.EnumInitNoThrow(mdtGenericParam, tdEnclosing))) + uint32_t genericArgCount; + if (FAILED(GetModule()->m_pTypeGenericInfoMap->GetGenericArgumentCountNoThrow(tdEnclosing, &genericArgCount, GetMDImport()))) { GetAssembly()->ThrowTypeLoadException(GetMDImport(), tdEnclosing, IDS_CLASSLOAD_BADFORMAT); } - if (hEnumGenericPars.EnumGetCount() != bmtGenerics->GetNumGenericArgs()) + if (genericArgCount != bmtGenerics->GetNumGenericArgs()) { BuildMethodTableThrowException(IDS_CLASSLOAD_ENUM_EXTRA_GENERIC_TYPE_PARAM); } @@ -2662,8 +2662,8 @@ MethodTableBuilder::EnumerateClassMethods() BOOL fIsClassValueType = IsValueClass(); BOOL fIsClassComImport = IsComImport(); BOOL fIsClassNotAbstract = (IsTdAbstract(GetAttrClass()) == 0); - PCCOR_SIGNATURE pMemberSignature; - ULONG cMemberSignature; + PCCOR_SIGNATURE pMemberSignature = NULL; + ULONG cMemberSignature = 0; // // Run through the method list and calculate the following: @@ -2701,6 +2701,9 @@ MethodTableBuilder::EnumerateClassMethods() METHOD_IMPL_TYPE implType; LPSTR strMethodName; + pMemberSignature = NULL; + cMemberSignature = 0; + // // Go to the next method and retrieve its attributes. // @@ -2711,7 +2714,7 @@ MethodTableBuilder::EnumerateClassMethods() { BuildMethodTableThrowException(BFA_METHOD_TOKEN_OUT_OF_RANGE); } - if (FAILED(pMDInternalImport->GetSigOfMethodDef(tok, &cMemberSignature, &pMemberSignature))) + if (!bmtProp->fNoSanityChecks && FAILED(pMDInternalImport->GetSigOfMethodDef(tok, &cMemberSignature, &pMemberSignature))) { BuildMethodTableThrowException(hr, BFA_BAD_SIGNATURE, mdMethodDefNil); } @@ -2757,9 +2760,17 @@ MethodTableBuilder::EnumerateClassMethods() } } - DWORD numGenericMethodArgs = 0; - + bool hasGenericMethodArgsComputed; + bool hasGenericMethodArgs = this->GetModule()->m_pMethodIsGenericMap->IsGeneric(tok, &hasGenericMethodArgsComputed); + if (!hasGenericMethodArgsComputed) { + if (pMemberSignature == NULL) + { + if (FAILED(pMDInternalImport->GetSigOfMethodDef(tok, &cMemberSignature, &pMemberSignature))) + { + BuildMethodTableThrowException(hr, BFA_BAD_SIGNATURE, mdMethodDefNil); + } + } SigParser genericArgParser(pMemberSignature, cMemberSignature); uint32_t ulCallConv; hr = genericArgParser.GetCallingConvInfo(&ulCallConv); @@ -2769,50 +2780,52 @@ MethodTableBuilder::EnumerateClassMethods() } // Only read the generic parameter table if the method signature is generic - if (ulCallConv & IMAGE_CEE_CS_CALLCONV_GENERIC) + hasGenericMethodArgs = !!(ulCallConv & IMAGE_CEE_CS_CALLCONV_GENERIC); + hasGenericMethodArgsComputed = true; + } + + if (hasGenericMethodArgs && !bmtProp->fNoSanityChecks) + { + HENUMInternalHolder hEnumTyPars(pMDInternalImport); + hr = hEnumTyPars.EnumInitNoThrow(mdtGenericParam, tok); + if (FAILED(hr)) { - HENUMInternalHolder hEnumTyPars(pMDInternalImport); - hr = hEnumTyPars.EnumInitNoThrow(mdtGenericParam, tok); - if (FAILED(hr)) - { - BuildMethodTableThrowException(hr, *bmtError); - } + BuildMethodTableThrowException(hr, *bmtError); + } - numGenericMethodArgs = hEnumTyPars.EnumGetCount(); - if (numGenericMethodArgs != 0) - { - HENUMInternalHolder hEnumGenericPars(pMDInternalImport); + uint32_t numGenericMethodArgs = hEnumTyPars.EnumGetCount(); + if (numGenericMethodArgs != 0) + { + HENUMInternalHolder hEnumGenericPars(pMDInternalImport); - hEnumGenericPars.EnumInit(mdtGenericParam, tok); + hEnumGenericPars.EnumInit(mdtGenericParam, tok); - for (unsigned methIdx = 0; methIdx < numGenericMethodArgs; methIdx++) + for (unsigned methIdx = 0; methIdx < numGenericMethodArgs; methIdx++) + { + mdGenericParam tkTyPar; + pMDInternalImport->EnumNext(&hEnumGenericPars, &tkTyPar); + DWORD flags; + if (FAILED(pMDInternalImport->GetGenericParamProps(tkTyPar, NULL, &flags, NULL, NULL, NULL))) { - mdGenericParam tkTyPar; - pMDInternalImport->EnumNext(&hEnumGenericPars, &tkTyPar); - DWORD flags; - if (FAILED(pMDInternalImport->GetGenericParamProps(tkTyPar, NULL, &flags, NULL, NULL, NULL))) - { - BuildMethodTableThrowException(IDS_CLASSLOAD_BADFORMAT); - } - - if (0 != (flags & ~(gpVarianceMask | gpSpecialConstraintMask))) - { - BuildMethodTableThrowException(IDS_CLASSLOAD_BADFORMAT); - } - switch (flags & gpVarianceMask) - { - case gpNonVariant: - break; + BuildMethodTableThrowException(IDS_CLASSLOAD_BADFORMAT); + } - case gpCovariant: // intentional fallthru - case gpContravariant: - BuildMethodTableThrowException(VLDTR_E_GP_ILLEGAL_VARIANT_MVAR); - break; + if (0 != (flags & ~(gpVarianceMask | gpSpecialConstraintMask))) + { + BuildMethodTableThrowException(IDS_CLASSLOAD_BADFORMAT); + } + switch (flags & gpVarianceMask) + { + case gpNonVariant: + break; - default: - BuildMethodTableThrowException(IDS_CLASSLOAD_BADFORMAT); - } + case gpCovariant: // intentional fallthru + case gpContravariant: + BuildMethodTableThrowException(VLDTR_E_GP_ILLEGAL_VARIANT_MVAR); + break; + default: + BuildMethodTableThrowException(IDS_CLASSLOAD_BADFORMAT); } } } @@ -3041,7 +3054,7 @@ MethodTableBuilder::EnumerateClassMethods() //@GENERICS: // Generic methods or methods in generic classes // may not be part of a COM Import class, PInvoke, internal call outside CoreLib. - if ((bmtGenerics->GetNumGenericArgs() != 0 || numGenericMethodArgs != 0) && + if ((bmtGenerics->GetNumGenericArgs() != 0 || hasGenericMethodArgs) && ( #ifdef FEATURE_COMINTEROP fIsClassComImport || @@ -3056,7 +3069,7 @@ MethodTableBuilder::EnumerateClassMethods() // Generic methods may not be marked "runtime". However note that // methods in generic delegate classes are, hence we don't apply this to // methods in generic classes in general. - if (numGenericMethodArgs != 0 && IsMiRuntime(dwImplFlags)) + if (hasGenericMethodArgs && IsMiRuntime(dwImplFlags)) { BuildMethodTableThrowException(BFA_GENERIC_METHOD_RUNTIME_IMPL); } @@ -3066,6 +3079,14 @@ MethodTableBuilder::EnumerateClassMethods() // checked for non-virtual static methods as they cannot be called variantly. if ((bmtGenerics->pVarianceInfo != NULL) && (IsMdVirtual(dwMemberAttrs) || !IsMdStatic(dwMemberAttrs))) { + if (pMemberSignature == NULL) + { + if (FAILED(pMDInternalImport->GetSigOfMethodDef(tok, &cMemberSignature, &pMemberSignature))) + { + BuildMethodTableThrowException(hr, BFA_BAD_SIGNATURE, mdMethodDefNil); + } + } + SigPointer sp(pMemberSignature, cMemberSignature); uint32_t callConv; IfFailThrow(sp.GetCallingConvInfo(&callConv)); @@ -3196,7 +3217,7 @@ MethodTableBuilder::EnumerateClassMethods() delegateMethodsSeen |= newDelegateMethodSeen; } - else if (numGenericMethodArgs != 0) + else if (hasGenericMethodArgs) { //We use an instantiated method desc to represent a generic method type = METHOD_TYPE_INSTANTIATED; @@ -3240,7 +3261,7 @@ MethodTableBuilder::EnumerateClassMethods() } // Generic methods should always be METHOD_TYPE_INSTANTIATED - if ((numGenericMethodArgs != 0) && (type != METHOD_TYPE_INSTANTIATED)) + if (hasGenericMethodArgs && (type != METHOD_TYPE_INSTANTIATED)) { BuildMethodTableThrowException(BFA_GENERIC_METHODS_INST); } @@ -4593,7 +4614,7 @@ bool MethodTableBuilder::IsEnclosingNestedTypePair( while (tkEncl != tkNest) { // Do this using the metadata APIs because MethodTableBuilder does // not construct type representations for enclosing type chains. - if (FAILED(pMDImport->GetNestedClassProps(tkNest, &tkNest))) + if (FAILED(pModule->m_pEnclosingTypeMap->GetEnclosingTypeNoThrow(tkNest, &tkNest, pMDImport))) { // tokNest is not a nested type. return false; } @@ -12040,12 +12061,7 @@ MethodTableBuilder::GatherGenericsInfo( IMDInternalImport * pInternalImport = pModule->GetMDImport(); // Enumerate the formal type parameters - HENUMInternal hEnumGenericPars; - HRESULT hr = pInternalImport->EnumInit(mdtGenericParam, cl, &hEnumGenericPars); - if (FAILED(hr)) - pModule->GetAssembly()->ThrowTypeLoadException(pInternalImport, cl, IDS_CLASSLOAD_BADFORMAT); - - DWORD numGenericArgs = pInternalImport->EnumGetCount(&hEnumGenericPars); + DWORD numGenericArgs = pModule->m_pTypeGenericInfoMap->GetGenericArgumentCount(cl, pInternalImport); // Work out what kind of EEClass we're creating w.r.t. generics. If there // are no generics involved this will be a VMFLAG_NONGENERIC. @@ -12103,6 +12119,11 @@ MethodTableBuilder::GatherGenericsInfo( TypeHandle * pDestInst = (TypeHandle *)inst.GetRawArgs(); { + HENUMInternal hEnumGenericPars; + HRESULT hr = pInternalImport->EnumInit(mdtGenericParam, cl, &hEnumGenericPars); + if (FAILED(hr)) + pModule->GetAssembly()->ThrowTypeLoadException(pInternalImport, cl, IDS_CLASSLOAD_BADFORMAT); + // Protect multi-threaded access to Module.m_GenericParamToDescMap. Other threads may be loading the same type // to break type recursion dead-locks CrstHolder ch(&pModule->GetClassLoader()->m_AvailableTypesLock); diff --git a/src/coreclr/vm/readytoruninfo.cpp b/src/coreclr/vm/readytoruninfo.cpp index aad72b92ef1e22..033d73720ee356 100644 --- a/src/coreclr/vm/readytoruninfo.cpp +++ b/src/coreclr/vm/readytoruninfo.cpp @@ -185,7 +185,7 @@ BOOL ReadyToRunInfo::TryLookupTypeTokenFromName(const NameHandle *pName, mdToken mdToken mdFoundTypeEncloser; BOOL inputTypeHasEncloser = !pName->GetBucket().IsNull(); - BOOL foundTypeHasEncloser = GetEnclosingToken(m_pModule->GetMDImport(), cl, &mdFoundTypeEncloser); + BOOL foundTypeHasEncloser = GetEnclosingToken(m_pModule->GetMDImport(), m_pModule, cl, &mdFoundTypeEncloser); if (inputTypeHasEncloser != foundTypeHasEncloser) continue; @@ -194,14 +194,14 @@ BOOL ReadyToRunInfo::TryLookupTypeTokenFromName(const NameHandle *pName, mdToken { const HashedTypeEntry::TokenTypeEntry& tokenBasedEncloser = pName->GetBucket().GetTokenBasedEntryValue(); - if (!CompareTypeNameOfTokens(tokenBasedEncloser.m_TypeToken, tokenBasedEncloser.m_pModule->GetMDImport(), mdFoundTypeEncloser, m_pModule->GetMDImport())) + if (!CompareTypeNameOfTokens(tokenBasedEncloser.m_TypeToken, tokenBasedEncloser.m_pModule->GetMDImport(), tokenBasedEncloser.m_pModule, mdFoundTypeEncloser, m_pModule->GetMDImport(), m_pModule)) continue; } } else { // Compare type name, namespace name, and enclosing types chain for a match - if (!CompareTypeNameOfTokens(pName->GetTypeToken(), pName->GetTypeModule()->GetMDImport(), cl, m_pModule->GetMDImport())) + if (!CompareTypeNameOfTokens(pName->GetTypeToken(), pName->GetTypeModule()->GetMDImport(), pName->GetTypeModule(), cl, m_pModule->GetMDImport(), m_pModule)) continue; } @@ -238,7 +238,7 @@ BOOL ReadyToRunInfo::GetTypeNameFromToken(IMDInternalImport * pImport, mdToken m return FALSE; } -BOOL ReadyToRunInfo::GetEnclosingToken(IMDInternalImport * pImport, mdToken mdType, mdToken * pEnclosingToken) +BOOL ReadyToRunInfo::GetEnclosingToken(IMDInternalImport * pImport, ModuleBase* pModule, mdToken mdType, mdToken * pEnclosingToken) { CONTRACTL { @@ -252,7 +252,7 @@ BOOL ReadyToRunInfo::GetEnclosingToken(IMDInternalImport * pImport, mdToken mdTy switch (TypeFromToken(mdType)) { case mdtTypeDef: - return SUCCEEDED(pImport->GetNestedClassProps(mdType, pEnclosingToken)); + return SUCCEEDED(pModule->m_pEnclosingTypeMap->GetEnclosingTypeNoThrow(mdType, pEnclosingToken, pImport)); case mdtTypeRef: if (SUCCEEDED(pImport->GetResolutionScopeOfTypeRef(mdType, pEnclosingToken))) @@ -268,7 +268,7 @@ BOOL ReadyToRunInfo::GetEnclosingToken(IMDInternalImport * pImport, mdToken mdTy return FALSE; } -BOOL ReadyToRunInfo::CompareTypeNameOfTokens(mdToken mdToken1, IMDInternalImport * pImport1, mdToken mdToken2, IMDInternalImport * pImport2) +BOOL ReadyToRunInfo::CompareTypeNameOfTokens(mdToken mdToken1, IMDInternalImport * pImport1, ModuleBase *pModule1, mdToken mdToken2, IMDInternalImport * pImport2, ModuleBase *pModule2) { CONTRACTL { @@ -296,7 +296,7 @@ BOOL ReadyToRunInfo::CompareTypeNameOfTokens(mdToken mdToken1, IMDInternalImport if (strcmp(pszName1, pszName2) != 0 || strcmp(pszNameSpace1, pszNameSpace2) != 0) return FALSE; - if ((hasEncloser = GetEnclosingToken(pImport1, mdToken1, &mdToken1)) != GetEnclosingToken(pImport2, mdToken2, &mdToken2)) + if ((hasEncloser = GetEnclosingToken(pImport1, pModule1, mdToken1, &mdToken1)) != GetEnclosingToken(pImport2, pModule2, mdToken2, &mdToken2)) return FALSE; } while (hasEncloser); @@ -920,6 +920,24 @@ ReadyToRunInfo::ReadyToRunInfo(Module * pModule, LoaderAllocator* pLoaderAllocat m_attributesPresence = newFilter; } + + IMAGE_DATA_DIRECTORY *typeGenericInfoMap = m_component.FindSection(ReadyToRunSectionType::TypeGenericInfoMap); + if (typeGenericInfoMap != NULL) + { + pModule->m_pTypeGenericInfoMap = (ReadyToRun_TypeGenericInfoMap*)m_pComposite->GetImage()->GetDirectoryData(typeGenericInfoMap); + } + + IMAGE_DATA_DIRECTORY *enclosingTypeMap = m_component.FindSection(ReadyToRunSectionType::EnclosingTypeMap); + if (enclosingTypeMap != NULL) + { + pModule->m_pEnclosingTypeMap = (ReadyToRun_EnclosingTypeMap*)m_pComposite->GetImage()->GetDirectoryData(enclosingTypeMap); + } + + IMAGE_DATA_DIRECTORY *methodIsGenericMap = m_component.FindSection(ReadyToRunSectionType::MethodIsGenericMap); + if (methodIsGenericMap != NULL) + { + pModule->m_pMethodIsGenericMap = (ReadyToRun_MethodIsGenericMap*)m_pComposite->GetImage()->GetDirectoryData(methodIsGenericMap); + } } static bool SigMatchesMethodDesc(MethodDesc* pMD, SigPointer &sig, ModuleBase * pModule) @@ -1771,5 +1789,161 @@ ModuleBase* CreateNativeManifestModule(LoaderAllocator* pLoaderAllocator, IMDInt void *mem = pamTracker->Track(pLoaderAllocator->GetLowFrequencyHeap()->AllocMem(S_SIZE_T(sizeof(NativeManifestModule)))); return new (mem) NativeManifestModule(pLoaderAllocator, pManifestMetadata, pModule, pamTracker); } - #endif // DACCESS_COMPILE + +ReadyToRun_EnclosingTypeMap ReadyToRun_EnclosingTypeMap::EmptyInstance; +ReadyToRun_TypeGenericInfoMap ReadyToRun_TypeGenericInfoMap::EmptyInstance; +ReadyToRun_MethodIsGenericMap ReadyToRun_MethodIsGenericMap::EmptyInstance; + +mdTypeDef ReadyToRun_EnclosingTypeMap::GetEnclosingType(mdTypeDef input, IMDInternalImport* pImport) +{ + uint32_t rid = RidFromToken(input); + if ((rid > TypeCount) || (rid == 0)) + { + mdTypeDef enclosingType; + HRESULT hr = pImport->GetNestedClassProps(input, &enclosingType); + + if (FAILED(hr)) + { + if (hr == CLDB_E_RECORD_NOTFOUND) + return mdTypeDefNil; + ThrowHR(hr); + } + + return enclosingType; + } + return TokenFromRid((&TypeCount)[rid], mdtTypeDef); +} + +HRESULT ReadyToRun_EnclosingTypeMap::GetEnclosingTypeNoThrow(mdTypeDef input, mdTypeDef *pEnclosingType, IMDInternalImport* pImport) +{ + uint32_t rid = RidFromToken(input); + if ((rid > TypeCount) || (rid == 0)) + { + return pImport->GetNestedClassProps(input, pEnclosingType); + } + + *pEnclosingType = TokenFromRid((&TypeCount)[rid], mdtTypeDef); + + if (*pEnclosingType == mdTypeDefNil) + { + return CLDB_E_RECORD_NOTFOUND; + } + return S_OK; +} + +ReadyToRunTypeGenericInfo ReadyToRun_TypeGenericInfoMap::GetTypeGenericInfo(mdTypeDef input, bool *foundResult) +{ + uint32_t rid = RidFromToken(input); + if ((rid > TypeCount) || (rid == 0)) + { + *foundResult = false; + return (ReadyToRunTypeGenericInfo)0; + } + uint32_t index = rid - 1; + uint8_t *pTypeGenericInfo = ((uint8_t*)&TypeCount) + sizeof(uint32_t); + uint8_t entry = pTypeGenericInfo[index / 2]; + if (!(index & 1)) + { + entry >>= 4; + } + *foundResult = true; + return static_cast(entry); +} + +bool ReadyToRun_TypeGenericInfoMap::IsGeneric(mdTypeDef input, IMDInternalImport* pImport) +{ + bool foundResult; + ReadyToRunTypeGenericInfo typeGenericInfo = GetTypeGenericInfo(input, &foundResult); + if (!foundResult) + { + HENUMInternalHolder hEnumTyPars(pImport); + hEnumTyPars.EnumInit(mdtGenericParam, input); + return (pImport->EnumGetCount(&hEnumTyPars) != 0); + } + return !!((uint8_t)typeGenericInfo & (uint8_t)ReadyToRunTypeGenericInfo::GenericCountMask); +} + +HRESULT ReadyToRun_TypeGenericInfoMap::IsGenericNoThrow(mdTypeDef input, bool *pIsGeneric, IMDInternalImport* pImport) +{ + bool foundResult; + bool result; + HRESULT hr; + ReadyToRunTypeGenericInfo typeGenericInfo = GetTypeGenericInfo(input, &foundResult); + if (!foundResult) + { + HENUMInternalHolder hEnumTyPars(pImport); + IfFailRet(hEnumTyPars.EnumInitNoThrow(mdtGenericParam, input)); + result = (pImport->EnumGetCount(&hEnumTyPars) != 0); + } + else + result = !!((uint8_t)typeGenericInfo & (uint8_t)ReadyToRunTypeGenericInfo::GenericCountMask); + + *pIsGeneric = result; + return S_OK; +} + +uint32_t ReadyToRun_TypeGenericInfoMap::GetGenericArgumentCount(mdTypeDef input, IMDInternalImport* pImport) +{ + bool foundResult; + ReadyToRunTypeGenericInfo typeGenericInfo = GetTypeGenericInfo(input, &foundResult); + uint32_t count = ((uint8_t)typeGenericInfo & (uint8_t)ReadyToRunTypeGenericInfo::GenericCountMask); + if (count > 2) + foundResult = false; + + if (!foundResult) + { + HENUMInternalHolder hEnumTyPars(pImport); + hEnumTyPars.EnumInit(mdtGenericParam, input); + return pImport->EnumGetCount(&hEnumTyPars); + } + return count; +} + +HRESULT ReadyToRun_TypeGenericInfoMap::GetGenericArgumentCountNoThrow(mdTypeDef input, uint32_t *pCount, IMDInternalImport* pImport) +{ + HRESULT hr; + bool foundResult; + ReadyToRunTypeGenericInfo typeGenericInfo = GetTypeGenericInfo(input, &foundResult); + uint32_t count = ((uint8_t)typeGenericInfo & (uint8_t)ReadyToRunTypeGenericInfo::GenericCountMask); + if (count > 2) + foundResult = false; + + if (!foundResult) + { + HENUMInternalHolder hEnumTyPars(pImport); + IfFailRet(hEnumTyPars.EnumInitNoThrow(mdtGenericParam, input)); + count = pImport->EnumGetCount(&hEnumTyPars); + } + + hr = S_OK; + *pCount = count; + return hr; +} + +bool ReadyToRun_TypeGenericInfoMap::HasVariance(mdTypeDef input, bool *foundResult) +{ + ReadyToRunTypeGenericInfo typeGenericInfo = GetTypeGenericInfo(input, foundResult); + return !!((uint8_t)typeGenericInfo & (uint8_t)ReadyToRunTypeGenericInfo::HasVariance); +} + +bool ReadyToRun_TypeGenericInfoMap::HasConstraints(mdTypeDef input, bool *foundResult) +{ + ReadyToRunTypeGenericInfo typeGenericInfo = GetTypeGenericInfo(input, foundResult); + return !!((uint8_t)typeGenericInfo & (uint8_t)ReadyToRunTypeGenericInfo::HasVariance); +} + +bool ReadyToRun_MethodIsGenericMap::IsGeneric(mdMethodDef input, bool *foundResult) +{ + uint32_t rid = RidFromToken(input); + if ((rid > MethodCount) || (rid == 0)) + { + *foundResult = false; + return 0; + } + + uint8_t chunk = ((uint8_t*)&MethodCount)[((rid - 1) / 8) + 4]; + chunk >>= 7 - ((rid - 1) % 8); + return !!(chunk & 1); +} + diff --git a/src/coreclr/vm/readytoruninfo.h b/src/coreclr/vm/readytoruninfo.h index d3d4b760ac1155..d1af0fdeb5dd25 100644 --- a/src/coreclr/vm/readytoruninfo.h +++ b/src/coreclr/vm/readytoruninfo.h @@ -48,6 +48,42 @@ class ReadyToRunCoreInfo typedef DPTR(class ReadyToRunInfo) PTR_ReadyToRunInfo; typedef DPTR(class ReadyToRunCoreInfo) PTR_ReadyToRunCoreInfo; + +class ReadyToRun_EnclosingTypeMap +{ +private: + uint16_t TypeCount = 0; +public: + static ReadyToRun_EnclosingTypeMap EmptyInstance; + + mdTypeDef GetEnclosingType(mdTypeDef input, IMDInternalImport* pImport); + HRESULT GetEnclosingTypeNoThrow(mdTypeDef input, mdTypeDef *pEnclosingType, IMDInternalImport* pImport); +}; + +class ReadyToRun_TypeGenericInfoMap +{ +private: + uint32_t TypeCount = 0; + ReadyToRunTypeGenericInfo GetTypeGenericInfo(mdTypeDef input, bool *foundResult); +public: + static ReadyToRun_TypeGenericInfoMap EmptyInstance; + + HRESULT IsGenericNoThrow(mdTypeDef input, bool *pIsGeneric, IMDInternalImport* pImport); + HRESULT GetGenericArgumentCountNoThrow(mdTypeDef input, uint32_t *pCount, IMDInternalImport* pImport); + bool IsGeneric(mdTypeDef input, IMDInternalImport* pImport); + uint32_t GetGenericArgumentCount(mdTypeDef input, IMDInternalImport* pImport); + bool HasVariance(mdTypeDef input, bool *foundResult); + bool HasConstraints(mdTypeDef input, bool *foundResult); +}; + +class ReadyToRun_MethodIsGenericMap +{ + uint32_t MethodCount = 0; +public: + static ReadyToRun_MethodIsGenericMap EmptyInstance; + bool IsGeneric(mdMethodDef input, bool *foundResult); +}; + class ReadyToRunInfo { friend class ReadyToRunJitManager; @@ -283,8 +319,8 @@ class ReadyToRunInfo BOOL IsImageVersionAtLeast(int majorVersion, int minorVersion); private: BOOL GetTypeNameFromToken(IMDInternalImport * pImport, mdToken mdType, LPCUTF8 * ppszName, LPCUTF8 * ppszNameSpace); - BOOL GetEnclosingToken(IMDInternalImport * pImport, mdToken mdType, mdToken * pEnclosingToken); - BOOL CompareTypeNameOfTokens(mdToken mdToken1, IMDInternalImport * pImport1, mdToken mdToken2, IMDInternalImport * pImport2); + BOOL GetEnclosingToken(IMDInternalImport * pImport, ModuleBase *pModule1, mdToken mdType, mdToken * pEnclosingToken); + BOOL CompareTypeNameOfTokens(mdToken mdToken1, IMDInternalImport * pImport1, ModuleBase *pModule1, mdToken mdToken2, IMDInternalImport * pImport2, ModuleBase *pModule2); PTR_MethodDesc GetMethodDescForEntryPointInNativeImage(PCODE entryPoint); void SetMethodDescForEntryPointInNativeImage(PCODE entryPoint, PTR_MethodDesc methodDesc); diff --git a/src/coreclr/vm/siginfo.cpp b/src/coreclr/vm/siginfo.cpp index 23185646df61a4..c9c72f785ab362 100644 --- a/src/coreclr/vm/siginfo.cpp +++ b/src/coreclr/vm/siginfo.cpp @@ -3065,7 +3065,7 @@ BOOL IsTypeDefExternallyVisible(mdToken tk, Module *pModule, DWORD dwAttrClass) { mdTypeDef tdEnclosing = mdTypeDefNil; - if (FAILED(pInternalImport->GetNestedClassProps(tdCurrent, &tdEnclosing))) + if (FAILED(pModule->m_pEnclosingTypeMap->GetEnclosingTypeNoThrow(tdCurrent, &tdEnclosing, pInternalImport))) return FALSE; tdCurrent = tdEnclosing; @@ -3186,9 +3186,7 @@ BOOL IsTypeDefEquivalent(mdToken tk, Module *pModule) (CompareTypeTokens(tkExtends, tdValueType, pModule, pSystemModule) && (tk != tdEnum || pModule != pSystemModule)) || (CompareTypeTokens(tkExtends, tdMCDelegate, pModule, pSystemModule))))) { - HENUMInternal hEnumGenericPars; - IfFailThrow(pInternalImport->EnumInit(mdtGenericParam, tk, &hEnumGenericPars)); - DWORD numGenericArgs = pInternalImport->EnumGetCount(&hEnumGenericPars); + DWORD numGenericArgs = pModule->m_pTypeGenericInfoMap->GetGenericArgumentCount(tk, pInternalImport); // 3. Type is not generic if (numGenericArgs > 0) @@ -3208,7 +3206,7 @@ BOOL IsTypeDefEquivalent(mdToken tk, Module *pModule) { mdTypeDef tdEnclosing = mdTypeDefNil; - IfFailThrow(pInternalImport->GetNestedClassProps(tk, &tdEnclosing)); + tdEnclosing = pModule->m_pEnclosingTypeMap->GetEnclosingType(tk, pInternalImport); if (!IsTypeDefEquivalent(tdEnclosing, pModule)) return FALSE; @@ -3317,8 +3315,8 @@ BOOL CompareTypeDefsForEquivalence(mdToken tk1, mdToken tk2, Module *pModule1, M mdToken tkEnclosing1; mdToken tkEnclosing2; - IfFailThrow(pInternalImport1->GetNestedClassProps(tk1, &tkEnclosing1)); - IfFailThrow(pInternalImport2->GetNestedClassProps(tk2, &tkEnclosing2)); + tkEnclosing1 = pModule1->m_pEnclosingTypeMap->GetEnclosingType(tk1, pInternalImport1); + tkEnclosing2 = pModule2->m_pEnclosingTypeMap->GetEnclosingType(tk2, pInternalImport2); if (!CompareTypeDefsForEquivalence(tkEnclosing1, tkEnclosing2, pModule1, pModule2, pVisited)) { @@ -3505,7 +3503,7 @@ BOOL CompareTypeTokens(mdToken tk1, mdToken tk2, ModuleBase *pModule1, ModuleBas } else { - if (FAILED(hr = pInternalImport1->GetNestedClassProps(tk1, &enclosingTypeTk1))) + if (FAILED(hr = pModule1->m_pEnclosingTypeMap->GetEnclosingTypeNoThrow(tk1, &enclosingTypeTk1, pInternalImport1))) { if (hr != CLDB_E_RECORD_NOTFOUND) { @@ -3526,7 +3524,8 @@ BOOL CompareTypeTokens(mdToken tk1, mdToken tk2, ModuleBase *pModule1, ModuleBas } else { - if (FAILED(hr = pInternalImport2->GetNestedClassProps(tk2, &enclosingTypeTk2))) + _ASSERTE(pModule1->IsFullModule()); + if (FAILED(hr = pModule2->m_pEnclosingTypeMap->GetEnclosingTypeNoThrow(tk2, &enclosingTypeTk2, pInternalImport2))) { if (hr != CLDB_E_RECORD_NOTFOUND) { @@ -4851,9 +4850,16 @@ BOOL MetaSig::CompareMethodConstraints(const Substitution *pSubst1, } CONTRACTL_END + IMDInternalImport *pInternalImport1 = pModule1->GetMDImport(); IMDInternalImport *pInternalImport2 = pModule2->GetMDImport(); + if (!pModule1->m_pTypeGenericInfoMap->IsGeneric(tok1, pInternalImport1)) + { + // The method's generic-ness must match. + return !pModule1->m_pTypeGenericInfoMap->IsGeneric(tok1, pInternalImport1); + } + HENUMInternalHolder hEnumTyPars1(pInternalImport1); HENUMInternalHolder hEnumTyPars2(pInternalImport2); diff --git a/src/coreclr/vm/typedesc.cpp b/src/coreclr/vm/typedesc.cpp index db045069c2691f..6590ada3215aa4 100644 --- a/src/coreclr/vm/typedesc.cpp +++ b/src/coreclr/vm/typedesc.cpp @@ -824,6 +824,14 @@ void TypeVarTypeDesc::LoadConstraints(ClassLoadLevel level /* = CLASS_LOADED */) else { _ASSERTE(TypeFromToken(defToken) == mdtTypeDef); + + bool foundResult = false; + if (!GetModule()->m_pTypeGenericInfoMap->HasConstraints(defToken, &foundResult) && foundResult) + { + m_numConstraints = 0; + return; + } + TypeHandle genericType = LoadOwnerType(); _ASSERTE(genericType.IsGenericTypeDefinition()); From 0c889829dd6aeebb48f2d8197de63ca3091611ad Mon Sep 17 00:00:00 2001 From: David Wrighton Date: Fri, 21 Apr 2023 15:16:17 -0700 Subject: [PATCH 03/23] Store index of MethodDesc as well as ChunkIndex. Makes MethodDesc::GetTemporaryEntryPoint *much* faster --- src/coreclr/vm/method.cpp | 55 ++------------ src/coreclr/vm/method.hpp | 103 ++++++++++++-------------- src/coreclr/vm/methodtablebuilder.cpp | 2 + 3 files changed, 58 insertions(+), 102 deletions(-) diff --git a/src/coreclr/vm/method.cpp b/src/coreclr/vm/method.cpp index c09056a0121fcc..ac0cae40b13db5 100644 --- a/src/coreclr/vm/method.cpp +++ b/src/coreclr/vm/method.cpp @@ -1720,6 +1720,7 @@ MethodDescChunk *MethodDescChunk::CreateChunk(LoaderHeap *pHeap, DWORD methodDes for (DWORD i = 0; i < count; i++) { pMD->SetChunkIndex(pChunk); + pMD->SetMethodDescIndex(i); pMD->SetClassification(classification); if (fNonVtableSlot) @@ -2204,7 +2205,7 @@ void MethodDesc::Reset() // We should go here only for the rental methods _ASSERTE(GetLoaderModule()->IsReflection()); - InterlockedUpdateFlags2(enum_flag2_HasStableEntryPoint | enum_flag2_HasPrecode, FALSE); + InterlockedUpdateFlags3(enum_flag3_HasStableEntryPoint | enum_flag3_HasPrecode, FALSE); *GetAddrOfSlot() = GetTemporaryEntryPoint(); } @@ -2774,7 +2775,7 @@ TADDR MethodDescChunk::AllocateCompactEntryPoints(LoaderAllocator *pLoaderAlloca } *(p+0) = X86_INSTR_MOV_AL; - int methodDescIndex = pMD->GetMethodDescIndex() - pBaseMD->GetMethodDescIndex(); + int methodDescIndex = pMD->GetMethodDescChunkIndex() - pBaseMD->GetMethodDescChunkIndex(); _ASSERTE(FitsInU1(methodDescIndex)); *(p+1) = (BYTE)methodDescIndex; @@ -2867,28 +2868,7 @@ PCODE MethodDesc::GetTemporaryEntryPoint() CONTRACTL_END; MethodDescChunk* pChunk = GetMethodDescChunk(); - int lo = 0, hi = pChunk->GetCount() - 1; - - // Find the temporary entrypoint in the chunk by binary search - while (lo < hi) - { - int mid = (lo + hi) / 2; - - TADDR pEntryPoint = pChunk->GetTemporaryEntryPoint(mid); - - MethodDesc * pMD = MethodDesc::GetMethodDescFromStubAddr(pEntryPoint); - if (PTR_HOST_TO_TADDR(this) == PTR_HOST_TO_TADDR(pMD)) - return pEntryPoint; - - if (PTR_HOST_TO_TADDR(this) > PTR_HOST_TO_TADDR(pMD)) - lo = mid + 1; - else - hi = mid - 1; - } - - _ASSERTE(lo == hi); - - TADDR pEntryPoint = pChunk->GetTemporaryEntryPoint(lo); + TADDR pEntryPoint = pChunk->GetTemporaryEntryPoint(GetMethodDescIndex()); #ifdef _DEBUG MethodDesc * pMD = MethodDesc::GetMethodDescFromStubAddr(pEntryPoint); @@ -2940,27 +2920,6 @@ void MethodDescChunk::CreateTemporaryEntryPoints(LoaderAllocator *pLoaderAllocat _ASSERTE(GetTemporaryEntryPoints() != NULL); } -//******************************************************************************* -void MethodDesc::InterlockedUpdateFlags2(BYTE bMask, BOOL fSet) -{ - WRAPPER_NO_CONTRACT; - - LONG* pLong = (LONG*)(&m_bFlags2 - 3); - static_assert_no_msg(offsetof(MethodDesc, m_bFlags2) % sizeof(LONG) == 3); - -#if BIGENDIAN - if (fSet) - InterlockedOr(pLong, (ULONG)bMask); - else - InterlockedAnd(pLong, ~(ULONG)bMask); -#else // !BIGENDIAN - if (fSet) - InterlockedOr(pLong, (LONG)bMask << (3 * 8)); - else - InterlockedAnd(pLong, ~((LONG)bMask << (3 * 8))); -#endif // !BIGENDIAN -} - //******************************************************************************* Precode* MethodDesc::GetOrCreatePrecode() { @@ -2998,7 +2957,7 @@ Precode* MethodDesc::GetOrCreatePrecode() } // Set the flags atomically - InterlockedUpdateFlags2(enum_flag2_HasStableEntryPoint | enum_flag2_HasPrecode, TRUE); + InterlockedUpdateFlags3(enum_flag3_HasStableEntryPoint | enum_flag3_HasPrecode, TRUE); return Precode::GetPrecodeFromEntryPoint(*pSlot); } @@ -3038,7 +2997,7 @@ bool MethodDesc::DetermineAndSetIsEligibleForTieredCompilation() // Policy - Tiered compilation is not disabled by the profiler !CORProfilerDisableTieredCompilation()) { - m_bFlags2 |= enum_flag2_IsEligibleForTieredCompilation; + m_wFlags3AndTokenRemainder |= enum_flag3_IsEligibleForTieredCompilation; _ASSERTE(IsVersionable()); return true; } @@ -3323,7 +3282,7 @@ BOOL MethodDesc::SetStableEntryPointInterlocked(PCODE addr) BOOL fResult = InterlockedCompareExchangeT(pSlot, addr, pExpected) == pExpected; - InterlockedUpdateFlags2(enum_flag2_HasStableEntryPoint, TRUE); + InterlockedUpdateFlags3(enum_flag3_HasStableEntryPoint, TRUE); return fResult; } diff --git a/src/coreclr/vm/method.hpp b/src/coreclr/vm/method.hpp index d27a7eb0e9d533..6502ffe64f87f4 100644 --- a/src/coreclr/vm/method.hpp +++ b/src/coreclr/vm/method.hpp @@ -51,7 +51,7 @@ GVAL_DECL(TADDR, g_MiniMetaDataBuffAddress); EXTERN_C VOID STDCALL NDirectImportThunk(); -#define METHOD_TOKEN_REMAINDER_BIT_COUNT 13 +#define METHOD_TOKEN_REMAINDER_BIT_COUNT 12 #define METHOD_TOKEN_REMAINDER_MASK ((1 << METHOD_TOKEN_REMAINDER_BIT_COUNT) - 1) #define METHOD_TOKEN_RANGE_BIT_COUNT (24 - METHOD_TOKEN_REMAINDER_BIT_COUNT) #define METHOD_TOKEN_RANGE_MASK ((1 << METHOD_TOKEN_RANGE_BIT_COUNT) - 1) @@ -144,19 +144,20 @@ enum MethodDescClassification // Method is static mdcStatic = 0x0080, - // unused = 0x0100, - // unused = 0x0200, + mdcValueTypeParametersWalked = 0x0100, // Indicates that all typeref's in the signature of the method have been resolved + // to typedefs (or that process failed). + + mdcValueTypeParametersLoaded = 0x0200, // Indicates if the valuetype parameter types have been loaded. // Duplicate method. When a method needs to be placed in multiple slots in the // method table, because it could not be packed into one slot. For eg, a method // providing implementation for two interfaces, MethodImpl, etc mdcDuplicate = 0x0400, - // Has this method been verified? - mdcVerifiedState = 0x0800, + mdcDoesNotHaveEquivalentValuetypeParameters = 0x0800, // Indicates that we have verified that there are no equivalent valuetype parameters + // for this method. - // Is the method verifiable? It needs to be verified first to determine this - mdcVerifiable = 0x1000, + mdcRequiresCovariantReturnTypeChecking = 0x1000, // Is this method ineligible for inlining? mdcNotInline = 0x2000, @@ -164,7 +165,7 @@ enum MethodDescClassification // Is the method synchronized mdcSynchronized = 0x4000, - // unused = 0x8000 + mdcIsIntrinsic = 0x8000 // Jit may expand method as an intrinsic }; #define METHOD_MAX_RVA 0x7FFFFFFF @@ -228,7 +229,7 @@ class MethodDesc { LIMITED_METHOD_DAC_CONTRACT; - return (m_bFlags2 & enum_flag2_HasStableEntryPoint) != 0; + return (m_wFlags3AndTokenRemainder & enum_flag3_HasStableEntryPoint) != 0; } inline PCODE GetStableEntryPoint() @@ -262,7 +263,7 @@ class MethodDesc { LIMITED_METHOD_DAC_CONTRACT; - return (m_bFlags2 & enum_flag2_HasPrecode) != 0; + return (m_wFlags3AndTokenRemainder & enum_flag3_HasPrecode) != 0; } inline Precode* GetPrecode() @@ -298,8 +299,6 @@ class MethodDesc return result; } - void InterlockedUpdateFlags2(BYTE bMask, BOOL fSet); - Precode* GetOrCreatePrecode(); // Given a code address return back the MethodDesc whenever possible @@ -760,13 +759,13 @@ class MethodDesc { LIMITED_METHOD_DAC_CONTRACT; - return (m_bFlags2 & enum_flag2_IsUnboxingStub) != 0; + return (m_wFlags3AndTokenRemainder & enum_flag3_IsUnboxingStub) != 0; } void SetIsUnboxingStub() { LIMITED_METHOD_CONTRACT; - m_bFlags2 |= enum_flag2_IsUnboxingStub; + m_wFlags3AndTokenRemainder |= enum_flag3_IsUnboxingStub; } @@ -994,7 +993,7 @@ class MethodDesc public: inline MethodDescChunk* GetMethodDescChunk() const; - inline int GetMethodDescIndex() const; + inline int GetMethodDescChunkIndex() const; // If this is an method desc. (whether non-generic shared-instantiated or exact-instantiated) // inside a shared class then get the method table for the representative // class. @@ -1123,7 +1122,7 @@ class MethodDesc LIMITED_METHOD_DAC_CONTRACT; #ifdef FEATURE_TIERED_COMPILATION - return (m_bFlags2 & enum_flag2_IsEligibleForTieredCompilation) != 0; + return (m_wFlags3AndTokenRemainder & enum_flag3_IsEligibleForTieredCompilation) != 0; #else return false; #endif @@ -1616,36 +1615,21 @@ class MethodDesc protected: enum { - // There are flags available for use here (currently 5 flags bits are available); however, new bits are hard to come by, so any new flags bits should + // There are flags available for use here (currently 4 flags bits are available); however, new bits are hard to come by, so any new flags bits should // have a fairly strong justification for existence. - enum_flag3_TokenRemainderMask = 0x1FFF, // This must equal METHOD_TOKEN_REMAINDER_MASK calculated higher in this file. - enum_flag3_ValueTypeParametersWalked = 0x2000, // Indicates that all typeref's in the signature of the method have been resolved - // to typedefs (or that process failed). - enum_flag3_ValueTypeParametersLoaded = 0x4000, // Indicates if the valuetype parameter types have been loaded. - enum_flag3_DoesNotHaveEquivalentValuetypeParameters = 0x8000, // Indicates that we have verified that there are no equivalent valuetype parameters + enum_flag3_TokenRemainderMask = 0x0FFF, // This must equal METHOD_TOKEN_REMAINDER_MASK calculated higher in this file. // for this method. + // enum_flag3_HasPrecode implies that enum_flag3_HasStableEntryPoint is set. + enum_flag3_HasStableEntryPoint = 0x1000, // The method entrypoint is stable (either precode or actual code) + enum_flag3_HasPrecode = 0x2000, // Precode has been allocated for this method + + enum_flag3_IsUnboxingStub = 0x4000, + enum_flag3_IsEligibleForTieredCompilation = 0x8000, }; UINT16 m_wFlags3AndTokenRemainder; BYTE m_chunkIndex; - - enum { - // enum_flag2_HasPrecode implies that enum_flag2_HasStableEntryPoint is set. - enum_flag2_HasStableEntryPoint = 0x01, // The method entrypoint is stable (either precode or actual code) - enum_flag2_HasPrecode = 0x02, // Precode has been allocated for this method - - enum_flag2_IsUnboxingStub = 0x04, - // unused = 0x08, - - enum_flag2_IsIntrinsic = 0x10, // Jit may expand method as an intrinsic - - enum_flag2_IsEligibleForTieredCompilation = 0x20, - - enum_flag2_RequiresCovariantReturnTypeChecking = 0x40 - - // unused = 0x80, - }; - BYTE m_bFlags2; + BYTE m_methodIndex; // Used to hold the index into the chunk of this MethodDesc. Currently all 8 bits are used, but we could likely work with only 7 bits // The slot number of this MethodDesc in the vtable array. WORD m_wSlotNumber; @@ -1656,6 +1640,17 @@ class MethodDesc void EnumMemoryRegions(CLRDataEnumMemoryFlags flags); #endif + BYTE GetMethodDescIndex() + { + return m_methodIndex; + } + + void SetMethodDescIndex(COUNT_T index) + { + _ASSERTE(index <= 255); + m_methodIndex = (BYTE)index; + } + public: inline DWORD GetClassification() const { @@ -1706,26 +1701,26 @@ class MethodDesc inline BOOL IsIntrinsic() { LIMITED_METHOD_DAC_CONTRACT; - return (m_bFlags2 & enum_flag2_IsIntrinsic) != 0; + return (m_wFlags & mdcIsIntrinsic) != 0; } inline void SetIsIntrinsic() { LIMITED_METHOD_CONTRACT; - m_bFlags2 |= enum_flag2_IsIntrinsic; + m_wFlags |= mdcIsIntrinsic; } BOOL RequiresCovariantReturnTypeChecking() { LIMITED_METHOD_DAC_CONTRACT; - return (m_bFlags2 & enum_flag2_RequiresCovariantReturnTypeChecking) != 0; + return (m_wFlags & mdcRequiresCovariantReturnTypeChecking) != 0; } void SetRequiresCovariantReturnTypeChecking() { LIMITED_METHOD_CONTRACT; - m_bFlags2 |= enum_flag2_RequiresCovariantReturnTypeChecking; + m_wFlags |= mdcRequiresCovariantReturnTypeChecking; } static const BYTE s_ClassificationSizeTable[]; @@ -1750,38 +1745,38 @@ class MethodDesc inline BOOL HaveValueTypeParametersBeenWalked() { LIMITED_METHOD_DAC_CONTRACT; - return (m_wFlags3AndTokenRemainder & enum_flag3_ValueTypeParametersWalked) != 0; + return (m_wFlags & mdcValueTypeParametersWalked) != 0; } inline void SetValueTypeParametersWalked() { LIMITED_METHOD_CONTRACT; - InterlockedUpdateFlags3(enum_flag3_ValueTypeParametersWalked, TRUE); + InterlockedUpdateFlags(mdcValueTypeParametersWalked, TRUE); } inline BOOL HaveValueTypeParametersBeenLoaded() { LIMITED_METHOD_DAC_CONTRACT; - return (m_wFlags3AndTokenRemainder & enum_flag3_ValueTypeParametersLoaded) != 0; + return (m_wFlags & mdcValueTypeParametersLoaded) != 0; } inline void SetValueTypeParametersLoaded() { LIMITED_METHOD_CONTRACT; - InterlockedUpdateFlags3(enum_flag3_ValueTypeParametersLoaded, TRUE); + InterlockedUpdateFlags(mdcValueTypeParametersLoaded, TRUE); } #ifdef FEATURE_TYPEEQUIVALENCE inline BOOL DoesNotHaveEquivalentValuetypeParameters() { LIMITED_METHOD_DAC_CONTRACT; - return (m_wFlags3AndTokenRemainder & enum_flag3_DoesNotHaveEquivalentValuetypeParameters) != 0; + return (m_wFlags & mdcDoesNotHaveEquivalentValuetypeParameters) != 0; } inline void SetDoesNotHaveEquivalentValuetypeParameters() { LIMITED_METHOD_CONTRACT; - InterlockedUpdateFlags3(enum_flag3_DoesNotHaveEquivalentValuetypeParameters, TRUE); + InterlockedUpdateFlags(mdcDoesNotHaveEquivalentValuetypeParameters, TRUE); } #endif // FEATURE_TYPEEQUIVALENCE @@ -2126,7 +2121,7 @@ class MethodDescChunk friend class CheckAsmOffsets; enum { - enum_flag_TokenRangeMask = 0x07FF, // This must equal METHOD_TOKEN_RANGE_MASK calculated higher in this file + enum_flag_TokenRangeMask = 0x0FFF, // This must equal METHOD_TOKEN_RANGE_MASK calculated higher in this file // These are separate to allow the flags space available and used to be obvious here // and for the logic that splits the token to be algorithmically generated based on the // #define @@ -2311,7 +2306,7 @@ class MethodDescChunk // Followed by array of method descs... }; -inline int MethodDesc::GetMethodDescIndex() const +inline int MethodDesc::GetMethodDescChunkIndex() const { LIMITED_METHOD_DAC_CONTRACT; @@ -2324,7 +2319,7 @@ inline MethodDescChunk *MethodDesc::GetMethodDescChunk() const return PTR_MethodDescChunk(dac_cast(this) - - (sizeof(MethodDescChunk) + (GetMethodDescIndex() * MethodDesc::ALIGNMENT))); + (sizeof(MethodDescChunk) + (GetMethodDescChunkIndex() * MethodDesc::ALIGNMENT))); } MethodDesc* NonVirtualEntry2MethodDesc(PCODE entryPoint); @@ -3492,7 +3487,7 @@ inline void MethodDesc::SetMemberDef(mdMethodDef mb) _ASSERTE((tokremainder & ~enum_flag3_TokenRemainderMask) == 0); m_wFlags3AndTokenRemainder = (m_wFlags3AndTokenRemainder & ~enum_flag3_TokenRemainderMask) | tokremainder; - if (GetMethodDescIndex() == 0) + if (GetMethodDescChunkIndex() == 0) { GetMethodDescChunk()->SetTokenRange(tokrange); } diff --git a/src/coreclr/vm/methodtablebuilder.cpp b/src/coreclr/vm/methodtablebuilder.cpp index ecddcd2ddee979..ec1f2690a127e5 100644 --- a/src/coreclr/vm/methodtablebuilder.cpp +++ b/src/coreclr/vm/methodtablebuilder.cpp @@ -7064,6 +7064,7 @@ VOID MethodTableBuilder::AllocAndInitMethodDescChunk(COUNT_T startIndex, COUNT_T MethodDesc * pMD = (MethodDesc *)((BYTE *)pChunk + offset); pMD->SetChunkIndex(pChunk); + pMD->SetMethodDescIndex(methodDescCount); InitNewMethodDesc(pMDMethod, pMD); @@ -7107,6 +7108,7 @@ VOID MethodTableBuilder::AllocAndInitMethodDescChunk(COUNT_T startIndex, COUNT_T // Reset the chunk index pUnboxedMD->SetChunkIndex(pChunk); + pUnboxedMD->SetMethodDescIndex(methodDescCount); if (bmtGenerics->GetNumGenericArgs() == 0) { pUnboxedMD->SetHasNonVtableSlot(); From 910f4c4f76fb31fc15000ccef3ff8604976f151b Mon Sep 17 00:00:00 2001 From: David Wrighton Date: Fri, 21 Apr 2023 17:38:38 -0700 Subject: [PATCH 04/23] Optimize the path for computing if a method is eligible for tiered compilation --- src/coreclr/vm/codeversion.cpp | 5 +-- src/coreclr/vm/codeversion.h | 2 +- src/coreclr/vm/method.cpp | 62 +++++++++++++++++++++++++--------- src/coreclr/vm/method.hpp | 7 +++- src/coreclr/vm/precode.cpp | 3 +- 5 files changed, 58 insertions(+), 21 deletions(-) diff --git a/src/coreclr/vm/codeversion.cpp b/src/coreclr/vm/codeversion.cpp index 6a5db21db37f69..00ed3ef841568c 100644 --- a/src/coreclr/vm/codeversion.cpp +++ b/src/coreclr/vm/codeversion.cpp @@ -2089,10 +2089,11 @@ void CodeVersionManager::OnAppDomainExit(AppDomain * pAppDomain) // get the final determination of versioning support for a given method. // //static -bool CodeVersionManager::IsMethodSupported(PTR_MethodDesc pMethodDesc) +bool CodeVersionManager::IsMethodSupported(MethodDescChunk* pMethodDescChunk) { WRAPPER_NO_CONTRACT; - _ASSERTE(pMethodDesc != NULL); + _ASSERTE(pMethodDescChunk != NULL); + MethodDesc* pMethodDesc = pMethodDescChunk->GetFirstMethodDesc(); return // CodeVersionManager data structures don't properly handle the lifetime semantics of dynamic code at this point diff --git a/src/coreclr/vm/codeversion.h b/src/coreclr/vm/codeversion.h index 66de4ba27257a4..da65ff5b73f69a 100644 --- a/src/coreclr/vm/codeversion.h +++ b/src/coreclr/vm/codeversion.h @@ -608,7 +608,7 @@ class CodeVersionManager static void OnAppDomainExit(AppDomain* pAppDomain); #endif - static bool IsMethodSupported(PTR_MethodDesc pMethodDesc); + static bool IsMethodSupported(MethodDescChunk* pMethodDescChunk); #ifndef DACCESS_COMPILE static bool InitialNativeCodeVersionMayNotBeTheDefaultNativeCodeVersion() diff --git a/src/coreclr/vm/method.cpp b/src/coreclr/vm/method.cpp index ac0cae40b13db5..58fee8dc0d6ab9 100644 --- a/src/coreclr/vm/method.cpp +++ b/src/coreclr/vm/method.cpp @@ -2962,6 +2962,33 @@ Precode* MethodDesc::GetOrCreatePrecode() return Precode::GetPrecodeFromEntryPoint(*pSlot); } +bool MethodDescChunk::DetermineIsEligibleForTieredCompilation() +{ +#ifdef FEATURE_TIERED_COMPILATION +#ifndef FEATURE_CODE_VERSIONING + #error Tiered compilation requires code versioning +#endif + return + // Policy + g_pConfig->TieredCompilation() && + + // Functional requirement + CodeVersionManager::IsMethodSupported(this) && + + // Policy - If QuickJit is disabled and the module does not have any pregenerated code, the method would effectively not + // be tiered currently, so make the method ineligible for tiering to avoid some unnecessary overhead + (g_pConfig->TieredCompilation_QuickJit() || GetMethodTable()->GetModule()->IsReadyToRun()) && + + // Policy - Tiered compilation is not disabled by the profiler + !CORProfilerDisableTieredCompilation() && + + // Policy - Generating optimized code is not disabled + !GetFirstMethodDesc()->IsJitOptimizationDisabledForAllMethodsInChunk(); +#else + return false; +#endif +} + bool MethodDesc::DetermineAndSetIsEligibleForTieredCompilation() { WRAPPER_NO_CONTRACT; @@ -2971,12 +2998,13 @@ bool MethodDesc::DetermineAndSetIsEligibleForTieredCompilation() #error Tiered compilation requires code versioning #endif + // This function should only be called if the chunk has already been checked. This is done + // to reduce the amount of flags checked for each MethodDesc + _ASSERTE(GetMethodDescChunk()->DetermineIsEligibleForTieredCompilation()); + // Keep in-sync with MethodTableBuilder::NeedsNativeCodeSlot(bmtMDMethod * pMDMethod) // to ensure native slots are available where needed. if ( - // Policy - g_pConfig->TieredCompilation() && - // Functional requirement - The NativeCodeSlot is required to hold the code pointer for the default code version because // the method's entry point slot will point to a precode or to the current code entry point HasNativeCodeSlot() && @@ -2984,18 +3012,8 @@ bool MethodDesc::DetermineAndSetIsEligibleForTieredCompilation() // Functional requirement - These methods have no IL that could be optimized !IsWrapperStub() && - // Functional requirement - CodeVersionManager::IsMethodSupported(this) && - - // Policy - If QuickJit is disabled and the module does not have any pregenerated code, the method would effectively not - // be tiered currently, so make the method ineligible for tiering to avoid some unnecessary overhead - (g_pConfig->TieredCompilation_QuickJit() || GetModule()->IsReadyToRun()) && - // Policy - Generating optimized code is not disabled - !IsJitOptimizationDisabled() && - - // Policy - Tiered compilation is not disabled by the profiler - !CORProfilerDisableTieredCompilation()) + !IsJitOptimizationDisabledForSpecificMethod()) { m_wFlags3AndTokenRemainder |= enum_flag3_IsEligibleForTieredCompilation; _ASSERTE(IsVersionable()); @@ -3012,13 +3030,25 @@ bool MethodDesc::IsJitOptimizationDisabled() { WRAPPER_NO_CONTRACT; + return IsJitOptimizationDisabledForAllMethodsInChunk() || + IsJitOptimizationDisabledForSpecificMethod(); +} + +bool MethodDesc::IsJitOptimizationDisabledForSpecificMethod() +{ + return (!IsNoMetadata() && IsMiNoOptimization(GetImplAttrs())); +} + +bool MethodDesc::IsJitOptimizationDisabledForAllMethodsInChunk() +{ + WRAPPER_NO_CONTRACT; + return g_pConfig->JitMinOpts() || #ifdef _DEBUG g_pConfig->GenDebuggableCode() || #endif - CORDisableJITOptimizations(GetModule()->GetDebuggerInfoBits()) || - (!IsNoMetadata() && IsMiNoOptimization(GetImplAttrs())); + CORDisableJITOptimizations(GetModule()->GetDebuggerInfoBits()); } #ifndef DACCESS_COMPILE diff --git a/src/coreclr/vm/method.hpp b/src/coreclr/vm/method.hpp index 6502ffe64f87f4..d51b00decaa8ce 100644 --- a/src/coreclr/vm/method.hpp +++ b/src/coreclr/vm/method.hpp @@ -1109,7 +1109,7 @@ class MethodDesc !IsWrapperStub() && // Functional requirement - CodeVersionManager::IsMethodSupported(PTR_MethodDesc(this)); + CodeVersionManager::IsMethodSupported(this->GetMethodDescChunk()); #else // FEATURE_REJIT return false; #endif @@ -1133,6 +1133,8 @@ class MethodDesc bool DetermineAndSetIsEligibleForTieredCompilation(); bool IsJitOptimizationDisabled(); + bool IsJitOptimizationDisabledForAllMethodsInChunk(); + bool IsJitOptimizationDisabledForSpecificMethod(); private: // This function is not intended to be called in most places, and is named as such to discourage calling it accidentally @@ -1663,6 +1665,7 @@ class MethodDesc { LIMITED_METHOD_CONTRACT; _ASSERTE((m_wFlags & mdcClassification) == 0); + _ASSERTE(classification != mcDynamic || (GetMethodDescChunkIndex() == 0)); // Dynamic methods can only exist in MethodDescChunks which have a single MethodDesc m_wFlags |= classification; } @@ -2141,6 +2144,8 @@ class MethodDescChunk MethodTable *initialMT, class AllocMemTracker *pamTracker); + bool DetermineIsEligibleForTieredCompilation(); + TADDR GetTemporaryEntryPoints() { LIMITED_METHOD_CONTRACT; diff --git a/src/coreclr/vm/precode.cpp b/src/coreclr/vm/precode.cpp index 5ad146a07cba0c..6ccdd370c55200 100644 --- a/src/coreclr/vm/precode.cpp +++ b/src/coreclr/vm/precode.cpp @@ -403,9 +403,10 @@ TADDR Precode::AllocateTemporaryEntryPoints(MethodDescChunk * pChunk, #endif { MethodDesc *pMD = pChunk->GetFirstMethodDesc(); + bool chunkContainsEligibleMethods = pChunk->DetermineIsEligibleForTieredCompilation(); for (int i = 0; i < count; ++i) { - if (pMD->DetermineAndSetIsEligibleForTieredCompilation()) + if (chunkContainsEligibleMethods && pMD->DetermineAndSetIsEligibleForTieredCompilation()) { _ASSERTE(pMD->IsEligibleForTieredCompilation()); _ASSERTE(!pMD->IsVersionableWithPrecode() || pMD->RequiresStableEntryPoint()); From 418af3c76f18fc825b3f14eb2e9312f68f3f1251 Mon Sep 17 00:00:00 2001 From: David Wrighton Date: Wed, 3 May 2023 16:19:43 -0700 Subject: [PATCH 05/23] Remove CanShareVtableChunksFrom concept - it was only needed to support NGen --- src/coreclr/vm/array.cpp | 34 +------- src/coreclr/vm/class.cpp | 2 - src/coreclr/vm/generics.cpp | 35 +------- src/coreclr/vm/methodtable.cpp | 17 ---- src/coreclr/vm/methodtable.h | 17 +--- src/coreclr/vm/methodtablebuilder.cpp | 115 ++------------------------ 6 files changed, 11 insertions(+), 209 deletions(-) diff --git a/src/coreclr/vm/array.cpp b/src/coreclr/vm/array.cpp index 54ecd0d7e7e7a3..dccf6a41531709 100644 --- a/src/coreclr/vm/array.cpp +++ b/src/coreclr/vm/array.cpp @@ -312,16 +312,6 @@ MethodTable* Module::CreateArrayMethodTable(TypeHandle elemTypeHnd, CorElementTy // Inherit top level class's interface map cbMT += pParentClass->GetNumInterfaces() * sizeof(InterfaceInfo_t); - BOOL canShareVtableChunks = MethodTable::CanShareVtableChunksFrom(pParentClass, this); - - size_t offsetOfUnsharedVtableChunks = cbMT; - - // We either share all of the parent's virtual slots or none of them - // If none, we need to allocate space for the slots - if (!canShareVtableChunks) - { - cbMT += numVirtuals * sizeof(MethodTable::VTableIndir2_t); - } // Canonical methodtable has an array of non virtual slots pointed to by the optional member size_t offsetOfNonVirtualSlots = 0; @@ -478,28 +468,8 @@ MethodTable* Module::CreateArrayMethodTable(TypeHandle elemTypeHnd, CorElementTy MethodTable::VtableIndirectionSlotIterator it = pMT->IterateVtableIndirectionSlots(); while (it.Next()) { - if (canShareVtableChunks) - { - // Share the parent chunk - it.SetIndirectionSlot(pParentClass->GetVtableIndirections()[it.GetIndex()]); - } - else - { - // Use the locally allocated chunk - it.SetIndirectionSlot((MethodTable::VTableIndir2_t *)(pMemory+cbArrayClass+offsetOfUnsharedVtableChunks)); - offsetOfUnsharedVtableChunks += it.GetSize(); - } - } - - // If we are not sharing parent chunks, copy down the slot contents - if (!canShareVtableChunks) - { - // Copy top level class's vtable - note, vtable is contained within the MethodTable - MethodTable::MethodDataWrapper hParentMTData(MethodTable::GetMethodData(pParentClass, FALSE)); - for (UINT32 i = 0; i < numVirtuals; i++) - { - pMT->CopySlotFrom(i, hParentMTData, pParentClass); - } + // Share the parent chunk + it.SetIndirectionSlot(pParentClass->GetVtableIndirections()[it.GetIndex()]); } if (pClass != NULL) diff --git a/src/coreclr/vm/class.cpp b/src/coreclr/vm/class.cpp index 976604bae2ca44..181cba16edd917 100644 --- a/src/coreclr/vm/class.cpp +++ b/src/coreclr/vm/class.cpp @@ -979,8 +979,6 @@ void ClassLoader::LoadExactParents(MethodTable *pMT) LoadExactParentAndInterfacesTransitively(pMT); - MethodTableBuilder::CopyExactParentSlots(pMT, pApproxParentMT); - PropagateCovariantReturnMethodImplSlots(pMT); // We can now mark this type as having exact parents diff --git a/src/coreclr/vm/generics.cpp b/src/coreclr/vm/generics.cpp index 1ccf8128445a1a..c8aa597479de4f 100644 --- a/src/coreclr/vm/generics.cpp +++ b/src/coreclr/vm/generics.cpp @@ -280,17 +280,6 @@ ClassLoader::CreateTypeHandleForNonCanonicalGenericInstantiation( ThrowHR(COR_E_OVERFLOW); } - BOOL canShareVtableChunks = MethodTable::CanShareVtableChunksFrom(pOldMT, pLoaderModule); - - SIZE_T offsetOfUnsharedVtableChunks = allocSize.Value(); - - // We either share all of the canonical's virtual slots or none of them - // If none, we need to allocate space for the slots - if (!canShareVtableChunks) - { - allocSize += S_SIZE_T( cSlots ) * S_SIZE_T( sizeof(MethodTable::VTableIndir2_t) ); - } - if (allocSize.IsOverflow()) { ThrowHR(COR_E_OVERFLOW); @@ -358,28 +347,8 @@ ClassLoader::CreateTypeHandleForNonCanonicalGenericInstantiation( MethodTable::VtableIndirectionSlotIterator it = pMT->IterateVtableIndirectionSlots(); while (it.Next()) { - if (canShareVtableChunks) - { - // Share the canonical chunk - it.SetIndirectionSlot(pOldMT->GetVtableIndirections()[it.GetIndex()]); - } - else - { - // Use the locally allocated chunk - it.SetIndirectionSlot((MethodTable::VTableIndir2_t *)(pMemory+offsetOfUnsharedVtableChunks)); - offsetOfUnsharedVtableChunks += it.GetSize(); - } - } - - // If we are not sharing parent chunks, copy down the slot contents - if (!canShareVtableChunks) - { - // Need to assign the slots one by one to filter out jump thunks - MethodTable::MethodDataWrapper hOldMTData(MethodTable::GetMethodData(pOldMT, FALSE)); - for (DWORD i = 0; i < cSlots; i++) - { - pMT->CopySlotFrom(i, hOldMTData, pOldMT); - } + // Share the canonical chunk + it.SetIndirectionSlot(pOldMT->GetVtableIndirections()[it.GetIndex()]); } // All flags on m_pNgenPrivateData data apart diff --git a/src/coreclr/vm/methodtable.cpp b/src/coreclr/vm/methodtable.cpp index 0c2a61ccb9f915..e7bf54e7800e45 100644 --- a/src/coreclr/vm/methodtable.cpp +++ b/src/coreclr/vm/methodtable.cpp @@ -1754,13 +1754,6 @@ MethodTable::IsExternallyVisible() return bIsVisible; } // MethodTable::IsExternallyVisible -BOOL MethodTable::CanShareVtableChunksFrom(MethodTable *pTargetMT, Module *pCurrentLoaderModule) -{ - WRAPPER_NO_CONTRACT; - - return pTargetMT->GetLoaderModule() == pCurrentLoaderModule; -} - BOOL MethodTable::IsAllGCPointers() { if (this->ContainsPointers()) @@ -4506,7 +4499,6 @@ struct DoFullyLoadLocals #ifdef FEATURE_TYPEEQUIVALENCE , fHasEquivalentStructParameter(FALSE) #endif - , fDependsOnEquivalentOrForwardedStructs(FALSE) { LIMITED_METHOD_CONTRACT; } @@ -4518,7 +4510,6 @@ struct DoFullyLoadLocals #ifdef FEATURE_TYPEEQUIVALENCE BOOL fHasEquivalentStructParameter; #endif - BOOL fDependsOnEquivalentOrForwardedStructs; }; #if defined(FEATURE_TYPEEQUIVALENCE) && !defined(DACCESS_COMPILE) @@ -4541,7 +4532,6 @@ static void CheckForEquivalenceAndFullyLoadType(Module *pModule, mdToken token, CONSISTENCY_CHECK(!th.IsNull()); th.DoFullyLoad(&pLocals->newVisited, pLocals->level, pLocals->pPending, &pLocals->fBailed, NULL); - pLocals->fDependsOnEquivalentOrForwardedStructs = TRUE; pLocals->fHasEquivalentStructParameter = TRUE; } } @@ -4795,13 +4785,6 @@ void MethodTable::DoFullyLoad(Generics::RecursionGraph * const pVisited, const } #endif //FEATURE_TYPEEQUIVALENCE - if (locals.fDependsOnEquivalentOrForwardedStructs) - { - // if this type declares a method that has an equivalent or type forwarded structure as a parameter type, - // make sure we come here and pre-load these structure types in NGENed cases as well - SetDependsOnEquivalentOrForwardedStructs(); - } - // The rules for constraint cycles are same as rules for access checks if (fNeedAccessChecks) { diff --git a/src/coreclr/vm/methodtable.h b/src/coreclr/vm/methodtable.h index 20fa2b091629e4..dcb2721cc55104 100644 --- a/src/coreclr/vm/methodtable.h +++ b/src/coreclr/vm/methodtable.h @@ -1047,18 +1047,6 @@ class MethodTable SetFlag(enum_flag_IsIntrinsicType); } - // See the comment in code:MethodTable.DoFullyLoad for detailed description. - inline BOOL DependsOnEquivalentOrForwardedStructs() - { - LIMITED_METHOD_CONTRACT; - return GetFlag(enum_flag_DependsOnEquivalentOrForwardedStructs); - } - - inline void SetDependsOnEquivalentOrForwardedStructs() - { - SetFlag(enum_flag_DependsOnEquivalentOrForwardedStructs); - } - // Is this a method table for a generic type instantiation, e.g. List? inline BOOL HasInstantiation(); @@ -1329,8 +1317,6 @@ class MethodTable VtableIndirectionSlotIterator IterateVtableIndirectionSlots(); VtableIndirectionSlotIterator IterateVtableIndirectionSlotsFrom(DWORD index); - static BOOL CanShareVtableChunksFrom(MethodTable *pTargetMT, Module *pCurrentLoaderModule); - inline BOOL HasNonVirtualSlots() { LIMITED_METHOD_DAC_CONTRACT; @@ -3485,8 +3471,7 @@ public : enum_flag_HasSingleNonVirtualSlot = 0x4000, - enum_flag_DependsOnEquivalentOrForwardedStructs= 0x8000, // Declares methods that have type equivalent or type forwarded structures in their signature - + // unused = 0x8000, }; // enum WFLAGS2_ENUM __forceinline void ClearFlag(WFLAGS_LOW_ENUM flag) diff --git a/src/coreclr/vm/methodtablebuilder.cpp b/src/coreclr/vm/methodtablebuilder.cpp index ec1f2690a127e5..3af11fcc134419 100644 --- a/src/coreclr/vm/methodtablebuilder.cpp +++ b/src/coreclr/vm/methodtablebuilder.cpp @@ -9189,95 +9189,6 @@ InsertMethodTable( } // InsertMethodTable -//******************************************************************************* -// -------------------------------------------------------------------------------------------- -// Copy virtual slots inherited from parent: -// -// In types created at runtime, inherited virtual slots are initialized using approximate parent -// during method table building. This method will update them based on the exact parent. -// In types loaded from NGen image, inherited virtual slots from cross-module parents are not -// initialized. This method will initialize them based on the actually loaded exact parent -// if necessary. -/* static */ -void MethodTableBuilder::CopyExactParentSlots(MethodTable *pMT, MethodTable *pApproxParentMT) -{ - CONTRACTL - { - STANDARD_VM_CHECK; - PRECONDITION(CheckPointer(pMT)); - } - CONTRACTL_END; - - DWORD nParentVirtuals = pMT->GetNumParentVirtuals(); - if (nParentVirtuals == 0) - return; - - _ASSERTE(nParentVirtuals == pApproxParentMT->GetNumVirtuals()); - - // - // Update all inherited virtual slots to match exact parent - // - - if (!pMT->IsCanonicalMethodTable()) - { - // - // Copy all slots for non-canonical methodtables to avoid touching methoddescs. - // - MethodTable * pCanonMT = pMT->GetCanonicalMethodTable(); - - // Do not write into vtable chunks shared with parent. It would introduce race - // with code:MethodDesc::SetStableEntryPointInterlocked. - // - // Non-canonical method tables either share everything or nothing so it is sufficient to check - // just the first indirection to detect sharing. - if (pMT->GetVtableIndirections()[0] != pCanonMT->GetVtableIndirections()[0]) - { - MethodTable::MethodDataWrapper hCanonMTData(MethodTable::GetMethodData(pCanonMT, FALSE)); - for (DWORD i = 0; i < nParentVirtuals; i++) - { - pMT->CopySlotFrom(i, hCanonMTData, pCanonMT); - } - } - } - else - { - MethodTable::MethodDataWrapper hMTData(MethodTable::GetMethodData(pMT, FALSE)); - - MethodTable * pParentMT = pMT->GetParentMethodTable(); - MethodTable::MethodDataWrapper hParentMTData(MethodTable::GetMethodData(pParentMT, FALSE)); - - for (DWORD i = 0; i < nParentVirtuals; i++) - { - // fix up wrongly-inherited method descriptors - MethodDesc* pMD = hMTData->GetImplMethodDesc(i); - CONSISTENCY_CHECK(CheckPointer(pMD)); - CONSISTENCY_CHECK(pMD == pMT->GetMethodDescForSlot(i)); - - if (pMD->GetMethodTable() == pMT) - continue; - - // We need to re-inherit this slot from the exact parent. - - DWORD indirectionIndex = MethodTable::GetIndexOfVtableIndirection(i); - if (pMT->GetVtableIndirections()[indirectionIndex] == pApproxParentMT->GetVtableIndirections()[indirectionIndex]) - { - // The slot lives in a chunk shared from the approximate parent MT - // If so, we need to change to share the chunk from the exact parent MT - - _ASSERTE(MethodTable::CanShareVtableChunksFrom(pParentMT, pMT->GetLoaderModule())); - - pMT->GetVtableIndirections()[indirectionIndex] = pParentMT->GetVtableIndirections()[indirectionIndex]; - - i = MethodTable::GetEndSlotForVtableIndirection(indirectionIndex, nParentVirtuals) - 1; - continue; - } - - // The slot lives in an unshared chunk. We need to update the slot contents - pMT->CopySlotFrom(i, hParentMTData, pParentMT); - } - } -} // MethodTableBuilder::CopyExactParentSlots - bool InstantiationIsAllTypeVariables(const Instantiation &inst) { for (auto i = inst.GetNumArgs(); i > 0;) @@ -10310,22 +10221,11 @@ MethodTable * MethodTableBuilder::AllocateNewMT( S_SIZE_T offsetOfUnsharedVtableChunks = cbTotalSize; - BOOL canShareVtableChunks = pMTParent && MethodTable::CanShareVtableChunksFrom(pMTParent, pLoaderModule - ); - - // If pMTParent has a generic instantiation, we cannot share its vtable chunks - // This is because pMTParent is only approximate at this point, and MethodTableBuilder::CopyExactParentSlots - // may swap in an exact parent that does not satisfy CanShareVtableChunksFrom - if (pMTParent && pMTParent->HasInstantiation()) - { - canShareVtableChunks = FALSE; - } - // We will share any parent vtable chunk that does not contain a method we overrode (or introduced) // For the rest, we need to allocate space for (DWORD i = 0; i < dwVirtuals; i++) { - if (!canShareVtableChunks || ChangesImplementationOfVirtualSlot(static_cast(i))) + if (ChangesImplementationOfVirtualSlot(static_cast(i))) { DWORD chunkStart = MethodTable::GetStartSlotForVtableIndirection(MethodTable::GetIndexOfVtableIndirection(i), dwVirtuals); DWORD chunkEnd = MethodTable::GetEndSlotForVtableIndirection(MethodTable::GetIndexOfVtableIndirection(i), dwVirtuals); @@ -10391,18 +10291,15 @@ MethodTable * MethodTableBuilder::AllocateNewMT( MethodTable::VtableIndirectionSlotIterator it = pMT->IterateVtableIndirectionSlots(); while (it.Next()) { - BOOL shared = canShareVtableChunks; + BOOL shared = TRUE; // Recalculate whether we will share this chunk - if (canShareVtableChunks) + for (DWORD i = it.GetStartSlot(); i < it.GetEndSlot(); i++) { - for (DWORD i = it.GetStartSlot(); i < it.GetEndSlot(); i++) + if (ChangesImplementationOfVirtualSlot(static_cast(i))) { - if (ChangesImplementationOfVirtualSlot(static_cast(i))) - { - shared = FALSE; - break; - } + shared = FALSE; + break; } } From 48dfa0e65efef24d98a1d7bd1851b86daaea81b9 Mon Sep 17 00:00:00 2001 From: David Wrighton Date: Wed, 10 May 2023 20:07:11 -0700 Subject: [PATCH 06/23] Fix up some more issues --- src/coreclr/vm/codeversion.cpp | 5 ++--- src/coreclr/vm/codeversion.h | 2 +- src/coreclr/vm/method.cpp | 6 +++--- src/coreclr/vm/method.hpp | 9 +++++---- src/coreclr/vm/precode.cpp | 37 ++++++++++++++++++++++++---------- 5 files changed, 37 insertions(+), 22 deletions(-) diff --git a/src/coreclr/vm/codeversion.cpp b/src/coreclr/vm/codeversion.cpp index 2d87f7bf495908..55fbb5cdd1e2ea 100644 --- a/src/coreclr/vm/codeversion.cpp +++ b/src/coreclr/vm/codeversion.cpp @@ -2086,11 +2086,10 @@ void CodeVersionManager::OnAppDomainExit(AppDomain * pAppDomain) // get the final determination of versioning support for a given method. // //static -bool CodeVersionManager::IsMethodSupported(MethodDescChunk* pMethodDescChunk) +bool CodeVersionManager::IsMethodSupported(PTR_MethodDesc pMethodDesc) { WRAPPER_NO_CONTRACT; - _ASSERTE(pMethodDescChunk != NULL); - MethodDesc* pMethodDesc = pMethodDescChunk->GetFirstMethodDesc(); + _ASSERTE(pMethodDesc != NULL); return // CodeVersionManager data structures don't properly handle the lifetime semantics of dynamic code at this point diff --git a/src/coreclr/vm/codeversion.h b/src/coreclr/vm/codeversion.h index da65ff5b73f69a..66de4ba27257a4 100644 --- a/src/coreclr/vm/codeversion.h +++ b/src/coreclr/vm/codeversion.h @@ -608,7 +608,7 @@ class CodeVersionManager static void OnAppDomainExit(AppDomain* pAppDomain); #endif - static bool IsMethodSupported(MethodDescChunk* pMethodDescChunk); + static bool IsMethodSupported(PTR_MethodDesc pMethodDesc); #ifndef DACCESS_COMPILE static bool InitialNativeCodeVersionMayNotBeTheDefaultNativeCodeVersion() diff --git a/src/coreclr/vm/method.cpp b/src/coreclr/vm/method.cpp index 7a14847ba94e11..2e147e90dab80b 100644 --- a/src/coreclr/vm/method.cpp +++ b/src/coreclr/vm/method.cpp @@ -2970,7 +2970,7 @@ Precode* MethodDesc::GetOrCreatePrecode() return Precode::GetPrecodeFromEntryPoint(*pSlot); } -bool MethodDescChunk::DetermineIsEligibleForTieredCompilation() +bool MethodDesc::DetermineIsEligibleForTieredCompilationInvariantForAllMethodsInChunk() { #ifdef FEATURE_TIERED_COMPILATION #ifndef FEATURE_CODE_VERSIONING @@ -2991,7 +2991,7 @@ bool MethodDescChunk::DetermineIsEligibleForTieredCompilation() !CORProfilerDisableTieredCompilation() && // Policy - Generating optimized code is not disabled - !GetFirstMethodDesc()->IsJitOptimizationDisabledForAllMethodsInChunk(); + !IsJitOptimizationDisabledForAllMethodsInChunk(); #else return false; #endif @@ -3008,7 +3008,7 @@ bool MethodDesc::DetermineAndSetIsEligibleForTieredCompilation() // This function should only be called if the chunk has already been checked. This is done // to reduce the amount of flags checked for each MethodDesc - _ASSERTE(GetMethodDescChunk()->DetermineIsEligibleForTieredCompilation()); + _ASSERTE(DetermineIsEligibleForTieredCompilationInvariantForAllMethodsInChunk()); // Keep in-sync with MethodTableBuilder::NeedsNativeCodeSlot(bmtMDMethod * pMDMethod) // to ensure native slots are available where needed. diff --git a/src/coreclr/vm/method.hpp b/src/coreclr/vm/method.hpp index 85d91f83c70dd1..8b1d0dd62c3bc9 100644 --- a/src/coreclr/vm/method.hpp +++ b/src/coreclr/vm/method.hpp @@ -1109,7 +1109,7 @@ class MethodDesc !IsWrapperStub() && // Functional requirement - CodeVersionManager::IsMethodSupported(this->GetMethodDescChunk()); + CodeVersionManager::IsMethodSupported(PTR_MethodDesc(this)); #else // FEATURE_REJIT return false; #endif @@ -1128,8 +1128,12 @@ class MethodDesc #endif } + // This method must return the same value for all methods in one MethodDescChunk + bool DetermineIsEligibleForTieredCompilationInvariantForAllMethodsInChunk(); + // Is this method allowed to be recompiled and the entrypoint redirected so that we // can optimize its performance? Eligibility is invariant for the lifetime of a method. + bool DetermineAndSetIsEligibleForTieredCompilation(); bool IsJitOptimizationDisabled(); @@ -1665,7 +1669,6 @@ class MethodDesc { LIMITED_METHOD_CONTRACT; _ASSERTE((m_wFlags & mdcClassification) == 0); - _ASSERTE(classification != mcDynamic || (GetMethodDescChunkIndex() == 0)); // Dynamic methods can only exist in MethodDescChunks which have a single MethodDesc m_wFlags |= classification; } @@ -2144,8 +2147,6 @@ class MethodDescChunk MethodTable *initialMT, class AllocMemTracker *pamTracker); - bool DetermineIsEligibleForTieredCompilation(); - TADDR GetTemporaryEntryPoints() { LIMITED_METHOD_CONTRACT; diff --git a/src/coreclr/vm/precode.cpp b/src/coreclr/vm/precode.cpp index f4619937dd4caf..d5a53c7b863368 100644 --- a/src/coreclr/vm/precode.cpp +++ b/src/coreclr/vm/precode.cpp @@ -403,24 +403,39 @@ TADDR Precode::AllocateTemporaryEntryPoints(MethodDescChunk * pChunk, #endif { MethodDesc *pMD = pChunk->GetFirstMethodDesc(); - bool chunkContainsEligibleMethods = pChunk->DetermineIsEligibleForTieredCompilation(); + bool chunkContainsEligibleMethods = pMD->DetermineIsEligibleForTieredCompilationInvariantForAllMethodsInChunk(); + +#ifdef _DEBUG + // Validate every MethodDesc has the same result for DetermineIsEligibleForTieredCompilationInvariantForAllMethodsInChunk + MethodDesc *pMDDebug = pChunk->GetFirstMethodDesc(); for (int i = 0; i < count; ++i) { - if (chunkContainsEligibleMethods && pMD->DetermineAndSetIsEligibleForTieredCompilation()) + _ASSERTE(chunkContainsEligibleMethods == pMDDebug->DetermineIsEligibleForTieredCompilationInvariantForAllMethodsInChunk()); + pMDDebug = (MethodDesc *)(dac_cast(pMDDebug) + pMDDebug->SizeOf()); + } +#endif +#ifndef HAS_COMPACT_ENTRYPOINTS + if (chunkContainsEligibleMethods) +#endif + { + for (int i = 0; i < count; ++i) { - _ASSERTE(pMD->IsEligibleForTieredCompilation()); - _ASSERTE(!pMD->IsVersionableWithPrecode() || pMD->RequiresStableEntryPoint()); - } + if (chunkContainsEligibleMethods && pMD->DetermineAndSetIsEligibleForTieredCompilation()) + { + _ASSERTE(pMD->IsEligibleForTieredCompilation()); + _ASSERTE(!pMD->IsVersionableWithPrecode() || pMD->RequiresStableEntryPoint()); + } #ifdef HAS_COMPACT_ENTRYPOINTS - if (pMD->IsVersionableWithPrecode()) - { - _ASSERTE(pMD->RequiresStableEntryPoint()); - hasMethodDescVersionableWithPrecode = true; - } + if (pMD->IsVersionableWithPrecode()) + { + _ASSERTE(pMD->RequiresStableEntryPoint()); + hasMethodDescVersionableWithPrecode = true; + } #endif - pMD = (MethodDesc *)(dac_cast(pMD) + pMD->SizeOf()); + pMD = (MethodDesc *)(dac_cast(pMD) + pMD->SizeOf()); + } } } From d274c5171e85f684eebc25617ac5d9b9480144b3 Mon Sep 17 00:00:00 2001 From: David Wrighton Date: Thu, 11 May 2023 15:06:34 -0700 Subject: [PATCH 07/23] Bring back late virtual propagation in the presence of covariant overrides only --- src/coreclr/vm/class.cpp | 8 +-- src/coreclr/vm/methodtablebuilder.cpp | 77 +++++++++++++++++++++++++++ src/coreclr/vm/methodtablebuilder.h | 3 +- src/coreclr/vm/siginfo.cpp | 3 +- 4 files changed, 85 insertions(+), 6 deletions(-) diff --git a/src/coreclr/vm/class.cpp b/src/coreclr/vm/class.cpp index e001897db8662f..fbabd922bcb012 100644 --- a/src/coreclr/vm/class.cpp +++ b/src/coreclr/vm/class.cpp @@ -1204,8 +1204,6 @@ void ClassLoader::LoadExactParents(MethodTable* pMT) } CONTRACT_END; - MethodTable *pApproxParentMT = pMT->GetParentMethodTable(); - if (!pMT->IsCanonicalMethodTable()) { EnsureLoaded(TypeHandle(pMT->GetCanonicalMethodTable()), CLASS_LOAD_EXACTPARENTS); @@ -1213,7 +1211,11 @@ void ClassLoader::LoadExactParents(MethodTable* pMT) LoadExactParentAndInterfacesTransitively(pMT); - PropagateCovariantReturnMethodImplSlots(pMT); + if (pMT->GetClass()->HasCovariantOverride()) + { + MethodTableBuilder::CopyExactParentSlots(pMT); + PropagateCovariantReturnMethodImplSlots(pMT); + } #ifdef EnC_SUPPORTED // Generics for EnC - create static FieldDescs. diff --git a/src/coreclr/vm/methodtablebuilder.cpp b/src/coreclr/vm/methodtablebuilder.cpp index 84c932d388afdb..11018f332ae3dc 100644 --- a/src/coreclr/vm/methodtablebuilder.cpp +++ b/src/coreclr/vm/methodtablebuilder.cpp @@ -9190,6 +9190,83 @@ InsertMethodTable( return FALSE; } // InsertMethodTable +//******************************************************************************* +// -------------------------------------------------------------------------------------------- +// Copy virtual slots inherited from parent: +// +// In types where covariant returns are used, inherited virtual slots may not yet be fully +// resolved during initial MethodTable building. This method will update them based on the +// values computed during exact parent calculation of the parent type. +/* static */ +void MethodTableBuilder::CopyExactParentSlots(MethodTable *pMT) +{ + CONTRACTL + { + STANDARD_VM_CHECK; + PRECONDITION(CheckPointer(pMT)); + } + CONTRACTL_END; + + DWORD nParentVirtuals = pMT->GetNumParentVirtuals(); + if (nParentVirtuals == 0) + return; + + // + // Update all inherited virtual slots to match exact parent + // + + if (!pMT->IsCanonicalMethodTable()) + { + // + // Copy all slots for non-canonical methodtables to avoid touching methoddescs. + // + MethodTable * pCanonMT = pMT->GetCanonicalMethodTable(); + + // Do not write into vtable chunks shared with parent. It would introduce race + // with code:MethodDesc::SetStableEntryPointInterlocked. + // + // Non-canonical method tables either share everything or nothing so it is sufficient to check + // just the first indirection to detect sharing. + if (pMT->GetVtableIndirections()[0] != pCanonMT->GetVtableIndirections()[0]) + { + MethodTable::MethodDataWrapper hCanonMTData(MethodTable::GetMethodData(pCanonMT, FALSE)); + for (DWORD i = 0; i < nParentVirtuals; i++) + { + pMT->CopySlotFrom(i, hCanonMTData, pCanonMT); + } + } + } + else + { + MethodTable::MethodDataWrapper hMTData(MethodTable::GetMethodData(pMT, FALSE)); + + MethodTable * pParentMT = pMT->GetParentMethodTable(); + MethodTable::MethodDataWrapper hParentMTData(MethodTable::GetMethodData(pParentMT, FALSE)); + + for (DWORD i = 0; i < nParentVirtuals; i++) + { + // fix up wrongly-inherited method descriptors + MethodDesc* pMD = hMTData->GetImplMethodDesc(i); + CONSISTENCY_CHECK(CheckPointer(pMD)); + CONSISTENCY_CHECK(pMD == pMT->GetMethodDescForSlot(i)); + + if (pMD->GetMethodTable() == pMT) + continue; + + // We need to re-inherit this slot from the exact parent. + + DWORD indirectionIndex = MethodTable::GetIndexOfVtableIndirection(i); + if (pMT->GetVtableIndirections()[indirectionIndex] == pParentMT->GetVtableIndirections()[indirectionIndex]) + { + // The slot lives in a chunk shared from the parent MT + continue; + } + + // The slot lives in an unshared chunk. We need to update the slot contents + pMT->CopySlotFrom(i, hParentMTData, pParentMT); + } + } +} // MethodTableBuilder::CopyExactParentSlots bool InstantiationIsAllTypeVariables(const Instantiation &inst) { diff --git a/src/coreclr/vm/methodtablebuilder.h b/src/coreclr/vm/methodtablebuilder.h index 8e91efe45ad472..34b53db345146f 100644 --- a/src/coreclr/vm/methodtablebuilder.h +++ b/src/coreclr/vm/methodtablebuilder.h @@ -2499,8 +2499,7 @@ class MethodTableBuilder // static void CopyExactParentSlots( - MethodTable *pMT, - MethodTable *pApproxParentMT); + MethodTable *pMT); // -------------------------------------------------------------------------------------------- // This is used at load time, using metadata-based comparisons. It returns the array of dispatch diff --git a/src/coreclr/vm/siginfo.cpp b/src/coreclr/vm/siginfo.cpp index c9c72f785ab362..a44f40cd9b34fb 100644 --- a/src/coreclr/vm/siginfo.cpp +++ b/src/coreclr/vm/siginfo.cpp @@ -3503,6 +3503,7 @@ BOOL CompareTypeTokens(mdToken tk1, mdToken tk2, ModuleBase *pModule1, ModuleBas } else { + _ASSERTE(pModule1->IsFullModule()); // Only full modules have typedefs if (FAILED(hr = pModule1->m_pEnclosingTypeMap->GetEnclosingTypeNoThrow(tk1, &enclosingTypeTk1, pInternalImport1))) { if (hr != CLDB_E_RECORD_NOTFOUND) @@ -3524,7 +3525,7 @@ BOOL CompareTypeTokens(mdToken tk1, mdToken tk2, ModuleBase *pModule1, ModuleBas } else { - _ASSERTE(pModule1->IsFullModule()); + _ASSERTE(pModule2->IsFullModule()); // Only full modules have typedefs if (FAILED(hr = pModule2->m_pEnclosingTypeMap->GetEnclosingTypeNoThrow(tk2, &enclosingTypeTk2, pInternalImport2))) { if (hr != CLDB_E_RECORD_NOTFOUND) From 5bae8790b2fd1d0d4f34d7c37fa125e7130b3c31 Mon Sep 17 00:00:00 2001 From: David Wrighton Date: Thu, 11 May 2023 19:26:46 -0700 Subject: [PATCH 08/23] Check correct flag on EEClass --- src/coreclr/vm/class.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/coreclr/vm/class.cpp b/src/coreclr/vm/class.cpp index fbabd922bcb012..faf9483aa45ab9 100644 --- a/src/coreclr/vm/class.cpp +++ b/src/coreclr/vm/class.cpp @@ -1211,7 +1211,7 @@ void ClassLoader::LoadExactParents(MethodTable* pMT) LoadExactParentAndInterfacesTransitively(pMT); - if (pMT->GetClass()->HasCovariantOverride()) + if (pMT->GetClass()->HasVTableMethodImpl()) { MethodTableBuilder::CopyExactParentSlots(pMT); PropagateCovariantReturnMethodImplSlots(pMT); From 9f9e586634561c41fe8720293a9f107362eba95b Mon Sep 17 00:00:00 2001 From: David Wrighton Date: Thu, 11 May 2023 19:27:34 -0700 Subject: [PATCH 09/23] Drive by fix for GetRestoredSlot. We don't need the handling of unrestored slots anymore --- src/coreclr/vm/methodtable.cpp | 40 ++++------------------------------ 1 file changed, 4 insertions(+), 36 deletions(-) diff --git a/src/coreclr/vm/methodtable.cpp b/src/coreclr/vm/methodtable.cpp index 8067251905943c..6a4d67eb8c9afb 100644 --- a/src/coreclr/vm/methodtable.cpp +++ b/src/coreclr/vm/methodtable.cpp @@ -8685,24 +8685,9 @@ PCODE MethodTable::GetRestoredSlot(DWORD slotNumber) // Keep in sync with code:MethodTable::GetRestoredSlotMT // - MethodTable * pMT = this; - while (true) - { - pMT = pMT->GetCanonicalMethodTable(); - - _ASSERTE(pMT != NULL); - - PCODE slot = pMT->GetSlot(slotNumber); - - if (slot != NULL) - { - return slot; - } - - // This is inherited slot that has not been fixed up yet. Find - // the value by walking up the inheritance chain - pMT = pMT->GetParentMethodTable(); - } + PCODE slot = GetCanonicalMethodTable()->GetSlot(slotNumber); + _ASSERTE(slot != NULL); + return slot; } //========================================================================================== @@ -8719,24 +8704,7 @@ MethodTable * MethodTable::GetRestoredSlotMT(DWORD slotNumber) // Keep in sync with code:MethodTable::GetRestoredSlot // - MethodTable * pMT = this; - while (true) - { - pMT = pMT->GetCanonicalMethodTable(); - - _ASSERTE(pMT != NULL); - - PCODE slot = pMT->GetSlot(slotNumber); - - if (slot != NULL) - { - return pMT; - } - - // This is inherited slot that has not been fixed up yet. Find - // the value by walking up the inheritance chain - pMT = pMT->GetParentMethodTable(); - } + return GetCanonicalMethodTable(); } //========================================================================================== From f5e72a682980338c263b65c8d32aed9da7092392 Mon Sep 17 00:00:00 2001 From: David Wrighton Date: Thu, 11 May 2023 20:00:06 -0700 Subject: [PATCH 10/23] Fix composite build with new tables --- .../ReadyToRun/EnclosingTypeMapNode.cs | 7 +++-- .../ReadyToRun/HeaderNode.cs | 28 +++++++++++++++++++ .../ReadyToRun/InliningInfoNode.cs | 22 ++------------- .../ReadyToRun/MethodEntryPointTableNode.cs | 13 ++------- .../ReadyToRun/MethodIsGenericMapNode.cs | 7 +++-- .../ReadyToRun/TypeGenericInfoMapNode.cs | 7 +++-- .../ReadyToRun/TypesTableNode.cs | 13 ++------- .../ReadyToRunCodegenNodeFactory.cs | 6 ++-- 8 files changed, 49 insertions(+), 54 deletions(-) diff --git a/src/coreclr/tools/aot/ILCompiler.ReadyToRun/Compiler/DependencyAnalysis/ReadyToRun/EnclosingTypeMapNode.cs b/src/coreclr/tools/aot/ILCompiler.ReadyToRun/Compiler/DependencyAnalysis/ReadyToRun/EnclosingTypeMapNode.cs index 4433dfffd3a2f4..24e75db7e0163c 100644 --- a/src/coreclr/tools/aot/ILCompiler.ReadyToRun/Compiler/DependencyAnalysis/ReadyToRun/EnclosingTypeMapNode.cs +++ b/src/coreclr/tools/aot/ILCompiler.ReadyToRun/Compiler/DependencyAnalysis/ReadyToRun/EnclosingTypeMapNode.cs @@ -4,6 +4,7 @@ using System; using Internal.Text; +using Internal.TypeSystem.Ecma; using System.Diagnostics; using System.Reflection; using System.Reflection.Metadata; @@ -12,13 +13,13 @@ namespace ILCompiler.DependencyAnalysis.ReadyToRun { - public class EnclosingTypeMapNode : HeaderTableNode + public class EnclosingTypeMapNode : ModuleSpecificHeaderTableNode { private MetadataReader _metadata; - public EnclosingTypeMapNode(MetadataReader metadata) + public EnclosingTypeMapNode(EcmaModule module) : base(module) { - _metadata = metadata; + _metadata = module.MetadataReader; // This map is only valid for assemblies with <= 0xFFFE types defined within if (!IsSupported(_metadata)) throw new InternalCompilerErrorException("EnclosingTypeMap made for assembly with more than 0xFFFE types"); diff --git a/src/coreclr/tools/aot/ILCompiler.ReadyToRun/Compiler/DependencyAnalysis/ReadyToRun/HeaderNode.cs b/src/coreclr/tools/aot/ILCompiler.ReadyToRun/Compiler/DependencyAnalysis/ReadyToRun/HeaderNode.cs index 7c57adaaa02372..6cbf0db592e5b3 100644 --- a/src/coreclr/tools/aot/ILCompiler.ReadyToRun/Compiler/DependencyAnalysis/ReadyToRun/HeaderNode.cs +++ b/src/coreclr/tools/aot/ILCompiler.ReadyToRun/Compiler/DependencyAnalysis/ReadyToRun/HeaderNode.cs @@ -7,6 +7,7 @@ using Internal.Runtime; using Internal.Text; using Internal.TypeSystem; +using Internal.TypeSystem.Ecma; using Internal.ReadyToRunConstants; using ILCompiler.DependencyAnalysisFramework; @@ -33,6 +34,33 @@ public override ObjectNodeSection GetSection(NodeFactory factory) } } + public abstract class ModuleSpecificHeaderTableNode : HeaderTableNode + { + protected readonly EcmaModule _module; + + public ModuleSpecificHeaderTableNode(EcmaModule module) + { + _module = module; + } + + public sealed override int CompareToImpl(ISortableNode other, CompilerComparer comparer) + { + ModuleSpecificHeaderTableNode otherModuleSpecificHeaderTableNode = (ModuleSpecificHeaderTableNode)other; + + if (_module == null) + { + Debug.Assert(otherModuleSpecificHeaderTableNode._module != null); + return -1; + } + else if (otherModuleSpecificHeaderTableNode._module == null) + { + return 1; + } + + return _module.Assembly.GetName().Name.CompareTo(otherModuleSpecificHeaderTableNode._module.Assembly.GetName().Name); + } + } + public abstract class HeaderNode : ObjectNode, ISymbolDefinitionNode { struct HeaderItem diff --git a/src/coreclr/tools/aot/ILCompiler.ReadyToRun/Compiler/DependencyAnalysis/ReadyToRun/InliningInfoNode.cs b/src/coreclr/tools/aot/ILCompiler.ReadyToRun/Compiler/DependencyAnalysis/ReadyToRun/InliningInfoNode.cs index 0d4438e047631f..669f5f578a41b8 100644 --- a/src/coreclr/tools/aot/ILCompiler.ReadyToRun/Compiler/DependencyAnalysis/ReadyToRun/InliningInfoNode.cs +++ b/src/coreclr/tools/aot/ILCompiler.ReadyToRun/Compiler/DependencyAnalysis/ReadyToRun/InliningInfoNode.cs @@ -18,7 +18,7 @@ namespace ILCompiler.DependencyAnalysis.ReadyToRun /// /// Stores information about what methods got inlined into other methods. /// - public class InliningInfoNode : HeaderTableNode + public class InliningInfoNode : ModuleSpecificHeaderTableNode { public enum InfoType { @@ -27,11 +27,10 @@ public enum InfoType CrossModuleAllMethods } - private readonly EcmaModule _module; private readonly InfoType _inlineInfoType; private ReadyToRunSymbolNodeFactory _symbolNodeFactory; - public InliningInfoNode(EcmaModule module, InfoType inlineInfoType) + public InliningInfoNode(EcmaModule module, InfoType inlineInfoType) : base(module) { _inlineInfoType = inlineInfoType; if (AllowCrossModuleInlines) @@ -42,7 +41,6 @@ public InliningInfoNode(EcmaModule module, InfoType inlineInfoType) { Debug.Assert(module != null); // InliningInfo2 is restricted to a single module at a time } - _module = module; } public void Initialize(ReadyToRunSymbolNodeFactory symbolNodeFactory) @@ -379,22 +377,6 @@ public override ObjectData GetData(NodeFactory factory, bool relocsOnly = false) definedSymbols: new ISymbolDefinitionNode[] { this }); } - public override int CompareToImpl(ISortableNode other, CompilerComparer comparer) - { - InliningInfoNode otherInliningInfo = (InliningInfoNode)other; - - if (_module == null) - { - Debug.Assert(otherInliningInfo._module != null); - return -1; - } - else if (otherInliningInfo._module == null) - { - return 1; - } - return _module.Assembly.GetName().Name.CompareTo(otherInliningInfo._module.Assembly.GetName().Name); - } - public override int ClassCode => -87382891; } } diff --git a/src/coreclr/tools/aot/ILCompiler.ReadyToRun/Compiler/DependencyAnalysis/ReadyToRun/MethodEntryPointTableNode.cs b/src/coreclr/tools/aot/ILCompiler.ReadyToRun/Compiler/DependencyAnalysis/ReadyToRun/MethodEntryPointTableNode.cs index 4aac38bec9fd96..58906d3daa0039 100644 --- a/src/coreclr/tools/aot/ILCompiler.ReadyToRun/Compiler/DependencyAnalysis/ReadyToRun/MethodEntryPointTableNode.cs +++ b/src/coreclr/tools/aot/ILCompiler.ReadyToRun/Compiler/DependencyAnalysis/ReadyToRun/MethodEntryPointTableNode.cs @@ -15,10 +15,8 @@ namespace ILCompiler.DependencyAnalysis.ReadyToRun { - public class MethodEntryPointTableNode : HeaderTableNode + public class MethodEntryPointTableNode : ModuleSpecificHeaderTableNode { - private readonly EcmaModule _module; - private struct EntryPoint { public static EntryPoint Null = new EntryPoint(-1, null); @@ -35,9 +33,8 @@ public EntryPoint(int methodIndex, MethodWithGCInfo method) } } - public MethodEntryPointTableNode(EcmaModule module) + public MethodEntryPointTableNode(EcmaModule module) : base(module) { - _module = module; } public override void AppendMangledName(NameMangler nameMangler, Utf8StringBuilder sb) @@ -113,12 +110,6 @@ public override ObjectData GetData(NodeFactory factory, bool relocsOnly = false) definedSymbols: new ISymbolDefinitionNode[] { this }); } - public override int CompareToImpl(ISortableNode other, CompilerComparer comparer) - { - MethodEntryPointTableNode otherMethodEntryPointTable = (MethodEntryPointTableNode)other; - return _module.Assembly.GetName().Name.CompareTo(otherMethodEntryPointTable._module.Assembly.GetName().Name); - } - protected internal override int Phase => (int)ObjectNodePhase.Ordered; public override int ClassCode => (int)ObjectNodeOrder.MethodEntrypointTableNode; } diff --git a/src/coreclr/tools/aot/ILCompiler.ReadyToRun/Compiler/DependencyAnalysis/ReadyToRun/MethodIsGenericMapNode.cs b/src/coreclr/tools/aot/ILCompiler.ReadyToRun/Compiler/DependencyAnalysis/ReadyToRun/MethodIsGenericMapNode.cs index 4f79f6aa33ca20..61c8c8f7457f6b 100644 --- a/src/coreclr/tools/aot/ILCompiler.ReadyToRun/Compiler/DependencyAnalysis/ReadyToRun/MethodIsGenericMapNode.cs +++ b/src/coreclr/tools/aot/ILCompiler.ReadyToRun/Compiler/DependencyAnalysis/ReadyToRun/MethodIsGenericMapNode.cs @@ -6,16 +6,17 @@ using Internal.Text; using System.Diagnostics; using System.Reflection.Metadata; +using Internal.TypeSystem.Ecma; namespace ILCompiler.DependencyAnalysis.ReadyToRun { - public class MethodIsGenericMapNode : HeaderTableNode + public class MethodIsGenericMapNode : ModuleSpecificHeaderTableNode { private MetadataReader _metadata; - public MethodIsGenericMapNode(MetadataReader metadata) + public MethodIsGenericMapNode(EcmaModule module) : base(module) { - _metadata = metadata; + _metadata = module.MetadataReader; } public static bool IsSupported(MetadataReader metadata) diff --git a/src/coreclr/tools/aot/ILCompiler.ReadyToRun/Compiler/DependencyAnalysis/ReadyToRun/TypeGenericInfoMapNode.cs b/src/coreclr/tools/aot/ILCompiler.ReadyToRun/Compiler/DependencyAnalysis/ReadyToRun/TypeGenericInfoMapNode.cs index 7e01d89db7012d..5b27e27747b4c7 100644 --- a/src/coreclr/tools/aot/ILCompiler.ReadyToRun/Compiler/DependencyAnalysis/ReadyToRun/TypeGenericInfoMapNode.cs +++ b/src/coreclr/tools/aot/ILCompiler.ReadyToRun/Compiler/DependencyAnalysis/ReadyToRun/TypeGenericInfoMapNode.cs @@ -9,16 +9,17 @@ using System.Reflection.Metadata; using Internal.ReadyToRunConstants; using Internal.TypeSystem; +using Internal.TypeSystem.Ecma; namespace ILCompiler.DependencyAnalysis.ReadyToRun { - public class TypeGenericInfoMapNode : HeaderTableNode + public class TypeGenericInfoMapNode : ModuleSpecificHeaderTableNode { private MetadataReader _metadata; - public TypeGenericInfoMapNode(MetadataReader metadata) + public TypeGenericInfoMapNode(EcmaModule module) : base (module) { - _metadata = metadata; + _metadata = module.MetadataReader; } public static bool IsSupported(MetadataReader metadata) diff --git a/src/coreclr/tools/aot/ILCompiler.ReadyToRun/Compiler/DependencyAnalysis/ReadyToRun/TypesTableNode.cs b/src/coreclr/tools/aot/ILCompiler.ReadyToRun/Compiler/DependencyAnalysis/ReadyToRun/TypesTableNode.cs index 715b9bb1743948..1681aad2c8ec5b 100644 --- a/src/coreclr/tools/aot/ILCompiler.ReadyToRun/Compiler/DependencyAnalysis/ReadyToRun/TypesTableNode.cs +++ b/src/coreclr/tools/aot/ILCompiler.ReadyToRun/Compiler/DependencyAnalysis/ReadyToRun/TypesTableNode.cs @@ -15,13 +15,10 @@ namespace ILCompiler.DependencyAnalysis.ReadyToRun { - public class TypesTableNode : HeaderTableNode + public class TypesTableNode : ModuleSpecificHeaderTableNode { - private readonly EcmaModule _module; - - public TypesTableNode(EcmaModule module) + public TypesTableNode(EcmaModule module) : base(module) { - _module = module; } public override void AppendMangledName(NameMangler nameMangler, Utf8StringBuilder sb) @@ -93,12 +90,6 @@ public override ObjectData GetData(NodeFactory factory, bool relocsOnly = false) definedSymbols: new ISymbolDefinitionNode[] { this }); } - public override int CompareToImpl(ISortableNode other, CompilerComparer comparer) - { - TypesTableNode otherTypesTable = (TypesTableNode)other; - return _module.Assembly.GetName().Name.CompareTo(otherTypesTable._module.Assembly.GetName().Name); - } - public override int ClassCode => -944318825; } } diff --git a/src/coreclr/tools/aot/ILCompiler.ReadyToRun/Compiler/DependencyAnalysis/ReadyToRunCodegenNodeFactory.cs b/src/coreclr/tools/aot/ILCompiler.ReadyToRun/Compiler/DependencyAnalysis/ReadyToRunCodegenNodeFactory.cs index 10899f8ea5befb..9c3849efaf5877 100644 --- a/src/coreclr/tools/aot/ILCompiler.ReadyToRun/Compiler/DependencyAnalysis/ReadyToRunCodegenNodeFactory.cs +++ b/src/coreclr/tools/aot/ILCompiler.ReadyToRun/Compiler/DependencyAnalysis/ReadyToRunCodegenNodeFactory.cs @@ -735,19 +735,19 @@ public void AttachToDependencyGraph(DependencyAnalyzerBase graph, I if (EnclosingTypeMapNode.IsSupported(inputModule.MetadataReader)) { - var node = new EnclosingTypeMapNode(inputModule.MetadataReader); + var node = new EnclosingTypeMapNode(inputModule); tableHeader.Add(Internal.Runtime.ReadyToRunSectionType.EnclosingTypeMap, node, node); } if (TypeGenericInfoMapNode.IsSupported(inputModule.MetadataReader)) { - var node = new TypeGenericInfoMapNode(inputModule.MetadataReader); + var node = new TypeGenericInfoMapNode(inputModule); tableHeader.Add(Internal.Runtime.ReadyToRunSectionType.TypeGenericInfoMap, node, node); } if (MethodIsGenericMapNode.IsSupported(inputModule.MetadataReader)) { - var node = new MethodIsGenericMapNode(inputModule.MetadataReader); + var node = new MethodIsGenericMapNode(inputModule); tableHeader.Add(Internal.Runtime.ReadyToRunSectionType.MethodIsGenericMap, node, node); } } From 8eaf4a039ac139fce0dccb56646e5c91d8d0a11f Mon Sep 17 00:00:00 2001 From: David Wrighton Date: Mon, 15 May 2023 10:01:27 -0700 Subject: [PATCH 11/23] Uniquify the mangled names --- .../ReadyToRun/EnclosingTypeMapNode.cs | 6 +----- .../DependencyAnalysis/ReadyToRun/HeaderNode.cs | 17 +++++++++++++++++ .../ReadyToRun/InliningInfoNode.cs | 14 +------------- .../ReadyToRun/MethodEntryPointTableNode.cs | 9 ++------- .../ReadyToRun/MethodIsGenericMapNode.cs | 6 +----- .../ReadyToRun/TypeGenericInfoMapNode.cs | 6 +----- .../ReadyToRun/TypesTableNode.cs | 9 ++------- 7 files changed, 25 insertions(+), 42 deletions(-) diff --git a/src/coreclr/tools/aot/ILCompiler.ReadyToRun/Compiler/DependencyAnalysis/ReadyToRun/EnclosingTypeMapNode.cs b/src/coreclr/tools/aot/ILCompiler.ReadyToRun/Compiler/DependencyAnalysis/ReadyToRun/EnclosingTypeMapNode.cs index 24e75db7e0163c..02af4259c7dd2b 100644 --- a/src/coreclr/tools/aot/ILCompiler.ReadyToRun/Compiler/DependencyAnalysis/ReadyToRun/EnclosingTypeMapNode.cs +++ b/src/coreclr/tools/aot/ILCompiler.ReadyToRun/Compiler/DependencyAnalysis/ReadyToRun/EnclosingTypeMapNode.cs @@ -37,11 +37,7 @@ public static bool IsSupported(MetadataReader metadata) public override int ClassCode => 990540812; - public override void AppendMangledName(NameMangler nameMangler, Utf8StringBuilder sb) - { - sb.Append(nameMangler.CompilationUnitPrefix); - sb.Append("__EnclosingTypeMap"); - } + protected override string ModuleSpecificName => "__EnclosingTypeMap"; public override ObjectData GetData(NodeFactory factory, bool relocsOnly = false) { diff --git a/src/coreclr/tools/aot/ILCompiler.ReadyToRun/Compiler/DependencyAnalysis/ReadyToRun/HeaderNode.cs b/src/coreclr/tools/aot/ILCompiler.ReadyToRun/Compiler/DependencyAnalysis/ReadyToRun/HeaderNode.cs index 6cbf0db592e5b3..d27731d68f21ff 100644 --- a/src/coreclr/tools/aot/ILCompiler.ReadyToRun/Compiler/DependencyAnalysis/ReadyToRun/HeaderNode.cs +++ b/src/coreclr/tools/aot/ILCompiler.ReadyToRun/Compiler/DependencyAnalysis/ReadyToRun/HeaderNode.cs @@ -59,6 +59,23 @@ public sealed override int CompareToImpl(ISortableNode other, CompilerComparer c return _module.Assembly.GetName().Name.CompareTo(otherModuleSpecificHeaderTableNode._module.Assembly.GetName().Name); } + + protected abstract string ModuleSpecificName { get; } + + public sealed override void AppendMangledName(NameMangler nameMangler, Utf8StringBuilder sb) + { + sb.Append(nameMangler.CompilationUnitPrefix); + if (_module != null) + { + sb.Append(ModuleSpecificName); + sb.Append(_module.Assembly.GetName().Name); + } + else + { + sb.Append(ModuleSpecificName); + } + + } } public abstract class HeaderNode : ObjectNode, ISymbolDefinitionNode diff --git a/src/coreclr/tools/aot/ILCompiler.ReadyToRun/Compiler/DependencyAnalysis/ReadyToRun/InliningInfoNode.cs b/src/coreclr/tools/aot/ILCompiler.ReadyToRun/Compiler/DependencyAnalysis/ReadyToRun/InliningInfoNode.cs index 669f5f578a41b8..e0623504015459 100644 --- a/src/coreclr/tools/aot/ILCompiler.ReadyToRun/Compiler/DependencyAnalysis/ReadyToRun/InliningInfoNode.cs +++ b/src/coreclr/tools/aot/ILCompiler.ReadyToRun/Compiler/DependencyAnalysis/ReadyToRun/InliningInfoNode.cs @@ -48,19 +48,7 @@ public void Initialize(ReadyToRunSymbolNodeFactory symbolNodeFactory) _symbolNodeFactory = symbolNodeFactory; } - public override void AppendMangledName(NameMangler nameMangler, Utf8StringBuilder sb) - { - sb.Append(nameMangler.CompilationUnitPrefix); - if (_module != null) - { - sb.Append("__ReadyToRunInliningInfoTable__"); - sb.Append(_module.Assembly.GetName().Name); - } - else - { - sb.Append("__ReadyToRunCrossModuleInliningInfoTable__"); - } - } + protected override string ModuleSpecificName => "__ReadyToRunInliningInfoTable__"; private bool AllowCrossModuleInlines => _inlineInfoType == InfoType.CrossModuleAllMethods || _inlineInfoType == InfoType.CrossModuleInliningForCrossModuleDataOnly; private bool ReportAllInlinesInSearch => _inlineInfoType == InfoType.CrossModuleAllMethods; diff --git a/src/coreclr/tools/aot/ILCompiler.ReadyToRun/Compiler/DependencyAnalysis/ReadyToRun/MethodEntryPointTableNode.cs b/src/coreclr/tools/aot/ILCompiler.ReadyToRun/Compiler/DependencyAnalysis/ReadyToRun/MethodEntryPointTableNode.cs index 58906d3daa0039..ff1a46918b186e 100644 --- a/src/coreclr/tools/aot/ILCompiler.ReadyToRun/Compiler/DependencyAnalysis/ReadyToRun/MethodEntryPointTableNode.cs +++ b/src/coreclr/tools/aot/ILCompiler.ReadyToRun/Compiler/DependencyAnalysis/ReadyToRun/MethodEntryPointTableNode.cs @@ -36,13 +36,8 @@ public EntryPoint(int methodIndex, MethodWithGCInfo method) public MethodEntryPointTableNode(EcmaModule module) : base(module) { } - - public override void AppendMangledName(NameMangler nameMangler, Utf8StringBuilder sb) - { - sb.Append(nameMangler.CompilationUnitPrefix); - sb.Append("__ReadyToRunMethodEntryPointTable__"); - sb.Append(_module.Assembly.GetName().Name); - } + + protected override string ModuleSpecificName => "__ReadyToRunMethodEntryPointTable__"; public override ObjectData GetData(NodeFactory factory, bool relocsOnly = false) { diff --git a/src/coreclr/tools/aot/ILCompiler.ReadyToRun/Compiler/DependencyAnalysis/ReadyToRun/MethodIsGenericMapNode.cs b/src/coreclr/tools/aot/ILCompiler.ReadyToRun/Compiler/DependencyAnalysis/ReadyToRun/MethodIsGenericMapNode.cs index 61c8c8f7457f6b..adfc0fe7543060 100644 --- a/src/coreclr/tools/aot/ILCompiler.ReadyToRun/Compiler/DependencyAnalysis/ReadyToRun/MethodIsGenericMapNode.cs +++ b/src/coreclr/tools/aot/ILCompiler.ReadyToRun/Compiler/DependencyAnalysis/ReadyToRun/MethodIsGenericMapNode.cs @@ -27,11 +27,7 @@ public static bool IsSupported(MetadataReader metadata) public override int ClassCode => 606284890; - public override void AppendMangledName(NameMangler nameMangler, Utf8StringBuilder sb) - { - sb.Append(nameMangler.CompilationUnitPrefix); - sb.Append("__MethodIsGenericMap"); - } + protected override string ModuleSpecificName => "__MethodIsGenericMap__"; public override ObjectData GetData(NodeFactory factory, bool relocsOnly = false) { diff --git a/src/coreclr/tools/aot/ILCompiler.ReadyToRun/Compiler/DependencyAnalysis/ReadyToRun/TypeGenericInfoMapNode.cs b/src/coreclr/tools/aot/ILCompiler.ReadyToRun/Compiler/DependencyAnalysis/ReadyToRun/TypeGenericInfoMapNode.cs index 5b27e27747b4c7..e6b4762433dfc2 100644 --- a/src/coreclr/tools/aot/ILCompiler.ReadyToRun/Compiler/DependencyAnalysis/ReadyToRun/TypeGenericInfoMapNode.cs +++ b/src/coreclr/tools/aot/ILCompiler.ReadyToRun/Compiler/DependencyAnalysis/ReadyToRun/TypeGenericInfoMapNode.cs @@ -30,11 +30,7 @@ public static bool IsSupported(MetadataReader metadata) public override int ClassCode => 1329419084; - public override void AppendMangledName(NameMangler nameMangler, Utf8StringBuilder sb) - { - sb.Append(nameMangler.CompilationUnitPrefix); - sb.Append("__TypeGenericInfoMapNode"); - } + protected override string ModuleSpecificName => "__TypeGenericInfoMapNode__"; public override ObjectData GetData(NodeFactory factory, bool relocsOnly = false) { diff --git a/src/coreclr/tools/aot/ILCompiler.ReadyToRun/Compiler/DependencyAnalysis/ReadyToRun/TypesTableNode.cs b/src/coreclr/tools/aot/ILCompiler.ReadyToRun/Compiler/DependencyAnalysis/ReadyToRun/TypesTableNode.cs index 1681aad2c8ec5b..45da68f26bd5cc 100644 --- a/src/coreclr/tools/aot/ILCompiler.ReadyToRun/Compiler/DependencyAnalysis/ReadyToRun/TypesTableNode.cs +++ b/src/coreclr/tools/aot/ILCompiler.ReadyToRun/Compiler/DependencyAnalysis/ReadyToRun/TypesTableNode.cs @@ -20,13 +20,8 @@ public class TypesTableNode : ModuleSpecificHeaderTableNode public TypesTableNode(EcmaModule module) : base(module) { } - - public override void AppendMangledName(NameMangler nameMangler, Utf8StringBuilder sb) - { - sb.Append(nameMangler.CompilationUnitPrefix); - sb.Append("__ReadyToRunAvailableTypesTable__"); - sb.Append(_module.Assembly.GetName().Name); - } + + protected override string ModuleSpecificName => "__ReadyToRunAvailableTypesTable__"; public override ObjectData GetData(NodeFactory factory, bool relocsOnly = false) { From ac7ec656688f7b93324dbd72e09adccd2f29c171 Mon Sep 17 00:00:00 2001 From: David Wrighton Date: Tue, 16 May 2023 19:50:04 -0700 Subject: [PATCH 12/23] Add more __ --- .../DependencyAnalysis/ReadyToRun/EnclosingTypeMapNode.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/coreclr/tools/aot/ILCompiler.ReadyToRun/Compiler/DependencyAnalysis/ReadyToRun/EnclosingTypeMapNode.cs b/src/coreclr/tools/aot/ILCompiler.ReadyToRun/Compiler/DependencyAnalysis/ReadyToRun/EnclosingTypeMapNode.cs index 02af4259c7dd2b..984fc83366a260 100644 --- a/src/coreclr/tools/aot/ILCompiler.ReadyToRun/Compiler/DependencyAnalysis/ReadyToRun/EnclosingTypeMapNode.cs +++ b/src/coreclr/tools/aot/ILCompiler.ReadyToRun/Compiler/DependencyAnalysis/ReadyToRun/EnclosingTypeMapNode.cs @@ -37,7 +37,7 @@ public static bool IsSupported(MetadataReader metadata) public override int ClassCode => 990540812; - protected override string ModuleSpecificName => "__EnclosingTypeMap"; + protected override string ModuleSpecificName => "__EnclosingTypeMap__"; public override ObjectData GetData(NodeFactory factory, bool relocsOnly = false) { From 1a57a0a6922b9fe8a6f5162b4a2ab6a1c1055530 Mon Sep 17 00:00:00 2001 From: David Wrighton Date: Thu, 18 May 2023 17:04:54 -0700 Subject: [PATCH 13/23] Initial pass at type skip verifation checker --- .../TypeSystem/Common/SignatureVariable.cs | 4 + .../ReadyToRun/HeaderNode.cs | 33 +- .../ReadyToRun/TypeValidationChecker.cs | 452 ++++++++++++++++++ .../ReadyToRunCodegenNodeFactory.cs | 8 +- .../Compiler/ReadyToRunCodegenCompilation.cs | 6 +- .../ReadyToRunCodegenCompilationBuilder.cs | 6 +- .../ILCompiler.ReadyToRun.csproj | 1 + .../aot/crossgen2/Crossgen2RootCommand.cs | 4 +- .../aot/crossgen2/Properties/Resources.resx | 2 +- 9 files changed, 500 insertions(+), 16 deletions(-) create mode 100644 src/coreclr/tools/aot/ILCompiler.ReadyToRun/Compiler/DependencyAnalysis/ReadyToRun/TypeValidationChecker.cs diff --git a/src/coreclr/tools/Common/TypeSystem/Common/SignatureVariable.cs b/src/coreclr/tools/Common/TypeSystem/Common/SignatureVariable.cs index 4a26cb619d57bc..0b72c0ef657c93 100644 --- a/src/coreclr/tools/Common/TypeSystem/Common/SignatureVariable.cs +++ b/src/coreclr/tools/Common/TypeSystem/Common/SignatureVariable.cs @@ -66,6 +66,10 @@ protected override TypeFlags ComputeTypeFlags(TypeFlags mask) flags |= TypeFlags.SignatureTypeVariable; } + flags |= TypeFlags.HasGenericVarianceComputed; + flags |= TypeFlags.HasFinalizerComputed; + flags |= TypeFlags.AttributeCacheComputed; + return flags; } diff --git a/src/coreclr/tools/aot/ILCompiler.ReadyToRun/Compiler/DependencyAnalysis/ReadyToRun/HeaderNode.cs b/src/coreclr/tools/aot/ILCompiler.ReadyToRun/Compiler/DependencyAnalysis/ReadyToRun/HeaderNode.cs index d27731d68f21ff..fdafc9198b79d1 100644 --- a/src/coreclr/tools/aot/ILCompiler.ReadyToRun/Compiler/DependencyAnalysis/ReadyToRun/HeaderNode.cs +++ b/src/coreclr/tools/aot/ILCompiler.ReadyToRun/Compiler/DependencyAnalysis/ReadyToRun/HeaderNode.cs @@ -10,6 +10,7 @@ using Internal.TypeSystem.Ecma; using Internal.ReadyToRunConstants; using ILCompiler.DependencyAnalysisFramework; +using System.Threading.Tasks; namespace ILCompiler.DependencyAnalysis.ReadyToRun { @@ -96,9 +97,20 @@ public HeaderItem(ReadyToRunSectionType id, DependencyNodeCore node private readonly List _items = new List(); private readonly ReadyToRunFlags _flags; + private readonly Task _shouldAddSkipTypeValidationFlag; + private readonly TypeValidationChecker _typeValidationChecker = new TypeValidationChecker(); - public HeaderNode(ReadyToRunFlags flags) + public HeaderNode(ReadyToRunFlags flags, EcmaModule moduleToCheckForSkipTypeValidation) { + + if (moduleToCheckForSkipTypeValidation != null) + { + _shouldAddSkipTypeValidationFlag = _typeValidationChecker.CanSkipValidation(moduleToCheckForSkipTypeValidation); + } + else + { + _shouldAddSkipTypeValidationFlag = Task.FromResult(false); + } _flags = flags; } @@ -139,7 +151,18 @@ public override ObjectData GetData(NodeFactory factory, bool relocsOnly = false) _items.MergeSort((x, y) => Comparer.Default.Compare((int)x.Id, (int)y.Id)); // ReadyToRunHeader.Flags - builder.EmitInt((int)_flags); + int flagsInt = (int)_flags; + if (_shouldAddSkipTypeValidationFlag.Result) + { + flagsInt |= (int)ReadyToRunFlags.READYTORUN_FLAG_SkipTypeValidation; + } + else + { +//#if DIAGNOSE_TYPE_VALIDATION_RULES + _typeValidationChecker.LogErrors(System.Console.WriteLine); +//#endif + } + builder.EmitInt(flagsInt); // ReadyToRunHeader.NumberOfSections ObjectDataBuilder.Reservation sectionCountReservation = builder.ReserveInt(); @@ -182,8 +205,8 @@ public override ObjectData GetData(NodeFactory factory, bool relocsOnly = false) public class GlobalHeaderNode : HeaderNode { - public GlobalHeaderNode(ReadyToRunFlags flags) - : base(flags) + public GlobalHeaderNode(ReadyToRunFlags flags, EcmaModule moduleToCheckForSkipValidation) + : base(flags, moduleToCheckForSkipValidation) { } @@ -211,7 +234,7 @@ public class AssemblyHeaderNode : HeaderNode private readonly int _index; public AssemblyHeaderNode(ReadyToRunFlags flags, int index) - : base(flags) + : base(flags, null) { _index = index; } diff --git a/src/coreclr/tools/aot/ILCompiler.ReadyToRun/Compiler/DependencyAnalysis/ReadyToRun/TypeValidationChecker.cs b/src/coreclr/tools/aot/ILCompiler.ReadyToRun/Compiler/DependencyAnalysis/ReadyToRun/TypeValidationChecker.cs new file mode 100644 index 00000000000000..52e5c5622a870d --- /dev/null +++ b/src/coreclr/tools/aot/ILCompiler.ReadyToRun/Compiler/DependencyAnalysis/ReadyToRun/TypeValidationChecker.cs @@ -0,0 +1,452 @@ +// Licensed to the .NET Foundation under one or more agreements. +// The .NET Foundation licenses this file to you under the MIT license. + +using System; +using System.Collections.Concurrent; +using System.Collections.Generic; +using System.Linq; +using System.Reflection; +using System.Text; +using System.Threading.Tasks; +using ILCompiler.Dataflow; +using Internal.TypeSystem; +using Internal.TypeSystem.Ecma; + +namespace ILCompiler.DependencyAnalysis.ReadyToRun +{ + internal class TypeValidationChecker + { + ConcurrentBag<(TypeDesc type, string reason)> _typeLoadValidationErrors = new ConcurrentBag<(TypeDesc type, string reason)>(); + + public void LogErrors(Action loggingFunction) + { + var typeLoadValidationErrors = _typeLoadValidationErrors.ToArray(); + Array.Sort(typeLoadValidationErrors, (ValueTuple left, ValueTuple right) => + { + return TypeSystemComparer.Instance.Compare(left.Item1, right.Item1); + }); + loggingFunction($"{_typeLoadValidationErrors.Count} type validation errors"); + loggingFunction("------"); + foreach (var reason in typeLoadValidationErrors) + { + loggingFunction($"{reason.type}: {reason.reason}"); + } + loggingFunction("------"); + } + + public async Task CanSkipValidation(EcmaModule module) + { + // The system module can always skip type validation +#if !DEBUG + if (module == module.Context.SystemModule) + return true; +#endif + + List> typesInModuleSkipValidationChecking = new List>(); + foreach (var type in module.GetAllTypes()) + { + if (type is EcmaType ecmaType) + typesInModuleSkipValidationChecking.Add(CanSkipValidation(ecmaType)); + } + typesInModuleSkipValidationChecking.Add(CanSkipValidation((EcmaType)module.GetGlobalModuleType())); + + await Task.WhenAll(typesInModuleSkipValidationChecking); + foreach (var type in typesInModuleSkipValidationChecking) + { + if (!type.Result) + return false; + } + return true; + } + + private ConcurrentDictionary> _skipValidationDict = new ConcurrentDictionary>(); + private Task CanSkipValidation(EcmaType type) + { + if (_skipValidationDict.TryGetValue(type, out var result)) return result; + Task skipValidatorForType = Task.Run(() => CanSkipValidationWorker(type)); + _skipValidationDict.TryAdd(type, skipValidatorForType); + return skipValidatorForType; + } + + public Task CanSkipValidation(TypeDesc type) + { + if (type == null) + return Task.FromResult(true); + + if (type is EcmaType ecmaType) + return CanSkipValidation(ecmaType); + else if (type is InstantiatedType instantiatedType) + return CanSkipValidation(instantiatedType); + else if (type is ParameterizedType parameterizedType) + return CanSkipValidation(parameterizedType.ParameterType); + else if (type is FunctionPointerType functionPointerType) + return CanSkipValidation(functionPointerType); + return Task.FromResult(true); + } + + private Task CanSkipValidation(InstantiatedType instantiatedType) + { + try + { + // Constraints should be satisfied + if (!instantiatedType.CheckConstraints()) + { + _typeLoadValidationErrors.Add((instantiatedType, "Constraint check failed")); + return Task.FromResult(false); + } + + return CanSkipValidation(instantiatedType.GetTypeDefinition()); + } + catch (Exception ex) + { + _typeLoadValidationErrors.Add((instantiatedType, $"due to exception '{ex}'")); + return Task.FromResult(false); + } +/* + if (_skipValidationDict.TryGetValue(instantiatedType, out var result)) return result; + Task skipValidatorForType = Task.Run(() => CanSkipValidationWorker(instantiatedType)); + _skipValidationDict.TryAdd(instantiatedType, skipValidatorForType); + return skipValidatorForType;*/ + } + + private async Task CanSkipValidationWorker(InstantiatedType instantiatedType) + { + try + { + // Constraints should be satisfied + if (!instantiatedType.CheckConstraints()) + { + _typeLoadValidationErrors.Add((instantiatedType, "Constraint check failed")); + return false; + } + + return await CanSkipValidation(instantiatedType.GetTypeDefinition()); + } + catch (Exception ex) + { + _typeLoadValidationErrors.Add((instantiatedType, $"due to exception '{ex}'")); + return false; + } + } + + private async Task CanSkipValidation(FunctionPointerType functionPointerType) + { + if (!await CanSkipValidation(functionPointerType.Signature.ReturnType)) + return false; + foreach (var type in functionPointerType.Signature) + { + if (!await CanSkipValidation(type)) + return false; + } + return true; + } + + private async Task CanSkipValidationWorker(EcmaType type) + { + Task CanSkipValidationWorkerHelper(TypeDesc typeToCheckForSkipValidation) + { + return CanSkipValidation(typeToCheckForSkipValidation.InstantiateSignature(type.Instantiation, new Instantiation())); + } + // The runtime has a number of checks in the type loader which it will skip running if the SkipValidation flag is set + // This function attempts to document all of them, and implement *some* of them. + + // This function performs a portion of the validation skipping that has been found to have some importance, or to serve as + // In addition, there are comments about all validation skipping activities that the runtime will perform. + try + { + var typeDef = type.MetadataReader.GetTypeDefinition(type.Handle); + + // Validate that the base type is loadable + if (type.BaseType != null) + { + if (!await CanSkipValidationWorkerHelper(type.BaseType)) + { + _typeLoadValidationErrors.Add((type, "BaseType failed validation")); + return false; + } + } + + // Validate that the base type is accessible to this type -- UNIMPLEMENTED + + foreach (var interfaceType in type.RuntimeInterfaces) + { + // Validate that the all referenced interface types are loadable + if (!await CanSkipValidationWorkerHelper(interfaceType)) + { + _typeLoadValidationErrors.Add((type, $"Interface type {interfaceType} failed validation")); + return false; + } + } + + // Validate that each interface type explicitly implemented on this type is accessible to this type -- UNIMPLEMENTED + + foreach (var field in type.GetFields()) + { + // Validate that all fields on the type are both loadable + if (!await CanSkipValidationWorkerHelper(field.FieldType)) + { + _typeLoadValidationErrors.Add((type, $"Field {field.Name}'s type failed validation")); + return false; + } + + // Validate that all fields on the type are accessible -- UNIMPLEMENTED + } + + // Per method rules + foreach (var methodDesc in type.GetMethods()) + { + var method = (EcmaMethod)methodDesc; + var methodDef = method.MetadataReader.GetMethodDefinition(method.Handle); + // Validate that the validateTokenSig algorithm on all methods defined on the type + // The validateTokenSig algorithm simply validates the phyical structure of the signature. Getting a MethodSignature object is a more complete check + try + { + var getSignature = method.Signature; + } + catch + { + _typeLoadValidationErrors.Add((type, $"Signature could not be loaded for method {method.Name}")); + return false; + } + + // Validate that enums have no methods + if (type.IsEnum) + { + _typeLoadValidationErrors.Add((type, $"Is enum type with a method")); + return false; + } + + // Validate that if the method has an RVA that (the Class is not a ComImport class, it is not abstract, it is not marked with the miRuntime flag, and is not marked as InternalCall) + if (methodDef.RelativeVirtualAddress != 0) + { + // Validate that if the method has an RVA that the Class is not a ComImport class -- UNIMPLEMENTED + // Validate that if the method has an RVA that the method is not abstract + if (methodDef.Attributes.HasFlag(System.Reflection.MethodAttributes.Abstract)) + { + _typeLoadValidationErrors.Add((type, $"{method} is an abstract method with a non-zero RVA")); + return false; + } + // Validate that if the method has an RVA is not marked with the miRuntime flag + if (methodDef.ImplAttributes.HasFlag(System.Reflection.MethodImplAttributes.Runtime)) + { + _typeLoadValidationErrors.Add((type, $"{method} is an miRuntime method with a non-zero RVA")); + return false; + } + // Validate that if the method has an RVA is not marked as InternalCall + if (methodDef.ImplAttributes.HasFlag(System.Reflection.MethodImplAttributes.InternalCall)) + { + _typeLoadValidationErrors.Add((type, $"{method} is an internal call method with a non-zero RVA")); + return false; + } + } + // Validate that abstract methods cannot exist on non-abstract classes + if (method.IsAbstract && !type.IsAbstract) + { + _typeLoadValidationErrors.Add((type, $"abstract method {method} defined on non-abstract type")); + return false; + } + // Validate that for instance methods, the abstract flag can only be set on a virtual method. + if (!methodDef.Attributes.HasFlag(MethodAttributes.Static) && method.IsAbstract) + { + if (!method.IsVirtual) + { + _typeLoadValidationErrors.Add((type, $"instance abstract method {method} not marked as virtual")); + return false; + } + } + // Validate that interfaces can only have rtSpecialName methods which are "_VtblGap" or ".cctor" methods + if (type.IsInterface) + { + if (method.IsSpecialName || methodDef.Attributes.HasFlag(MethodAttributes.RTSpecialName)) + { + if ((method.Name != ".cctor") && !method.Name.StartsWith("_VtblGap")) + { + _typeLoadValidationErrors.Add((type, $"Special name method {method} defined on interface")); + return false; + } + } + } + if (method.IsVirtual) + { + // Validate that if a method is virtual that it cannot be a p/invoke + if (method.IsPInvoke) + { + _typeLoadValidationErrors.Add((type, $"'{method}' is both virtual and a p/invoke")); + return false; + } + // Validate that if a method is virtual and static it can only exist on an interface + if (methodDef.Attributes.HasFlag(MethodAttributes.Static) && !type.IsInterface) + { + _typeLoadValidationErrors.Add((type, $"'{method}' is a virtual static method not defined on an interface")); + return false; + } + // Validate that constructors cannot be marked as virtual + if (method.IsConstructor || method.IsStaticConstructor) + { + _typeLoadValidationErrors.Add((type, $"'{method}' is a virtual constructor")); + return false; + } + } + // Validate that no synchronized methods may exist on a value type + if (type.IsValueType) + { + if (method.IsSynchronized) + { + _typeLoadValidationErrors.Add((type, $"'{method}' is synchronized method on a value type")); + return false; + } + } + // validate that the global class cannot have instance methods + if (type.EcmaModule.GetGlobalModuleType() == type && !methodDef.Attributes.HasFlag(MethodAttributes.Static)) + { + _typeLoadValidationErrors.Add((type, $"'{method}' is an instance method defined on the global type")); + return false; + } + // Validate that a generic method cannot be on a ComImport class, or a ComEventInterface -- UNIMPLEMENTED + // Validate that a generic method cannot be a p/invoke + if (method.IsPInvoke) + { + if (type.HasInstantiation) + { + _typeLoadValidationErrors.Add((type, $"'{method}' is an pinvoke defined on a generic type")); + return false; + } + if (method.HasInstantiation) + { + _typeLoadValidationErrors.Add((type, $"'{method}' is an pinvoke defined as a generic method")); + return false; + } + } + // Validate that outside of CoreLib, that a generic method cannot be an internal call method + if (type.Context.SystemModule != type.Module && method.IsInternalCall) + { + if (method.HasInstantiation) + { + _typeLoadValidationErrors.Add((type, $"'{method}' is an internal call generic method")); + return false; + } + if (type.HasInstantiation) + { + _typeLoadValidationErrors.Add((type, $"'{method}' is an internal call method on generic type")); + return false; + } + } + // Validate that a generic method cannot be marked as runtime + if (method.HasInstantiation && method.IsRuntimeImplemented) + { + _typeLoadValidationErrors.Add((type, $"'{method}' is an runtime-impl generic method")); + return false; + } + // Validate that generic variance is properly respected in method signatures -- UNIMPLEMENTED + // Validate that there are no cyclical method constraints -- UNIMPLEMENTED + // Validate that constraints are all acccessible to the method using them -- UNIMPLEMENTED + + } + + // Generic class special rules + // Validate that a generic class cannot be a ComImport class, or a ComEventInterface class -- UNIMPLEMENTED + // Validate that there are no cyclical class or method constraints, and that constraints are all acccessible to the type using them -- UNIMPLEMENTED + + // Override rules + // Validate that for every override involving generic methods that the generic method constraints are matching. -- UNIMPLEMENTED + // Validate that each override results does not violate accessibility rules + + foreach (var methodImplHandle in typeDef.GetMethodImplementations()) + { + var methodImpl = type.MetadataReader.GetMethodImplementation(methodImplHandle); + var methodBody = type.EcmaModule.GetMethod(methodImpl.MethodBody); + var methodDecl = type.EcmaModule.GetMethod(methodImpl.MethodDeclaration); + + // Validate that all MethodImpls actually match signatures closely enough + if (!methodDecl.Signature.EqualsWithCovariantReturnType(methodBody.Signature)) + { + _typeLoadValidationErrors.Add((type, $"MethodImpl with Body '{methodBody}' and Decl '{methodDecl}' do not have matching signatures")); + return false; + } + + if (!methodDecl.IsVirtual) + { + _typeLoadValidationErrors.Add((type, $"MethodImpl with Decl '{methodDecl}' points at non-virtual decl method")); + return false; + } + + if (!methodDecl.IsFinal) + { + _typeLoadValidationErrors.Add((type, $"MethodImpl with Decl '{methodDecl}' points at sealed decl method")); + return false; + } + + bool isStatic = methodBody.Signature.IsStatic; + if (methodBody.OwningType.IsInterface && !isStatic && !methodBody.IsFinal) + { + _typeLoadValidationErrors.Add((type, $"MethodImpl with Body '{methodBody}' and Decl '{methodDecl}' implements interface on another interface with a non-sealed method")); + return false; + } + + if (isStatic && methodBody.IsVirtual) + { + _typeLoadValidationErrors.Add((type, $"MethodImpl with Body '{methodBody}' and Decl '{methodDecl}' implements a static virtual method with a virtual static method")); + return false; + } + + if (!isStatic && !methodBody.IsVirtual) + { + _typeLoadValidationErrors.Add((type, $"MethodImpl with Body '{methodBody}' and Decl '{methodDecl}' implements a instance virtual method with a non-virtual instance method")); + return false; + } + + // Validate that the MethodImpl follows MethodImpl accessibility rules -- UNIMPLEMENTED + } + + // Validate that all virtual static methods are actually implemented if the type is not abstract + // Validate that all virtual instance methods are actually implemented if the type is concrete (Not abstract or interface) + bool isConcrete = !type.IsInterface && !type.IsAbstract; + var virtualMethodAlgorithm = type.Context.GetVirtualMethodAlgorithmForType(type); + foreach (var interfaceImplemented in type.RuntimeInterfaces) + { + foreach (var interfaceMethod in interfaceImplemented.GetVirtualMethods()) + { + if (interfaceMethod.Signature.IsStatic) + { + var resolvedMethod = virtualMethodAlgorithm.ResolveInterfaceMethodToStaticVirtualMethodOnType(interfaceMethod, type); + if (null == resolvedMethod || resolvedMethod.IsAbstract) + { + if (virtualMethodAlgorithm.ResolveInterfaceMethodToDefaultImplementationOnType(interfaceMethod, type, out var impl) != DefaultInterfaceMethodResolution.DefaultImplementation || impl.IsAbstract) + { + _typeLoadValidationErrors.Add((type, $"Interface method '{interfaceMethod}' does not have implementation")); + return false; + } + } + } + else + { + if (null == virtualMethodAlgorithm.ResolveInterfaceMethodToVirtualMethodOnType(interfaceMethod, type)) + { + _typeLoadValidationErrors.Add((type, $"Interface method '{interfaceMethod}' does not have implementation")); + return false; + } + } + } + } + + foreach (var virtualMethod in type.GetAllVirtualMethods()) + { + var implementationMethod = virtualMethodAlgorithm.FindVirtualFunctionTargetMethodOnObjectType(virtualMethod, type); + if (implementationMethod == null || implementationMethod.IsAbstract) + { + _typeLoadValidationErrors.Add((type, $"Interface method '{virtualMethod}' does not have implementation")); + return false; + } + } + + return true; + } + catch (Exception ex) + { + // If we throw an exception, clearly type validation skipping was not to be + _typeLoadValidationErrors.Add((type, ex.ToString())); + return false; + } + } + } +} diff --git a/src/coreclr/tools/aot/ILCompiler.ReadyToRun/Compiler/DependencyAnalysis/ReadyToRunCodegenNodeFactory.cs b/src/coreclr/tools/aot/ILCompiler.ReadyToRun/Compiler/DependencyAnalysis/ReadyToRunCodegenNodeFactory.cs index 9c3849efaf5877..2f523b9a77ac1e 100644 --- a/src/coreclr/tools/aot/ILCompiler.ReadyToRun/Compiler/DependencyAnalysis/ReadyToRunCodegenNodeFactory.cs +++ b/src/coreclr/tools/aot/ILCompiler.ReadyToRun/Compiler/DependencyAnalysis/ReadyToRunCodegenNodeFactory.cs @@ -47,7 +47,7 @@ public TValue GetOrAdd(TKey key) public sealed class NodeFactoryOptimizationFlags { public bool OptimizeAsyncMethods; - public bool SkipTypeValidation; + public bool? SkipTypeValidation; } // To make the code future compatible to the composite R2R story @@ -187,7 +187,8 @@ public NodeFactory( ResourceData win32Resources, ReadyToRunFlags flags, NodeFactoryOptimizationFlags nodeFactoryOptimizationFlags, - ulong imageBase) + ulong imageBase, + EcmaModule associatedModule) { OptimizationFlags = nodeFactoryOptimizationFlags; TypeSystemContext = context; @@ -199,7 +200,8 @@ public NodeFactory( CopiedCorHeaderNode = corHeaderNode; DebugDirectoryNode = debugDirectoryNode; Resolver = compilationModuleGroup.Resolver; - Header = new GlobalHeaderNode(flags); + + Header = new GlobalHeaderNode(flags, associatedModule); ImageBase = imageBase; if (!win32Resources.IsEmpty) Win32ResourcesNode = new Win32ResourcesNode(win32Resources); diff --git a/src/coreclr/tools/aot/ILCompiler.ReadyToRun/Compiler/ReadyToRunCodegenCompilation.cs b/src/coreclr/tools/aot/ILCompiler.ReadyToRun/Compiler/ReadyToRunCodegenCompilation.cs index dde8c9a87f4d45..ea7343d5e5325a 100644 --- a/src/coreclr/tools/aot/ILCompiler.ReadyToRun/Compiler/ReadyToRunCodegenCompilation.cs +++ b/src/coreclr/tools/aot/ILCompiler.ReadyToRun/Compiler/ReadyToRunCodegenCompilation.cs @@ -407,7 +407,8 @@ private void RewriteComponentFile(string inputFile, string outputFile, string ow { flags |= ReadyToRunFlags.READYTORUN_FLAG_PlatformNeutralSource; } - if (_nodeFactory.OptimizationFlags.SkipTypeValidation) + if (_nodeFactory.OptimizationFlags.SkipTypeValidation.HasValue && + _nodeFactory.OptimizationFlags.SkipTypeValidation.Value) { flags |= ReadyToRunFlags.READYTORUN_FLAG_SkipTypeValidation; } @@ -428,7 +429,8 @@ private void RewriteComponentFile(string inputFile, string outputFile, string ow win32Resources: new Win32Resources.ResourceData(inputModule), flags, _nodeFactory.OptimizationFlags, - _nodeFactory.ImageBase); + _nodeFactory.ImageBase, + _nodeFactory.OptimizationFlags.SkipTypeValidation.HasValue ? null : inputModule); IComparer> comparer = new SortableDependencyNode.ObjectNodeComparer(CompilerComparer.Instance); DependencyAnalyzerBase componentGraph = new DependencyAnalyzer, NodeFactory>(componentFactory, comparer); diff --git a/src/coreclr/tools/aot/ILCompiler.ReadyToRun/Compiler/ReadyToRunCodegenCompilationBuilder.cs b/src/coreclr/tools/aot/ILCompiler.ReadyToRun/Compiler/ReadyToRunCodegenCompilationBuilder.cs index 1169e6508ad610..34d31537cea5bd 100644 --- a/src/coreclr/tools/aot/ILCompiler.ReadyToRun/Compiler/ReadyToRunCodegenCompilationBuilder.cs +++ b/src/coreclr/tools/aot/ILCompiler.ReadyToRun/Compiler/ReadyToRunCodegenCompilationBuilder.cs @@ -242,7 +242,7 @@ public override ICompilation ToCompilation() { flags |= ReadyToRunFlags.READYTORUN_FLAG_PlatformNeutralSource; } - if (_nodeFactoryOptimizationFlags.SkipTypeValidation) + if (_nodeFactoryOptimizationFlags.SkipTypeValidation.HasValue && _nodeFactoryOptimizationFlags.SkipTypeValidation.Value) { flags |= ReadyToRunFlags.READYTORUN_FLAG_SkipTypeValidation; } @@ -258,8 +258,8 @@ public override ICompilation ToCompilation() win32Resources, flags, _nodeFactoryOptimizationFlags, - _imageBase - ); + _imageBase, + _nodeFactoryOptimizationFlags.SkipTypeValidation.HasValue ? null : singleModule); factory.CompositeImageSettings = _compositeImageSettings; diff --git a/src/coreclr/tools/aot/ILCompiler.ReadyToRun/ILCompiler.ReadyToRun.csproj b/src/coreclr/tools/aot/ILCompiler.ReadyToRun/ILCompiler.ReadyToRun.csproj index df753c01a79aa4..afa9daff0ea19c 100644 --- a/src/coreclr/tools/aot/ILCompiler.ReadyToRun/ILCompiler.ReadyToRun.csproj +++ b/src/coreclr/tools/aot/ILCompiler.ReadyToRun/ILCompiler.ReadyToRun.csproj @@ -148,6 +148,7 @@ + diff --git a/src/coreclr/tools/aot/crossgen2/Crossgen2RootCommand.cs b/src/coreclr/tools/aot/crossgen2/Crossgen2RootCommand.cs index 6511e7587c507c..f2b8738f0b9d18 100644 --- a/src/coreclr/tools/aot/crossgen2/Crossgen2RootCommand.cs +++ b/src/coreclr/tools/aot/crossgen2/Crossgen2RootCommand.cs @@ -37,8 +37,8 @@ internal class Crossgen2RootCommand : RootCommand new(new[] { "--optimize-space", "--Os" }, SR.OptimizeSpaceOption); public Option OptimizeTime { get; } = new(new[] { "--optimize-time", "--Ot" }, SR.OptimizeSpeedOption); - public Option SkipTypeValidation { get; } = - new(new[] { "--skip-type-validation"}, () => true, SR.SkipTypeValidation); + public Option SkipTypeValidation { get; } = + new(new[] { "--skip-type-validation"}, () => null, SR.SkipTypeValidation); public Option InputBubble { get; } = new(new[] { "--inputbubble" }, SR.InputBubbleOption); public Option> InputBubbleReferenceFilePaths { get; } = diff --git a/src/coreclr/tools/aot/crossgen2/Properties/Resources.resx b/src/coreclr/tools/aot/crossgen2/Properties/Resources.resx index 53baa343f11af2..962bd646c2b207 100644 --- a/src/coreclr/tools/aot/crossgen2/Properties/Resources.resx +++ b/src/coreclr/tools/aot/crossgen2/Properties/Resources.resx @@ -400,6 +400,6 @@ .NET Crossgen2 Compiler - Configure the runtime so that it does not validate metadata for types defined in compiled assemblies + Configure the runtime so that it does not validate metadata for types defined in compiled assemblies (default: automatic) \ No newline at end of file From 5f770c6c3abe85494f697ca6ce3f9a5fd53c0e4d Mon Sep 17 00:00:00 2001 From: David Wrighton Date: Fri, 19 May 2023 09:49:22 -0700 Subject: [PATCH 14/23] Fix logging and some correctness issues --- .../ReadyToRun/TypeValidationChecker.cs | 100 ++++++++++-------- 1 file changed, 56 insertions(+), 44 deletions(-) diff --git a/src/coreclr/tools/aot/ILCompiler.ReadyToRun/Compiler/DependencyAnalysis/ReadyToRun/TypeValidationChecker.cs b/src/coreclr/tools/aot/ILCompiler.ReadyToRun/Compiler/DependencyAnalysis/ReadyToRun/TypeValidationChecker.cs index 52e5c5622a870d..e91d9d2d7664b0 100644 --- a/src/coreclr/tools/aot/ILCompiler.ReadyToRun/Compiler/DependencyAnalysis/ReadyToRun/TypeValidationChecker.cs +++ b/src/coreclr/tools/aot/ILCompiler.ReadyToRun/Compiler/DependencyAnalysis/ReadyToRun/TypeValidationChecker.cs @@ -34,6 +34,11 @@ public void LogErrors(Action loggingFunction) loggingFunction("------"); } + private void AddTypeValidationError(TypeDesc type, string error) + { + _typeLoadValidationErrors.Add((type, error)); + } + public async Task CanSkipValidation(EcmaModule module) { // The system module can always skip type validation @@ -91,7 +96,7 @@ private Task CanSkipValidation(InstantiatedType instantiatedType) // Constraints should be satisfied if (!instantiatedType.CheckConstraints()) { - _typeLoadValidationErrors.Add((instantiatedType, "Constraint check failed")); + AddTypeValidationError(instantiatedType, "Constraint check failed"); return Task.FromResult(false); } @@ -99,7 +104,7 @@ private Task CanSkipValidation(InstantiatedType instantiatedType) } catch (Exception ex) { - _typeLoadValidationErrors.Add((instantiatedType, $"due to exception '{ex}'")); + AddTypeValidationError(instantiatedType, $"due to exception '{ex}'"); return Task.FromResult(false); } /* @@ -116,7 +121,7 @@ private async Task CanSkipValidationWorker(InstantiatedType instantiatedTy // Constraints should be satisfied if (!instantiatedType.CheckConstraints()) { - _typeLoadValidationErrors.Add((instantiatedType, "Constraint check failed")); + AddTypeValidationError(instantiatedType, "Constraint check failed"); return false; } @@ -124,7 +129,7 @@ private async Task CanSkipValidationWorker(InstantiatedType instantiatedTy } catch (Exception ex) { - _typeLoadValidationErrors.Add((instantiatedType, $"due to exception '{ex}'")); + AddTypeValidationError(instantiatedType, $"due to exception '{ex}'"); return false; } } @@ -161,7 +166,7 @@ Task CanSkipValidationWorkerHelper(TypeDesc typeToCheckForSkipValidation) { if (!await CanSkipValidationWorkerHelper(type.BaseType)) { - _typeLoadValidationErrors.Add((type, "BaseType failed validation")); + AddTypeValidationError(type, "BaseType failed validation"); return false; } } @@ -173,7 +178,7 @@ Task CanSkipValidationWorkerHelper(TypeDesc typeToCheckForSkipValidation) // Validate that the all referenced interface types are loadable if (!await CanSkipValidationWorkerHelper(interfaceType)) { - _typeLoadValidationErrors.Add((type, $"Interface type {interfaceType} failed validation")); + AddTypeValidationError(type, $"Interface type {interfaceType} failed validation"); return false; } } @@ -205,14 +210,14 @@ Task CanSkipValidationWorkerHelper(TypeDesc typeToCheckForSkipValidation) } catch { - _typeLoadValidationErrors.Add((type, $"Signature could not be loaded for method {method.Name}")); + AddTypeValidationError(type, $"Signature could not be loaded for method {method.Name}"); return false; } // Validate that enums have no methods if (type.IsEnum) { - _typeLoadValidationErrors.Add((type, $"Is enum type with a method")); + AddTypeValidationError(type, $"Is enum type with a method"); return false; } @@ -223,26 +228,26 @@ Task CanSkipValidationWorkerHelper(TypeDesc typeToCheckForSkipValidation) // Validate that if the method has an RVA that the method is not abstract if (methodDef.Attributes.HasFlag(System.Reflection.MethodAttributes.Abstract)) { - _typeLoadValidationErrors.Add((type, $"{method} is an abstract method with a non-zero RVA")); + AddTypeValidationError(type, $"{method} is an abstract method with a non-zero RVA"); return false; } // Validate that if the method has an RVA is not marked with the miRuntime flag if (methodDef.ImplAttributes.HasFlag(System.Reflection.MethodImplAttributes.Runtime)) { - _typeLoadValidationErrors.Add((type, $"{method} is an miRuntime method with a non-zero RVA")); + AddTypeValidationError(type, $"{method} is an miRuntime method with a non-zero RVA"); return false; } // Validate that if the method has an RVA is not marked as InternalCall if (methodDef.ImplAttributes.HasFlag(System.Reflection.MethodImplAttributes.InternalCall)) { - _typeLoadValidationErrors.Add((type, $"{method} is an internal call method with a non-zero RVA")); + AddTypeValidationError(type, $"{method} is an internal call method with a non-zero RVA"); return false; } } // Validate that abstract methods cannot exist on non-abstract classes if (method.IsAbstract && !type.IsAbstract) { - _typeLoadValidationErrors.Add((type, $"abstract method {method} defined on non-abstract type")); + AddTypeValidationError(type, $"abstract method {method} defined on non-abstract type"); return false; } // Validate that for instance methods, the abstract flag can only be set on a virtual method. @@ -250,18 +255,18 @@ Task CanSkipValidationWorkerHelper(TypeDesc typeToCheckForSkipValidation) { if (!method.IsVirtual) { - _typeLoadValidationErrors.Add((type, $"instance abstract method {method} not marked as virtual")); + AddTypeValidationError(type, $"instance abstract method {method} not marked as virtual"); return false; } } // Validate that interfaces can only have rtSpecialName methods which are "_VtblGap" or ".cctor" methods if (type.IsInterface) { - if (method.IsSpecialName || methodDef.Attributes.HasFlag(MethodAttributes.RTSpecialName)) + if (methodDef.Attributes.HasFlag(MethodAttributes.RTSpecialName)) { if ((method.Name != ".cctor") && !method.Name.StartsWith("_VtblGap")) { - _typeLoadValidationErrors.Add((type, $"Special name method {method} defined on interface")); + AddTypeValidationError(type, $"Special name method {method} defined on interface"); return false; } } @@ -271,19 +276,19 @@ Task CanSkipValidationWorkerHelper(TypeDesc typeToCheckForSkipValidation) // Validate that if a method is virtual that it cannot be a p/invoke if (method.IsPInvoke) { - _typeLoadValidationErrors.Add((type, $"'{method}' is both virtual and a p/invoke")); + AddTypeValidationError(type, $"'{method}' is both virtual and a p/invoke"); return false; } // Validate that if a method is virtual and static it can only exist on an interface if (methodDef.Attributes.HasFlag(MethodAttributes.Static) && !type.IsInterface) { - _typeLoadValidationErrors.Add((type, $"'{method}' is a virtual static method not defined on an interface")); + AddTypeValidationError(type, $"'{method}' is a virtual static method not defined on an interface"); return false; } // Validate that constructors cannot be marked as virtual if (method.IsConstructor || method.IsStaticConstructor) { - _typeLoadValidationErrors.Add((type, $"'{method}' is a virtual constructor")); + AddTypeValidationError(type, $"'{method}' is a virtual constructor"); return false; } } @@ -292,14 +297,14 @@ Task CanSkipValidationWorkerHelper(TypeDesc typeToCheckForSkipValidation) { if (method.IsSynchronized) { - _typeLoadValidationErrors.Add((type, $"'{method}' is synchronized method on a value type")); + AddTypeValidationError(type, $"'{method}' is synchronized method on a value type"); return false; } } // validate that the global class cannot have instance methods if (type.EcmaModule.GetGlobalModuleType() == type && !methodDef.Attributes.HasFlag(MethodAttributes.Static)) { - _typeLoadValidationErrors.Add((type, $"'{method}' is an instance method defined on the global type")); + AddTypeValidationError(type, $"'{method}' is an instance method defined on the global type"); return false; } // Validate that a generic method cannot be on a ComImport class, or a ComEventInterface -- UNIMPLEMENTED @@ -308,12 +313,12 @@ Task CanSkipValidationWorkerHelper(TypeDesc typeToCheckForSkipValidation) { if (type.HasInstantiation) { - _typeLoadValidationErrors.Add((type, $"'{method}' is an pinvoke defined on a generic type")); + AddTypeValidationError(type, $"'{method}' is an pinvoke defined on a generic type"); return false; } if (method.HasInstantiation) { - _typeLoadValidationErrors.Add((type, $"'{method}' is an pinvoke defined as a generic method")); + AddTypeValidationError(type, $"'{method}' is an pinvoke defined as a generic method"); return false; } } @@ -322,19 +327,19 @@ Task CanSkipValidationWorkerHelper(TypeDesc typeToCheckForSkipValidation) { if (method.HasInstantiation) { - _typeLoadValidationErrors.Add((type, $"'{method}' is an internal call generic method")); + AddTypeValidationError(type, $"'{method}' is an internal call generic method"); return false; } if (type.HasInstantiation) { - _typeLoadValidationErrors.Add((type, $"'{method}' is an internal call method on generic type")); + AddTypeValidationError(type, $"'{method}' is an internal call method on generic type"); return false; } } // Validate that a generic method cannot be marked as runtime if (method.HasInstantiation && method.IsRuntimeImplemented) { - _typeLoadValidationErrors.Add((type, $"'{method}' is an runtime-impl generic method")); + AddTypeValidationError(type, $"'{method}' is an runtime-impl generic method"); return false; } // Validate that generic variance is properly respected in method signatures -- UNIMPLEMENTED @@ -358,40 +363,40 @@ Task CanSkipValidationWorkerHelper(TypeDesc typeToCheckForSkipValidation) var methodDecl = type.EcmaModule.GetMethod(methodImpl.MethodDeclaration); // Validate that all MethodImpls actually match signatures closely enough - if (!methodDecl.Signature.EqualsWithCovariantReturnType(methodBody.Signature)) + if (!methodBody.Signature.EqualsWithCovariantReturnType(methodDecl.Signature)) { - _typeLoadValidationErrors.Add((type, $"MethodImpl with Body '{methodBody}' and Decl '{methodDecl}' do not have matching signatures")); + AddTypeValidationError(type, $"MethodImpl with Body '{methodBody}' and Decl '{methodDecl}' do not have matching signatures"); return false; } if (!methodDecl.IsVirtual) { - _typeLoadValidationErrors.Add((type, $"MethodImpl with Decl '{methodDecl}' points at non-virtual decl method")); + AddTypeValidationError(type, $"MethodImpl with Decl '{methodDecl}' points at non-virtual decl method"); return false; } - if (!methodDecl.IsFinal) + if (methodDecl.IsFinal) { - _typeLoadValidationErrors.Add((type, $"MethodImpl with Decl '{methodDecl}' points at sealed decl method")); + AddTypeValidationError(type, $"MethodImpl with Decl '{methodDecl}' points at sealed decl method"); return false; } bool isStatic = methodBody.Signature.IsStatic; if (methodBody.OwningType.IsInterface && !isStatic && !methodBody.IsFinal) { - _typeLoadValidationErrors.Add((type, $"MethodImpl with Body '{methodBody}' and Decl '{methodDecl}' implements interface on another interface with a non-sealed method")); + AddTypeValidationError(type, $"MethodImpl with Body '{methodBody}' and Decl '{methodDecl}' implements interface on another interface with a non-sealed method"); return false; } if (isStatic && methodBody.IsVirtual) { - _typeLoadValidationErrors.Add((type, $"MethodImpl with Body '{methodBody}' and Decl '{methodDecl}' implements a static virtual method with a virtual static method")); + AddTypeValidationError(type, $"MethodImpl with Body '{methodBody}' and Decl '{methodDecl}' implements a static virtual method with a virtual static method"); return false; } if (!isStatic && !methodBody.IsVirtual) { - _typeLoadValidationErrors.Add((type, $"MethodImpl with Body '{methodBody}' and Decl '{methodDecl}' implements a instance virtual method with a non-virtual instance method")); + AddTypeValidationError(type, $"MethodImpl with Body '{methodBody}' and Decl '{methodDecl}' implements a instance virtual method with a non-virtual instance method"); return false; } @@ -399,30 +404,36 @@ Task CanSkipValidationWorkerHelper(TypeDesc typeToCheckForSkipValidation) } // Validate that all virtual static methods are actually implemented if the type is not abstract - // Validate that all virtual instance methods are actually implemented if the type is concrete (Not abstract or interface) - bool isConcrete = !type.IsInterface && !type.IsAbstract; + // Validate that all virtual instance methods are actually implemented if the type is not abstract + if (!type.IsAbstract) + { var virtualMethodAlgorithm = type.Context.GetVirtualMethodAlgorithmForType(type); foreach (var interfaceImplemented in type.RuntimeInterfaces) { foreach (var interfaceMethod in interfaceImplemented.GetVirtualMethods()) { - if (interfaceMethod.Signature.IsStatic) - { - var resolvedMethod = virtualMethodAlgorithm.ResolveInterfaceMethodToStaticVirtualMethodOnType(interfaceMethod, type); + MethodDesc resolvedMethod; + bool staticInteraceMethod = interfaceMethod.Signature.IsStatic; + if (staticInteraceMethod) if (null == resolvedMethod || resolvedMethod.IsAbstract) { if (virtualMethodAlgorithm.ResolveInterfaceMethodToDefaultImplementationOnType(interfaceMethod, type, out var impl) != DefaultInterfaceMethodResolution.DefaultImplementation || impl.IsAbstract) { - _typeLoadValidationErrors.Add((type, $"Interface method '{interfaceMethod}' does not have implementation")); + resolvedMethod = virtualMethodAlgorithm.ResolveInterfaceMethodToStaticVirtualMethodOnType(interfaceMethod, type); return false; } } } else { - if (null == virtualMethodAlgorithm.ResolveInterfaceMethodToVirtualMethodOnType(interfaceMethod, type)) + resolvedMethod = type.ResolveInterfaceMethodTarget(interfaceMethod); + } + + if (null == resolvedMethod || (staticInteraceMethod && resolvedMethod.IsAbstract)) { - _typeLoadValidationErrors.Add((type, $"Interface method '{interfaceMethod}' does not have implementation")); + if (virtualMethodAlgorithm.ResolveInterfaceMethodToDefaultImplementationOnType(interfaceMethod, type, out var impl) != DefaultInterfaceMethodResolution.DefaultImplementation || impl.IsAbstract) + { + AddTypeValidationError(type, $"Interface method '{interfaceMethod}' does not have implementation"); return false; } } @@ -432,19 +443,20 @@ Task CanSkipValidationWorkerHelper(TypeDesc typeToCheckForSkipValidation) foreach (var virtualMethod in type.GetAllVirtualMethods()) { var implementationMethod = virtualMethodAlgorithm.FindVirtualFunctionTargetMethodOnObjectType(virtualMethod, type); - if (implementationMethod == null || implementationMethod.IsAbstract) + if (implementationMethod == null || implementationMethod.IsAbstract) { - _typeLoadValidationErrors.Add((type, $"Interface method '{virtualMethod}' does not have implementation")); + AddTypeValidationError(type, $"Interface method '{virtualMethod}' does not have implementation"); return false; } } + } return true; } catch (Exception ex) { // If we throw an exception, clearly type validation skipping was not to be - _typeLoadValidationErrors.Add((type, ex.ToString())); + AddTypeValidationError(type, $"due to exception '{ex.ToString()}'"); return false; } } From 52bd2202c93f2fbee1ece098c36e5e05992199ae Mon Sep 17 00:00:00 2001 From: David Wrighton Date: Fri, 19 May 2023 17:03:03 -0700 Subject: [PATCH 15/23] Enable the more of type checking - Notably, the recursive stuff now works - Also fix a bug in constraint checking involving open types in the type system --- .../Common/TypeSystem/Common/CastingHelper.cs | 12 +- .../TypeSystem/Common/GenericParameterDesc.cs | 6 + .../TypeSystem/Ecma/EcmaGenericParameter.cs | 19 +- .../ReadyToRun/HeaderNode.cs | 12 +- .../ReadyToRun/TypeValidationChecker.cs | 214 ++++++++---------- 5 files changed, 141 insertions(+), 122 deletions(-) diff --git a/src/coreclr/tools/Common/TypeSystem/Common/CastingHelper.cs b/src/coreclr/tools/Common/TypeSystem/Common/CastingHelper.cs index 85f3799dfa1804..cb73f69dc77c3e 100644 --- a/src/coreclr/tools/Common/TypeSystem/Common/CastingHelper.cs +++ b/src/coreclr/tools/Common/TypeSystem/Common/CastingHelper.cs @@ -195,9 +195,19 @@ private static bool CanCastGenericParameterTo(this GenericParameterDesc thisType return true; } + InstantiationContext context; + if (thisType.AssociatedTypeOrMethod is MethodDesc method) + { + context = new InstantiationContext(method.OwningType.Instantiation, method.Instantiation); + } + else + { + context = new InstantiationContext(((TypeDesc)thisType.AssociatedTypeOrMethod).Instantiation, default(Instantiation)); + } foreach (var typeConstraint in thisType.TypeConstraints) { - if (typeConstraint.CanCastToInternal(otherType, protect)) + TypeDesc instantiatedConstraint = typeConstraint.InstantiateSignature(context.TypeInstantiation, context.MethodInstantiation); + if (instantiatedConstraint.CanCastToInternal(otherType, protect)) { return true; } diff --git a/src/coreclr/tools/Common/TypeSystem/Common/GenericParameterDesc.cs b/src/coreclr/tools/Common/TypeSystem/Common/GenericParameterDesc.cs index 85f438e736412b..7cedb0f6649ca1 100644 --- a/src/coreclr/tools/Common/TypeSystem/Common/GenericParameterDesc.cs +++ b/src/coreclr/tools/Common/TypeSystem/Common/GenericParameterDesc.cs @@ -87,6 +87,12 @@ public virtual string Name /// public abstract int Index { get; } + /// + /// The associated type or method which defines this Generic Parameter + /// + public abstract TypeSystemEntity AssociatedTypeOrMethod { get; } + + /// /// Gets a value indicating the variance of this generic parameter. /// diff --git a/src/coreclr/tools/Common/TypeSystem/Ecma/EcmaGenericParameter.cs b/src/coreclr/tools/Common/TypeSystem/Ecma/EcmaGenericParameter.cs index ee59976625bcb9..1f6b5e8cddcb61 100644 --- a/src/coreclr/tools/Common/TypeSystem/Ecma/EcmaGenericParameter.cs +++ b/src/coreclr/tools/Common/TypeSystem/Ecma/EcmaGenericParameter.cs @@ -3,7 +3,7 @@ using System.Collections.Generic; using System.Reflection.Metadata; - +using System.Threading; using Debug = System.Diagnostics.Debug; using GenericParameterAttributes = System.Reflection.GenericParameterAttributes; @@ -13,6 +13,7 @@ public sealed partial class EcmaGenericParameter : GenericParameterDesc { private EcmaModule _module; private GenericParameterHandle _handle; + private TypeSystemEntity _associatedTypeOrMethod; internal EcmaGenericParameter(EcmaModule module, GenericParameterHandle handle) { @@ -78,6 +79,22 @@ public override GenericParameterKind Kind } } + public override TypeSystemEntity AssociatedTypeOrMethod + { + get + { + TypeSystemEntity tse = Volatile.Read(ref _associatedTypeOrMethod); + if (tse == null) + { + GenericParameter parameter = _module.MetadataReader.GetGenericParameter(_handle); + + tse = (TypeSystemEntity)_module.GetObject(parameter.Parent); + Volatile.Write(ref _associatedTypeOrMethod, tse); + } + return tse; + } + } + public override int Index { get diff --git a/src/coreclr/tools/aot/ILCompiler.ReadyToRun/Compiler/DependencyAnalysis/ReadyToRun/HeaderNode.cs b/src/coreclr/tools/aot/ILCompiler.ReadyToRun/Compiler/DependencyAnalysis/ReadyToRun/HeaderNode.cs index fdafc9198b79d1..1d4a814ad18cc3 100644 --- a/src/coreclr/tools/aot/ILCompiler.ReadyToRun/Compiler/DependencyAnalysis/ReadyToRun/HeaderNode.cs +++ b/src/coreclr/tools/aot/ILCompiler.ReadyToRun/Compiler/DependencyAnalysis/ReadyToRun/HeaderNode.cs @@ -97,19 +97,18 @@ public HeaderItem(ReadyToRunSectionType id, DependencyNodeCore node private readonly List _items = new List(); private readonly ReadyToRunFlags _flags; - private readonly Task _shouldAddSkipTypeValidationFlag; - private readonly TypeValidationChecker _typeValidationChecker = new TypeValidationChecker(); + private readonly Task<(bool canSkipValidation, string[] reasons)> _shouldAddSkipTypeValidationFlag; public HeaderNode(ReadyToRunFlags flags, EcmaModule moduleToCheckForSkipTypeValidation) { if (moduleToCheckForSkipTypeValidation != null) { - _shouldAddSkipTypeValidationFlag = _typeValidationChecker.CanSkipValidation(moduleToCheckForSkipTypeValidation); + _shouldAddSkipTypeValidationFlag = TypeValidationChecker.CanSkipValidation(moduleToCheckForSkipTypeValidation); } else { - _shouldAddSkipTypeValidationFlag = Task.FromResult(false); + _shouldAddSkipTypeValidationFlag = Task.FromResult((false, new string[0])); } _flags = flags; } @@ -152,14 +151,15 @@ public override ObjectData GetData(NodeFactory factory, bool relocsOnly = false) // ReadyToRunHeader.Flags int flagsInt = (int)_flags; - if (_shouldAddSkipTypeValidationFlag.Result) + if (_shouldAddSkipTypeValidationFlag.Result.canSkipValidation) { flagsInt |= (int)ReadyToRunFlags.READYTORUN_FLAG_SkipTypeValidation; } else { //#if DIAGNOSE_TYPE_VALIDATION_RULES - _typeValidationChecker.LogErrors(System.Console.WriteLine); +// foreach (string reason in _shouldAddSkipTypeValidationFlag.Result.reasons) +// System.Console.WriteLine(reason); //#endif } builder.EmitInt(flagsInt); diff --git a/src/coreclr/tools/aot/ILCompiler.ReadyToRun/Compiler/DependencyAnalysis/ReadyToRun/TypeValidationChecker.cs b/src/coreclr/tools/aot/ILCompiler.ReadyToRun/Compiler/DependencyAnalysis/ReadyToRun/TypeValidationChecker.cs index e91d9d2d7664b0..3502c45a653332 100644 --- a/src/coreclr/tools/aot/ILCompiler.ReadyToRun/Compiler/DependencyAnalysis/ReadyToRun/TypeValidationChecker.cs +++ b/src/coreclr/tools/aot/ILCompiler.ReadyToRun/Compiler/DependencyAnalysis/ReadyToRun/TypeValidationChecker.cs @@ -16,9 +16,14 @@ namespace ILCompiler.DependencyAnalysis.ReadyToRun { internal class TypeValidationChecker { - ConcurrentBag<(TypeDesc type, string reason)> _typeLoadValidationErrors = new ConcurrentBag<(TypeDesc type, string reason)>(); + private ConcurrentBag<(TypeDesc type, string reason)> _typeLoadValidationErrors = new ConcurrentBag<(TypeDesc type, string reason)>(); + private ConcurrentDictionary> _firstStageValidationChecks = new ConcurrentDictionary>(); + private ConcurrentDictionary _crossTypeValidationDependencies = new ConcurrentDictionary(); + private ConcurrentQueue> _tasksThatMustFinish = new ConcurrentQueue>(); - public void LogErrors(Action loggingFunction) + private TypeValidationChecker() { } + + private void LogErrors(Action loggingFunction) { var typeLoadValidationErrors = _typeLoadValidationErrors.ToArray(); Array.Sort(typeLoadValidationErrors, (ValueTuple left, ValueTuple right) => @@ -39,7 +44,7 @@ private void AddTypeValidationError(TypeDesc type, string error) _typeLoadValidationErrors.Add((type, error)); } - public async Task CanSkipValidation(EcmaModule module) + private async Task CanSkipValidationInstance(EcmaModule module) { // The system module can always skip type validation #if !DEBUG @@ -47,110 +52,46 @@ public async Task CanSkipValidation(EcmaModule module) return true; #endif - List> typesInModuleSkipValidationChecking = new List>(); foreach (var type in module.GetAllTypes()) { if (type is EcmaType ecmaType) - typesInModuleSkipValidationChecking.Add(CanSkipValidation(ecmaType)); + _tasksThatMustFinish.Enqueue(ValidateType(this, ecmaType)); } - typesInModuleSkipValidationChecking.Add(CanSkipValidation((EcmaType)module.GetGlobalModuleType())); + _tasksThatMustFinish.Enqueue(ValidateType(this, (EcmaType)module.GetGlobalModuleType())); - await Task.WhenAll(typesInModuleSkipValidationChecking); - foreach (var type in typesInModuleSkipValidationChecking) + bool failAtEnd = false; + while (_tasksThatMustFinish.TryDequeue(out var taskToComplete)) { - if (!type.Result) - return false; + if (!await taskToComplete) + failAtEnd = true; } - return true; - } - private ConcurrentDictionary> _skipValidationDict = new ConcurrentDictionary>(); - private Task CanSkipValidation(EcmaType type) - { - if (_skipValidationDict.TryGetValue(type, out var result)) return result; - Task skipValidatorForType = Task.Run(() => CanSkipValidationWorker(type)); - _skipValidationDict.TryAdd(type, skipValidatorForType); - return skipValidatorForType; + return !failAtEnd; } - public Task CanSkipValidation(TypeDesc type) + public static async Task<(bool canSkipValidation, string[] reasonsWhyItFailed)> CanSkipValidation(EcmaModule module) { - if (type == null) - return Task.FromResult(true); - - if (type is EcmaType ecmaType) - return CanSkipValidation(ecmaType); - else if (type is InstantiatedType instantiatedType) - return CanSkipValidation(instantiatedType); - else if (type is ParameterizedType parameterizedType) - return CanSkipValidation(parameterizedType.ParameterType); - else if (type is FunctionPointerType functionPointerType) - return CanSkipValidation(functionPointerType); - return Task.FromResult(true); - } - - private Task CanSkipValidation(InstantiatedType instantiatedType) - { - try - { - // Constraints should be satisfied - if (!instantiatedType.CheckConstraints()) - { - AddTypeValidationError(instantiatedType, "Constraint check failed"); - return Task.FromResult(false); - } - - return CanSkipValidation(instantiatedType.GetTypeDefinition()); - } - catch (Exception ex) - { - AddTypeValidationError(instantiatedType, $"due to exception '{ex}'"); - return Task.FromResult(false); - } -/* - if (_skipValidationDict.TryGetValue(instantiatedType, out var result)) return result; - Task skipValidatorForType = Task.Run(() => CanSkipValidationWorker(instantiatedType)); - _skipValidationDict.TryAdd(instantiatedType, skipValidatorForType); - return skipValidatorForType;*/ + TypeValidationChecker checker = new TypeValidationChecker(); + bool canSkipValidation = await checker.CanSkipValidationInstance(module); + List reasons = new List(); + checker.LogErrors(reasons.Add); + return (canSkipValidation, reasons.ToArray()); } - private async Task CanSkipValidationWorker(InstantiatedType instantiatedType) + private static Task ValidateType(TypeValidationChecker checker, EcmaType type) { - try - { - // Constraints should be satisfied - if (!instantiatedType.CheckConstraints()) - { - AddTypeValidationError(instantiatedType, "Constraint check failed"); - return false; - } - - return await CanSkipValidation(instantiatedType.GetTypeDefinition()); - } - catch (Exception ex) - { - AddTypeValidationError(instantiatedType, $"due to exception '{ex}'"); - return false; - } + if (checker._firstStageValidationChecks.TryGetValue(type, out var result)) return result; + Task skipValidatorForType = Task.Run(() => checker.ValidateTypeWorker(type)); + checker._firstStageValidationChecks.TryAdd(type, skipValidatorForType); + return skipValidatorForType; } - private async Task CanSkipValidation(FunctionPointerType functionPointerType) - { - if (!await CanSkipValidation(functionPointerType.Signature.ReturnType)) - return false; - foreach (var type in functionPointerType.Signature) - { - if (!await CanSkipValidation(type)) - return false; - } - return true; - } - private async Task CanSkipValidationWorker(EcmaType type) + private async Task ValidateTypeWorker(EcmaType type) { - Task CanSkipValidationWorkerHelper(TypeDesc typeToCheckForSkipValidation) + Task ValidateTypeWorkerHelper(TypeDesc typeToCheckForSkipValidation) { - return CanSkipValidation(typeToCheckForSkipValidation.InstantiateSignature(type.Instantiation, new Instantiation())); + return ValidateTypeHelper(typeToCheckForSkipValidation.InstantiateSignature(type.Instantiation, new Instantiation())); } // The runtime has a number of checks in the type loader which it will skip running if the SkipValidation flag is set // This function attempts to document all of them, and implement *some* of them. @@ -164,7 +105,7 @@ Task CanSkipValidationWorkerHelper(TypeDesc typeToCheckForSkipValidation) // Validate that the base type is loadable if (type.BaseType != null) { - if (!await CanSkipValidationWorkerHelper(type.BaseType)) + if (!await ValidateTypeWorkerHelper(type.BaseType)) { AddTypeValidationError(type, "BaseType failed validation"); return false; @@ -176,7 +117,7 @@ Task CanSkipValidationWorkerHelper(TypeDesc typeToCheckForSkipValidation) foreach (var interfaceType in type.RuntimeInterfaces) { // Validate that the all referenced interface types are loadable - if (!await CanSkipValidationWorkerHelper(interfaceType)) + if (!await ValidateTypeWorkerHelper(interfaceType)) { AddTypeValidationError(type, $"Interface type {interfaceType} failed validation"); return false; @@ -184,13 +125,12 @@ Task CanSkipValidationWorkerHelper(TypeDesc typeToCheckForSkipValidation) } // Validate that each interface type explicitly implemented on this type is accessible to this type -- UNIMPLEMENTED - foreach (var field in type.GetFields()) { // Validate that all fields on the type are both loadable - if (!await CanSkipValidationWorkerHelper(field.FieldType)) + if (!await ValidateTypeWorkerHelper(field.FieldType)) { - _typeLoadValidationErrors.Add((type, $"Field {field.Name}'s type failed validation")); + AddTypeValidationError(type, $"Field {field.Name}'s type failed validation"); return false; } @@ -407,49 +347,43 @@ Task CanSkipValidationWorkerHelper(TypeDesc typeToCheckForSkipValidation) // Validate that all virtual instance methods are actually implemented if the type is not abstract if (!type.IsAbstract) { - var virtualMethodAlgorithm = type.Context.GetVirtualMethodAlgorithmForType(type); - foreach (var interfaceImplemented in type.RuntimeInterfaces) - { - foreach (var interfaceMethod in interfaceImplemented.GetVirtualMethods()) + var virtualMethodAlgorithm = type.Context.GetVirtualMethodAlgorithmForType(type); + foreach (var interfaceImplemented in type.RuntimeInterfaces) { + foreach (var interfaceMethod in interfaceImplemented.GetVirtualMethods()) + { MethodDesc resolvedMethod; bool staticInteraceMethod = interfaceMethod.Signature.IsStatic; if (staticInteraceMethod) - if (null == resolvedMethod || resolvedMethod.IsAbstract) { - if (virtualMethodAlgorithm.ResolveInterfaceMethodToDefaultImplementationOnType(interfaceMethod, type, out var impl) != DefaultInterfaceMethodResolution.DefaultImplementation || impl.IsAbstract) - { resolvedMethod = virtualMethodAlgorithm.ResolveInterfaceMethodToStaticVirtualMethodOnType(interfaceMethod, type); - return false; - } } - } - else - { + else + { resolvedMethod = type.ResolveInterfaceMethodTarget(interfaceMethod); } if (null == resolvedMethod || (staticInteraceMethod && resolvedMethod.IsAbstract)) { if (virtualMethodAlgorithm.ResolveInterfaceMethodToDefaultImplementationOnType(interfaceMethod, type, out var impl) != DefaultInterfaceMethodResolution.DefaultImplementation || impl.IsAbstract) - { + { AddTypeValidationError(type, $"Interface method '{interfaceMethod}' does not have implementation"); - return false; + return false; + } } } } - } - foreach (var virtualMethod in type.GetAllVirtualMethods()) - { - var implementationMethod = virtualMethodAlgorithm.FindVirtualFunctionTargetMethodOnObjectType(virtualMethod, type); - if (implementationMethod == null || implementationMethod.IsAbstract) + foreach (var virtualMethod in type.GetAllVirtualMethods()) { + var implementationMethod = virtualMethodAlgorithm.FindVirtualFunctionTargetMethodOnObjectType(virtualMethod, type); + if (implementationMethod == null || implementationMethod.IsAbstract) + { AddTypeValidationError(type, $"Interface method '{virtualMethod}' does not have implementation"); - return false; + return false; + } } } - } return true; } @@ -459,6 +393,58 @@ Task CanSkipValidationWorkerHelper(TypeDesc typeToCheckForSkipValidation) AddTypeValidationError(type, $"due to exception '{ex.ToString()}'"); return false; } + + Task ValidateTypeHelper(TypeDesc typeDesc) + { + if (typeDesc == null) + return Task.FromResult(true); + + if (typeDesc is EcmaType ecmaType) + { + // Trigger the ecmaType to have its type checked, but do not check the task immediately. Unfortunately this can be recursive. + _tasksThatMustFinish.Enqueue(ValidateType(this, ecmaType)); + return Task.FromResult(true); + } + else if (typeDesc is InstantiatedType instantiatedType) + return ValidateTypeHelperInstantiatedType(instantiatedType); + else if (typeDesc is ParameterizedType parameterizedType) + return ValidateTypeHelper(parameterizedType.ParameterType); + else if (typeDesc is FunctionPointerType functionPointerType) + return ValidateTypeHelperFunctionPointerType(functionPointerType); + return Task.FromResult(true); + } + + Task ValidateTypeHelperInstantiatedType(InstantiatedType instantiatedType) + { + try + { + // Constraints should be satisfied + if (!instantiatedType.CheckConstraints()) + { + AddTypeValidationError(type, $"Constraint check failed validating {instantiatedType}"); + return Task.FromResult(false); + } + + return ValidateTypeHelper(instantiatedType.GetTypeDefinition()); + } + catch (Exception ex) + { + AddTypeValidationError(instantiatedType, $"due to exception '{ex}'"); + return Task.FromResult(false); + } + } + + async Task ValidateTypeHelperFunctionPointerType(FunctionPointerType functionPointerType) + { + if (!await ValidateTypeHelper(functionPointerType.Signature.ReturnType)) + return false; + foreach (var type in functionPointerType.Signature) + { + if (!await ValidateTypeHelper(type)) + return false; + } + return true; + } } } } From dfecc6fd8fbf7a527c1eaa2a046b8e4995576509 Mon Sep 17 00:00:00 2001 From: David Wrighton Date: Tue, 30 May 2023 14:23:14 -0700 Subject: [PATCH 16/23] Fix build breaks involving new feature of GenericParameterDesc --- .../Internal/TypeSystem/RuntimeGenericParameterDesc.cs | 8 ++++++++ .../src/Internal/TypeSystem/RuntimeMethodDesc.cs | 4 +++- .../Internal/TypeSystem/TypeSystemContext.Runtime.cs | 4 ++++ .../tools/Common/TypeSystem/Common/CastingHelper.cs | 10 ++++++---- .../ILVerification/InstantiatedGenericParameter.cs | 2 ++ .../TypeRefTypeSystemGenericParameter.cs | 8 ++++++++ 6 files changed, 31 insertions(+), 5 deletions(-) diff --git a/src/coreclr/nativeaot/System.Private.TypeLoader/src/Internal/TypeSystem/RuntimeGenericParameterDesc.cs b/src/coreclr/nativeaot/System.Private.TypeLoader/src/Internal/TypeSystem/RuntimeGenericParameterDesc.cs index 8133420d8934ad..fb887c892af48f 100644 --- a/src/coreclr/nativeaot/System.Private.TypeLoader/src/Internal/TypeSystem/RuntimeGenericParameterDesc.cs +++ b/src/coreclr/nativeaot/System.Private.TypeLoader/src/Internal/TypeSystem/RuntimeGenericParameterDesc.cs @@ -9,6 +9,7 @@ internal class RuntimeGenericParameterDesc : GenericParameterDesc private readonly int _index; private readonly TypeSystemContext _context; private readonly GenericVariance _variance; + private TypeSystemEntity _associatedTypeOrMethod; public RuntimeGenericParameterDesc(GenericParameterKind kind, int index, TypeSystemContext context, GenericVariance variance) { @@ -25,5 +26,12 @@ public RuntimeGenericParameterDesc(GenericParameterKind kind, int index, TypeSys public override TypeSystemContext Context => _context; public override GenericVariance Variance => _variance; + + public override TypeSystemEntity AssociatedTypeOrMethod => _associatedTypeOrMethod; + + internal void SetAssociatedTypeOrMethod(TypeSystemEntity associatedTypeOrMethod) + { + _associatedTypeOrMethod = associatedTypeOrMethod; + } } } diff --git a/src/coreclr/nativeaot/System.Private.TypeLoader/src/Internal/TypeSystem/RuntimeMethodDesc.cs b/src/coreclr/nativeaot/System.Private.TypeLoader/src/Internal/TypeSystem/RuntimeMethodDesc.cs index 6ee7b8d2f6cbdf..36738ccfbedd14 100644 --- a/src/coreclr/nativeaot/System.Private.TypeLoader/src/Internal/TypeSystem/RuntimeMethodDesc.cs +++ b/src/coreclr/nativeaot/System.Private.TypeLoader/src/Internal/TypeSystem/RuntimeMethodDesc.cs @@ -52,7 +52,9 @@ public override Instantiation Instantiation TypeDesc[] genericParameters = new TypeDesc[genericArgCount]; for (int i = 0; i < genericParameters.Length; i++) { - genericParameters[i] = new RuntimeGenericParameterDesc(GenericParameterKind.Method, i, Context, GenericVariance.None); + var newGenericParameter = new RuntimeGenericParameterDesc(GenericParameterKind.Method, i, Context, GenericVariance.None); + newGenericParameter.SetAssociatedTypeOrMethod(this); + genericParameters[i] = newGenericParameter; } _instantiation = new Instantiation(genericParameters); } diff --git a/src/coreclr/nativeaot/System.Private.TypeLoader/src/Internal/TypeSystem/TypeSystemContext.Runtime.cs b/src/coreclr/nativeaot/System.Private.TypeLoader/src/Internal/TypeSystem/TypeSystemContext.Runtime.cs index d37e5a684a4d6c..07045cde66b019 100644 --- a/src/coreclr/nativeaot/System.Private.TypeLoader/src/Internal/TypeSystem/TypeSystemContext.Runtime.cs +++ b/src/coreclr/nativeaot/System.Private.TypeLoader/src/Internal/TypeSystem/TypeSystemContext.Runtime.cs @@ -213,6 +213,10 @@ public TypeDesc ResolveRuntimeTypeHandle(RuntimeTypeHandle rtth) } returnedType = new NoMetadataType(this, rtth, null, new Instantiation(genericParameters), rtth.GetHashCode()); + for (int i = 0; i < genericParameters.Length; i++) + { + ((RuntimeGenericParameterDesc)genericParameters[i]).SetAssociatedTypeOrMethod(returnedType); + } } } else if (RuntimeAugments.IsGenericType(rtth)) diff --git a/src/coreclr/tools/Common/TypeSystem/Common/CastingHelper.cs b/src/coreclr/tools/Common/TypeSystem/Common/CastingHelper.cs index cb73f69dc77c3e..67d8337b936c96 100644 --- a/src/coreclr/tools/Common/TypeSystem/Common/CastingHelper.cs +++ b/src/coreclr/tools/Common/TypeSystem/Common/CastingHelper.cs @@ -195,18 +195,20 @@ private static bool CanCastGenericParameterTo(this GenericParameterDesc thisType return true; } - InstantiationContext context; + Instantiation typeInstantiation; + Instantiation methodInstantiation = default(Instantiation); if (thisType.AssociatedTypeOrMethod is MethodDesc method) { - context = new InstantiationContext(method.OwningType.Instantiation, method.Instantiation); + typeInstantiation = method.OwningType.Instantiation; + methodInstantiation = method.Instantiation; } else { - context = new InstantiationContext(((TypeDesc)thisType.AssociatedTypeOrMethod).Instantiation, default(Instantiation)); + typeInstantiation = ((TypeDesc)thisType.AssociatedTypeOrMethod).Instantiation; } foreach (var typeConstraint in thisType.TypeConstraints) { - TypeDesc instantiatedConstraint = typeConstraint.InstantiateSignature(context.TypeInstantiation, context.MethodInstantiation); + TypeDesc instantiatedConstraint = typeConstraint.InstantiateSignature(typeInstantiation, methodInstantiation); if (instantiatedConstraint.CanCastToInternal(otherType, protect)) { return true; diff --git a/src/coreclr/tools/ILVerification/InstantiatedGenericParameter.cs b/src/coreclr/tools/ILVerification/InstantiatedGenericParameter.cs index 588bcbd274f7ad..c467d625c20af3 100644 --- a/src/coreclr/tools/ILVerification/InstantiatedGenericParameter.cs +++ b/src/coreclr/tools/ILVerification/InstantiatedGenericParameter.cs @@ -76,6 +76,8 @@ private InstantiatedGenericParameter(GenericParameterDesc genericParam) public override GenericConstraints Constraints => _genericParam.Constraints; + public override TypeSystemEntity AssociatedTypeOrMethod => _genericParam.AssociatedTypeOrMethod; + public override IEnumerable TypeConstraints { get diff --git a/src/coreclr/tools/dotnet-pgo/TypeRefTypeSystem/TypeRefTypeSystemGenericParameter.cs b/src/coreclr/tools/dotnet-pgo/TypeRefTypeSystem/TypeRefTypeSystemGenericParameter.cs index 9748b53a6b5093..772c9929626844 100644 --- a/src/coreclr/tools/dotnet-pgo/TypeRefTypeSystem/TypeRefTypeSystemGenericParameter.cs +++ b/src/coreclr/tools/dotnet-pgo/TypeRefTypeSystem/TypeRefTypeSystemGenericParameter.cs @@ -59,6 +59,14 @@ public override GenericParameterKind Kind } } + public override TypeSystemEntity AssociatedTypeOrMethod + { + get + { + return (_owningMethod != null) ? _owningMethod : _owningType; + } + } + public override int Index { get From eff81d59acd860b70b6b6e95a200eb3095b4ed35 Mon Sep 17 00:00:00 2001 From: David Wrighton Date: Wed, 31 May 2023 17:08:06 -0700 Subject: [PATCH 17/23] Add documentation for R2R format changes Fix command line parameter to be more reasonable, and allow logging on command Fix the rest of issues noted in crossgen2 testing --- docs/design/coreclr/botr/readytorun-format.md | 31 ++++ .../TypeSystem/Common/GenericParameterDesc.cs | 8 + .../ReadyToRun/HeaderNode.cs | 22 ++- .../ReadyToRun/TypeValidationChecker.cs | 167 +++++++++++++++--- .../ReadyToRunCodegenNodeFactory.cs | 10 +- .../Compiler/ReadyToRunCodegenCompilation.cs | 6 +- .../ReadyToRunCodegenCompilationBuilder.cs | 5 +- .../aot/crossgen2/Crossgen2RootCommand.cs | 8 +- src/coreclr/tools/aot/crossgen2/Program.cs | 2 +- .../aot/crossgen2/Properties/Resources.resx | 4 +- src/coreclr/vm/siginfo.cpp | 2 +- 11 files changed, 221 insertions(+), 44 deletions(-) diff --git a/docs/design/coreclr/botr/readytorun-format.md b/docs/design/coreclr/botr/readytorun-format.md index 2c63997332e487..09f4c2184c8d81 100644 --- a/docs/design/coreclr/botr/readytorun-format.md +++ b/docs/design/coreclr/botr/readytorun-format.md @@ -179,6 +179,10 @@ The following section types are defined and described later in this document: | PgoInstrumentationData | 117 | Image (added in V5.2) | ManifestAssemblyMvids | 118 | Image (added in V5.3) | CrossModuleInlineInfo | 119 | Image (added in V6.3) +| HotColdMap | 120 | Undocumented +| MethodIsGenericMap | 121 | Assembly (Added in V9.0) +| EnclosingTypeMap | 122 | Assembly (Added in V9.0) +| TypeGenericInfoMap | 123 | Assembly (Added in V9.0) ## ReadyToRunSectionType.CompilerIdentifier @@ -644,6 +648,33 @@ The entry of the hashtable is a counted sequence of compressed unsigned integers This section may be included in addition to a InliningInfo2 section. +## ReadyToRunSectionType.MethodIsGenericMap (v9.0+) +This optional section holds a bit vector to indicate if the MethodDef's contained within the assembly have generic parameters or not. This allowes determinining if a method is generic or not by querying a bit vector (which is fast, and efficient) as opposed to examining the GenericParameter table, or the signature of the Method. + +The section begins with a single 32bit integer indicating the number of bits in the bit vector. Following that integer is the actual bit vector of all of the data. The data is grouped into 8 bit bytes, where the least significant bit of the byte is the bit which represents the lowest MethodDef. + +For instance, the first byte in the bit vector represents the MethodDefs 06000001 to 06000008, and the least signficant bit of that first byte is the bit representing the IsGeneric bit for MethodDef 06000001. + +## ReadyToRunSectionType.EnclosingTypeMap (v9.0+) + +This optional section allows for efficient O(1) lookup from the enclosed type to the type which encloses it without requiring the binary search that is necessary if using the ECMA 335 defined NestedClass table (which encodes exactly the same information). This section may only be included in the assembly if the assembly has fewer than 0xFFFE types defined within it. + +The structure of this section is: +A single 16 bit unsigned integer listing the count of entries in the map. +This count is followed by a 16 bit unsigned integer for each TypeDef defined in the assembly. This typedef is the RID of the enclosing type, or 0 if the typedef is not enclosed by another type. + +## ReadyToRunSectionType.TypeGenericInfoMap (v9.0+) +This optional section represents a condensed view of some generic details about types. This can make it more efficient to load types. + +The structure of this section is: +A single 32bit integer representing the number of entries in the map followed by a series of 4 bit entries, one per type. These 4 bit entries are grouped into bytes, where each byte holds 2 entries, and the entry in the most signficant 4 bits of the byte is the entry representing a lower TypeDef RID. + +TypeGenericInfoMap entries have 4 bits representing 3 different sets of information. + +1. What is the count of generic parameters (0, 1, 2, MoreThanTwo) (This is represented in the least significant 2 bits of the TypeGenericInfoMap entry) +2. Are there any constraints on the generic parameters? (This is the 3rd bit of the entry) +3. Do any of the generic parameters have co or contra variance? (This is the 4th bit of the entry) + # Native Format Native format is set of encoding patterns that allow persisting type system data in a binary format that is diff --git a/src/coreclr/tools/Common/TypeSystem/Common/GenericParameterDesc.cs b/src/coreclr/tools/Common/TypeSystem/Common/GenericParameterDesc.cs index 7cedb0f6649ca1..ddcbd4a4ea8466 100644 --- a/src/coreclr/tools/Common/TypeSystem/Common/GenericParameterDesc.cs +++ b/src/coreclr/tools/Common/TypeSystem/Common/GenericParameterDesc.cs @@ -150,6 +150,14 @@ public bool HasDefaultConstructorConstraint } } + public bool HasAcceptByRefLikeConstraint + { + get + { + return (Constraints & GenericConstraints.AcceptByRefLike) != 0; + } + } + public bool IsCovariant { get diff --git a/src/coreclr/tools/aot/ILCompiler.ReadyToRun/Compiler/DependencyAnalysis/ReadyToRun/HeaderNode.cs b/src/coreclr/tools/aot/ILCompiler.ReadyToRun/Compiler/DependencyAnalysis/ReadyToRun/HeaderNode.cs index 1d4a814ad18cc3..4b6da6a4cc085d 100644 --- a/src/coreclr/tools/aot/ILCompiler.ReadyToRun/Compiler/DependencyAnalysis/ReadyToRun/HeaderNode.cs +++ b/src/coreclr/tools/aot/ILCompiler.ReadyToRun/Compiler/DependencyAnalysis/ReadyToRun/HeaderNode.cs @@ -151,16 +151,20 @@ public override ObjectData GetData(NodeFactory factory, bool relocsOnly = false) // ReadyToRunHeader.Flags int flagsInt = (int)_flags; - if (_shouldAddSkipTypeValidationFlag.Result.canSkipValidation) - { - flagsInt |= (int)ReadyToRunFlags.READYTORUN_FLAG_SkipTypeValidation; - } - else + if (!relocsOnly) { -//#if DIAGNOSE_TYPE_VALIDATION_RULES -// foreach (string reason in _shouldAddSkipTypeValidationFlag.Result.reasons) -// System.Console.WriteLine(reason); -//#endif + if (_shouldAddSkipTypeValidationFlag.Result.canSkipValidation) + { + flagsInt |= (int)ReadyToRunFlags.READYTORUN_FLAG_SkipTypeValidation; + } + else + { + if (factory.OptimizationFlags.TypeValidation == TypeValidationRule.AutomaticWithLogging) + { + foreach (string reason in _shouldAddSkipTypeValidationFlag.Result.reasons) + System.Console.WriteLine(reason); + } + } } builder.EmitInt(flagsInt); diff --git a/src/coreclr/tools/aot/ILCompiler.ReadyToRun/Compiler/DependencyAnalysis/ReadyToRun/TypeValidationChecker.cs b/src/coreclr/tools/aot/ILCompiler.ReadyToRun/Compiler/DependencyAnalysis/ReadyToRun/TypeValidationChecker.cs index 3502c45a653332..c879415449f57a 100644 --- a/src/coreclr/tools/aot/ILCompiler.ReadyToRun/Compiler/DependencyAnalysis/ReadyToRun/TypeValidationChecker.cs +++ b/src/coreclr/tools/aot/ILCompiler.ReadyToRun/Compiler/DependencyAnalysis/ReadyToRun/TypeValidationChecker.cs @@ -66,6 +66,17 @@ private async Task CanSkipValidationInstance(EcmaModule module) failAtEnd = true; } +#if DEBUG + if (module == module.Context.SystemModule) + { + // Spot check that the system module ALWAYS succeeds + if (failAtEnd) + { + throw new InternalCompilerErrorException("System module failed to validate all types"); + } + } +#endif + return !failAtEnd; } @@ -285,7 +296,6 @@ Task ValidateTypeWorkerHelper(TypeDesc typeToCheckForSkipValidation) // Validate that generic variance is properly respected in method signatures -- UNIMPLEMENTED // Validate that there are no cyclical method constraints -- UNIMPLEMENTED // Validate that constraints are all acccessible to the method using them -- UNIMPLEMENTED - } // Generic class special rules @@ -293,9 +303,10 @@ Task ValidateTypeWorkerHelper(TypeDesc typeToCheckForSkipValidation) // Validate that there are no cyclical class or method constraints, and that constraints are all acccessible to the type using them -- UNIMPLEMENTED // Override rules - // Validate that for every override involving generic methods that the generic method constraints are matching. -- UNIMPLEMENTED - // Validate that each override results does not violate accessibility rules - + // Validate that each override results does not violate accessibility rules -- UNIMPLEMENTED + + HashSet overridenDeclMethods = new HashSet(); + foreach (var methodImplHandle in typeDef.GetMethodImplementations()) { var methodImpl = type.MetadataReader.GetMethodImplementation(methodImplHandle); @@ -340,43 +351,104 @@ Task ValidateTypeWorkerHelper(TypeDesc typeToCheckForSkipValidation) return false; } + // Validate that multiple MethodImpls don't override the same method + if (!overridenDeclMethods.Add(methodDecl)) + { + AddTypeValidationError(type, $"Multiple MethodImpl records override '{methodDecl}'"); + return false; + } + // Validate that the MethodImpl follows MethodImpl accessibility rules -- UNIMPLEMENTED } - // Validate that all virtual static methods are actually implemented if the type is not abstract - // Validate that all virtual instance methods are actually implemented if the type is not abstract - if (!type.IsAbstract) + var virtualMethodAlgorithm = type.Context.GetVirtualMethodAlgorithmForType(type); + VirtualMethodAlgorithm baseTypeVirtualMethodAlgorithm = null; + if (type.BaseType != null && !type.IsInterface && !type.IsValueType) { - var virtualMethodAlgorithm = type.Context.GetVirtualMethodAlgorithmForType(type); - foreach (var interfaceImplemented in type.RuntimeInterfaces) + baseTypeVirtualMethodAlgorithm = type.Context.GetVirtualMethodAlgorithmForType(type.BaseType); + } + + foreach (var interfaceImplemented in type.RuntimeInterfaces) + { + foreach (var interfaceMethod in interfaceImplemented.GetVirtualMethods()) { - foreach (var interfaceMethod in interfaceImplemented.GetVirtualMethods()) + MethodDesc resolvedMethod; + bool staticInterfaceMethod = interfaceMethod.Signature.IsStatic; + if (staticInterfaceMethod) { - MethodDesc resolvedMethod; - bool staticInteraceMethod = interfaceMethod.Signature.IsStatic; - if (staticInteraceMethod) - { - resolvedMethod = virtualMethodAlgorithm.ResolveInterfaceMethodToStaticVirtualMethodOnType(interfaceMethod, type); - } - else + resolvedMethod = virtualMethodAlgorithm.ResolveInterfaceMethodToStaticVirtualMethodOnType(interfaceMethod, type); + } + else + { + resolvedMethod = type.ResolveInterfaceMethodTarget(interfaceMethod); + } + + if (resolvedMethod != null) + { + // Validate that for every override involving generic methods that the generic method constraints are matching + if (!CompareMethodConstraints(interfaceMethod, resolvedMethod)) { - resolvedMethod = type.ResolveInterfaceMethodTarget(interfaceMethod); + AddTypeValidationError(type, $"Interface method '{interfaceMethod}' overriden by method '{resolvedMethod}' which does not have matching generic constraints"); + return false; } + } - if (null == resolvedMethod || (staticInteraceMethod && resolvedMethod.IsAbstract)) + // Validate that all virtual static methods are actually implemented if the type is not abstract + // Validate that all virtual instance methods are actually implemented if the type is not abstract + if (!type.IsAbstract) + { + if (null == resolvedMethod || (staticInterfaceMethod && resolvedMethod.IsAbstract)) { if (virtualMethodAlgorithm.ResolveInterfaceMethodToDefaultImplementationOnType(interfaceMethod, type, out var impl) != DefaultInterfaceMethodResolution.DefaultImplementation || impl.IsAbstract) { AddTypeValidationError(type, $"Interface method '{interfaceMethod}' does not have implementation"); return false; } + + if (impl != null) + { + // Validate that for every override involving generic methods that the generic method constraints are matching + if (!CompareMethodConstraints(interfaceMethod, impl)) + { + AddTypeValidationError(type, $"Interface method '{interfaceMethod}' overriden by method '{impl}' which does not have matching generic constraints"); + return false; + } + } + } + } + } + } + + foreach (var virtualMethod in type.EnumAllVirtualSlots()) + { + var implementationMethod = virtualMethodAlgorithm.FindVirtualFunctionTargetMethodOnObjectType(virtualMethod, type); + + if (implementationMethod != null) + { + // Validate that for every override involving generic methods that the generic method constraints are matching + if (!CompareMethodConstraints(virtualMethod, implementationMethod)) + { + AddTypeValidationError(type, $"Virtual method '{virtualMethod}' overriden by method '{implementationMethod}' which does not have matching generic constraints"); + return false; + } + + // Validate that if the decl method for the virtual is not on the immediate base type, that the intermediate type did not establish a + // covariant return type which requires the implementation method to specify a more specific base type + if ((virtualMethod.OwningType != type.BaseType) && (virtualMethod.OwningType != type) && (baseTypeVirtualMethodAlgorithm != null)) + { + var implementationOnBaseType = baseTypeVirtualMethodAlgorithm.FindVirtualFunctionTargetMethodOnObjectType(virtualMethod, type.BaseType); + if (!implementationMethod.Signature.EqualsWithCovariantReturnType(implementationOnBaseType.Signature)) + { + AddTypeValidationError(type, $"Virtual method '{virtualMethod}' overriden by method '{implementationMethod}' does not satisfy the covariant return type introduced with '{implementationOnBaseType}'"); + return false; } } } - foreach (var virtualMethod in type.GetAllVirtualMethods()) + // Validate that all virtual static methods are actually implemented if the type is not abstract + // Validate that all virtual instance methods are actually implemented if the type is not abstract + if (!type.IsAbstract) { - var implementationMethod = virtualMethodAlgorithm.FindVirtualFunctionTargetMethodOnObjectType(virtualMethod, type); if (implementationMethod == null || implementationMethod.IsAbstract) { AddTypeValidationError(type, $"Interface method '{virtualMethod}' does not have implementation"); @@ -394,6 +466,59 @@ Task ValidateTypeWorkerHelper(TypeDesc typeToCheckForSkipValidation) return false; } + static bool CompareGenericParameterConstraint(MethodDesc declMethod, GenericParameterDesc parameterOfDecl, MethodDesc implMethod, GenericParameterDesc parameterOfImpl) + { + if (parameterOfImpl.HasDefaultConstructorConstraint) + if (!parameterOfDecl.HasDefaultConstructorConstraint && !parameterOfDecl.HasNotNullableValueTypeConstraint) + return false; + + if (parameterOfImpl.HasNotNullableValueTypeConstraint) + if (!parameterOfDecl.HasNotNullableValueTypeConstraint) + return false; + + if (parameterOfImpl.HasReferenceTypeConstraint) + if (!parameterOfDecl.HasReferenceTypeConstraint) + return false; + + if (parameterOfDecl.HasAcceptByRefLikeConstraint) + if (!parameterOfImpl.HasAcceptByRefLikeConstraint) + return false; + + HashSet constraintsOnDecl = new HashSet(); + foreach (var constraint in parameterOfDecl.TypeConstraints) + { + constraintsOnDecl.Add(constraint.InstantiateSignature(declMethod.Instantiation, default(Instantiation))); + } + + foreach (var constraint in parameterOfImpl.TypeConstraints) + { + if (!constraintsOnDecl.Contains(constraint.InstantiateSignature(implMethod.Instantiation, default(Instantiation)))) + return false; + } + + return true; + } + + static bool CompareMethodConstraints(MethodDesc methodDecl, MethodDesc methodImpl) + { + // Validate that methodDecl's method constraints are at least as stringent as methodImpls + // The Decl is permitted to be more stringent. + + if (methodDecl.Instantiation.Length != methodImpl.Instantiation.Length) + return false; + for (int i = 0; i < methodDecl.Instantiation.Length; i++) + { + var genericParameterDescOnImpl = (GenericParameterDesc)methodImpl.GetTypicalMethodDefinition().Instantiation[i]; + var genericParameterDescOnDecl = (GenericParameterDesc)methodDecl.GetTypicalMethodDefinition().Instantiation[i]; + if (!CompareGenericParameterConstraint(methodDecl, genericParameterDescOnDecl, methodImpl, genericParameterDescOnImpl)) + { + return false; + } + } + + return true; + } + Task ValidateTypeHelper(TypeDesc typeDesc) { if (typeDesc == null) diff --git a/src/coreclr/tools/aot/ILCompiler.ReadyToRun/Compiler/DependencyAnalysis/ReadyToRunCodegenNodeFactory.cs b/src/coreclr/tools/aot/ILCompiler.ReadyToRun/Compiler/DependencyAnalysis/ReadyToRunCodegenNodeFactory.cs index 2f523b9a77ac1e..904620681ddf46 100644 --- a/src/coreclr/tools/aot/ILCompiler.ReadyToRun/Compiler/DependencyAnalysis/ReadyToRunCodegenNodeFactory.cs +++ b/src/coreclr/tools/aot/ILCompiler.ReadyToRun/Compiler/DependencyAnalysis/ReadyToRunCodegenNodeFactory.cs @@ -44,10 +44,18 @@ public TValue GetOrAdd(TKey key) } } + public enum TypeValidationRule + { + Automatic, + AutomaticWithLogging, + AlwaysValidate, + SkipTypeValidation + } + public sealed class NodeFactoryOptimizationFlags { public bool OptimizeAsyncMethods; - public bool? SkipTypeValidation; + public TypeValidationRule TypeValidation; } // To make the code future compatible to the composite R2R story diff --git a/src/coreclr/tools/aot/ILCompiler.ReadyToRun/Compiler/ReadyToRunCodegenCompilation.cs b/src/coreclr/tools/aot/ILCompiler.ReadyToRun/Compiler/ReadyToRunCodegenCompilation.cs index ea7343d5e5325a..eaf6d051b0f0b4 100644 --- a/src/coreclr/tools/aot/ILCompiler.ReadyToRun/Compiler/ReadyToRunCodegenCompilation.cs +++ b/src/coreclr/tools/aot/ILCompiler.ReadyToRun/Compiler/ReadyToRunCodegenCompilation.cs @@ -407,8 +407,8 @@ private void RewriteComponentFile(string inputFile, string outputFile, string ow { flags |= ReadyToRunFlags.READYTORUN_FLAG_PlatformNeutralSource; } - if (_nodeFactory.OptimizationFlags.SkipTypeValidation.HasValue && - _nodeFactory.OptimizationFlags.SkipTypeValidation.Value) + bool automaticTypeValidation = _nodeFactory.OptimizationFlags.TypeValidation == TypeValidationRule.Automatic || _nodeFactory.OptimizationFlags.TypeValidation == TypeValidationRule.AutomaticWithLogging; + if (_nodeFactory.OptimizationFlags.TypeValidation == TypeValidationRule.SkipTypeValidation) { flags |= ReadyToRunFlags.READYTORUN_FLAG_SkipTypeValidation; } @@ -430,7 +430,7 @@ private void RewriteComponentFile(string inputFile, string outputFile, string ow flags, _nodeFactory.OptimizationFlags, _nodeFactory.ImageBase, - _nodeFactory.OptimizationFlags.SkipTypeValidation.HasValue ? null : inputModule); + automaticTypeValidation ? inputModule : null); IComparer> comparer = new SortableDependencyNode.ObjectNodeComparer(CompilerComparer.Instance); DependencyAnalyzerBase componentGraph = new DependencyAnalyzer, NodeFactory>(componentFactory, comparer); diff --git a/src/coreclr/tools/aot/ILCompiler.ReadyToRun/Compiler/ReadyToRunCodegenCompilationBuilder.cs b/src/coreclr/tools/aot/ILCompiler.ReadyToRun/Compiler/ReadyToRunCodegenCompilationBuilder.cs index 34d31537cea5bd..64d39b723960c4 100644 --- a/src/coreclr/tools/aot/ILCompiler.ReadyToRun/Compiler/ReadyToRunCodegenCompilationBuilder.cs +++ b/src/coreclr/tools/aot/ILCompiler.ReadyToRun/Compiler/ReadyToRunCodegenCompilationBuilder.cs @@ -242,7 +242,8 @@ public override ICompilation ToCompilation() { flags |= ReadyToRunFlags.READYTORUN_FLAG_PlatformNeutralSource; } - if (_nodeFactoryOptimizationFlags.SkipTypeValidation.HasValue && _nodeFactoryOptimizationFlags.SkipTypeValidation.Value) + bool automaticTypeValidation = _nodeFactoryOptimizationFlags.TypeValidation == TypeValidationRule.Automatic || _nodeFactoryOptimizationFlags.TypeValidation == TypeValidationRule.AutomaticWithLogging; + if (_nodeFactoryOptimizationFlags.TypeValidation == TypeValidationRule.SkipTypeValidation) { flags |= ReadyToRunFlags.READYTORUN_FLAG_SkipTypeValidation; } @@ -259,7 +260,7 @@ public override ICompilation ToCompilation() flags, _nodeFactoryOptimizationFlags, _imageBase, - _nodeFactoryOptimizationFlags.SkipTypeValidation.HasValue ? null : singleModule); + automaticTypeValidation ? singleModule : null); factory.CompositeImageSettings = _compositeImageSettings; diff --git a/src/coreclr/tools/aot/crossgen2/Crossgen2RootCommand.cs b/src/coreclr/tools/aot/crossgen2/Crossgen2RootCommand.cs index f2b8738f0b9d18..dcc7b41642daef 100644 --- a/src/coreclr/tools/aot/crossgen2/Crossgen2RootCommand.cs +++ b/src/coreclr/tools/aot/crossgen2/Crossgen2RootCommand.cs @@ -8,7 +8,7 @@ using System.CommandLine.Parsing; using System.IO; using System.Runtime.InteropServices; - +using ILCompiler.DependencyAnalysis; using Internal.TypeSystem; namespace ILCompiler @@ -37,8 +37,8 @@ internal class Crossgen2RootCommand : RootCommand new(new[] { "--optimize-space", "--Os" }, SR.OptimizeSpaceOption); public Option OptimizeTime { get; } = new(new[] { "--optimize-time", "--Ot" }, SR.OptimizeSpeedOption); - public Option SkipTypeValidation { get; } = - new(new[] { "--skip-type-validation"}, () => null, SR.SkipTypeValidation); + public Option TypeValidation { get; } = + new(new[] { "--type-validation"}, () => TypeValidationRule.Automatic, SR.TypeValidation); public Option InputBubble { get; } = new(new[] { "--inputbubble" }, SR.InputBubbleOption); public Option> InputBubbleReferenceFilePaths { get; } = @@ -202,7 +202,7 @@ public Crossgen2RootCommand(string[] args) : base(SR.Crossgen2BannerText) AddOption(OptimizeDisabled); AddOption(OptimizeSpace); AddOption(OptimizeTime); - AddOption(SkipTypeValidation); + AddOption(TypeValidation); AddOption(InputBubble); AddOption(InputBubbleReferenceFilePaths); AddOption(Composite); diff --git a/src/coreclr/tools/aot/crossgen2/Program.cs b/src/coreclr/tools/aot/crossgen2/Program.cs index c12581e7b6da30..d4080ad70c6820 100644 --- a/src/coreclr/tools/aot/crossgen2/Program.cs +++ b/src/coreclr/tools/aot/crossgen2/Program.cs @@ -581,7 +581,7 @@ private void RunSingleCompilation(Dictionary inFilePaths, Instru NodeFactoryOptimizationFlags nodeFactoryFlags = new NodeFactoryOptimizationFlags(); nodeFactoryFlags.OptimizeAsyncMethods = Get(_command.AsyncMethodOptimization); - nodeFactoryFlags.SkipTypeValidation = Get(_command.SkipTypeValidation); + nodeFactoryFlags.TypeValidation = Get(_command.TypeValidation); builder .UseMapFile(Get(_command.Map)) diff --git a/src/coreclr/tools/aot/crossgen2/Properties/Resources.resx b/src/coreclr/tools/aot/crossgen2/Properties/Resources.resx index 962bd646c2b207..4d8e00ff620b60 100644 --- a/src/coreclr/tools/aot/crossgen2/Properties/Resources.resx +++ b/src/coreclr/tools/aot/crossgen2/Properties/Resources.resx @@ -399,7 +399,7 @@ .NET Crossgen2 Compiler - - Configure the runtime so that it does not validate metadata for types defined in compiled assemblies (default: automatic) + + Configure the runtime's behavior around validating the correctness of types defined in assemblies compiled via crossgen2 \ No newline at end of file diff --git a/src/coreclr/vm/siginfo.cpp b/src/coreclr/vm/siginfo.cpp index a44f40cd9b34fb..cbad3871a003d0 100644 --- a/src/coreclr/vm/siginfo.cpp +++ b/src/coreclr/vm/siginfo.cpp @@ -4858,7 +4858,7 @@ BOOL MetaSig::CompareMethodConstraints(const Substitution *pSubst1, if (!pModule1->m_pTypeGenericInfoMap->IsGeneric(tok1, pInternalImport1)) { // The method's generic-ness must match. - return !pModule1->m_pTypeGenericInfoMap->IsGeneric(tok1, pInternalImport1); + return !pModule2->m_pTypeGenericInfoMap->IsGeneric(tok2, pInternalImport2); } HENUMInternalHolder hEnumTyPars1(pInternalImport1); From db08221d3038e1f4a3838ea012707d6e976482e0 Mon Sep 17 00:00:00 2001 From: David Wrighton Date: Thu, 1 Jun 2023 11:36:53 -0700 Subject: [PATCH 18/23] Fix implementation of CompareMethodContraints. instead of using IsGeneric map, check to see if the method is generic in the first place. It turns out we have an efficient way to check in every place that matters --- src/coreclr/vm/methodtablebuilder.cpp | 30 ++++++++++++++++++--------- src/coreclr/vm/siginfo.cpp | 6 ------ 2 files changed, 20 insertions(+), 16 deletions(-) diff --git a/src/coreclr/vm/methodtablebuilder.cpp b/src/coreclr/vm/methodtablebuilder.cpp index 31f0cfe80a8b44..69e03f52ee8cdd 100644 --- a/src/coreclr/vm/methodtablebuilder.cpp +++ b/src/coreclr/vm/methodtablebuilder.cpp @@ -254,12 +254,19 @@ MethodTableBuilder::LoaderFindMethodInParentClass( { if (pMethodConstraintsMatch != NULL) { - // Check the constraints are consistent, - // and return the result to the caller. - // We do this here to avoid recalculating pSubst. - *pMethodConstraintsMatch = MetaSig::CompareMethodConstraints( - methodSig.GetSubstitution(), methodSig.GetModule(), methodSig.GetToken(), - entrySig.GetSubstitution(), entrySig.GetModule(), entrySig.GetToken()); + if ((*methodSig.GetSignature()) & IMAGE_CEE_CS_CALLCONV_GENERIC) + { + // Check the constraints are consistent, + // and return the result to the caller. + // We do this here to avoid recalculating pSubst. + *pMethodConstraintsMatch = MetaSig::CompareMethodConstraints( + methodSig.GetSubstitution(), methodSig.GetModule(), methodSig.GetToken(), + entrySig.GetSubstitution(), entrySig.GetModule(), entrySig.GetToken()); + } + else + { + *pMethodConstraintsMatch = TRUE; // If the method isn't generic, just mark that constraints match + } } return pEntryMethod; @@ -6346,10 +6353,13 @@ MethodTableBuilder::MethodImplCompareSignatures( } //now compare the method constraints - if (!MetaSig::CompareMethodConstraints(implSig.GetSubstitution(), implSig.GetModule(), implSig.GetToken(), - declSig.GetSubstitution(), declSig.GetModule(), declSig.GetToken())) + if ((*declSig.GetSignature()) & IMAGE_CEE_CS_CALLCONV_GENERIC) { - BuildMethodTableThrowException(dwConstraintErrorCode, implSig.GetToken()); + if (!MetaSig::CompareMethodConstraints(implSig.GetSubstitution(), implSig.GetModule(), implSig.GetToken(), + declSig.GetSubstitution(), declSig.GetModule(), declSig.GetToken())) + { + BuildMethodTableThrowException(dwConstraintErrorCode, implSig.GetToken()); + } } } @@ -6901,7 +6911,7 @@ VOID MethodTableBuilder::ValidateInterfaceMethodConstraints() } // Now compare the method constraints. - if (!MetaSig::CompareMethodConstraints(pSubstTgt, + if ((pTargetMD->GetClassification() == mcInstantiated) && !MetaSig::CompareMethodConstraints(pSubstTgt, pTargetModule, pTargetMD->GetMemberDef(), &pItf->GetSubstitution(), diff --git a/src/coreclr/vm/siginfo.cpp b/src/coreclr/vm/siginfo.cpp index cbad3871a003d0..27bfe457db6f5a 100644 --- a/src/coreclr/vm/siginfo.cpp +++ b/src/coreclr/vm/siginfo.cpp @@ -4855,12 +4855,6 @@ BOOL MetaSig::CompareMethodConstraints(const Substitution *pSubst1, IMDInternalImport *pInternalImport1 = pModule1->GetMDImport(); IMDInternalImport *pInternalImport2 = pModule2->GetMDImport(); - if (!pModule1->m_pTypeGenericInfoMap->IsGeneric(tok1, pInternalImport1)) - { - // The method's generic-ness must match. - return !pModule2->m_pTypeGenericInfoMap->IsGeneric(tok2, pInternalImport2); - } - HENUMInternalHolder hEnumTyPars1(pInternalImport1); HENUMInternalHolder hEnumTyPars2(pInternalImport2); From 3b21879a60e152bbc3d3e1d55542c63eb58cc12d Mon Sep 17 00:00:00 2001 From: David Wrighton Date: Thu, 1 Jun 2023 16:20:01 -0700 Subject: [PATCH 19/23] Fix nits noticed by Aaron --- docs/design/coreclr/botr/readytorun-format.md | 6 +++--- src/coreclr/inc/readytorun.h | 2 ++ .../Internal/Runtime/ReadyToRunConstants.cs | 1 - .../TypeSystem/Common/GenericParameterDesc.cs | 19 ++++++++++++++++++- .../ReadyToRun/EnclosingTypeMapNode.cs | 1 + 5 files changed, 24 insertions(+), 5 deletions(-) diff --git a/docs/design/coreclr/botr/readytorun-format.md b/docs/design/coreclr/botr/readytorun-format.md index 09f4c2184c8d81..deed8947d22c57 100644 --- a/docs/design/coreclr/botr/readytorun-format.md +++ b/docs/design/coreclr/botr/readytorun-format.md @@ -649,9 +649,9 @@ The entry of the hashtable is a counted sequence of compressed unsigned integers This section may be included in addition to a InliningInfo2 section. ## ReadyToRunSectionType.MethodIsGenericMap (v9.0+) -This optional section holds a bit vector to indicate if the MethodDef's contained within the assembly have generic parameters or not. This allowes determinining if a method is generic or not by querying a bit vector (which is fast, and efficient) as opposed to examining the GenericParameter table, or the signature of the Method. +This optional section holds a bit vector to indicate if the MethodDefs contained within the assembly have generic parameters or not. This allows determining if a method is generic or not by querying a bit vector (which is fast, and efficient) as opposed to examining the GenericParameter table, or the signature of the Method. -The section begins with a single 32bit integer indicating the number of bits in the bit vector. Following that integer is the actual bit vector of all of the data. The data is grouped into 8 bit bytes, where the least significant bit of the byte is the bit which represents the lowest MethodDef. +The section begins with a single 32 bit integer indicating the number of bits in the bit vector. Following that integer is the actual bit vector of all of the data. The data is grouped into 8 bit bytes, where the least significant bit of the byte is the bit which represents the lowest MethodDef. For instance, the first byte in the bit vector represents the MethodDefs 06000001 to 06000008, and the least signficant bit of that first byte is the bit representing the IsGeneric bit for MethodDef 06000001. @@ -667,7 +667,7 @@ This count is followed by a 16 bit unsigned integer for each TypeDef defined in This optional section represents a condensed view of some generic details about types. This can make it more efficient to load types. The structure of this section is: -A single 32bit integer representing the number of entries in the map followed by a series of 4 bit entries, one per type. These 4 bit entries are grouped into bytes, where each byte holds 2 entries, and the entry in the most signficant 4 bits of the byte is the entry representing a lower TypeDef RID. +A single 32 bit integer representing the number of entries in the map followed by a series of 4 bit entries, one per type. These 4 bit entries are grouped into bytes, where each byte holds 2 entries, and the entry in the most significant 4 bits of the byte is the entry representing a lower TypeDef RID. TypeGenericInfoMap entries have 4 bits representing 3 different sets of information. diff --git a/src/coreclr/inc/readytorun.h b/src/coreclr/inc/readytorun.h index 2d4459ab156684..5bd653458cea5d 100644 --- a/src/coreclr/inc/readytorun.h +++ b/src/coreclr/inc/readytorun.h @@ -124,6 +124,7 @@ enum class ReadyToRunImportSectionFlags : uint16_t PCode = 0x0004, // Section contains pointers to code }; +// All values in this enum should within a nibble (4 bits). enum class ReadyToRunTypeGenericInfo : uint8_t { GenericCountMask = 0x3, @@ -131,6 +132,7 @@ enum class ReadyToRunTypeGenericInfo : uint8_t HasVariance = 0x8, }; +// All values in this enum should fit within 2 bits. enum class ReadyToRunGenericInfoGenericCount : uint32_t { Zero = 0, diff --git a/src/coreclr/tools/Common/Internal/Runtime/ReadyToRunConstants.cs b/src/coreclr/tools/Common/Internal/Runtime/ReadyToRunConstants.cs index 3c108321482e27..4c15ab6ded7c79 100644 --- a/src/coreclr/tools/Common/Internal/Runtime/ReadyToRunConstants.cs +++ b/src/coreclr/tools/Common/Internal/Runtime/ReadyToRunConstants.cs @@ -105,7 +105,6 @@ public enum ReadyToRunGenericInfoGenericCount : uint MoreThanTwo = 3 } - public enum DictionaryEntryKind { EmptySlot = 0, diff --git a/src/coreclr/tools/Common/TypeSystem/Common/GenericParameterDesc.cs b/src/coreclr/tools/Common/TypeSystem/Common/GenericParameterDesc.cs index ddcbd4a4ea8466..e1bf789b1123ed 100644 --- a/src/coreclr/tools/Common/TypeSystem/Common/GenericParameterDesc.cs +++ b/src/coreclr/tools/Common/TypeSystem/Common/GenericParameterDesc.cs @@ -92,7 +92,6 @@ public virtual string Name /// public abstract TypeSystemEntity AssociatedTypeOrMethod { get; } - /// /// Gets a value indicating the variance of this generic parameter. /// @@ -126,6 +125,9 @@ public virtual IEnumerable TypeConstraints } } + /// + /// Does this generic parameter have the NotNullableValueType constraint flag + /// public bool HasNotNullableValueTypeConstraint { get @@ -134,6 +136,9 @@ public bool HasNotNullableValueTypeConstraint } } + /// + /// Does this generic parameter have the ReferenceType constraint flag + /// public bool HasReferenceTypeConstraint { get @@ -142,6 +147,9 @@ public bool HasReferenceTypeConstraint } } + /// + /// Does this generic parameter have the DefaultConstructor constraint flag + /// public bool HasDefaultConstructorConstraint { get @@ -150,6 +158,9 @@ public bool HasDefaultConstructorConstraint } } + /// + /// Does this generic parameter have the AcceptByRefLike flag + /// public bool HasAcceptByRefLikeConstraint { get @@ -158,6 +169,9 @@ public bool HasAcceptByRefLikeConstraint } } + /// + /// Is this generic parameter Covariant + /// public bool IsCovariant { get @@ -166,6 +180,9 @@ public bool IsCovariant } } + /// + /// Is this generic parameter Contravariant + /// public bool IsContravariant { get diff --git a/src/coreclr/tools/aot/ILCompiler.ReadyToRun/Compiler/DependencyAnalysis/ReadyToRun/EnclosingTypeMapNode.cs b/src/coreclr/tools/aot/ILCompiler.ReadyToRun/Compiler/DependencyAnalysis/ReadyToRun/EnclosingTypeMapNode.cs index 984fc83366a260..81c94973ca0700 100644 --- a/src/coreclr/tools/aot/ILCompiler.ReadyToRun/Compiler/DependencyAnalysis/ReadyToRun/EnclosingTypeMapNode.cs +++ b/src/coreclr/tools/aot/ILCompiler.ReadyToRun/Compiler/DependencyAnalysis/ReadyToRun/EnclosingTypeMapNode.cs @@ -49,6 +49,7 @@ public override ObjectData GetData(NodeFactory factory, bool relocsOnly = false) builder.AddSymbol(this); // This map is only valid for assemblies with <= 0xFFFE types defined within + Debug.Assert(_metadata.TypeDefinitions.Count <= 0xFFFE); builder.EmitUShort(checked((ushort)_metadata.TypeDefinitions.Count)); foreach (var typeDefinitionHandle in _metadata.TypeDefinitions) From 06b6ffd80b601363d2654204c745ba68f4a6d062 Mon Sep 17 00:00:00 2001 From: David Wrighton Date: Thu, 1 Jun 2023 16:46:59 -0700 Subject: [PATCH 20/23] Add some const correctness to the world --- src/coreclr/vm/ceeload.h | 6 +++--- src/coreclr/vm/readytoruninfo.cpp | 26 +++++++++++++------------- src/coreclr/vm/readytoruninfo.h | 28 ++++++++++++++-------------- 3 files changed, 30 insertions(+), 30 deletions(-) diff --git a/src/coreclr/vm/ceeload.h b/src/coreclr/vm/ceeload.h index cf8fac5b39f84a..56dd2a4d25a6ab 100644 --- a/src/coreclr/vm/ceeload.h +++ b/src/coreclr/vm/ceeload.h @@ -565,7 +565,7 @@ class ModuleBase return NULL; }; - ReadyToRun_EnclosingTypeMap *m_pEnclosingTypeMap = &ReadyToRun_EnclosingTypeMap::EmptyInstance; + const ReadyToRun_EnclosingTypeMap *m_pEnclosingTypeMap = &ReadyToRun_EnclosingTypeMap::EmptyInstance; #ifndef DACCESS_COMPILE // The vtable needs to match between DAC and non-DAC, but we don't want any use of ThrowTypeLoadException in the DAC @@ -1619,8 +1619,8 @@ class Module : public ModuleBase // Self-initializing accessor for domain-independent IJW thunk heap LoaderHeap *GetDllThunkHeap(); - ReadyToRun_MethodIsGenericMap *m_pMethodIsGenericMap = &ReadyToRun_MethodIsGenericMap::EmptyInstance; - ReadyToRun_TypeGenericInfoMap *m_pTypeGenericInfoMap = &ReadyToRun_TypeGenericInfoMap::EmptyInstance; + const ReadyToRun_MethodIsGenericMap *m_pMethodIsGenericMap = &ReadyToRun_MethodIsGenericMap::EmptyInstance; + const ReadyToRun_TypeGenericInfoMap *m_pTypeGenericInfoMap = &ReadyToRun_TypeGenericInfoMap::EmptyInstance; protected: diff --git a/src/coreclr/vm/readytoruninfo.cpp b/src/coreclr/vm/readytoruninfo.cpp index 033d73720ee356..0744fd530a54ca 100644 --- a/src/coreclr/vm/readytoruninfo.cpp +++ b/src/coreclr/vm/readytoruninfo.cpp @@ -1791,11 +1791,11 @@ ModuleBase* CreateNativeManifestModule(LoaderAllocator* pLoaderAllocator, IMDInt } #endif // DACCESS_COMPILE -ReadyToRun_EnclosingTypeMap ReadyToRun_EnclosingTypeMap::EmptyInstance; -ReadyToRun_TypeGenericInfoMap ReadyToRun_TypeGenericInfoMap::EmptyInstance; -ReadyToRun_MethodIsGenericMap ReadyToRun_MethodIsGenericMap::EmptyInstance; +const ReadyToRun_EnclosingTypeMap ReadyToRun_EnclosingTypeMap::EmptyInstance; +const ReadyToRun_TypeGenericInfoMap ReadyToRun_TypeGenericInfoMap::EmptyInstance; +const ReadyToRun_MethodIsGenericMap ReadyToRun_MethodIsGenericMap::EmptyInstance; -mdTypeDef ReadyToRun_EnclosingTypeMap::GetEnclosingType(mdTypeDef input, IMDInternalImport* pImport) +mdTypeDef ReadyToRun_EnclosingTypeMap::GetEnclosingType(mdTypeDef input, IMDInternalImport* pImport) const { uint32_t rid = RidFromToken(input); if ((rid > TypeCount) || (rid == 0)) @@ -1815,7 +1815,7 @@ mdTypeDef ReadyToRun_EnclosingTypeMap::GetEnclosingType(mdTypeDef input, IMDInte return TokenFromRid((&TypeCount)[rid], mdtTypeDef); } -HRESULT ReadyToRun_EnclosingTypeMap::GetEnclosingTypeNoThrow(mdTypeDef input, mdTypeDef *pEnclosingType, IMDInternalImport* pImport) +HRESULT ReadyToRun_EnclosingTypeMap::GetEnclosingTypeNoThrow(mdTypeDef input, mdTypeDef *pEnclosingType, IMDInternalImport* pImport) const { uint32_t rid = RidFromToken(input); if ((rid > TypeCount) || (rid == 0)) @@ -1832,7 +1832,7 @@ HRESULT ReadyToRun_EnclosingTypeMap::GetEnclosingTypeNoThrow(mdTypeDef input, md return S_OK; } -ReadyToRunTypeGenericInfo ReadyToRun_TypeGenericInfoMap::GetTypeGenericInfo(mdTypeDef input, bool *foundResult) +ReadyToRunTypeGenericInfo ReadyToRun_TypeGenericInfoMap::GetTypeGenericInfo(mdTypeDef input, bool *foundResult) const { uint32_t rid = RidFromToken(input); if ((rid > TypeCount) || (rid == 0)) @@ -1851,7 +1851,7 @@ ReadyToRunTypeGenericInfo ReadyToRun_TypeGenericInfoMap::GetTypeGenericInfo(mdTy return static_cast(entry); } -bool ReadyToRun_TypeGenericInfoMap::IsGeneric(mdTypeDef input, IMDInternalImport* pImport) +bool ReadyToRun_TypeGenericInfoMap::IsGeneric(mdTypeDef input, IMDInternalImport* pImport) const { bool foundResult; ReadyToRunTypeGenericInfo typeGenericInfo = GetTypeGenericInfo(input, &foundResult); @@ -1864,7 +1864,7 @@ bool ReadyToRun_TypeGenericInfoMap::IsGeneric(mdTypeDef input, IMDInternalImport return !!((uint8_t)typeGenericInfo & (uint8_t)ReadyToRunTypeGenericInfo::GenericCountMask); } -HRESULT ReadyToRun_TypeGenericInfoMap::IsGenericNoThrow(mdTypeDef input, bool *pIsGeneric, IMDInternalImport* pImport) +HRESULT ReadyToRun_TypeGenericInfoMap::IsGenericNoThrow(mdTypeDef input, bool *pIsGeneric, IMDInternalImport* pImport) const { bool foundResult; bool result; @@ -1883,7 +1883,7 @@ HRESULT ReadyToRun_TypeGenericInfoMap::IsGenericNoThrow(mdTypeDef input, bool *p return S_OK; } -uint32_t ReadyToRun_TypeGenericInfoMap::GetGenericArgumentCount(mdTypeDef input, IMDInternalImport* pImport) +uint32_t ReadyToRun_TypeGenericInfoMap::GetGenericArgumentCount(mdTypeDef input, IMDInternalImport* pImport) const { bool foundResult; ReadyToRunTypeGenericInfo typeGenericInfo = GetTypeGenericInfo(input, &foundResult); @@ -1900,7 +1900,7 @@ uint32_t ReadyToRun_TypeGenericInfoMap::GetGenericArgumentCount(mdTypeDef input, return count; } -HRESULT ReadyToRun_TypeGenericInfoMap::GetGenericArgumentCountNoThrow(mdTypeDef input, uint32_t *pCount, IMDInternalImport* pImport) +HRESULT ReadyToRun_TypeGenericInfoMap::GetGenericArgumentCountNoThrow(mdTypeDef input, uint32_t *pCount, IMDInternalImport* pImport) const { HRESULT hr; bool foundResult; @@ -1921,19 +1921,19 @@ HRESULT ReadyToRun_TypeGenericInfoMap::GetGenericArgumentCountNoThrow(mdTypeDef return hr; } -bool ReadyToRun_TypeGenericInfoMap::HasVariance(mdTypeDef input, bool *foundResult) +bool ReadyToRun_TypeGenericInfoMap::HasVariance(mdTypeDef input, bool *foundResult) const { ReadyToRunTypeGenericInfo typeGenericInfo = GetTypeGenericInfo(input, foundResult); return !!((uint8_t)typeGenericInfo & (uint8_t)ReadyToRunTypeGenericInfo::HasVariance); } -bool ReadyToRun_TypeGenericInfoMap::HasConstraints(mdTypeDef input, bool *foundResult) +bool ReadyToRun_TypeGenericInfoMap::HasConstraints(mdTypeDef input, bool *foundResult) const { ReadyToRunTypeGenericInfo typeGenericInfo = GetTypeGenericInfo(input, foundResult); return !!((uint8_t)typeGenericInfo & (uint8_t)ReadyToRunTypeGenericInfo::HasVariance); } -bool ReadyToRun_MethodIsGenericMap::IsGeneric(mdMethodDef input, bool *foundResult) +bool ReadyToRun_MethodIsGenericMap::IsGeneric(mdMethodDef input, bool *foundResult) const { uint32_t rid = RidFromToken(input); if ((rid > MethodCount) || (rid == 0)) diff --git a/src/coreclr/vm/readytoruninfo.h b/src/coreclr/vm/readytoruninfo.h index d1af0fdeb5dd25..c43dd0b2b152d0 100644 --- a/src/coreclr/vm/readytoruninfo.h +++ b/src/coreclr/vm/readytoruninfo.h @@ -54,34 +54,34 @@ class ReadyToRun_EnclosingTypeMap private: uint16_t TypeCount = 0; public: - static ReadyToRun_EnclosingTypeMap EmptyInstance; + const static ReadyToRun_EnclosingTypeMap EmptyInstance; - mdTypeDef GetEnclosingType(mdTypeDef input, IMDInternalImport* pImport); - HRESULT GetEnclosingTypeNoThrow(mdTypeDef input, mdTypeDef *pEnclosingType, IMDInternalImport* pImport); + mdTypeDef GetEnclosingType(mdTypeDef input, IMDInternalImport* pImport) const; + HRESULT GetEnclosingTypeNoThrow(mdTypeDef input, mdTypeDef *pEnclosingType, IMDInternalImport* pImport) const; }; class ReadyToRun_TypeGenericInfoMap { private: uint32_t TypeCount = 0; - ReadyToRunTypeGenericInfo GetTypeGenericInfo(mdTypeDef input, bool *foundResult); + ReadyToRunTypeGenericInfo GetTypeGenericInfo(mdTypeDef input, bool *foundResult) const; public: - static ReadyToRun_TypeGenericInfoMap EmptyInstance; - - HRESULT IsGenericNoThrow(mdTypeDef input, bool *pIsGeneric, IMDInternalImport* pImport); - HRESULT GetGenericArgumentCountNoThrow(mdTypeDef input, uint32_t *pCount, IMDInternalImport* pImport); - bool IsGeneric(mdTypeDef input, IMDInternalImport* pImport); - uint32_t GetGenericArgumentCount(mdTypeDef input, IMDInternalImport* pImport); - bool HasVariance(mdTypeDef input, bool *foundResult); - bool HasConstraints(mdTypeDef input, bool *foundResult); + const static ReadyToRun_TypeGenericInfoMap EmptyInstance; + + HRESULT IsGenericNoThrow(mdTypeDef input, bool *pIsGeneric, IMDInternalImport* pImport) const; + HRESULT GetGenericArgumentCountNoThrow(mdTypeDef input, uint32_t *pCount, IMDInternalImport* pImport) const; + bool IsGeneric(mdTypeDef input, IMDInternalImport* pImport) const; + uint32_t GetGenericArgumentCount(mdTypeDef input, IMDInternalImport* pImport) const; + bool HasVariance(mdTypeDef input, bool *foundResult) const; + bool HasConstraints(mdTypeDef input, bool *foundResult) const; }; class ReadyToRun_MethodIsGenericMap { uint32_t MethodCount = 0; public: - static ReadyToRun_MethodIsGenericMap EmptyInstance; - bool IsGeneric(mdMethodDef input, bool *foundResult); + const static ReadyToRun_MethodIsGenericMap EmptyInstance; + bool IsGeneric(mdMethodDef input, bool *foundResult) const; }; class ReadyToRunInfo From df3a418a244519d187d96b386dc5c224d2a04c14 Mon Sep 17 00:00:00 2001 From: David Wrighton Date: Fri, 2 Jun 2023 16:57:52 -0700 Subject: [PATCH 21/23] Fix issues noted by Michal, as well as remaining constrain checking issues --- .../TypeSystem/RuntimeGenericParameterDesc.cs | 14 +++----- .../Internal/TypeSystem/RuntimeMethodDesc.cs | 3 +- .../TypeSystem/RuntimeNoMetadataType.cs | 35 ++++++++++++++++++- .../TypeSystem/TypeSystemContext.Runtime.cs | 18 ++-------- .../TypeSystem/Common/SignatureVariable.cs | 2 -- ...impleArrayOfTRuntimeInterfacesAlgorithm.cs | 10 +++--- .../TypeSystem/Ecma/EcmaGenericParameter.cs | 11 ++---- .../ILVerification/ILVerification.projitems | 4 ++- .../InstantiatedGenericParameter.cs | 15 +++++++- .../ReadyToRun/TypeValidationChecker.cs | 4 +-- .../Compiler/ReadyToRunCompilerContext.cs | 7 +++- .../ILCompiler.TypeSystem.csproj | 3 ++ 12 files changed, 75 insertions(+), 51 deletions(-) rename src/coreclr/tools/{ILVerification => Common/TypeSystem/Common}/SimpleArrayOfTRuntimeInterfacesAlgorithm.cs (91%) diff --git a/src/coreclr/nativeaot/System.Private.TypeLoader/src/Internal/TypeSystem/RuntimeGenericParameterDesc.cs b/src/coreclr/nativeaot/System.Private.TypeLoader/src/Internal/TypeSystem/RuntimeGenericParameterDesc.cs index fb887c892af48f..a24f4789b50648 100644 --- a/src/coreclr/nativeaot/System.Private.TypeLoader/src/Internal/TypeSystem/RuntimeGenericParameterDesc.cs +++ b/src/coreclr/nativeaot/System.Private.TypeLoader/src/Internal/TypeSystem/RuntimeGenericParameterDesc.cs @@ -7,15 +7,14 @@ internal class RuntimeGenericParameterDesc : GenericParameterDesc { private readonly GenericParameterKind _kind; private readonly int _index; - private readonly TypeSystemContext _context; private readonly GenericVariance _variance; - private TypeSystemEntity _associatedTypeOrMethod; + private readonly TypeSystemEntity _associatedTypeOrMethod; - public RuntimeGenericParameterDesc(GenericParameterKind kind, int index, TypeSystemContext context, GenericVariance variance) + public RuntimeGenericParameterDesc(GenericParameterKind kind, int index, TypeSystemEntity associatedTypeOrMethod, GenericVariance variance) { _kind = kind; _index = index; - _context = context; + _associatedTypeOrMethod = associatedTypeOrMethod; _variance = variance; } @@ -23,15 +22,10 @@ public RuntimeGenericParameterDesc(GenericParameterKind kind, int index, TypeSys public override int Index => _index; - public override TypeSystemContext Context => _context; + public override TypeSystemContext Context => _associatedTypeOrMethod.Context; public override GenericVariance Variance => _variance; public override TypeSystemEntity AssociatedTypeOrMethod => _associatedTypeOrMethod; - - internal void SetAssociatedTypeOrMethod(TypeSystemEntity associatedTypeOrMethod) - { - _associatedTypeOrMethod = associatedTypeOrMethod; - } } } diff --git a/src/coreclr/nativeaot/System.Private.TypeLoader/src/Internal/TypeSystem/RuntimeMethodDesc.cs b/src/coreclr/nativeaot/System.Private.TypeLoader/src/Internal/TypeSystem/RuntimeMethodDesc.cs index 36738ccfbedd14..d2dcf815171a34 100644 --- a/src/coreclr/nativeaot/System.Private.TypeLoader/src/Internal/TypeSystem/RuntimeMethodDesc.cs +++ b/src/coreclr/nativeaot/System.Private.TypeLoader/src/Internal/TypeSystem/RuntimeMethodDesc.cs @@ -52,8 +52,7 @@ public override Instantiation Instantiation TypeDesc[] genericParameters = new TypeDesc[genericArgCount]; for (int i = 0; i < genericParameters.Length; i++) { - var newGenericParameter = new RuntimeGenericParameterDesc(GenericParameterKind.Method, i, Context, GenericVariance.None); - newGenericParameter.SetAssociatedTypeOrMethod(this); + var newGenericParameter = new RuntimeGenericParameterDesc(GenericParameterKind.Method, i, this, GenericVariance.None); genericParameters[i] = newGenericParameter; } _instantiation = new Instantiation(genericParameters); diff --git a/src/coreclr/nativeaot/System.Private.TypeLoader/src/Internal/TypeSystem/RuntimeNoMetadataType.cs b/src/coreclr/nativeaot/System.Private.TypeLoader/src/Internal/TypeSystem/RuntimeNoMetadataType.cs index bcbc5c6a10c386..3e4b9c96641184 100644 --- a/src/coreclr/nativeaot/System.Private.TypeLoader/src/Internal/TypeSystem/RuntimeNoMetadataType.cs +++ b/src/coreclr/nativeaot/System.Private.TypeLoader/src/Internal/TypeSystem/RuntimeNoMetadataType.cs @@ -34,13 +34,46 @@ internal class NoMetadataType : DefType // "_baseType == this" means "base type was not initialized yet" private DefType _baseType; - public NoMetadataType(TypeSystemContext context, RuntimeTypeHandle genericTypeDefinition, DefType genericTypeDefinitionAsDefType, Instantiation instantiation, int hashcode) + public unsafe NoMetadataType(TypeSystemContext context, RuntimeTypeHandle genericTypeDefinition, int instantiationLength, ReadOnlySpan runtimeVarianceData, int hashcode) + { + TypeDesc[] genericParameters; + if (instantiationLength == 0) + { + genericParameters = Array.Empty(); + } + else + { + genericParameters = new TypeDesc[instantiationLength]; + for (int i = 0; i < genericParameters.Length; i++) + { + GenericVariance variance = runtimeVarianceData.Length == 0 ? GenericVariance.None : runtimeVarianceData[i] switch + { + Runtime.GenericVariance.Contravariant => GenericVariance.Contravariant, + Runtime.GenericVariance.Covariant => GenericVariance.Covariant, + Runtime.GenericVariance.NonVariant or Runtime.GenericVariance.ArrayCovariant => GenericVariance.None, + _ => throw new NotImplementedException() + }; + genericParameters[i] = new RuntimeGenericParameterDesc(GenericParameterKind.Type, i, this, variance); + } + } + + Instantiation instantiation = new Instantiation(genericParameters); + Init(context, genericTypeDefinition, null, instantiation, hashcode); + } + + public unsafe NoMetadataType(TypeSystemContext context, RuntimeTypeHandle genericTypeDefinition, DefType genericTypeDefinitionAsDefType, Instantiation instantiation, int hashcode) + { + Init(context, genericTypeDefinition, genericTypeDefinitionAsDefType, instantiation, hashcode); + } + + private void Init(TypeSystemContext context, RuntimeTypeHandle genericTypeDefinition, DefType genericTypeDefinitionAsDefType, Instantiation instantiation, int hashcode) { _hashcode = hashcode; _context = context; _genericTypeDefinition = genericTypeDefinition; _genericTypeDefinitionAsDefType = genericTypeDefinitionAsDefType; _genericTypeDefinitionAsDefType ??= this; + _instantiation = instantiation; // Instantiation must either be: diff --git a/src/coreclr/nativeaot/System.Private.TypeLoader/src/Internal/TypeSystem/TypeSystemContext.Runtime.cs b/src/coreclr/nativeaot/System.Private.TypeLoader/src/Internal/TypeSystem/TypeSystemContext.Runtime.cs index 702b6c76c28bca..7abe07757f0d3c 100644 --- a/src/coreclr/nativeaot/System.Private.TypeLoader/src/Internal/TypeSystem/TypeSystemContext.Runtime.cs +++ b/src/coreclr/nativeaot/System.Private.TypeLoader/src/Internal/TypeSystem/TypeSystemContext.Runtime.cs @@ -200,23 +200,9 @@ public TypeDesc ResolveRuntimeTypeHandle(RuntimeTypeHandle rtth) TypeDesc[] genericParameters = new TypeDesc[rtth.ToEETypePtr()->GenericParameterCount]; Runtime.GenericVariance* runtimeVariance = rtth.ToEETypePtr()->HasGenericVariance ? rtth.ToEETypePtr()->GenericVariance : null; - for (int i = 0; i < genericParameters.Length; i++) - { - GenericVariance variance = runtimeVariance == null ? GenericVariance.None : runtimeVariance[i] switch - { - Runtime.GenericVariance.Contravariant => GenericVariance.Contravariant, - Runtime.GenericVariance.Covariant => GenericVariance.Covariant, - Runtime.GenericVariance.NonVariant or Runtime.GenericVariance.ArrayCovariant => GenericVariance.None, - _ => throw new NotImplementedException() - }; - genericParameters[i] = genericParameters[i] = new RuntimeGenericParameterDesc(GenericParameterKind.Type, i, this, variance); - } + ReadOnlySpan varianceData = new ReadOnlySpan(runtimeVariance, runtimeVariance == null ? 0 : genericParameters.Length); - returnedType = new NoMetadataType(this, rtth, null, new Instantiation(genericParameters), rtth.GetHashCode()); - for (int i = 0; i < genericParameters.Length; i++) - { - ((RuntimeGenericParameterDesc)genericParameters[i]).SetAssociatedTypeOrMethod(returnedType); - } + returnedType = new NoMetadataType(this, rtth, genericParameters.Length, varianceData, rtth.GetHashCode()); } } else if (RuntimeAugments.IsGenericType(rtth)) diff --git a/src/coreclr/tools/Common/TypeSystem/Common/SignatureVariable.cs b/src/coreclr/tools/Common/TypeSystem/Common/SignatureVariable.cs index 0b72c0ef657c93..b3184284f675bd 100644 --- a/src/coreclr/tools/Common/TypeSystem/Common/SignatureVariable.cs +++ b/src/coreclr/tools/Common/TypeSystem/Common/SignatureVariable.cs @@ -66,8 +66,6 @@ protected override TypeFlags ComputeTypeFlags(TypeFlags mask) flags |= TypeFlags.SignatureTypeVariable; } - flags |= TypeFlags.HasGenericVarianceComputed; - flags |= TypeFlags.HasFinalizerComputed; flags |= TypeFlags.AttributeCacheComputed; return flags; diff --git a/src/coreclr/tools/ILVerification/SimpleArrayOfTRuntimeInterfacesAlgorithm.cs b/src/coreclr/tools/Common/TypeSystem/Common/SimpleArrayOfTRuntimeInterfacesAlgorithm.cs similarity index 91% rename from src/coreclr/tools/ILVerification/SimpleArrayOfTRuntimeInterfacesAlgorithm.cs rename to src/coreclr/tools/Common/TypeSystem/Common/SimpleArrayOfTRuntimeInterfacesAlgorithm.cs index 402d936d3ff81c..166f09fc7deb41 100644 --- a/src/coreclr/tools/ILVerification/SimpleArrayOfTRuntimeInterfacesAlgorithm.cs +++ b/src/coreclr/tools/Common/TypeSystem/Common/SimpleArrayOfTRuntimeInterfacesAlgorithm.cs @@ -3,18 +3,16 @@ using System; using System.Diagnostics; -using Internal.IL; -using Internal.TypeSystem; -namespace ILVerify +namespace Internal.TypeSystem { - internal class SimpleArrayOfTRuntimeInterfacesAlgorithm : RuntimeInterfacesAlgorithm + public class SimpleArrayOfTRuntimeInterfacesAlgorithm : RuntimeInterfacesAlgorithm { private DefType[] _arrayRuntimeInterfaces; private MetadataType[] _genericRuntimeInterfaces; private ModuleDesc _systemModule; - private static readonly string[] s_genericRuntimeInterfacesNames = + private static readonly string[] s_genericRuntimeInterfacesNames = { "IEnumerable`1", "ICollection`1", @@ -28,7 +26,7 @@ public SimpleArrayOfTRuntimeInterfacesAlgorithm(ModuleDesc systemModule) _systemModule = systemModule; // initialize interfaces - _arrayRuntimeInterfaces = _systemModule.GetType("System", "Array")?.RuntimeInterfaces + _arrayRuntimeInterfaces = _systemModule.GetType("System", "Array")?.RuntimeInterfaces ?? Array.Empty(); _genericRuntimeInterfaces = new MetadataType[s_genericRuntimeInterfacesNames.Length]; diff --git a/src/coreclr/tools/Common/TypeSystem/Ecma/EcmaGenericParameter.cs b/src/coreclr/tools/Common/TypeSystem/Ecma/EcmaGenericParameter.cs index 1f6b5e8cddcb61..ac6cd4307ca7c5 100644 --- a/src/coreclr/tools/Common/TypeSystem/Ecma/EcmaGenericParameter.cs +++ b/src/coreclr/tools/Common/TypeSystem/Ecma/EcmaGenericParameter.cs @@ -13,7 +13,6 @@ public sealed partial class EcmaGenericParameter : GenericParameterDesc { private EcmaModule _module; private GenericParameterHandle _handle; - private TypeSystemEntity _associatedTypeOrMethod; internal EcmaGenericParameter(EcmaModule module, GenericParameterHandle handle) { @@ -83,15 +82,9 @@ public override TypeSystemEntity AssociatedTypeOrMethod { get { - TypeSystemEntity tse = Volatile.Read(ref _associatedTypeOrMethod); - if (tse == null) - { - GenericParameter parameter = _module.MetadataReader.GetGenericParameter(_handle); + GenericParameter parameter = _module.MetadataReader.GetGenericParameter(_handle); - tse = (TypeSystemEntity)_module.GetObject(parameter.Parent); - Volatile.Write(ref _associatedTypeOrMethod, tse); - } - return tse; + return (TypeSystemEntity)_module.GetObject(parameter.Parent); } } diff --git a/src/coreclr/tools/ILVerification/ILVerification.projitems b/src/coreclr/tools/ILVerification/ILVerification.projitems index 61a4a8c280df0b..8c5e2e5049dd71 100644 --- a/src/coreclr/tools/ILVerification/ILVerification.projitems +++ b/src/coreclr/tools/ILVerification/ILVerification.projitems @@ -6,7 +6,6 @@ - @@ -52,6 +51,9 @@ TypeSystem\Common\ResolutionFailure.cs + + TypeSystem\Common\SimpleArrayOfTRuntimeInterfacesAlgorithm.cs + TypeSystem\Common\TypeSystemEntity.cs diff --git a/src/coreclr/tools/ILVerification/InstantiatedGenericParameter.cs b/src/coreclr/tools/ILVerification/InstantiatedGenericParameter.cs index c467d625c20af3..ef92f4d0dbd7f0 100644 --- a/src/coreclr/tools/ILVerification/InstantiatedGenericParameter.cs +++ b/src/coreclr/tools/ILVerification/InstantiatedGenericParameter.cs @@ -76,7 +76,20 @@ private InstantiatedGenericParameter(GenericParameterDesc genericParam) public override GenericConstraints Constraints => _genericParam.Constraints; - public override TypeSystemEntity AssociatedTypeOrMethod => _genericParam.AssociatedTypeOrMethod; + public override TypeSystemEntity AssociatedTypeOrMethod + { + get + { + if (_genericParam.AssociatedTypeOrMethod is TypeDesc associatedType) + { + return associatedType.InstantiateSignature(_typeInstantiation, _methodInstantiation); + } + else + { + return ((MethodDesc)_genericParam.AssociatedTypeOrMethod).InstantiateSignature(_typeInstantiation, _methodInstantiation); + } + } + } public override IEnumerable TypeConstraints { diff --git a/src/coreclr/tools/aot/ILCompiler.ReadyToRun/Compiler/DependencyAnalysis/ReadyToRun/TypeValidationChecker.cs b/src/coreclr/tools/aot/ILCompiler.ReadyToRun/Compiler/DependencyAnalysis/ReadyToRun/TypeValidationChecker.cs index c879415449f57a..7f8a754f256981 100644 --- a/src/coreclr/tools/aot/ILCompiler.ReadyToRun/Compiler/DependencyAnalysis/ReadyToRun/TypeValidationChecker.cs +++ b/src/coreclr/tools/aot/ILCompiler.ReadyToRun/Compiler/DependencyAnalysis/ReadyToRun/TypeValidationChecker.cs @@ -487,12 +487,12 @@ static bool CompareGenericParameterConstraint(MethodDesc declMethod, GenericPara HashSet constraintsOnDecl = new HashSet(); foreach (var constraint in parameterOfDecl.TypeConstraints) { - constraintsOnDecl.Add(constraint.InstantiateSignature(declMethod.Instantiation, default(Instantiation))); + constraintsOnDecl.Add(constraint.InstantiateSignature(declMethod.OwningType.Instantiation, implMethod.Instantiation).InstantiateSignature(implMethod.OwningType.Instantiation, new Instantiation())); } foreach (var constraint in parameterOfImpl.TypeConstraints) { - if (!constraintsOnDecl.Contains(constraint.InstantiateSignature(implMethod.Instantiation, default(Instantiation)))) + if (!constraintsOnDecl.Contains(constraint.InstantiateSignature(implMethod.OwningType.Instantiation, implMethod.Instantiation))) return false; } diff --git a/src/coreclr/tools/aot/ILCompiler.ReadyToRun/Compiler/ReadyToRunCompilerContext.cs b/src/coreclr/tools/aot/ILCompiler.ReadyToRun/Compiler/ReadyToRunCompilerContext.cs index 6eed36223b0992..54a1aefdddf933 100644 --- a/src/coreclr/tools/aot/ILCompiler.ReadyToRun/Compiler/ReadyToRunCompilerContext.cs +++ b/src/coreclr/tools/aot/ILCompiler.ReadyToRun/Compiler/ReadyToRunCompilerContext.cs @@ -37,6 +37,7 @@ public partial class ReadyToRunCompilerContext : CompilerTypeSystemContext private VectorOfTFieldLayoutAlgorithm _vectorOfTFieldLayoutAlgorithm; private VectorFieldLayoutAlgorithm _vectorFieldLayoutAlgorithm; private Int128FieldLayoutAlgorithm _int128FieldLayoutAlgorithm; + private RuntimeInterfacesAlgorithm _arrayOfTRuntimeInterfacesAlgorithm; public ReadyToRunCompilerContext(TargetDetails details, SharedGenericsMode genericsMode, bool bubbleIncludesCorelib, InstructionSetSupport instructionSetSupport, CompilerTypeSystemContext oldTypeSystemContext = null) : base(details, genericsMode) @@ -140,7 +141,11 @@ protected override bool ComputeHasGCStaticBase(FieldDesc field) /// protected override RuntimeInterfacesAlgorithm GetRuntimeInterfacesAlgorithmForNonPointerArrayType(ArrayType type) { - return BaseTypeRuntimeInterfacesAlgorithm.Instance; + if (_arrayOfTRuntimeInterfacesAlgorithm == null) + { + _arrayOfTRuntimeInterfacesAlgorithm = new SimpleArrayOfTRuntimeInterfacesAlgorithm(SystemModule); + } + return _arrayOfTRuntimeInterfacesAlgorithm; } TypeDesc _asyncStateMachineBox; diff --git a/src/coreclr/tools/aot/ILCompiler.TypeSystem/ILCompiler.TypeSystem.csproj b/src/coreclr/tools/aot/ILCompiler.TypeSystem/ILCompiler.TypeSystem.csproj index 23f3ac4005a57c..6109776c1d2e03 100644 --- a/src/coreclr/tools/aot/ILCompiler.TypeSystem/ILCompiler.TypeSystem.csproj +++ b/src/coreclr/tools/aot/ILCompiler.TypeSystem/ILCompiler.TypeSystem.csproj @@ -159,6 +159,9 @@ TypeSystem\Common\TypeSystemException.cs + + TypeSystem\Common\SimpleArrayOfTRuntimeInterfacesAlgorithm.cs + TypeSystem\Common\TypeSystemException.Resources.cs From 1f4457514e38fa3fa14c673fe24ff8fb8d2ea2c1 Mon Sep 17 00:00:00 2001 From: David Wrighton Date: Mon, 5 Jun 2023 14:28:55 -0700 Subject: [PATCH 22/23] Code review details --- src/coreclr/inc/readytorun.h | 2 +- src/coreclr/vm/readytoruninfo.h | 11 +++++++++++ 2 files changed, 12 insertions(+), 1 deletion(-) diff --git a/src/coreclr/inc/readytorun.h b/src/coreclr/inc/readytorun.h index 5bd653458cea5d..90092857e6bc2d 100644 --- a/src/coreclr/inc/readytorun.h +++ b/src/coreclr/inc/readytorun.h @@ -93,7 +93,7 @@ enum class ReadyToRunSectionType : uint32_t CrossModuleInlineInfo = 119, // Added in V6.2 HotColdMap = 120, // Added in V8.0 MethodIsGenericMap = 121, // Added in V9.0 - EnclosingTypeMap = 122, // Added in V9.0 + EnclosingTypeMap = 122, // Added in V9.0 TypeGenericInfoMap = 123, // Added in V9.0 // If you add a new section consider whether it is a breaking or non-breaking change. diff --git a/src/coreclr/vm/readytoruninfo.h b/src/coreclr/vm/readytoruninfo.h index c43dd0b2b152d0..68882845f95e97 100644 --- a/src/coreclr/vm/readytoruninfo.h +++ b/src/coreclr/vm/readytoruninfo.h @@ -53,7 +53,10 @@ class ReadyToRun_EnclosingTypeMap { private: uint16_t TypeCount = 0; + ReadyToRun_EnclosingTypeMap() = default; public: + ReadyToRun_EnclosingTypeMap& operator=(const ReadyToRun_EnclosingTypeMap& other) = delete; + ReadyToRun_EnclosingTypeMap(const ReadyToRun_EnclosingTypeMap& other) = delete; const static ReadyToRun_EnclosingTypeMap EmptyInstance; mdTypeDef GetEnclosingType(mdTypeDef input, IMDInternalImport* pImport) const; @@ -65,7 +68,11 @@ class ReadyToRun_TypeGenericInfoMap private: uint32_t TypeCount = 0; ReadyToRunTypeGenericInfo GetTypeGenericInfo(mdTypeDef input, bool *foundResult) const; + ReadyToRun_TypeGenericInfoMap() = default; public: + ReadyToRun_TypeGenericInfoMap& operator=(const ReadyToRun_TypeGenericInfoMap& other) = delete; + ReadyToRun_TypeGenericInfoMap(const ReadyToRun_TypeGenericInfoMap& other) = delete; + const static ReadyToRun_TypeGenericInfoMap EmptyInstance; HRESULT IsGenericNoThrow(mdTypeDef input, bool *pIsGeneric, IMDInternalImport* pImport) const; @@ -79,7 +86,11 @@ class ReadyToRun_TypeGenericInfoMap class ReadyToRun_MethodIsGenericMap { uint32_t MethodCount = 0; + ReadyToRun_MethodIsGenericMap() = default; public: + ReadyToRun_MethodIsGenericMap& operator=(const ReadyToRun_MethodIsGenericMap& other) = delete; + ReadyToRun_MethodIsGenericMap(const ReadyToRun_MethodIsGenericMap& other) = delete; + const static ReadyToRun_MethodIsGenericMap EmptyInstance; bool IsGeneric(mdMethodDef input, bool *foundResult) const; }; From 2648965323345128462ad27ec20fd012e7caa114 Mon Sep 17 00:00:00 2001 From: David Wrighton Date: Mon, 12 Jun 2023 13:01:15 -0700 Subject: [PATCH 23/23] Code review from trylek --- src/coreclr/inc/readytorun.h | 5 +++++ .../Common/Internal/Runtime/ReadyToRunConstants.cs | 5 +++++ .../ReadyToRun/EnclosingTypeMapNode.cs | 12 ++++++------ .../DependencyAnalysis/ReadyToRun/HeaderNode.cs | 3 +++ src/coreclr/vm/readytoruninfo.cpp | 4 +++- 5 files changed, 22 insertions(+), 7 deletions(-) diff --git a/src/coreclr/inc/readytorun.h b/src/coreclr/inc/readytorun.h index 90092857e6bc2d..3e5e4ff5008472 100644 --- a/src/coreclr/inc/readytorun.h +++ b/src/coreclr/inc/readytorun.h @@ -141,6 +141,11 @@ enum class ReadyToRunGenericInfoGenericCount : uint32_t MoreThanTwo = 3 }; +enum class ReadyToRunEnclosingTypeMap +{ + MaxTypeCount = 0xFFFE +}; + // // READYTORUN_IMPORT_SECTION describes image range with references to code or runtime data structures // diff --git a/src/coreclr/tools/Common/Internal/Runtime/ReadyToRunConstants.cs b/src/coreclr/tools/Common/Internal/Runtime/ReadyToRunConstants.cs index 4c15ab6ded7c79..da8a01a543adc3 100644 --- a/src/coreclr/tools/Common/Internal/Runtime/ReadyToRunConstants.cs +++ b/src/coreclr/tools/Common/Internal/Runtime/ReadyToRunConstants.cs @@ -105,6 +105,11 @@ public enum ReadyToRunGenericInfoGenericCount : uint MoreThanTwo = 3 } + public enum ReadyToRunEnclosingTypeMap : uint + { + MaxTypeCount = 0xFFFE + } + public enum DictionaryEntryKind { EmptySlot = 0, diff --git a/src/coreclr/tools/aot/ILCompiler.ReadyToRun/Compiler/DependencyAnalysis/ReadyToRun/EnclosingTypeMapNode.cs b/src/coreclr/tools/aot/ILCompiler.ReadyToRun/Compiler/DependencyAnalysis/ReadyToRun/EnclosingTypeMapNode.cs index 81c94973ca0700..54119453693e13 100644 --- a/src/coreclr/tools/aot/ILCompiler.ReadyToRun/Compiler/DependencyAnalysis/ReadyToRun/EnclosingTypeMapNode.cs +++ b/src/coreclr/tools/aot/ILCompiler.ReadyToRun/Compiler/DependencyAnalysis/ReadyToRun/EnclosingTypeMapNode.cs @@ -20,19 +20,19 @@ public class EnclosingTypeMapNode : ModuleSpecificHeaderTableNode public EnclosingTypeMapNode(EcmaModule module) : base(module) { _metadata = module.MetadataReader; - // This map is only valid for assemblies with <= 0xFFFE types defined within + // This map is only valid for assemblies with <= ReadyToRunEnclosingTypeMap.MaxTypeCount types defined within if (!IsSupported(_metadata)) - throw new InternalCompilerErrorException("EnclosingTypeMap made for assembly with more than 0xFFFE types"); + throw new InternalCompilerErrorException($"EnclosingTypeMap made for assembly with more than 0x{(uint)ReadyToRunEnclosingTypeMap.MaxTypeCount:x} types"); } public static bool IsSupported(MetadataReader metadata) { - // This map is only valid for assemblies with <= 0xFFFE types defined within + // This map is only valid for assemblies with <= ReadyToRunEnclosingTypeMap.MaxTypeCount types defined within // and really shouldn't be generated for tiny assemblies, as the map provides very little to no value // in those situations int typeDefinitionCount = metadata.TypeDefinitions.Count; - return ((typeDefinitionCount > 10) && (typeDefinitionCount <= 0xFFFE)); + return ((typeDefinitionCount > 10) && (typeDefinitionCount <= (int)ReadyToRunEnclosingTypeMap.MaxTypeCount)); } public override int ClassCode => 990540812; @@ -48,8 +48,8 @@ public override ObjectData GetData(NodeFactory factory, bool relocsOnly = false) ObjectDataBuilder builder = new ObjectDataBuilder(factory, relocsOnly); builder.AddSymbol(this); - // This map is only valid for assemblies with <= 0xFFFE types defined within - Debug.Assert(_metadata.TypeDefinitions.Count <= 0xFFFE); + // This map is only valid for assemblies with <= ReadyToRunEnclosingTypeMap.MaxTypeCount types defined within + Debug.Assert(_metadata.TypeDefinitions.Count <= (int)ReadyToRunEnclosingTypeMap.MaxTypeCount); builder.EmitUShort(checked((ushort)_metadata.TypeDefinitions.Count)); foreach (var typeDefinitionHandle in _metadata.TypeDefinitions) diff --git a/src/coreclr/tools/aot/ILCompiler.ReadyToRun/Compiler/DependencyAnalysis/ReadyToRun/HeaderNode.cs b/src/coreclr/tools/aot/ILCompiler.ReadyToRun/Compiler/DependencyAnalysis/ReadyToRun/HeaderNode.cs index 4b6da6a4cc085d..37406d6eafc71e 100644 --- a/src/coreclr/tools/aot/ILCompiler.ReadyToRun/Compiler/DependencyAnalysis/ReadyToRun/HeaderNode.cs +++ b/src/coreclr/tools/aot/ILCompiler.ReadyToRun/Compiler/DependencyAnalysis/ReadyToRun/HeaderNode.cs @@ -161,6 +161,9 @@ public override ObjectData GetData(NodeFactory factory, bool relocsOnly = false) { if (factory.OptimizationFlags.TypeValidation == TypeValidationRule.AutomaticWithLogging) { + // If we are in automatic with logging mode, we reach here when we are unable to enable + // skip validation. When logging is enabled, write out the reasons we found for + // not doing so. foreach (string reason in _shouldAddSkipTypeValidationFlag.Result.reasons) System.Console.WriteLine(reason); } diff --git a/src/coreclr/vm/readytoruninfo.cpp b/src/coreclr/vm/readytoruninfo.cpp index 0744fd530a54ca..dbf615ed1aac8a 100644 --- a/src/coreclr/vm/readytoruninfo.cpp +++ b/src/coreclr/vm/readytoruninfo.cpp @@ -1798,6 +1798,7 @@ const ReadyToRun_MethodIsGenericMap ReadyToRun_MethodIsGenericMap::EmptyInstance mdTypeDef ReadyToRun_EnclosingTypeMap::GetEnclosingType(mdTypeDef input, IMDInternalImport* pImport) const { uint32_t rid = RidFromToken(input); + _ASSERTE(TypeCount <= (uint32_t)ReadyToRunEnclosingTypeMap::MaxTypeCount); if ((rid > TypeCount) || (rid == 0)) { mdTypeDef enclosingType; @@ -1818,6 +1819,7 @@ mdTypeDef ReadyToRun_EnclosingTypeMap::GetEnclosingType(mdTypeDef input, IMDInte HRESULT ReadyToRun_EnclosingTypeMap::GetEnclosingTypeNoThrow(mdTypeDef input, mdTypeDef *pEnclosingType, IMDInternalImport* pImport) const { uint32_t rid = RidFromToken(input); + _ASSERTE(TypeCount <= (uint32_t)ReadyToRunEnclosingTypeMap::MaxTypeCount); if ((rid > TypeCount) || (rid == 0)) { return pImport->GetNestedClassProps(input, pEnclosingType); @@ -1942,7 +1944,7 @@ bool ReadyToRun_MethodIsGenericMap::IsGeneric(mdMethodDef input, bool *foundResu return 0; } - uint8_t chunk = ((uint8_t*)&MethodCount)[((rid - 1) / 8) + 4]; + uint8_t chunk = ((uint8_t*)&MethodCount)[((rid - 1) / 8) + sizeof(uint32_t)]; chunk >>= 7 - ((rid - 1) % 8); return !!(chunk & 1); }