[mono][System.Reflection.Emit] VTable initialization failure for class implementing an interface

[lambdageek]

When using System.Reflection.Emit with Mono, using MethodBuilder.SetReturnType and MethodBuilder.SetParameters on methods that are supposed to implement interface methods does not work - the method is not considered to implement the interface method. Using the long form of TypeBuilder.DefineMethod works.

using System;
using System.Reflection;
using System.Reflection.Emit;

public interface IStr
{
    public string MStr(string x);
}

public class P {
    public static void Main()
    {
        var aname = "TestAssembly";
        var assemblyName = new AssemblyName(aname);
        var assemblyBuilder = AssemblyBuilder.DefineDynamicAssembly(assemblyName, AssemblyBuilderAccess.Run);
        var moduleBuilder = assemblyBuilder.DefineDynamicModule(aname);

        var typeBuilder = moduleBuilder.DefineType("TestType", TypeAttributes.Public, typeof(object), new Type[] { typeof(IStr) });

#if false
        //works
        var methodBuilder = typeBuilder.DefineMethod("MStr", MethodAttributes.Public | MethodAttributes.Virtual, typeof(string), new Type[] { typeof(string) });
#else
        // throws "Invalid VTable" from CreateInstance, below
        var methodBuilder = typeBuilder.DefineMethod("MStr", MethodAttributes.Public | MethodAttributes.Virtual);
        methodBuilder.SetReturnType(typeof(string));
        methodBuilder.SetParameters(new Type[] { typeof(string) });
#endif

        var ilg = methodBuilder.GetILGenerator();
        ilg.Emit(OpCodes.Ldarg_1);
        ilg.Emit(OpCodes.Ret);

        var type = typeBuilder.CreateType();
        var obj = Activator.CreateInstance(type);
        var m = type.GetMethod("MStr");
        var r = m.Invoke(obj, new object[] { "hello" });
        Console.WriteLine($"r = {r}");
    }
}

Update simpler repro and better diagnosis; original issue below

---

Needs some investigation still. Some of the System.Reflection.Emit code generates a class with a bad vtable on net8.0.

The TypedClientBuilder is from aspnetcore:

https://github.com/dotnet/aspnetcore/blob/556f39af49b1a32f14089d3809557825932e4ca5/src/SignalR/server/Core/src/Internal/TypedClientBuilder.cs#L49

Repro:

Clone this repo https://github.com/lambdageek/repro-iclientproxy-vtable-fail

Expected

$ dotnet run
SendCoreAsync - M1

Actual

$ dotnet run
Unhandled Exception:
System.TypeLoadException: VTable setup of type Microsoft.AspNetCore.SignalR.TypedClientBuilder.I1Impl failed
   at System.RuntimeType.GetMethodsByName(String name, BindingFlags bindingAttr, MemberListType listType, RuntimeType reflectedType)
   at System.RuntimeType.GetMethodCandidates(String name, BindingFlags bindingAttr, CallingConventions callConv, Type[] types, Int32 genericParamCount, Boolean allowPrefixLookup)
   at System.RuntimeType.GetMethodImpl(String name, Int32 genericParamCount, BindingFlags bindingAttr, Binder binder, CallingConventions callConv, Type[] types, ParameterModifier[] modifiers)
   at System.RuntimeType.GetMethodImpl(String name, BindingFlags bindingAttr, Binder binder, CallingConventions callConvention, Type[] types, ParameterModifier[] modifiers)
   at System.Type.GetMethod(String name, BindingFlags bindingAttr)
   at Retrofit.TypedClientBuilder`1[[I1, promo, Version=1.0.0.0, Culture=neutral, PublicKeyToken=null]].GenerateClientBuilder() in /private/tmp/promo/TypedClientBuilder.cs:line 42
   at System.Lazy`1[[System.Func`2[[Retrofit.IClientProxy, promo, Version=1.0.0.0, Culture=neutral, PublicKeyToken=null],[I1, promo, Version=1.0.0.0, Culture=neutral, PublicKeyToken=null]], System.Private.CoreLib, Version=8.0.0.0, Culture=neutral, PublicKeyToken=7cec85d7bea7798e]].ViaFactory(LazyThreadSafetyMode mode)
   at System.Lazy`1[[System.Func`2[[Retrofit.IClientProxy, promo, Version=1.0.0.0, Culture=neutral, PublicKeyToken=null],[I1, promo, Version=1.0.0.0, Culture=neutral, PublicKeyToken=null]], System.Private.CoreLib, Version=8.0.0.0, Culture=neutral, PublicKeyToken=7cec85d7bea7798e]].ExecutionAndPublication(LazyHelper executionAndPublication, Boolean useDefaultConstructor)
   at System.Lazy`1[[System.Func`2[[Retrofit.IClientProxy, promo, Version=1.0.0.0, Culture=neutral, PublicKeyToken=null],[I1, promo, Version=1.0.0.0, Culture=neutral, PublicKeyToken=null]], System.Private.CoreLib, Version=8.0.0.0, Culture=neutral, PublicKeyToken=7cec85d7bea7798e]].CreateValue()
   at System.Lazy`1[[System.Func`2[[Retrofit.IClientProxy, promo, Version=1.0.0.0, Culture=neutral, PublicKeyToken=null],[I1, promo, Version=1.0.0.0, Culture=neutral, PublicKeyToken=null]], System.Private.CoreLib, Version=8.0.0.0, Culture=neutral, PublicKeyToken=7cec85d7bea7798e]].get_Value()
   at Retrofit.TypedClientBuilder`1[[I1, promo, Version=1.0.0.0, Culture=neutral, PublicKeyToken=null]].Build(IClientProxy proxy) in /private/tmp/promo/TypedClientBuilder.cs:line 24
   at P.Main() in /private/tmp/promo/Program.cs:line 19
   at P.<Main>()

[lambdageek]

This is making aspnetcore tests fail with net8.0 when Mono is used as the runtime.

See #94437 (comment)

[lambdageek]

/cc @SamMonoRT

[lambdageek]

Oh I bet I know what it is.

https://github.com/lambdageek/repro-iclientproxy-vtable-fail/blob/5f9da0a04fb30087cb08902a27e739e878e7ce00/TypedClientBuilder.cs#L50

I bet something goes wrong when this typeof(T) parameter is used as an interface for moduleBuilder.DefineType.

Although I'm not sure how we actually end up seeing a generic param there. Running with MONO_ENV_OPTIONS=--interp or with MONO_ENV_OPTIONS=--optimize=-gshared still fails.

[tmds]

cc @uweigand @janani66 @Sapana-Khemkar @Swapnali911 @omajid

[lambdageek]

It's not really a vtable problem. It's some problem with the IL generation in TypedClientBuilder.BuildMethod. Somehow the IL in there ends up being bad and mono ends up not creating a method

[lambdageek]

Ok, really it has to do with typeBuilder.DefineMethod and methodBuidler.SetReturnType.

Apparently if you use methodBuilder = typeBuilder.DefineMethod(methodName, methodAttributes, returnType, paramTypes) instead of

   methodBuilder = typeBuidler.DefineMethod(methodName, methodAttributes);
   methodBuilder.SetReturnType(returnType);
   methodBuilder.SetParameters(paramTypes);

the code works.

The problem is that if you use the longer version with SetParameters, mono doesn't consider that the methodBuilder implements an interface method and when it goes to construct the class it fails because not all the interface methods are implemented.

Simple repro:

public interface IStr
{
    public string MStr(string x);
}

public class P {
    public static void Main()
    {
        var aname = "TestAssembly";
        var assemblyName = new AssemblyName(aname);
        var assemblyBuilder = AssemblyBuilder.DefineDynamicAssembly(assemblyName, AssemblyBuilderAccess.Run);
        var moduleBuilder = assemblyBuilder.DefineDynamicModule(aname);

        var typeBuilder = moduleBuilder.DefineType("TestType", TypeAttributes.Public, typeof(object), new Type[] { typeof(IStr) });

#if false
        //works
        var methodBuilder = typeBuilder.DefineMethod("MStr", MethodAttributes.Public | MethodAttributes.Virtual, typeof(string), new Type[] { typeof(string) });
#else
        // throws "Invalid VTable" from CreateInstance, below
        var methodBuilder = typeBuilder.DefineMethod("MStr", MethodAttributes.Public | MethodAttributes.Virtual);
        methodBuilder.SetReturnType(typeof(string));
        methodBuilder.SetParameters(new Type[] { typeof(string) });
#endif

        var ilg = methodBuilder.GetILGenerator();
        ilg.Emit(OpCodes.Ldarg_1);
        ilg.Emit(OpCodes.Ret);

        var type = typeBuilder.CreateType();
        var obj = Activator.CreateInstance(type);
        var m = type.GetMethod("MStr");
        var r = m.Invoke(obj, new object[] { "hello" });
        Console.WriteLine($"r = {r}");
    }
}

[lambdageek]

@tmds @uweigand @janani66 consider patching aspnetcore here:

https://github.com/dotnet/aspnetcore/blob/a2536e6a4e384182825e89ee064115b5d841e97e/src/SignalR/server/Core/src/Internal/TypedClientBuilder.cs#L119-L137

to replace

  var methodBuilder = type.DefineMethod(interfaceMethodInfo.Name, methodAttributes);
  ...
  methodBuilder.SetReturnType(interfaceMethodInfo.ReturnType);
  methodBuilder.SetParameters(paramTypes);

by

  var methodBuilder = type.DefineMethod(interfaceMethodInfo.Name, methodAttributes, interfaceMethodInfo.ReturnType, paramTypes);
   ...

[alhad-deshpande]

@lambdageek
Ran the aspnetcore tests with above fix on ppc64le architecture and both Signalr tests passed.

Test Run Results:
Passed! - Failed: 0, Passed: 819, Skipped: 4, Total: 823, Duration: 1 m 17 s - Microsoft.AspNetCore.SignalR.Client.FunctionalTests.dll (net8.0) Passed! - Failed: 0, Passed: 426, Skipped: 1, Total: 427, Duration: 17 s - Microsoft.AspNetCore.SignalR.Tests.dll (net8.0)

[tmds]

@lambdageek will this issue be fixed on the short term?

If not, it would be good to include your workaround directly in aspnetcore so SignalR gets fixed for everyone doing Mono based .NET builds.

[lambdageek]

@tmds I think the fix will probably miss the first couple of servicing releases for .NET 8 due to winter holidays and scheduling.

That said, here's what I think needs to be fixed @ivanpovazan @buyaa-n

Compare SetSignatureCore between CoreCLR:

runtime/src/coreclr/System.Private.CoreLib/src/System/Reflection/Emit/RuntimeMethodBuilder.cs

Lines 624 to 633 in 8ebe003

	if (returnType != null)
	{
	m_returnType = returnType;
	}
	if (parameterTypes != null)
	{
	m_parameterTypes = new Type[parameterTypes.Length];
	Array.Copy(parameterTypes, m_parameterTypes, parameterTypes.Length);
	}

and Mono:

runtime/src/mono/System.Private.CoreLib/src/System/Reflection/Emit/RuntimeMethodBuilder.Mono.cs

Lines 595 to 606 in 8ebe003

	if (parameterTypes != null)
	{
	foreach (Type t in parameterTypes)
	{
	ArgumentNullException.ThrowIfNull(t, nameof(parameterTypes));
	}
	this.parameters = new Type[parameterTypes.Length];
	Array.Copy(parameterTypes, this.parameters, parameterTypes.Length);
	}
	rtype = returnType;

I think Mono should do:

if (returnType != null)
  rtype = returnType;

SetSignatureCore is called from SetParameters and SetReturnType:

runtime/src/libraries/System.Private.CoreLib/src/System/Reflection/Emit/MethodBuilder.cs

Lines 77 to 89 in 8ebe003

	public void SetParameters(params Type[] parameterTypes)
	=> SetSignature(null, null, null, parameterTypes, null, null);
	public void SetReturnType(Type? returnType)
	=> SetSignature(returnType, null, null, null, null, null);
	public void SetSignature(Type? returnType, Type[]? returnTypeRequiredCustomModifiers, Type[]? returnTypeOptionalCustomModifiers,
	Type[]? parameterTypes, Type[][]? parameterTypeRequiredCustomModifiers, Type[][]? parameterTypeOptionalCustomModifiers)
	=> SetSignatureCore(returnType, returnTypeRequiredCustomModifiers, returnTypeOptionalCustomModifiers,
	parameterTypes, parameterTypeRequiredCustomModifiers, parameterTypeOptionalCustomModifiers);
	protected abstract void SetSignatureCore(Type? returnType, Type[]? returnTypeRequiredCustomModifiers, Type[]? returnTypeOptionalCustomModifiers,
	Type[]? parameterTypes, Type[][]? parameterTypeRequiredCustomModifiers, Type[][]? parameterTypeOptionalCustomModifiers);

[ivanpovazan]

Reopening until the backport is not merged:

• [release/8.0-staging] [mono][System.Reflection.Emit] Fix explicitly setting the return type on a dynamic method #95271

[akoeplinger]

@ivanpovazan the backport PR was approved by tactics so you can merge it

[ivanpovazan]

Thanks @akoeplinger!

Closing as the backport has also been merged.