Support function pointers in field marshalers

[AaronRobinsonMSFT]

Fixes dotnet/runtime#37295

This was fixed in .NET 5 when field marshalling was completely rewritten.

Customer Impact

Users will be unable to have function pointers as fields in structs that are used in interop scenarios. Function pointers are now supported in the C# language.

Workaround

This can naturally be worked around by using IntPtr, UIntPtr, void*, nint, or nuint as the field and then casting to the appropriate function pointer type at the usage site in managed code. This would severely degrade the UX for usage of function pointers though.

Regression?

No. This is supporting a new C# feature and previous .NET runtimes never tested/supported this scenario.

Testing

Validated both 32 and 64 bit struct of the field marshalling scenario. This follows the same behavior as a void* pointer so no new functionality is added just reuse of an existing code path for a new type.

Risk

Low.

/cc @tannergooding @333fred @jkoritzinsky @jkotas

[AaronRobinsonMSFT]

/cc @GrabYourPitchforks

[tannergooding]

@AaronRobinsonMSFT, is it worth updating the customer impact from:

Users will be unable to have function pointers as fields in structs. Function pointers are now supported in the C# language.
to
Users will be unable to have function pointers as fields in structs that are used in interop scenarios. Function pointers are now supported in the C# language.

Since they "work" just fine from managed code, its only when trying to use them with interop or when relying on the layout to actually be Sequential that it will be problematic.

[AaronRobinsonMSFT]

@tannergooding Yes. That clarification is important - thanks. Updated.

[jkotas]

I am sorry but I do not think this makes the bar for backport to 3.1. Feel free to do bar check with servicing ship room.

We are focused on making sure that the latest .NET Runtime works well with latest C#. If you want to use latest C# with older .NET Runtimes, your experience will vary. We are not servicing the older runtimes nor going out of our way to create special OOB packages to make it work better. For reference, here is a similar discussion about nullability: dotnet/runtime#30493 (comment)

[tannergooding]

@jkotas is that going to be true when the latest .NET Runtime isn't an LTS? I'd understand not supporting 3.0 or 2.2 or 2.1 from that perspective, but given 3.1 is the latest LTS it would be nice to have libraries working on "latest LTS" + "latest runtime"...

[jkotas]

You can make the libraries work by using the workaround, on all .NET Core 2.1, 3.1 and .NET Framework.

[tannergooding]

You can make the libraries work by using the workaround, on all .NET Core 2.1, 3.1 and .NET Framework.

Fair enough. Although I think I'd rather just drop netcoreapp3.1 support than have to add casts to the ~14,000 function pointers that are in TerraFX.Interop.Windows COM VTBLs 😄.

It also looks like nint and IntPtr currently go through op_Explicit (will log a bug on this): https://sharplab.io/#v2:EYLgtghgzgLgpgJwDQxASwDYB8ACAmARgFgAoHAZgAIBXAOyggDM5L9KBhSgb1Mr9ao4ALJQCyACgCU3XvznjxAEzgY4AcwjwAVJViKAxhAwYAPMIB8kgB6SpAbll8Avo8quADgjQA3TS1potDCUVg4kTkA=

You need to use void* to avoid any IL overhead: https://sharplab.io/#v2:EYLgtghgzgLgpgJwDQxASwDYB8ACAmARgFgAoHAZgAIBXAOyggDM5L9KBhSgb1Mr9ao4ALJQCyACgCU3XvznjxAEzgY4AcwjwAVJViKAxhAwYAPMIB8kgB6SpAbll8Avo8quADgjQA3TS2E6Vg4kTkA=

[AaronRobinsonMSFT]

I am sorry but I do not think this makes the bar for backport to 3.1. Feel free to do bar check with servicing ship room.

@jkotas That is okay. This didn't require a lot to investigate nor fix so figured I would toss it up here and see. I would like to support it, but understand since a workaround exists that that may reduce the need for servicing. Will defer to ship room bar check.

[jkotas]

TerraFX.Interop.Windows COM VTBLs

FWIW, it would better to avoid the strongly typed vtables and do the cast of the slot to the right type inline, e.g.:

void** lpVtbl;
...
   return ((delegate ...)(lpVtbl[3]))->Release((IXMLDOMDocument*)Unsafe.AsPointer(ref this));

The strongly types vtables are unnecessary overhead and work poorly with IL linking (keeping around stuff that can be trimmed away).

I assume that you are autogenerating this stuff and not typing all this manually, and so having slot numbers inline is not a big deal.

[AaronRobinsonMSFT]

I assume that you are autogenerating this stuff and not typing all this manually, and so having slot numbers inline is not a big deal.

I think I would push back on that. Part of the auto-generating approach is producing code that is debuggable and easier to diagnose. The above code snippet is not fun in a debugger at all. Not that is should be the highest priority, but being able to debug some of this is a benefit we should consider. Do we know how much are we actually saving in the example or in practice by removing all these types?

[tannergooding]

I logged terrafx/terrafx.interop.windows#83 to investigate. It certainly hurts readability.

The bindings are all autogenerated, including wrapper methods to fixup bits like struct returns and to handle the this pointer (which currently uses Unsafe.AsPointer(ref this) under the assumption that this is a T* from native land and therefore okay).

[jkotas]

I do not think that the debugging experience is significantly different between strongly typed vtables vs. inlined slots. You can make it easier to read by auto-generating this into multiple lines, introduce a local for the function pointer, etc.

Do we know how much are we actually saving in the example or in practice by removing all these types?

The strongly typed vtables increase the number of types required to make a thing work by 2x. The extra overhead from keeping extra stuff around with IL linker on can be arbitrary high.

[AaronRobinsonMSFT]

I do not think that the debugging experience is significantly different between strongly typed vtables vs. inlined slots. You can make it easier to read by auto-generating this into multiple lines, introduce a local for the function pointer, etc.

So my interpretation of this was wrong. The point is by defining void** the hidden vtable type - typically used only for dispatch purposes - doesn't need to exist and we save the duplication between the exposed function with signature that will be called and the hidden type's copy.

That is a convincing argument for not needing it - I'm on board.

[jkotas]

auto-generating approach is producing code that is debuggable and easier to diagnose

This argument was used a lot during MCG development. What happened in practice was:

• The auto-generated code is right pretty much all the time. It is rare for customers to debug it and benefit from the better readability of the auto-generated code.
• Since the code is auto-generated, there is a ton of it and so there is a strong desire to make it as lean as possible. It has a measurable benefit for all customers.

The result is that the auto-generators end up favoring efficiency over readability when there is a choice.

[tannergooding]

That just leaves cases like WNDCLASSEX, which is a regular struct containing some function pointer field: https://github.com/terrafx/terrafx.interop.windows/blob/master/sources/Interop/Windows/um/WinUser/WNDCLASSEXW.cs

Having this be void* is confusing as the actual signature isn't clear, but it also isn't the end of the world 😄. I count 283 instances (most of these are WinUser callbacks) of that pattern in my existing bindings (out of the 13,845 function pointers that currently exist, roughly half of which will go away if the VTBLs are no longer strongly typed).