Miscellaneous CPAOT fixes for several Pri# bug buckets by trylek · Pull Request #7590 · dotnet/corert

trylek · 2019-07-05T10:12:15Z

We were missing support for structs with explicit layout. I have
added the appropriate logic.
Nullable is not blittable (however it is managed sequential if
its instantiation type is managed sequential).
Structs with explicit layout aren't managed sequential.
We were putting the cctor trigger helper into the wrong import
table - DispatchImports emits GC ref map and cctor trigger doesn't
need one because it's not a method call helper. This was crashing
R2RDump when trying to disassemble some R2R executables.
I have added two unit tests to the R2R unit test suite by
adapting two CoreCLR tests that were crashing in interesting ways
when manipulating explicit layouts and nullables.
Instantiated type signature encoding was incorrect in large
version bubble case. As JanV described earlier, the context for
encoding of the instantiation type parameters is the outer context,
not the generic type context.
Always emit all import tables even when they are empty, otherwise
the fixup encoding gets out of sync as it refers to import table
indices.
When SuperIlc calls R2RDump in the large version bubble mode, it
needs to pass CORE_ROOT as the "reference path" parameter so that
R2RDump can resolve framework assembly references.
Field encoding was doing an unnecessarily strong check - replaced
ContainsType with VersionsWithType.

Thanks

Tomas

jkotas · 2019-07-05T15:53:05Z

@@ -59,6 +59,10 @@ public static bool IsBlittableType(TypeDesc type)
        public static bool IsManagedSequentialType(TypeDesc type)


This is becoming more and more specific to R2R/CoreCLR layout rules. Would be better to move this to the R2R specific file?

For now I have removed the superfluous new condition in IsBlittableType. For IsManagedSequentialType, that has always been only used by the R2R compiler. Are you thinking about something along the lines of CorInfoImpl.ReadyToRun / RyuJit or would you prever moving the IsManagedSequentialType check to a completely new class?

I would move it to ReadyToRunMetadataFieldLayoutAlgorithm.cs. It is specific to the current CoreCLR field layout algorithm.

I am temporarily rolling back all changes to MarshalUtils as they require more work and delay merging in other useful parts of this change, most notably fixes for import table consistency and for the large version bubble. I'm going to send this change out in a separate future PR after performing the comparative analysis with Crossgen par Jan's suggestion.

jkotas · 2019-07-05T16:00:47Z

        public static bool IsManagedSequentialType(TypeDesc type)
        {
            type = type.UnderlyingType;
+            if (type is MetadataType metadataType && metadataType.IsExplicitLayout)


Is this doing the right think for mix of explicit layout and non-explicit layout fields?

From my limited understanding the IsManagedSequential check in CoreCLR basically performs a series of AND operations on several boolean indicators when calculating the outcome. I believe this roughly corresponds to the current structure of the recursive method where basically all fast paths return false (when one of the boolean indicators is incorrect).

In this sense I believe that the presence of any instance fields with explicit layout types guarantees that the type will be identified as non-managed-sequential in the for() loop over the instance fields calling recursively into the IsManagedSequentialType check. I apologize if I misunderstood your concern; if that was the case, please clarify.

What kind of testing have you done on this?

It may be useful to add temporary instrumentation that logs the value of IsManagedSequential in both crossgen and cpaot, run all tests we have around (coreclr, corefx, winforms, wpf, ...); and see whether it is 100% match between the two.

I am worried that the field layout plan we are on for cpaot is going to have never ending bug tail.

Forgetting about IsManagedSequential for a moment and looking at the bigger picture - from my experience in messing around in the CoreCLR codebase, we'll need to 100% match all the p/invoke field marshallers to get the right answers for instance layout. This is because IsBlittable is basically implemented as "go over all the instance fields and see if the marshaller for the field is the one that just copies bytes over". Then there's the logic that chooses the marshallers that is... quite complex.

The question is how much do we need to match this to be compatible. We could store the instance layout information along with our IsBlittable/IsManagedSequential decisions in the image for CoreCLR to deserialize like we discussed before. This would mean we don't need to match CoreCLR exactly.

But if our decisions are different from CoreCLR's, would that be observable to badly annotated p/invokes like dotnet/corefx#39285? (This p/invoke is missing the OutAttribute on the pvBuffer parameter, so the runtime wouldn't copy back the values from native, but since CoreCLR considers USEROBJECTFLAGS blittable, we just pin the managed value and there's no copy to native/from native. The FX code using the p/invoke considers pvBuffer an out parameter.) Would breaking badly annotated p/invokes be acceptable?

For Jan's question regarding the level of testing, within my inner loop I was mostly running the CoreCLR JIT Pri#1 tests in debug / release mode with / without the large version bubble; I however only looked at individual IsManagedSequential calculations manually in the debugger for some types where I originally identified a mismatch between Crossgen and CPAOT. I like the instrumentation idea, that should be relatively straightforward to implement.

I believe we're in general agreement that persisting (some) field information in the R2R file is the way to go but I doubt it's possible to design, implement and sufficiently validate in the planned preview timeframe; so my initial assumption was that, for now, we'd just replicate the CoreCLR logic the best we can, and as a next step we'd proceed towards implementing the persisted field info as a more robust longer-term solution for the problem.

trylek · 2019-07-07T21:23:37Z

@jkotas - I tried to address most of your feedback in 2nd commit. Could you please take another look and clarify the remaining things I'm unsure about?

1) We were missing support for structs with explicit layout. I have added the appropriate logic. 2) Nullable is not blittable (however it is managed sequential if its instantiation type is managed sequential). 3) Structs with explicit layout aren't managed sequential. 4) We were putting the cctor trigger helper into the wrong import table - DispatchImports emits GC ref map and cctor trigger doesn't need one because it's not a method call helper. This was crashing R2RDump when trying to disassemble some R2R executables. 5) I have added two unit tests to the R2R unit test suite by adapting two CoreCLR tests that were crashing in interesting ways when manipulating explicit layouts and nullables. 6) Instantiated type signature encoding was incorrect in large version bubble case. As JanV described earlier, the context for encoding of the instantiation type parameters is the outer context, not the generic type context. 7) Always emit all import tables even when they are empty, otherwise the fixup encoding gets out of sync as it refers to import table indices. 8) When SuperIlc calls R2RDump in the large version bubble mode, it needs to pass CORE_ROOT as the "reference path" parameter so that R2RDump can resolve framework assembly references. 9) Field encoding was doing an unnecessarily strong check - replaced ContainsType with VersionsWithType. Thanks Tomas

I'm reverting this part of the change; I'm going to send it out for a new PR after performing the instrumented measurements as suggested by Jan Kotas in the PR discussion. Thanks Tomas

trylek · 2019-07-09T15:33:54Z

I have force-pushed the change rebased against current CoreRT master and I have reverted all changes to MarshalUtils as they seem to be the only "controversial" bits blocking merge of the rest of this change. I'm going to send out the MarshalUtils changes (move of the IsManagedSequential method and changes to its functionality to make it better match Crossgen) after performing the instrumented measurements as suggested by JanK.

MichalStrehovsky · 2019-07-09T16:53:43Z

 using Internal.TypeSystem;
 using Internal.TypeSystem.Ecma;
 using Internal.TypeSystem.Interop;
+using ILCompiler.DependencyAnalysis;


Do we need this?

Removed in 5th commit; thanks for noticing!

trylek · 2019-07-09T19:05:18Z

I'm investigating the CI failures right now, they seem real even though I have a bit of a hard time reproducing them locally. Hope to be able to send out an update shortly, apologies about the inconvenience.

trylek · 2019-07-09T19:09:02Z

Ahh, sorry about that, one of the unit tests I originally added requires the improved IsManagedSequential functionality I temporarily reverted. I'll disable the test for now.

This test requires changes to the IsManagedSequential method I have temporarily reverted from CPAOT because of extensive PR feedback suggesting additional analytic work. In light of this fact I'm temporarily disabling this method before the IsManagedSequential issue is completely resolved. Thanks Tomas

trylek requested a review from nattress July 5, 2019 10:12

jkotas reviewed Jul 5, 2019

View reviewed changes

Comment thread src/Common/src/TypeSystem/Interop/IL/MarshalUtils.cs Outdated

jkotas reviewed Jul 5, 2019

View reviewed changes

Comment thread src/Common/src/TypeSystem/Interop/IL/MarshalUtils.cs Outdated

jkotas reviewed Jul 5, 2019

View reviewed changes

trylek added 3 commits July 9, 2019 17:18

Address JanK's initial PR feedback

c648a0f

Revert all changes to MarshalUtils.cs

c08a087

I'm reverting this part of the change; I'm going to send it out for a new PR after performing the instrumented measurements as suggested by Jan Kotas in the PR discussion. Thanks Tomas

trylek force-pushed the JitDirectedFixes branch from 246267e to c08a087 Compare July 9, 2019 15:31

MichalStrehovsky approved these changes Jul 9, 2019

View reviewed changes

Remove unneded using statements per Michal's PR feedback

86de900

trylek merged commit 2470dfb into dotnet:master Jul 10, 2019

trylek deleted the JitDirectedFixes branch July 10, 2019 06:41

		@@ -59,6 +59,10 @@ public static bool IsBlittableType(TypeDesc type)
		public static bool IsManagedSequentialType(TypeDesc type)

Uh oh!

Conversation

trylek commented Jul 5, 2019

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

trylek commented Jul 7, 2019

Uh oh!

trylek commented Jul 9, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

trylek commented Jul 9, 2019

Uh oh!

trylek commented Jul 9, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants