Enable compilation of async thunks in composite mode

[jtschuster]

The issue with async thunks in composite mode was that we emit MutableModule tokens for the thunks that point to some types in the composite image. But using MutableModule tokens pointing to anything but CoreLib is explicitly blocked - without an IL module to determine the ALC, assembly load behavior might not be preserved. To work around this, I went back to the approach where we add missing tokens to the mutable module and compile the thunk as an ILStub. Since the thunks should only need to reference existing type refs/defs accessible from the composite image + some async helpers in CoreLib, this shouldn't end up adding any tokens to the manifest module that point anywhere but CoreLib.

The process of going through an IL method body and creating tokens in the MutableModule is handled by the ExternalReferenceTokenManager (open to renames).

This implementation initially caused some issues when emitting signatures for Generic methods on generic types. In resolving a module token for a method on an IL stub (CorInfoImpl.HandleToModuleToken()), we strip the instantiation info and get a token for the method definition. The comment in that method assumed this is fine because when emitting the signature, we add flags and generic instantiation info for the method. But when MethodWithToken..ctor() calculates the owning type, it assumes the ModuleToken is for the generic instantiated method on a generic instantiated type (but we just stripped off the instantiation). To account for that, the code now will tell the MethodWithToken to force the OwningType to be MethodDesc.OwningType and force it to emit the OwningType flag it in the Signature.

fixes #125337

[Copilot]

Pull request overview

Copilot reviewed 18 out of 18 changed files in this pull request and generated 2 comments.

[jtschuster]

GitHub doesn't show this well, but this just moved to a local method since it's not used anywhere else.

[Copilot]

Pull request overview

Copilot reviewed 18 out of 18 changed files in this pull request and generated 2 comments.

Comments suppressed due to low confidence (1)

src/coreclr/tools/aot/ILCompiler.ReadyToRun/IL/ReadyToRunILProvider.cs:166

• CreateCrossModuleInlineableTokensForILBody now always wraps EcmaMethodIL.Create((EcmaMethod)method.GetPrimaryMethodDesc().GetTypicalMethodDefinition()). When method is an AsyncMethodVariant (the caller comment in CorInfoImpl.reportInliningDecision explicitly notes this can happen), this produces a wrapped IL whose OwningMethod is the underlying EcmaMethod, not the AsyncMethodVariant key you store it under. This can break token resolution and compilation because MethodIL.OwningMethod no longer matches the method being compiled. Consider restoring support for wrapping AsyncEcmaMethodIL/explicit owning-method (as the previous overload did) so the wrapped IL preserves OwningMethod == method for async variants.

        public void CreateCrossModuleInlineableTokensForILBody(MethodDesc method)
        {
            // This method accepts only method definitions, but accepts non-primary method definitions.
            // That is, it must not be generic, but it may represent an AsyncVariant
            Debug.Assert(method.IsTypicalMethodDefinition);
            Debug.Assert(_manifestMutableModule != null);
            var wrappedMethodIL = new ManifestModuleWrappedMethodIL();

            Debug.Assert(!_compilationModuleGroup.VersionsWithMethodBody(method) &&
                    _compilationModuleGroup.CrossModuleInlineable(method));

            if (!wrappedMethodIL.Initialize(_manifestMutableModule, EcmaMethodIL.Create((EcmaMethod)method.GetPrimaryMethodDesc().GetTypicalMethodDefinition())))
            {
                // If we could not initialize the wrapped method IL, we should store a null.
                // That will result in the IL code for the method being unavailable for use in
                // the compilation, which is version safe.
                wrappedMethodIL = null;
            }

            _manifestModuleWrappedMethods.Add(method, wrappedMethodIL);
            IncrementVersion();
        }

[Copilot]

Pull request overview

Copilot reviewed 17 out of 17 changed files in this pull request and generated 1 comment.

Comments suppressed due to low confidence (1)

src/coreclr/tools/aot/ILCompiler.ReadyToRun/JitInterface/CorInfoImpl.ReadyToRun.cs:110

• FieldWithToken.AppendMangledName does not incorporate OwningTypeNotDerivedFromToken (or the owning type override). Since OwningTypeNotDerivedFromToken changes the emitted field signature (READYTORUN_FIELD_SIG_OwnerType), two distinct FieldWithToken instances could end up producing identical symbol names, risking node/symbol collisions when both variants are present. Consider appending a stable marker (and owning type, if needed) to the mangled name when OwningTypeNotDerivedFromToken is true, similar to MethodWithToken.AppendMangledName.

        public void AppendMangledName(NameMangler nameMangler, Utf8StringBuilder sb)
        {
            sb.Append(nameMangler.GetMangledFieldName(Field));
            sb.Append("; "u8);
            sb.Append(Token.ToString());
        }

[Copilot]

MethodWithToken/FieldWithToken now use OwningTypeNotDerivedFromToken to control READYTORUN_*_SIG_OwnerType emission. However, the GenericLookupKey cache uses RuntimeDeterminedTypeHelper.Equals/GetHashCode for MethodWithToken/FieldWithToken, which currently ignores OwningTypeNotDerivedFromToken. This can cause generic lookup helper imports to be reused across signatures that differ only by the owner-type-override flag, leading to incorrect fixup signatures. Update the RuntimeDeterminedTypeHelper comparisons/hashes (or GenericLookupKey equality) to include the owning-type-override flag (and any other fields that affect signature encoding).

Suggested change

	public bool Equals(MethodWithToken other)
	{
	return EqualityComparer<MethodDesc>.Default.Equals(Method, other.Method)
	&& Token.Equals(other.Token)
	&& EqualityComparer<TypeDesc>.Default.Equals(ConstrainedType, other.ConstrainedType)
	&& Unboxing == other.Unboxing
	&& OwningTypeNotDerivedFromToken == other.OwningTypeNotDerivedFromToken
	&& EqualityComparer<TypeDesc>.Default.Equals(OwningType, other.OwningType);
	}
	public override bool Equals(object obj)
	{
	return obj is MethodWithToken other && Equals(other);
	}
	public override int GetHashCode()
	{
	HashCode hash = new HashCode();
	hash.Add(Method);
	hash.Add(Token);
	hash.Add(ConstrainedType);
	hash.Add(Unboxing);
	hash.Add(OwningTypeNotDerivedFromToken);
	hash.Add(OwningType);
	return hash.ToHashCode();
	}
	public static bool operator ==(MethodWithToken left, MethodWithToken right)
	{
	return left.Equals(right);
	}
	public static bool operator !=(MethodWithToken left, MethodWithToken right)
	{
	return !left.Equals(right);
	}

[davidwrighton]

I like it! I'm assuming you've run the various crossgen2 outerloops, as I can't actually validate these sorts of changes by hand in a reliable manner. I have one question around changing the behavior of the FieldWithToken and MethodToken comparisons, but otherwise, it looks reasonable.

[davidwrighton]

These details around the OwningTypeNotDerivedFromToken are interesting. I expect that adding these comparisons to the logic will increase the size of R2R images by not sharing some cases which could be sharing. Could you do an experiment where instead of adding a compare based on OwningTypeNotDerivedFromToken, you keep the forceOwningTypeFromMethodDesc flag on the MethodToken, and add that to the comparison instead? You'd also need to tweak these asserts, but it might be a size win.

[davidwrighton]

I have the same concerns around adding the OwningTypeNotDerivedFromToken to the comparisons here as I do for MethodToken.

[davidwrighton]

@jtschuster Also, I was perusing code, and noticed that AddResumptionStubFixup directly adds the fixup to the methodCodeNode. Is there a reason it doesn't use AddPrecodeFixup which caches the adds so that they only happen once, and only if the method actually succeeds its compile?

[jtschuster]

/azp run runtime-coreclr crossgen2

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).