Implement handling PreserveSig = false for interfaces #1206
Conversation
This was done by pretending return value is out argument passed as last value. We still need to return that value, so this require change in the Marshaller.cs so it would not throw assertion, since previously only Return values can return value.
|
I opt out of DelegateMarshallingMethodThunk because when these classes created with |
… for PreserveSig=false methods. In this case I fallback to default marshalling implementation.
|
I add special case for handling blittable types which are return type for PreserveSig=false methods. In this case I fallback to default marshalling implementation. |
jkotas
changed the title
It would be nice to make it work the same way as in CoreCLR. |
| { | ||
| TypeDesc managedReturnType = _targetMethod.Signature.ReturnType; | ||
| bool supportedType = managedReturnType.IsPrimitive || managedReturnType.IsInterface || managedReturnType.IsObject; | ||
| if (!supportedType) |
There was a problem hiding this comment.
What prevents the other cases from being supported? I would expect the implementation to work for all marshalers without special casing.
There was a problem hiding this comment.
It require more testing. Nothing in principle, just my inability to see all supported cases.
Also https://github.com/dotnet/runtime/blob/1843cdef72e1e7c810c4ea85c677d679ff3b66f4/src/coreclr/vm/dllimport.cpp#L3560-L3564 indicate cases which are not supported, but that does not give me much information what is supported. I think I can add Enum and String without probably much problems if needed.
There was a problem hiding this comment.
All types that can be marshalled should be supported.
The unsupported cases in CoreCLR are just a legacy baggage from the times where the marshalling stubs were hand-emitted x86 assembly and handling them would require writing a pile of new hand-emitted x86 assembly. We should not need need to worry about artificially throwing exceptions for the cases that are unsupported in CoreCLR for no good reason.
I think that the transformation for PreserveSig = false should be very regular, it should depend on the regular [out] marshalling. It should not depend on the actual type at all.
[PreserveSig = false]
RETVAL foo(...);
should be transformed to:
RETVAL foo(...)
{
RETVAL retNative = default;
marshal everything usual
actual_call(..., &retNative);
unmarshal everything as usual (including retNative / retManaged)
return retManaged;
}
src/coreclr/tools/Common/TypeSystem/IL/Stubs/PInvokeILEmitter.cs
Outdated
Show resolved
Hide resolved
Reverse marshalling I do not fully grasp yet. I do not yet hit case where I was involved enough to implement. |
| @@ -97,7 +97,9 @@ private static Marshaller[] InitializeMarshallers(MethodDesc targetMethod, Inter | |||
| TypeDesc parameterType = (i == 0) | |||
There was a problem hiding this comment.
(i == 0 && isHRSwapping) condition is repeated multiple times here. Also, I do not think you should need the check for DelegateMarshallingMethodThunk here. We should make sure to set PreserveSig=true for marshalled delegates earlier (UnmanagedFunctionPointerAttribute won't allow you to specify PreserveSig=false so we do not need to worry about handling it).
I think it would be a bit more efficient and readable like this:
TypeDesc parameterType;
bool isHRSwappedRetVal = false;
if (i == 0)
{
//first item is the return type
parameterType = methodSig.ReturnType;
if (!flags.PreserveSig && !parameterType.IsVoid)
{
// PreserveSig = false can only show up an regular forward PInvokes
Debug.Assert(direction == MarshalDirection.Forward);
parameterType = methodSig.Context.GetByRefType(parameterType);
isHRSwappedRetVal = true;
}
}
else
{
parameterType = methodSig[i - 1];
}
| ILEmitter emitter = ilCodeStreams.Emitter; | ||
| ILCodeStream fnptrLoadStream = ilCodeStreams.FunctionPointerLoadStream; | ||
| ILCodeStream callsiteSetupCodeStream = ilCodeStreams.CallsiteSetupCodeStream; | ||
| TypeSystemContext context = _targetMethod.Context; | ||
|
|
||
| bool isHRSwapping = !_flags.PreserveSig && _targetMethod.Signature.ReturnType != _targetMethod.Context.GetWellKnownType(WellKnownType.Void) |
There was a problem hiding this comment.
== targetMethod.Context.GetWellKnownType(WellKnownType.Void) can be just IsVoid
|
It would be nice to add one test for this to smoke tests. I think this is tricky enough to have it covered in the smoke tests. |
| @@ -512,7 +512,7 @@ protected virtual void EmitMarshalReturnValueManagedToNative() | |||
|
|
|||
| public virtual void LoadReturnValue(ILCodeStream codeStream) | |||
| { | |||
| Debug.Assert(Return); | |||
| Debug.Assert(Return || Index == 0); | |||
There was a problem hiding this comment.
It would be nice to replace all Index == 0 checks with a property. It would make the code much easier to understand.
| @@ -903,7 +912,7 @@ class BlittableValueMarshaller : Marshaller | |||
| { | |||
| protected override void EmitMarshalArgumentManagedToNative() | |||
| { | |||
| if (IsNativeByRef && MarshalDirection == MarshalDirection.Forward) | |||
| if (IsNativeByRef && MarshalDirection == MarshalDirection.Forward && Index > 0) | |||
There was a problem hiding this comment.
I would move the special case first and then leave the rest of the method intact.
if (IsHRSwappedRetVal)
{
...;
return;
}
Co-authored-by: Jan Kotas <jkotas@microsoft.com>
This was done by pretending return value is out argument passed as last value.
We still need to return that value, so this require change in the Marshaller.cs so it would not throw assertion, since previously only Return values can return value.
Closes #1183