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.

Sign up for GitHub

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

Jump to bottom

Implement handling PreserveSig = false for interfaces #1206

Merged
merged 9 commits into from
Jun 8, 2021
Merged

Implement handling PreserveSig = false for interfaces #1206

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
ba68a4b
Implement handling PerserveSig = false for interfaces
kant2002 Jun 7, 2021
f614212
Ignore delegating marshalling thunks
kant2002 Jun 7, 2021
15a5c68
I add special case for handling blittable types which are return type…
kant2002 Jun 7, 2021
48de7aa
Remove artificial constraing on PreserveSig=false
kant2002 Jun 7, 2021
0b5bb1c
Address PR feedback
kant2002 Jun 7, 2021
dbdb239
Fix
kant2002 Jun 8, 2021
5903921
Add simple tests for PreservSig=false functionality
kant2002 Jun 8, 2021
df9bdb1
Update src/coreclr/tools/Common/TypeSystem/Interop/IL/Marshaller.cs
kant2002 Jun 8, 2021
5e098f6
Reformat code based on PR feedback
kant2002 Jun 8, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
35 changes: 28 additions & 7 deletions src/coreclr/tools/Common/TypeSystem/IL/Stubs/PInvokeILEmitter.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -97,7 +97,9 @@ private static Marshaller[] InitializeMarshallers(MethodDesc targetMethod, Inter
TypeDesc parameterType = (i == 0)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(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];
}

? methodSig.ReturnType //first item is the return type
: methodSig[i - 1];
marshallers[i] = Marshaller.CreateMarshaller(parameterType,
bool isHRSwapping = !flags.PreserveSig && !parameterType.IsVoid
&& targetMethod is not DelegateMarshallingMethodThunk;
marshallers[i] = Marshaller.CreateMarshaller((i == 0 && isHRSwapping) ? methodSig.Context.GetByRefType(parameterType): parameterType,
parameterIndex,
methodSig.GetEmbeddedSignatureData(),
MarshallerType.Argument,
Expand All @@ -108,8 +110,8 @@ private static Marshaller[] InitializeMarshallers(MethodDesc targetMethod, Inter
indexOffset + parameterMetadata.Index,
flags,
parameterMetadata.In,
parameterMetadata.Out,
parameterMetadata.Return
(i == 0 && isHRSwapping) ? true : parameterMetadata.Out,
(i == 0 && isHRSwapping) ? false : parameterMetadata.Return
);
}

Expand Down Expand Up @@ -231,16 +233,23 @@ private void EmitDelegateCall(DelegateMarshallingMethodThunk delegateMethod, PIn

private void EmitPInvokeCall(PInvokeILCodeStreams ilCodeStreams)
{
if (!_flags.PreserveSig && _targetMethod.Signature.ReturnType != _targetMethod.Context.GetWellKnownType(WellKnownType.Void))
throw new NotSupportedException();
if (!_flags.PreserveSig)
{
TypeDesc managedReturnType = _targetMethod.Signature.ReturnType;
bool supportedType = managedReturnType.IsPrimitive || managedReturnType.IsInterface || managedReturnType.IsObject;
jkotas marked this conversation as resolved.
Show resolved Hide resolved
if (!supportedType)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What prevents the other cases from being supported? I would expect the implementation to work for all marshalers without special casing.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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;
}

throw new NotSupportedException();
}

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)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

== targetMethod.Context.GetWellKnownType(WellKnownType.Void) can be just IsVoid

&& _targetMethod is not DelegateMarshallingMethodThunk;
TypeDesc nativeReturnType = _flags.PreserveSig ? _marshallers[0].NativeParameterType : context.GetWellKnownType(WellKnownType.Int32);
TypeDesc[] nativeParameterTypes = new TypeDesc[_marshallers.Length - 1];
TypeDesc[] nativeParameterTypes = new TypeDesc[isHRSwapping ? _marshallers.Length : _marshallers.Length - 1];

// if the SetLastError flag is set in DllImport, clear the error code before doing P/Invoke
if (_flags.SetLastError)
Expand All @@ -254,6 +263,11 @@ private void EmitPInvokeCall(PInvokeILCodeStreams ilCodeStreams)
nativeParameterTypes[i - 1] = _marshallers[i].NativeParameterType;
}

if (isHRSwapping)
{
nativeParameterTypes[_marshallers.Length - 1] = _marshallers[0].NativeParameterType;
}

if (!_pInvokeILEmitterConfiguration.GenerateDirectCall(_targetMethod, out _))
{
MetadataType lazyHelperType = context.GetHelperType("InteropHelpers");
Expand Down Expand Up @@ -357,11 +371,18 @@ private MethodIL EmitIL()
cleanupCodestream.BeginHandler(tryFinally);

// Marshal the arguments
for (int i = 0; i < _marshallers.Length; i++)
bool isHRSwapping = !_flags.PreserveSig && _targetMethod.Signature.ReturnType != _targetMethod.Context.GetWellKnownType(WellKnownType.Void)
&& _targetMethod is not DelegateMarshallingMethodThunk;
for (int i = isHRSwapping ? 1 : 0; i < _marshallers.Length; i++)
{
_marshallers[i].EmitMarshallingIL(pInvokeILCodeStreams);
}

if (isHRSwapping)
{
_marshallers[0].EmitMarshallingIL(pInvokeILCodeStreams);
}

// make the call
switch (_targetMethod)
{
Expand Down
25 changes: 19 additions & 6 deletions src/coreclr/tools/Common/TypeSystem/Interop/IL/Marshaller.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -512,7 +512,7 @@ protected virtual void EmitMarshalReturnValueManagedToNative()

public virtual void LoadReturnValue(ILCodeStream codeStream)
{
Debug.Assert(Return);
Debug.Assert(Return || Index == 0);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be nice to replace all Index == 0 checks with a property. It would make the code much easier to understand.


switch (MarshalDirection)
{
Expand Down Expand Up @@ -653,9 +653,18 @@ protected void PropagateFromByRefArg(ILCodeStream stream, Home home)
/// </summary>
protected void PropagateToByRefArg(ILCodeStream stream, Home home)
{
stream.EmitLdArg(Index - 1);
home.LoadValue(stream);
stream.EmitStInd(ManagedType);
// If by-ref arg has index == 0 then that argument is used for HR swapping and we just return that value.
if (Index == 0)
{
// Returning result would be handled by LoadReturnValue
return;
}
else
{
stream.EmitLdArg(Index - 1);
home.LoadValue(stream);
stream.EmitStInd(ManagedType);
}
kant2002 marked this conversation as resolved.
Show resolved Hide resolved
}

protected virtual void EmitMarshalArgumentManagedToNative()
Expand Down Expand Up @@ -903,7 +912,7 @@ class BlittableValueMarshaller : Marshaller
{
protected override void EmitMarshalArgumentManagedToNative()
{
if (IsNativeByRef && MarshalDirection == MarshalDirection.Forward)
if (IsNativeByRef && MarshalDirection == MarshalDirection.Forward && Index > 0)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would move the special case first and then leave the rest of the method intact.

if (IsHRSwappedRetVal)
{
    ...;
    return;
}

{
ILCodeStream marshallingCodeStream = _ilCodeStreams.MarshallingCodeStream;
ILEmitter emitter = _ilCodeStreams.Emitter;
Expand All @@ -917,10 +926,14 @@ protected override void EmitMarshalArgumentManagedToNative()
marshallingCodeStream.EmitStLoc(native);
_ilCodeStreams.CallsiteSetupCodeStream.EmitLdLoc(native);
}
else
else if (Index > 0)
{
_ilCodeStreams.CallsiteSetupCodeStream.EmitLdArg(Index - 1);
}
else
{
base.EmitMarshalArgumentManagedToNative();
}
}

protected override void EmitMarshalArgumentNativeToManaged()
Expand Down