Marshal blittable structs via memcpy even if nested within non-blittable struct #20194

jkoritzinsky · 2018-09-28T23:23:33Z

In master, when a fixed buffer is marshaled as part of a non-blittable structure, the marshaling code treats the type of a fixed buffer field fixed byte buffer[2] as a regular structure. However, the custom structure generated for a fixed buffer field has only one field of the type of the buffer. See the IL below for an example:

.class nested public sequential ansi sealed beforefieldinit '<number2>e__FixedBuffer'
	extends [System.Runtime]System.ValueType
{
	.custom instance void [System.Runtime]System.Runtime.CompilerServices.CompilerGeneratedAttribute::.ctor() = (
		01 00 00 00
	)
	.custom instance void [System.Runtime]System.Runtime.CompilerServices.UnsafeValueTypeAttribute::.ctor() = (
		01 00 00 00
	)
	.pack 0
	.size 2

	// Fields
	.field public uint8 FixedElementField

} // end of class <number2>e__FixedBuffer

So, when this member is marshaled as a regular structure only the first member is marshaled since technically it is the only member of the structure.
As a result, the rest of the data is lost.

This PR marshals nested blittable structs via a memcpy. This enables fixed buffers like the one above to be correctly marshaled across managed<-> native. This should also allow us to marshal opaque structs nested inside non-blittable structs across since they are technically blittable.

This PR does not enable correctly marshaling non-blittable fixed buffers. We decided (in PR conversation below) to not support that feature.

Fixes #18521.

…opy of the length of the fixed buffer.

jkotas · 2018-09-28T23:26:09Z

src/vm/fieldmarshaler.cpp

+
+    HRESULT hRESULT = pInternalImport->GetCustomAttributeByName(field, "System.Runtime.CompilerServices.FixedBufferAttribute", NULL, NULL);
+    return hRESULT == S_OK; 
+    return false;


Unreachable code

jkotas · 2018-09-28T23:55:43Z

Should this rather cover all opaque structs with explicit layout, not just "fixed buffers" ?

Consider:

[StructLayout(LayoutKind.Explicit, Size = 16)]
struct OpaqueStruct
{
}

struct MyStruct1
{
   OpaqueStruct _f;
   int _i;
}

struct MyStruct2
{
   OpaqueStruct _f;
   string _s;
}

I expect that the opaque struct will be copied over in the first case, but it won't be copied over in the second case today. Is that correct?

jkotas · 2018-09-29T00:46:36Z

If you run following with your change on checked runtime:

using System;
using System.Runtime.InteropServices;

unsafe struct MyStruct
{
    fixed char X[1];
}

unsafe class My {
    static void Main() {
        Console.WriteLine(Marshal.SizeOf(typeof(MyStruct)));
    }
}

It will hit assert:

Assert failure(PID 22880 [0x00005960], Thread: 14996 [0x3a94]): pEEClassLayoutInfoOut->m_ManagedLargestAlignmentRequirementOfAllMembers == pEEClassLayoutInfoOut->m_LargestAlignmentRequirementOfAllMembers

I am starting to worry that this change has subtle butterfly effects that it is impossible to reason about. It may be too risky to fix in the interop marshalling rules built into the runtime.

luqunl · 2018-10-01T17:01:41Z

CC: @jeffschwMSFT , @AaronRobinsonMSFT

luqunl · 2018-10-01T17:04:52Z

src/vm/fieldmarshaler.cpp

+                    {
+                        ULONG classSize = 0;
+
+                        if (pInternalImport->GetClassTotalSize(thNestedType.GetMethodTable()->GetCl(), &classSize) >= 0)


nit: Please Use if(SUCCEEDED(...

luqunl · 2018-10-01T17:12:15Z

src/vm/nsenums.h

@@ -73,4 +73,6 @@ DEFINE_NFT(NFT_ILLEGAL,                     1,                      true)
 DEFINE_NFT(NFT_WINDOWSFOUNDATIONIREFERENCE, sizeof(IUnknown*),      true)  // Windows.Foundation.IReference`1 is marshaled to System.Nullable`1.
 #endif // FEATURE_COMINTEROP

+DEFINE_NFT(NFT_FIXEDBUFFER,                 0,                      true) // Inline fixed fields


This should be false instead of true, since FixedBufferAttribute isn't a valid winrt attribute.

luqunl · 2018-10-01T17:15:32Z

src/vm/methodtable.cpp

@@ -2807,6 +2807,24 @@ bool MethodTable::ClassifyEightBytesWithNativeLayout(SystemVStructRegisterPassin

            continue;
        }
+        else if (cls == NFT_FIXEDBUFFER)


This code path is same as NFT_NESTEDVALUECLASS, how about
else if(cls == NFT_NESTEDVALUECLASS || cls == NFT_FIXEDBUFFER)

luqunl · 2018-10-01T17:19:49Z

src/vm/fieldmarshaler.h

@@ -1982,6 +1989,132 @@ class FieldMarshaler_DateTimeOffset : public FieldMarshaler

 #endif // FEATURE_COMINTEROP

+class FieldMarshaler_FixedBuffer : public FieldMarshaler


is it possible to update FieldMarshaller_NestedValueClass marshaller instead of adding a new one? For me, These two marshaller are very similar.

Possibly. I'll try to combine them.

luqunl · 2018-10-01T17:26:48Z

src/vm/fieldmarshaler.h

+        memcpyNoGCRefs(pNative, pCLR, sizeof(size));
+    }
+
+    VOID ScalarUpdateCLRImpl(const VOID *pNative, LPVOID pCLR) const


I have a concern for these ScalarUpdateXXXImpl method. such as bool type, in managed side it is 1 byte and in native side(by default, it is WinBool), it is 4 bytes. memcpy may mess up its value.

jkoritzinsky · 2018-10-01T17:28:08Z

@jkotas I figured out why that's failing on the checked build. I mistakenly assumed that all fixed buffers were blittable, forgetting that char and bool buffers won't be.
@luqunl This is the same issue as your review comment. I have some ideas based off the fact that a scalar copy won't work for how to combine FixedBuffer and NestedValueClass marshalers.

AaronRobinsonMSFT · 2018-10-01T17:33:57Z

src/vm/fieldmarshaler.cpp

+    CONTRACTL_END;
+
+    HRESULT hRESULT = pInternalImport->GetCustomAttributeByName(field, g_FixedBufferAttribute, NULL, NULL);
+    return hRESULT == S_OK;


There is some subtly here. The result of this operation is not an instance of type BOOL, but rather the C++ type bool. The difference is as follows: sizeof(BOOL) == 4 and sizeof(bool) == 1. In this case there is an implicit conversion, but that is less than ideal most of the time. In this these cases I normally do hResult == S_OK ? TRUE : FALSE so types are correct. I am okay with it in this case, but there is a difference and it should be known.

AaronRobinsonMSFT · 2018-10-01T17:34:23Z

src/vm/fieldmarshaler.cpp

+    }
+    CONTRACTL_END;
+
+    HRESULT hRESULT = pInternalImport->GetCustomAttributeByName(field, g_FixedBufferAttribute, NULL, NULL);


Please change the name to hr or hResult.

I would prefer hr. since most of our code use hr. only 1 instance use hResult.

AaronRobinsonMSFT · 2018-10-01T19:48:56Z

src/vm/fieldmarshaler.h

+
+    FieldMarshaler_FixedBuffer(ULONG bufferSize, MethodTable* pMT)
+    {
+        size = bufferSize;


Please us initializer list so size can be made const. (i.e. size{ bufferSize })

…eld to pretend that there are multiple fields consecutively in the structure.

… appropriate.

jkoritzinsky · 2018-10-01T20:31:21Z

@jkotas @luqunl @AaronRobinsonMSFT I've re-implemented my fix in a new direction. Since not all fixed buffers are blittable, a scalar copy doesn't work.

So, I decided to treat a fixed buffer member as though it has multiple consecutive fields of the same type as the first field (as it is described in the C# language feature). The buffer length is pulled from the FixedBufferAttribute. I re-use the FieldMarshaler instance to help emulate the second and onwards fields.

This fix also corrects the crash in the checked build mentioned above.

…n't an error.

jkoritzinsky · 2018-10-02T19:28:04Z

@jkotas I've identified the bug that causes the heap overflow. It's another CoreCLR bug with handling fixed buffer fields that has not been found because #18521 has been masking it.

When calculating the native size of a fixed buffer structure, the runtime doesn't account for the fact that the native size of each field in a fixed buffer might be more than the managed size. Since the runtime sees only one field, it calculates the "native size of struct based on fields" = native size of one element of the fixed buffer. The struct managed size is encoded into the metadata by the compiler as (managed size of element * number of elements). The struct size is set to max("native size of struct based on fields", "size in metadata"). So, if the native size of one element of the fixed buffer < size in the metadata or the fixed buffer has more than one element, then the runtime incorrectly calculates the native size of the fixed buffer. Since the current marshaling code in master doesn't write more than the first element, this mis-allocation isn't discovered without also discovering #18521. This fix discovers the issue since it (as of what I've currently pushed) assumes that the runtime correctly calculated the native size of the structure.

I've added some asserts on my local copy to discover this issue when marshaling. I'm currently brainstorming fixes for the masked bug.

jkotas · 2018-10-02T20:40:56Z

Even once you fix that other bug, the change is still going to be changing the behavior in potentially breaking way - imagine that somebody had a hardcoded size or called sizeof instead of Marshal.SizeOf.

…ulated).

jkoritzinsky · 2018-10-02T21:33:54Z

@jkotas I've talked with @AaronRobinsonMSFT and we've decided that the cleanest way to do this would be to revert to the current behavior when the type of the fixed buffer is non-blittable (i.e. char and bool). The usage of fixed buffers of chars and bools will stay the same as today (avoiding the writing out of bounds in the marshaler bug and a possible breaking change as you mentioned), but the use of fixed buffers of blittable types as done in #18521 will work correctly.

jkotas · 2018-10-02T21:38:24Z

we've decided that the cleanest way to do this would be to revert to the current behavior when the type of the fixed buffer is non-blittable

Sounds good.

jkotas · 2018-10-02T22:08:20Z

src/vm/fieldmarshaler.cpp

@@ -659,7 +661,15 @@ do                                                      \
                {
                    if (IsStructMarshalable(thNestedType))
                    {
-                        INITFIELDMARSHALER(NFT_NESTEDVALUECLASS, FieldMarshaler_NestedValueClass, (thNestedType.GetMethodTable()));
+                        LONG numBufferElements;
+                        if (IsFixedBuffer(pfwalk->m_MD, pInternalImport, &numBufferElements) && thNestedType.GetMethodTable()->IsBlittable())


Does it still make sense to use NFT_NESTEDVALUECLASS for this now that it is limited to blittable types only? It seems to be just an expensive way to do memcpy now.

I can revert it back to being a new NFT type. If I do that though, I have to replicate the non-marshaling codepaths that use NFTs to use the same code as NFT_NESTEDVALUECLASS. For reference, blittable types contained within non-blittable types still go through the NestedValueClass field marshaler.

I do not think we need a new NFT type for this. I think that the fix is a lot more complex (and worse perf) than what it needs to be.

Can we fix it by just changing FieldMarshaler_NestedValueClass::NestedValueClassUpdateCLRImpl to:

MethodTable* pMT = GetMethodTable(); // would be better to detect this at class load time (that have a nested value // class with no layout) but don't have a way to know if (!pMT->GetLayoutInfo()) COMPlusThrow(kArgumentException, IDS_NOLAYOUT_IN_EMBEDDED_VALUECLASS); if (pMT->IsBlittable()) { memcpyNoGCRefs((BYTE*)(*ppProtectedCLR) + startoffset, pNative, pMT->GetNativeSize()); } else { LayoutUpdateCLR( (LPVOID*)ppProtectedCLR, startoffset, GetMethodTable(), (BYTE *)pNative); } }

And similar change in FieldMarshaler_NestedValueClass::NestedValueClassUpdateNativeImpl ?

That should work. I'll try it out.

luqunl · 2018-10-02T22:09:50Z

imagine that somebody had a hardcoded size or called sizeof instead of Marshal.SizeOf

The usage of fixed buffers of chars and bools will stay the same as today (avoiding the writing out of bounds in the marshaler bug and a possible breaking change as you mentioned)

if customer is use fixed char/bool[] and sizeof, it will buffer overrun(likely) either before this change or after this change. so I am not too worried that it will break customer.

BTW, I would like to know what testcase matrix we want to add for this change..

jkoritzinsky · 2018-10-02T22:18:52Z

One of the reasons we came up with for not supporting non-blittable fixed buffers at all is that the metadata needed to correctly calculate the native size of the generated fixed buffer structure available at that time, and it would require some weird special casing in the type loader to work around that. And also in the process we'd likely have to change the behavior of opaque structs that contain char or bool fields (which also probably don't currently work as expected anyway).

…ure calculated)." This reverts commit 496eef5.

luqunl · 2018-10-02T22:37:01Z

and it would require some weird special casing in the type loader to work around that.

Just curious, why it need type loader to workaround? maybe I miss something. We know its(the struct generated by roslyn) field marshaller(such as bool/char) and its fieldmarshaller will give us its nativesize(m_cbNativeSize), is it?

jkoritzinsky · 2018-10-02T22:47:08Z

The native size calculation is done while loading the class into the runtime, so that's why I mentioned the type loader. The code is technically in fieldmarshaler.cpp. So we don't actually know that the structure generated by Roslyn is for a fixed buffer field. We can determine it from heuristics, (the attributes listed above, the field name, the struct name being non-legal C#), but the actual attribute that specifies that a field is a fixed buffer is on the field itself. So, we basically have one of two options as far as I can see it.

When the fixed buffer type is non-blittable, ignore the .size metadata on the struct when calculating its native size and re-calculate it to just be the calculated native size of the structure. Then, when loading the type with the fixed field, re-adjust the size of the fixed buffer type to the correct size by using the metadata.
Don't do anything special when loading a structure if it is non-blittable. When loading a field that is a fixed buffer, check if the field in the generated structure is non-blittable. If it is non-blittable, recalculate the size of the buffer type and update it.

jkoritzinsky · 2018-10-02T22:49:59Z

The third option would be to use a heuristic to determine that a structure was generated by the compiler to represent a fixed buffer and then take size in metadata / managed size of field type * native size of field type to get the correct native size.

I really don't like the heuristic idea though because it makes us even more dependent on C# compiler output of generated structures for fixed buffers not changing.

…/coreclr into fixes/coreclr/18521

jkotas · 2018-10-03T04:38:58Z

@dotnet-bot test OSX10.12 x64 Checked Innerloop Build and Test please

AaronRobinsonMSFT · 2018-10-03T17:52:52Z

@jkoritzinsky This is a relatively new piece of functionality and extends a scenario that needs additional test coverage. What is the plan for testing this and where is that PR? Please link that PR ASAP.

jkoritzinsky · 2018-10-04T16:29:28Z

So currently the PR dotnet/corefx#32538 has the testing for this functionality. I'll add in a test there for the opaque struct case as well.

jkoritzinsky added 3 commits September 28, 2018 15:32

Add regression test for dotnet/coreclr#18521.

d12b55b

Add custom marshaler for fixed buffers that acts as a scalar memory c…

a829208

…opy of the length of the fixed buffer.

Remove regression test. Moving it to a unit test in corefx.

20ddf24

jkoritzinsky added the area-Interop label Sep 28, 2018

jkoritzinsky requested review from jkotas and luqunl September 28, 2018 23:23

jkoritzinsky mentioned this pull request Sep 28, 2018

Add test for dotnet/coreclr#18521 fix dotnet/corefx#32538

Merged

Move attribute class name into classnames.h

cea1523

jkotas reviewed Sep 28, 2018

View reviewed changes

Remove unreachable code left over from debugging.

1393b09

luqunl reviewed Oct 1, 2018

View reviewed changes

AaronRobinsonMSFT reviewed Oct 1, 2018

View reviewed changes

jkoritzinsky added 2 commits October 1, 2018 13:08

Marshal fixed buffers by reusing the field marshaler of the single fi…

412502b

…eld to pretend that there are multiple fields consecutively in the structure.

Remove now-dead code paths.

f9b6ae7

jkoritzinsky changed the title ~~Marshal fixed buffers via a memcpy.~~ Marshal fixed buffers by emulating the implied fields. Oct 1, 2018

jkoritzinsky added 2 commits October 1, 2018 13:27

Use initializers in FieldMarshaler_NestedValueClass constructor where…

a04cbf1

… appropriate.

Clean up IsFixedBuffer implementation.

e29416f

jkoritzinsky added 2 commits October 1, 2018 14:26

Remove unused GC_PROTECTs.

9f5d22e

Specifically check that the attribute exists, not just that there was…

aa6641c

…n't an error.

jkoritzinsky changed the title ~~Marshal fixed buffers by emulating the implied fields.~~ WIP: Marshal fixed buffers by emulating the implied fields. Oct 2, 2018

Add asserts so we don't corrupt the heap.

f125ac8

Add unit test for masked bug (incorrect native size of structure calc…

496eef5

…ulated).

Don't use new behavior on non-blittable fixed buffers.

5236477

jkoritzinsky changed the title ~~WIP: Marshal fixed buffers by emulating the implied fields.~~ Marshal fixed buffers by emulating the implied fields. Oct 2, 2018

jkotas reviewed Oct 2, 2018

View reviewed changes

Revert "Add unit test for masked bug (incorrect native size of struct…

3e40bb0

…ure calculated)." This reverts commit 496eef5.

Use memcpy instead of field emulation.

ad0cb17

jkoritzinsky changed the title ~~Marshal fixed buffers by emulating the implied fields.~~ Marshal blittable structs via memcpy even if nested within non-blittable struct Oct 2, 2018

jkoritzinsky and others added 5 commits October 2, 2018 16:46

Remove unused forward-declared class.

b915e99

Clean up code. Refactor one GetMethodTable call I missed.

89fddc5

Merge branch 'fixes/coreclr/18521' of https://github.com/jkoritzinsky…

ce4d8a6

…/coreclr into fixes/coreclr/18521

Remove now-unneeded custom attribute includes. More diff cleanup.

9bc37ce

Remove unneeded FixedBufferAttribute define.

960f560

jkotas approved these changes Oct 3, 2018

View reviewed changes

jkoritzinsky merged commit 302630e into dotnet:master Oct 3, 2018

jkoritzinsky deleted the fixes/coreclr/18521 branch October 3, 2018 16:32

jkoritzinsky mentioned this pull request Oct 23, 2018

Correctly marshal non-blittable fixed buffers #20558

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Marshal blittable structs via memcpy even if nested within non-blittable struct #20194

Marshal blittable structs via memcpy even if nested within non-blittable struct #20194

jkoritzinsky commented Sep 28, 2018 •

edited

Loading

jkotas Sep 28, 2018

jkotas commented Sep 28, 2018 •

edited

Loading

jkotas commented Sep 29, 2018

luqunl commented Oct 1, 2018

luqunl Oct 1, 2018

luqunl Oct 1, 2018

luqunl Oct 1, 2018 •

edited

Loading

luqunl Oct 1, 2018

jkoritzinsky Oct 1, 2018

luqunl Oct 1, 2018

jkoritzinsky commented Oct 1, 2018 •

edited

Loading

AaronRobinsonMSFT Oct 1, 2018

AaronRobinsonMSFT Oct 1, 2018

luqunl Oct 1, 2018

AaronRobinsonMSFT Oct 1, 2018 •

edited

Loading

jkoritzinsky commented Oct 1, 2018 •

edited

Loading

jkoritzinsky commented Oct 2, 2018

jkotas commented Oct 2, 2018

jkoritzinsky commented Oct 2, 2018

jkotas commented Oct 2, 2018

jkotas Oct 2, 2018 •

edited

Loading

jkoritzinsky Oct 2, 2018

jkotas Oct 2, 2018 •

edited

Loading

jkoritzinsky Oct 2, 2018

luqunl commented Oct 2, 2018

jkoritzinsky commented Oct 2, 2018

luqunl commented Oct 2, 2018

jkoritzinsky commented Oct 2, 2018

jkoritzinsky commented Oct 2, 2018

jkotas commented Oct 3, 2018

AaronRobinsonMSFT commented Oct 3, 2018

jkoritzinsky commented Oct 4, 2018

		@@ -1982,6 +1989,132 @@ class FieldMarshaler_DateTimeOffset : public FieldMarshaler

		#endif // FEATURE_COMINTEROP

		class FieldMarshaler_FixedBuffer : public FieldMarshaler

Marshal blittable structs via memcpy even if nested within non-blittable struct #20194

Marshal blittable structs via memcpy even if nested within non-blittable struct #20194

Conversation

jkoritzinsky commented Sep 28, 2018 • edited Loading

Choose a reason for hiding this comment

jkotas commented Sep 28, 2018 • edited Loading

jkotas commented Sep 29, 2018

luqunl commented Oct 1, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

luqunl Oct 1, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jkoritzinsky commented Oct 1, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AaronRobinsonMSFT Oct 1, 2018 • edited Loading

Choose a reason for hiding this comment

jkoritzinsky commented Oct 1, 2018 • edited Loading

jkoritzinsky commented Oct 2, 2018

jkotas commented Oct 2, 2018

jkoritzinsky commented Oct 2, 2018

jkotas commented Oct 2, 2018

jkotas Oct 2, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jkotas Oct 2, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

luqunl commented Oct 2, 2018

jkoritzinsky commented Oct 2, 2018

luqunl commented Oct 2, 2018

jkoritzinsky commented Oct 2, 2018

jkoritzinsky commented Oct 2, 2018

jkotas commented Oct 3, 2018

AaronRobinsonMSFT commented Oct 3, 2018

jkoritzinsky commented Oct 4, 2018

jkoritzinsky commented Sep 28, 2018 •

edited

Loading

jkotas commented Sep 28, 2018 •

edited

Loading

luqunl Oct 1, 2018 •

edited

Loading

jkoritzinsky commented Oct 1, 2018 •

edited

Loading

AaronRobinsonMSFT Oct 1, 2018 •

edited

Loading

jkoritzinsky commented Oct 1, 2018 •

edited

Loading

jkotas Oct 2, 2018 •

edited

Loading

jkotas Oct 2, 2018 •

edited

Loading