[release/7.0-rc2] Do not request type layout for RuntimeDetermined types#75789
Merged
carlossanlop merged 1 commit intorelease/7.0-rc2from Sep 19, 2022
Merged
Conversation
#74123 broke compiling TechEmpower benchmarks with NativeAOT. We now crash with: ``` > ILCompiler.TypeSystem.dll!Internal.TypeSystem.RuntimeDeterminedFieldLayoutAlgorithm.ComputeInstanceLayout(Internal.TypeSystem.DefType defType, Internal.TypeSystem.InstanceLayoutKind layoutKind) Line 19 C# ILCompiler.TypeSystem.dll!Internal.TypeSystem.DefType.ComputeInstanceLayout(Internal.TypeSystem.InstanceLayoutKind layoutKind) Line 436 C# ILCompiler.TypeSystem.dll!Internal.TypeSystem.DefType.IsInt128OrHasInt128Fields.get() Line 150 C# ILCompiler.TypeSystem.dll!Internal.TypeSystem.MetadataFieldLayoutAlgorithm.ComputeAutoFieldLayout(Internal.TypeSystem.MetadataType type, int numInstanceFields) Line 483 C# ILCompiler.Compiler.dll!ILCompiler.CompilerMetadataFieldLayoutAlgorithm.ComputeInstanceFieldLayout(Internal.TypeSystem.MetadataType type, int numInstanceFields) Line 56 C# ILCompiler.TypeSystem.dll!Internal.TypeSystem.MetadataFieldLayoutAlgorithm.ComputeInstanceLayout(Internal.TypeSystem.DefType defType, Internal.TypeSystem.InstanceLayoutKind layoutKind) Line 164 C# ILCompiler.TypeSystem.dll!Internal.TypeSystem.DefType.ComputeInstanceLayout(Internal.TypeSystem.InstanceLayoutKind layoutKind) Line 436 C# ILCompiler.TypeSystem.dll!Internal.TypeSystem.DefType.InstanceFieldSize.get() Line 165 C# ILCompiler.TypeSystem.dll!Internal.TypeSystem.MetadataFieldLayoutAlgorithm.ComputeFieldSizeAndAlignment(Internal.TypeSystem.TypeDesc fieldType, bool hasLayout, int packingSize, out bool layoutAbiStable, out bool fieldTypeHasAutoLayout, out bool fieldTypeHasInt128Field) Line 838 C# ILCompiler.TypeSystem.dll!Internal.TypeSystem.MetadataFieldLayoutAlgorithm.ComputeStaticFieldLayout(Internal.TypeSystem.DefType defType, Internal.TypeSystem.StaticLayoutKind layoutKind) Line 215 C# ILCompiler.TypeSystem.dll!Internal.TypeSystem.DefType.ComputeStaticFieldLayout(Internal.TypeSystem.StaticLayoutKind layoutKind) Line 473 C# ILCompiler.TypeSystem.dll!Internal.TypeSystem.DefType.GCStaticFieldSize.get() Line 302 C# ILCompiler.Compiler.dll!ILCompiler.DependencyAnalysis.NativeLayoutTemplateTypeLayoutVertexNode.GetStaticDependencies(ILCompiler.DependencyAnalysis.NodeFactory context) Line 997 C# ``` https://github.com/dotnet/runtime/blob/4cf1383c8458945b7eb27ae5f57338c10ed25d54/src/coreclr/tools/Common/TypeSystem/RuntimeDetermined/RuntimeDeterminedFieldLayoutAlgorithm.cs#L17-L19 I was not able to come up with a standalone repro case, but this fixes compiling the benchmark. I'm not clear why native layout operates on runtime determined forms, but what I'm doing here should have equivalent behaviors, except avoiding the problem that we can no longer compute layouts of runtime determined things in some obscure scenarios due to the new code.
github-actions
bot
requested a review
from MichalStrehovsky
as a code owner
ghost
added
the
jkotas
approved these changes
Sep 17, 2022
Member
|
@MichalStrehovsky Could you please take care of the servicing bar paperwork? |
MichalStrehovsky
added
the
Servicing-consider
jeffschwMSFT
approved these changes
Sep 18, 2022
Member
jeffschwMSFT
left a comment
jeffschwMSFT
left a comment
There was a problem hiding this comment.
approved. we will take for consideration in RC2
Member
|
Steve signed off on this by email. Should I flip the label to servicing approved? (Or who is supposed to flip the label?) |
jeffschwMSFT
added
Servicing-approved
Contributor
|
CI is green. Approved and signed off. Ready to merge. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Backport of #75746 to release/7.0-rc2
/cc @jkotas @MichalStrehovsky
Customer Impact
It's not possible to build the TechEmpower benchmarks (or the default MVC Razor template) with NativeAOT because the compiler crashes. See #75746 for stack.
Testing
Verified the benchmark can now be built. Passes all NativeAOT testing we have.
Risk
Low, the fix is relatively conservative and switched from looking at the layout of
RuntimeDeterminedtypes to__Canontypes (that behave identically as far as layout is concerned). This codepath is exercised pretty extensively. The buggy codepath required some very unique conditions to repro and is specific to layouts of RuntimeDetermined types.