Block P/Invokes with UnmanagedCallersOnlyAttribute

[AaronRobinsonMSFT]

See #38209 (comment) regarding mono implementation.

/cc @jkotas @lambdageek

[lambdageek]

@AaronRobinsonMSFT there's no mono implementation yet. 😁 I just had an idea this morning and thought it was worth writing down.

[AaronRobinsonMSFT]

@lambdageek Ah. Someone pointed out your comment offline. Once I realized it wasn't handled in coreclr I decided to investigate. Took very little to actually validate and see the problem. This seems more like an optimization rather than anything that we need to support, but after talking it out I realized it does have value and I don't think too much risk. Let me know if you think it is worth adding or blocking.

[jkotas]

This seems more like an optimization

I do not think it is just an optimization. It affects the function pointer behavior you get from these in observable way. Before this change, the function pointer was managed. After this change, the function pointer is unmanaged.

There are other places in the runtime that get method entrypoint and that do not go through TryGetMultiCallableAddrOfCode. They will still get the old behavior.

Do we need a test for this? E.g. what happens when you create delegate to one of these methods and call that delegate; what happens when you call this method via reflection; etc.

I am wondering whether it would be best to error out for this case.

[AaronRobinsonMSFT]

I meant scenario optimization, not performance. Perhaps "convenience semantic" is a more appropriate description. The only reason I can see doing this is to permit the ability to reuse a variable/member. For example, a variable/member is initialized with a function pointer that was constructed by a DllImport and then later on the variable/member could be updated by the result of the NativeLibrary.GetExport() API. The developer could then use a C# function pointer variable/member regardless of origin. This is what I meant by "optimization".

Your statement about the result of the & is indeed correct. From a users perspective I think applying the semantics of UnmanagedCallersOnlyAttribute only as the result of & is unusual but could be easily described if one considers the intent of the two attributes. The combination of UnmanagedCallersOnlyAttribute and DllImport seems weird in general except for the previously stated use case.

There are other places in the runtime that get method entrypoint and that do not go through TryGetMultiCallableAddrOfCode. They will still get the old behavior.

Do we need a test for this? E.g. what happens when you create delegate to one of these methods and call that delegate; what happens when you call this method via reflection; etc.

Yes, tests are warranted. This PR isn't complete since I didn't add tests and figured I missed something - as alluded to above. What other entry-points? I looked through the layers and honestly it was a bit tough to navigate. The guts of TryGetMultiCallableAddrOfCode are a bit opaque to say the least. Suggestions on how to trigger these code paths are appreciated.

I am wondering whether it would be best to error out for this case.

That is another reason I didn't want to write tests. Without function pointers in the repro these tests get a bit wordy. However, I assume that all the places I would need to make this work are the same places I would need to throw an exception so a few tests will be needed either way.

[jkotas]

What other entry-points?

E.g. MethodDesc::GetSingleCallableAddrOfCode. MethodDesc::GetSingle/MultipleCallableAddrOfCode and other similar entrypoint API are meant to just produce a function pointer that one can call, without doing any heavy lifting to produce the code that is behind the function pointer. For example, these APIs won't do any JITing today.

Checks like this may be better to add to the existing places where we are doing the heavy lifting, e.g. where we are generating or JITing the marshalling stubs.