From 71fffe9807d7c65cd3d459b70029641163bfe4b8 Mon Sep 17 00:00:00 2001 From: Jeremy Koritzinsky Date: Tue, 23 Mar 2021 15:46:00 -0700 Subject: [PATCH 1/5] When marshalling a layout class, fall-back to dynamically marshalling the type if it doesn't match the static type in the signature. --- src/coreclr/src/vm/corelib.h | 4 + src/coreclr/src/vm/ilmarshalers.cpp | 92 ++++++++++++++++++- src/coreclr/src/vm/ilmarshalers.h | 1 + src/coreclr/src/vm/metasig.h | 4 + src/coreclr/src/vm/mlinfo.cpp | 9 ++ .../Interop/LayoutClass/LayoutClassNative.cpp | 24 ++++- .../Interop/LayoutClass/LayoutClassTest.cs | 69 +++++++++++++- 7 files changed, 199 insertions(+), 4 deletions(-) diff --git a/src/coreclr/src/vm/corelib.h b/src/coreclr/src/vm/corelib.h index 75d13d2e52985d..9fada354c4ba86 100644 --- a/src/coreclr/src/vm/corelib.h +++ b/src/coreclr/src/vm/corelib.h @@ -488,6 +488,10 @@ DEFINE_METHOD(MARSHAL, ALLOC_CO_TASK_MEM, AllocCoTa DEFINE_METHOD(MARSHAL, FREE_CO_TASK_MEM, FreeCoTaskMem, SM_IntPtr_RetVoid) DEFINE_FIELD(MARSHAL, SYSTEM_MAX_DBCS_CHAR_SIZE, SystemMaxDBCSCharSize) +DEFINE_METHOD(MARSHAL, STRUCTURE_TO_PTR, StructureToPtr, SM_Obj_IntPtr_Bool_RetVoid) +DEFINE_METHOD(MARSHAL, PTR_TO_STRUCTURE, PtrToStructure, SM_IntPtr_Obj_RetVoid) +DEFINE_METHOD(MARSHAL, DESTROY_STRUCTURE, DestroyStructure, SM_IntPtr_Type_RetVoid) + DEFINE_CLASS(NATIVELIBRARY, Interop, NativeLibrary) DEFINE_METHOD(NATIVELIBRARY, LOADLIBRARYCALLBACKSTUB, LoadLibraryCallbackStub, SM_Str_AssemblyBase_Bool_UInt_RetIntPtr) diff --git a/src/coreclr/src/vm/ilmarshalers.cpp b/src/coreclr/src/vm/ilmarshalers.cpp index 0e74014a45f158..e435270f203b06 100644 --- a/src/coreclr/src/vm/ilmarshalers.cpp +++ b/src/coreclr/src/vm/ilmarshalers.cpp @@ -2264,7 +2264,22 @@ void ILLayoutClassPtrMarshalerBase::EmitClearNativeTemp(ILCodeStream* pslILEmit) } } +bool ILLayoutClassPtrMarshalerBase::EmitExactTypeCheck(ILCodeStream* pslILEmit, ILCodeLabel* isNotMatchingTypeLabel) +{ + if (m_pargs->m_pMT->IsSealed()) + { + // If the provided type cannot be derived from, then we don't need to emit the type check. + return false; + } + EmitLoadManagedValue(pslILEmit); + pslILEmit->EmitCALL(METHOD__OBJECT__GET_TYPE, 1, 1); + pslILEmit->EmitLDTOKEN(pslILEmit->GetToken(m_pargs->m_pMT)); + pslILEmit->EmitCALL(METHOD__TYPE__GET_TYPE_FROM_HANDLE, 1, 1); + pslILEmit->EmitCALLVIRT(pslILEmit->GetToken(CoreLibBinder::GetMethod(METHOD__OBJECT__EQUALS)), 1, 1); + pslILEmit->EmitBRFALSE(isNotMatchingTypeLabel); + return true; +} void ILLayoutClassPtrMarshaler::EmitConvertContentsCLRToNative(ILCodeStream* pslILEmit) { @@ -2281,6 +2296,9 @@ void ILLayoutClassPtrMarshaler::EmitConvertContentsCLRToNative(ILCodeStream* psl pslILEmit->EmitLDC(uNativeSize); pslILEmit->EmitINITBLK(); + ILCodeLabel* isNotMatchingTypeLabel = pslILEmit->NewCodeLabel(); + bool emittedTypeCheck = EmitExactTypeCheck(pslILEmit, isNotMatchingTypeLabel); + MethodDesc* pStructMarshalStub = NDirect::CreateStructMarshalILStub(m_pargs->m_pMT); EmitLoadManagedValue(pslILEmit); @@ -2290,6 +2308,18 @@ void ILLayoutClassPtrMarshaler::EmitConvertContentsCLRToNative(ILCodeStream* psl EmitLoadCleanupWorkList(pslILEmit); pslILEmit->EmitCALL(pslILEmit->GetToken(pStructMarshalStub), 4, 0); + + if (emittedTypeCheck) + { + pslILEmit->EmitBR(pNullRefLabel); + + pslILEmit->EmitLabel(isNotMatchingTypeLabel); + EmitLoadManagedValue(pslILEmit); + EmitLoadNativeValue(pslILEmit); + pslILEmit->EmitLDC(0); + pslILEmit->EmitCALL(METHOD__MARSHAL__STRUCTURE_TO_PTR, 3, 0); + } + pslILEmit->EmitLabel(pNullRefLabel); } @@ -2302,6 +2332,9 @@ void ILLayoutClassPtrMarshaler::EmitConvertContentsNativeToCLR(ILCodeStream* psl EmitLoadManagedValue(pslILEmit); pslILEmit->EmitBRFALSE(pNullRefLabel); + ILCodeLabel* isNotMatchingTypeLabel = pslILEmit->NewCodeLabel(); + bool emittedTypeCheck = EmitExactTypeCheck(pslILEmit, isNotMatchingTypeLabel); + MethodDesc* pStructMarshalStub = NDirect::CreateStructMarshalILStub(m_pargs->m_pMT); EmitLoadManagedValue(pslILEmit); @@ -2311,6 +2344,15 @@ void ILLayoutClassPtrMarshaler::EmitConvertContentsNativeToCLR(ILCodeStream* psl EmitLoadCleanupWorkList(pslILEmit); pslILEmit->EmitCALL(pslILEmit->GetToken(pStructMarshalStub), 4, 0); + if (emittedTypeCheck) + { + pslILEmit->EmitBR(pNullRefLabel); + + pslILEmit->EmitLabel(isNotMatchingTypeLabel); + EmitLoadNativeValue(pslILEmit); + EmitLoadManagedValue(pslILEmit); + pslILEmit->EmitCALL(METHOD__MARSHAL__PTR_TO_STRUCTURE, 2, 0); + } pslILEmit->EmitLabel(pNullRefLabel); } @@ -2318,6 +2360,10 @@ void ILLayoutClassPtrMarshaler::EmitClearNativeContents(ILCodeStream * pslILEmit { STANDARD_VM_CONTRACT; + ILCodeLabel* isNotMatchingTypeLabel = pslILEmit->NewCodeLabel(); + ILCodeLabel* cleanedUpLabel = pslILEmit->NewCodeLabel(); + bool emittedTypeCheck = EmitExactTypeCheck(pslILEmit, isNotMatchingTypeLabel); + MethodDesc* pStructMarshalStub = NDirect::CreateStructMarshalILStub(m_pargs->m_pMT); EmitLoadManagedValue(pslILEmit); @@ -2327,6 +2373,19 @@ void ILLayoutClassPtrMarshaler::EmitClearNativeContents(ILCodeStream * pslILEmit EmitLoadCleanupWorkList(pslILEmit); pslILEmit->EmitCALL(pslILEmit->GetToken(pStructMarshalStub), 4, 0); + + if (emittedTypeCheck) + { + pslILEmit->EmitBR(cleanedUpLabel); + + pslILEmit->EmitLabel(isNotMatchingTypeLabel); + EmitLoadNativeValue(pslILEmit); + EmitLoadManagedValue(pslILEmit); + pslILEmit->EmitCALL(METHOD__OBJECT__GET_TYPE, 1, 1); + pslILEmit->EmitCALL(METHOD__MARSHAL__DESTROY_STRUCTURE, 2, 0); + } + + pslILEmit->EmitLabel(cleanedUpLabel); } @@ -2341,6 +2400,9 @@ void ILBlittablePtrMarshaler::EmitConvertContentsCLRToNative(ILCodeStream* pslIL EmitLoadNativeValue(pslILEmit); pslILEmit->EmitBRFALSE(pNullRefLabel); + ILCodeLabel* isNotMatchingTypeLabel = pslILEmit->NewCodeLabel(); + bool emittedTypeCheck = EmitExactTypeCheck(pslILEmit, isNotMatchingTypeLabel); + EmitLoadNativeValue(pslILEmit); // dest EmitLoadManagedValue(pslILEmit); @@ -2349,6 +2411,17 @@ void ILBlittablePtrMarshaler::EmitConvertContentsCLRToNative(ILCodeStream* pslIL pslILEmit->EmitLDC(uNativeSize); // size pslILEmit->EmitCPBLK(); + + if (emittedTypeCheck) + { + pslILEmit->EmitBR(pNullRefLabel); + + pslILEmit->EmitLabel(isNotMatchingTypeLabel); + EmitLoadManagedValue(pslILEmit); + EmitLoadNativeValue(pslILEmit); + pslILEmit->EmitLDC(0); + pslILEmit->EmitCALL(METHOD__MARSHAL__STRUCTURE_TO_PTR, 3, 0); + } pslILEmit->EmitLabel(pNullRefLabel); } @@ -2360,6 +2433,9 @@ void ILBlittablePtrMarshaler::EmitConvertContentsNativeToCLR(ILCodeStream* pslIL UINT uNativeSize = m_pargs->m_pMT->GetNativeSize(); int fieldDef = pslILEmit->GetToken(CoreLibBinder::GetField(FIELD__RAW_DATA__DATA)); + ILCodeLabel* isNotMatchingTypeLabel = pslILEmit->NewCodeLabel(); + bool emittedTypeCheck = EmitExactTypeCheck(pslILEmit, isNotMatchingTypeLabel); + EmitLoadManagedValue(pslILEmit); pslILEmit->EmitBRFALSE(pNullRefLabel); @@ -2371,12 +2447,26 @@ void ILBlittablePtrMarshaler::EmitConvertContentsNativeToCLR(ILCodeStream* pslIL pslILEmit->EmitLDC(uNativeSize); // size pslILEmit->EmitCPBLK(); + + if (emittedTypeCheck) + { + pslILEmit->EmitBR(pNullRefLabel); + + pslILEmit->EmitLabel(isNotMatchingTypeLabel); + EmitLoadNativeValue(pslILEmit); + EmitLoadManagedValue(pslILEmit); + pslILEmit->EmitCALL(METHOD__MARSHAL__PTR_TO_STRUCTURE, 2, 0); + } + pslILEmit->EmitLabel(pNullRefLabel); } bool ILBlittablePtrMarshaler::CanMarshalViaPinning() { - return IsCLRToNative(m_dwMarshalFlags) && !IsByref(m_dwMarshalFlags) && !IsFieldMarshal(m_dwMarshalFlags); + return IsCLRToNative(m_dwMarshalFlags) && + !IsByref(m_dwMarshalFlags) && + !IsFieldMarshal(m_dwMarshalFlags) && + m_pargs->m_pMT->IsSealed(); // We can't marshal via pinning if we might need to marshal differently at runtime. } void ILBlittablePtrMarshaler::EmitMarshalViaPinning(ILCodeStream* pslILEmit) diff --git a/src/coreclr/src/vm/ilmarshalers.h b/src/coreclr/src/vm/ilmarshalers.h index c08f4f3efc49be..21b552eb76f780 100644 --- a/src/coreclr/src/vm/ilmarshalers.h +++ b/src/coreclr/src/vm/ilmarshalers.h @@ -2915,6 +2915,7 @@ class ILLayoutClassPtrMarshalerBase : public ILMarshaler bool NeedsClearNative() override; void EmitClearNative(ILCodeStream* pslILEmit) override; void EmitClearNativeTemp(ILCodeStream* pslILEmit) override; + bool EmitExactTypeCheck(ILCodeStream* pslILEmit, ILCodeLabel* isNotMatchingTypeLabel); }; class ILLayoutClassPtrMarshaler : public ILLayoutClassPtrMarshalerBase diff --git a/src/coreclr/src/vm/metasig.h b/src/coreclr/src/vm/metasig.h index 179dcd206378e5..4915f7d6b0fa4f 100644 --- a/src/coreclr/src/vm/metasig.h +++ b/src/coreclr/src/vm/metasig.h @@ -604,6 +604,10 @@ DEFINE_METASIG_T(SM(Array_Int_Array_Int_Int_RetVoid, C(ARRAY) i C(ARRAY) i i, v) DEFINE_METASIG_T(SM(Array_Int_Obj_RetVoid, C(ARRAY) i j, v)) DEFINE_METASIG_T(SM(Array_Int_PtrVoid_RetRefObj, C(ARRAY) i P(v), r(j))) +DEFINE_METASIG(SM(Obj_IntPtr_Bool_RetVoid, j I F, v)) +DEFINE_METASIG(SM(IntPtr_Obj_RetVoid, I j, v)) +DEFINE_METASIG_T(SM(IntPtr_Type_RetVoid, I C(TYPE), v)) + // Undefine macros in case we include the file again in the compilation unit #undef DEFINE_METASIG diff --git a/src/coreclr/src/vm/mlinfo.cpp b/src/coreclr/src/vm/mlinfo.cpp index 36b07edcfce33c..c6f5a207b62a10 100644 --- a/src/coreclr/src/vm/mlinfo.cpp +++ b/src/coreclr/src/vm/mlinfo.cpp @@ -1231,6 +1231,8 @@ MarshalInfo::MarshalInfo(Module* pModule, m_pMT = NULL; m_pMD = pMD; m_onInstanceMethod = onInstanceMethod; + // For backward compatibility reasons, some marshalers imply [In, Out] behavior when marked as [Out]. + BOOL outImpliesInOut = FALSE; #ifdef FEATURE_COMINTEROP m_fDispItf = FALSE; @@ -2007,6 +2009,7 @@ MarshalInfo::MarshalInfo(Module* pModule, } m_type = IsFieldScenario() ? MARSHAL_TYPE_BLITTABLE_LAYOUTCLASS : MARSHAL_TYPE_BLITTABLEPTR; m_args.m_pMT = m_pMT; + outImpliesInOut = TRUE; } else if (m_pMT->HasLayout()) { @@ -2533,6 +2536,12 @@ MarshalInfo::MarshalInfo(Module* pModule, } } + + // For marshalers that expect [Out] behavior to match [In, Out] behavior, update that here. + if (m_out && outImpliesInOut) + { + m_in = TRUE; + } } lReallyExit: diff --git a/src/tests/Interop/LayoutClass/LayoutClassNative.cpp b/src/tests/Interop/LayoutClass/LayoutClassNative.cpp index 0bf0af967f0637..808bc8ffc95a89 100644 --- a/src/tests/Interop/LayoutClass/LayoutClassNative.cpp +++ b/src/tests/Interop/LayoutClass/LayoutClassNative.cpp @@ -5,7 +5,16 @@ #include typedef void *voidPtr; - + +struct EmptyBase +{ +}; + +struct DerivedSeqClass : public EmptyBase +{ + int a; +}; + struct SeqClass { int a; @@ -14,7 +23,7 @@ struct SeqClass }; struct ExpClass -{ +{ int a; int padding; //padding needs to be added here as we have added 8 byte offset. union @@ -46,6 +55,17 @@ DLL_EXPORT BOOL STDMETHODCALLTYPE SimpleSeqLayoutClassByRef(SeqClass* p) return TRUE; } +extern "C" +DLL_EXPORT BOOL STDMETHODCALLTYPE DerivedSeqLayoutClassByRef(EmptyBase* p, int expected) +{ + if(((DerivedSeqClass*)p)->a != expected) + { + printf("FAIL: p->a=%d, expected %d\n", ((DerivedSeqClass*)p)->a, expected); + return FALSE; + } + return TRUE; +} + extern "C" DLL_EXPORT BOOL STDMETHODCALLTYPE SimpleExpLayoutClassByRef(ExpClass* p) { diff --git a/src/tests/Interop/LayoutClass/LayoutClassTest.cs b/src/tests/Interop/LayoutClass/LayoutClassTest.cs index 32ecfcb3548f93..a3cf72c4c432e5 100644 --- a/src/tests/Interop/LayoutClass/LayoutClassTest.cs +++ b/src/tests/Interop/LayoutClass/LayoutClassTest.cs @@ -8,7 +8,23 @@ namespace PInvokeTests { [StructLayout(LayoutKind.Sequential)] - public class SeqClass + public class EmptyBase + { + } + + [StructLayout(LayoutKind.Sequential)] + public class SeqDerivedClass : EmptyBase + { + public int a; + + public SeqDerivedClass(int _a) + { + a = _a; + } + } + + [StructLayout(LayoutKind.Sequential)] + public sealed class SeqClass { public int a; public bool b; @@ -78,6 +94,17 @@ public Blittable(int _a) } } + [StructLayout(LayoutKind.Sequential)] + public sealed class SealedBlittable + { + public int a; + + public SealedBlittable(int _a) + { + a = _a; + } + } + public struct NestedLayout { public SeqClass value; @@ -99,6 +126,9 @@ class StructureTests [DllImport("LayoutClassNative")] private static extern bool SimpleSeqLayoutClassByRef(SeqClass p); + [DllImport("LayoutClassNative")] + private static extern bool DerivedSeqLayoutClassByRef(EmptyBase p, int expected); + [DllImport("LayoutClassNative")] private static extern bool SimpleExpLayoutClassByRef(ExpClass p); @@ -108,6 +138,12 @@ class StructureTests [DllImport("LayoutClassNative")] private static extern bool SimpleBlittableSeqLayoutClassByOutAttr([Out] Blittable p); + [DllImport("LayoutClassNative")] + private static extern bool SimpleBlittableSeqLayoutClassByRef(SealedBlittable p); + + [DllImport("LayoutClassNative")] + private static extern bool SimpleBlittableSeqLayoutClassByOutAttr([Out] SealedBlittable p); + [DllImport("LayoutClassNative")] private static extern bool SimpleNestedLayoutClassByValue(NestedLayout p); @@ -123,6 +159,15 @@ public static void SequentialClass() Assert.IsTrue(SimpleSeqLayoutClassByRef(p)); } + public static void DerivedClassWithEmptyBase() + { + Console.WriteLine($"Running {nameof(DerivedClassWithEmptyBase)}..."); + + string s = "before"; + var p = new SeqDerivedClass(42); + Assert.IsTrue(DerivedSeqLayoutClassByRef(p, 42)); + } + public static void ExplicitClass() { Console.WriteLine($"Running {nameof(ExplicitClass)}..."); @@ -150,6 +195,25 @@ public static void BlittableClassByOutAttr() Assert.AreEqual(expected, p.a); } + public static void SealedBlittableClass() + { + Console.WriteLine($"Running {nameof(SealedBlittableClass)}..."); + + SealedBlittable p = new SealedBlittable(10); + Assert.IsTrue(SimpleBlittableSeqLayoutClassByRef(p)); + } + + public static void SealedBlittableClassByOutAttr() + { + Console.WriteLine($"Running {nameof(SealedBlittableClassByOutAttr)}..."); + + int a = 10; + int expected = a + 1; + SealedBlittable p = new SealedBlittable(a); + Assert.IsTrue(SimpleBlittableSeqLayoutClassByOutAttr(p)); + Assert.AreEqual(expected, p.a); + } + public static void NestedLayoutClass() { Console.WriteLine($"Running {nameof(NestedLayoutClass)}..."); @@ -175,9 +239,12 @@ public static int Main(string[] argv) try { SequentialClass(); + DerivedClassWithEmptyBase(); ExplicitClass(); BlittableClass(); + SealedBlittableClass(); BlittableClassByOutAttr(); + SealedBlittableClassByOutAttr(); NestedLayoutClass(); RecursiveNativeLayout(); } From e8d54a1070a81e2d0368404de5a12c88a5929611 Mon Sep 17 00:00:00 2001 From: Jeremy Koritzinsky Date: Tue, 23 Mar 2021 16:35:51 -0700 Subject: [PATCH 2/5] Allocate the correct amount of space for the native data based on the runtime type. --- src/coreclr/src/vm/corelib.h | 1 + src/coreclr/src/vm/ilmarshalers.cpp | 35 +++++++++++++++++++++++++++-- 2 files changed, 34 insertions(+), 2 deletions(-) diff --git a/src/coreclr/src/vm/corelib.h b/src/coreclr/src/vm/corelib.h index 9fada354c4ba86..ad892439138409 100644 --- a/src/coreclr/src/vm/corelib.h +++ b/src/coreclr/src/vm/corelib.h @@ -491,6 +491,7 @@ DEFINE_FIELD(MARSHAL, SYSTEM_MAX_DBCS_CHAR_SIZE, SystemMax DEFINE_METHOD(MARSHAL, STRUCTURE_TO_PTR, StructureToPtr, SM_Obj_IntPtr_Bool_RetVoid) DEFINE_METHOD(MARSHAL, PTR_TO_STRUCTURE, PtrToStructure, SM_IntPtr_Obj_RetVoid) DEFINE_METHOD(MARSHAL, DESTROY_STRUCTURE, DestroyStructure, SM_IntPtr_Type_RetVoid) +DEFINE_METHOD(MARSHAL, SIZEOF_TYPE, SizeOf, SM_Type_RetInt) DEFINE_CLASS(NATIVELIBRARY, Interop, NativeLibrary) DEFINE_METHOD(NATIVELIBRARY, LOADLIBRARYCALLBACKSTUB, LoadLibraryCallbackStub, SM_Str_AssemblyBase_Bool_UInt_RetIntPtr) diff --git a/src/coreclr/src/vm/ilmarshalers.cpp b/src/coreclr/src/vm/ilmarshalers.cpp index e435270f203b06..6fbaf1510dab50 100644 --- a/src/coreclr/src/vm/ilmarshalers.cpp +++ b/src/coreclr/src/vm/ilmarshalers.cpp @@ -2149,14 +2149,30 @@ void ILLayoutClassPtrMarshalerBase::EmitConvertSpaceCLRToNative(ILCodeStream* ps EmitLoadManagedValue(pslILEmit); pslILEmit->EmitBRFALSE(pNullRefLabel); + ILCodeLabel* pTypeMismatchedLabel = pslILEmit->NewCodeLabel(); + bool emittedTypeCheck = EmitExactTypeCheck(pslILEmit, pTypeMismatchedLabel); + DWORD sizeLocal = pslILEmit->NewLocal(LocalDesc(ELEMENT_TYPE_I4)); + pslILEmit->EmitLDC(uNativeSize); + if (emittedTypeCheck) + { + ILCodeLabel* pHaveSizeLabel = pslILEmit->NewCodeLabel(); + pslILEmit->EmitBR(pHaveSizeLabel); + pslILEmit->EmitLabel(pTypeMismatchedLabel); + EmitLoadManagedValue(pslILEmit); + pslILEmit->EmitCALL(METHOD__OBJECT__GET_TYPE, 1, 1); + pslILEmit->EmitCALL(METHOD__MARSHAL__SIZEOF_TYPE, 1, 1); + pslILEmit->EmitLabel(pHaveSizeLabel); + } + pslILEmit->EmitSTLOC(sizeLocal); + pslILEmit->EmitLDLOC(sizeLocal); pslILEmit->EmitCALL(METHOD__MARSHAL__ALLOC_CO_TASK_MEM, 1, 1); pslILEmit->EmitDUP(); // for INITBLK EmitStoreNativeValue(pslILEmit); // initialize local block we just allocated pslILEmit->EmitLDC(0); - pslILEmit->EmitLDC(uNativeSize); + pslILEmit->EmitLDLOC(sizeLocal); pslILEmit->EmitINITBLK(); pslILEmit->EmitLabel(pNullRefLabel); @@ -2180,15 +2196,30 @@ void ILLayoutClassPtrMarshalerBase::EmitConvertSpaceCLRToNativeTemp(ILCodeStream EmitLoadManagedValue(pslILEmit); pslILEmit->EmitBRFALSE(pNullRefLabel); + ILCodeLabel* pTypeMismatchedLabel = pslILEmit->NewCodeLabel(); + bool emittedTypeCheck = EmitExactTypeCheck(pslILEmit, pTypeMismatchedLabel); + DWORD sizeLocal = pslILEmit->NewLocal(LocalDesc(ELEMENT_TYPE_I4)); pslILEmit->EmitLDC(uNativeSize); + if (emittedTypeCheck) + { + ILCodeLabel* pHaveSizeLabel = pslILEmit->NewCodeLabel(); + pslILEmit->EmitBR(pHaveSizeLabel); + pslILEmit->EmitLabel(pTypeMismatchedLabel); + EmitLoadManagedValue(pslILEmit); + pslILEmit->EmitCALL(METHOD__OBJECT__GET_TYPE, 1, 1); + pslILEmit->EmitCALL(METHOD__MARSHAL__SIZEOF_TYPE, 1, 1); + pslILEmit->EmitLabel(pHaveSizeLabel); + } + pslILEmit->EmitSTLOC(sizeLocal); + pslILEmit->EmitLDLOC(sizeLocal); pslILEmit->EmitLOCALLOC(); pslILEmit->EmitDUP(); // for INITBLK EmitStoreNativeValue(pslILEmit); // initialize local block we just allocated pslILEmit->EmitLDC(0); - pslILEmit->EmitLDC(uNativeSize); + pslILEmit->EmitLDLOC(sizeLocal); pslILEmit->EmitINITBLK(); pslILEmit->EmitLabel(pNullRefLabel); From a3785ea72ac497b85a3558ace44990a83c3fbec4 Mon Sep 17 00:00:00 2001 From: Jeremy Koritzinsky Date: Wed, 24 Mar 2021 10:20:50 -0700 Subject: [PATCH 3/5] Apply suggestions from code review --- src/coreclr/src/vm/ilmarshalers.cpp | 4 +++- src/coreclr/src/vm/mlinfo.cpp | 2 +- 2 files changed, 4 insertions(+), 2 deletions(-) diff --git a/src/coreclr/src/vm/ilmarshalers.cpp b/src/coreclr/src/vm/ilmarshalers.cpp index 6fbaf1510dab50..ea81d334a4fbb1 100644 --- a/src/coreclr/src/vm/ilmarshalers.cpp +++ b/src/coreclr/src/vm/ilmarshalers.cpp @@ -2297,6 +2297,8 @@ void ILLayoutClassPtrMarshalerBase::EmitClearNativeTemp(ILCodeStream* pslILEmit) bool ILLayoutClassPtrMarshalerBase::EmitExactTypeCheck(ILCodeStream* pslILEmit, ILCodeLabel* isNotMatchingTypeLabel) { + STANDARD_VM_CONTRACT; + if (m_pargs->m_pMT->IsSealed()) { // If the provided type cannot be derived from, then we don't need to emit the type check. @@ -2497,7 +2499,7 @@ bool ILBlittablePtrMarshaler::CanMarshalViaPinning() return IsCLRToNative(m_dwMarshalFlags) && !IsByref(m_dwMarshalFlags) && !IsFieldMarshal(m_dwMarshalFlags) && - m_pargs->m_pMT->IsSealed(); // We can't marshal via pinning if we might need to marshal differently at runtime. + m_pargs->m_pMT->IsSealed(); // We can't marshal via pinning if we might need to marshal differently at runtime. See calls to EmitExactTypeCheck where we check the runtime type of the object being marshalled. } void ILBlittablePtrMarshaler::EmitMarshalViaPinning(ILCodeStream* pslILEmit) diff --git a/src/coreclr/src/vm/mlinfo.cpp b/src/coreclr/src/vm/mlinfo.cpp index c6f5a207b62a10..9335fa084b136b 100644 --- a/src/coreclr/src/vm/mlinfo.cpp +++ b/src/coreclr/src/vm/mlinfo.cpp @@ -1231,7 +1231,7 @@ MarshalInfo::MarshalInfo(Module* pModule, m_pMT = NULL; m_pMD = pMD; m_onInstanceMethod = onInstanceMethod; - // For backward compatibility reasons, some marshalers imply [In, Out] behavior when marked as [Out]. + // [Compat] For backward compatibility reasons, some marshalers imply [In, Out] behavior when marked as [Out]. BOOL outImpliesInOut = FALSE; #ifdef FEATURE_COMINTEROP From ad25cc448d089b6ab0cb4bf2878a7bed0d2c6354 Mon Sep 17 00:00:00 2001 From: Elinor Fung Date: Fri, 2 Apr 2021 16:09:37 -0700 Subject: [PATCH 4/5] Keep blittable layout class In/Out by default (#50655) --- src/coreclr/src/vm/mlinfo.cpp | 24 +++---- .../Interop/LayoutClass/LayoutClassNative.cpp | 10 +-- .../Interop/LayoutClass/LayoutClassTest.cs | 68 ++++++++++++++----- 3 files changed, 63 insertions(+), 39 deletions(-) diff --git a/src/coreclr/src/vm/mlinfo.cpp b/src/coreclr/src/vm/mlinfo.cpp index 9335fa084b136b..3606df2c84874a 100644 --- a/src/coreclr/src/vm/mlinfo.cpp +++ b/src/coreclr/src/vm/mlinfo.cpp @@ -1231,8 +1231,8 @@ MarshalInfo::MarshalInfo(Module* pModule, m_pMT = NULL; m_pMD = pMD; m_onInstanceMethod = onInstanceMethod; - // [Compat] For backward compatibility reasons, some marshalers imply [In, Out] behavior when marked as [Out]. - BOOL outImpliesInOut = FALSE; + // [Compat] For backward compatibility reasons, some marshalers imply [In, Out] behavior when marked as [In], [Out], or not marked with either. + BOOL byValAlwaysInOut = FALSE; #ifdef FEATURE_COMINTEROP m_fDispItf = FALSE; @@ -2009,7 +2009,7 @@ MarshalInfo::MarshalInfo(Module* pModule, } m_type = IsFieldScenario() ? MARSHAL_TYPE_BLITTABLE_LAYOUTCLASS : MARSHAL_TYPE_BLITTABLEPTR; m_args.m_pMT = m_pMT; - outImpliesInOut = TRUE; + byValAlwaysInOut = TRUE; } else if (m_pMT->HasLayout()) { @@ -2517,10 +2517,16 @@ MarshalInfo::MarshalInfo(Module* pModule, } } - // If neither IN nor OUT are true, this signals the URT to use the default - // rules. - if (!m_in && !m_out) + if (!m_byref && byValAlwaysInOut) { + // Some marshalers expect [In, Out] behavior with [In], [Out], or no directional attributes. + m_in = TRUE; + m_out = TRUE; + } + else if (!m_in && !m_out) + { + // If neither IN nor OUT are true, this signals the URT to use the default + // rules. if (m_byref || (mtype == ELEMENT_TYPE_CLASS && !(sig.IsStringType(pModule, pTypeContext)) @@ -2536,12 +2542,6 @@ MarshalInfo::MarshalInfo(Module* pModule, } } - - // For marshalers that expect [Out] behavior to match [In, Out] behavior, update that here. - if (m_out && outImpliesInOut) - { - m_in = TRUE; - } } lReallyExit: diff --git a/src/tests/Interop/LayoutClass/LayoutClassNative.cpp b/src/tests/Interop/LayoutClass/LayoutClassNative.cpp index 808bc8ffc95a89..59c4645afba321 100644 --- a/src/tests/Interop/LayoutClass/LayoutClassNative.cpp +++ b/src/tests/Interop/LayoutClass/LayoutClassNative.cpp @@ -78,21 +78,13 @@ DLL_EXPORT BOOL STDMETHODCALLTYPE SimpleExpLayoutClassByRef(ExpClass* p) } extern "C" -DLL_EXPORT BOOL STDMETHODCALLTYPE SimpleBlittableSeqLayoutClassByRef(BlittableClass* p) +DLL_EXPORT BOOL STDMETHODCALLTYPE SimpleBlittableSeqLayoutClass_UpdateField(BlittableClass* p) { if(p->a != 10) { printf("FAIL: p->a=%d\n", p->a); return FALSE; } - return TRUE; -} - -extern "C" -DLL_EXPORT BOOL STDMETHODCALLTYPE SimpleBlittableSeqLayoutClassByOutAttr(BlittableClass* p) -{ - if(!SimpleBlittableSeqLayoutClassByRef(p)) - return FALSE; p->a++; return TRUE; diff --git a/src/tests/Interop/LayoutClass/LayoutClassTest.cs b/src/tests/Interop/LayoutClass/LayoutClassTest.cs index a3cf72c4c432e5..81048a22b332d8 100644 --- a/src/tests/Interop/LayoutClass/LayoutClassTest.cs +++ b/src/tests/Interop/LayoutClass/LayoutClassTest.cs @@ -123,6 +123,8 @@ public struct RecursiveTestStruct class StructureTests { + private const string SimpleBlittableSeqLayoutClass_UpdateField = nameof(SimpleBlittableSeqLayoutClass_UpdateField); + [DllImport("LayoutClassNative")] private static extern bool SimpleSeqLayoutClassByRef(SeqClass p); @@ -132,17 +134,23 @@ class StructureTests [DllImport("LayoutClassNative")] private static extern bool SimpleExpLayoutClassByRef(ExpClass p); - [DllImport("LayoutClassNative")] + [DllImport("LayoutClassNative", EntryPoint = SimpleBlittableSeqLayoutClass_UpdateField)] private static extern bool SimpleBlittableSeqLayoutClassByRef(Blittable p); - [DllImport("LayoutClassNative")] + [DllImport("LayoutClassNative", EntryPoint = SimpleBlittableSeqLayoutClass_UpdateField)] + private static extern bool SimpleBlittableSeqLayoutClassByInAttr([In] Blittable p); + + [DllImport("LayoutClassNative", EntryPoint = SimpleBlittableSeqLayoutClass_UpdateField)] private static extern bool SimpleBlittableSeqLayoutClassByOutAttr([Out] Blittable p); - [DllImport("LayoutClassNative")] - private static extern bool SimpleBlittableSeqLayoutClassByRef(SealedBlittable p); + [DllImport("LayoutClassNative", EntryPoint = SimpleBlittableSeqLayoutClass_UpdateField)] + private static extern bool SealedBlittableSeqLayoutClassByRef(SealedBlittable p); - [DllImport("LayoutClassNative")] - private static extern bool SimpleBlittableSeqLayoutClassByOutAttr([Out] SealedBlittable p); + [DllImport("LayoutClassNative", EntryPoint = SimpleBlittableSeqLayoutClass_UpdateField)] + private static extern bool SealedBlittableSeqLayoutClassByInAttr([In] SealedBlittable p); + + [DllImport("LayoutClassNative", EntryPoint = SimpleBlittableSeqLayoutClass_UpdateField)] + private static extern bool SealedBlittableSeqLayoutClassByOutAttr([Out] SealedBlittable p); [DllImport("LayoutClassNative")] private static extern bool SimpleNestedLayoutClassByValue(NestedLayout p); @@ -176,42 +184,64 @@ public static void ExplicitClass() Assert.IsTrue(SimpleExpLayoutClassByRef(p)); } + private static void ValidateBlittableClassInOut(Func pinvoke) + { + int a = 10; + int expected = a + 1; + Blittable p = new Blittable(a); + Assert.IsTrue(pinvoke(p)); + Assert.AreEqual(expected, p.a); + } + public static void BlittableClass() { + // [Compat] Marshalled with [In, Out] behaviour by default Console.WriteLine($"Running {nameof(BlittableClass)}..."); + ValidateBlittableClassInOut(SimpleBlittableSeqLayoutClassByRef); + } - Blittable p = new Blittable(10); - Assert.IsTrue(SimpleBlittableSeqLayoutClassByRef(p)); + public static void BlittableClassByInAttr() + { + // [Compat] Marshalled with [In, Out] behaviour even when only [In] is specified + Console.WriteLine($"Running {nameof(BlittableClassByInAttr)}..."); + ValidateBlittableClassInOut(SimpleBlittableSeqLayoutClassByInAttr); } public static void BlittableClassByOutAttr() { + // [Compat] Marshalled with [In, Out] behaviour even when only [Out] is specified Console.WriteLine($"Running {nameof(BlittableClassByOutAttr)}..."); + ValidateBlittableClassInOut(SimpleBlittableSeqLayoutClassByOutAttr); + } + private static void ValidateSealedBlittableClassInOut(Func pinvoke) + { int a = 10; int expected = a + 1; - Blittable p = new Blittable(a); - Assert.IsTrue(SimpleBlittableSeqLayoutClassByOutAttr(p)); + SealedBlittable p = new SealedBlittable(a); + Assert.IsTrue(pinvoke(p)); Assert.AreEqual(expected, p.a); } public static void SealedBlittableClass() { + // [Compat] Marshalled with [In, Out] behaviour by default Console.WriteLine($"Running {nameof(SealedBlittableClass)}..."); + ValidateSealedBlittableClassInOut(SealedBlittableSeqLayoutClassByRef); + } - SealedBlittable p = new SealedBlittable(10); - Assert.IsTrue(SimpleBlittableSeqLayoutClassByRef(p)); + public static void SealedBlittableClassByInAttr() + { + // [Compat] Marshalled with [In, Out] behaviour even when only [In] is specified + Console.WriteLine($"Running {nameof(SealedBlittableClassByOutAttr)}..."); + ValidateSealedBlittableClassInOut(SealedBlittableSeqLayoutClassByInAttr); } public static void SealedBlittableClassByOutAttr() { + // [Compat] Marshalled with [In, Out] behaviour even when only [Out] is specified Console.WriteLine($"Running {nameof(SealedBlittableClassByOutAttr)}..."); - - int a = 10; - int expected = a + 1; - SealedBlittable p = new SealedBlittable(a); - Assert.IsTrue(SimpleBlittableSeqLayoutClassByOutAttr(p)); - Assert.AreEqual(expected, p.a); + ValidateSealedBlittableClassInOut(SealedBlittableSeqLayoutClassByOutAttr); } public static void NestedLayoutClass() @@ -243,6 +273,8 @@ public static int Main(string[] argv) ExplicitClass(); BlittableClass(); SealedBlittableClass(); + BlittableClassByInAttr(); + SealedBlittableClassByInAttr(); BlittableClassByOutAttr(); SealedBlittableClassByOutAttr(); NestedLayoutClass(); From 619e8e1d5cdcdfd0e03f1add82032e4ad5d2a7ce Mon Sep 17 00:00:00 2001 From: Jeremy Koritzinsky Date: Mon, 5 Apr 2021 10:05:40 -0700 Subject: [PATCH 5/5] Do the exact type check after the null check. --- src/coreclr/src/vm/ilmarshalers.cpp | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/coreclr/src/vm/ilmarshalers.cpp b/src/coreclr/src/vm/ilmarshalers.cpp index ea81d334a4fbb1..f224d7c477c7a4 100644 --- a/src/coreclr/src/vm/ilmarshalers.cpp +++ b/src/coreclr/src/vm/ilmarshalers.cpp @@ -2466,12 +2466,12 @@ void ILBlittablePtrMarshaler::EmitConvertContentsNativeToCLR(ILCodeStream* pslIL UINT uNativeSize = m_pargs->m_pMT->GetNativeSize(); int fieldDef = pslILEmit->GetToken(CoreLibBinder::GetField(FIELD__RAW_DATA__DATA)); - ILCodeLabel* isNotMatchingTypeLabel = pslILEmit->NewCodeLabel(); - bool emittedTypeCheck = EmitExactTypeCheck(pslILEmit, isNotMatchingTypeLabel); - EmitLoadManagedValue(pslILEmit); pslILEmit->EmitBRFALSE(pNullRefLabel); + ILCodeLabel* isNotMatchingTypeLabel = pslILEmit->NewCodeLabel(); + bool emittedTypeCheck = EmitExactTypeCheck(pslILEmit, isNotMatchingTypeLabel); + EmitLoadManagedValue(pslILEmit); pslILEmit->EmitLDFLDA(fieldDef); // dest