From c8b9cf1bf6654d5d1ad3fdab1a966590d707de4e Mon Sep 17 00:00:00 2001 From: Jan Vorlicek Date: Thu, 2 Apr 2020 15:11:13 +0200 Subject: [PATCH] Fix large version bubble field offset computation (#34401) This change fixes two bugs in field offset computation where the results that crossgen2 was getting was different from what runtime computes. In both cases, the problem was caused by alignment of a derived class being done differently. The first issue was happening for the case when the base and derived classes are in different assemblies. Runtime detect if two assemblies are in the same version bubble using the native manifest metadata table containing a list of assemblies that was supposed to contain all assemblies that the assembly being compiled was found to reference. However, it contained only assemblies that were not in the original assembly reference list, e.g. ones pulled in by inlining. So runtime wasn't getting the same view on what's in the bubble. The second issue happened for the case when both the base and derived class were from the same assembly, but one of the ancestor classes had a field of a value class type that was from another assembly and could be transitively decomposed to fields of types from the same assembly or types like primitive types, object, pointer or enums. The alignment of a derived class members is determined based on that and runtime decision is to align if there is any type from another assembly in the type hierarchy of a class or in fields of any ancestors. For example, the decision would be different for the following scenario: Assembly A: struct AA { int a; } Assembly B: class B1 { AA aa; } class B2 : B1 { int x; } Here crossgen2 would not align the first member but runtime would. So the layout of B2 produced by crossgen2 would be: ``` Offset Field 0 MethodTable 8 a 12 x ``` Layout produced by the runtime would be ``` Offset Field 0 MethodTable 8 a 16 x ``` The fix for the first issue is to put all referenced assemblies into the native manifest metadata. The fix for the second issue is to stop decomposing members of value classes once we hit a value class that's from another module. --- .../CompilationModuleGroup.ReadyToRun.cs | 5 +++++ .../ReadyToRun/ManifestMetadataTableNode.cs | 14 +++++++++----- .../ReadyToRunCompilationModuleGroupBase.cs | 18 ++++++------------ ...oRunSingleAssemblyCompilationModuleGroup.cs | 2 ++ .../SingleMethodCompilationModuleGroup.cs | 2 ++ .../src/tools/crossgen2/crossgen2/Program.cs | 2 ++ 6 files changed, 26 insertions(+), 17 deletions(-) diff --git a/src/coreclr/src/tools/crossgen2/ILCompiler.ReadyToRun/Compiler/CompilationModuleGroup.ReadyToRun.cs b/src/coreclr/src/tools/crossgen2/ILCompiler.ReadyToRun/Compiler/CompilationModuleGroup.ReadyToRun.cs index 83fc679d826e5..acd8aa324e2a4 100644 --- a/src/coreclr/src/tools/crossgen2/ILCompiler.ReadyToRun/Compiler/CompilationModuleGroup.ReadyToRun.cs +++ b/src/coreclr/src/tools/crossgen2/ILCompiler.ReadyToRun/Compiler/CompilationModuleGroup.ReadyToRun.cs @@ -77,6 +77,11 @@ public bool EnforceOwningType(EcmaModule module) /// public abstract bool IsCompositeBuildMode { get; } + /// + /// Returns true when the compiler is running in large version bubble mode + /// + public abstract bool IsInputBubble { get; } + /// /// List of input modules to use for the compilation. /// diff --git a/src/coreclr/src/tools/crossgen2/ILCompiler.ReadyToRun/Compiler/DependencyAnalysis/ReadyToRun/ManifestMetadataTableNode.cs b/src/coreclr/src/tools/crossgen2/ILCompiler.ReadyToRun/Compiler/DependencyAnalysis/ReadyToRun/ManifestMetadataTableNode.cs index d01315059a6e1..b54979b1447da 100644 --- a/src/coreclr/src/tools/crossgen2/ILCompiler.ReadyToRun/Compiler/DependencyAnalysis/ReadyToRun/ManifestMetadataTableNode.cs +++ b/src/coreclr/src/tools/crossgen2/ILCompiler.ReadyToRun/Compiler/DependencyAnalysis/ReadyToRun/ManifestMetadataTableNode.cs @@ -80,12 +80,16 @@ public ManifestMetadataTableNode(NodeFactory nodeFactory) { MetadataReader mdReader = _nodeFactory.CompilationModuleGroup.CompilationModuleSet.Single().MetadataReader; _assemblyRefCount = mdReader.GetTableRowCount(TableIndex.AssemblyRef) + 1; - for (int assemblyRefIndex = 1; assemblyRefIndex < _assemblyRefCount; assemblyRefIndex++) + + if (!_nodeFactory.CompilationModuleGroup.IsInputBubble) { - AssemblyReferenceHandle assemblyRefHandle = MetadataTokens.AssemblyReferenceHandle(assemblyRefIndex); - AssemblyReference assemblyRef = mdReader.GetAssemblyReference(assemblyRefHandle); - string assemblyName = mdReader.GetString(assemblyRef.Name); - _assemblyRefToModuleIdMap[assemblyName] = assemblyRefIndex; + for (int assemblyRefIndex = 1; assemblyRefIndex < _assemblyRefCount; assemblyRefIndex++) + { + AssemblyReferenceHandle assemblyRefHandle = MetadataTokens.AssemblyReferenceHandle(assemblyRefIndex); + AssemblyReference assemblyRef = mdReader.GetAssemblyReference(assemblyRefHandle); + string assemblyName = mdReader.GetString(assemblyRef.Name); + _assemblyRefToModuleIdMap[assemblyName] = assemblyRefIndex; + } } // AssemblyRefCount + 1 corresponds to ROWID 0 in the manifest metadata diff --git a/src/coreclr/src/tools/crossgen2/ILCompiler.ReadyToRun/Compiler/ReadyToRunCompilationModuleGroupBase.cs b/src/coreclr/src/tools/crossgen2/ILCompiler.ReadyToRun/Compiler/ReadyToRunCompilationModuleGroupBase.cs index d313f1fa3b637..031764ac1c8e2 100644 --- a/src/coreclr/src/tools/crossgen2/ILCompiler.ReadyToRun/Compiler/ReadyToRunCompilationModuleGroupBase.cs +++ b/src/coreclr/src/tools/crossgen2/ILCompiler.ReadyToRun/Compiler/ReadyToRunCompilationModuleGroupBase.cs @@ -20,6 +20,7 @@ public abstract class ReadyToRunCompilationModuleGroupBase : CompilationModuleGr private Dictionary _typeRefsInCompilationModuleSet; private readonly bool _compileGenericDependenciesFromVersionBubbleModuleSet; private readonly bool _isCompositeBuildMode; + private readonly bool _isInputBubble; private readonly ConcurrentDictionary _containsTypeLayoutCache = new ConcurrentDictionary(); private readonly ConcurrentDictionary _versionsWithTypeCache = new ConcurrentDictionary(); private readonly ConcurrentDictionary _versionsWithMethodCache = new ConcurrentDictionary(); @@ -27,12 +28,14 @@ public abstract class ReadyToRunCompilationModuleGroupBase : CompilationModuleGr public ReadyToRunCompilationModuleGroupBase( TypeSystemContext context, bool isCompositeBuildMode, + bool isInputBubble, IEnumerable compilationModuleSet, IEnumerable versionBubbleModuleSet, bool compileGenericDependenciesFromVersionBubbleModuleSet) { _compilationModuleSet = new HashSet(compilationModuleSet); _isCompositeBuildMode = isCompositeBuildMode; + _isInputBubble = isInputBubble; Debug.Assert(_isCompositeBuildMode || _compilationModuleSet.Count == 1); @@ -88,18 +91,7 @@ private bool ContainsTypeLayoutUncached(TypeDesc type) var defType = (MetadataType)type; if (!ContainsType(defType)) { - if (!defType.IsValueType) - { - // Eventually, we may respect the non-versionable attribute for reference types too. For now, we are going - // to play is safe and ignore it. - return false; - } - - // Valuetypes with non-versionable attribute are candidates for fixed layout. Reject the rest. - if (!defType.HasCustomAttribute("System.Runtime.Versioning", "NonVersionableAttribute")) - { - return false; - } + return false; } if (!defType.IsValueType && !ContainsTypeLayout(defType.BaseType)) { @@ -220,6 +212,8 @@ public sealed override bool TryGetModuleTokenForExternalType(TypeDesc type, out public sealed override bool IsCompositeBuildMode => _isCompositeBuildMode; + public sealed override bool IsInputBubble => _isInputBubble; + public sealed override IEnumerable CompilationModuleSet => _compilationModuleSet; private bool ComputeTypeVersionsWithCode(TypeDesc type) diff --git a/src/coreclr/src/tools/crossgen2/ILCompiler.ReadyToRun/Compiler/ReadyToRunSingleAssemblyCompilationModuleGroup.cs b/src/coreclr/src/tools/crossgen2/ILCompiler.ReadyToRun/Compiler/ReadyToRunSingleAssemblyCompilationModuleGroup.cs index f4e85d18aa270..01a6c1a18cbfd 100644 --- a/src/coreclr/src/tools/crossgen2/ILCompiler.ReadyToRun/Compiler/ReadyToRunSingleAssemblyCompilationModuleGroup.cs +++ b/src/coreclr/src/tools/crossgen2/ILCompiler.ReadyToRun/Compiler/ReadyToRunSingleAssemblyCompilationModuleGroup.cs @@ -20,11 +20,13 @@ public class ReadyToRunSingleAssemblyCompilationModuleGroup : ReadyToRunCompilat public ReadyToRunSingleAssemblyCompilationModuleGroup( TypeSystemContext context, bool isCompositeBuildMode, + bool isInputBubble, IEnumerable compilationModuleSet, IEnumerable versionBubbleModuleSet, bool compileGenericDependenciesFromVersionBubbleModuleSet) : base(context, isCompositeBuildMode, + isInputBubble, compilationModuleSet, versionBubbleModuleSet, compileGenericDependenciesFromVersionBubbleModuleSet) diff --git a/src/coreclr/src/tools/crossgen2/ILCompiler.ReadyToRun/Compiler/SingleMethodCompilationModuleGroup.cs b/src/coreclr/src/tools/crossgen2/ILCompiler.ReadyToRun/Compiler/SingleMethodCompilationModuleGroup.cs index 11c0b2ea288d7..f32c2295b5503 100644 --- a/src/coreclr/src/tools/crossgen2/ILCompiler.ReadyToRun/Compiler/SingleMethodCompilationModuleGroup.cs +++ b/src/coreclr/src/tools/crossgen2/ILCompiler.ReadyToRun/Compiler/SingleMethodCompilationModuleGroup.cs @@ -20,12 +20,14 @@ public class SingleMethodCompilationModuleGroup : ReadyToRunCompilationModuleGro public SingleMethodCompilationModuleGroup( TypeSystemContext context, bool isCompositeBuildMode, + bool isInputBubble, IEnumerable compilationModuleSet, IEnumerable versionBubbleModuleSet, bool compileGenericDependenciesFromVersionBubbleModuleSet, MethodDesc method) : base(context, isCompositeBuildMode, + isInputBubble, compilationModuleSet, versionBubbleModuleSet, compileGenericDependenciesFromVersionBubbleModuleSet) diff --git a/src/coreclr/src/tools/crossgen2/crossgen2/Program.cs b/src/coreclr/src/tools/crossgen2/crossgen2/Program.cs index 6f98ee6677b0b..ecb8954d0ff9b 100644 --- a/src/coreclr/src/tools/crossgen2/crossgen2/Program.cs +++ b/src/coreclr/src/tools/crossgen2/crossgen2/Program.cs @@ -295,6 +295,7 @@ private int Run() compilationGroup = new SingleMethodCompilationModuleGroup( typeSystemContext, _commandLineOptions.Composite, + _commandLineOptions.InputBubble, inputModules, versionBubbleModules, _commandLineOptions.CompileBubbleGenerics, @@ -307,6 +308,7 @@ private int Run() compilationGroup = new ReadyToRunSingleAssemblyCompilationModuleGroup( typeSystemContext, _commandLineOptions.Composite, + _commandLineOptions.InputBubble, inputModules, versionBubbleModules, _commandLineOptions.CompileBubbleGenerics);