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

Handle blittable byref returns in built-in marshalling #72433

Merged
merged 1 commit into from
Jul 20, 2022
Merged

Handle blittable byref returns in built-in marshalling #72433

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
59 changes: 27 additions & 32 deletions src/coreclr/tools/Common/TypeSystem/Interop/IL/MarshalHelpers.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -165,17 +165,14 @@ internal static TypeDesc GetNativeTypeFromMarshallerKind(TypeDesc type,
case MarshallerKind.LayoutClassPtr:
case MarshallerKind.AsAnyA:
case MarshallerKind.AsAnyW:
return context.GetWellKnownType(WellKnownType.IntPtr);

case MarshallerKind.ComInterface:
case MarshallerKind.CustomMarshaler:
case MarshallerKind.BlittableValueClassByRefReturn:
return context.GetWellKnownType(WellKnownType.IntPtr);

#if !READYTORUN
case MarshallerKind.Variant:
return InteropTypes.GetVariant(context);

case MarshallerKind.CustomMarshaler:
return context.GetWellKnownType(WellKnownType.IntPtr);
#endif

case MarshallerKind.OleCurrency:
Expand Down Expand Up @@ -228,29 +225,35 @@ internal static MarshallerKind GetMarshallerKind(
{
elementMarshallerKind = MarshallerKind.Invalid;

bool isByRef = false;
TypeSystemContext context = type.Context;
NativeTypeKind nativeType = NativeTypeKind.Default;
bool isField = marshallerType == MarshallerType.Field;

if (marshalAs != null)
nativeType = marshalAs.Type;

if (type.IsByRef)
{
isByRef = true;

type = type.GetParameterType();

if (!type.IsPrimitive && type.IsValueType && marshallerType != MarshallerType.Field
if (type.IsValueType && !type.IsPrimitive && !type.IsEnum && !isField
Copy link
Member Author

@jkotas jkotas Jul 19, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Trying to make these conditions consistent and more in sync with CoreCLR logic. This path should be only ever used by managed C++ so it does not matter a whole lot anyway.

&& HasCopyConstructorCustomModifier(parameterIndex, customModifierData))
{
return MarshallerKind.BlittableValueClassWithCopyCtor;
}

// Compat note: CLR allows ref returning blittable structs for IJW
if (isReturn)
{
// Allow ref returning blittable structs for IJW
if (type.IsValueType &&
(nativeType == NativeTypeKind.Struct || nativeType == NativeTypeKind.Default) &&
MarshalUtils.IsBlittableType(type))
{
return MarshallerKind.BlittableValueClassByRefReturn;
}
return MarshallerKind.Invalid;
}
}
TypeSystemContext context = type.Context;
NativeTypeKind nativeType = NativeTypeKind.Default;
bool isField = marshallerType == MarshallerType.Field;

if (marshalAs != null)
nativeType = marshalAs.Type;

if (nativeType == NativeTypeKind.CustomMarshaler)
{
Expand Down Expand Up @@ -452,14 +455,6 @@ internal static MarshallerKind GetMarshallerKind(
}
else if (type.IsSzArray)
{
#if READYTORUN
jkotas marked this conversation as resolved.
Show resolved Hide resolved
// We don't want the additional test/maintenance cost of this in R2R.
if (isByRef)
return MarshallerKind.Invalid;
#else
_ = isByRef;
#endif

if (nativeType == NativeTypeKind.Default)
nativeType = NativeTypeKind.Array;

Expand Down Expand Up @@ -515,16 +510,16 @@ internal static MarshallerKind GetMarshallerKind(
}
else if (type.IsPointer)
{
if (nativeType == NativeTypeKind.Default)
type = type.GetParameterType();

if (type.IsValueType && !type.IsPrimitive && !type.IsEnum && !isField
&& HasCopyConstructorCustomModifier(parameterIndex, customModifierData))
{
var pointedAtType = type.GetParameterType();
if (!pointedAtType.IsPrimitive && !type.IsEnum && marshallerType != MarshallerType.Field
Copy link
Member Author

@jkotas jkotas Jul 19, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

type.IsEnum was always false here since type was a byref.

&& HasCopyConstructorCustomModifier(parameterIndex, customModifierData))
{
return MarshallerKind.BlittableValueClassWithCopyCtor;
}
return MarshallerKind.BlittableValue;
return MarshallerKind.BlittableValueClassWithCopyCtor;
}

if (nativeType == NativeTypeKind.Default)
return MarshallerKind.BlittableValue;
else
return MarshallerKind.Invalid;
}
Expand Down
14 changes: 14 additions & 0 deletions src/coreclr/tools/Common/TypeSystem/Interop/IL/Marshaller.Aot.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -92,6 +92,9 @@ protected static Marshaller CreateMarshaller(MarshallerKind kind)
return new VariantMarshaller();
case MarshallerKind.CustomMarshaler:
return new CustomTypeMarshaller();
case MarshallerKind.BlittableValueClassByRefReturn:
return new BlittableValueClassByRefReturn();

default:
// ensures we don't throw during create marshaller. We will throw NSE
// during EmitIL which will be handled and an Exception method body
Expand Down Expand Up @@ -1279,4 +1282,15 @@ protected void EmitCleanUpNativeData(ILCodeStream codeStream)
codeStream.Emit(ILOpcode.callvirt, emitter.NewToken(cleanupNativeDataMethod));
}
}

class BlittableValueClassByRefReturn : Marshaller
{
protected override void SetupArgumentsForReturnValueMarshalling()
{
ILEmitter emitter = _ilCodeStreams.Emitter;

_managedHome = new Home(emitter.NewLocal(ManagedParameterType), ManagedParameterType, isByRef: false);
Copy link
Member Author

Choose a reason for hiding this comment

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

We just need to create locals of the right type. The default implementation works fine otherwise.

_nativeHome = new Home(emitter.NewLocal(NativeType), NativeType, isByRef: false);
}
}
}
1 change: 1 addition & 0 deletions src/coreclr/tools/Common/TypeSystem/Interop/IL/MarshallerKind.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,7 @@ enum MarshallerKind
AsAnyW,
FailedTypeLoad,
ComInterface,
BlittableValueClassByRefReturn,
BlittableValueClassWithCopyCtor,
CustomMarshaler,
Invalid
Expand Down
7 changes: 7 additions & 0 deletions src/tests/nativeaot/SmokeTests/PInvoke/PInvoke.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,9 @@ internal class Program
[DllImport("PInvokeNative", CallingConvention = CallingConvention.StdCall)]
private static extern int VerifyByRefFoo(ref Foo value);

[DllImport("PInvokeNative", CallingConvention = CallingConvention.StdCall)]
private static extern ref Foo VerifyByRefFooReturn();

[DllImport("PInvokeNative", CallingConvention = CallingConvention.StdCall, CharSet = CharSet.Unicode)]
private static extern bool GetNextChar(ref char c);

Expand Down Expand Up @@ -432,6 +435,10 @@ private static void TestByRef()

ThrowIfNotEquals(foo.a, 11, "By ref struct unmarshalling failed");
ThrowIfNotEquals(foo.b, 21.0f, "By ref struct unmarshalling failed");

ref Foo retfoo = ref VerifyByRefFooReturn();
ThrowIfNotEquals(retfoo.a, 42, "By ref struct return failed");
ThrowIfNotEquals(retfoo.b, 43.0, "By ref struct return failed");
}

private static void TestString()
Expand Down
7 changes: 7 additions & 0 deletions src/tests/nativeaot/SmokeTests/PInvoke/PInvokeNative.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -112,6 +112,13 @@ DLL_EXPORT int __stdcall VerifyByRefFoo(Foo *val)
return 0;
}

static Foo s_foo = { 42, 43.0 };

DLL_EXPORT Foo* __stdcall VerifyByRefFooReturn()
{
return &s_foo;
}

DLL_EXPORT bool __stdcall GetNextChar(short *value)
{
if (value == NULL)
Expand Down