46239 v2 (no runtime layout changes) #53424

trylek · 2021-05-28T12:19:37Z

This is a simplified version of my original fix for 46239 as the full fix including runtime layout cleanup,

has an enormous bug tail and seems too risky to take relatively late in the release cycle.

Thanks

Tomas

/cc @dotnet/crossgen-contrib

jkotas · 2021-05-28T12:45:57Z

Since we agreed that this is just a stop-gap measure, I think it would better to omit the last commit that introduces a bunch of undesirable dependencies, like making ILVerify depend on interop marshalling.

trylek · 2021-05-28T14:46:15Z

@jkotas - Hmm, so am I right to understand that you're suggesting I should basically undo the moves of marshalling-related files originally suggested by Michal and implement the BlittableOrManagedSequential check using a virtual override in ReadyToRunMetadataFieldLayoutAlgorithm?

MichalStrehovsky · 2021-05-28T14:52:36Z

@jkotas - Hmm, so am I right to understand that you're suggesting I should basically undo the moves of marshalling-related files originally suggested by Michal and implement the BlittableOrManagedSequential check using a virtual override in ReadyToRunMetadataFieldLayoutAlgorithm?

I agree with Jan - since having interop intertwined with type layout is not our long term plan per the conversation in the other pull request (and we want to get your other change in after .NET 6 cuts off), it doesn't make sense to move interop into type system just to undo that later this year.

jkotas · 2021-05-28T15:55:37Z

src/coreclr/tools/Common/TypeSystem/Common/MetadataFieldLayoutAlgorithm.cs

@@ -349,6 +352,7 @@ protected static ComputedInstanceFieldLayout ComputeExplicitFieldLayout(Metadata
                    );
                if (needsToBeAligned)
                {
+                    largestAlignmentRequired = LayoutInt.Max(largestAlignmentRequired, type.Context.Target.LayoutPointerSize);


I would expect that ComputeFieldSizeAndAlignment will return sufficient alignment for GC pointers or anything with GC pointers. Is this really needed? If yes, it would be worth a comment explaining the situation where it is needed.

I believe you're right, I have replaced this with just an assert double-checking that at this point largestAlignmentRequired must be greater than or equal to PointerSize.

jkotas · 2021-05-28T16:44:30Z

src/coreclr/tools/Common/TypeSystem/Common/MetadataFieldLayoutAlgorithm.cs

            cumulativeInstanceFieldPos -= offsetBias;

            var layoutMetadata = type.GetClassLayout();
+            LayoutInt instanceSize = cumulativeInstanceFieldPos + new LayoutInt(layoutMetadata.Size) + offsetBias;


ComputeInstanceSize bumps this up to layoutMetadata.Size again.

I assume that this needs to be here because ComputeInstanceSize applies it on top of InstanceByteCountUnaligned, but this one applies it on top InstanceByteCount? A comment may be useful since this does not make sense by just looking at the code.

Thanks for pointing that out, auditing this aspect has revealed a few rough edges around the base class size calculation; I'm now locally testing an update cleaning this up by consistently using CalculateFieldBaseOffset in all the relevant places, I'll push the extra commit once I make sure it works reasonably well.

src/coreclr/tools/Common/TypeSystem/Common/MetadataFieldLayoutAlgorithm.cs

trylek · 2021-05-28T20:30:57Z

OK, I believe the initial results aren't the worst, the test 46239 still fails on arm but otherwise the Pri1 runs seem mostly clean. I'm working on retesting the change after cleaning up base class size manipulation per JanK's PR feedback and I'm going to look into the arm issue but I believe the change is mostly functional.

The regression test <code>src\tests\JIT\Regressions\JitBlue\Runtime_46239</code> exercises various interesting corner cases of type layout that weren't handled properly in Crossgen2 on x86 and ARM[32]. This change fixes the remaining deficiencies and it also adds provisions for better runtime logging upon type layout mismatches. Thanks Tomas

With this change, the only remaining pipelines using Crossgen1 are "r2r.yml", "r2r-extra.yml" and "release-tests.yml". I haven't yet identified the pipeline running the "release-tests.yml" script; for the "r2r*.yml", these now remain the only pipelines exercising Crossgen1. I don't think it makes sense to switch them over to CG2 as we already have their CG2 counterparts; my expectation is that, once CG1 is finally decommissioned, they will be just deleted. Thanks Tomas

…ntial

1) Mimic CoreCLR runtime bug filed under dotnet#53542; 2) Fix pointer alignment for byref-like fields.

trylek · 2021-06-03T15:28:13Z

OK, I finally have a green outerloop run (including the CG2 legs). I believe the change is ready to be merged in.

jkotas · 2021-06-03T16:51:47Z

src/coreclr/tools/Common/TypeSystem/Common/MetadataFieldLayoutAlgorithm.cs

+            LayoutInt cumulativeInstanceFieldPos = CalculateFieldBaseOffset(type, requiresAlign8: false, requiresAlignedBase: false) - offsetBias;
+            if (!type.IsValueType)
+            {
+                // CoreCLR runtime has a bug in field offset calculation in the presence of class inheritance


What is a simple repro to demonstrate this bug?

I have tried:

using System; using System.Runtime.InteropServices; [StructLayout(LayoutKind.Explicit)] class A1 { [FieldOffset(0)] public int x; } [StructLayout(LayoutKind.Explicit)] class A2 : A1 { [FieldOffset(0)] public int y; } [StructLayout(LayoutKind.Explicit)] class A3 : A2 { [FieldOffset(0)] public int z; } class My { static unsafe void Main() { A3 a = new A3(); fixed (int *px = &a.x) fixed (int *pz = &a.z) { Console.WriteLine(pz - px); } } }

It prints 2 for me. I would expect it to print more than that given this bug that doubles the base offsets.

When I modify your example like this:

using System; using System.Runtime.InteropServices; [StructLayout(LayoutKind.Explicit)] class A1 { [FieldOffset(0)] public byte x; [FieldOffset(2)] public char x2; } [StructLayout(LayoutKind.Explicit)] class A2 : A1 { [FieldOffset(0)] public int y; } [StructLayout(LayoutKind.Explicit)] class A3 : A2 { [FieldOffset(0)] public byte z; } class My { static unsafe void Main() { A3 a = new A3(); fixed (byte *px = &a.x) fixed (byte *pz = &a.z) { Console.WriteLine(pz - px); } } }

I receive 24; if properly packed, I think it should say 8 like you said. Your example doesn't exhibit the issue because it hits this weird statement I stumbled upon a couple of times when working on the change:

runtime/src/coreclr/vm/methodtablebuilder.cpp

Line 8563 in 01a8f49

if (IsBlittable() || IsManagedSequential())

Thanks

Tomas

I guess I should probably add this condition to the quirk, will do.

I receive 24;

And it prints 19 on .NET Framework and .NET Core 3.1. It looks like we accidentally changing behavior here. @jkoritzinsky Can it be caused by your field layout refactoring? It would be nice to get this fixed and covered by tests.

I guess I should probably add this condition to the quirk, will do.

I do not think we need to maintain this quirk. I would delete this, disable the test that it hitting this corner case and file a bug to get it fixed. Note that fixing this is not going to be R2R breaking change.

Possibly. I think I changed a little bit about how classes with inheritance are considered blittable/non-blittable. That might have changed this.

OK, I'll remove the quirk and I'll retest to see what exactly breaks.

trylek · 2021-06-03T20:05:12Z

Removal of the quirk naturally hit one of the typesystem tests. I'll fix that once the outerloop tests finish along with any potential additions to issues.targets for tests affected by this issue.

trylek · 2021-06-03T22:00:14Z

OK, I have removed the double base size quirk and we're discussing the fix on the issue thread #53542. My previous outerloop testing was green so I'm not planning to run a new outerloop test as that doesn't exercise the Crossgen2 unit testing. Please let me know if there's anything else that needs addressing before merging this long-standing change in.

davidwrighton

I've been following this change, and it looks good to me now.

jkotas

LGTM as well. Thank you!

These numbers changed in #53424. One of the reasons why I'm not a huge fan of too much commenting...

mangod9 · 2021-06-04T17:33:59Z

Thanks @trylek on fixing this!

These numbers changed in #53424. One of the reasons why I'm not a huge fan of too much commenting...

trylek added the area-crossgen2-coreclr label May 28, 2021

trylek added this to the 6.0.0 milestone May 28, 2021

trylek requested review from jkotas and davidwrighton May 28, 2021 12:19

trylek force-pushed the 46239-NoRuntimeLayoutChanges branch from 5ed9f3c to 1c41aaa Compare May 28, 2021 15:27

jkotas reviewed May 28, 2021

View reviewed changes

src/coreclr/tools/Common/TypeSystem/Common/MetadataFieldLayoutAlgorithm.cs Outdated Show resolved Hide resolved

CertifiedRice approved these changes May 29, 2021

View reviewed changes

trylek force-pushed the 46239-NoRuntimeLayoutChanges branch 3 times, most recently from b2497bc to 87067ae Compare June 2, 2021 15:19

trylek added 12 commits June 2, 2021 22:01

One more fix for explicit layout with zero fields

790f45f

Remove no longer needed R2RMFLA override of IsBlittableOrManagedSeque…

a9aa8c3

…ntial

Implement IsBlittableOrManagedSequential using a R2R-specific override

1e2abf3

Rename the method per JanK's PR feedback

d21272e

Fix two explicit layout details

a9549d4

1) Mimic CoreCLR runtime bug filed under dotnet#53542; 2) Fix pointer alignment for byref-like fields.

Fix bad conflict merge

c83c6f0

Mark remaining CG2 outerloop failures to clean up the CI results

a50d93f

Remove artificial alignment change per DavidWr's PR feedback

a4e3aee

Fix the StructPacking test on ARM

54e4dd4

Improve fix for StructPacking, finally fix 46239 on ARM

3dcdc31

trylek force-pushed the 46239-NoRuntimeLayoutChanges branch from 87067ae to 3dcdc31 Compare June 2, 2021 20:01

Further refining of the ARM case

cabf41c

Fix subtle difference in x86 alignment with vs. without layout

642659b

jkotas reviewed Jun 3, 2021

View reviewed changes

Remove the duplicate base type size quirk per JanK's PR feedback

f14893c

trylek force-pushed the 46239-NoRuntimeLayoutChanges branch from 16c32ae to f14893c Compare June 3, 2021 18:00

trylek mentioned this pull request Jun 3, 2021

Bug regarding inheritance in classes with explicit layout #53542

Closed

Update the typesystem unit tests to removal of the base size quirk

c5ac961

davidwrighton approved these changes Jun 4, 2021

View reviewed changes

jkotas approved these changes Jun 4, 2021

View reviewed changes

trylek merged commit b0b317e into dotnet:main Jun 4, 2021

trylek deleted the 46239-NoRuntimeLayoutChanges branch June 4, 2021 08:40

MichalStrehovsky added a commit that referenced this pull request Jun 4, 2021

Update comments in type system tests

1bcba0e

These numbers changed in #53424. One of the reasons why I'm not a huge fan of too much commenting...

MichalStrehovsky mentioned this pull request Jun 4, 2021

Update comments in type system tests #53718

Merged

stephentoub pushed a commit that referenced this pull request Jun 4, 2021

Update comments in type system tests (#53718)

eee7a19

These numbers changed in #53424. One of the reasons why I'm not a huge fan of too much commenting...

ghost locked as resolved and limited conversation to collaborators Jul 4, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

46239 v2 (no runtime layout changes) #53424

46239 v2 (no runtime layout changes) #53424

trylek commented May 28, 2021

jkotas commented May 28, 2021

trylek commented May 28, 2021

MichalStrehovsky commented May 28, 2021

jkotas May 28, 2021

trylek May 28, 2021

jkotas May 28, 2021 •

edited

Loading

trylek May 28, 2021

trylek commented May 28, 2021

trylek commented Jun 3, 2021

jkotas Jun 3, 2021

trylek Jun 3, 2021 •

edited

Loading

trylek Jun 3, 2021

jkotas Jun 3, 2021

jkotas Jun 3, 2021

jkoritzinsky Jun 3, 2021

trylek Jun 3, 2021

trylek Jun 3, 2021

trylek commented Jun 3, 2021

trylek commented Jun 3, 2021

davidwrighton left a comment

jkotas left a comment

mangod9 commented Jun 4, 2021

46239 v2 (no runtime layout changes) #53424

46239 v2 (no runtime layout changes) #53424

Conversation

trylek commented May 28, 2021

jkotas commented May 28, 2021

trylek commented May 28, 2021

MichalStrehovsky commented May 28, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jkotas May 28, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

trylek commented May 28, 2021

trylek commented Jun 3, 2021

Choose a reason for hiding this comment

trylek Jun 3, 2021 • 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

trylek commented Jun 3, 2021

trylek commented Jun 3, 2021

davidwrighton left a comment

Choose a reason for hiding this comment

jkotas left a comment

Choose a reason for hiding this comment

mangod9 commented Jun 4, 2021

jkotas May 28, 2021 •

edited

Loading

trylek Jun 3, 2021 •

edited

Loading