From 17eb82fbbc673b9c476c5ff93eccd9273f000dc5 Mon Sep 17 00:00:00 2001 From: Elinor Fung Date: Fri, 2 Apr 2021 10:42:40 -0700 Subject: [PATCH 1/2] Keep blittable layout class In/Out by default --- src/coreclr/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/vm/mlinfo.cpp b/src/coreclr/vm/mlinfo.cpp index a6f8852aa3670a..1e28e120f5bade 100644 --- a/src/coreclr/vm/mlinfo.cpp +++ b/src/coreclr/vm/mlinfo.cpp @@ -1133,8 +1133,8 @@ MarshalInfo::MarshalInfo(Module* pModule, CorElementType corElemType = ELEMENT_TYPE_END; m_pMT = NULL; m_pMD = pMD; - // [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 alwaysInOut = FALSE; #ifdef FEATURE_COMINTEROP m_fDispItf = FALSE; @@ -1879,7 +1879,7 @@ MarshalInfo::MarshalInfo(Module* pModule, } m_type = IsFieldScenario() ? MARSHAL_TYPE_BLITTABLE_LAYOUTCLASS : MARSHAL_TYPE_BLITTABLEPTR; m_args.m_pMT = m_pMT; - outImpliesInOut = TRUE; + alwaysInOut = TRUE; } else if (m_pMT->HasLayout()) { @@ -2372,10 +2372,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 (alwaysInOut) { + // 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)) @@ -2390,12 +2396,6 @@ MarshalInfo::MarshalInfo(Module* pModule, m_out = FALSE; } } - - // 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 27dec87ad075feaa565d28c11945b55544ac2bdb Mon Sep 17 00:00:00 2001 From: Elinor Fung Date: Fri, 2 Apr 2021 12:11:43 -0700 Subject: [PATCH 2/2] By-val only --- src/coreclr/vm/mlinfo.cpp | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/coreclr/vm/mlinfo.cpp b/src/coreclr/vm/mlinfo.cpp index 1e28e120f5bade..cc4bcdc710bd4c 100644 --- a/src/coreclr/vm/mlinfo.cpp +++ b/src/coreclr/vm/mlinfo.cpp @@ -1134,7 +1134,7 @@ MarshalInfo::MarshalInfo(Module* pModule, m_pMT = NULL; m_pMD = pMD; // [Compat] For backward compatibility reasons, some marshalers imply [In, Out] behavior when marked as [In], [Out], or not marked with either. - BOOL alwaysInOut = FALSE; + BOOL byValAlwaysInOut = FALSE; #ifdef FEATURE_COMINTEROP m_fDispItf = FALSE; @@ -1879,7 +1879,7 @@ MarshalInfo::MarshalInfo(Module* pModule, } m_type = IsFieldScenario() ? MARSHAL_TYPE_BLITTABLE_LAYOUTCLASS : MARSHAL_TYPE_BLITTABLEPTR; m_args.m_pMT = m_pMT; - alwaysInOut = TRUE; + byValAlwaysInOut = TRUE; } else if (m_pMT->HasLayout()) { @@ -2372,7 +2372,7 @@ MarshalInfo::MarshalInfo(Module* pModule, } } - if (alwaysInOut) + if (!m_byref && byValAlwaysInOut) { // Some marshalers expect [In, Out] behavior with [In], [Out], or no directional attributes. m_in = TRUE;