[release/5.0] Remove the sealed blittable layoutclass correctness fix…

… because of backcompat. (#54244) * [release/5.0] Remove the sealed blittable layoutclass correctness fix because of backcompat. * Add tests.
dotnet · Jul 8, 2021 · 9b6d3ab · 9b6d3ab
1 parent fcc7737
commit 9b6d3ab
Show file tree

Hide file tree

Showing 5 changed files with 37 additions and 18 deletions.
diff --git a/src/coreclr/src/vm/classlayoutinfo.cpp b/src/coreclr/src/vm/classlayoutinfo.cpp
@@ -653,12 +653,8 @@ VOID EEClassLayoutInfo::CollectLayoutFieldMetadataThrowing(
         DEBUGARG(szName)
         );
 
-    // Type is blittable only if parent is also blittable and is not empty.
-    if (isBlittable && fHasNonTrivialParent)
-    {
-        isBlittable = pParentMT->IsBlittable()  // Check parent
-            && (!pParentLayoutInfo || !pParentLayoutInfo->IsZeroSized()); // Ensure non-zero size
-    }
+    // Type is blittable only if parent is also blittable
+    isBlittable = isBlittable && (fHasNonTrivialParent ? pParentMT->IsBlittable() : TRUE);
     pEEClassLayoutInfoOut->SetIsBlittable(isBlittable);
 
     S_UINT32 cbSortArraySize = S_UINT32(cTotalFields) * S_UINT32(sizeof(LayoutRawFieldInfo*));

diff --git a/src/coreclr/src/vm/ilmarshalers.cpp b/src/coreclr/src/vm/ilmarshalers.cpp
@@ -2496,10 +2496,14 @@ void ILBlittablePtrMarshaler::EmitConvertContentsNativeToCLR(ILCodeStream* pslIL
 
 bool ILBlittablePtrMarshaler::CanMarshalViaPinning()
 {
+    // [COMPAT] For correctness, 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.
+    // However, we previously supported pinning non-sealed blittable classes, even though that could result
+    // in some data still being unmarshalled if a subclass is provided. This optimization is incorrect,
+    // but libraries like NAudio have taken a hard dependency on this incorrect behavior, so we need to preserve it.
     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. See calls to EmitExactTypeCheck where we check the runtime type of the object being marshalled.
+        !IsFieldMarshal(m_dwMarshalFlags);
 }
 
 void ILBlittablePtrMarshaler::EmitMarshalViaPinning(ILCodeStream* pslILEmit)

diff --git a/src/coreclr/src/vm/mlinfo.cpp b/src/coreclr/src/vm/mlinfo.cpp
@@ -1231,8 +1231,6 @@ 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 [In], [Out], or not marked with either.
-    BOOL byValAlwaysInOut           = FALSE;
 
 #ifdef FEATURE_COMINTEROP
     m_fDispItf                      = FALSE;
@@ -2009,7 +2007,6 @@ MarshalInfo::MarshalInfo(Module* pModule,
                     }
                     m_type = IsFieldScenario() ? MARSHAL_TYPE_BLITTABLE_LAYOUTCLASS : MARSHAL_TYPE_BLITTABLEPTR;
                     m_args.m_pMT = m_pMT;
-                    byValAlwaysInOut = TRUE;
                 }
                 else if (m_pMT->HasLayout())
                 {
@@ -2517,13 +2514,7 @@ MarshalInfo::MarshalInfo(Module* pModule,
             }
         }
 
-        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 (!m_in && !m_out)
         {
             // If neither IN nor OUT are true, this signals the URT to use the default
             // rules.

diff --git a/src/tests/Interop/LayoutClass/LayoutClassNative.cpp b/src/tests/Interop/LayoutClass/LayoutClassNative.cpp
@@ -96,6 +96,12 @@ DLL_EXPORT BOOL STDMETHODCALLTYPE SimpleNestedLayoutClassByValue(NestedLayoutCla
     return SimpleSeqLayoutClassByRef(&v.str);
 }
 
+extern "C"
+DLL_EXPORT BOOL STDMETHODCALLTYPE PointersEqual(void* ptr, void* ptr2)
+{
+    return ptr == ptr2 ? TRUE : FALSE;
+}
+
 extern "C"
 DLL_EXPORT void __cdecl Invalid(...)
 {

diff --git a/src/tests/Interop/LayoutClass/LayoutClassTest.cs b/src/tests/Interop/LayoutClass/LayoutClassTest.cs
@@ -155,6 +155,12 @@ class StructureTests
         [DllImport("LayoutClassNative")]
         private static extern bool SimpleNestedLayoutClassByValue(NestedLayout p);
 
+        [DllImport("LayoutClassNative")]
+        private static extern bool PointersEqual(SealedBlittable obj, ref int field);
+
+        [DllImport("LayoutClassNative")]
+        private static extern bool PointersEqual(Blittable obj, ref int field);
+
         [DllImport("LayoutClassNative", EntryPoint = "Invalid")]
         private static extern void RecursiveNativeLayoutInvalid(RecursiveTestStruct str);
 
@@ -264,6 +270,20 @@ public static void RecursiveNativeLayout()
             Assert.Throws<TypeLoadException>(() => RecursiveNativeLayoutInvalid(new RecursiveTestStruct()));
         }
 
+        public static void SealedBlittablePinned()
+        {
+            Console.WriteLine($"Running {nameof(SealedBlittablePinned)}...");
+            var blittable = new SealedBlittable(1);
+            Assert.IsTrue(PointersEqual(blittable, ref blittable.a));
+        }
+
+        public static void BlittablePinned()
+        {
+            Console.WriteLine($"Running {nameof(BlittablePinned)}...");
+            var blittable = new Blittable(1);
+            Assert.IsTrue(PointersEqual(blittable, ref blittable.a));
+        }
+
         public static int Main(string[] argv)
         {
             try
@@ -279,6 +299,8 @@ public static int Main(string[] argv)
                 SealedBlittableClassByOutAttr();
                 NestedLayoutClass();
                 RecursiveNativeLayout();
+                SealedBlittablePinned();
+                BlittablePinned();
             }
             catch (Exception e)
             {