Keep blittable layout class In/Out by default (dotnet#50655)

elinor-fung · elinor-fung · commit ad25cc448d08 · 2021-04-02T16:24:07.000-07:00
diff --git 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
@@ -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
@@ -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<Blittable, bool> 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<SealedBlittable, bool> 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();

Original file line number	Diff line number	Diff line change
`@@ -1231,8 +1231,8 @@ MarshalInfo::MarshalInfo(Module* pModule,`
`1231`	`1231`	`m_pMT = NULL;`
`1232`	`1232`	`m_pMD = pMD;`
`1233`	`1233`	`m_onInstanceMethod = onInstanceMethod;`
`1234`		`- // [Compat] For backward compatibility reasons, some marshalers imply [In, Out] behavior when marked as [Out].`
`1235`		`- BOOL outImpliesInOut = FALSE;`
	`1234`	`+ // [Compat] For backward compatibility reasons, some marshalers imply [In, Out] behavior when marked as [In], [Out], or not marked with either.`
	`1235`	`+ BOOL byValAlwaysInOut = FALSE;`
`1236`	`1236`
`1237`	`1237`	`#ifdef FEATURE_COMINTEROP`
`1238`	`1238`	`m_fDispItf = FALSE;`
`@@ -2009,7 +2009,7 @@ MarshalInfo::MarshalInfo(Module* pModule,`
`2009`	`2009`	`}`
`2010`	`2010`	`m_type = IsFieldScenario() ? MARSHAL_TYPE_BLITTABLE_LAYOUTCLASS : MARSHAL_TYPE_BLITTABLEPTR;`
`2011`	`2011`	`m_args.m_pMT = m_pMT;`
`2012`		`- outImpliesInOut = TRUE;`
	`2012`	`+ byValAlwaysInOut = TRUE;`
`2013`	`2013`	`}`
`2014`	`2014`	`else if (m_pMT->HasLayout())`
`2015`	`2015`	`{`
`@@ -2517,10 +2517,16 @@ MarshalInfo::MarshalInfo(Module* pModule,`
`2517`	`2517`	`}`
`2518`	`2518`	`}`
`2519`	`2519`
`2520`		`- // If neither IN nor OUT are true, this signals the URT to use the default`
`2521`		`- // rules.`
`2522`		`- if (!m_in && !m_out)`
	`2520`	`+ if (!m_byref && byValAlwaysInOut)`
`2523`	`2521`	`{`
	`2522`	`+ // Some marshalers expect [In, Out] behavior with [In], [Out], or no directional attributes.`
	`2523`	`+ m_in = TRUE;`
	`2524`	`+ m_out = TRUE;`
	`2525`	`+ }`
	`2526`	`+ else if (!m_in && !m_out)`
	`2527`	`+ {`
	`2528`	`+ // If neither IN nor OUT are true, this signals the URT to use the default`
	`2529`	`+ // rules.`
`2524`	`2530`	`if (m_byref \|\|`
`2525`	`2531`	`(mtype == ELEMENT_TYPE_CLASS`
`2526`	`2532`	`&& !(sig.IsStringType(pModule, pTypeContext))`
`@@ -2536,12 +2542,6 @@ MarshalInfo::MarshalInfo(Module* pModule,`
`2536`	`2542`	`}`
`2537`	`2543`
`2538`	`2544`	`}`
`2539`		`-`
`2540`		`- // For marshalers that expect [Out] behavior to match [In, Out] behavior, update that here.`
`2541`		`- if (m_out && outImpliesInOut)`
`2542`		`- {`
`2543`		`- m_in = TRUE;`
`2544`		`- }`
`2545`	`2545`	`}`
`2546`	`2546`
`2547`	`2547`	`lReallyExit:`
Original file line number	Diff line number	Diff line change
`@@ -78,21 +78,13 @@ DLL_EXPORT BOOL STDMETHODCALLTYPE SimpleExpLayoutClassByRef(ExpClass* p)`
`78`	`78`	`}`
`79`	`79`
`80`	`80`	`extern "C"`
`81`		`-DLL_EXPORT BOOL STDMETHODCALLTYPE SimpleBlittableSeqLayoutClassByRef(BlittableClass* p)`
	`81`	`+DLL_EXPORT BOOL STDMETHODCALLTYPE SimpleBlittableSeqLayoutClass_UpdateField(BlittableClass* p)`
`82`	`82`	`{`
`83`	`83`	`if(p->a != 10)`
`84`	`84`	`{`
`85`	`85`	`printf("FAIL: p->a=%d\n", p->a);`
`86`	`86`	`return FALSE;`
`87`	`87`	`}`
`88`		`- return TRUE;`
`89`		`-}`
`90`		`-`
`91`		`-extern "C"`
`92`		`-DLL_EXPORT BOOL STDMETHODCALLTYPE SimpleBlittableSeqLayoutClassByOutAttr(BlittableClass* p)`
`93`		`-{`
`94`		`- if(!SimpleBlittableSeqLayoutClassByRef(p))`
`95`		`- return FALSE;`
`96`	`88`
`97`	`89`	`p->a++;`
`98`	`90`	`return TRUE;`