- 
                Notifications
    You must be signed in to change notification settings 
- Fork 5.2k
Add support for "extended" layouts in the runtimes and add CStruct layout #116082
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR adds support for an “extended” layout flag in the runtimes, introduces the ExtendedLayoutAttribute and CStruct layout kind, and wires through layout info across types and metadata emitters.
- Add IsExtendedLayoutandGetExtendedLayoutInfoAPIs to core metadata types
- Implement ComputeCStructFieldLayoutand dispatch in field layout algorithms
- Update metadata headers, ILASM grammar, and reflection emit to recognize extendedlayout
Reviewed Changes
Copilot reviewed 45 out of 45 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description | 
|---|---|
| src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/CompilerMetadataFieldLayoutAlgorithm.cs | Dispatch ComputeExtendedFieldLayoutbefore explicit layout | 
| src/coreclr/tools/Common/TypeSystem/Interop/IL/PInvokeDelegateWrapper.cs | Stub out IsExtendedLayout&GetExtendedLayoutInfo | 
| src/coreclr/tools/Common/TypeSystem/Interop/IL/NativeStructType.cs | Override extended layout members and simplify GetClassLayout | 
| src/coreclr/tools/Common/TypeSystem/Interop/IL/MarshalUtils.cs | Treat CStructas blittable | 
| src/coreclr/tools/Common/TypeSystem/Interop/IL/InlineArrayType.cs | Stub out IsExtendedLayout&GetExtendedLayoutInfo | 
| src/coreclr/tools/Common/TypeSystem/Ecma/EcmaType.cs | Parse ExtendedLayoutAttributeand implementIsExtendedLayout | 
| src/coreclr/tools/Common/TypeSystem/Common/TypeWithRepeatedFields.cs | Forward extended layout members | 
| src/coreclr/tools/Common/TypeSystem/Common/TypeSystemHelpers.cs | Include IsExtendedLayoutinHasLayout | 
| src/coreclr/tools/Common/TypeSystem/Common/MetadataType.cs | Add abstract IsExtendedLayout,GetExtendedLayoutInfo, and supporting types | 
| src/coreclr/tools/Common/TypeSystem/Common/MetadataFieldLayoutAlgorithm.cs | Implement ComputeCStructFieldLayoutandComputeExtendedFieldLayout | 
| src/coreclr/tools/Common/TypeSystem/Common/InstantiatedType.Metadata.cs | Override extended layout members | 
| src/coreclr/tools/Common/TypeSystem/Canon/CanonTypes.Metadata.cs | Stub out extended layout members | 
| src/coreclr/nativeaot/Runtime.Base/src/System/Runtime/InteropServices/LayoutKind.cs | Add LayoutKind.Extendedenum | 
| src/coreclr/md/compiler/custattr_emit.cpp | Handle tdExtendedLayoutin custom attr emitter | 
| src/coreclr/inc/il_kywd.h | Add IL keyword extended | 
| src/coreclr/inc/corhdr.h | Define tdExtendedLayoutandIsTdExtendedLayout | 
| src/coreclr/ilasm/prebuilt/asmparse.grammar | Recognize extendedin grammar | 
| src/coreclr/ilasm/asmparse.y | Add EXTENDED_token and grammar rules | 
| src/coreclr/System.Private.CoreLib/src/System/Reflection/RuntimeCustomAttributeData.cs | Map ExtendedLayouttoLayoutKind.Extended | 
| src/coreclr/System.Private.CoreLib/src/System/Reflection/Emit/RuntimeTypeBuilder.cs | Remove obsolete layout-mask validation | 
Comments suppressed due to low confidence (1)
src/coreclr/tools/Common/TypeSystem/Common/MetadataFieldLayoutAlgorithm.cs:502
- There are no unit tests covering the new CStruct layout path (ComputeCStructFieldLayout and ComputeExtendedFieldLayout). Adding tests for valid field offsets, alignment boundaries, and error conditions will help catch regressions.
private static ComputedInstanceFieldLayout ComputeCStructFieldLayout(MetadataType type, int numInstanceFields)
        
          
                src/coreclr/tools/Common/TypeSystem/Interop/IL/PInvokeDelegateWrapper.cs
          
            Show resolved
            Hide resolved
        
              
          
                src/libraries/System.Private.CoreLib/src/System/Reflection/TypeAttributes.cs
          
            Show resolved
            Hide resolved
        
      | I'll update the Ecma augments doc. It looks like we don't actually need to do any updates on Cecil to ensure correct behavior. Cecil reads layout info directly from the attributes bits, handles masked values right, and doesn't do any validation of valid or invalid values. Cecil could use nice APIs to read if a type is ExtendedLayout, but it isn't required. Based on my tests, ILSpy will need to react to accurately represent the bits in their decompilation, but that's expected. | 
| This looks good to me otherwise. Is your Roslyn PR on track for .NET 10? We should make sure that either both this PR and Roslyn PR are in .NET 10, or neither. | 
| I'm waiting on @jaredpar to let me know if Roslyn will have the resources to review the PR over there for this release. | 
| /azp run runtime | 
| Azure Pipelines successfully started running 1 pipeline(s). | 
| @jkotas now that we're in the .NET 11 time frame, can we get this merged in? Or do you want to wait until the Roslyn PR goes in before we get this in? | 
| Fine with me. Is there an ETA for the Roslyn PR merge? | 
| Still need to work with @jaredpar on exact timing. Their main branch opens for .NET 11 in Sep/Oct timeframe. I'd like to get it in by Nov but it might slip until after the holidays due to vacations/leave/etc. | 
| /ba-g websockets failure and linker failures unrelated | 
Add support for the ExtendedLayout flag, the
System.Runtime.InteropServices.ExtendedLayoutAttributeand theCStructlayout from #100896.Contributes to #100896
Leaving other layouts from #100896 for later PRs to minimize diffs.