Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

JIT throws MissingMethodException compiling dynamic method that invokes init property setter declared in generic class #45102

Closed
AArnott opened this issue Nov 23, 2020 · 14 comments
Labels
api-suggestion Early API idea and discussion, it is NOT ready for implementation area-System.Reflection.Emit
Milestone

Comments

@AArnott
Copy link
Contributor

AArnott commented Nov 23, 2020

Description

Very strangely, when a property setter is defined as init in C# 9, and that property is declared in a generic type, then dynamic generated code throws when calling it.

Repro

#nullable enable

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

var milk = new Product<int> { Name = "milk" };

MethodInfo setter = milk.GetType().GetProperty(nameof(Product<int>.Name))!.SetMethod!;
setter.Invoke(milk, new object?[] { "bread" });
Console.WriteLine(milk);

var assemblyBuilder = AssemblyBuilder.DefineDynamicAssembly(new AssemblyName("test"), AssemblyBuilderAccess.Run);
var moduleBuilder = assemblyBuilder.DefineDynamicModule("test.dll");
var typeBuilder = moduleBuilder.DefineType("SomeClass", TypeAttributes.Public | TypeAttributes.Sealed | TypeAttributes.Abstract, typeof(object));

var dynamicMethod = typeBuilder.DefineMethod("SetName", MethodAttributes.Public | MethodAttributes.Static, null, new Type[] { typeof(Product<int>), typeof(string) });
var il = dynamicMethod.GetILGenerator();
il.Emit(OpCodes.Ldarg_0);
il.Emit(OpCodes.Ldarg_1);
il.Emit(OpCodes.Call, setter);
il.Emit(OpCodes.Ret);

Type builtType = typeBuilder.CreateType()!;
MethodInfo method = builtType.GetMethod("SetName", BindingFlags.Public | BindingFlags.Static)!;

var del = method.CreateDelegate<Action<Product<int>, string>>();
del(milk, "toast");
Console.WriteLine(milk);

public class Product<T> {
    public string Name { get; init; }
}

Configuration

.NET 5.0. Repros on .NET Framework 4.8 as well.
x64

Regression?

Not a regression, since the repro depends on init property setters which was only introduced in .NET 5.0.

Other information

See MessagePack-CSharp/MessagePack-CSharp#1134 which tracks this within the MessagePack repo.

Why does .NET 5.0 throw at runtime as if the property setter was not defined? And why only when the class is generic?

If I change from init to set (or from generic class to non-generic), the problem goes away, even though the emitted IL is identical. The init setter and set setter are nearly identical too. There's just one small bit of metadata on an init setter that's different (that messagepack doesn't care about):

+instance void modreq([System.Runtime]System.Runtime.CompilerServices.IsExternalInit) set_Name (
-instance void set_Name (

The problem does not repro when using DynamicMethod. It only fails when using MethodBuilder.

@Dotnet-GitSync-Bot
Copy link
Collaborator

I couldn't figure out the best area label to add to this issue. If you have write-permissions please help me learn by adding exactly one area label.

@Dotnet-GitSync-Bot Dotnet-GitSync-Bot added the untriaged New issue has not been triaged by the area owner label Nov 23, 2020
@AArnott
Copy link
Contributor Author

AArnott commented Nov 23, 2020

Based on this doc it seems likely that our IL code gen has to be updated to include modreq([System.Runtime]System.Runtime.CompilerServices.IsExternalInit) in the called method signature somehow. It seems all C# compiler generated calls to init methods require this, and it's evident in decompilers but not in our code-generated calls to it.

As to why it only fails in generic classes may be a bug/detail of the .NET 5 runtime.

Is this actually a bug in reflection or ILGenerator? Should it be including the modreq return type modifier in the call IL instruction but is leaving it out?

@AArnott
Copy link
Contributor Author

AArnott commented Nov 23, 2020

Confirmed: when the class is not generic, ILGenerator produces this:

image

But when the class is generic, ILGenerator only produces this:

image

So the modreq is missing in the generic class case.

@wzchua
Copy link
Contributor

wzchua commented Nov 23, 2020

This is related to #25958.

#40587 fixed this and #44452 fix a bug I had in the fix

@AArnott
Copy link
Contributor Author

AArnott commented Nov 23, 2020

@steveharter advocated for #40587 to be merged into 6.0 due to risk, it seems.
Now that it's there, can we port to 5.0? While the bug has been around for a while, the C# 9 init property accessor and positional records features make hitting this much more likely and is breaking not just mocking frameworks but deserializers.

@AArnott
Copy link
Contributor Author

AArnott commented Nov 23, 2020

@wzchua does [this](https://github.com/dotnet/runtime/issues/25958#issuecomment-543403476] describe a workaround that I could possibly implement to fix-up the generated IL so I can run on current versions of runtimes?
Does this bug happen on .NET Framework as well?

@AArnott
Copy link
Contributor Author

AArnott commented Nov 23, 2020

Ya, .NET Framework has this bug as well.

I tried using SignatureHelper.GetMethodSigHelper to workaround the problem by building my own signature, but the public APIs on that type are woefully deficient it seems. There's no way to set required modifiers on the return type, and even adding arguments throws NRE when calling it in what seems like a reasonable way. But I'm guessing because I can't find any good documentation for this class or sample uses of it outside of dotnet/runtime where everything used is internal.

@wzchua
Copy link
Contributor

wzchua commented Nov 23, 2020

@AArnott no. That describes why the previous attempt at fixing the issue broke something.

I thought SignatureHelper would work but it can't. There's a step where the class token is operated with the method signature to generate some token. It calls into the unmanaged part of the runtime.

But I think setting the backing field directly should work but it's not the same as calling the setter.

@AArnott
Copy link
Contributor Author

AArnott commented Nov 23, 2020

I think setting the backing field directly should work but it's not the same as calling the setter.

That would require some way to associate the backing field with the property setter, assuming there even is a backing field that setting would be equivalent.

We should also consider adding API to SignatureHelper and ILGenerator to allow folks to do this sort of thing themselves, IMO.
In the meantime, since DynamicMethod works (albeit it takes a slower path in MessagePack to do so) I'll see what I can do to automatically "fail over" to that path when init property setters are encountered.

@wzchua
Copy link
Contributor

wzchua commented Nov 24, 2020

This is probably super fragile but I extracted the code path a generic class + non-generic methodInfo takes for token generation
https://gist.github.com/wzchua/a72cfea67ad514234364c10da92fe08f

@steveharter steveharter added this to the 6.0.0 milestone Nov 24, 2020
@steveharter steveharter added api-suggestion Early API idea and discussion, it is NOT ready for implementation and removed untriaged New issue has not been triaged by the area owner labels Nov 24, 2020
@steveharter
Copy link
Member

@AArnott does this still repro in 6.0? If not, I assume you want #40587 and #44452 ported to 5.0?

Changed this to API suggestion for now, since SignatureHelper should have a way to specify the modifiers.

@AArnott
Copy link
Contributor Author

AArnott commented Nov 24, 2020

Thanks for the workaround, @wzchua. Ya, reflecting into internals sounds quite fragile, as you say.

@wzchua : Is there a (safer?) way to simply detect if your fix is present in a given runtime? Right now I'm changing MessagePack to not even try to support the scenario since it's broken, but I'd like it to "light up" properly when running on a fixed runtime (whether that's 6.0 or 5.0 after it's fixed).

@steveharter : it turns out at least for VS's own purposes, we use messagepack settings that lead to it using DynamicMethod internally so this isn't impacting VS. And VS isn't using .NET 5.0 anyway, so no, I won't push hard to port the fix to .NET 5.0, although I would support it since I expect it will impact other messagepack customers before .NET 6 becomes mainstream.

@wzchua
Copy link
Contributor

wzchua commented Nov 26, 2020

@AArnott, there is really no other way other than generating a method, invoking it and catching the exception.

@steveharter
Copy link
Member

Closing per feedback:

this isn't impacting VS. And VS isn't using .NET 5.0 anyway, so no, I won't push hard to port the fix to .NET 5.0,

@ghost ghost locked as resolved and limited conversation to collaborators Sep 3, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
api-suggestion Early API idea and discussion, it is NOT ready for implementation area-System.Reflection.Emit
Projects
No open projects
Development

No branches or pull requests

5 participants