dotnet / runtime

.NET is a cross-platform runtime for cloud, mobile, desktop, and IoT apps.
https://docs.microsoft.com/dotnet/core/
MIT License
15.15k stars 4.71k forks source link

[mono] System.Reflection.Emit doesn't propagate marshalling attributes to the created type #58783

Open evilzhou opened 3 years ago

evilzhou commented 3 years ago

See the comment below for a concise repro: https://github.com/dotnet/runtime/issues/58783#issuecomment-915439461

Some values relevant to marshaling from a StructLayoutAttribute and MarshalAsAttribute are not propagated to the dynamically created type.


Original bug report:

I tested it in Xamarin, but dotnet runtime also exists:

B8e011799c6fc43a1b8891993a90da9ac637658856013765092_20210830-100000-image B9efe2a3af0c148e0b69db8730f9537f8637658856958724359_20210830-100135-image Bf65da309c9ed4158be6cbdc4474836fe637658858144282684_20210830-100333-image

The correct size should be 100, which is correct on .net framework but wrong on mono. When I debug this code with mono on win, I found that this error is related to mono’s implementation of DynamicImage, which is caused by the attribute not being successfully applied during marshal.

dotnet-issue-labeler[bot] commented 3 years ago

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.

AaronRobinsonMSFT commented 3 years ago

@evilzhou Instead of posing screenshots, please post code that can be copied into an editor. Attempting to reproduce these scenarios locally is error prone and cumbersome attempting to transpose from an image.

AaronRobinsonMSFT commented 3 years ago
[StructLayout(LayoutKind.Sequential, Pack = 1)]
struct Static
{
    [MarshalAs(UnmanagedType.ByValArray, SizeConst = 100)]
    public byte[] buffer;
}

var name = new AssemblyName("Foobar");
var ab = AssemblyBuilder.DefineDynamicAssembly(name, AssemblyBuilderAccess.RunAndCollect);
var mb = ab.DefineDynamicModule(name.Name + ".dll");

var tb = mb.DefineType("teststruct", TypeAttributes.Public | TypeAttributes.SequentialLayout, typeof(ValueType), PackingSize.Size1);
var bf = tb.DefineField("buffer", typeof(byte[]), FieldAttributes.Public | FieldAttributes.HasFieldMarshal);

bf.SetCustomAttribute(new CustomAttributeBuilder(typeof(MarshalAsAttribute).GetConstructor(new Type[] { typeof(UnmanagedType) }),
    new object[] { UnmanagedType.ByValArray },
    new[] { typeof(MarshalAsAttribute).GetField("SizeConst") },
    new object[] { 100 }
)) ;

Type t = tb.CreateType();

Console.WriteLine($"Dynamic Size: {Marshal.SizeOf(t)}");
Console.WriteLine($"Static Size: {Marshal.SizeOf(typeof(Static))}");

Output on Mono:

Dynamic Size: 8
Static Size: 100
AaronRobinsonMSFT commented 3 years ago

Based on some local debugging the Mono Reflection.Emit code paths are not propagating some TypeBuilder values. The most obvious ones are the PackingSize.Size1, which is t.StructLayoutAttribute.Pack == 8 instead of 1, and the custom attribute placed on the buffer field, t.GetFields()[0].CustomAttributes.Count() == 0.

If t is set to typeof(Static), the above values are respectively 1 and 1, as expected.

/cc @lambdageek

ghost commented 3 years ago

Tagging subscribers to this area: See info in area-owners.md if you want to be subscribed.

Issue Details
I tested it in Xamarin, but dotnet runtime also exists: ![B8e011799c6fc43a1b8891993a90da9ac637658856013765092_20210830-100000-image](https://user-images.githubusercontent.com/14306064/132432954-eb54c2d2-7891-4de5-9d4a-adeee9dd756b.png) ![B9efe2a3af0c148e0b69db8730f9537f8637658856958724359_20210830-100135-image](https://user-images.githubusercontent.com/14306064/132432962-632c48e7-4e90-47b5-bede-9e755e4709f1.png) ![Bf65da309c9ed4158be6cbdc4474836fe637658858144282684_20210830-100333-image](https://user-images.githubusercontent.com/14306064/132432968-5e2d8639-ceb3-4a72-a0ca-9604fbdd7ded.png) The correct size should be 100, which is correct on .net framework but wrong on mono. When I debug this code with mono on win, I found that this error is related to mono’s implementation of DynamicImage, which is caused by the attribute not being successfully applied during marshal.
Author: evilzhou
Assignees: -
Labels: `untriaged`, `area-Interop-mono`, `area-System.Reflection-mono`
Milestone: -
lambdageek commented 2 years ago

@fanyang-mono I think this is related to the issue we talked about.

So essentially the problem is that we need to extend typebuilder_setup_one_field in sre.c to do something with the MonoReflectionFieldBuilder:marshal_info field - basically convert it into some marshal specs.

Compare with what reflection_methodbuilder_to_mono_method does with the MonoReflectionParamBuilder:marshal_info fields around here https://github.com/dotnet/runtime/blob/1e48eca9577433526c5c649a87ca2f2545d35958/src/mono/mono/metadata/sre.c#L3098-L3115 It allocates an array of MonoMarshalSpec* and fills it with the result of calling mono_marshal_spec_from_builder.

So we need to do something similar in typebuilder_setup_fields:

  1. Allocate a new MonoMarshalType with enough space to hold info about all the fields.
  2. Populate the MonoMarshalField struct in the MonoMarshalType:fields array with the MonoClassField and MonoMarshalSpec (the former is created in typebuilder_setup_one_field the latter is the result of calling mono_marshal_spec_from_type_builder).
  3. Then call mono_class_set_marshal_info (klass, info) to associate the MonoMarshalType with the constructed class that the typebuilder creates.

Compare with what is done for non-reflection classes in mono_marshal_load_type_info except instead of calling functions like mono_class_get_fields_internal and mono_metadata_field_info_with_mempool you would instead create the marshal info for each field manually from the MonoReflectionFieldBuilder and its MonoReflectionMarshal member.

fanyang-mono commented 2 years ago

@evilzhou Has this code worked correctly for you before?

evilzhou commented 2 years ago

@evilzhou Has this code worked correctly for you before?

@fanyang-mono No, there are always problems

fanyang-mono commented 2 years ago

@evilzhou Has this code worked correctly for you before?

@fanyang-mono No, there are always problems

Thanks for confirming that this is a missing functionality rather than a regression.

fanyang-mono commented 2 years ago

Moving to 8.0.0

fanyang-mono commented 1 year ago

Moving to 9.0.0

fanyang-mono commented 1 year ago

cc: @ivanpovazan

lambdageek commented 2 months ago

There's an outline of the work that might be required in https://github.com/dotnet/runtime/issues/58783#issuecomment-960428396, however using System.Reflection.Emit to create P/Invokes is somewhat rare on the platforms that use Mono (because they're mostly AOT)