Skip to content

Commit

Permalink
Fix type generator test failures (#44041)
Browse files Browse the repository at this point in the history 
- Fix issue where method token referred to derived type but method was actually defined on base type
  - Previously the methods did not carry enough state to determine when to emit the owner type of the method, and which exact type was the owning type. The new logic computes it from the token in the presence of a proper instantiation context, which allows for correct operation.

- Fix issue where constrained dispatch on a method of a valuetype would not put in the correct owner type
  - Issue fixed by determining if the token resolves to a method on the same type as the eventual target method, or if it needs to have a specific owning type specified

In general these issues where both caused by confusion around exactly the correct owning type, and it turned out that computation could not be computed within the signature emitter code, but instead needed to be computed in the JIT at point of use. Fortunately, we had a structure `MethodWithToken` that is used in these (and only these situations). Finally, I also made a pass through the emitter and related logic to remove various band-aids that had built up over the last few years to make all the tests and applications pass. I believe that the new logic should be correct in the general case.

Bonus tweak...  Use parallelism when compiling the framework composite images with crossgen2, and fix bug in composite image generation where mangled symbol names might conflict.

Fixes #43466 and fixes #43467
  • Loading branch information
@davidwrighton
davidwrighton authored Nov 1, 2020
1 parent f1de95e commit 8560a2e
Show file tree
Hide file tree
Showing 17 changed files with 389 additions and 332 deletions.
10 changes: 5 additions & 5 deletions ...aot/ILCompiler.ReadyToRun/Compiler/DependencyAnalysis/ReadyToRun/DelegateCtorSignature.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -14,12 +14,12 @@ public class DelegateCtorSignature : Signature

private readonly IMethodNode _targetMethod;

private readonly ModuleToken _methodToken;
private readonly MethodWithToken _methodToken;

public DelegateCtorSignature(
TypeDesc delegateType,
IMethodNode targetMethod,
ModuleToken methodToken)
MethodWithToken methodToken)
{
_delegateType = delegateType;
_targetMethod = targetMethod;
Expand All @@ -40,10 +40,10 @@ public override ObjectData GetData(NodeFactory factory, bool relocsOnly = false)

if (!relocsOnly)
{
SignatureContext innerContext = builder.EmitFixup(factory, ReadyToRunFixupKind.DelegateCtor, _methodToken.Module, factory.SignatureContext);
SignatureContext innerContext = builder.EmitFixup(factory, ReadyToRunFixupKind.DelegateCtor, _methodToken.Token.Module, factory.SignatureContext);

builder.EmitMethodSignature(
new MethodWithToken(_targetMethod.Method, _methodToken, constrainedType: null, unboxing: false),
_methodToken,
enforceDefEncoding: false,
enforceOwningType: false,
innerContext,
Expand Down Expand Up @@ -88,7 +88,7 @@ public override int CompareToImpl(ISortableNode other, CompilerComparer comparer
if (result != 0)
return result;

return _methodToken.CompareTo(otherNode._methodToken);
return _methodToken.CompareTo(otherNode._methodToken, comparer);
}
}
}
3 changes: 2 additions & 1 deletion ...s/aot/ILCompiler.ReadyToRun/Compiler/DependencyAnalysis/ReadyToRun/FieldFixupSignature.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -68,7 +68,8 @@ public override ObjectData GetData(NodeFactory factory, bool relocsOnly = false)
public override void AppendMangledName(NameMangler nameMangler, Utf8StringBuilder sb)
{
sb.Append(nameMangler.CompilationUnitPrefix);
sb.Append($@"TypeFixupSignature({_fixupKind.ToString()}): {_fieldDesc.ToString()}");
sb.Append($@"FieldFixupSignature({_fixupKind.ToString()}): ");
sb.Append(nameMangler.GetMangledFieldName(_fieldDesc));
}

public override int CompareToImpl(ISortableNode other, CompilerComparer comparer)
Expand Down
2 changes: 1 addition & 1 deletion ...Compiler.ReadyToRun/Compiler/DependencyAnalysis/ReadyToRun/InstanceEntryPointTableNode.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -61,7 +61,7 @@ public override ObjectData GetData(NodeFactory factory, bool relocsOnly = false)

ArraySignatureBuilder signatureBuilder = new ArraySignatureBuilder();
signatureBuilder.EmitMethodSignature(
new MethodWithToken(method.Method, moduleToken, constrainedType: null, unboxing: false),
new MethodWithToken(method.Method, moduleToken, constrainedType: null, unboxing: false, context: null),
enforceDefEncoding: true,
enforceOwningType: _factory.CompilationModuleGroup.EnforceOwningType(moduleToken.Module),
factory.SignatureContext,
Expand Down
16 changes: 12 additions & 4 deletions .../aot/ILCompiler.ReadyToRun/Compiler/DependencyAnalysis/ReadyToRun/MethodFixupSignature.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,9 @@
// The .NET Foundation licenses this file to you under the MIT license.

using System;
using System.Diagnostics;
using System.Reflection.Metadata;
using System.Reflection.Metadata.Ecma335;

using Internal.JitInterface;
using Internal.Text;
Expand Down Expand Up @@ -32,6 +35,8 @@ public MethodFixupSignature(
// Ensure types in signature are loadable and resolvable, otherwise we'll fail later while emitting the signature
CompilerTypeSystemContext compilerContext = (CompilerTypeSystemContext)method.Method.Context;
compilerContext.EnsureLoadableMethod(method.Method);
compilerContext.EnsureLoadableType(_method.OwningType);

if (method.ConstrainedType != null)
compilerContext.EnsureLoadableType(method.ConstrainedType);
}
Expand Down Expand Up @@ -68,8 +73,11 @@ public override ObjectData GetData(NodeFactory factory, bool relocsOnly = false)
}
else if (_method.Token.TokenType == CorTokenType.mdtMemberRef)
{
fixupKind = ReadyToRunFixupKind.MethodEntry_RefToken;
optimized = true;
if (!_method.OwningTypeNotDerivedFromToken)
{
fixupKind = ReadyToRunFixupKind.MethodEntry_RefToken;
optimized = true;
}
}
}
}
Expand All @@ -80,13 +88,13 @@ public override ObjectData GetData(NodeFactory factory, bool relocsOnly = false)
{
if (method.Token.TokenType == CorTokenType.mdtMethodSpec)
{
method = new MethodWithToken(method.Method, factory.SignatureContext.GetModuleTokenForMethod(method.Method, throwIfNotFound: false), method.ConstrainedType, unboxing: _method.Unboxing);
method = new MethodWithToken(method.Method, factory.SignatureContext.GetModuleTokenForMethod(method.Method, throwIfNotFound: false), method.ConstrainedType, unboxing: _method.Unboxing, null);
}
else if (!optimized && (method.Token.TokenType == CorTokenType.mdtMemberRef))
{
if (method.Method.OwningType.GetTypeDefinition() is EcmaType)
{
method = new MethodWithToken(method.Method, factory.SignatureContext.GetModuleTokenForMethod(method.Method, throwIfNotFound: false), method.ConstrainedType, unboxing: _method.Unboxing);
method = new MethodWithToken(method.Method, factory.SignatureContext.GetModuleTokenForMethod(method.Method, throwIfNotFound: false), method.ConstrainedType, unboxing: _method.Unboxing, null);
}
}
}
Expand Down
77 changes: 6 additions & 71 deletions ...ools/aot/ILCompiler.ReadyToRun/Compiler/DependencyAnalysis/ReadyToRun/SignatureBuilder.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -412,62 +412,12 @@ public void EmitMethodSignature(
{
flags |= (uint)ReadyToRunMethodSigFlags.READYTORUN_METHOD_SIG_Constrained;
}
if (enforceOwningType)
if (enforceOwningType || method.OwningTypeNotDerivedFromToken)
{
flags |= (uint)ReadyToRunMethodSigFlags.READYTORUN_METHOD_SIG_OwnerType;
}

if ((method.Method.HasInstantiation || method.Method.OwningType.HasInstantiation) && !method.Method.IsGenericMethodDefinition)
{
EmitMethodSpecificationSignature(method, flags, enforceDefEncoding, context);
}
else
{
switch (method.Token.TokenType)
{
case CorTokenType.mdtMethodDef:
{
EmitUInt(flags);
if ((flags & (uint)ReadyToRunMethodSigFlags.READYTORUN_METHOD_SIG_OwnerType) != 0)
{
EmitTypeSignature(method.Method.OwningType, context);
}
EmitMethodDefToken(method.Token);
}
break;

case CorTokenType.mdtMemberRef:
{
flags |= (uint)ReadyToRunMethodSigFlags.READYTORUN_METHOD_SIG_MemberRefToken;

// Owner type is needed for type specs to instantiating stubs or generics with signature variables still present
if (!method.Method.OwningType.IsDefType &&
((flags & (uint)ReadyToRunMethodSigFlags.READYTORUN_METHOD_SIG_InstantiatingStub) != 0 || method.Method.OwningType.ContainsSignatureVariables()))
{
flags |= (uint)ReadyToRunMethodSigFlags.READYTORUN_METHOD_SIG_OwnerType;
}
else if (method.Method.IsArrayMethod())
{
var memberRefMethod = method.Token.Module.GetMethod(MetadataTokens.EntityHandle((int)method.Token.Token));
if (memberRefMethod.OwningType != method.Method.OwningType)
{
flags |= (uint)ReadyToRunMethodSigFlags.READYTORUN_METHOD_SIG_OwnerType;
}
}

EmitUInt(flags);
if ((flags & (uint)ReadyToRunMethodSigFlags.READYTORUN_METHOD_SIG_OwnerType) != 0)
{
EmitTypeSignature(method.Method.OwningType, context);
}
EmitMethodRefToken(method.Token);
}
break;

default:
throw new NotImplementedException();
}
}
EmitMethodSpecificationSignature(method, flags, enforceDefEncoding, context);

if (method.ConstrainedType != null)
{
Expand All @@ -491,7 +441,7 @@ private void EmitMethodSpecificationSignature(MethodWithToken method,
uint flags, bool enforceDefEncoding, SignatureContext context)
{
ModuleToken methodToken = method.Token;
if (method.Method.HasInstantiation)
if (method.Method.HasInstantiation && !method.Method.IsGenericMethodDefinition)
{
flags |= (uint)ReadyToRunMethodSigFlags.READYTORUN_METHOD_SIG_MethodInstantiation;
if (!method.Token.IsNull)
Expand All @@ -504,23 +454,7 @@ private void EmitMethodSpecificationSignature(MethodWithToken method,
}
}

if (methodToken.IsNull && !enforceDefEncoding)
{
methodToken = context.GetModuleTokenForMethod(method.Method, throwIfNotFound: false);
}
if (methodToken.IsNull)
{
flags |= (uint)ReadyToRunMethodSigFlags.READYTORUN_METHOD_SIG_OwnerType;
methodToken = context.GetModuleTokenForMethod(method.Method);
}

if (method.Method.OwningType.HasInstantiation)
{
// resolveToken currently resolves the token in the context of a given scope;
// in such case, we receive a method on instantiated type along with the
// generic definition token.
flags |= (uint)ReadyToRunMethodSigFlags.READYTORUN_METHOD_SIG_OwnerType;
}
Debug.Assert(!methodToken.IsNull);

switch (methodToken.TokenType)
{
Expand All @@ -538,7 +472,8 @@ private void EmitMethodSpecificationSignature(MethodWithToken method,
EmitUInt(flags);
if ((flags & (uint)ReadyToRunMethodSigFlags.READYTORUN_METHOD_SIG_OwnerType) != 0)
{
EmitTypeSignature(method.Method.OwningType, context);
// The type here should be the type referred to by the memberref (if this is one, not the type where the method was eventually found!
EmitTypeSignature(method.OwningType, context);
}
EmitTokenRid(methodToken.Token);
if ((flags & (uint)ReadyToRunMethodSigFlags.READYTORUN_METHOD_SIG_MethodInstantiation) != 0)
Expand Down
2 changes: 1 addition & 1 deletion ...ols/aot/ILCompiler.ReadyToRun/Compiler/DependencyAnalysis/ReadyToRunCodegenNodeFactory.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -396,7 +396,7 @@ public IEnumerable<MethodWithGCInfo> EnumerateCompiledMethods(EcmaModule moduleT
EcmaModule module = ((EcmaMethod)method.GetTypicalMethodDefinition()).Module;
ModuleToken moduleToken = Resolver.GetModuleTokenForMethod(method, throwIfNotFound: true);

IMethodNode methodNodeDebug = MethodEntrypoint(new MethodWithToken(method, moduleToken, constrainedType: null, unboxing: false), false, false);
IMethodNode methodNodeDebug = MethodEntrypoint(new MethodWithToken(method, moduleToken, constrainedType: null, unboxing: false, context: null), false, false);
MethodWithGCInfo methodCodeNodeDebug = methodNodeDebug as MethodWithGCInfo;
if (methodCodeNodeDebug == null && methodNodeDebug is DelayLoadMethodImport DelayLoadMethodImport)
{
Expand Down
2 changes: 1 addition & 1 deletion ...ools/aot/ILCompiler.ReadyToRun/Compiler/DependencyAnalysis/ReadyToRunSymbolNodeFactory.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -124,7 +124,7 @@ private void CreateNodeCaches()
_codegenNodeFactory,
_codegenNodeFactory.HelperImports,
ReadyToRunHelper.DelayLoad_Helper_ObjObj,
new DelegateCtorSignature(ctorKey.Type, targetMethodNode, ctorKey.Method.Token));
new DelegateCtorSignature(ctorKey.Type, targetMethodNode, ctorKey.Method));
});

_checkTypeLayoutCache = new NodeCache<TypeDesc, ISymbolNode>(key =>
Expand Down
Loading

0 comments on commit 8560a2e

Please sign in to comment.