Skip to content

Conversation

@davidwrighton
Copy link
Member

@davidwrighton davidwrighton commented Jan 6, 2022

In support of CreateSpan (#60948), improve alignment for RVA static pre-initialized fields to align memory blocks which may contain long, ulong, or double primitive arrays on 8 byte boundaries.

Mono fix is more involved

  • Fix Ref-Emit generated RVA statics
  • Set the HasFieldRVA bit. NOTE: earlier code that attempts to set the bit doesn't actually do that as the FieldRVA bit is in FieldAttributes.ReservedMask which is masked away in the FieldBuilder constructor
  • Fix the Swizzle lookup. We should not be using the 8 byte swizzle in the final else clause.
  • Enhance ref-emitted field to allow for use with CreateSpan
    • Ref-emitted fields should specify a pack size appropriate for minimum alignment of the CreateSpan targetted data
    • Respect the packing_size specified so that RVA static fields generated for CreateSpan can be properly aligned

Fixes #62314

@ghost
Copy link

ghost commented Jan 6, 2022

Tagging subscribers to this area: @dotnet/area-system-reflection-emit
See info in area-owners.md if you want to be subscribed.

Issue Details

In support of CreateSpan (#60948), improve alignment for RVA static pre-initialized fields to align memory blocks which may contain long, ulong, or double primitive arrays on 8 byte boundaries.

In Mono, the system will automatically align all RVA static data emitted via Reflection.Emit on 16 byte boundaries, so there is no fix needed.

Fixes #62314

Author: davidwrighton
Assignees: -
Labels:

area-System.Reflection.Emit

Milestone: -

@AaronRobinsonMSFT AaronRobinsonMSFT self-requested a review January 6, 2022 15:59
- Enhance test case to verify that the RVA static actually has the pre-initialized data
- Fix Ref-Emit generated RVA statics
 - Set the HasFieldRVA bit. NOTE: earlier code that attempts to set the bit doesn't actually do that as the FieldRVA bit is in  FieldAttributes.ReservedMask which is masked away in the FieldBuilder constructor
 - Fix the Swizzle lookup. We should not be using the 8 byte swizzle in the final else clause.
- Enhance ref-emitted field to allow for use with CreateSpan
  - Ref-emitted fields should specify a pack size appropriate for minimum alignment of the CreateSpan targetted data
  - Respect the packing_size specified so that RVA static fields generated for CreateSpan can be properly aligned
@davidwrighton davidwrighton dismissed AaronRobinsonMSFT’s stale review January 14, 2022 00:22

I like the approval, but now that I found the more significant issues to fix on Mono, I don't want a signoff without Mono involvement

@davidwrighton
Copy link
Member Author

@lambdageek Could you take a look. This fix has become substantially more complex in Mono.

@lambdageek
Copy link
Member

@lambdageek Could you take a look. This fix has become substantially more complex in Mono.

I'm off on sick time for a bit longer - I'll try to get to it. @vargaz could you take a look too

@vargaz
Copy link
Contributor

vargaz commented Jan 16, 2022

The mono changes look ok to me.

@marek-safar
Copy link
Contributor

This fails on AOT platforms. @vargaz could you have a look at the failure?

@ghost ghost locked as resolved and limited conversation to collaborators Feb 27, 2022
@davidwrighton davidwrighton deleted the align_rva_fields_in_reflection_emit branch April 13, 2023 18:50
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Reflection.Emit does not reliably allow use of CreateSpan<T> with non 8 byte aligned data

5 participants