Add explicit layout checks to Crossgen2 #27054

trylek · 2019-10-06T01:05:06Z

Several tests under "Loader/classloader/explicitlayout" fail today
because Crossgen2 is missing GC consistency checks for types with
explicit layout - according to my understanding of the corresponding
CoreCLR algorithm we basically need to make sure that no GC fields
overlap non-GC fields. I have reimplemented the algorithm instead
of porting it as the CoreCLR version is heavily tied to the method
table builder and hard to separate out.

Thanks

Tomas

MichalStrehovsky · 2019-10-07T09:45:04Z

This is rather difficult for me to follow with the accessPath and all. Do I understand it correctly that this goes over each field and for each field we check whether it overlaps any other field (by doing another foreach), basically doing n * n comparisons in the end?

I feel like this would be easier to follow (and more performant) if we do it like CoreCLR does it by allocating a byte array and setting bytes in it as we walk the fields in a single pass:

coreclr/src/vm/methodtablebuilder.cpp

Lines 8324 to 8328 in af55c1b

    
           BYTE *pFieldLayout = (BYTE*) qb.AllocThrows(instanceSliceSize * sizeof(BYTE)); 
        
           for (i=0; i < instanceSliceSize; i++) 
        
           { 
        
               pFieldLayout[i] = empty; 
        
           }

I'm concerned that there's bugs in this around handling of pointer-typed fields (seems like we don't consider an overlap an error, but CoreCLR does):

    [StructLayout(LayoutKind.Explicit)]
    unsafe struct A
    {
        [FieldOffset(0)] object o;
        [FieldOffset(0)] int* z;
    }
}

We also don't consider an overlap in padding an error, but CoreCLR does:

    [StructLayout(LayoutKind.Explicit)]
    unsafe struct A
    {
        [FieldOffset(8)] object o;
    }

    [StructLayout(LayoutKind.Explicit)]
    unsafe struct B
    {
        [FieldOffset(0)] A a;
        [FieldOffset(0)] object o;
    }

Two more things:

We should do this check before ComputeExplicitFieldLayout returns - this is a core type system invariant that we should check for and not just an R2R addon.
The exception should match what CoreCLR does. ExceptionStringID.ClassLoadExplicitLayout is the right ID to use (it matches IDS_CLASSLOAD_EXPLICIT_LAYOUT), but the last parameter is an offset. We do this for compat - we want the exception messages to match what CoreCLR would throw. (The compat aspect is more pronounced in full AOT mode, but it's also here.)

Ideally, this would be accompanied by a unit test since it's a type system change and we need those to be well tested, but unfortunately we haven't ported unit tests to this repo :(

trylek · 2019-10-30T14:46:20Z

@MichalStrehovsky - I believe I have addressed your PR feedback in the latest iteration; could you please take another look when you have a chance? At this point I haven't done anything about unit testing. The change fixes about 10~20 failures in existing CoreCLR tests under Loader\classloader\explicitlayout. Did you allude to porting some existing tests from the CoreRT repo?

MichalStrehovsky · 2019-10-30T14:55:27Z

src/tools/crossgen2/Common/TypeSystem/Common/ExplicitLayoutValidator.cs

+
+namespace Internal.TypeSystem
+{
+    public class ExplicitLayoutValidator


Can this be a struct?

Of course, thanks for pointing that out!

MichalStrehovsky · 2019-10-30T14:55:36Z

src/tools/crossgen2/Common/TypeSystem/Common/ExplicitLayoutValidator.cs

+
+using Internal.TypeSystem;
+using System;
+using System.Net;


What do we use System.Net for?

No use, I just accidentally copied that over along with the copyright header. Deleted.

MichalStrehovsky · 2019-10-30T14:56:03Z

src/tools/crossgen2/Common/TypeSystem/Common/ExplicitLayoutValidator.cs

+// The .NET Foundation licenses this file to you under the MIT license.
+// See the LICENSE file in the project root for more information.
+
+using Internal.TypeSystem;


Nit: I think this is unnecessary

MichalStrehovsky · 2019-10-30T15:01:33Z

src/tools/crossgen2/Common/TypeSystem/Common/ExplicitLayoutValidator.cs

+                    }
+                }
+            }
+            else if (fieldType.IsPointer || fieldType.IsByRef || fieldType.IsByRefLike)


IsByRefLike is a ref struct - those will respond to IsValueType above.

We don't allow ByRef fields right now, so maybe we don't need to handle that.

Pointer typed fields are okay to be misaligned. This works:

[StructLayout(LayoutKind.Explicit)] unsafe class Program { [FieldOffset(2)] int* f; static unsafe void Main() { } }

Removed IsByRefLike from the conditional check. Added a new clause for IsPointer setting the range to NonORef.

MichalStrehovsky · 2019-10-30T15:04:04Z

src/tools/crossgen2/Common/TypeSystem/Common/ExplicitLayoutValidator.cs

+            }
+            else if (fieldType.IsValueType)
+            {
+                MetadataType mdType = (MetadataType)fieldType;


If the field is a ByRef-like struct, it also needs to be aligned at pointer boundary. This should throw:

[StructLayout(LayoutKind.Explicit)] ref struct Program { [FieldOffset(2)] Span<int> f; static unsafe void Main() { } }

Added alignment check for IsByRefLike within the IsValueType conditional block.

MichalStrehovsky · 2019-10-30T15:05:53Z

src/tools/crossgen2/Common/TypeSystem/Common/ExplicitLayoutValidator.cs

+            }
+            else
+            {
+                throw new NotImplementedException();


Nit: Instead of an else branch that throws NotImplemented, I tend to use asserts. It doesn't make sense to ship the throw to customers and if we ever add new type system entities, the assert will hit.

Changed NotImplementedException throw to assertion failure.

MichalStrehovsky · 2019-10-30T15:50:35Z

Did you allude to porting some existing tests from the CoreRT repo?

I was just thinking how to do the testing for this (e.g. all the cases that I pointed out in this review would make good unit test cases).

We could make this change in CoreRT repo with unit tests included and then commit just the product part here. But I'm not going to block on that because it's awkward in the short term (although having unit tests is better in long term).

I do want to bring over the tests at some point.

Based on Michal's PR feedback I have refactored the explicit layout checks to use a loose port of the CoreCLR algorithm i.e. creating an array representing the individual bytes of the type layout and gradually filling it in with GC reference / non-GC reference markers for the individual fields. Thanks Tomas

1) Remove superfluous using clauses; 2) Make ExplicitLayoutValidator a struct; 3) Fixing handling of IsByRefLike and IsPointer cases; 4) Changing the final throw NotImplementedException to an assert. Thanks Tomas

trylek · 2019-10-31T05:20:47Z

OK, that should be it (modulo the tests I haven't done anything about at this point), @MichalStrehovsky - could you please take another look?

MichalStrehovsky · 2019-10-31T13:15:58Z

src/tools/crossgen2/Common/TypeSystem/Common/ExplicitLayoutValidator.cs

+                }
+                SetFieldLayout(offset, _pointerSize, FieldLayoutTag.ORef);
+            }
+            else if (fieldType.IsPointer)


Suggested change

else if (fieldType.IsPointer)

else if (fieldType.IsPointer || fieldType.IsFunctionPointer)

Fixed in 3rd commit.

MichalStrehovsky

Looks good modulo the function pointer case.

janvorli

LGTM, thank you!

trylek requested review from janvorli and MichalStrehovsky October 6, 2019 01:05

trylek force-pushed the ExplicitLayoutChecks branch from 9f9ae31 to 6fd02d1 Compare October 28, 2019 16:02

MichalStrehovsky reviewed Oct 30, 2019

View reviewed changes

trylek added 2 commits October 30, 2019 20:28

Address Michal's PR feedback

a51c425

1) Remove superfluous using clauses; 2) Make ExplicitLayoutValidator a struct; 3) Fixing handling of IsByRefLike and IsPointer cases; 4) Changing the final throw NotImplementedException to an assert. Thanks Tomas

trylek force-pushed the ExplicitLayoutChecks branch from 6fd02d1 to a51c425 Compare October 31, 2019 05:19

MichalStrehovsky reviewed Oct 31, 2019

View reviewed changes

MichalStrehovsky approved these changes Oct 31, 2019

View reviewed changes

Add handling for function pointers per Michal's PR feedback

ff58fb8

trylek added the area-crossgen2 label Oct 31, 2019

janvorli approved these changes Oct 31, 2019

View reviewed changes

trylek merged commit 97c5829 into dotnet:master Oct 31, 2019

trylek deleted the ExplicitLayoutChecks branch October 31, 2019 20:59

MichalStrehovsky mentioned this pull request Feb 2, 2022

Add tests for ref field overlap with OBJECTREF and non-aligned ref fields. dotnet/runtime#64643

Merged

MichalStrehovsky mentioned this pull request Feb 14, 2022

Validate IntPtr and ByRef field overlap dotnet/runtime#65246

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add explicit layout checks to Crossgen2 #27054

Add explicit layout checks to Crossgen2 #27054

trylek commented Oct 6, 2019

MichalStrehovsky commented Oct 7, 2019

trylek commented Oct 30, 2019

MichalStrehovsky Oct 30, 2019

trylek Oct 31, 2019

MichalStrehovsky Oct 30, 2019

trylek Oct 31, 2019

MichalStrehovsky Oct 30, 2019

trylek Oct 31, 2019

MichalStrehovsky Oct 30, 2019

trylek Oct 31, 2019

MichalStrehovsky Oct 30, 2019

trylek Oct 31, 2019

MichalStrehovsky Oct 30, 2019

trylek Oct 31, 2019

MichalStrehovsky commented Oct 30, 2019

trylek commented Oct 31, 2019

MichalStrehovsky Oct 31, 2019

trylek Oct 31, 2019

MichalStrehovsky left a comment

janvorli left a comment

	else if (fieldType.IsPointer)
	else if (fieldType.IsPointer \|\| fieldType.IsFunctionPointer)

Add explicit layout checks to Crossgen2 #27054

Add explicit layout checks to Crossgen2 #27054

Conversation

trylek commented Oct 6, 2019

MichalStrehovsky commented Oct 7, 2019

trylek commented Oct 30, 2019

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

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

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

MichalStrehovsky commented Oct 30, 2019

trylek commented Oct 31, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MichalStrehovsky left a comment

Choose a reason for hiding this comment

janvorli left a comment

Choose a reason for hiding this comment