Skip to content

Commit

Permalink
Remove the sealed blittable layoutclass correctness fix because of …
Browse files Browse the repository at this point in the history
…backcompat.

Fixes dotnet#54132

Fixes dotnet#53542
  • Loading branch information
jkoritzinsky committed Jun 15, 2021
1 parent ddae375 commit 3848a6d
Show file tree
Hide file tree
Showing 5 changed files with 37 additions and 18 deletions.
8 changes: 2 additions & 6 deletions src/coreclr/vm/classlayoutinfo.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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*));
Expand Down
8 changes: 6 additions & 2 deletions src/coreclr/vm/ilmarshalers.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down
11 changes: 1 addition & 10 deletions src/coreclr/vm/mlinfo.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -1138,8 +1138,6 @@ 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 [In], [Out], or not marked with either.
BOOL byValAlwaysInOut = FALSE;

#ifdef FEATURE_COMINTEROP
m_fDispItf = FALSE;
Expand Down Expand Up @@ -1884,7 +1882,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())
{
Expand Down Expand Up @@ -2377,13 +2374,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.
Expand Down
6 changes: 6 additions & 0 deletions src/tests/Interop/LayoutClass/LayoutClassNative.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -108,6 +108,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(...)
{
Expand Down
22 changes: 22 additions & 0 deletions src/tests/Interop/LayoutClass/LayoutClassTest.cs
Original file line number Diff line number Diff line change
Expand Up @@ -174,6 +174,12 @@ class StructureTests
[DllImport("LayoutClassNative", EntryPoint = SimpleBlittableSeqLayoutClass_UpdateField)]
private static extern bool SealedBlittableSeqLayoutClassByOutAttr([Out] SealedBlittable 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")]
private static extern bool SimpleNestedLayoutClassByValue(NestedLayout p);

Expand Down Expand Up @@ -280,6 +286,20 @@ public static void SealedBlittableClassByOutAttr()
ValidateSealedBlittableClassInOut(SealedBlittableSeqLayoutClassByOutAttr);
}

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 void NestedLayoutClass()
{
Console.WriteLine($"Running {nameof(NestedLayoutClass)}...");
Expand Down Expand Up @@ -317,6 +337,8 @@ public static int Main(string[] argv)
SealedBlittableClassByOutAttr();
NestedLayoutClass();
RecursiveNativeLayout();
SealedBlittablePinned();
BlittablePinned();
}
catch (Exception e)
{
Expand Down

0 comments on commit 3848a6d

Please sign in to comment.